Skip to content

Conversation

tausbn
Copy link
Contributor

@tausbn tausbn commented Mar 11, 2021

Adds support for accessing built-ins via the API graph. These are placed as members of the builtins module, mimicking how this works in Python 3.

In Python 2, this module is called __builtin__ and we silently alias this to builtins. In principle this may cause surprise if people try to write, say, API::moduleImport("__builtin__").getMember("open"), but ideally they should be using API::builtin("open") for this anyway (and with Python 2 being officially deprecated, I see little reason to support this very narrow use-case).

At present, this implementation does not try to account for redefinitions of built-ins (as witnessed by the test with the SPURIOUS annotation).

Also, there is some weirdness in one of the tests (the one that has the MISSING annotation), but this appears to be unrelated to the present implementation, and more likely due to missing flow into locally-defined functions.

@tausbn tausbn requested a review from a team as a code owner March 11, 2021 22:03
@tausbn tausbn changed the title Python: Support builtins in API graphs Python: Support built-ins in API graphs Mar 11, 2021
yoff
yoff previously approved these changes Mar 12, 2021
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One possible rewrite suggested. Also, should we gate the aliasing of __builtin__ on the Python version?

@tausbn
Copy link
Contributor Author

tausbn commented Mar 12, 2021

[...] should we gate the aliasing of __builtin__ on the Python version?

*sigh* I guess we really must, since you could have a module called __builtin__.py in Python 3. 🤦‍♂️

Also moves the filtering on `name` to before the big disjunction in
`MkModuleImport`.
@tausbn
Copy link
Contributor Author

tausbn commented Mar 12, 2021

[...] (and with Python being officially deprecated, [...]

Just noticed this... I of course mean Python 2. 🤣
(I edited it to avoid confusion.)

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yoff yoff merged commit a760ed8 into github:main Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants