Fix issue where analyzers are missing when computing diagnostics OOP #79821
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasonmalinowski
commented
Aug 7, 2025
src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs
Show resolved
Hide resolved
jasonmalinowski
force-pushed
the
fix-analyzers-missing-in-project-analyzers
branch
from
5c027e3 to
5dbd4e2
Compare
dotnet-policy-service
bot
added
Needs UX Triage
Needs API Review
jasonmalinowski
removed
Needs UX Triage
Needs API Review
jasonmalinowski
force-pushed
the
fix-analyzers-missing-in-project-analyzers
branch
2 times, most recently
from
f231f34 to
50cb458
Compare
If you were to have the same analyzer added as both a host analyzer and a project analyzer, we might end up removing it from the "project analyzer" set when we create the project CompilationWithAnalyzers. This confuses later code since it expects the analyzers to be present. This was regressed in c860af4, where a mechanical refactoring took us from having a single CompilationWithAnalyzers to two, one for project analyzers and one for host analyzers. The loop in the method was copied into two, but the set wasn't cleared between the two loops. I believe the intent was to clear out the set (rather than the set persist to deal with duplicates between the host and project sets), given the comment implies we do want duplicates to overwrite in the analyzerMap that we're also producing. I also split the two analyzer maps, one for project analyzers and one for host analyzers. If you don't do this, it means that a conflict where the same analyzer existing in both sets will overwrite each other, later code will then return a project analyzer which might be a different isntance than the host analyzer equivalent.
jasonmalinowski
force-pushed
the
fix-analyzers-missing-in-project-analyzers
branch
from
50cb458 to
3fcabbd
Compare
jasonmalinowski
commented
Aug 8, 2025
Comment on lines
+749
to
+751
| // In this case, the analyzers are ran twice. This appears to be a bug in SkippedHostAnalyzersInfo.Create, because it calls | ||
| // HostDiagnosticAnalyzers.CreateProjectDiagnosticAnalyzersPerReference which already filters out references, it doesn't return any | ||
| // references to skip. |
Member
Author
There was a problem hiding this comment.
It is entirely possible this is the "real" bug in this scenario. Fundamentally, the issue happening on user machines here is we have host-provided analyzers and project-provided analyzers that are overlapping in various ways, which eventually leads to crashes in the DiagnosticComputer. You can imagine fixing this two ways:
- We fix the logic for deduplication so that DiagnosticComputer never gets duplicates in any way.
- We fix DiagnosticComputer to deal with the duplicates.
I'm doing 2 in this PR. That's the approach I was following until writing this test, and then I had to debug what's going on here, so at this point I'm biased towards that! But for 17.14, I suspect that's the right thing to do, since:
- DiagnosticComputer has comments and code trying to deal with the duplicates (which makes me think it is expected to treat that as valid input.)
- The changes there to fix the duplicate handling appear to be straightforward changes to the code and addressing simple oversights, which is higher confidence than trying to ensure it never gets invalid inputs in the first place.
| { | ||
| projectBuilder.Add(analyzer); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
aside. i kinda wish these two loops were just two calls to a helper function.
CyrusNajmabadi
approved these changes
Aug 8, 2025
CyrusNajmabadi
approved these changes
Aug 8, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
CyrusNajmabadi
approved these changes
Aug 9, 2025
Member
Author
|
/backport to release/dev17.14 |
Contributor
|
Started backporting to release/dev17.14: https://github.com/dotnet/roslyn/actions/runs/16843503072 |
Merged
2 tasks
AbhitejJohn
added a commit
that referenced
this pull request
Aug 12, 2025
…ng diagnostics OOP (#79861) Backport of #79821 to release/dev17.14 /cc @jasonmalinowski ## Customer Impact ## Regression - [ ] Yes - [ ] No [If yes, specify when the regression was introduced. Provide the PR or commit if known.] ## Testing [How was the fix verified? How was the issue missed previously? What tests were added?] ## Risk [High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
This was referenced Aug 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If you were to have the same analyzer added as both a host analyzer and a project analyzer, we might end up removing it from the "project analyzer" set when we create the project CompilationWithAnalyzers. This confuses later code since it expects the analyzers to be present.
This was regressed in c860af4, where a mechanical refactoring took us from having a single CompilationWithAnalyzers to two, one for project analyzers and one for host analyzers. The loop in the method was copied into two, but the set wasn't cleared between the two loops. I believe the intent was to clear out the set (rather than the set persist to deal with duplicates between the host and project sets), given the comment implies we do want duplicates to overwrite in the analyzerMap that we're also producing.
I also split the two analyzer maps, one for project analyzers and one for host analyzers. If you don't do this, it means that a conflict where the same analyzer existing in both sets will overwrite each other, later code will then return a project analyzer which might be a different instance than the host analyzer equivalent.
Fixes #79706
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2325694