Update VSMEF001PropertyMustHaveSetter.cs #345
Conversation
| @@ -40,25 +40,25 @@ public class VSMEF001PropertyMustHaveSetter : DiagnosticAnalyzer | |||
| public override void Initialize(AnalysisContext context) | |||
| { | |||
| context.EnableConcurrentExecution(); | |||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.ReportDiagnostics); | |||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); | |||
There was a problem hiding this comment.
If the intent is to report diagnostics in generated code, then it should be Analyze | ReportDiagnostics
There was a problem hiding this comment.
This strikes me as a bug fix then, is that correct? Is the idea that without this extra flag, we won't report diagnostics on generated code, despite the existing flag we set? It seems odd that ReportDiagnostics would be broken if used by itself. Is there documentation or other bugs that I can research to learn more about this?
There was a problem hiding this comment.
@AArnott To my understanding, the presence of Analyze determines whether or not you get callbacks in generated code, and the presence of ReportDiagnostics determines whether or not a reported diagnostic in generated code will be suppressed.
If you want to treat generated code the same as non-generated code (which appears to be the original intent here), you specify both.
There are cases where it's useful to only use Analyze, for example, consider an analyzer that reports unused methods, and you don't want to report diagnostics in generated code.
Analyze | ReportDiagnostic: will not work because it reports diagnostics in generated code.None: will not work, a non-generated method may only be called in generated code. You'll report it as unused while it's used by generated code.Analyzegives the correct behavior. You don't report unused methods in generated code, but you get callbacks for generated code so that you know whether a non-generated method is used by generated code.
I don't know if ReportDiagnostics alone is ever useful.
There was a problem hiding this comment.
I don't know if ReportDiagnostics alone is ever useful.
This is the crux of my question. The flag isn't documented as broken without Analyze also specified. It seems unlikely or impossible to report diagnostics on code you haven't analyzed. So why would ReportDiagnostics not imply (or even bitwise-OR in) the Analyze flag as well?
@CyrusNajmabadi can you advise?
There was a problem hiding this comment.
I cannot. But @mavasani definitely can :)
There was a problem hiding this comment.
It seems unlikely or impossible to report diagnostics on code you haven't analyzed
You definitely can do so. For example, if you have a CompilationEnd analyzer that wants to analyze only non-generated code, but report diagnostics anywhere in the compilation (including in generated code), then specifying only ReportDiagnostics would be required. I agree this is not a common analyzer configuration, but is theoretically possible.
We introduced GeneratedCodeAnalysisFlags as a Flags enum to provide complete flexibility to analyzer authors on whether or not to receive callbacks and/or report diagnostics for generated code.
Codecov Report
@@ Coverage Diff @@
## main #345 +/- ##
=======================================
Coverage 76.17% 76.17%
=======================================
Files 110 110
Lines 7194 7194
=======================================
Hits 5480 5480
Misses 1714 1714
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Thanks for your submission. However without a bug filed or PR description that explains how, why, etc. of your change, it's very difficult to know why we should merge this PR.
The only meaningful change appears to be related to the flags. If that's the only functional change, all other irrelevant changes should be reverted and the PR description set to explain the problem and why this is the appropriate solution.
If you can update the PR to fit this, I'll be happy to take another look.
| @@ -14,7 +14,7 @@ namespace Microsoft.VisualStudio.Composition.Analyzers | |||
| /// Creates a diagnostic when `[Import]` is applied to a property with no setter. | |||
| /// </summary> | |||
| [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] | |||
| public class VSMEF001PropertyMustHaveSetter : DiagnosticAnalyzer | |||
| public sealed class VSMEF001PropertyMustHaveSetter : DiagnosticAnalyzer | |||
There was a problem hiding this comment.
Making this change to just one class deviates this class from the norm for this project.
Aside: sealing classes is way overdone, IMO.
There was a problem hiding this comment.
@AArnott I don't see any other analyzers in the repo. Can you point to them? I can update them all if you want (whether in this PR or a separate PR)
There was a problem hiding this comment.
Ah, my mistake then. Most of our analyzer projects have many analyzers. But we don't mark any of them as sealed. Unless there's a compelling reason to do so, please revert this change.
src/Microsoft.VisualStudio.Composition.Analyzers/VSMEF001PropertyMustHaveSetter.cs
Show resolved
Hide resolved
| @@ -40,25 +40,25 @@ public class VSMEF001PropertyMustHaveSetter : DiagnosticAnalyzer | |||
| public override void Initialize(AnalysisContext context) | |||
| { | |||
| context.EnableConcurrentExecution(); | |||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.ReportDiagnostics); | |||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); | |||
There was a problem hiding this comment.
This strikes me as a bug fix then, is that correct? Is the idea that without this extra flag, we won't report diagnostics on generated code, despite the existing flag we set? It seems odd that ReportDiagnostics would be broken if used by itself. Is there documentation or other bugs that I can research to learn more about this?
No description provided.