[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_names
query inre_exports.rs
so 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 aDefinition
for_Y
inimporter.py
as a result of the*
import, leading to false positives later on when_Y
is 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_info
orsys.platform
tests 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.rs
or 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.rs
so 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_info
test resolves totrue
orfalse
inre_exports.rs
, the "upper bound" of__all__
is{"X", "Y", "Z"}
. Therefore there would be no need to create aDefinition
forFoo
if it saw a*
import referencing this module; it would be able to only createDefinition
s forX
,Y
andZ
: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.rs
and retrieve the value ofbar.__all__
, I'm not at all confident that we would be able to correctly resolve the type of thebar
symbol 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