[red-knot] fix eager nested scopes handling #16916
Conversation
mtshiba
requested review from
carljm,
AlexWaygood,
sharkdp and
dcreager
as code owners
|
crates/red_knot_python_semantic/resources/mdtest/annotations/deferred.md
Outdated
Show resolved
Hide resolved
| // If the current scope is an eager scope within the class definition, | ||
| // the class name is invisible (unless it is deferred) because it has not yet been registered. | ||
| if !self.is_deferred() && scope.is_eager(db) { | ||
| if let Some(class_ty) = symbol | ||
| .symbol | ||
| .ignore_possibly_unbound() | ||
| .and_then(Type::into_class_literal) | ||
| { | ||
| for (ancestor_scope, _) in self.index.ancestor_scopes(file_scope_id) { | ||
| let ancestor_scope = ancestor_scope.to_scope_id(db, self.file()); | ||
| if ancestor_scope == class_ty.body_scope(db) { | ||
| symbol = Symbol::Unbound.into(); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is implemented as a special-case patch fix for one very specific case (references to classes inside the class definition), when what we need is a more general fix to the semantics implemented for eager nested scopes. Here is one example of a case that we currently handle wrongly, and is not fixed by this patch:
# this line should be an unresolved-reference, but we don't error on it:
[x for _ in [1]]
x = 1We should add this to the test suite. The fix should be implemented more like I described in my comment on your prior PR: when snapshotting visible bindings for names in eager nested scopes in semantic indexing, we also need to snapshot the fact that there are no visible bindings, and then respect that when we check eager_bindings above.
There was a problem hiding this comment.
Does this mean that we should add undefined symbols to the symbol table of the enclosing scope and register snapshots of empty bindings in eager_bindings?
Should we also register the builtin symbols as undefined ones?
There was a problem hiding this comment.
I don't think we should add the symbol to any outer scope. It may be that we don't actually need any change in semantic index building? We already will store no bindings in eager_bindings if there are no bindings for a name in a scope. It may be that all we need is a bit more sophisticated logic here in infer_name_load, where we need to track as we iterate enclosing scopes if we are still in "eager" context (that is, every scope we've traversed is eager), and if we find no eager binding in a scope while still in eager context, we need to just continue to the next scope rather than falling back to normal non-eager symbol resolution. Once we are out of "eager" context (that is, we have hit a non-eager scope), then eager bindings are no longer relevant and we should check normally for non-eager bindings in each scope. (So I don't think we need any special handling for builtin symbols.)
We should have tests for a case like this, too:
x = 1
def f():
class C:
y = [x for _ in [1]]
x = 2
reveal_type(C.y) # should reveal [1]And the same should be true if x = 1 occurs after the definition of f instead of before it.
There was a problem hiding this comment.
@dcreager would you be able to take a couple minutes to double-check my logic here and make sure I'm suggesting a sensible direction, since you wrote the initial eager-nested-scopes handling?
Summary
From #16861, and the continuation of #16915.
This PR fixes the incorrect behavior of
TypeInferenceBuilder::infer_name_loadin eager nested scopes.And this PR closes #16341.
Test Plan
New test cases are added in
annotations/deferred.md.