[red-knot] feat: introduce a new [Type::Todo] variant

[Slyces]

This variant shows inference that is not yet implemented..

Summary

PR #13500 reopened the idea of adding a new type variant to keep track of not-implemented features in Red Knot.

It was based off of #12986 with a more generic approach of keeping track of different kind of unknowns. Discussion in #13500 agreed that keeping track of different Unknown is complicated for now, and this feature is better achieved through a new variant of Type.

Requirements

Requirements for this implementation can be summed up with some extracts of comment from @carljm on the previous PR

So at the moment we are leaning towards simplifying this PR to just use a new top-level variant, which behaves like Any and Unknown but represents inference that is not yet implemented in red-knot.

I think the general rule should be that Todo should propagate only when the presence of the input Todo caused the output to be unknown.

To take a specific example, the inferred result of addition must be Unknown if either operand is Unknown. That is, Unknown + X will always be Unknown regardless of what X is. (Same for X + Unknown.) In this case, I believe that Unknown + Todo (or Todo + Unknown) should result in Unknown, not result in Todo. If we fix the upstream source of the Todo, the result would still be Unknown, so it's not useful to propagate the Todo in this case: it wrongly suggests that the output is unknown because of a todo item.

Test Plan

This PR does not introduce new tests, but it did required to edit some tests with the display of [Type::Todo] (currently @Todo), which suggests that those test are placeholders requirements for features we don't support yet.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[AlexWaygood]

Nice! This looks pretty good to me. Here's a few nitpicks about comments

[AlexWaygood]

Thank you! I think there are a few other places where we could use this new variant:

(1) Both of these branches here:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 502 to 506 in bee498d

	Type::IntLiteral(_) => {
	// TODO raise error
	Type::Unknown
	}
	Type::BooleanLiteral(_) => Type::Unknown,

Objects that have literal-int or literal-bool types do have attributes and methods; we shouldn't just infer Unknown for all member accesses in these branches. For all attribute lookups in these branches, we'll either need to special-case the specific attribute or defer to the typeshed stubs for bool / int; Type::TODO seems better than Type::Unknown to indicate that.

(2) These two branches here:

ruff/crates/red_knot_python_semantic/src/types.rs

Lines 732 to 735 in bee498d

	// TODO: `type[Any]`?
	Type::Any => Type::Any,
	// TODO: `type[Unknown]`?
	Type::Unknown => Type::Unknown,

Once we support type[T], we can infer type[Any] and type[Unknown] for these branches, which is more precise than Any and Unknown. For example, whereas <instance_of_Any>.__module__ is Any, <instance_of_type_Any>.__module__ is a str. So Type::TODO is appropriate here, I think.

(3) These branches:

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

Lines 783 to 785 in bee498d

	// TODO(dhruvmanila): Annotation expression is resolved at the enclosing scope, infer the
	// parameter type from there
	let annotated_ty = Type::Unknown;

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

Lines 1027 to 1031 in bee498d

	// TODO(dhruvmanila): The correct way to infer types here is to perform structural matching
	// against the subject expression type (which we can query via `infer_expression_types`)
	// and extract the type at the `index` position if the pattern matches. This will be
	// similar to the logic in `self.infer_assignment_definition`.
	self.add_binding(pattern.into(), definition, Type::Unknown);

As the comments say, we should be able to infer much more precise types for these once we've implemented some currently-missing features

(4) The Unknown branch here:

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

Lines 980 to 986 in bee498d

	// TODO: anything that's a consistent subtype of
	// `type[BaseException] | tuple[type[BaseException], ...]` should be valid;
	// anything else should be invalid --Alex
	match node_ty {
	Type::Any | Type::Unknown => node_ty,
	Type::Class(class_ty) => Type::Instance(class_ty),
	_ => Type::Unknown,

We should be able to infer better types for bindings in situations such as except (OSError, TypeError) as e -- we currently infer the type of e as Unknown, but it should really be OSError | TypeError. Similarly for except EXCEPTIONS as f where EXCEPTIONS is a variable that has type tuple[type[OSError], type[TypeError]] -- we'll currently just infer Unknown for f, but the type of f should also be OSError | TypeError.

---

There's also one place where I think we should add a comment, because Unknown is correct but it's not obvious why it's correct:

--- a/crates/red_knot_python_semantic/src/types/infer.rs
+++ b/crates/red_knot_python_semantic/src/types/infer.rs
@@ -968,6 +968,8 @@ impl<'db> TypeInferenceBuilder<'db> {
         let node_ty = except_handler_definition
             .handled_exceptions()
             .map(|ty| self.infer_expression(ty))
+            // If there is no handled exception, it's invalid syntax;
+            // a diagnostic will have already been emitted
             .unwrap_or(Type::Unknown);
 
         let symbol_ty = if except_handler_definition.is_star() {

[Slyces]

Thank you so much for looking into this with so much detail, and sorry for missing some cases. Branches 1, 3, 4 lead to no issues with existing tests.

Branch 2 however requires additional code for invalid syntax in comprehensions as:

• Invalid comprehensions can have Type::Unknown as the target iterator
• Type::Unknown's metaclass being Unknown, the method Type::iterate end up yielding Unknown
• When Type::Unknown's metaclass becomes Type::Todo, this does not work anymore

I fixed this by introducing an explicit handling of Unknown (and we might need one for Any as well, but I didn't find a way to reliably create an Any var to test it)

[AlexWaygood]

LGTM! I'll wait before merging in case Micha or Carl have any concerns, though.

Possibly we could change the variant so that it is instead Type::Todo(&'static str) -- and we could store the TODO message in the variant itself, including it as part of the variant's display. That would mean we wouldn't have to have a separate // TODO comment for every time where we use this variant. However, I think that that could/should be implemented as a followup, since it adds more complexity to the current code.

[carljm]

This is great, thank you!! I think this will really improve debuggability. Just a few small nits before we merge.