[red-knot] Make' Type::in_type_expression()' exhaustive for Type::KnownInstance #16836
Conversation
…ts for KnownInstance
|
@AlexWaygood what do you think of this to start? |
|
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This looks pretty good! Just Protocol needs attention.
crates/red_knot_python_semantic/resources/mdtest/annotations/optional.md
Outdated
Show resolved
Hide resolved
|
I'm not fully sure about the names BareSpecialFormAtLeastOne and BareSpecialFormExactlyOne, @AlexWaygood @carljm what do you think? |
|
The split you've made looks good. I think the names you've chosen are clear and I wouldn't argue with them, but it would probably also be ok to be a bit less verbose and go with |
crates/red_knot_python_semantic/resources/mdtest/annotations/callable.md
Show resolved
Hide resolved
Co-authored-by: Dhruv Manilawala <dhruvmanila@gmail.com>
…om/MatthewMckee4/ruff into make-in-type-expression-exhaustive
| /// Some types always require exactly one argument when used in a type expression | ||
| RequiresOneArgument(Type<'db>), | ||
| /// The `Protocol` type is invalid in type expressions | ||
| ProtocolInTypeExpression, |
There was a problem hiding this comment.
Personally I feel like the InTypeExpression part of this is redundant with the enum name InvalidTypeExpression; I would just name this variant Protocol. But I see there is already precedent with ClassVarInTypeExpression and FinalInTypeExpression, so I'll go ahead and merge it this way for consistency.
|
Thank you for the PR!! |
| Type::KnownInstance( | ||
| KnownInstanceType::TypingSelf | ||
| | KnownInstanceType::ReadOnly | ||
| | KnownInstanceType::TypeAlias | ||
| | KnownInstanceType::NotRequired | ||
| | KnownInstanceType::Concatenate | ||
| | KnownInstanceType::TypeIs | ||
| | KnownInstanceType::TypeGuard | ||
| | KnownInstanceType::Unpack | ||
| | KnownInstanceType::Required, | ||
| ) => Ok(todo_type!( | ||
| "Invalid or unsupported `KnownInstanceType` in `Type::to_type_expression`" | ||
| )), |
There was a problem hiding this comment.
I think the only two of these that are valid in type expressions when they are not parameterized are TypingSelf and TypeAlias:
ReadOnly,NotRequired,TypeIs,TypeGuard,UnpackandRequiredall require exactly one argumentConcatenaterequires at least two arguments
There was a problem hiding this comment.
ReadOnly, NotRequired and Required are also type qualifiers, so they are only valid in annotation expressions, not type expressions, even when they do have the necessary number of arguments
There was a problem hiding this comment.
arent ReadOnly, NotRequired, TypeIs, TypeGuard, Unpack and Required only allowed in certain places though? Like how ClassVar and Final are invalid here?
There was a problem hiding this comment.
That's correct. But the KnownInstance branches of this method are essentially asking the question, "Is this symbol valid in a type expression if it appears without any type arguments?". And the answer is definitely "no" for all of ReadOnly, NotRequired, TypeIs, TypeGuard, Unpack and Required. It's true that these symbols wouldn't even be allowed in a all type expressions even if they did have the correct number of type arguments. But that's not the question that this method is asking ;)
There was a problem hiding this comment.
In terms of the exact error messages we should emit, I think probably:
ReadOnly,NotRequired,Required=>"Type qualifier `{}` is not allowed in type expressions (only in annotation expressions, and only with exactly one argument)"TypeIs,TypeGuard,Unpack=>"`{}` requires exactly one argument when used in a type expression"- you could reuse your existing
InvalidTypeExpression::RequiresOneArgumentvariant for these
- you could reuse your existing
Concatenate=>"`{}` requires at least two arguments when used in a type expression"- you could maybe rework the
InvalidTypeExpression::BareAnnotatedvariant so that it can be used to report errors forConcatenateas well asAnnotated
- you could maybe rework the
Would you be interested in filing a followup PR? :-)
It could also be nice to have more precise todo_type!() messages for the TypingSelf and TypeAlias variants: todo_type!("Support for `typing.Self`") and todo_type!("Support for `typing.TypeAlias`"), respectively
There was a problem hiding this comment.
Yeah ill get started on that, thanks
There was a problem hiding this comment.
ReadOnly, NotRequired, Required => "Type qualifier
{}is not allowed in type expressions (only in annotation expressions, and only with exactly one argument)"
Am i right in saying that these should be the same message as FinalInTypeExpression and ClassVarInTypeExpression?
There was a problem hiding this comment.
Amazing, thank you! You're doing great work :D
There was a problem hiding this comment.
Am i right in saying that these should be the same message as FinalInTypeExpression and ClassVarInTypeExpression?
Not quite. ClassVar, Final, Required, NotRequired and ReadOnly are all type qualifiers, so they're all valid in annotation expressions but invalid in type expressions. Nonetheless, I think it would be nice to have different error messages because of the fact that they differ in the number of arguments they accept when they appear in annotation expressions:
Finalcan be used both with and without type arguments in an annotation expressionClassVarit's... sort-of unclear whether it's valid without type arguments or not (see discussion at https://discuss.python.org/t/bare-classvar-annotation/81705)- All the rest need exactly one argument when they appear in annotation expressions
So that implies that for the error messages, Final and ClassVar should have this error message:
"Type qualifier `{}` is not allowed in type expressions (only in annotation expressions)"
But for the other three, they should have this error message:
"Type qualifier `{}` is not allowed in type expressions (only in annotation expressions, and only with exactly one argument)"
4 participants
Summary
fixes #15048
We want to handle more types from Type::KnownInstance
Test Plan
Add tests for each type added explicitly in the match