When deduplicating diagnostics, look at how it is rendered instead of its internal state#153794
When deduplicating diagnostics, look at how it is rendered instead of its internal state#153794estebank wants to merge 1 commit intorust-lang:mainfrom
Conversation
… its internal state If two diagnostics are created differently (like two different structured diagnostics, or one using the Diag APIs), their internal state might be slightly different (because of the replacement variables). Which mean that the compiler can emit two errors that are visually indistinguishable. Change the hashing logic to consider the post expansion text for messages and labels so that they get properly deduplicated. Currently only two errors are directly fixed by this, but there are some duplicated errors with slightly different contents that can be modified to look the same so they are considered duplicates.
rustbot
added
S-waiting-on-review
|
r? @jackh726 rustbot has assigned @jackh726. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
||
| #[derive(Diagnostic)] | ||
| #[diag("constant evaluation is taking a long time")] | ||
| #[note("number of steps elapsed: {$force_duplicate}")] |
There was a problem hiding this comment.
This number was not meant to be user-visible. Now that it is, we have to worry about what it represents. It counts how many ConstEvalCounter there have been... I don't actually know when and how those get emitted, and whether it is appropriate to call each of them one "step".
There was a problem hiding this comment.
I'm fine showing it to users, but we need to filter it in UI tests, otherwise a lot of random changes will keep changing these tests
There was a problem hiding this comment.
An option would be to add an explicit disambiguation field to Diag independent of the message variables.
There was a problem hiding this comment.
I could totally imagine adding some kind of way to force including a field in the hash, regardless of whether it's shown in the output. That seems better than outputting the number of steps. Or even, having a way for diagnostics to "force no deduplication".
But alas, I'm also okay to go this path, but as Oli said, we should hide them in ui tests.
All in all, it's probably easier to edit the diagnostic hashing than wire through something special for ui tests.
|
It seems like you also changed at least one diagnostic to actually change its output to include previously deliberately hidden details. Are there other such cases? I am surprised this is not mentioned in the PR description. |
This is indeed the only case I found in the entire test suite. I was going to try and find a separate alternative for that, specifically ensure that the first warning is actually emitted and the subsequent ones are silenced. |
|
☔ The latest upstream changes (presumably #154008) made this pull request unmergeable. Please resolve the merge conflicts. |
jackh726
added
S-waiting-on-author
5 participants
If two diagnostics are created differently (like two different structured diagnostics, or one using the Diag APIs), their internal state might be slightly different (because of the replacement variables). Which mean that the compiler can emit two errors that are visually indistinguishable. Change the hashing logic to consider the post expansion text for messages and labels so that they get properly deduplicated. Currently only two errors are directly fixed by this, but there are some duplicated errors with slightly different contents that can be modified to look the same so they are considered duplicates.