Port few IDE analyzers/fixers to shared layer to be enabled in CodeStyle NuGet package #41363

mavasani · 2020-02-03T14:30:10Z

NOTE: More than half the changed files in this PR are xlf files and majority of source file changes are just using resources from a shared resx file. Strongly recommend reviewing with GitHub filter feature to collapse all xlf file changes: Review Link

Following analyzers/fixers have been ported to the shared layer and will get enabled in CodeStyle NuGet package:

Remove unused private members (IDE0051): An analyzer without options.
C# make struct fields writable (IDE0064): Another simple analyzer without options.
C# convert switch statement to expression (IDE0066): An analyzer with a C# code style option. Also includes refactoring of options infrastructure and test framework code to support option based analyzers and corresponding unit tests.
C# use implicit or explicit type style analyzers (IDE0007 and IDE0008): Typical code style analyzers with bunch of utility types.

Remove unnecessary usings/imports (IDE0005): Analyzer which used some language services, and is very commonly requested analyzer for CI enforcement.
NOTE: This analyzer works on command line only if XML document comments are enabled for the project. This seems to be a compiler bug (Compiler does not report unused imports when XML doc comments are disabled #41640):

roslyn/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

Lines 2116 to 2118 in 41bc895

    
           internal override void ReportUnusedImports(SyntaxTree? filterTree, DiagnosticBag diagnostics, CancellationToken cancellationToken) 
        
           { 
        
               if (_lazyImportInfos != null && filterTree?.Options.DocumentationMode != DocumentationMode.None)

#41462 tracks follow-up items once this PR goes in.

…e layer

src/Analyzers/Core/CodeFixes/CodeFixes.shproj

sharwell · 2020-02-03T15:02:02Z

My main expectations for code layout in the new package is:

No new shared projects. Use linked files if/when necessary since accomplish the same goal but are easier to maintain in the IDE.
Tests for ported analyzers use the new analyzer testing library. This already works with .editorconfig with minimal configuration, as seen in FormattingAnalyzerTests.
No OptionSet. The ported analyzers operate directly against AnalyzerConfigOptions. This already works with formatting analyzer as an example.
MEF is not really an option for NuGet-installed analyzers. Even the code fixes are "exported", they are not constructed through MEF and don't have access to the export provider. The same limitations apply for refactorings.

src/EditorFeatures/Test/CodeActions/AbstractCodeActionOrUserDiagnosticTest.cs

mavasani · 2020-02-03T15:14:35Z

No new shared projects. Use linked files if/when necessary since accomplish the same goal but are easier to maintain in the IDE.

Lets talk at the design meeting today. Using linked files makes this an extremely terrible experience for shared analyzers/fixers/test files, and forces each analyzer author to know to add their new analyzer/fixer/tests to both set of projects (Features + CodeStyle) and also now needs additional effort of understanding project structure in code style layer. All of this goes away with shared projects where you just add a file and it light ups wherever required.

Tests for ported analyzers use the new analyzer testing library. This already works with .editorconfig with minimal configuration, as seen in FormattingAnalyzerTests.

I think we really need to be practical here instead of idealistic. Forcing this requirement to port every analyzer/fi is putting tremendous extra work on initial porting effort and unnecessary delaying the overall port effort and customer value add. With my proposed change, porting each set of analyzer + fixer + tests can be done by simple file moves and some edits. Rewriting all tests to a different test framework should be done as a follow-up item so we don't end up again blocking this feature work, which has happened numerous times in the past because of our desire to be idealistic here.

No OptionSet. The ported analyzers operate directly against AnalyzerConfigOptions. This already works with formatting analyzer as an example.

Current ports use AnalyzerConfigOptions for analyzer/fixer in CodeStyle layer but still use the OptionSet for the implementation in Features layer to avoid any potential regressions. We can discuss this at the design meeting how we want to proceed here.

MEF is not really an option for NuGet-installed analyzers. Even the code fixes are "exported", they are not constructed through MEF and don't have access to the export provider. The same limitations apply for refactorings.

I don't understand this - the PR already refactors the MEF parts of code so the existing mechanism of document.GetLanguageService works in the code fixes in CodeStyle layer. I don't understand your concern here.

…ndFixUtilities

CyrusNajmabadi · 2020-02-03T20:24:08Z

Somewhat concerning to see a port add 6000 new lines :-/ Is there a reason for that much addition?

...Sharp/Analyzers/RemoveUnnecessaryImports/CSharpRemoveUnnecessaryImportsDiagnosticAnalyzer.cs

...nalyzers/CSharp/Analyzers/UseImplicitOrExplicitType/CSharpTypeStyleDiagnosticAnalyzerBase.cs

...ore/Analyzers/RemoveUnnecessaryImports/AbstractRemoveUnnecessaryImportsDiagnosticAnalyzer.cs

src/CodeStyle/BannedSymbols.txt

src/CodeStyle/CSharp/Analyzers/xlf/CSharpCodeStyleResources.cs.xlf

src/CodeStyle/Core/CodeFixes/FormattingCodeFixProvider.cs

mavasani · 2020-02-03T20:32:02Z

Somewhat concerning to see a port add 6000 new lines

Almost all of those are coming from splitting our workspace layer extension method files into 2 separate files: workspace dependent and independent, with the latter being usable from analyzers. Unfortunately, GitHub sees this as new code addition, and also puts lot of merge pain on this branch :)

...cModelWorkspaceService/SemanticModelWorkspaceServiceFactory.SemanticModelWorkspaceService.cs

src/CodeStyle/Core/CodeFixes/xlf/CodeStyleFixesResources.pt-BR.xlf

CyrusNajmabadi · 2020-02-03T20:33:23Z

oh, this is a draft PR. bleagh. sorry.

in terms of the full PR, if we can make it simpler to review, that would be excellent. thanks! :)

