Skip to content

[ty] Implement global handling and load-before-global-declaration syntax error #17637

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.

Sign up for GitHub

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
ntBre merged 23 commits into from
May 8, 2025
Merged

[ty] Implement global handling and load-before-global-declaration syntax error #17637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6cd7c58
[red-knot] Copy and paste ruff's `global` handling
ntBre Apr 25, 2025
f359e77
refactor globals handling, resolve invalid-assignments, break errors
ntBre May 2, 2025
87b177e
fix unresolved-reference todo
ntBre May 2, 2025
2f211bf
resolve the last typing todo
ntBre May 2, 2025
bef6172
emit syntax errors on `global` lines
ntBre May 2, 2025
96443aa
tidy up
ntBre May 2, 2025
11f4805
use Alex's globals_per_scope idea
ntBre May 2, 2025
6e6422d
use a HashSet instead of Map, range is unused
ntBre May 2, 2025
f35849e
apply suggestion
ntBre May 5, 2025
fc0e337
try adding an mdtest to reproduce the issue
ntBre May 5, 2025
361fa8c
try Alex's suggestion here too
ntBre May 5, 2025
c4ea922
Merge branch 'main' into brent/simple-red-knot-globals
ntBre May 5, 2025
4cdb754
Merge branch 'main' into brent/simple-red-knot-globals
ntBre May 6, 2025
5f9867e
clean up outdated comment
ntBre May 6, 2025
16b19f1
use a type alias instead of very thin wrapper struct
ntBre May 6, 2025
610f050
use FileScopeId::is_global
ntBre May 6, 2025
5cda462
store ScopedSymbolId instead of Name
ntBre May 6, 2025
e3193bf
fix docs reference to Globals
ntBre May 6, 2025
b5dc8c4
Separate mdtest cases, add more descriptive names
ntBre May 8, 2025
899ffb3
print -> reveal_type in final case
ntBre May 8, 2025
d0f9520
apply docs suggestion
ntBre May 8, 2025
9aef28a
factor out TypeInferenceBuilder::skip_non_global_scopes
ntBre May 8, 2025
6d18f68
add TODO for annotated global
ntBre May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions crates/ty_python_semantic/resources/mdtest/diagnostics/semantic_syntax_errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,12 +252,8 @@ def _():

## Load before `global` declaration

This should be an error, but it's not yet.

TODO implement `SemanticSyntaxContext::global`

```py
def f():
x = 1
global x
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
```
106 changes: 71 additions & 35 deletions crates/ty_python_semantic/resources/mdtest/scopes/global.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,14 @@ def f():
y = ""

global x
# TODO: error: [invalid-assignment] "Object of type `Literal[""]` is not assignable to `int`"
# error: [invalid-assignment] "Object of type `Literal[""]` is not assignable to `int`"
x = ""

global z
# error: [invalid-assignment] "Object of type `Literal[""]` is not assignable to `int`"
z = ""

z: int
Copy link
Contributor

@carljm carljm May 8, 2025

Choose a reason for hiding this comment

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

I think there's a set of cases that we aren't testing, which is when we mark a name global in a nested scope, and then declare that name with a new type in that scope, e.g.

x: int = 1

def f():
    global x
    x: str = "foo"

We should definitely error on that assignment, since it's an assignment to the global with the wrong type. I think we should probably error any time a global name is declared with a type in the local scope, even if the type is the same -- that kind of local re-declaration of a global gives the wrong impression and doesn't make sense.

It's OK if this is a TODO in this PR, but we should at least add a TODO for it.

Copy link
Member

@AlexWaygood AlexWaygood May 8, 2025

Choose a reason for hiding this comment

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

that's actually a (currently unimplemented?) syntax error to provide a local-scope annotation for a name declared global. So I don't think we should emit an error about it in type inference — that would lead to unnecessary double diagnostics.

We should implement the syntax error (either here or in a followup PR) though! And we should add tests now.

>>> x: int = 1
>>> def f():
...     global x
...     x: str = "foo"
  File "<string>", line 3
SyntaxError: annotated name 'x' can't be global

Copy link
Contributor

@carljm carljm May 8, 2025

Choose a reason for hiding this comment

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

Oh cool, didn't realize that was a syntax error -- even if the declaration comes after the global statement. Agreed.

Copy link
Contributor Author

@ntBre ntBre May 8, 2025

Choose a reason for hiding this comment

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

I added this to the global mdtest as a TODO and added the syntax error to #17412!

