Move fixer down to code-style layer #66780

CyrusNajmabadi · 2023-02-09T17:52:54Z

This required a bunch of layering work as this fixer depends on the internal ICodeGenerationService. So supporting this meant moving that service (And ancillary types) into teh shared source project we have between CodeStyle and Workspace layers.

This will also help move further analyzers/fixers down that had a similar dependency.

src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj

….csproj

…into moveFixer

CyrusNajmabadi · 2023-02-09T19:02:15Z

src/Analyzers/CSharp/CodeFixes/MakeLocalFunctionStatic/MakeLocalFunctionStaticCodeFixHelper.cs

@@ -134,13 +133,18 @@ internal static class MakeLocalFunctionStaticCodeFixHelper
                        {
                            syntaxEditor.ReplaceNode(
                                identifierNode,
-                                (node, generator) => generator.IdentifierName(parameter.Name.ToIdentifierToken()).WithTriviaFrom(node));
+                                (node, generator) => SyntaxFactory.IdentifierName(parameter.Name.ToIdentifierToken()).WithTriviaFrom(node));


this existing SyntaxGenerator helper is internal. Easiest workaround was to just do what it does (since we're at the C# layer).

CyrusNajmabadi · 2023-02-09T19:02:32Z

src/EditorFeatures/Test/CodeGeneration/CodeGenerationTests.cs

@@ -262,7 +262,7 @@ public partial class CodeGenerationTests
                exception = e;
            }

-            var expectedMessage = string.Format(WorkspacesResources.Cannot_generate_code_for_unsupported_operator_0, method.Name);
+            var expectedMessage = string.Format(WorkspaceExtensionsResources.Cannot_generate_code_for_unsupported_operator_0, method.Name);


a bunch of resources had to move as well. that's the bulk of the add/removes in this PR.

CyrusNajmabadi · 2023-02-09T19:03:15Z

src/Workspaces/Core/Portable/Shared/Extensions/ISymbolExtensions.cs

-                isOverride: symbol.IsOverride,
-                isSealed: symbol.IsSealed,
-                isRequired: symbol.IsRequired());
-        }


moved to shared location. i did'nt want to move all these extensions over as it had viral impact of forcing more stuff to move.

CyrusNajmabadi · 2023-02-09T19:03:22Z

src/Workspaces/Core/Portable/Shared/Extensions/ITypeSymbolExtensions.cs

-            result ??= new List<ITypeParameterSymbol>();
-            type?.Accept(new CollectTypeParameterSymbolsVisitor(result, onlyMethodTypeParameters: false));
-            return result;
-        }


moved to shared location.

CyrusNajmabadi · 2023-02-09T19:03:45Z

...Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CSharpWorkspaceExtensions.projitems

+    <Compile Include="$(MSBuildThisFileDirectory)CodeGeneration\ParameterGenerator.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)CodeGeneration\PropertyGenerator.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)CodeGeneration\StatementGenerator.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)CodeGeneration\TypeParameterGenerator.cs" />


Moving all thsee ICodeGenerationService pieces for C# into shared source project.

CyrusNajmabadi · 2023-02-09T19:04:26Z

...ces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/CSharpFlagsEnumGenerator.cs

