-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Improve diagnostics deduplication #58220
Conversation
@typescript-bot test it |
@gabritto Here are the results of running the user tests comparing Everything looks good! |
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@gabritto Here are the results of running the top 400 repos comparing Everything looks good! |
@typescript-bot run dt |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@@ -304,7 +305,7 @@ export function isExternalModuleNameRelative(moduleName: string): boolean { | |||
} | |||
|
|||
export function sortAndDeduplicateDiagnostics<T extends Diagnostic>(diagnostics: readonly T[]): SortedReadonlyArray<T> { | |||
return sortAndDeduplicate<T>(diagnostics, compareDiagnostics); | |||
return sortAndDeduplicate<T>(diagnostics, compareDiagnostics, diagnosticsEqualityComparer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, DiagnosticCollection
's .add
method is still using compareDiagnosticsSkipRelatedInformation
, so it's just first-in-wins (with the same root message) - should that also get updated to prefer the "lesser" message like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compareDiagnosticsSkipRelatedInformation
is still the same in terms of what it considers equal - two diagnostics with the same location and head message but different elaboration will still be considered different by that function, and so both will be added via DiagnosticCollection.add
. Only later, when we call sortAndDeduplicateDiagnostics
, will we use diagnosticsEqualityComparer
and get rid of all but one of the diagnostics that we now consider equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it, if we want to keep using functions like sortAndDeduplicate
and insertSorted
, I think we need to have two separate functions for comparison and equality: one that fully compares diagnostics for purposes of sorting (and sorting a diagnostic with more elaboration before one with less), and another for purposes of deduplication that is more permissive and only compares location and head message for equality. Otherwise we'd have to implement a function that somehow says "yes, those two diagnostics are equal, but one of them is preferred".
Fixes #58207.
Before, we only considered two diagnostics the same if everything about them was equal, including the message chain and related information. With this PR, we now consider diagnostics at the same location and with the same head message as being the same, regardless of the rest of the message chain and related information.
That is necessary because, as the new test shows, we can generate the "same" diagnostic at different moments in type checking, and they will have different elaboration (different message chains and related information).
This PR also makes it so deduplication keeps the diagnostic with more elaboration between two diagnostics considered to be equal.
Note that this doesn't completely solve the issue, because not all diagnostics that should be considered equal have the same code and head message.
Edit: follow-up PR #58318 fixes this.