Refactor extensions and utilities in Workspaces layer into shared pro… #41510

mavasani · 2020-02-08T05:25:08Z

…jects

Extracts out the changes to core Workspace layer from #41363.

Changes that were undone from #41363 for easier review:

Shared projects for analyzer, code fixes and unit tests
Porting of IDE analyzers, code fixes and unit tests to CodeStyle layer
All the changes related to Options
All the test framework changes
Most of the changes in CodeStyle layer.
Changes to most of the resource files. I still had to retain changes add some resources in CodeStyle layers. You can exclude these resx and xlf files from your review by using GitHub's file filter: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/filtering-files-in-a-pull-request

…jects Extracts out the changes to core Workspace changes from dotnet#41363.

mavasani · 2020-02-08T05:27:19Z

@CyrusNajmabadi I believe this PR should be much easier to review. Even though large number of files are touched, most of them are moves or file splits. Breaking it down further is just too much pain and needs almost to redo the entire work step by step from scratch, which is not worth.

CyrusNajmabadi · 2020-02-08T09:26:49Z

src/CodeStyle/Core/CodeFixes/Host/Mef/CodeStyleHostLanguageServices.MefHostExportProvider.cs

+            }
+        }
+    }
+}


skipping this file. i will leave to mef people to look at :)

Yes, this is most likely a temporary approach for discovering language services in CodeStyle layer. #41462 tracks removal of MEF, and likely would need @jasonmalinowski or @sharwell to figure out a good non-MEF based solution to implement it.

...Workspace/Core/LanguageServices/GeneratedCodeRecognition/IGeneratedCodeRecognitionService.cs

...ensions/Workspace/VisualBasic/LanguageServices/VisualBasicRemoveUnnecessaryImportsService.vb

…ith dotnet#41363

src/CodeStyle/CSharp/Analyzers/Microsoft.CodeAnalysis.CSharp.CodeStyle.csproj

src/CodeStyle/Core/CodeFixes/Host/Mef/CodeStyleHostLanguageServices.MefHostExportProvider.cs

src/Features/CSharp/Portable/AddImport/CSharpAddImportFeatureService.cs

CyrusNajmabadi · 2020-02-11T00:24:22Z

Will try to review this tonight (unless you'd like me to hold off). LMK what your pref is.

mavasani · 2020-02-11T00:06:14Z

src/CodeStyle/Core/Analyzers/Microsoft.CodeAnalysis.CodeStyle.csproj

    <InternalsVisibleTo Include="Microsoft.CodeAnalysis.VisualBasic.CodeStyle.UnitTests" />
  </ItemGroup>
  <ItemGroup>
-    <Compile Include="..\..\..\Compilers\Core\Portable\InternalUtilities\Debug.cs" Link="Formatting\Debug.cs" />


All these linked files moved into shared project now.

mavasani · 2020-02-11T00:06:39Z

src/CodeStyle/Core/Analyzers/Microsoft.CodeAnalysis.CodeStyle.csproj

    <Compile Include="..\..\..\Workspaces\Core\Portable\Shared\Collections\IIntervalIntrospector.cs" Link="Formatting\Context\IIntervalIntrospector.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\Shared\Collections\IntervalTree`1.cs" Link="Formatting\Context\IntervalTree`1.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\Shared\Collections\IntervalTree`1.Node.cs" Link="Formatting\Context\IntervalTree`1.Node.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\Shared\Collections\SimpleIntervalTree.cs" Link="Formatting\SimpleIntervalTree.cs" />


There is scope to move more utilities down to the shared layer in future.

mavasani · 2020-02-11T00:08:50Z

src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs


            var solution = await fixAllService.GetFixAllChangedSolutionAsync(
-                fixCollection.FixAllState.CreateFixAllContext(progressTracker, cancellationToken)).ConfigureAwait(false);
+                new FixAllContext(fixCollection.FixAllState, progressTracker, cancellationToken)).ConfigureAwait(false);


