Remove IDE heuristic for IsCompilationEndAnalyzer by mavasani · Pull Request #48762 · dotnet/roslyn

mavasani · 2020-10-19T14:56:43Z

Addresses the core functionality part of #47754 (comment).

A follow-up change will address the second part of generating an IDE-only diagnostic when an analyzer reports a diagnostic in CompilationEnd action which does not have the CompilationEnd custom tag.

…i/roslyn into IsCompilationEndAnalyzer

…nalyzer

sharwell · 2020-10-19T18:49:26Z

@mavasani Can you fill in this table?

	IDE Warning without FSA	IDE Warning with FSA
Analyzer without custom tag	❓	❓
Analyzer with custom tag	❓	❓

mavasani · 2020-10-19T19:03:26Z

@sharwell That is not in scope of this PR - this PR just changes the existing logic for determining if a diagnostic reported from build should be treated as a build-only/compilation-end diagnostic or not. Before this PR, that determination was done by an expensive, fragile heuristic that involves invoking callbacks into the analyzer. After this PR, that determination is done via an explicit custom tag on the diagnostic descriptors registered by the analyzer.

mavasani · 2020-10-19T19:09:57Z

To clarify my comment further - FSA setting is not being used to determine if a build diagnostic determined to be a compilation-end diagnostic should be treated as an intellisense supported diagnostic or not (probably it should be). However, those semantics are not being changed in this PR, so changing anything w.r.t. FSA setting is completely orthogonal to any work being done to resolve #47754

sharwell · 2020-10-19T19:14:02Z

@mavasani That's fine (and would be fine to indicate in the table what the follow-up behavior will be). I'm mostly trying to understand the impact this will have on users in each of the four states listed above. For each of the four states, the following could be impacted:

Behavior after a new solution/file is opened for editing (no prior build or analysis)
Behavior when Run Code Analysis is invoked in the IDE
Behavior when a file is opened after a build reported compilation end diagnostics
Behavior when an analyzer reports diagnostics from normal callbacks and separately registers a compilation end callback

mavasani · 2020-10-20T18:58:45Z

Filed #48793 to track investigating and fixing any build/live de-duping in presence of FSA being on/off. That option is unrelated to this PR and #47754 in general.

mavasani · 2020-10-22T14:29:10Z

@sharwell I performed bunch of validations and hopefully the results below should give us much stronger confidence in the experience:

Current Experience

FSA enabled: Compilation end diagnostics behave just like regular non-compilation end diagnostics. They are converted from build diagnostics to intellisense diagnostics during build/live de-duping and get properly refreshed by background analysis while editing code.
FSA disabled: Compilation end diagnostics are build-only diagnostics, that only get refreshed on explicit build.

New Experience

Analyzer package with custom tag

Exactly identical to Current Experience above.

Analyzer package without custom tag

FSA enabled: Exactly same as Current Experience above.
FSA disabled: Compilation end diagnostics are reported on build, but only remain in the error list for closed files. As soon as file is opened, these diagnostics disappear from the error list in "Build + Intellisense" setting. The diagnostics still appear in error list when switched to "Build only" setting.

Run Code Analysis command Experience

This command only force completes analysis for the entire project, it does not override FSA setting. For example, if FSA is off, running this command will only force complete non-compilation end actions for the entire project. This is done specifically to ensure that the command plays nicely with background analysis and generates the same diagnostics as if user had opened every file in the project.

mavasani · 2020-10-22T14:35:37Z

src/Compilers/Core/Portable/Diagnostic/WellKnownDiagnosticTags.cs

+        /// Indicates that the diagnostic is a compilation end diagnostic reported
+        /// from a compilation end action.
+        /// </summary>
+        public const string CompilationEnd = nameof(CompilationEnd);


@dotnet/roslyn-compiler This is the only compiler change in this PR (along with the corresponding public API change in below file). Please see #47754 for more context.

Plan is to mark this obsolete (unused) if we ever re-implement the deduplication algorithm to not need it.

…end actions Required to account for IDE experience for compilation end diagnostics as a consequence of dotnet/roslyn#48762 As many security rules are compilation end diagnostics, I have made the new parameter `isReportedAtCompilationEnd` a required parameter for `SecurityHelpers.CreateDiagnosticDescriptor` helper.

cston

