Skip to content

[red-knot] Make' Type::in_type_expression()' exhaustive for Type::KnownInstance #16836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 19, 2025
Merged

[red-knot] Make' Type::in_type_expression()' exhaustive for Type::KnownInstance #16836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1042121
Create InvalidBareType type in InvalidTypeExpression and add some tes…
MatthewMckee4 Mar 18, 2025
50fbcf4
Catch each match case of KnownInstanceType
MatthewMckee4 Mar 18, 2025
6306fd8
Remove unused test
MatthewMckee4 Mar 18, 2025
dcf394b
Fix Protocol error in type expression
MatthewMckee4 Mar 19, 2025
cd16ad6
Split up BareSpecialForm into BareSpecialFormAtLeastOne and BareSpeci…
MatthewMckee4 Mar 19, 2025
6079ee4
Make InvalidTypeExpression type less verbose
MatthewMckee4 Mar 19, 2025
5a17838
Update intersection_types.md
MatthewMckee4 Mar 19, 2025
8b28a93
Move md tests to correct sections and add back link
MatthewMckee4 Mar 19, 2025
ff73a7f
Merge branch 'make-in-type-expression-exhaustive' of https://github.c…
MatthewMckee4 Mar 19, 2025
fb56dc2
Fix tests
MatthewMckee4 Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/red_knot_python_semantic/resources/mdtest/annotations/literal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def f():
```py
from typing import Literal

# error: [invalid-type-form] "`Literal` requires at least one argument when used in a type expression"
# error: [invalid-type-form] "`typing.Literal` requires at least one argument when used in a type expression"
def _(x: Literal):
reveal_type(x) # revealed: Unknown
```
10 changes: 10 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/annotations/optional.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,13 @@ def f():
# revealed: int | None
reveal_type(a)
```

## Invalid

```py
from typing import Optional

# error: [invalid-type-form] "`typing.Optional` requires exactly one argument when used in a type expression"
def f(x: Optional) -> None:
reveal_type(x) # revealed: Unknown
```
10 changes: 10 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/annotations/union.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,13 @@ def f():
# revealed: int | str
reveal_type(a)
```

## Invalid

```py
from typing import Union

# error: [invalid-type-form] "`typing.Union` requires at least one argument when used in a type expression"
def f(x: Union) -> None:
reveal_type(x) # revealed: Unknown
```
14 changes: 14 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/intersection_types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,5 +846,19 @@ def mixed(
reveal_type(i4) # revealed: Any & Unknown
```

## Invalid

```py
from knot_extensions import Intersection, Not

# error: [invalid-type-form] "`knot_extensions.Intersection` requires at least one argument when used in a type expression"
def f(x: Intersection) -> None:
reveal_type(x) # revealed: Unknown

# error: [invalid-type-form] "`knot_extensions.Not` requires exactly one argument when used in a type expression"
def f(x: Not) -> None:
reveal_type(x) # revealed: Unknown
```

[complement laws]: https://en.wikipedia.org/wiki/Complement_(set_theory)
[de morgan's laws]: https://en.wikipedia.org/wiki/De_Morgan%27s_laws
10 changes: 10 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/protocols.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,13 @@ def _(some_int: int, some_literal_int: Literal[1], some_indexable: SupportsIndex
b: SupportsIndex = some_literal_int
c: SupportsIndex = some_indexable
```

## Invalid

```py
from typing import Protocol

# error: [invalid-type-form] "`typing.Protocol` is not allowed in type expressions"
def f(x: Protocol) -> None:
reveal_type(x) # revealed: Unknown
```
8 changes: 8 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/type_api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,10 @@ def type_of_annotation() -> None:

# error: "Special form `knot_extensions.TypeOf` expected exactly one type parameter"
t: TypeOf[int, str, bytes]

# error: [invalid-type-form] "`knot_extensions.TypeOf` requires exactly one argument when used in a type expression"
def f(x: TypeOf) -> None:
reveal_type(x) # revealed: Unknown
```

## `CallableTypeFromFunction`
Expand All @@ -418,6 +422,10 @@ def f3(x: int, y: str) -> None:
c1: CallableTypeFromFunction[f1, f2]
# error: [invalid-type-form] "Expected the first argument to `knot_extensions.CallableTypeFromFunction` to be a function literal, but got `Literal[int]`"
c2: CallableTypeFromFunction[int]

# error: [invalid-type-form] "`knot_extensions.CallableTypeFromFunction` requires exactly one argument when used in a type expression"
def f(x: CallableTypeFromFunction) -> None:
reveal_type(x) # revealed: Unknown
```

Using it in annotation to reveal the signature of the function:
Expand Down
64 changes: 55 additions & 9 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3256,10 +3256,6 @@ impl<'db> Type<'db> {
],
fallback_type: Type::unknown(),
}),
Type::KnownInstance(KnownInstanceType::Literal) => Err(InvalidTypeExpressionError {
invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareLiteral],
fallback_type: Type::unknown(),
}),
Type::KnownInstance(KnownInstanceType::Unknown) => Ok(Type::unknown()),
Type::KnownInstance(KnownInstanceType::AlwaysTruthy) => Ok(Type::AlwaysTruthy),
Type::KnownInstance(KnownInstanceType::AlwaysFalsy) => Ok(Type::AlwaysFalsy),
Expand All @@ -3269,7 +3265,44 @@ impl<'db> Type<'db> {
GeneralCallableType::unknown(db),
)))
}
Type::KnownInstance(_) => Ok(todo_type!(
Type::KnownInstance(
KnownInstanceType::Literal
| KnownInstanceType::Union
| KnownInstanceType::Intersection,
) => Err(InvalidTypeExpressionError {
invalid_expressions: smallvec::smallvec![InvalidTypeExpression::RequiresArguments(
*self
)],
fallback_type: Type::unknown(),
}),
Type::KnownInstance(
KnownInstanceType::Optional
| KnownInstanceType::Not
| KnownInstanceType::TypeOf
| KnownInstanceType::CallableTypeFromFunction,
) => Err(InvalidTypeExpressionError {
invalid_expressions: smallvec::smallvec![
InvalidTypeExpression::RequiresOneArgument(*self)
],
fallback_type: Type::unknown(),
}),
Type::KnownInstance(KnownInstanceType::Protocol) => Err(InvalidTypeExpressionError {
invalid_expressions: smallvec::smallvec![
InvalidTypeExpression::ProtocolInTypeExpression
],
fallback_type: Type::unknown(),
}),
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`"
)),
Comment on lines +3295 to 3307
Copy link
Member

@AlexWaygood AlexWaygood Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, Unpack and Required all require exactly one argument
  • Concatenate requires at least two arguments

Copy link
Member

@AlexWaygood AlexWaygood Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MatthewMckee4 MatthewMckee4 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arent ReadOnly, NotRequired, TypeIs, TypeGuard, Unpack and Required only allowed in certain places though? Like how ClassVar and Final are invalid here?

Copy link
Member

@AlexWaygood AlexWaygood Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@AlexWaygood AlexWaygood Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::RequiresOneArgument variant for these
  • Concatenate => "`{}` requires at least two arguments when used in a type expression"
    • you could maybe rework the InvalidTypeExpression::BareAnnotated variant so that it can be used to report errors for Concatenate as well as Annotated

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

Copy link
Contributor Author

@MatthewMckee4 MatthewMckee4 Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah ill get started on that, thanks

Copy link
Contributor Author

@MatthewMckee4 MatthewMckee4 Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@AlexWaygood AlexWaygood Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thank you! You're doing great work :D

Copy link
Member

@AlexWaygood AlexWaygood Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • Final can be used both with and without type arguments in an annotation expression
  • ClassVar it'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)"

Type::Instance(_) => Ok(todo_type!(
Expand Down Expand Up @@ -3542,8 +3575,12 @@ impl<'db> InvalidTypeExpressionError<'db> {
enum InvalidTypeExpression<'db> {
/// `x: Annotated` is invalid as an annotation
BareAnnotated,
/// `x: Literal` is invalid as an annotation
BareLiteral,
/// Some types always require at least one argument when used in a type expression
RequiresArguments(Type<'db>),
/// 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,
Copy link
Contributor

@carljm carljm Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// The `ClassVar` type qualifier was used in a type expression
ClassVarInTypeExpression,
/// The `Final` type qualifier was used in a type expression
Expand All @@ -3565,8 +3602,17 @@ impl<'db> InvalidTypeExpression<'db> {
InvalidTypeExpression::BareAnnotated => f.write_str(
"`Annotated` requires at least two arguments when used in an annotation or type expression"
),
InvalidTypeExpression::BareLiteral => f.write_str(
"`Literal` requires at least one argument when used in a type expression"
InvalidTypeExpression::RequiresOneArgument(ty) => write!(
f,
"`{ty}` requires exactly one argument when used in a type expression",
ty = ty.display(self.db)),
InvalidTypeExpression::RequiresArguments(ty) => write!(
f,
"`{ty}` requires at least one argument when used in a type expression",
ty = ty.display(self.db)
),
InvalidTypeExpression::ProtocolInTypeExpression => f.write_str(
"`typing.Protocol` is not allowed in type expressions"
),
InvalidTypeExpression::ClassVarInTypeExpression => f.write_str(
"Type qualifier `typing.ClassVar` is not allowed in type expressions (only in annotation expressions)"
Expand Down
Loading