-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Revert "Allow (non-assert) type predicates to narrow by discriminant" #57750
Conversation
Self-check is failing because of this change. |
@typescript-bot cherry-pick this to release-5.4 |
Hey, @RyanCavanaugh! I was unable to cherry-pick this PR. Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/8254939580 |
This will need to be manually cherry-picked; not sure if someone's done that already. |
Is there any chance that this fix happens before the next patch release of 5.4? It's somewhat thrashy to have to migrate code back and forth especially between just patch versions.
switch
...
default:
- assertNever(foo.type); // foo is never now so this isn't allowed
+ assertNever(foo);
switch
...
default:
- assertNever(foo);
+ assertNever(foo.type); // have to change it back
switch
...
default:
- assertNever(foo.type);
+ assertNever(foo); // have to change it back again |
No, a future fix would be in 5.5 or later, hence the nebulous "in the future, we might come up with a fix". We typically just revert anything that is nontrivial to fix for patch releases. I guess that's "thrashy", but TS 5.4's only been out for two weeks and reverting this is safest. |
It looks like this revert was not included in v5.4.3 that was just released. Will this revert be included in the next patch release? |
This was definitely included in 5.4.3: #57795 |
Fixes #57690.
Fixes #57705.
The underlying problem that my original PR exposes seems to be that, when we try to narrow by type predicate, we call
getEffectsSignature
, which then in many cases results in callingresolveSignature
, which results in type checking something we're already in the middle of type checking, resulting in circularities.I think there isn't an immediate fix that would work for all of the reported examples or all possible new circularity cases, so I'm reverting the change for now. But in the future, we might come up with a fix for the underlying problem and we can then bring this change back.