Improve diagnostics deduplication #58220
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@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.
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.
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.
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.