Skip to content

Python: Support import * in API graphs #6274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 10, 2021

Conversation

tausbn
Copy link
Contributor

@tausbn tausbn commented Jul 13, 2021

A slightly awkward implementation, mostly due to the fact that import * may appear in a local scope, and we don't want this to pollute unrelated scopes.

Also, our use of GlobalSsaVariables means we don't consider the first reference to a global (which may come from an import * in the global scope) to be a LocalSourceNode. This should go away once we have fixed the remaining issues surrounding EssaNodes.


Needs a performance test (as we're growing the API graph yet again), and possibly also a change note.

tausbn added 3 commits July 13, 2021 17:46
Moves the current test out of `test.py`, as otherwise any unknown global
(like, say, `sink`) would _also_ be considered to be something
potentially defined in `unknown`.
This will make it possible to reuse for names defined in `import *`.
@tausbn tausbn added the no-change-note-required This PR does not need a change note label Jul 14, 2021
@tausbn tausbn marked this pull request as ready for review July 14, 2021 14:22
@tausbn tausbn requested a review from a team as a code owner July 14, 2021 14:22
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

nice 💪 besides a few minor things 😉

I'm also thinking that maybe we should only merge this after the final 3.2 branch has been cut, just to be on the safe side 😊

tausbn and others added 2 commits July 20, 2021 15:19
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
I debated whether to add a
`MISSING: use=moduleImport("builtins").getMember("print").getReturn()`
annotation to the last line.

Ultimately, I decided to add it, as we likely _do_ want this information
to propagate into inner functions (even if the value of `var2` may
change before `func4` is called).
@tausbn tausbn requested a review from RasmusWL July 20, 2021 13:45
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Besides having reviewed performance, this looks good to me 👍

tausbn and others added 2 commits September 7, 2021 14:13
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Internal performance test looks fine, so no cause for concerns there ✔️


There was a brief moment when I thought that the following annotation meant that we think that bar is print 😆 (and I was super confused)

print(bar2) #$ use=moduleImport("builtins").getMember("print").getReturn()

@RasmusWL RasmusWL merged commit db78e3a into github:main Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants