Move few `CodeFixProvider`s to Analyzers layer by Youssef1313 · Pull Request #60524 · dotnet/roslyn

Youssef1313 · 2022-04-01T14:25:48Z

This doesn't move everything. I may open follow up PRs.

@mavasani Can you take a look please?

Youssef1313 · 2022-04-01T14:36:52Z

Making a draft for now since it doesn't yet build successfully. I'll be force-pushing so you can wait on reviewing until it builds.

Youssef1313 · 2022-04-01T19:22:38Z

@mavasani One of the codefixes I'm moving in this PR is using ExpressionGenerator, which isn't available in CodeStyle. Any advice here? Should I add it as a public API in SyntaxGenerator?

Youssef1313 · 2022-04-01T23:15:19Z

@mavasani I attempted to move those CodeGeneration classes into the shared workspaces project, but:

There was many cascaded changes due to that.
It still doesn't build. Embarrassing that I don't understand the compile errors. They're like this:

4>C:\Users\PC\Desktop\Roslyn\src\Workspaces\SharedUtilitiesAndExtensions\Workspace\CSharp\CodeGeneration\CSharpSyntaxGenerator.cs(48,40,48,50): error CS0115: 'CSharpSyntaxGenerator.Whitespace(string)': no suitable method found to override
4>C:\Users\PC\Desktop\Roslyn\src\Workspaces\SharedUtilitiesAndExtensions\Workspace\CSharp\CodeGeneration\CSharpSyntaxGenerator.cs(27,20,27,41): error CS0534: 'CSharpSyntaxGenerator' does not implement inherited abstract member 'SyntaxGenerator.Whitespace(string)'

This is very confusing :(

CyrusNajmabadi · 2022-04-01T23:30:09Z

src/Analyzers/CSharp/CodeFixes/HideBase/HideBaseCodeFixProvider.AddNewKeywordAction.cs

+                var options = _document.Project.AnalyzerOptions.GetAnalyzerOptionSet(_node.SyntaxTree, cancellationToken);
+#else
+                var options = await _document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
+#endif


man, i would love an extension for this.

CyrusNajmabadi · 2022-04-01T23:30:39Z

src/Analyzers/Core/CodeFixes/AddExplicitCast/AbstractAddExplicitCastCodeFixProvider.cs

+                CodeFixesResources.Add_explicit_cast,
                actions.ToImmutable(), isInlinable: false),
                context.Diagnostics);
+#pragma warning restore RS0005 // Do not use generic 'CodeAction.Create' to create 'CodeAction'


seems problematic.

CyrusNajmabadi · 2022-04-01T23:30:55Z

src/Analyzers/Core/CodeFixes/AliasAmbiguousType/AbstractAliasAmbiguousTypeCodeFixProvider.cs

+                var groupingTitle = string.Format(CodeFixesResources.Alias_ambiguous_type_0, diagnosticNode.ToString());
+#pragma warning disable RS0005 // Do not use generic 'CodeAction.Create' to create 'CodeAction'
+                var groupingCodeAction = CodeAction.Create(groupingTitle, codeActionsBuilder.ToImmutable(), isInlinable: true);
+#pragma warning restore RS0005 // Do not use generic 'CodeAction.Create' to create 'CodeAction'


seems problematic.

@CyrusNajmabadi I don't think there is a problem regarding the suppression.

The analyzer will be removed soon anyway when @mavasani PR that fixes telemetry is merged.

There are already few suppressions also anyway.

CyrusNajmabadi · 2022-04-02T01:36:47Z

One of the codefixes I'm moving in this PR is using ExpressionGenerator, which isn't available in CodeStyle. Any advice here? Should I add it as a public API in SyntaxGenerator?

Source link the generator code

Youssef1313 · 2022-04-02T13:12:33Z

One of the codefixes I'm moving in this PR is using ExpressionGenerator, which isn't available in CodeStyle. Any advice here? Should I add it as a public API in SyntaxGenerator?

Source link the generator code

