[red-knot] Support TypeGuard and TypeIs#16314
[red-knot] Support TypeGuard and TypeIs#16314InSyncWithFoo wants to merge 3 commits intoastral-sh:mainfrom
TypeGuard and TypeIs#16314Conversation
| } | ||
|
|
||
| #[salsa::interned] | ||
| pub struct TypeGuardType<'db> { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
This PR is incomplete (hence the empty description). Some major issues (all reflected in tests):
|
| # error: [invalid-type-guard-definition] | ||
| def _(**kwargs) -> TypeIs[str]: ... | ||
|
|
||
| class _: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
InSyncWithFoo
force-pushed
the
rk-type-guards
branch
from
1f02c40 to
a6e76bc
Compare
InSyncWithFoo
force-pushed
the
rk-type-guards
branch
from
a6e76bc to
ade4c79
Compare
|
I think this PR has become stale or do you plan on continue working on this PR? |
|
@MichaReiser I'm awaiting @carljm's reply at #117. In the meanwhile, I think I can at least split the working |
|
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. |
|
@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! |
|
Opened #18294 as a replacement for this one, since rebasing this would take too much effort. |
4 participants
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.