Move Options and CodeStyle APIs to shared layer #42323

mavasani · 2020-03-10T23:09:12Z

Strongly recommend reviewing commit by commit. I have tried to add descriptive comments to each commit for aid in review. Gist of the change:

Move common Options and CodeStyle related public and internal types from the Workspaces layer into the shared layer. First commit of the PR 6692e76 has these core changes. The public types are renamed to end with a 2 suffix and are internal types, most of which have implicit conversions to the public type. For example, the shared layer has new internal types IOption2, Option2<T>, PerLanguageOption2<T>, CodeStyleOption<2>, CodeStyleOptions2 and so on. These are the core types that will replace the public Options types in our shared analyzer and fixer layer, and possibly even all of Roslyn IDE, to avoid name clashes. The public types are now just thin shims that are forwarders to their internal counterparts and primarily for external consumption.
a2c78bd has very similar changes made to FormattingOptions and CSharpFormattingOptions.
3ba9025 shows how our exposed options transition to be based on these new option types.
7077431 moves our core analyzer base types to be based off these new types
e182acd moves our shared analyzer layer to these types, a bunch of nice cleanup and no longer need of #if CODE_STYLE in this layer.
2143656 is similar to the previous commit, but applied to all the IDE analyzers in Features layer - very mechanical change.
a9da22c adds a BannedSymbols.txt to Features layer to prevent accidental use of public CodeStyle and Options related types from Workspaces in analyzer/fixer layer. This should keep our IDE layer clean and stick to a single Options and CodeStyle API - you will get a warning if you use the wrong types in these layers.
Rest of the commits are pretty mechanical changes, though large scale that touch many files:
1. 00b3593: Move EditorFeatures layer to use new types
2. 6e1e5dd: Move VS layer to use new types
3. f1b4348: CodeStyle layer test library split to avoid CodeStyle fixes test project having type ambiguity errors for shared option types from Workspaces and CodeStyle analyzer layer.
4. e4c7e8e: Cleanup #if CODE_STYLE directive in analyzer tests in shared layer
5. 7e3c47e: Biggest commit that moves our tests in different layers (Workspaces, Features, EditorFeatures and VS) to new options types
6. c765e1f: Commit for all project file, resx moves and xlf file changes. Feel free to skip or skim through these.

…nto the shared layer. The public types are renamed to end with a "2" suffix and are internal. These will be the core types that will replace the public Options types to avoid name clashes.

…ce and option set

…core impl types in shared layer with "2" suffix and shim public API types in Workspaces layer.

…ng options

…ions to the shared layer, and I will not be doing it in this PR. That will be in a follow-up PR.

…w option types

…d type. Also update options related helpers in shared analyzer layer.

… shared layer

…tyle types - this cleans up the `#if CODE_STYLE` mess from our shared layer analyzers.

…e style API

… public CodeStyle and Options related types in Features layer. This should keep our IDE layer clean and onto a single Options and CodeStyle API.

…al API

…ons to shared layer.

… legacy test framework and the new test framework. This was needed to avoid type ambiguity from same internal options types from Workspaces and CodeStyle. Also added a bunch of test helper methods for new option types and ensured that we serialize naming styles in both tests frameworks

…layer. Should be much cleaner now.

mavasani · 2020-03-10T23:13:27Z