```

## Nested intervening scope
Expand All @@ -48,8 +54,7 @@ def outer():

def inner():
global x
# TODO: revealed: int
reveal_type(x) # revealed: str
reveal_type(x) # revealed: int
```

## Narrowing
Expand Down Expand Up @@ -87,8 +92,7 @@ def f():
```py
def f():
global x
# TODO this should also not be an error
y = x # error: [unresolved-reference] "Name `x` used when not defined"
y = x
x = 1 # No error.

x = 2
Expand All @@ -99,79 +103,111 @@ x = 2
Using a name prior to its `global` declaration in the same scope is a syntax error.

```py
x = 1

def f():
print(x) # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
print(x)
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
print(x)

def f():
global x
print(x) # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
print(x)
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
print(x)

def f():
print(x) # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x, y
print(x)
global x, y # error: [invalid-syntax] "name `x` is used prior to global declaration"
print(x)

def f():
global x, y
print(x) # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x, y
print(x)
global x, y # error: [invalid-syntax] "name `x` is used prior to global declaration"
print(x)

def f():
x = 1 # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
x = 1
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
x = 1

def f():
global x
x = 1 # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
x = 1
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
x = 1

def f():
del x # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x, y
del x
global x, y # error: [invalid-syntax] "name `x` is used prior to global declaration"
del x

def f():
global x, y
del x # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x, y
del x
global x, y # error: [invalid-syntax] "name `x` is used prior to global declaration"
del x

def f():
del x # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
del x
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
del x

def f():
global x
del x # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
del x
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
del x

def f():
del x # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x, y
del x
global x, y # error: [invalid-syntax] "name `x` is used prior to global declaration"
del x

def f():
global x, y
del x # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x, y
del x
global x, y # error: [invalid-syntax] "name `x` is used prior to global declaration"
del x

def f():
print(f"{x=}") # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
print(f"{x=}")
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"

# still an error in module scope
x = None # TODO: error: [invalid-syntax] name `x` is used prior to global declaration
global x
x = None
global x # error: [invalid-syntax] "name `x` is used prior to global declaration"
```

## Local bindings override preceding `global` bindings

```py
x = 42

def f():
global x
reveal_type(x) # revealed: Unknown | Literal[42]
x = "56"
reveal_type(x) # revealed: Literal["56"]
```

## Local assignment prevents falling back to the outer scope

```py
Copy link
Contributor

@carljm carljm May 7, 2025

Choose a reason for hiding this comment

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

Probably best to give this a separate heading. Two unnamed code blocks under a single heading are merged into a single file for checking. Seems to be ok here at the moment, but this can lead to future confusion when we're dealing with global scope. E.g. at the moment the first test is only showing Literal[42] because the second test also assigns x = 42 -- if the second test assigned something different to x, the first test would see that instead.

Copy link
Contributor Author

@ntBre ntBre May 8, 2025

Choose a reason for hiding this comment

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

Oh I see, thanks! This was a good excuse to come up with better headings anyway.

x = 42

def f():
# error: [unresolved-reference] "Name `x` used when not defined"
reveal_type(x) # revealed: Unknown
x = "56"
reveal_type(x) # revealed: Literal["56"]
```

## Annotating a `global` binding is a syntax error

```py
x: int = 1

