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

chriskrycho · 2024-08-27T23:18:53Z

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!

crates/red_knot_python_semantic/src/types.rs

crates/red_knot_python_semantic/src/types/infer.rs

carljm · 2024-08-28T12:27:53Z

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 · 2024-08-28T13:35:14Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

- 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.

Co-authored-by: Alex Waygood <alex.waygood@gmail.com> Co-authored-by: Carl Meyer <carl@astral.sh>

Co-authored-by: Alex Waygood <alex.waygood@gmail.com>

This does not yet *actually* fix the underlying issue we were hoping it would fix, i.e. a panic when inferring types in `typing.pyi` (as seen when testing against `tomllib`), but it should be the first step in the right direction to enabling that.

This suggests that we are going to end up with a lot of this code scattered around. Once we have enough examples, we may want to take a step back and work out how to refactor this to be less ad hoc.

We require that all nodes have a type, even if that type is `Unknown` or `Unbound`, so in the case of annotation and type expressions, we must recurse through all child expressions to identify their types, even if we know they are actually “nonsense”. This will also be helpful in the future when we have a language server, where we would want to show as much valid information as possible even in invalid contexts.

crates/red_knot_python_semantic/src/types/infer.rs

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!

crates/red_knot_python_semantic/src/types/infer.rs

chriskrycho · 2024-08-29T22:46:28Z

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.

crates/red_knot_python_semantic/src/types/infer.rs

Co-authored-by: Carl Meyer <carl@oddbird.net>

Co-authored-by: Carl Meyer <carl@astral.sh>

codspeed-hq · 2024-08-29T23:18:20Z

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!

crates/red_knot_python_semantic/src/types/infer.rs

MichaReiser · 2024-09-03T07:40:37Z

crates/red_knot_python_semantic/src/types/infer.rs

@@ -77,6 +77,7 @@ fn infer_definition_types_cycle_recovery<'db>(
    _cycle: &salsa::Cycle,
    input: Definition<'db>,
 ) -> TypeInference<'db> {
+    tracing::trace!("infer_definition_types_cycle_recovery");


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).

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 · 2024-09-03T07:41:31Z

crates/red_knot_python_semantic/src/types/infer.rs

@@ -2059,6 +2086,173 @@ impl<'db> TypeInferenceBuilder<'db> {
    }
 }

+/// Annotation expressions.
+impl<'db> TypeInferenceBuilder<'db> {


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

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.

chriskrycho requested review from carljm, MichaReiser and AlexWaygood as code owners August 27, 2024 23:18

carljm added the red-knot Multi-file analysis & type inference label Aug 27, 2024

AlexWaygood reviewed Aug 28, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

chriskrycho and others added 9 commits August 29, 2024 14:43

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

8de0877

- 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.

[red-knot] insert annotation expressions into type expressions

d7500e9

[red-knot] improve doc for FunctionType::returns

b30904b

Co-authored-by: Alex Waygood <alex.waygood@gmail.com> Co-authored-by: Carl Meyer <carl@astral.sh>

[red-knot] use debug_assert for name ctx in infer_type_expression

4962e8d

Co-authored-by: Alex Waygood <alex.waygood@gmail.com>

[red-knot] add more tracing for type inference

5d503b8

[red-knot] handle deferred functions in type params inference

16cac96

This suggests that we are going to end up with a lot of this code scattered around. Once we have enough examples, we may want to take a step back and work out how to refactor this to be less ad hoc.

[red-knot] trace inference chain

a1eddd8

AlexWaygood reviewed Aug 29, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

carljm requested changes Aug 29, 2024

View reviewed changes

[red-knot] correct a couple TODO comments

a3d82a8

chriskrycho commented Aug 29, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

chriskrycho and others added 3 commits August 29, 2024 16:53

[red-knot] clean up extraneous tracing calls

b8072c7

Co-authored-by: Carl Meyer <carl@oddbird.net>

[red-knot] consistent naming for infer_*

0eaf748

Co-authored-by: Carl Meyer <carl@astral.sh>

[red-knot] eliminate duplication across annotation and type expressions

6af652c

carljm approved these changes Aug 30, 2024

View reviewed changes

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved

Update crates/red_knot_python_semantic/src/types/infer.rs

407c27e

carljm merged commit f8656ff into astral-sh:main Aug 30, 2024
20 checks passed

chriskrycho deleted the rk-basic-annotation-expressions branch August 30, 2024 15:29

MichaReiser reviewed Sep 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chriskrycho commented Aug 27, 2024

AlexWaygood left a comment

carljm commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading

carljm left a comment

chriskrycho commented Aug 29, 2024

codspeed-hq bot commented Aug 29, 2024 •

edited

Loading

carljm left a comment

MichaReiser Sep 3, 2024

carljm Sep 3, 2024

MichaReiser Sep 3, 2024

carljm Sep 3, 2024

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

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

Conversation

chriskrycho commented Aug 27, 2024

Summary

Test Plan

AlexWaygood left a comment

Choose a reason for hiding this comment

carljm commented Aug 28, 2024 • edited Loading

github-actions bot commented Aug 28, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

carljm left a comment

Choose a reason for hiding this comment

chriskrycho commented Aug 29, 2024

codspeed-hq bot commented Aug 29, 2024 • edited Loading

CodSpeed Performance Report

Merging #13130 will not alter performance

Summary

carljm left a comment

Choose a reason for hiding this comment

MichaReiser Sep 3, 2024

Choose a reason for hiding this comment

carljm Sep 3, 2024

Choose a reason for hiding this comment

MichaReiser Sep 3, 2024

Choose a reason for hiding this comment

carljm Sep 3, 2024

Choose a reason for hiding this comment

carljm commented Aug 28, 2024 •

edited

Loading

github-actions bot commented Aug 28, 2024 •

edited

Loading

`ruff-ecosystem` results

codspeed-hq bot commented Aug 29, 2024 •

edited

Loading