Compiler changes LGTM.

333fred

Compiler changes LGTM (commit 7)

...ditorFeatures/CSharpTest/Diagnostics/Suppression/RemoveUnnecessaryPragmaSuppressionsTests.cs

...res/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_BuildSynchronization.cs

sharwell

Looks good aside from the concern of a possible test gap

mavasani · 2020-10-23T01:00:58Z

Thanks @sharwell - I have added the requested test.

…nalyzer

ghost · 2020-10-23T02:05:13Z

Hello @mavasani!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

…end actions Required to account for IDE experience for compilation end diagnostics as a consequence of dotnet/roslyn#48762 As many security rules are compilation end diagnostics, I have made the new parameter `isReportedAtCompilationEnd` a required parameter for `SecurityHelpers.CreateDiagnosticDescriptor` helper.

mavasani added 5 commits September 16, 2020 19:18

WIP

a3ed615

WIP

9869d40

Merge branch 'IsCompilationEndAnalyzer' of https://github.com/mavasan…

ac54525

…i/roslyn into IsCompilationEndAnalyzer

Merge remote-tracking branch 'upstream/master' into IsCompilationEndA…

6971285

…nalyzer

Fix incorrectly resolved conflicts

8a76200

This comment has been minimized.

Sign in to view

Update public API file

5f2acb0

Fix test

e9ee729

mavasani mentioned this pull request Oct 20, 2020

Investigate and fix build/live de-duping w.r.t Full Solution Analysis setting #48793

Closed

mavasani marked this pull request as ready for review October 22, 2020 14:33

mavasani requested review from a team as code owners October 22, 2020 14:33

mavasani requested a review from sharwell October 22, 2020 14:33

mavasani commented Oct 22, 2020

View reviewed changes

mavasani added this to the 16.9.P2 milestone Oct 22, 2020

mavasani added the Area-Analyzers label Oct 22, 2020

mavasani mentioned this pull request Oct 22, 2020

Add CompilationEnd custom tag to diagnostics reported in compilation end actions dotnet/roslyn-analyzers#4355

Merged

cston approved these changes Oct 22, 2020

View reviewed changes

333fred approved these changes Oct 22, 2020

View reviewed changes

sharwell reviewed Oct 22, 2020

View reviewed changes

...ditorFeatures/CSharpTest/Diagnostics/Suppression/RemoveUnnecessaryPragmaSuppressionsTests.cs Show resolved Hide resolved

sharwell reviewed Oct 22, 2020

View reviewed changes

...res/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_BuildSynchronization.cs Outdated Show resolved Hide resolved

mavasani commented Oct 22, 2020

View reviewed changes

...res/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_BuildSynchronization.cs Outdated Show resolved Hide resolved

Revert unrelated change

585718a

sharwell approved these changes Oct 22, 2020

View reviewed changes

Add test for positive and negative case for CompilationEnd tag

2e66d46

mavasani added 2 commits October 22, 2020 18:24

Merge remote-tracking branch 'upstream/master' into IsCompilationEndA…

10c3485

…nalyzer

Merge remote-tracking branch 'upstream/master' into IsCompilationEndA…

47f18a5

…nalyzer

mavasani added the auto-merge label Oct 23, 2020

ghost approved these changes Oct 23, 2020

View reviewed changes

ghost merged commit 480acbd into dotnet:master Oct 23, 2020

ghost modified the milestones: 16.9.P2, Next Oct 23, 2020

mavasani deleted the IsCompilationEndAnalyzer branch October 23, 2020 14:44

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

This pull request was closed.

Conversation

mavasani commented Oct 19, 2020

Uh oh!

This comment has been minimized.

sharwell commented Oct 19, 2020

Uh oh!

mavasani commented Oct 19, 2020

Uh oh!

mavasani commented Oct 19, 2020

Uh oh!

sharwell commented Oct 19, 2020

Uh oh!

mavasani commented Oct 20, 2020

Uh oh!

mavasani commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Experience

New Experience

Analyzer package with custom tag

Analyzer package without custom tag

Run Code Analysis command Experience

Uh oh!

mavasani Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

cston left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

mavasani commented Oct 23, 2020

Uh oh!

ghost commented Oct 23, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mavasani commented Oct 22, 2020 •

edited

Loading

mavasani Oct 22, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.