[red-knot] Emit errors for more AST nodes that are invalid (or only valid in specific contexts) in type expressions

[MatthewMckee4]

Summary

Add error messages for invalid nodes in type expressions

Fixes #16816

Test Plan

Extend annotations/invalid.md to handle these invalid AST nodes error messages

[MatthewMckee4]

@AlexWaygood What do you think of making this a instance method?

[MatthewMckee4]

I was having issues with mutable references in ast::Expr::BoolOp as infer_boolean_expression requires a mutable reference to self, and i believe the closure does too

[github-actions]

mypy_primer results

No ecosystem changes detected ✅

[MatthewMckee4]

I havent covered slice type expression yet

[MatthewMckee4]

I believe tests failing is mainly due to Callable type inference calls this function with tuple expression instead of the individual arguments, ill look into this more

[MatthewMckee4]

The errors mainly stem from "Tuples are not allowed in type expressions"

In generic arguments with > 1 generic parameter

I'm not really sure the best way to handle this.

Perhaps we can create a new function that can call infer_type_expression_no_store if no tuples are expected in the Expr and else it can try to "flatten" the type and / or call infer_type_expression_no_store on each type in the Tuple
*or infer_type_expression

@AlexWaygood What are your thoughts?

[dhruvmanila]

I believe tests failing is mainly due to Callable type inference calls this function with tuple expression instead of the individual arguments, ill look into this more

Yeah, I haven't looked at the test failures but I think those are correct because we fallback to calling infer_type_expression for the slice expression if it's an invalid type form for a specific special form like Callable. For example:

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

Lines 6584 to 6590 in f9ee155

	let [first_argument, second_argument] = arguments.as_slice() else {
	report_invalid_arguments_to_callable(self.db(), &self.context, subscript);
	self.infer_type_expression(arguments_slice);
	return Type::Callable(CallableType::General(GeneralCallableType::unknown(
	self.db(),
	)));
	};

This is a requirement because currently the tests require that every expression has an inferred type even in the case of failure:

ruff/crates/red_knot_project/tests/check.rs

Lines 165 to 171 in f9ee155

	fn pull_types(db: &ProjectDatabase, file: File) {
	let mut visitor = PullTypesVisitor::new(db, file);
	let ast = parsed_module(db, file);
	visitor.visit_body(ast.suite());
	}

[MatthewMckee4]

    fn infer_type_expression_or_tuple_type_expression(
        &mut self,
        expression: &ast::Expr,
    ) -> Type<'db> {
        match expression {
            ast::Expr::Tuple(ast::ExprTuple {
                elts: expressions, ..
            }) => Type::Tuple(TupleType::new(
                self.db(),
                expressions
                    .iter()
                    .map(|expression| self.infer_type_expression(expression))
                    .collect::<Box<_>>(),
            )),
            _ => self.infer_type_expression(expression),
        }
    }

This was sort of what i was thinking, and anywhere we call infer_type_expression and want to allow tuple expressions, we can call this. This is useful for

            KnownInstanceType::Dict => {
                self.infer_type_expression(arguments_slice);
                KnownClass::Dict.to_instance(self.db())
            }

Where if we used infer_type_expression_or_tuple_type_expression, we can actually get the Type of the tuple back, for the future as these generics are currently not supported. But for KnownInstaneType::List, we continue to use infer_type_expression

[AlexWaygood]

I'd be inclined to postpone the complicated ones like Expr::Tuple and Expr::List for now and just do the easy ones in this PR -- we can tackle the complicated ones in followup(s). PRs are cheap! :-)

[MatthewMckee4]

Okay can revert them to no error message

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[carljm]

LGTM, thank you! Will leave it for @AlexWaygood to merge in case he has any review comments.

[MatthewMckee4]

I need to test Slice node and ill also improve some error messages

[carljm]

FWIW I'm not personally a big fan of providing arbitrary "examples" in the error messages ("like 1 := 'foo'", "like 1 < 2") in the error messages. It seems a bit arbitrary what kind of example to select, it seems confusing to me to give specific code examples that don't actually come from the user's code, and it seems unnecessary because, by definition, we already have a suitable example at hand: the one from the user's code that we are emitting the diagnostic on!

So I would be inclined to remove these from the error messages, but open to what others think.

[MatthewMckee4]

That's fair, my thinking was that "Unary operations", and others, could a bit vague for some users.

Then again, you could argue, people who find themselves writing these examples in type expressions, probably know what they are called.

I'm happy to revert either way

[carljm]

I think the strongest argument against is that the diagnostic will literally always be pointing to a specific example in the user's code, which seems like it should make things pretty clear.

Given Alex's thumbs-up above, yeah I think you should go ahead and remove these from the messages.

[AlexWaygood]

Thank you, this is great! I pushed a couple of nitpick changes, but nothing major.