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

Always resetErrorInfo if structuredTypeRelatedTo succeeds #49718

Merged
merged 3 commits into from Jul 6, 2022

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jun 28, 2022

Fixes #49475

If typeRelatedToDiscriminatedType succeeds, then we have to clear the errors from previous steps as they are no longer true. Most of the previous steps do this too (but not all, confusingly).

This manifested as being "uncomment this code, and the error goes away" because the call expression's call to isRelatedTo had reportErrors=false, while the class' call had reportErrors=true. The class check is deferred, so the call expression happened first, and the successful relationship was cached, then the error didn't appear later. Without the call expression, the error was reported even though it shouldn't have been.

Rather than remembering to place those resetErrorInfo calls everywhere, turn the function into a worker and always reset if the relation succeeded.

@jakebailey
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline

@typescript-bot
Copy link
Collaborator

Heya @jakebailey, I'm starting to run the diff-based user code test suite on this PR at b77d3e9. Hold tight - I'll update this comment with the log link once the build has been queued.

@typescript-bot
Copy link
Collaborator

Heya @jakebailey, I'm starting to run the extended test suite on this PR at b77d3e9. Hold tight - I'll update this comment with the log link once the build has been queued.

@RyanCavanaugh
Copy link
Member

I guess my question is whether there are other "missing" calls in this function, e.g. at https://github.com/microsoft/TypeScript/pull/49718/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8L19836

@jakebailey
Copy link
Member Author

I totally agree, and so I made the change to wrap structuredTypeRelatedTo and always restore on failure, but that seemed to break things in confusing ways. I'll go post that as a separate PR based on this one to show what I mean.

@typescript-bot
Copy link
Collaborator

Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

@jakebailey
Copy link
Member Author

All of the differences were my mistake; I'll push the change here. But, I don't have any more tests for cases I'm missing. At least it'll feel like it won't be broken :D

@jakebailey jakebailey changed the title Reset relation error info if typeRelatedToDiscriminatedType succeeds Always resetErrorInfo if structuredTypeRelatedTo succeeds Jun 28, 2022
return result;
}

function structuredTypeRelatedToWorker(source: Type, target: Type, reportErrors: boolean, intersectionState: IntersectionState, saveErrorInfo: ReturnType<typeof captureErrorCalculationState>): Ternary {
Copy link
Member Author

Choose a reason for hiding this comment

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

saveErrorInfo is kept because there are still some calls which explicitly reset it in order to hide certain errors that would be less helpful than others.

PR Backlog automation moved this from Not started to Needs merge Jul 6, 2022
@jakebailey jakebailey merged commit af70f24 into microsoft:main Jul 6, 2022
PR Backlog automation moved this from Needs merge to Done Jul 6, 2022
@jakebailey jakebailey deleted the fix-49475 branch July 6, 2022 19:57
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
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Discriminated unions behave inconsistently since v4.6
4 participants