CreateFixAllContext was inlined here as the FixAllState.cs has been moved to the shared layer, but creating a new FixAllContext is only required from IDE code fix service engine and test layer.

src/Features/CSharp/Portable/AddImport/CSharpAddImportFeatureService.cs

src/Features/CSharp/Portable/CodeFixes/Nullable/CSharpDeclareAsNullableCodeFixProvider.cs

mavasani · 2020-02-11T00:45:26Z

...ces/SharedUtilitiesAndExtensions/Workspace/VisualBasic/Extensions/SemanticModelExtensions.vb

+Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
+    Partial Friend Module SemanticModelExtensions
+
+        Private Const s_defaultParameterName = "p"


All existing code that was just moved as is.

mavasani · 2020-02-11T00:46:16Z

...ensions/Workspace/VisualBasic/LanguageServices/VisualBasicRemoveUnnecessaryImportsService.vb

+            End Get
+        End Property
+
+        Public Overrides Async Function RemoveUnnecessaryImportsAsync(


Existing code that was just moved.

mavasani · 2020-02-11T00:46:40Z

...edUtilitiesAndExtensions/Workspace/VisualBasic/Utilities/VisualBasicSimplificationHelpers.vb

+Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
+
+Namespace Microsoft.CodeAnalysis.VisualBasic.Simplification
+    Friend Module VisualBasicSimplificationHelpers


Existing code extracted out into a shared module.

mavasani · 2020-02-11T00:47:29Z

src/Workspaces/VisualBasic/Portable/Extensions/SyntaxNodeExtensions_SharedWithCodeStyle.vb

-Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
-
-Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
-    Partial Friend Module SyntaxNodeExtensions


Moved into shared layer in existing extension method file

mavasani · 2020-02-11T00:48:06Z

src/Workspaces/VisualBasic/Portable/Simplification/VisualBasicSimplificationService.vb

@@ -61,29 +61,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Simplification
            End Using
        End Function

-        Public Shared Function TryEscapeIdentifierToken(identifierToken As SyntaxToken) As SyntaxToken


Moved into extension method file.

mavasani · 2020-02-11T00:49:53Z

Will try to review this tonight (unless you'd like me to hold off). LMK what your pref is.

Thanks, that would be great! I will try my best to see what all pieces can be threaded out. Hoping that once this PR merged in, it allows further refactoring like your PRs to move further code down and reduces chances of merge conflicts.

mavasani · 2020-02-11T00:51:49Z

src/Compilers/Core/Portable/InternalUtilities/EncodingExtensions.cs

            }

-#if WORKSPACE
+#if CODE_STYLE


@dotnet/roslyn-compiler The only changes to compiler layer are in 3 files: this one and the 2 that follow. The changes are just adding a new #if CODE_STYLE for these utility types that are now also included in the CodeStyle layer.

jaredpar

jaredpar · 2020-02-11T05:45:21Z

Approved the three changes to the Compiler layer

tmat

Assuming this is just a subset of he original PR, which LGTM.

mavasani · 2020-02-11T18:18:45Z

Assuming this is just a subset of he original PR, which LGTM.

Yes, it just extracts out the Workspace related pieces from that PR.

src/Workspaces/Core/Portable/Editing/ImportAdderService.cs

src/Workspaces/Core/Portable/xlf/WorkspacesResources.cs.xlf

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ICompilationExtensions.cs

...es/SharedUtilitiesAndExtensions/Compiler/VisualBasic/VisualBasicCompilerExtensions.projitems

...ndExtensions/Workspace/CSharp/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

...e/Portable/Completion/Providers/ImportCompletionProvider/AbstractImportCompletionProvider.cs

...iesAndExtensions/Workspace/Core/CodeFixes/SyntaxEditorBasedCodeFixProvider.FixAllProvider.cs

...CSharp/Portable/RemoveUnnecessaryImports/CSharpRemoveUnnecessaryImportsDiagnosticAnalyzer.cs

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

...Core/Portable/RemoveUnnecessaryImports/AbstractRemoveUnnecessaryImportsDiagnosticAnalyzer.cs

CyrusNajmabadi · 2020-02-11T19:19:10Z

...c/Portable/RemoveUnnecessaryImports/VisualBasicRemoveUnnecessaryImportsDiagnosticAnalyzer.vb

        Protected Overrides Function MergeImports(unnecessaryImports As ImmutableArray(Of SyntaxNode)) As ImmutableArray(Of SyntaxNode)
            Dim result = ArrayBuilder(Of SyntaxNode).GetInstance()
-            Dim importsClauses = unnecessaryImports.CastArray(Of ImportsClauseSyntax)
+            Dim importsClauses = unnecessaryImports.ToImmutableHashSet()


undo if not necessary.

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/BatchFixAllProvider.cs

...tilitiesAndExtensions/Workspace/VisualBasic/LanguageServices/VisualBasicAddImportsService.vb

CyrusNajmabadi · 2020-02-11T19:28:02Z

So overall i skimmed. files that were pure-add/pure-delete were skipped as i assume those represent the extension-method moves. I'm a littel wary of how much i had to skip as there might have been relevant code in there tat i just didn't see.

However, overall, this seems like a very safe, very reasonable change to make. i highly doubt anything bad is sneaking in. And, if there is any issues hwere, i think we're likely to discover it immediately.

I would ask that we file some issues to ensure proper cleanup happens in future PRs coming in the next couple of weeks. Also, i left a few final comments on things we might want to look into in this PR.

CyrusNajmabadi

Overall this is good enough that i feel ok taking as is.

My personal feedback though is that in the future we should try harder up front to break up these changes into more manageable pieces. There are still several changes here that i think would have been better to break out just to build confidence and assurance that the big changes weren't potentially doing anything that required review with a fine-toothed comb.

Thanks!

mavasani · 2020-02-11T23:16:04Z

Sync'ed with @sharwell offline, and he is fine with going ahead with this merge. Lets get to the follow-up items soon.

Refactor extensions and utilities in Workspaces layer into shared pro…

1d8444d

…jects Extracts out the changes to core Workspace changes from dotnet#41363.

CyrusNajmabadi reviewed Feb 8, 2020

View reviewed changes

This was referenced Feb 8, 2020

Move static helper methods into extensions. #41511

Merged

Cleanup fix all code. #41512

Merged

Remove unused parameters. #41513

Merged

CyrusNajmabadi reviewed Feb 8, 2020

View reviewed changes

...Workspace/Core/LanguageServices/GeneratedCodeRecognition/IGeneratedCodeRecognitionService.cs Show resolved Hide resolved

CyrusNajmabadi mentioned this pull request Feb 8, 2020

Extract type into its own file. #41514

Merged

CyrusNajmabadi reviewed Feb 8, 2020

View reviewed changes

...ensions/Workspace/VisualBasic/LanguageServices/VisualBasicRemoveUnnecessaryImportsService.vb Show resolved Hide resolved

mavasani added 11 commits February 8, 2020 11:52

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

28ba549

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

f8d0bbc

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

8c37abe

Delete duplicate file added by automatic merge conflict resolution

3894e2d

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

e6caa1c

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

42d03a5

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

ba06655

Comment out some imports in CodeStyle layer. These will be reverted w…

7910709

…ith dotnet#41363

Merge remote-tracking branch 'upstream/master' into WorkspaceRefactoring

11b32b2

Cleanup

b550e0b

Fix typo in comment

e14a1cf

mavasani commented Feb 10, 2020

View reviewed changes

mavasani mentioned this pull request Feb 10, 2020

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

Closed

Move PerformanceSensitiveAttribute.cs to shared project

2b619ba

mavasani commented Feb 11, 2020

View reviewed changes

mavasani marked this pull request as ready for review February 11, 2020 00:50

mavasani requested review from a team as code owners February 11, 2020 00:50

mavasani commented Feb 11, 2020

View reviewed changes

jaredpar approved these changes Feb 11, 2020

View reviewed changes

tmat approved these changes Feb 11, 2020

View reviewed changes