def f():
global x
x: str = "foo" # TODO: error: [invalid-syntax] "annotated name 'x' can't be global"
```
13 changes: 13 additions & 0 deletions crates/ty_python_semantic/src/semantic_index.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ pub(crate) struct SemanticIndex<'db> {
/// Map from the file-local [`FileScopeId`] to the salsa-ingredient [`ScopeId`].
scope_ids_by_scope: IndexVec<FileScopeId, ScopeId<'db>>,

/// Map from the file-local [`FileScopeId`] to the set of explicit-global symbols it contains.
globals_by_scope: FxHashMap<FileScopeId, FxHashSet<ScopedSymbolId>>,

/// Use-def map for each scope in this file.
use_def_maps: IndexVec<FileScopeId, Arc<UseDefMap<'db>>>,

Expand Down Expand Up @@ -255,6 +258,16 @@ impl<'db> SemanticIndex<'db> {
self.scope_ids_by_scope.iter().copied()
}

pub(crate) fn symbol_is_global_in_scope(
&self,
symbol: ScopedSymbolId,
scope: FileScopeId,
) -> bool {
self.globals_by_scope
.get(&scope)
.is_some_and(|globals| globals.contains(&symbol))
}

/// Returns the id of the parent scope.
pub(crate) fn parent_scope_id(&self, scope_id: FileScopeId) -> Option<FileScopeId> {
let scope = self.scope(scope_id);
Expand Down
42 changes: 39 additions & 3 deletions crates/ty_python_semantic/src/semantic_index/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use ruff_python_ast::name::Name;
use ruff_python_ast::visitor::{walk_expr, walk_pattern, walk_stmt, Visitor};
use ruff_python_ast::{self as ast, PySourceType, PythonVersion};
use ruff_python_parser::semantic_errors::{
SemanticSyntaxChecker, SemanticSyntaxContext, SemanticSyntaxError,
SemanticSyntaxChecker, SemanticSyntaxContext, SemanticSyntaxError, SemanticSyntaxErrorKind,
};
use ruff_text_size::TextRange;

Expand Down Expand Up @@ -106,6 +106,7 @@ pub(super) struct SemanticIndexBuilder<'db> {
use_def_maps: IndexVec<FileScopeId, UseDefMapBuilder<'db>>,
scopes_by_node: FxHashMap<NodeWithScopeKey, FileScopeId>,
scopes_by_expression: FxHashMap<ExpressionNodeKey, FileScopeId>,
globals_by_scope: FxHashMap<FileScopeId, FxHashSet<ScopedSymbolId>>,
definitions_by_node: FxHashMap<DefinitionNodeKey, Definitions<'db>>,
expressions_by_node: FxHashMap<ExpressionNodeKey, Expression<'db>>,
imported_modules: FxHashSet<ModuleName>,
Expand Down Expand Up @@ -144,6 +145,7 @@ impl<'db> SemanticIndexBuilder<'db> {
scopes_by_node: FxHashMap::default(),
definitions_by_node: FxHashMap::default(),
expressions_by_node: FxHashMap::default(),
globals_by_scope: FxHashMap::default(),

imported_modules: FxHashSet::default(),
generator_functions: FxHashSet::default(),
Expand Down Expand Up @@ -1085,6 +1087,7 @@ impl<'db> SemanticIndexBuilder<'db> {
self.scopes_by_node.shrink_to_fit();
self.generator_functions.shrink_to_fit();
self.eager_snapshots.shrink_to_fit();
self.globals_by_scope.shrink_to_fit();

SemanticIndex {
symbol_tables,
Expand All @@ -1093,6 +1096,7 @@ impl<'db> SemanticIndexBuilder<'db> {
definitions_by_node: self.definitions_by_node,
expressions_by_node: self.expressions_by_node,
scope_ids_by_scope: self.scope_ids_by_scope,
globals_by_scope: self.globals_by_scope,
ast_ids,
scopes_by_expression: self.scopes_by_expression,
scopes_by_node: self.scopes_by_node,
Expand Down Expand Up @@ -1898,7 +1902,38 @@ where
// Everything in the current block after a terminal statement is unreachable.
self.mark_unreachable();
}

ast::Stmt::Global(ast::StmtGlobal { range: _, names }) => {
for name in names {
let symbol_id = self.add_symbol(name.id.clone());
let symbol_table = self.current_symbol_table();
let symbol = symbol_table.symbol(symbol_id);
if symbol.is_bound() || symbol.is_declared() || symbol.is_used() {
self.report_semantic_error(SemanticSyntaxError {
kind: SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration {
name: name.to_string(),
start: name.range.start(),
},
range: name.range,
python_version: self.python_version,
});
}
let scope_id = self.current_scope();
self.globals_by_scope
.entry(scope_id)
.or_default()
.insert(symbol_id);
}
walk_stmt(self, stmt);
}
ast::Stmt::Delete(ast::StmtDelete { targets, range: _ }) => {
for target in targets {
if let ast::Expr::Name(ast::ExprName { id, .. }) = target {
let symbol_id = self.add_symbol(id.clone());
self.current_symbol_table().mark_symbol_used(symbol_id);
}
}
walk_stmt(self, stmt);
}
_ => {
walk_stmt(self, stmt);
}
Expand Down Expand Up @@ -2387,7 +2422,8 @@ impl SemanticSyntaxContext for SemanticIndexBuilder<'_> {
self.source_text().as_str()
}

// TODO(brent) handle looking up `global` bindings
// We handle the one syntax error that relies on this method (`LoadBeforeGlobalDeclaration`)
// directly in `visit_stmt`, so this just returns a placeholder value.
fn global(&self, _name: &str) -> Option<TextRange> {
None
}
Expand Down
Loading
Loading