Skip to content

[red-knot] Add some knowledge of __all__ to *-import machinery #17373

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 1 commit into from
Apr 15, 2025

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 13, 2025

Summary

This PR adds some logic to the exported_names query in re_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:

  • If so, it returns all names defined in the module's global scope, even underscore-prefixed names and imported names in stubs that are not explicitly re-exported.
  • If not, the query has the same behaviour that it does on 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__. On main, we do not even create a Definition for _Y in importer.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:

_Y = True

__all__ = ["_Y"]

importer.py:

from exporter import *

reveal_type(_Y)  # false-positive error from us here on `main`

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:

_Y = True

__all__ = []

importer.py:

from exporter import *

reveal_type(_Y)  # We'll no longer catch the `NameError` here with this PR

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 resolve sys.version_info or sys.platform tests as always truthy or always falsy; this cannot be done from re_exports.rs. Instead, I think the value of __all__ can only be finally resolved at type-inference time rather than in re_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__:

PredicateNode::StarImportPlaceholder(star_import) => {
let symbol_table = symbol_table(db, star_import.scope(db));
let symbol_name = symbol_table.symbol(star_import.symbol_id(db)).name();
match imported_symbol(db, star_import.referenced_file(db), symbol_name).symbol {
crate::symbol::Symbol::Type(_, crate::symbol::Boundness::Bound) => {
Truthiness::AlwaysTrue
}
crate::symbol::Symbol::Type(_, crate::symbol::Boundness::PossiblyUnbound) => {
Truthiness::Ambiguous
}
crate::symbol::Symbol::Unbound => Truthiness::AlwaysFalse,
}
}

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 the sys.version_info test resolves to true or false in re_exports.rs, the "upper bound" of __all__ is {"X", "Y", "Z"}. Therefore there would be no need to create a Definition for Foo if it saw a * import referencing this module; it would be able to only create Definitions for X, Y and Z:

import sys

X = 1
Y = 2
Z = 3
Foo = 4

__all__ = ["X"]

if sys.version_info >= (3, 10):
    __all__.append("Y")
else:
    __all__ += ["Z"]

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:

import sys

__all__ = []

if sys.version_info >= (3, 10):
    from . import bar

    __all__.extend(bar.__all__)

While it might be possible to look across the module boundary in re_exports.rs and retrieve the value of bar.__all__, I'm not at all confident that we would be able to correctly resolve the type of the bar 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 to re_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-all

Closes #14169.

Test Plan

Updated some assertions in mdtests, and added some new ones

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Apr 13, 2025
Copy link
Contributor

mypy_primer results

No ecosystem changes detected ✅

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great!

@AlexWaygood AlexWaygood merged commit 312a487 into main Apr 15, 2025
23 checks passed
@AlexWaygood AlexWaygood deleted the alex/star-imports-dunder-all-2 branch April 15, 2025 11:56
dcreager added a commit that referenced this pull request Apr 15, 2025
* main: (31 commits)
  [red-knot] Add some knowledge of `__all__` to `*`-import machinery (#17373)
  Update taiki-e/install-action digest to be7c31b (#17379)
  Update Rust crate mimalloc to v0.1.46 (#17382)
  Update PyO3/maturin-action action to v1.49.1 (#17384)
  Update Rust crate anyhow to v1.0.98 (#17380)
  dependencies: switch from `chrono` to `jiff`
  Update Rust crate bstr to v1.12.0 (#17385)
  [red-knot] Further optimize `*`-import visibility constraints (#17375)
  [red-knot] Minor 'member_lookup_with_policy' fix (#17407)
  [red-knot] Initial support for `dataclass`es (#17353)
  Sync vendored typeshed stubs (#17402)
  [red-knot] improve function/bound method type display (#17294)
  [red-knot] Move relation methods from `CallableType` to `Signature` (#17365)
  [syntax-errors] `await` outside async functions (#17363)
  [red-knot] optimize is_subtype_of for literals (#17394)
  [red-knot] add a large-union-of-string-literals benchmark (#17393)
  Update pre-commit dependencies (#17383)
  [red-knot] mypy_primer: Fail job on panic or internal errors (#17389)
  [red-knot] Document limitations of diagnostics-silencing in unreachable code (#17387)
  [red-knot] detect unreachable attribute assignments (#16852)
  ...
dcreager added a commit that referenced this pull request Apr 16, 2025
* main: (44 commits)
  [`airflow`] Extend `AIR311` rules (#17422)
  [red-knot] simplify union size limit handling (#17429)
  [`airflow`] Extract `AIR311` from `AIR301` rules (`AIR301`, `AIR311`) (#17310)
  [red-knot] set a size limit on unions of literals (#17419)
  [red-knot] make large-union benchmark slow again (#17418)
  [red-knot] optimize building large unions of literals (#17403)
  [red-knot] Fix comments in type_api.md (#17425)
  [red-knot] Do not assume that `x != 0` if `x` inhabits `~Literal[0]` (#17370)
  [red-knot] make large-union benchmark more challenging (#17416)
  [red-knot] Acknowledge that `T & anything` is assignable to `T` (#17413)
  Update Rust crate clap to v4.5.36 (#17381)
  Raise syntax error when `\` is at end of file (#17409)
  [red-knot] Add regression tests for narrowing constraints cycles (#17408)
  [red-knot] Add some knowledge of `__all__` to `*`-import machinery (#17373)
  Update taiki-e/install-action digest to be7c31b (#17379)
  Update Rust crate mimalloc to v0.1.46 (#17382)
  Update PyO3/maturin-action action to v1.49.1 (#17384)
  Update Rust crate anyhow to v1.0.98 (#17380)
  dependencies: switch from `chrono` to `jiff`
  Update Rust crate bstr to v1.12.0 (#17385)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[red-knot] support * imports
2 participants