Code generator fallback options by tmat · Pull Request #60882 · dotnet/roslyn

tmat · 2022-04-21T15:46:51Z

Features that use code generation require different sets of options depending on what kind of operations they are performing on the generated code. We can identify 4 sets of options that features commonly require:

CodeGenerationOptions
Options that directly affect code generation such as preferences for expression bodies, etc.
CodeAndImportGenerationOptions = CodeGerationOptions + ImportPlacementOptions
Code generation that also updates namespace imports.
CodeCleanupOptions = SyntaxFormattingOptions + SimplifierOptions + AddImportPlacementOptions
Options that affect code cleanup/prettification
CleanCodeGenerationOptions = CodeGenerationOptions + CodeCleanupOptions
Code generation followed by code cleanup.

This change introduces these sets of options and corresponding providers (XyzOptionsProvider) that retrieve them from an option storage.

Previous PRs introduced XyzOptionsProvider as delegates. Given that one option storage can provide multiple sets of options it turned out to be beneficial to switch these types to interfaces. It simplifies access patterns that occur very frequently and also avoid delegate allocations. Small amount of code got a bit more complex but the tradeoff is in favor of using interfaces.

The first couple of commits rename current CodeGeneratorPreferences to CodeGeneratorOptions and CodeGeneratorOptions to CodeGeneratorContextInfo to fit the naming pattern of -Options types. CodeGeneratorContextInfo combines CodeGeneratorOptions, CodeGeneratorContext and LanguageVersion. Further refactoring may be possible that merges Context and ContextInfo, but is out of scope of this PR.

ExtractMethodGenerationOptions and ImplementTypeGenerationOptions added in this PR are also worth mentioning.
They combine primitive options (i.e. ExtractMethodOptions, ImplementTypeOptions) with additional code generation/cleanup options. Separating these is important for option set composition. E.g. CodeActionOptions include all options needed for extract method and implement type features. The primitive sets are included there along with all the primitive code generation options.

Fixes #60794

Passed VS validation build tests: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/396534

tmat · 2022-04-21T16:11:17Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeFixes/CodeActionOptions.cs

+    }

-    internal delegate CodeActionOptions CodeActionOptionsProvider(HostLanguageServices languageService);
+    internal abstract class AbstractCodeActionOptionsProvider : CodeActionOptionsProvider


Makes it easier to implement CodeActionOption providers that fetch all CodeActionOptions and then provide all the subsets of options needed by code actions.

tmat · 2022-04-21T16:11:51Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeFixes/CodeActionOptions.cs

+#endif
+    }
+
+    internal sealed class DelegatingCodeActionOptionsProvider : AbstractCodeActionOptionsProvider


Helper to allow inline definition of a CodeActionOptionProvider using a lambda.

tmat · 2022-04-21T16:22:26Z

src/EditorFeatures/CSharp/CodeGeneration/CSharpCodeGenerationOptionsStorage.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.CodeGeneration;
+
+internal static class CSharpCodeGenerationOptionsStorage


Code gen options for C#

tmat · 2022-04-21T16:27:00Z