@CyrusNajmabadi I ended up with the same compile errors after adding some needed files using <Compile Include="..." Link="..." />:

  <ItemGroup>
    <Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\ExpressionGenerator.cs" Link="CodeGeneration\CSharp\ExpressionGenerator.cs" />
    <Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\CSharpFlagsEnumGenerator.cs" Link="CodeGeneration\CSharp\CSharpFlagsEnumGenerator.cs" />
    <Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\CSharpSyntaxGenerator.cs" Link="CodeGeneration\CSharp\CSharpSyntaxGenerator.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\CodeGenerationHelpers.cs" Link="CodeGeneration\Core\ExpressionGenerator.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\CodeGenerationOptions.cs" Link="CodeGeneration\Core\CodeGenerationOptions.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\CodeGenerationContext.cs" Link="CodeGeneration\Core\CodeGenerationContext.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\LiteralSpecialValues.cs" Link="CodeGeneration\Core\LiteralSpecialValues.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\AbstractFlagsEnumGenerator.cs" Link="CodeGeneration\Core\AbstractFlagsEnumGenerator.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\CodeGenerator.cs" Link="CodeGeneration\Core\CodeGenerator.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\ICodeGenerationService.cs" Link="CodeGeneration\Core\ICodeGenerationService.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\Symbols\CodeGenerationSymbol.cs" Link="CodeGeneration\Core\Symbols\CodeGenerationContext.cs" />
    <Compile Include="..\..\..\Workspaces\Core\Portable\CodeGeneration\CodeGenerationDestination.cs" Link="CodeGeneration\Core\CodeGenerationDestination.cs" />
  </ItemGroup>

Youssef1313 · 2022-04-02T13:50:43Z

@CyrusNajmabadi I think I understood why it doesn't compile. The members that have the errors I mentioned above are internal abstract. The type SyntaxGenerator is already visible since it's public, so it doesn't need to be source-linked. Hence, there is no way to implement the internal abstract methods.

I tried adding IVT, but I get too many errors similar to:

9>C:\Users\PC\Desktop\Roslyn\src\Workspaces\Core\Portable\CodeGeneration\Symbols\CodeGenerationSymbol.cs(19,47,19,67): warning CS0436: The type 'CodeGenerationSymbol' in 'C:\Users\PC\Desktop\Roslyn\src\Workspaces\Core\Portable\CodeGeneration\Symbols\CodeGenerationSymbol.cs' conflicts with the imported type 'CodeGenerationSymbol' in 'Microsoft.CodeAnalysis.Workspaces, Version=42.42.42.42, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. Using the type defined in 'C:\Users\PC\Desktop\Roslyn\src\Workspaces\Core\Portable\CodeGeneration\Symbols\CodeGenerationSymbol.cs'.
9>C:\Users\PC\Desktop\Roslyn\src\Analyzers\CSharp\CodeFixes\UsePatternMatching\CSharpUseNotPatternCodeFixProvider.cs(24,65,24,97): error CS0433: The type 'SyntaxEditorBasedCodeFixProvider' exists in both 'Microsoft.CodeAnalysis.CodeStyle.Fixes, Version=42.42.42.42, Culture=neutral, PublicKeyToken=31bf3856ad364e35' and 'Microsoft.CodeAnalysis.Workspaces, Version=42.42.42.42, Culture=neutral, PublicKeyToken=31bf3856ad364e35'

Youssef1313 · 2022-04-02T14:17:28Z

@mavasani @CyrusNajmabadi I'm going to move the specific problematic CodeFixProvider to Features back.

Youssef1313 · 2022-04-02T14:23:45Z

@CyrusNajmabadi @mavasani This is ready for review.

CyrusNajmabadi · 2022-04-02T15:43:48Z

I believe we have SyntaxGenerator and SyntaxGeneratorInternal for this reason.

Youssef1313 · 2022-04-02T15:50:24Z

@CyrusNajmabadi I'll take a look into that in another PR. There are lots of work to move everything to Analyzers anyway :)

mavasani · 2022-04-04T06:11:42Z

@Youssef1313 I had taken a stab at moving CodeGeneration pieces down to the shared layer, and just as you disccovered, it leads to a lot of cascading changes. I also filed #43056 for making relevant APIs public, but that would need a design proposal and discussion. If you are interested, feel free to take a stab at coming up with the API proposal.

