[red-knot] Add some knowledge of __all__ to *-import machinery
#17373
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds some logic to the
exported_namesquery inre_exports.rsso that it detects if__all__is defined in the file. The behaviour of the query now differs depending on whether or not__all__is defined:main: it only returns the global-scope names that are not underscore-prefixed, and it excludes imported names in stubs that are not explicitly re-exported.This PR is necessary because you can have situations like this, where an underscore-prefixed name is re-exported by virtue of it being included in
__all__. Onmain, we do not even create aDefinitionfor_Yinimporter.pyas a result of the*import, leading to false positives later on when_Yis used:Cases where we have false positives
exporter.py:importer.py:This PR is necessary but not sufficient for handling all issues relating to
__all__and*imports. The immediate effect of the PR will be that it gets rid of some false positives, but at the cost of introducing some false negatives. For example, we will now not emit an error on something like this, even though it fails at runtime:New false negatives introduced by this PR
exporter.py:importer.py:In order to fix this false negative, we'd need to actually understand which names are defined in
__all__at runtime. But that's pretty hard, because of the various ways in which__all__can be conditionally defined or conditionally mutated at runtime. Resolving the value of__all__requires us to resolvesys.version_infoorsys.platformtests as always truthy or always falsy; this cannot be done fromre_exports.rs. Instead, I think the value of__all__can only be finally resolved at type-inference time rather than inre_exports.rsor semantic indexing; therefore the proper solution to these new false negatives will be to adjust the logic here such that if the exporting module contains__all__, the visibility constraint applied to a certain*-import definition is resolved to always falsy if the symbol name is not present in the resolved value of__all__:ruff/crates/red_knot_python_semantic/src/semantic_index/visibility_constraints.rs
Lines 655 to 667 in 3aa3ee8
I considered adding logic to
re_exports.rsso that the visitor kept track of an "upper bound" for the values of__all__. E.g. for something like this, you could calculate that, even though we don't know whether thesys.version_infotest resolves totrueorfalseinre_exports.rs, the "upper bound" of__all__is{"X", "Y", "Z"}. Therefore there would be no need to create aDefinitionforFooif it saw a*import referencing this module; it would be able to only createDefinitions forX,YandZ:But I realised that even calculating an "upper bound" is not possible to do with any accuracy in
re_exports.rs. This is because the typing spec mandates that type checkers must be able to understand an__all__mutation like this:While it might be possible to look across the module boundary in
re_exports.rsand retrieve the value ofbar.__all__, I'm not at all confident that we would be able to correctly resolve the type of thebarsymbol itself here from this query. The fact that there were serious questions about how accurate this calculation of "upper bounds" might be, the fact that it shouldn't be necessary to do it for correctness (it would just be an optimisation), and the fact the fact that attempting this calculation added significant complexity tore_exports.rs, all led me to abandon this idea. But if you're interested in it anyway, the other branch where I tried it out is here: main...alex/star-imports-dunder-allCloses #14169.
Test Plan
Updated some assertions in mdtests, and added some new ones