[red-knot] Resolve references in eager nested scopes eagerly

[dcreager]

We now resolve references in "eager" scopes correctly — using the bindings and declarations that are visible at the point where the eager scope is created, not the "public" type of the symbol (typically the bindings visible at the end of the scope).

Co-authored by @AlexWaygood

[MichaReiser]

This is great. I haven't reviewed the semantic changes but I do like it a lot that we moved the state to the UseDefMap

[dcreager]

Probably the best solution here is to add a new query that gets the type of the global symbol at the point the eager scope was created, rather than to add a new parameter to the global_scope() query.

It might actually be okay to push this issue to a followup PR! But I'd prefer to at least add some failing tests, since at the moment the behaviour here is a little inconsistent.

Added some failing tests for this. I think I can get the fix onto this PR as well

[dcreager]

Added some failing tests for this. I think I can get the fix onto this PR as well

This is fixed! This fix also affected the deferred expressions tests that @carljm suggested. (We were always showing lazy results because we weren't hitting the global scope, not because of how we were handling deferred expressions)

[carljm]

Big fan of this implementation! Really nice and clean, avoids duplicating eager-lookup logic between semantic index and type inference. Thank you!!

[carljm]

I added this locally and all tests passed. I think it's correct and could save on recording some class-scope bindings? Though only in relatively unusual cases, when a class or comprehension is nested directly in a class scope. So I suppose it's possible the cost of doing this check isn't actually worth it? One reason to do it regardless is just that it makes the comment in TypeInferenceBuilder::infer_name_load that "the semantic index builder takes care of only registering eager bindings for nested scopes that are actually eager, and for enclosing scopes that actually contain bindings that we should use when resolving the reference" more fully true. (Although it works even without being fully true because infer_name_load skips class scopes before checking for an eager binding.)

Suggested change

	let enclosing_scope_id = enclosing_scope_info.file_scope_id;
	let enclosing_scope_id = enclosing_scope_info.file_scope_id;
	// Names bound in class scopes are never visible to nested scopes, so we never need to
	// save eager scope bindings in a class scope.
	if matches!(self.scopes[enclosing_scope_id].kind(), ScopeKind::Class) {
	continue;
	}

(If we actually do this we should probably make it a bit nicer with an is_class() method on ScopeKind, and perhaps consolidate the two different self.scopes[enclosing_scope_id] lookups, here and below where we check eager-ness.)

[dcreager]

Done

[AlexWaygood]

Excellent -- thank you!

[AlexWaygood]

I wonder if it might be good to explicitly include an example of a generator-expression scope that does not run eagerly? It might be good to document that yes, this causes some incorrect assumptions from us in some edge cases, but that this is a cost we accept because (as you say) generator expressions nearly always run eagerly in practice. One example might be something like this: we report the revealed type of z is Literal[42]:

from typing_extensions import reveal_type

def _(flag: bool):
    y = (0,)
    z = 42
    gen = (x + reveal_type(z) for x in y)
    if flag:
        z = 56
    print(next(gen))

but at runtime, the print() call reveals that the runtime value of z can actually be 56 when the scope of gen() is actually evaluated:

>>> from typing import reveal_type
... 
... def _(flag: bool):
...     y = (0,)
...     z = 42
...     gen = (x + reveal_type(z) for x in y)
...     if flag:
...         z = 56
...     print(next(gen))
...     
>>> _(True)
Runtime type is 'int'
56

(Again, I'm not saying we should try to account for this -- generator expressions are almost always evaluated eagerly, and it would be hard to detect cases like this when they're not! Just suggesting we could add a test to explicitly document the shortcoming.)

[dcreager]

Done. I also added a test for how the "first iterable" is evaluated eagerly, even if the generator itself is not used immediately.

This example tells me that we'd want to handle generators the same as functions, if we ever start tracking where they're called as part of the public types work (#16079 (review)). After all, that's precisely the weirdness that's happening here — the generator includes a __next__ method that might be called at any arbitrary place, just like the f function in your linked comment might be called at any arbitrary place. If we start tracking that, we should use the same mechanism for both.

[AlexWaygood]

these are excellent, thank you!