Skip to content

Python: Add example of top-level module shadowing stdlib (v2) #4693

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

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Nov 19, 2020

This PR adds a test-case to show a specific kind of bad module resolution that happens when using codeql database create, since a locally defined module doesn't take precedence in module resolution. It turned out that we don't invoke the extractor in the same way when running our qltests, and that the way qltests behaves is more correct (compared with Python interpreter). Obvious fixes didn't work:

  • Make extractor better: Hard, since you need to figure out exactly at what paths the Python interpreter is going to be invoked, so you can simulate adding that path as sys.path[0].
  • Change qltest extractor to match codeql database create: I naively attempted this, but it turned out to be a much bigger tasks than I am ready to take on right now.

So instead I changed the test manually to simulate what the result of codeql database create is (and even that was tricky). Reboot of #4655, but a bit more clear now that I've figured out what to do.


Kind of a bonus test hiding within all of this is the unique_name, which just shows that we're not able to resolve it properly when it resides in non-package directory... but it's not the essential part I'm trying to tests here, I think I just included it as a safety check.

Although this test is added under the `wrong` folder, the current results from
this CodeQL test is actually correct (compared with the Python
interpreter). However, they don't match what the extractor does when invoked
with `codeql database create`.

Since I deemed it "more than an easy fix" to change the extractor behavior for
`codeql database create` to match the real python behavior, and it turned out to
be quite a challenge to change the extractor behavior for all tests, I'm just
going to make THIS ONE test-case behave like the extractor will with `codeql
database create`...

This is a first commit, to show how the extractor works with qltest by default.

Inspired by the debugging in github#4640
@RasmusWL RasmusWL force-pushed the python-add-import-test-shadowing-stdlib-v2 branch from dd47465 to 7e407d4 Compare November 19, 2020 13:56
@RasmusWL
Copy link
Member Author

I accidentally included a change to a file that wasn't supposed to be part of this branch, and force pushed to remove this...

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Nice and clear example of the bad behaviour. 👍

I have made two suggestion with regard to typos, otherwise this looks good to me.

Co-authored-by: Taus <tausbn@github.com>
@codeql-ci codeql-ci merged commit d3cded3 into github:main Nov 27, 2020
@RasmusWL RasmusWL deleted the python-add-import-test-shadowing-stdlib-v2 branch November 27, 2020 10:32
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.

3 participants