Skip to content
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

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

gabritto
Copy link
Member

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 calling resolveSignature, 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.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 12, 2024
@gabritto
Copy link
Member Author

gabritto commented Mar 12, 2024

Self-check is failing because of this change.

@RyanCavanaugh
Copy link
Member

@typescript-bot cherry-pick this to release-5.4

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 12, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.4 ✅ Started ❌ Results

@typescript-bot
Copy link
Collaborator

Hey, @RyanCavanaugh! I was unable to cherry-pick this PR.

Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/8254939580

@RyanCavanaugh RyanCavanaugh merged commit b009837 into main Mar 15, 2024
19 checks passed
@RyanCavanaugh RyanCavanaugh deleted the gabritto/revert57358 branch March 15, 2024 18:59
@jakebailey
Copy link
Member

This will need to be manually cherry-picked; not sure if someone's done that already.

@lazytype
Copy link

lazytype commented Mar 20, 2024

But in the future, we might come up with a fix for the underlying problem and we can then bring this change back.

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.

  • TS 5.4 comes out
switch
...
default:
-  assertNever(foo.type);  // foo is never now so this isn't allowed
+  assertNever(foo);
  • This PR gets is released
switch
...
default:
-  assertNever(foo);
+  assertNever(foo.type);  // have to change it back
  • Future fix
switch
...
default:
-  assertNever(foo.type);
+  assertNever(foo);  // have to change it back again

@jakebailey
Copy link
Member

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.

@niels-vdz
Copy link

This will need to be manually cherry-picked; not sure if someone's done that already.

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?

@jakebailey
Copy link
Member

This was definitely included in 5.4.3: #57795

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS7022 circularity regression in 5.4.2 Narrowed type not "carried" iteratively
7 participants