src/Workspaces/Core/Portable/CodeGeneration/CodeGenerationContext.cs


 namespace Microsoft.CodeAnalysis.CodeGeneration
 {
+    internal readonly record struct CodeGenerationSolutionContext(


Groups information needed for generating code to arbitrary file in the solution. This may require generating code in more than one language and thus language specific fallback options.

tmat · 2022-04-21T16:29:26Z

.../Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.ExpressionCodeGenerator.cs

                    AnalyzerResult analyzerResult,
-                    OptionSet options,
+                    CSharpCodeGenerationOptions options,
+                    NamingStylePreferencesProvider namingPreferences,


Naming preferences are separate from code gen options since they are fairly expensive and not always needed. Currently they are also not messagepack-serializable. Will follow up on that.

tmat · 2022-04-21T16:32:44Z

src/Workspaces/Core/Portable/ExtractMethod/ExtractMethodOptions.cs

    public static readonly ExtractMethodOptions Default = new();
 }
+
+internal readonly record struct ExtractMethodGenerationOptions(


ExtractMethodOptions only contains options specific to method extraction. Additional options needed for code generation, naming and formatting are added in ExtractMethodGenerationOptions. Separating these options makes them composable to other option sets w/o duplication, e.g. CodeActionOptions.

CyrusNajmabadi · 2022-05-02T18:36:45Z

src/EditorFeatures/CSharp/CodeGeneration/CSharpCodeGenerationOptionsStorage.cs

+        }
+
+        public CodeGenerationOptions GetOptions(IGlobalOptionService globalOptions)
+            => GetCSharpCodeGenerationOptions(globalOptions);


aside, why not pull in IGlobalOptionService in teh importing constructor here. tehn, the client of ICodeGenerationOptionsStorage doesn't need to get IGlobalOptionService themselves. they just call .GetOptions() without having to care bout the impl detail of how that works.

That would be possible, but the call site is an extension method on IGlobalOptionService:

public static CodeGenerationOptions GetCodeGenerationOptions(this IGlobalOptionService globalOptions, HostLanguageServices languageServices) => languageServices.GetRequiredService<ICodeGenerationOptionsStorage>().GetOptions(globalOptions);

The caller already has the global options and asks for various options from it. So the ICodeGenerationOptionsStorage is the implementation detail here needed only to instantiate language specific options object.

CyrusNajmabadi · 2022-05-02T18:39:03Z

src/EditorFeatures/Core/InlineRename/AbstractEditorInlineRenameService.cs

            var sourceText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
            var triggerText = sourceText.ToString(triggerToken.Span);
-            var fallbackOptions = _globalOptions.GetCodeCleanupOptionsProvider();
+            var fallbackOptions = _globalOptions.CreateProvider();


hrmm.. this reads a bit odd. what sort of provider is this making?

This one: https://github.com/dotnet/roslyn/pull/60882/files/4394aa5ebde133b78ccfec65addba40bf7ddbf69#diff-0b999bc4c9feee2f0732a23368a04773ea9665f5b2f652144461123d6113366eR27

Yeah, it's not very descriptive. I'll rename to CreateCodeActionOptionsProvider. It's just temporary. the IGlobalOptionsService should itself implement the provider interfaces. Currently that's not possible due to layering, but eventually it should be.

Actually, not sure about the rename to CreateCodeActionOptionsProvider. It creates a provider that implements various options. It's not just CodeActionOptions, but all kinds of other options. While CodeActionOptions generally have all of these options, it's misleading to call the factory CreateCodeActionOptionsProvider since the call site might not actually care about code action options but other options. So I think I'd rather keep the name as is for now.

CyrusNajmabadi · 2022-05-02T18:39:55Z

src/EditorFeatures/VisualBasic/CodeGeneration/VisualBasicCodeGenerationOptionsStorage.vb

+            End Sub
+
+            Public Function GetOptions(globalOptions As IGlobalOptionService) As CodeGenerationOptions Implements ICodeGenerationOptionsStorage.GetOptions
+                Return VisualBasicCodeGenerationOptions.Default


interesting. is thsi expected? VB doesn't have any special options here?

Expected in this phase. Next PR will add one :)

ghost added the Area-IDE label Apr 21, 2022

tmat force-pushed the GenOptions branch from 8175fc1 to 7ff7d86 Compare April 21, 2022 15:49

tmat commented Apr 21, 2022

View reviewed changes

tmat force-pushed the GenOptions branch from 7ff7d86 to e715a45 Compare April 21, 2022 16:14

tmat commented Apr 21, 2022

View reviewed changes

tmat force-pushed the GenOptions branch 4 times, most recently from cac7c43 to d7be96d Compare April 27, 2022 16:49

tmat marked this pull request as ready for review April 27, 2022 16:56

tmat requested review from a team, 333fred and JoeRobich as code owners April 27, 2022 16:56

tmat force-pushed the GenOptions branch 7 times, most recently from 21df354 to 4394aa5 Compare April 30, 2022 21:08

CyrusNajmabadi reviewed May 2, 2022

View reviewed changes

CyrusNajmabadi approved these changes May 2, 2022

View reviewed changes

Renames

0cc30d2

tmat added 3 commits May 2, 2022 12:16

Rename files

73afa6d

Code generation fallback options

d171b0b

Fix tests

0c106a8

tmat force-pushed the GenOptions branch from 4394aa5 to 0c106a8 Compare May 2, 2022 19:24

tmat enabled auto-merge (squash) May 2, 2022 19:24

tmat merged commit 9a994fe into dotnet:main May 2, 2022

ghost added this to the Next milestone May 2, 2022

tmat deleted the GenOptions branch May 2, 2022 20:59

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022


		namespace Microsoft.CodeAnalysis.CSharp.CodeGeneration;

		internal static class CSharpCodeGenerationOptionsStorage

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tmat commented Apr 21, 2022 •

edited

Loading