[red-knot] infer basic (name-based) annotation expressions

[chriskrycho]

Summary

• Introduce methods for inferring annotation and type expressions.
• Correctly infer explicit return types from functions where they are simple names that can be resolved in scope.

Contributes to #12701 by way of helping unlock call expressions (this does not remotely finish that, as it stands, but it gets us moving that direction).

Test Plan

Added a test for function return types which use the name form of an annotation expression, since this is aiming toward call expressions. When we extend this to working for other annotation and type expression positions, we should add explicit tests for those as well.

[AlexWaygood]

Nice! A couple of nitpicks below.

Here's some high-level thoughts that shouldn't block this PR, just things we could think about as followups:

• it might be nice if infer_annotation_expression took a custom TypeExpression enum as its input argument rather than the ast::Expr enum, to codify the fact that a valid type expression necessarily excludes certain Python expression nodes entirely, such as Expr::Calls, Expr::Lambdas, Expr::Named, etc.
• currently we're not looking at any qualifiers an annotation expression might or might not include that wrap the annotation's type expression. But in the long run, we'll want to take note of those and record them somewhere. It might make sense for infer_annotation_expression to return something like (HasSet<TypeQualifier>, Type<'db>) rather than simply Type<'db>.

Again, these high-level comments are just me thinking out loud about how we might want this code to work in the long run; not really anything you need to respond to in this PR!

[carljm]

Replying to a couple of the high-level thoughts here, though I agree that neither of them should be implemented in this PR:

• it might be nice if infer_annotation_expression took a custom TypeExpression enum as its input argument rather than the ast::Expr enum, to codify the fact that a valid type expression necessarily excludes certain Python expression nodes entirely, such as Expr::Calls, Expr::Lambdas, Expr::Named, etc.

I'm not sure about this. It implies that we would then need to have some other method that takes an ast::Expr, emits diagnostics for invalid forms, and returns the custom enum. But it seems to me that it will be simpler, clearer, and more efficient if that is the job of infer_annotation_expression itself. Why match twice when we can match once, emit diagnostics on invalid forms and perform correct resolution of valid forms?

It seems to me like this is an idea we can keep in mind if at some point things reach a level of complexity where it really concretely improves type safety (for instance, if we have built up a large collection of methods that are subsidiary to infer_annotation_expression and all should operate only on a valid type expression form -- but I'm not convinced this will ever happen, because I think the subsidiary methods are more likely to operate on a specific expression kind instead), but this isn't something we should be a priori aiming for, or introducing for its own sake if its only taken by a single method.

currently we're not looking at any qualifiers an annotation expression might or might not include that wrap the annotation's type expression. But in the long run, we'll want to take note of those and record them somewhere. It might make sense for infer_annotation_expression to return something like (HasSet<TypeQualifier>, Type<'db>) rather than simply Type<'db>.

Yes, agreed. Probably best to sort out the details here once we are adding support for a type qualifier, so we can design it in context with how the information is used.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[carljm]

Great work! In terms of the broad strokes, this looks excellent.

A lot of my comments are basically just a single comment that I don't think we should add such fine-grained tracing of type inference by default; if we did it everywhere, it'd be so noisy in any realistic case that you'd never be able to find anything. When debugging at that level, it works better IMO to manually add the specific dbg!() calls that are relevant to the case you're debugging. Open to discussing this if you think I'm missing the value of this tracing!

[chriskrycho]

Ha, very strongly agreed re: tracing instrumentation – I was going to ask if you thought any of those were valuable to keep, and I think the answer here is basically “no”. I’ll double check on the annotation-vs.-type-expressions thing as well and respond on that specific comment.

[codspeed-hq]

CodSpeed Performance Report

Merging #13130 will not alter performance

_{Comparing chriskrycho:rk-basic-annotation-expressions (407c27e) with main (34b4732)}

Summary

✅ 32 untouched benchmarks

[carljm]

Looks great!

[MichaReiser]

We should change this to an actual sentence rather than the method name. Tracing messages are user facing (not trace but we might decide to make them user facing in the future).

[carljm]

This function is a partially-broken temporary crutch just to get us through until we have fixpoint iteration and full deferred resolution of annotations in the necessary places; it should go away. So I'm not too worried about this particular trace message sticking around for a long time. But this is a good point.

[MichaReiser]

What's the motivation for placing each method in its own impl block?

[carljm]

The idea was not "each method" but "each set of related methods", though at the moment it is just one method for annotation expressions and one for type expressions; that will change. It was just a way to more clearly separate inference of value expressions from inference of annotation expressions and type expressions.

This was Chris' idea and it seemed fine to me, but I'm also fine with just using comment headers to achieve the same separation.