@@ -10,20 +10,20 @@ namespace Microsoft.CodeAnalysis.CSharp.CodeGeneration
 {
    internal class CSharpFlagsEnumGenerator : AbstractFlagsEnumGenerator
    {
-        internal static readonly CSharpFlagsEnumGenerator Instance = new();
-        private static readonly SyntaxGenerator s_generatorInstance = CSharpSyntaxGenerator.Instance;


shared layer can't refer to this internal type. So all these types now have the SyntaxGenerator passed into them. it was a lot less impactful than i thought it would be.

CyrusNajmabadi · 2023-02-09T19:04:52Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/EventGenerator.cs

@@ -214,7 +214,7 @@ private static BlockSyntax GenerateBlock(IMethodSymbol accessor)
                }
                else
                {
-                    AddAccessibilityModifiers(@event.DeclaredAccessibility, tokens, info, Accessibility.Private);
+                    CSharpCodeGenerationHelpers.AddAccessibilityModifiers(@event.DeclaredAccessibility, tokens, info, Accessibility.Private);


had to qualify this as we have both a method and namespace with the same name in this layer.

CyrusNajmabadi · 2023-02-09T19:06:59Z

.../SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/AbstractCodeGenerationService.cs

            if (context.Context.AddImports)
            {
                var addImportsOptions = await newDocument.GetAddImportPlacementOptionsAsync(context.FallbackOptions, cancellationToken).ConfigureAwait(false);
                newDocument = await ImportAdder.AddImportsFromSymbolAnnotationAsync(newDocument, addImportsOptions, cancellationToken).ConfigureAwait(false);
            }
+#endif


carved out as it's not needed by the feature we actually moved down, and because this led to pulling in a huge amount of additional code.

Do we want a tracking issue to make this available in code style layer with a follow-up change? Might lead to subtle bugs if we move down a fixer to the shared layer that relies on this codegen option.

Sure. I'll file follow-up!

CyrusNajmabadi · 2023-02-09T19:07:40Z

...AndExtensions/Workspace/Core/CodeGeneration/AbstractCodeGenerationService_FindDeclaration.cs

@@ -77,13 +77,15 @@ public bool CanAddTo(SyntaxNode destination, Solution solution, CancellationToke
                return false;
            }

+#if !CODE_STYLE
            // If we are avoiding generating into files marked as generated (but are still regular files)
            // then check accordingly. This is distinct from the prior check in that we as a fallback
            // will generate into these files is we have no alternative.
            if (checkGeneratedCode && document.IsGeneratedCode(cancellationToken))


IsGeneratedCode is not available in CODE_STYLE layer. We only have IsGeneratedCodeAsync instead. But trying to use that virally exploded into too much becoming async. So i just carved this out as not supported.

This sounds reasonable as code fixers likely won't be dealing with generated code.

CyrusNajmabadi · 2023-02-09T19:08:18Z

...rkspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/CodeGenerationHelpers.cs

@@ -129,7 +129,7 @@ public static int GetPreferredIndex(int index, IList<bool>? availableIndices, bo
            var newLineStarter = string.Concat("\n", commentStarter);

            // Start the comment with an empty line for visual clarity.
-            comment = string.Concat(commentStarter, "\r\n", commentStarter, xml.Replace("\n", newLineStarter));
+            comment = string.Concat(commentStarter, "\r\n", commentStarter, xml!.Replace("\n", newLineStarter));


We have a call to string.IsNullOrEmpty above. But it appars as if nullability info isn't flowing along from that in this layer. not sure why.

CodeStyle layer is likely depending on older compiler bits, so probably a bug fixed recently in nullability analysis.

mavasani · 2023-02-11T05:29:10Z

...redUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/CSharpCodeGenerationContextInfo.cs

+        protected override SyntaxGenerator GeneratorImpl
+            => Service.LanguageServices.GetRequiredService<SyntaxGenerator>();
+


Does this need to be in language specific layer? Can it be moved up to CodeGenerationContextInfo base type?

I'll try that in follow-up!

mavasani · 2023-02-11T05:40:31Z

This required a bunch of layering work as this fixer depends on the internal ICodeGenerationService. So supporting this meant moving that service (And ancillary types) into teh shared source project we have between CodeStyle and Workspace layers.

This will also help move further analyzers/fixers down that had a similar dependency.

I presume this means we don't need #43056 anymore?

mavasani

LGTM. This is fantastic work and should help move more fixers to the shared layer.

mavasani · 2023-02-11T05:49:19Z

This PR should also close out #43070

mavasani · 2023-02-11T05:51:33Z

@CyrusNajmabadi we should also be able to move down the below code fixer to shared layer after this PR goes in:

roslyn/src/Features/CSharp/Portable/UseLocalFunction/CSharpUseLocalFunctionCodeFixProvider.cs

Line 32 in 321601a

    
           internal class CSharpUseLocalFunctionCodeFixProvider : SyntaxEditorBasedCodeFixProvider

CyrusNajmabadi · 2023-02-11T14:48:05Z

I presume this means we don't need #43056 anymore?

Correct

CyrusNajmabadi · 2023-02-11T14:48:30Z

@CyrusNajmabadi we should also be able to move down the below code fixer to shared layer after this PR goes in:

Will do next!

CyrusNajmabadi added 4 commits November 1, 2022 13:24

Move fixer down to code-style layer

c842c68

Merge remote-tracking branch 'upstream/main' into moveFixer

bc5d45d

move more down

8f50cc7

move more

ed0b4ac

CyrusNajmabadi requested a review from a team as a code owner February 9, 2023 17:52

CyrusNajmabadi marked this pull request as draft February 9, 2023 17:53

dotnet-issue-labeler bot added the Area-IDE label Feb 9, 2023

CyrusNajmabadi commented Feb 9, 2023

View reviewed changes

src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 5 commits February 9, 2023 09:53

Update src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces…

ae8a049

….csproj

Move more down

f010a2c

Merge branch 'moveFixer' of https://github.com/CyrusNajmabadi/roslyn …

4932a49

…into moveFixer

Fix

d406579

Merge remote-tracking branch 'upstream/main' into moveFixer

a715af6

CyrusNajmabadi commented Feb 9, 2023

View reviewed changes

Simplify

9a281b3

CyrusNajmabadi commented Feb 9, 2023

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review February 9, 2023 19:10

Fix casing

b4adc79

CyrusNajmabadi requested a review from mavasani February 10, 2023 20:04

mavasani reviewed Feb 11, 2023

View reviewed changes

mavasani approved these changes Feb 11, 2023

View reviewed changes

CyrusNajmabadi merged commit 90b68d5 into dotnet:main Feb 11, 2023

CyrusNajmabadi deleted the moveFixer branch February 11, 2023 14:49

ghost added this to the Next milestone Feb 11, 2023

allisonchou mentioned this pull request Feb 14, 2023

[Automated] PRs inserted in VS build main-33413.323 #66870

Closed

RikkiGibson modified the milestones: Next, 17.6 P2 Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move fixer down to code-style layer #66780

Move fixer down to code-style layer #66780

CyrusNajmabadi commented Feb 9, 2023 •

edited by mavasani

Loading

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

CyrusNajmabadi Feb 9, 2023

mavasani Feb 11, 2023

CyrusNajmabadi Feb 11, 2023

CyrusNajmabadi Feb 9, 2023

mavasani Feb 11, 2023

CyrusNajmabadi Feb 9, 2023

mavasani Feb 11, 2023

mavasani Feb 11, 2023 •

edited

Loading

CyrusNajmabadi Feb 11, 2023

mavasani commented Feb 11, 2023

mavasani left a comment

mavasani commented Feb 11, 2023

mavasani commented Feb 11, 2023

CyrusNajmabadi commented Feb 11, 2023

CyrusNajmabadi commented Feb 11, 2023

		protected override SyntaxGenerator GeneratorImpl
		=> Service.LanguageServices.GetRequiredService<SyntaxGenerator>();

Move fixer down to code-style layer #66780

Move fixer down to code-style layer #66780

Conversation

CyrusNajmabadi commented Feb 9, 2023 • edited by mavasani Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Feb 11, 2023

mavasani left a comment

Choose a reason for hiding this comment

mavasani commented Feb 11, 2023

mavasani commented Feb 11, 2023

CyrusNajmabadi commented Feb 11, 2023

CyrusNajmabadi commented Feb 11, 2023

CyrusNajmabadi commented Feb 9, 2023 •

edited by mavasani

Loading

mavasani Feb 11, 2023 •

edited

Loading