CyrusNajmabadi · 2020-02-03T20:35:43Z

Unfortunately, GitHub sees this as new code addition, and also puts lot of merge pain on this branch :)

Yeah. i would be ok with this going in as a two or three PR merge. specifically: one PR is just the real code changes. It would not be expected to pass CI. The other PR is just the loc changes. The third is the merge of the other two. It would pass, but wouldn't really need reviewing.

Note: i did the same when doing my regex PR as my log changes absolutely crippled github and made it so hard to review.

…ndFixUtilities

mavasani · 2020-02-12T20:52:45Z

...ditorFeatures/DiagnosticsTestUtilities/CodeActions/AbstractCodeActionOrUserDiagnosticTest.cs

@@ -92,11 +100,134 @@ public TestParameters WithIncludeDiagnosticsOutsideSelection(bool includeDiagnos
                 ? TestWorkspace.Create(initialMarkup, openDocuments: false)
                 : CreateWorkspaceFromFile(initialMarkup, parameters);

+            // For CodeStyle layer testing, we create an .editorconfig at project root


This is the primary test IDE analyzer/code fix test framework file that has been changed to enable supporting Code Style layer unit tests for CodeStyle analyzers/fixes.

mavasani · 2020-02-12T20:53:31Z

...ditorFeatures/DiagnosticsTestUtilities/CodeActions/AbstractCodeActionOrUserDiagnosticTest.cs

@@ -199,138 +330,6 @@ protected Task TestSmartTagGlyphTagsAsync(string initialMarkup, ImmutableArray<s
            }
        }

-        protected async Task TestAddDocumentInRegularAndScriptAsync(


Split into a separate partial file which is not included in CodeStyle test utilities project.

mavasani · 2020-02-12T20:54:09Z

...agnosticsTestUtilities/CodeActions/AbstractCodeActionOrUserDiagnosticTest_TestAddDocument.cs

+
+namespace Microsoft.CodeAnalysis.Editor.UnitTests.CodeActions
+{
+    public abstract partial class AbstractCodeActionOrUserDiagnosticTest


Existing code just moved into a separate partial declaration.

mavasani · 2020-02-12T20:54:46Z

...es/DiagnosticsTestUtilities/Diagnostics/AbstractDiagnosticProviderBasedUserDiagnosticTest.cs

@@ -224,56 +227,5 @@ private void AssertNoAnalyzerExceptionDiagnostics(IEnumerable<Diagnostic> diagno
            var analyzerExceptionDiagnostics = diagnostics.Where(diag => diag.Descriptor.CustomTags.Contains(WellKnownDiagnosticTags.AnalyzerException));
            AssertEx.Empty(analyzerExceptionDiagnostics, "Found analyzer exception diagnostics");
        }
-
-        #region Parentheses options


Moved into a separate partial file which is not included in CodeStyle test utilities project (yet).

mavasani · 2020-02-12T20:55:47Z

src/EditorFeatures/DiagnosticsTestUtilities/Diagnostics/AbstractUserDiagnosticTest.cs

@@ -293,158 +292,5 @@ protected Document GetDocumentAndAnnotatedSpan(TestWorkspace workspace, out stri

            Assert.True(expectedTextSpans.SetEquals(actualTextSpans));
        }
-
-        internal async Task TestWithMockedGenerateTypeDialog(


Moved into a separate partial declaration.

…ndFixUtilities

Extracted from dotnet#41363

333fred

Compiler.sln change lgtm. Did not look at other changes.

Extracted from dotnet#41363 Third follow-up item from dotnet#41462 Apart from adding resx files, changes also include: 1. Moving resource strings duplicated across Workspaces and CodeStyle layer into the shared resx files 2. Source file changes to use the resource strings from the shared resx. We should no longer require use of `#if CODE_STYLE` in the shared layer for the purpose of resource strings.

mavasani · 2020-02-20T15:18:53Z

Closing this big PR in favor of a smaller PR: #41815

Extracted out of dotnet#41363 so it only contains the following changes: 1. Port analyzer/fixer/test files for ConvertSwitchStatementToExpression to shared layer 2. Options related namespace changes (see comment dotnet#41363 (comment) for details) 3. MEF based language service discovery in CodeStyle layer. dotnet#41462 tracks replacing this with a non-MEF based approach. 4. Minimal resx/xlf file changes - few were needed as the resources used by analyzer/fixer were moved to shared resx file. 5. Enable .editorconfig based unit test support for CodeStyle layer. NOTE: First commit dotnet@b006324 just ports changes from dotnet#40513 which are required to enable this support. This commit should become redundant once that PR is merged in.

mavasani added 5 commits January 30, 2020 21:16

Analyzer and code fixes refactoring

f18cea7

Port CSharpMakeStructFieldsWritableDiagnosticAnalyzer and fixer

c3ecec5

Port ConvertSwitchStatementToExpression to CodeStyle layer

2e32204

Port C# type style analyzers (UseImplicitOrExplicitType) to code styl…

df116e9

…e layer

Port RemoveUnnecessaryImports analyzer/fixer to code style layer

a6579cc

sharwell reviewed Feb 3, 2020

View reviewed changes

src/Analyzers/Core/CodeFixes/CodeFixes.shproj Outdated Show resolved Hide resolved

sharwell reviewed Feb 3, 2020

View reviewed changes

src/EditorFeatures/Test/CodeActions/AbstractCodeActionOrUserDiagnosticTest.cs Outdated Show resolved Hide resolved

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

2750a86

…ndFixUtilities