Implement de-duplication of NuGet and VSIX analyzer execution and diagnostics #41687
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
…uGet and VSIX analyzers reporting same/overlapping diagnostic IDs - dotnet#18818 Verified all the added unit tests fail currently as both NuGet and VSIX analyzers always execute and no diagnostic filtering is done.
This was referenced Mar 6, 2020
sharwell
reviewed
Mar 17, 2020
| /// <summary> | ||
| /// Information about host analyzers that can be skipped for the given project ID. | ||
| /// </summary> | ||
| private readonly ConditionalWeakTable<ProjectId, ISkippedAnalyzersInfo> _skippedAnalyzersInfo; |
Contributor
There was a problem hiding this comment.
❔ Any reason to not use the same IReadOnlyList<AnalyzerReference> key that CodeFixService uses?
Contributor
Author
There was a problem hiding this comment.
We only have the project ID in the project removal callback when we clear this cache entry.
Contributor
There was a problem hiding this comment.
Wouldn't it clear itself if we use CWT?
sharwell
reviewed
Mar 17, 2020
src/Workspaces/Core/Portable/Diagnostics/ISkippedAnalyzersInfo.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Diagnostics/ISkippedAnalyzersInfo.cs
Outdated
Show resolved
Hide resolved
sharwell
reviewed
Mar 17, 2020
| var asyncToken = _asyncOperationListener.BeginAsyncOperation(nameof(AnalyzeInProcAsync)); | ||
| var _ = FireAndForgetReportAnalyzerPerformanceAsync(project, client, analysisResult, cancellationToken).CompletesAsyncOperation(asyncToken); | ||
|
|
||
| // get skipped analyzers info |
Contributor
There was a problem hiding this comment.
Comment is redundant
Suggested change
| // get skipped analyzers info |
mavasani
commented
Mar 18, 2020
| /// <summary> | ||
| /// Information about host analyzers that can be skipped for the given project analyzers. | ||
| /// </summary> | ||
| private readonly ConditionalWeakTable<IReadOnlyList<AnalyzerReference>, ISkippedAnalyzersInfo> _skippedAnalyzersInfo; |
mavasani
commented
Mar 18, 2020
| return _skippedAnalyzersInfo.GetValue(project.AnalyzerReferences, createValueCallback); | ||
| } | ||
|
|
||
| public ISkippedAnalyzersInfo GetOrCreateSkippedAnalyzersInfo(Project project, IEnumerable<AnalyzerReference> hostAnalyzers) |
Contributor
Author
There was a problem hiding this comment.
This API is used for computing this info in OOP, as we don't have HostDiagnosticAnalyzers available in OOP.
…e is an existing unit test which caught this issue.
sharwell
approved these changes
Mar 18, 2020
Contributor
Author
|
Thank you @sharwell |
ghost
merged commit
This was referenced Mar 18, 2020
This pull request was closed.
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.
Implements the design in #18818 for analyzers. De-duping for code actions will be implemented in a follow-up PR.
First commit implements the unit tests as per the design mentioned in #18818 - all the added unit tests failed after this commit
Second commit implements the above logic in the IDE diagnostic analyzer engine.