Additionally, this issue tracks bunch more cleanup/porting/follow-up items for the move down to the shared analyzers/fixes layer. Feel free to pick up anything from there if you are interested.

Thank you again for your efforts here!

...haredUtilitiesAndExtensions/Workspace/Core/LanguageServices/AddImports/IAddImportsService.cs

mavasani

Mostly LGTM - have a code refactoring suggestion. Once you address/respond, I can merge the PR. Thanks!

Youssef1313 · 2022-04-04T07:38:50Z

@mavasani Thanks for review! Can you take another look?

...haredUtilitiesAndExtensions/Workspace/Core/LanguageServices/AddImports/IAddImportsService.cs

mavasani · 2022-04-05T04:09:28Z

@Youssef1313 You also have merge conflicts.

Youssef1313 requested a review from a team as a code owner April 1, 2022 14:25

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Analyzers labels Apr 1, 2022

Youssef1313 force-pushed the move-some-fixes branch from 685eb2e to 603e64f Compare April 1, 2022 14:36

Youssef1313 marked this pull request as draft April 1, 2022 14:36

Youssef1313 force-pushed the move-some-fixes branch 5 times, most recently from 92d4545 to e8c0ca8 Compare April 1, 2022 17:45

Youssef1313 changed the title ~~Move few 'CodeFixProvider's to Analyzers layer~~ Move few CodeFixProviders to Analyzers layer Apr 1, 2022

Youssef1313 force-pushed the move-some-fixes branch 3 times, most recently from 47ac429 to ece3473 Compare April 1, 2022 19:19

Youssef1313 force-pushed the move-some-fixes branch from ece3473 to 62f628a Compare April 1, 2022 22:51

Move few 'CodeFixProvider's to Analyzers layer

087bc2c

Youssef1313 force-pushed the move-some-fixes branch from 62f628a to 087bc2c Compare April 1, 2022 23:13

CyrusNajmabadi reviewed Apr 1, 2022

View reviewed changes

jinujoseph assigned mavasani Apr 2, 2022

Revert bad changes

82f75a5

Fix errors and remove a hard CodeFixProvider back to Features

9a7897a

Update projitems

f82a7a1

Youssef1313 marked this pull request as ready for review April 2, 2022 14:23

Move tests

baa1cd7

mavasani reviewed Apr 4, 2022

View reviewed changes

...haredUtilitiesAndExtensions/Workspace/Core/LanguageServices/AddImports/IAddImportsService.cs Outdated Show resolved Hide resolved

mavasani approved these changes Apr 4, 2022

View reviewed changes

Youssef1313 added 2 commits April 4, 2022 09:25

Merge branch 'main' into move-some-fixes

6db1fc1

Address review comment

ca0c17c

mavasani reviewed Apr 5, 2022

View reviewed changes

...haredUtilitiesAndExtensions/Workspace/Core/LanguageServices/AddImports/IAddImportsService.cs Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into move-some-fixes

97ba8a2

mavasani merged commit 60a165c into dotnet:main Apr 5, 2022

ghost added this to the Next milestone Apr 5, 2022

Youssef1313 deleted the move-some-fixes branch April 5, 2022 23:00

dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022

Conversation

Youssef1313 commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented Apr 1, 2022

Uh oh!

Youssef1313 commented Apr 1, 2022

Uh oh!

Youssef1313 commented Apr 1, 2022

Uh oh!

CyrusNajmabadi Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 1, 2022

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 2, 2022

Uh oh!

Youssef1313 commented Apr 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented Apr 2, 2022

Uh oh!

Youssef1313 commented Apr 2, 2022

Uh oh!

Youssef1313 commented Apr 2, 2022

Uh oh!

CyrusNajmabadi commented Apr 2, 2022

Uh oh!

Youssef1313 commented Apr 2, 2022

Uh oh!

mavasani commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Apr 4, 2022

Uh oh!

Uh oh!

mavasani commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Youssef1313 commented Apr 1, 2022 •

edited

Loading

Youssef1313 commented Apr 2, 2022 •

edited

Loading

mavasani commented Apr 4, 2022 •

edited

Loading