Skip to content

[red-knot] support implicit global name lookups#12374

Merged
carljm merged 2 commits intomainfrom
cjm/scope-lookups
Jul 18, 2024
Merged

[red-knot] support implicit global name lookups#12374
carljm merged 2 commits intomainfrom
cjm/scope-lookups

Conversation

@carljm
Copy link
Copy Markdown
Contributor

@carljm carljm commented Jul 18, 2024

Support falling back to a global name lookup if a name isn't defined in the local scope, in the cases where that is correct according to Python semantics.

In class scopes, a name lookup checks the local namespace first, and if the name isn't found there, looks it up in globals.

In function scopes (and type parameter scopes, which are function-like), if a name has any definitions in the local scope, it is a local, and accessing it when none of those definitions have executed yet just results in an UnboundLocalError, it does not fall back to a global. If the name does not have any definitions in the local scope, then it is an implicit global.

Public symbol type lookups never include such a fall back. For example, if a name is not defined in a class scope, it is not available as a member on that class, even if a name lookup within the class scope would have fallen back to a global lookup.

This PR makes the @override lint rule work again.

Not yet included/supported in this PR:

  • Support for free variables / closures: a free symbol in a nested function-like scope referring to a symbol in an outer function-like scope.
  • Support for global and nonlocal statements, which force a symbol to be treated as global or nonlocal even if it has definitions in the local scope.
  • Module-global lookups should fall back to builtins if the name isn't found in the module scope.

I would like to expose nicer APIs for the various kinds of symbols (explicit global, implicit global, free, etc), but this will also wait for a later PR, when more kinds of symbols are supported.

@carljm carljm added the ty Multi-file analysis & type inference label Jul 18, 2024
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Jul 18, 2024

CodSpeed Performance Report

Merging #12374 will degrade performances by 10.44%

Comparing cjm/scope-lookups (b2b4bd7) with main (811f78d)

Summary

❌ 3 (👁 3) regressions
✅ 30 untouched benchmarks

Benchmarks breakdown

Benchmark main cjm/scope-lookups Change
👁 red_knot_check_file[without_parse] 234.7 µs 262.1 µs -10.44%
👁 red_knot_check_file[incremental] 89.2 µs 95.5 µs -6.58%
👁 red_knot_check_file[cold] 318.8 µs 345.4 µs -7.7%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 18, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@carljm
Copy link
Copy Markdown
Contributor Author

carljm commented Jul 18, 2024

As expected, this gives back the wins (and a bit more) that we saw previously when switching to per-definition inference and removing this lookup to other scopes. Not only are we now doing extra work on names that are global references (and actually also wrongly on names that are arguments, because we currently don't create definitions for arguments), but that means we actually now recognize the decorator and run the lint rule, so we're just doing a lot more work in the benchmark than before.

Comment thread crates/red_knot_python_semantic/src/semantic_index/symbol.rs
Comment thread crates/red_knot_python_semantic/src/types.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/infer.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types.rs Outdated
Comment thread crates/red_knot_python_semantic/src/types/infer.rs Outdated
@carljm carljm force-pushed the cjm/use-def-improvements branch from f341f70 to aa4222d Compare July 18, 2024 16:09
@carljm carljm force-pushed the cjm/scope-lookups branch from 611d062 to a5cedb7 Compare July 18, 2024 16:24
Base automatically changed from cjm/use-def-improvements to main July 18, 2024 16:25
@carljm carljm force-pushed the cjm/scope-lookups branch from a5cedb7 to b2b4bd7 Compare July 18, 2024 16:25
@carljm carljm merged commit 519eca9 into main Jul 18, 2024
@carljm carljm deleted the cjm/scope-lookups branch July 18, 2024 17:50
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.

3 participants