Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#5779 enhancement closed fixed (fixed)

Conch relies on a CPython implementation detail

Reported by: Alex Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/conch-should-not-use-builtins-5779
(github, patch, buildbot, log)
Author: cyli Launchpad Bug:


It uses __builtins__ and assumes that it will be a dict. According to this is a) an implementation detail of CPython, and b) not guaranteed to be a dict! it will sometimes be a module.

Attachments (1)

t5779-builtins.diff (575 bytes) - added by Alex 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc z3p added

Changed 3 years ago by Alex

comment:2 Changed 3 years ago by Alex

  • Keywords review added

comment:3 Changed 3 years ago by cyli

  • Author set to cyli
  • Branch set to branches/conch-should-not-use-builtins-5779

(In [34803]) Branching to 'conch-should-not-use-builtins-5779'

comment:4 Changed 3 years ago by spiv

I suppose strictly speaking this is correct, but:

  • is gmpy even available for non-CPython interpreters, and
  • if it is available, are we sure it's faster?

e.g. I'm wondering if for pypy we'd be better off not doing this hack. I guess optimising exponentiation isn't pypy's focus, but who knows…

Anyway, this patch does seem to be slightly more correct in theory, so we may as well use it :)

comment:5 Changed 3 years ago by cyli

(In [34804]) Apply Alex's t5779-builtins.diff patch refs #5779

comment:6 Changed 3 years ago by Alex

spiv: No, it's not, and in fact from reading the source I seriously doubt it will ever compile against either CPyExt (PyPy's compatibility layer) or IronClad (IronPython). This was submitted purely as a correctness issue, not a compatibility one.

comment:7 Changed 3 years ago by cyli

(In [34807]) Add a newsfile refs #5779

comment:8 Changed 3 years ago by cyli

Thanks for finding and fixing this Alex! In the future, could you also include a news file in your patch?

buildbot tests pass. Merging this.

comment:9 Changed 3 years ago by cyli

  • Resolution set to fixed
  • Status changed from new to closed

(In [34811]) Merge conch-should-not-use-builtins-5779: conch should not depend on builtlins being a dict

Author: Alex Reviewer: cyli, spiv Fixes: #5779

Conch uses builtins and assumes that it will be a dict, which is an implementation of CPython and not guaranteed to dict instead of a module

comment:10 Changed 3 years ago by cyli

  • Keywords review removed

Oops, forgot to remove review keyword

Note: See TracTickets for help on using tickets.