Skip to content
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

Don't read scripts without extensions as modules in namespace mode #14335

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Don't read scripts without extensions as modules in namespace mode #14335

merged 3 commits into from
Jan 10, 2023

Conversation

getim
Copy link
Contributor

@getim getim commented Dec 21, 2022

The FindModuleCache currently matches files without an extension when --namespace_packages is enabled while the docs don't mention that this should be the case. The "near-miss" logic collects candidates for modules, which could correctly include a directory foo/bar/baz when looking for foo/bar/baz. However, the current logic also picks up a file foo/bar/baz. This means that, if both a file foo/bar/baz and foo/bar/baz.py exist, the first one is actually picked, resulting in unexpected behaviour.

The condition that checks exists_case on foo/bar/baz should also check that it is indeed a directory by checking that it is not a file. I'm open to different fixes of course, but this seemed like the most obvious and least impactful change to make.

This PR modifies 2 tests:

  • add test-data/packages/modulefinder/pkg1/a to verify that ModuleFinderSuite.test__no_namespace_packages__find_a_in_pkg1 is indeed working correctly even without the patch because it's not running in namespace mode.
  • add test-data/packages/modulefinder/nsx-pkg3/nsx/c/c, making ModuleFinderSuite.test__find_nsx_c_c_in_pkg3 fail, which the patch fixes.

To give one real-world example of this scenario: Bazel's Python rules construct a wrapper-script with the same name as the main Python-file without the extension for a py_binary-target. If some other Python rule depends on this //foo/bar:baz py_binary-target, it sees both foo/bar/baz and foo/bar/baz.py in the same directory, incorrectly picking up the wrapper-script instead of the module. Dependencies on a py_binary might be a bit of an edge-case, but Python execution of these targets does pick up the right file, so Mypy should probably as well.

@getim getim changed the title don't read scripts without extensions as modules in namespace mode Don't read scripts without extensions as modules in namespace mode Dec 21, 2022
@github-actions

This comment has been minimized.

mypy/modulefinder.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good!

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja hauntsaninja merged commit e965275 into python:master Jan 10, 2023
@getim getim deleted the tim/dont-find-scripts-as-modules branch January 11, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants