Skip to content

[ty] Recursive protocols #17929

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.

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 8 commits into from
May 9, 2025
Merged

[ty] Recursive protocols #17929

merged 8 commits into from
May 9, 2025

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented May 7, 2025

Summary

Use a self-reference "marker" and fixpoint iteration to solve the stack overflow problems with recursive protocols. This is not pretty and somewhat tedious, but seems to work fine. Much better than all my fixpoint-iteration attempts anyway.

closes astral-sh/ty#93

Test Plan

New Markdown tests.

@sharkdp sharkdp added the ty Multi-file analysis & type inference label May 7, 2025
Copy link
Contributor

github-actions bot commented May 7, 2025

mypy_primer results

No ecosystem changes detected ✅

@sharkdp sharkdp force-pushed the david/recursive-protocols branch 3 times, most recently from 8649c75 to d51c5d5 Compare May 8, 2025 13:13
@sharkdp sharkdp marked this pull request as ready for review May 8, 2025 13:17
@sharkdp sharkdp closed this May 8, 2025
@sharkdp sharkdp reopened this May 8, 2025
@sharkdp
Copy link
Contributor Author

sharkdp commented May 8, 2025

Moving this out of draft-mode to get a general review before I spend more time resolving all the minor TODOs.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

This looks like an excellent solution to me!

@sharkdp sharkdp force-pushed the david/recursive-protocols branch from 201323e to e0bb333 Compare May 8, 2025 14:20
Self::Callable(callable) => Self::Callable(callable.replace_self_reference(db, class)),

Self::GenericAlias(_) | Self::TypeVar(_) => {
// TODO: replace self-references in generic aliases and typevars
Copy link
Member

Choose a reason for hiding this comment

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

Was just about to ask if we could re-use this technique for generics 😄

👍 for it being a TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The doc-comment of this function is probably more relevant? This is just the place where we would also have to look for references to the protocol class.

Instead of ClassLiteral, I would imagine that this function would maybe have to take Type as an argument if we want to handle self-referential type aliases like type Tree = dict[str, Tree]. Or maybe an enum (different things that we would want to replace).

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Thank you!!

This clearly addresses a lot of the practical cases we see, but it does still have a problem with mutual recursion of multiple different classes.

I think we could consider landing this anyway as an initial step, but I think we will need to figure out how to generalize it beyond simple self-reference.

#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, salsa::Update, PartialOrd, Ord)]
pub(super) enum ProtocolInterface<'db> {
Members(ProtocolInterfaceMembers<'db>),
SelfReference,
Copy link
Contributor

@carljm carljm May 8, 2025

Choose a reason for hiding this comment

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

It seems like this approach has a significant limitation, in that it can handle direct self-reference, but it can't handle mutual recursion (because we can only represent a self-reference, not a "recursive reference to arbitrary something", which means we can't break a recursive loop involving more than one protocol.) And indeed, this still stack overflows on this branch:

from __future__ import annotations

from typing import Protocol
from ty_extensions import is_fully_static, is_subtype_of, static_assert

class Foo(Protocol):
    x: Bar

class Bar(Protocol):
    x: Foo

static_assert(is_fully_static(Foo))
static_assert(is_fully_static(Bar))

@sharkdp
Copy link
Contributor Author

sharkdp commented May 8, 2025

After talking to @carljm we decided to merge this as an immediate solution to an observed real-world problem. I don't think it introduces any wrong behavior either. And as Carl pointed out, the tests are hopefully useful in any case.

That said, we also think that there are severe limitations here (see Carl's comment regarding mutually-recursive protocols). And it's also not clear if this is a generalizable solution that would also work for self-referential type aliases or generic classes. @dcreager: it's probably not useful to spend time right now trying to make this specific approach here work for generics.

What I will do instead after merging this is to take one step back and think about the broader problem here, and do some research. Hopefully that leads to a general solution. I might not be able to spend the necessary amount of time on this before Tuesday. So if anyone else wants to work on this before then, feel free to do that (let me know).

@sharkdp sharkdp force-pushed the david/recursive-protocols branch from e35088e to 88e5b58 Compare May 9, 2025 12:44
@sharkdp sharkdp merged commit 642eac4 into main May 9, 2025
35 checks passed
@sharkdp sharkdp deleted the david/recursive-protocols branch May 9, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflows for recursive protocols
5 participants