Skip to content

[red-knot] Support TypeGuard and TypeIs #16314

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

Closed
wants to merge 3 commits into from

Conversation

InSyncWithFoo
Copy link
Contributor

@InSyncWithFoo InSyncWithFoo commented Feb 22, 2025

Summary

Part of #13694.

This PR's description is currently empty. See this comment for a list of unresolved major issues.

Test Plan

Markdown tests.

@AlexWaygood AlexWaygood added the ty Multi-file analysis & type inference label Feb 22, 2025
}

#[salsa::interned]
pub struct TypeGuardType<'db> {
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the two types are exactly identical except for the name. Could we use a shared type (similar to Class) or make this type only thin wrapper types around a shared type? It would avoid the need for a macro

Copy link
Contributor Author

@InSyncWithFoo InSyncWithFoo Feb 22, 2025

Choose a reason for hiding this comment

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

I started with that, but the various match/case function ended up quite unreadable, not to mention the subtle differences (e.g., TypeGuard[] is covariant, but TypeIs[] is invariant). All in all, I think the current approach is the best, despite being a little bit non-DRY.

@InSyncWithFoo
Copy link
Contributor Author

This PR is incomplete (hence the empty description). Some major issues (all reflected in tests):

  • Invalid definitions are supposed to be reported.
    I thought the logic could be added to .infer_function_definition(), but calling .infer_type_expression()/.infer_type_expression_no_store() there (on returns) leads to panic.

  • if b where b is a bound guard is not recognized correctly.
    .evaluate_expr_name() needs some changes. However, I'm not sure if this approach scales, since b might not be a name.

  • TypeGuard[]-based constraints should overrule all other constraints
    I have yet to find out a way to do this. It seems that constraints are considered always invertible and compatible with one another.

# error: [invalid-type-guard-definition]
def _(**kwargs) -> TypeIs[str]: ...

class _:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's possible or reasonable here but I found many very specific mdtests much more helpful when debugging regression than a few tests that have many unrelated assertions.

There are also fewer trade-offs when using smaller tests compared to Ruff because it doesn't require you to create a new file. A single header is enough to create an isolated test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't all of these about function arity? I think this test is pretty small already, unless you mean the second file below (which is for subtype relation in TypeIs functions)?

@MichaReiser
Copy link
Member

I think this PR has become stale or do you plan on continue working on this PR?

@InSyncWithFoo
Copy link
Contributor Author

@MichaReiser I'm awaiting @carljm's reply at #117. In the meanwhile, I think I can at least split the working TypeIs parts into a separate PR; I'll do that next week.

@MichaReiser
Copy link
Member

Thanks for the update :) I just went through some older PRs and it wasn't clear to me what we're waiting on for this one.

@carljm
Copy link
Contributor

carljm commented May 22, 2025

@InSyncWithFoo sorry I lost track of that! Replied now, let me know if you have other questions. Thanks for your work on this, and sorry we let it get stale!

@InSyncWithFoo
Copy link
Contributor Author

Opened #18294 as a replacement for this one, since rebasing this would take too much effort.

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.

4 participants