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

[red-knot] Recognize an assignment in a function/class scopes as a global-scope definition for purposes of * imports if the assigned symbol is declared global #16959

Closed
wants to merge 1 commit into from

Conversation

AlexWaygood
Copy link
Member

Summary

Further work towards #14169. If something like this exists in a foo.py module:

def f():
    global a
    a: bool = True

and a bar.py module has this:

from foo import *

reveal_type(a)

then we will now recognize a as bound in bar.py. We don't yet reveal bool as the inferred type, but that depends on fixing #15385.

Test Plan

Assertions in existing mdtests are modified, and some new assertions are added

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Mar 24, 2025
@AlexWaygood AlexWaygood changed the title [red-knot] Recognize an assignment in a function/class scopes as a global-scope definition if the assigned symbol is declared global [red-knot] Recognize an assignment in a function/class scopes as a global-scope definition for purposes of * imports if the assigned symbol is declared global Mar 24, 2025
Copy link
Contributor

github-actions bot commented Mar 24, 2025

mypy_primer results

No ecosystem changes detected ✅

}
}

fn possibly_add_export(&mut self, name: &Name) {
if name.starts_with('_') {
return;
}
if self.scope_kind.is_not_global() && !self.global_declarations_in_scope.contains(name) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how hot this code path is but we could consider using hashbrown's hash table to only compute the hash key once (for global_declarations_in_scope and exports). But that's probably a premature optimization :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this for now (following our in-person conversation) but happy to look into this later if it shows up in profiling etc.!

@AlexWaygood AlexWaygood force-pushed the alex/global-statements branch from 498eb5e to 42ecd9a Compare March 24, 2025 21:32
…obal-scope definition if the assigned symbol is declared `global`
@AlexWaygood AlexWaygood force-pushed the alex/global-statements branch from 42ecd9a to ed5678c Compare March 25, 2025 21:53
@sharkdp sharkdp removed their request for review March 28, 2025 12:48
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.

Sorry for not getting to clarity on this sooner, but I feel like we shouldn't support this. While this PR doesn't add a lot of complexity, fully supporting this pattern will, because it means that the types of symbols in an outer scope can depend on an arbitrary nested scope.

I think it is reasonable to require that if you use global in a function and assign to that name, the name must explicitly exist in the global scope.

If we do end up deciding we have to support this pattern, I want to add full support for it altogether (that is, not do star-import support for it separately from actual type inference support) so we can see the full picture of what it requires.

@AlexWaygood
Copy link
Member Author

yes, after discussing this with you the other day, I agree that we probably shouldn't do this right now. Let's add support for global statements generally first, and see where we land on that. We can possibly revive this after we do that, but it doesn't seem like a priority in any event.

@AlexWaygood AlexWaygood deleted the alex/global-statements branch March 28, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants