Skip to content

[ty] fix panic with direct recursive type alias #21434

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

Closed
wants to merge 1 commit into from
+19 −1
Closed

[ty] fix panic with direct recursive type alias #21434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions crates/ty_python_semantic/resources/mdtest/pep695_type_aliases.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,16 @@ def f(x: IntOr, y: OrInt):
reveal_type(x) # revealed: Never
if not isinstance(y, int):
reveal_type(y) # revealed: Never

# TODO emit a diagnostic on this line
type Itself = Itself
Copy link
Member

@MichaReiser MichaReiser Nov 13, 2025

Choose a reason for hiding this comment

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

Does this fix also work for compound types like list[Itself]?

Copy link
Contributor

@carljm carljm Nov 13, 2025

Choose a reason for hiding this comment

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

Those generally already work, without this PR: https://play.ty.dev/a65c2d75-e9c1-4a74-85e3-f96db2dd9c60

Copy link
Contributor

@carljm carljm Nov 13, 2025

Choose a reason for hiding this comment

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

Another case that may be worth testing is multi-step direct recursion:

type A = B
type B = A

Copy link
Contributor Author

@mtshiba mtshiba Nov 14, 2025

Choose a reason for hiding this comment

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

Hmm, unfortunately, it seems the panic still persists in the case of mutually recursive aliases:

type A = B
type B = A

def bar(a: B):
    x: a

Therefore, the measure is insufficient, and we should check the consequences of eagerly expanding a type alias value type before we first use of it.
This may be an issue that needs to be resolved in #20566.


def foo(Itself: Itself):
x: Itself
Comment on lines +251 to +252
Copy link
Contributor

@carljm carljm Nov 13, 2025

Choose a reason for hiding this comment

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

IMO it's a bit odd to test that you can annotate a parameter as Itself, and then use that parameter again as an annotation. It may work due to how we treat Unknown in a type annotation, but I don't think this is something we want to commit to in a test.

I would probably just have done this instead:

Suggested change
def foo(Itself: Itself):
x: Itself
def foo(x: Itself):
reveal_type(x) # Unknown

Is there a particular reason why you think it's important to test this "annotated value as annotation" behavior?

Copy link
Contributor Author

@mtshiba mtshiba Nov 14, 2025

Choose a reason for hiding this comment

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

In fact, in that example, no panic occurs, and the revealed type is Never.

https://play.ty.dev/4732e30a-301c-4753-b95f-aed34cf08585

Should the original code be placed in the corpus as a regression test?

Copy link
Contributor

@carljm carljm Nov 14, 2025

Choose a reason for hiding this comment

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

Ah, I wondered if this part was necessary to reproduce the panic. That explains why I didn't catch this case in my previous work on recursive type aliases. Makes it much less likely people will run into this, but of course it should still be fixed.

I think it's OK for a test like this to be in mdtests, I would just expect some commentary (specifically about annotating with a value type) that "this is a very strange thing to do, but this is a regression test to ensure it doesn't panic"


# A type alias defined with invalid recursion behaves as a dynamic type.
foo(42)
foo("hello")
```

### With legacy generic
Expand Down
10 changes: 9 additions & 1 deletion crates/ty_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11486,7 +11486,15 @@ impl<'db> PEP695TypeAliasType<'db> {
let type_alias_stmt_node = scope.node(db).expect_type_alias();
let definition = self.definition(db);

definition_expression_type(db, definition, &type_alias_stmt_node.node(&module).value)
let ty =
definition_expression_type(db, definition, &type_alias_stmt_node.node(&module).value);
// A type alias where a value type points to itself is meaningless and would lead to infinite recursion. Replace it with `Unknown`.
if ty == Type::TypeAlias(TypeAliasType::PEP695(self)) {
// TODO: Emit a diagnostic for invalid recursive type alias.
Type::unknown()
} else {
ty
}
}

fn apply_function_specialization(self, db: &'db dyn Db, ty: Type<'db>) -> Type<'db> {
Expand Down
Loading