Skip to content

Comments

[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
AlexWaygood wants to merge 1 commit intomainfrom
alex/global-statements
Closed

[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
AlexWaygood wants to merge 1 commit intomainfrom
alex/global-statements

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 24, 2025

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 astral-sh/ty#220.

Test Plan

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

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Mar 24, 2025
@AlexWaygood AlexWaygood mentioned this pull request Mar 24, 2025
Closed
@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
@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅

MichaReiser
MichaReiser reviewed Mar 24, 2025
View reviewed changes
crates/red_knot_python_semantic/src/semantic_index/re_exports.rs
if name.starts_with('_') {
return;
}
if self.scope_kind.is_not_global() && !self.global_declarations_in_scope.contains(name) {
Copy link
Member

@MichaReiser MichaReiser Mar 24, 2025

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

@AlexWaygood AlexWaygood Mar 24, 2025

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
@AlexWaygood
[red-knot] Recognize an assignment in a function/class scopes as a gl…
ed5678c 
…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
carljm
carljm reviewed Mar 28, 2025
View reviewed changes
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

AlexWaygood commented Mar 28, 2025

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

Reviewers

@carljm carljm carljm left review comments

@MichaReiser MichaReiser MichaReiser left review comments

@dcreager dcreager Awaiting requested review from dcreager

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AlexWaygood @carljm @MichaReiser