src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs

 {
-    internal interface ICodeStyleOption


Moved to shared layer

mavasani · 2020-03-10T23:16:26Z

src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs

    public class CodeStyleOption<T> : ICodeStyleOption, IEquatable<CodeStyleOption<T>>
    {
+        private readonly CodeStyleOption2<T> _codeStyleOptionImpl;


All the existing code from this type was copied directly into CodeStyleOption2<T> being added in shared layer. This public type now just has a handle to the instance of CodeStyleOption2<T> that has all the implementation. Very similar approach has been used for rest of the public types in this PR.

mavasani · 2020-03-10T23:17:24Z

src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs

 {
+    /// <inheritdoc cref="CodeStyleOptions2"/>
    public class CodeStyleOptions


This type now just has simple public fields that forward to CodeStyleOptions2. All the deleted code below is just moved to CodeStyleOptions2 without any semantic changes.

mavasani · 2020-03-10T23:18:44Z

src/Workspaces/Core/Portable/CodeStyle/NotificationOption.cs

    public class NotificationOption
    {
-        public string Name { get; set; }
+        private readonly NotificationOption2 _notificationOptionImpl;


Now delegates to internal shared layer type NotificationOption2.

mavasani · 2020-03-10T23:20:05Z

src/Workspaces/Core/Portable/Options/EditorConfig/EditorConfigFileGenerator.cs

@@ -90,119 +86,5 @@ string GetEditorConfigString(IOption option, IEditorConfigStorageLocation2 edito
                return editorConfigString;
            }
        }
-
-        public static void AppendNamingStylePreferencesToEditorConfig(NamingStylePreferences namingStylePreferences, string language, StringBuilder editorconfig)


The deleted code here is just moved to a separate partial declaration down in the shared layer. Unfortunately, I missed including that new file in this commit - it is in one of the following commits, without any semantic changes.

mavasani · 2020-03-10T23:34:56Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/Option2`1.cs

+    /// <summary>
+    /// Marker interface for language specific options.
+    /// </summary>
+    internal interface ILanguageSpecificOption : IOptionWithGroup


This is an existing type just moved to shared layer.

mavasani · 2020-03-10T23:36:28Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/Option2`1.cs

+    /// <summary>
+    /// Marker interface for language specific options.
+    /// </summary>
+    internal interface ILanguageSpecificOption<T> : ILanguageSpecificOption


This is an new interface type added to allow being used as a common base interface for Option<T> and Option2<T>, so certain APIs can directly just use ILanguageSpecificOption<T> as a parameter rather then have two similar overloads. This is not always feasible though so is not used everywhere.

mavasani · 2020-03-10T23:36:59Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/Option2`1.cs

+        }
+
+#if !CODE_STYLE
+        public static implicit operator Option<T>(Option2<T> option)


New code which adds a conversion from internal type to public type.

mavasani · 2020-03-10T23:38:03Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/OptionKey2.cs

+namespace Microsoft.CodeAnalysis.Options
+{
+    [NonDefaultable]
+    internal readonly struct OptionKey2 : IEquatable<OptionKey2>


This is a clone of OptionKey as the code is very trivial. I have added implicit conversion to public type at the bottom of the file.

mavasani · 2020-03-10T23:39:06Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/OptionKey2.cs

+        }
+
+#if !CODE_STYLE
+        public static implicit operator OptionKey(OptionKey2 optionKey)


Note the reverse conversion is not possible because OptionKey.Option has type IOption, which can be any third party provided implementation of IOption that cannot be represented by OptionKey2

mavasani · 2020-03-10T23:42:25Z

src/Workspaces/Core/Portable/Options/IOptionService.cs

+        /// Gets the current value of the specific option.
+        /// </summary>
+        [return: MaybeNull]
+        T GetOption<T>(Option2<T> option);


Note that instead of adding a new overload for Option2<T> and PerLanguageOption2<T>, I tried to just have single overloads for ILanguageSpecificOption<T> and IPerLanguageOption<T>, but that caused overload resolution issues as we also need an overload that takes OptionKey, which has implicit conversions from each of the option types and so there is no best match implicit conversion.

See comment elsewhere: can we just have an extension method for this so we don't have to push the implementation everywhere?

src/Workspaces/CSharp/Portable/Formatting/CSharpFormattingOptions.cs

mavasani · 2020-03-10T23:44:14Z

...rkspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/CSharpFormattingOptions2.cs

 #endif

 namespace Microsoft.CodeAnalysis.CSharp.Formatting
 {
-    public static partial class CSharpFormattingOptions
+    internal static partial class CSharpFormattingOptions2


Finally, one change that GitHub smartly recognized as file move + type rename instead of showing a new file addition!

mavasani · 2020-03-10T23:45:46Z

...rkspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Formatting/CSharpFormattingOptions2.cs

@@ -419,7 +417,11 @@ static CSharpFormattingOptions()
        }
    }

+#if CODE_STYLE
+    internal enum LabelPositionOptions
+#else
    public enum LabelPositionOptions


I did not clone public enum types such as these into shared layer, as that is too much pain, especially to ensure the fields and values are always in sync. I think we should be fine for enums with this approach.

mavasani · 2020-03-10T23:46:24Z

...paces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/FormattingOptions.IndentStyle.cs

+    public static partial class FormattingOptions
+#endif
+    {
+        public enum IndentStyle


Extracted into its own partial declaration - retained as a public type in workspaces, and internal enum in code style layer.

mavasani · 2020-03-10T23:49:12Z

src/Analyzers/Core/Analyzers/Helpers/AnalyzerHelper.cs

 #if CODE_STYLE
-using Microsoft.CodeAnalysis.Internal.Options;
+using TOption = Microsoft.CodeAnalysis.Options.IOption2;


A rare case where #if CODE_STYLE in a helper methods file is helping avoid duplicate code, which needs to operate on core fields of IOption and IOptions.

mavasani · 2020-03-10T23:49:46Z

src/Workspaces/Core/Portable/Diagnostics/Extensions.cs

@@ -176,30 +175,5 @@ private static string GetAssemblyQualifiedName(Type type)

            return builder.ToImmutable();
        }
-
-        public static NotificationOption ToNotificationOption(this ReportDiagnostic reportDiagnostic, DiagnosticSeverity defaultSeverity)


Moved to shared layer.

mavasani · 2020-03-20T11:07:34Z

Do we need a doc for a newbie to our repository to understand how this works? This PR does a great job using type safety and analyzers to keep some of the worst mistakes from happening, but I'd worry that the "how to use options" might not be obvious for somebody starting out, since we're more or less abandoning our public surface area. If somebody is trying to something that matches what's been done before it might be easy to just copy/paste and follow the pattern, but any "newer" code is decidedly less obvious how to write

In theory, all of our code base should continue working on either set of types - outside of the shared layer, using either of these types should not cause any issues. I completely get your point though that is indeed adding an extra layer of confusion because you now have two available similar looking types to choose from. Let this change bake in for a week or so and I can ask the team how often they are hitting this hurdle and the common pain points and confusions, so the doc has appropriate content. In meantime, hopefully we can convince the compiler team to move Options down + update CodeStyle layer to newer Microsoft.CodeAnalysis, and I would be extremely happy to just delete the internal types added here and avoid anyone going through the pain of reading through this stuff.

How do we test this safely? There's a number of IVT risks as well as a few places where a subtle change could pretty easily break something. Do we have a good plan in place?

Our existing unit tests caught quite a bit of stuff during the implementation to give me enough confidence, but I agree that there can be things missed out, so we would have to just keep an eye on bugs from dogfooding. Regarding future IVT risks, we should encourage everyone to move the external access layer for internal APIs being used - I was safely able to avoid any breaking changes for F# usage as they follow this model very nicely, other teams should follow suite if they wish to be resistant to such breaks.

What's the next steps? We're still tracking thinking of possible better approaches, or there a good list of next steps? This moves us forward, but at the same time still feels like we're creating debt that needs to be cleaned up...

I am going to file a separate follow-up issue with suitable context for compiler team for possibility of moving Options down to compiler layer (unless @sharwell already filed in the past which we can re-open). If that does not fly, we should consider your suggestion of packaging Workspaces assembly somehow with the NuGet package. I do feel the latter is quite an inferior solution due to the large number of compat/versioning/appdomain issues that come along with it, so hopefully it doesn't come down to it.

…e for back compat.

src/Analyzers/CSharp/Tests/AddBraces/AddBracesTests.cs

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

.../VisualBasicTest/Diagnostics/AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest.vb

src/Workspaces/Core/Portable/ExternalAccess/Pythia/Api/PythiaOptions.cs

src/Workspaces/Core/Portable/Options/OptionKey.cs

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/IOption2.cs

…rvice for different combinations of our internal and public option types. Also fix the issue discovered from these tests.

mavasani · 2020-03-20T20:51:49Z

With @sharwell's help, I have added a matrix of tests for option setter/getter APIs, which hopefully increases our confidence on this PR, especially in terms of invalid cast exception failures.

jasonmalinowski · 2020-03-24T00:02:05Z

Regarding future IVT risks, we should encourage everyone to move the external access layer for internal APIs being used - I was safely able to avoid any breaking changes for F# usage as they follow this model very nicely, other teams should follow suite if they wish to be resistant to such breaks.

Have we at least done a smoke test with the F# and TypeScript experiences? Or want to have them do some testing too?

jasonmalinowski · 2020-03-24T00:08:46Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/IOption2.cs

+    internal interface IOption2 : IEquatable<IOption2?>
+#if !CODE_STYLE
+        , IOption
+#endif
+    {
+        OptionDefinition OptionDefinition { get; }
+
+#if CODE_STYLE
+        string Feature { get; }
+        string Name { get; }
+        Type Type { get; }
+        object? DefaultValue { get; }
+        bool IsPerLanguage { get; }
+
+        ImmutableArray<OptionStorageLocation2> StorageLocations { get; }
+#endif


This file seems sufficiently magic as to need some comment in it somewhere.

Sure, will do in a follow-up PR to avoid re-running all the tests and risking another merge conflict resolution round :-). Hope that is fine.

jasonmalinowski · 2020-03-24T00:09:29Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Options/OptionDefinition.cs

+            hash = unchecked((hash * (int)0xA5555529) + this.Name.GetHashCode());
+            hash = unchecked((hash * (int)0xA5555529) + this.IsPerLanguage.GetHashCode());
+
+            if (!(this.DefaultValue is ICodeStyleOption))


Why the exception for this?

These will be different between CodeStyleOption and CodeStyleOption2. Theoretically, I think we don't even need to check for default value when checking equatability of options - I don't believe anyone will or should define options with same name, feature name and group, but just different default values. We can clean this up to remove the default value check even for non-code style options in future.

mavasani · 2020-03-24T00:18:23Z

Have we at least done a smoke test with the F# and TypeScript experiences? Or want to have them do some testing too?

I am pretty confident on F# side due to them having defined all the options they access in their external access layer. I will verify the TS experience locally.

mavasani · 2020-03-24T00:46:58Z

I am going to send a follow-up PR to address some pending feedback, but am going to merge this one to avoid further merge conflicts from further delay as this PR touches lot of files...

Thanks @sharwell and @jasonmalinowski for your help here.

ghost

Auto-approval

tmat · 2020-03-24T01:01:56Z

@mavasani Check with @allisonchou, she was recently testing TypeScript IVT changes knows how to do validation.

tmat · 2020-03-24T01:02:40Z

F# does not have any IVTs anymore, except for VS layer.

Follow-up to #42323

mavasani added 22 commits March 10, 2020 10:50

Move common Options and CodeStyle related public and internal types i…

6692e76

…nto the shared layer. The public types are renamed to end with a "2" suffix and are internal. These will be the core types that will replace the public Options types to avoid name clashes.

Add API overloads for new shared layer options types to options servi…

cd860f7

…ce and option set

Make similar changes to FormattingOptions and C# FormattingOptions - …

a2c78bd

…core impl types in shared layer with "2" suffix and shim public API types in Workspaces layer.

Some more follow-up changes in CodeStyle related files

762c09c

Updating formatting rules and engine to use the shared layer formatti…

2848a31

…ng options

VB CodeStyle related changes. We still haven't moved VB CodeStyle opt…

70b4089

…ions to the shared layer, and I will not be doing it in this PR. That will be in a follow-up PR.

Move bunch of options in Workspaces and Features layers to use the ne…

3ba9025

…w option types

Move the base CodeStyle and naming analyzer to the new options relate…

7077431

…d type. Also update options related helpers in shared analyzer layer.

Some cleanup

30b4e99

VB formatting engine and rules moved to new formatting options API in…

e96fd8f

… shared layer

Move all the shared layer analyzers to use the new options and code s…

e182acd

…tyle types - this cleans up the `#if CODE_STYLE` mess from our shared layer analyzers.

Move all the IDE (Features layer) analyzer to the new options and cod…

2143656

…e style API

Add BannedSymbols.txt for Features layer to prevent accidental use of…

a9da22c

… public CodeStyle and Options related types in Features layer. This should keep our IDE layer clean and onto a single Options and CodeStyle API.

Changes to CodeStyle layer to delete linked files and trim public APIs

b9cf84d

Move EditorFeatures layer to new options types

00b3593

Move VS layer and some of the options it depends on to the new intern…

6e1e5dd

…al API

Move part of EditorConfigFileGenerator that can serialize naming opti…

74b90a8

…ons to shared layer.

Fix some build and test failures

cbead33

Cleanup the conditional directives in analyzer/fixer tests in shared …

e4c7e8e

…layer. Should be much cleaner now.

Tons of test changes to move them to the new Options APIs.

7e3c47e

Commit for all project file, resx moves and xlf file changes.

c765e1f

mavasani commented Mar 10, 2020

View reviewed changes

src/Workspaces/CSharp/Portable/Formatting/CSharpFormattingOptions.cs Outdated Show resolved Hide resolved

mavasani commented Mar 10, 2020

View reviewed changes

sharwell added 3 commits March 19, 2020 22:10

Unwrap CodeStyleOption<T> without allocating

717c508

Improve accuracy of TestHostAnalyzers

17ce912

Use explicit operators, since they are no longer used implicitly

c533f82

mavasani added 2 commits March 20, 2020 04:09

Ensure that we use "CodeStyleOption" as the serialization element nam…

6878054

…e for back compat.

Address feedback

f139867

sharwell reviewed Mar 20, 2020

View reviewed changes

mavasani added 2 commits March 20, 2020 13:25

Add matrix of tests for Get/Set option APIs on OptionSet and OptionSe…

6aa9798

…rvice for different combinations of our internal and public option types. Also fix the issue discovered from these tests.

Address feedback

81ff9eb

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

17fbad5

mavasani requested a review from jasonmalinowski March 20, 2020 20:55

sharwell approved these changes Mar 20, 2020

View reviewed changes

jinujoseph modified the milestones: 16.6.P3, 16.6 Mar 22, 2020

mavasani added 2 commits March 23, 2020 13:39

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

4c01f51

Fix conflicts

950b3b6

jasonmalinowski approved these changes Mar 24, 2020

View reviewed changes

mavasani added the auto-merge label Mar 24, 2020

ghost approved these changes Mar 24, 2020

View reviewed changes

ghost merged commit e16d390 into dotnet:master Mar 24, 2020

mavasani mentioned this pull request Mar 24, 2020

Follow-up to #42323 #42713

Merged

jinujoseph modified the milestones: 16.6, 16.6.P3 Mar 24, 2020

ghost pushed a commit that referenced this pull request Mar 24, 2020

Merge pull request #42713 from mavasani/FollowUp

d2e50fd

Follow-up to #42323

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Options and CodeStyle APIs to shared layer #42323

Move Options and CodeStyle APIs to shared layer #42323

mavasani commented Mar 10, 2020 •

edited

Loading

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020 •

edited by sharwell

Loading

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

jasonmalinowski Mar 19, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani Mar 10, 2020

mavasani commented Mar 20, 2020

mavasani commented Mar 20, 2020

jasonmalinowski commented Mar 24, 2020

jasonmalinowski Mar 24, 2020

mavasani Mar 24, 2020

jasonmalinowski Mar 24, 2020

mavasani Mar 24, 2020

mavasani commented Mar 24, 2020

mavasani commented Mar 24, 2020

ghost left a comment

tmat commented Mar 24, 2020

tmat commented Mar 24, 2020

Move Options and CodeStyle APIs to shared layer #42323

Move Options and CodeStyle APIs to shared layer #42323

Conversation

mavasani commented Mar 10, 2020 • edited 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

mavasani Mar 10, 2020 • edited by sharwell 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

mavasani commented Mar 20, 2020

mavasani commented Mar 20, 2020

jasonmalinowski commented Mar 24, 2020

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 commented Mar 24, 2020

mavasani commented Mar 24, 2020

ghost left a comment

Choose a reason for hiding this comment

tmat commented Mar 24, 2020

tmat commented Mar 24, 2020

mavasani commented Mar 10, 2020 •

edited

Loading

mavasani Mar 10, 2020 •

edited by sharwell

Loading