Merge pull request #42931 from mavasani/FixOptionCastException

Ensure that the public GetOption API returns the public CodeStyle opt…
dotnet · Apr 1, 2020 · a376b3e · a376b3e
2 parents b225471 + dea7a94
commit a376b3e
Show file tree

Hide file tree

Showing 18 changed files with 111 additions and 48 deletions.
diff --git a/...eatures/Core/Portable/CodeRefactorings/MoveType/AbstractMoveTypeService.MoveTypeEditor.cs b/...eatures/Core/Portable/CodeRefactorings/MoveType/AbstractMoveTypeService.MoveTypeEditor.cs
@@ -121,7 +121,7 @@ private async Task<Document> AddNewDocumentWithSingleTypeDeclarationAndImportsAs
             private async Task<SyntaxNode> AddFinalNewLineIfDesiredAsync(Document document, SyntaxNode modifiedRoot)
             {
                 var options = await document.GetOptionsAsync(CancellationToken).ConfigureAwait(false);
-                var insertFinalNewLine = options.GetOption(FormattingOptions.InsertFinalNewLine);
+                var insertFinalNewLine = options.GetOption(FormattingOptions2.InsertFinalNewLine);
                 if (insertFinalNewLine)
                 {
                     var endOfFileToken = ((ICompilationUnitSyntax)modifiedRoot).EndOfFileToken;

diff --git a/src/Features/Core/Portable/Diagnostics/AnalyzerConfigOptionSet.cs b/src/Features/Core/Portable/Diagnostics/AnalyzerConfigOptionSet.cs
@@ -24,7 +24,7 @@ public AnalyzerConfigOptionSet(AnalyzerConfigOptions analyzerConfigOptions, Opti
             _optionSet = optionSet;
         }
 
-        public override object GetOption(OptionKey optionKey)
+        private protected override object GetOptionCore(OptionKey optionKey)
         {
             // First try to find the option from the .editorconfig options parsed by the compiler.
             if (_analyzerConfigOptions.TryGetEditorConfigOption<object>(optionKey.Option, out var value))

diff --git a/src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs b/src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs
@@ -140,7 +140,13 @@ public static async ValueTask<T> GetOptionAsync<T>(this AnalyzerOptions analyzer
 #pragma warning disable CS0612 // Type or member is obsolete
             var optionSet = await analyzerOptions.GetDocumentOptionSetAsync(syntaxTree, cancellationToken).ConfigureAwait(false);
 #pragma warning restore CS0612 // Type or member is obsolete
-            return (T)optionSet?.GetOption(new OptionKey(option, language)) ?? (T)option.DefaultValue!;
+
+            if (optionSet != null)
+            {
+                value = optionSet.GetOption<T>(new OptionKey(option, language));
+            }
+
+            return value ?? (T)option.DefaultValue!;
         }
 
         [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/23582", OftenCompletesSynchronously = true)]

diff --git a/src/Features/Core/Portable/Wrapping/AbstractCodeActionComputer.cs b/src/Features/Core/Portable/Wrapping/AbstractCodeActionComputer.cs
@@ -78,7 +78,7 @@ public AbstractCodeActionComputer(
                 UseTabs = options.GetOption(FormattingOptions.UseTabs);
                 TabSize = options.GetOption(FormattingOptions.TabSize);
                 NewLine = options.GetOption(FormattingOptions.NewLine);
-                WrappingColumn = options.GetOption(FormattingOptions.PreferredWrappingColumn);
+                WrappingColumn = options.GetOption(FormattingOptions2.PreferredWrappingColumn);
 
                 var generator = SyntaxGenerator.GetGenerator(document);
                 NewLineTrivia = new SyntaxTriviaList(generator.EndOfLine(NewLine));

diff --git a/src/VisualStudio/Core/Impl/Options/AbstractOptionPreviewViewModel.cs b/src/VisualStudio/Core/Impl/Options/AbstractOptionPreviewViewModel.cs
@@ -67,29 +67,14 @@ protected AbstractOptionPreviewViewModel(OptionStore optionStore, IServiceProvid
 
         public void SetOptionAndUpdatePreview<T>(T value, IOption option, string preview)
         {
-            if (option is Option<CodeStyleOption<T>>)
+            var key = new OptionKey(option, option.IsPerLanguage ? Language : null);
+            if (option.DefaultValue is ICodeStyleOption codeStyleOption)
             {
-                var opt = OptionStore.GetOption((Option<CodeStyleOption<T>>)option);
-                opt.Value = value;
-                OptionStore.SetOption((Option<CodeStyleOption<T>>)option, opt);
-            }
-            else if (option is PerLanguageOption<CodeStyleOption<T>>)
-            {
-                var opt = OptionStore.GetOption((PerLanguageOption<CodeStyleOption<T>>)option, Language);
-                opt.Value = value;
-                OptionStore.SetOption((PerLanguageOption<CodeStyleOption<T>>)option, Language, opt);
-            }
-            else if (option is Option<T>)
-            {
-                OptionStore.SetOption((Option<T>)option, value);
-            }
-            else if (option is PerLanguageOption<T>)
-            {
-                OptionStore.SetOption((PerLanguageOption<T>)option, Language, value);
+                OptionStore.SetOption(key, codeStyleOption.WithValue(value));
             }
             else
             {
-                throw new InvalidOperationException("Unexpected option type");
+                OptionStore.SetOption(key, value);
             }
 
             UpdateDocument(preview);

diff --git a/src/VisualStudio/Core/Impl/Options/OptionStore.cs b/src/VisualStudio/Core/Impl/Options/OptionStore.cs
@@ -26,6 +26,7 @@ public OptionStore(OptionSet optionSet, IEnumerable<IOption> registeredOptions)
         }
 
         public object GetOption(OptionKey optionKey) => _optionSet.GetOption(optionKey);
+        public T GetOption<T>(OptionKey optionKey) => _optionSet.GetOption<T>(optionKey);
         public T GetOption<T>(Option<T> option) => _optionSet.GetOption(option);
         internal T GetOption<T>(Option2<T> option) => _optionSet.GetOption(option);
         public T GetOption<T>(PerLanguageOption<T> option, string language) => _optionSet.GetOption(option, language);

diff --git a/src/VisualStudio/Core/Test.Next/Mocks/TestOptionSet.cs b/src/VisualStudio/Core/Test.Next/Mocks/TestOptionSet.cs
@@ -23,7 +23,7 @@ private TestOptionSet(ImmutableDictionary<OptionKey, object> values)
             _values = values;
         }
 
-        public override object GetOption(OptionKey optionKey)
+        private protected override object GetOptionCore(OptionKey optionKey)
         {
             Contract.ThrowIfFalse(_values.TryGetValue(optionKey, out var value));
 

diff --git a/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs b/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs
@@ -33,6 +33,7 @@ public T Value
         ICodeStyleOption ICodeStyleOption.WithNotification(NotificationOption2 notification) => new CodeStyleOption<T>(Value, (NotificationOption)notification);
         ICodeStyleOption ICodeStyleOption.AsCodeStyleOption<TCodeStyleOption>()
             => this is TCodeStyleOption ? this : (ICodeStyleOption)_codeStyleOptionImpl;
+        ICodeStyleOption ICodeStyleOption.AsPublicCodeStyleOption() => this;
 
         public NotificationOption Notification
         {

diff --git a/src/Workspaces/Core/Portable/Options/DocumentOptionSet.cs b/src/Workspaces/Core/Portable/Options/DocumentOptionSet.cs
@@ -26,9 +26,12 @@ internal DocumentOptionSet(OptionSet backingOptionSet, string language)
             _language = language;
         }
 
-        public override object? GetOption(OptionKey optionKey)
+        private protected override object? GetOptionCore(OptionKey optionKey)
             => _backingOptionSet.GetOption(optionKey);
 
+        public new object? GetOption(OptionKey optionKey)
+            => base.GetOption(optionKey);
+
         public T GetOption<T>(PerLanguageOption<T> option)
             => _backingOptionSet.GetOption(option, _language);
 

diff --git a/src/Workspaces/Core/Portable/Options/OptionServiceFactory.cs b/src/Workspaces/Core/Portable/Options/OptionServiceFactory.cs
@@ -187,7 +187,7 @@ public DocumentSpecificOptionSet(List<IDocumentOptions> documentOptions, OptionS
                 }
 
                 [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/30819", AllowLocks = false)]
-                public override object? GetOption(OptionKey optionKey)
+                private protected override object? GetOptionCore(OptionKey optionKey)
                 {
                     // If we already know the document specific value, we're done
                     if (_values.TryGetValue(optionKey, out var value))

diff --git a/src/Workspaces/Core/Portable/Options/OptionSet.cs b/src/Workspaces/Core/Portable/Options/OptionSet.cs
@@ -22,40 +22,55 @@ public abstract partial class OptionSet
         /// </summary>
         private ImmutableDictionary<string, AnalyzerConfigOptions> _lazyAnalyzerConfigOptions = s_emptyAnalyzerConfigOptions;
 
+        private protected abstract object? GetOptionCore(OptionKey optionKey);
+
         /// <summary>
         /// Gets the value of the option, or the default value if not otherwise set.
         /// </summary>
-        public abstract object? GetOption(OptionKey optionKey);
+        public object? GetOption(OptionKey optionKey)
+            => OptionsHelpers.GetPublicOption(optionKey, GetOptionCore);
+
+        /// <summary>
+        /// Gets the value of the option cast to type <typeparamref name="T"/>, or the default value if not otherwise set.
+        /// </summary>
+        public T GetOption<T>(OptionKey optionKey)
+            => OptionsHelpers.GetOption<T>(optionKey, GetOptionCore);
 
         /// <summary>
         /// Gets the value of the option, or the default value if not otherwise set.
         /// </summary>
         internal object? GetOption(OptionKey2 optionKey)
-            => OptionsHelpers.GetOption<object?>(optionKey, GetOption);
+            => OptionsHelpers.GetOption<object?>(optionKey, GetOptionCore);
+
+        /// <summary>
+        /// Gets the value of the option cast to type <typeparamref name="T"/>, or the default value if not otherwise set.
+        /// </summary>
+        internal T GetOption<T>(OptionKey2 optionKey)
+            => OptionsHelpers.GetOption<T>(optionKey, GetOptionCore);
 
         /// <summary>
         /// Gets the value of the option, or the default value if not otherwise set.
         /// </summary>
         public T GetOption<T>(Option<T> option)
-            => OptionsHelpers.GetOption(option, GetOption);
+            => OptionsHelpers.GetOption(option, GetOptionCore);
 
         /// <summary>
         /// Gets the value of the option, or the default value if not otherwise set.
         /// </summary>
         internal T GetOption<T>(Option2<T> option)
-            => OptionsHelpers.GetOption(option, GetOption);
+            => OptionsHelpers.GetOption(option, GetOptionCore);
 
         /// <summary>
         /// Gets the value of the option, or the default value if not otherwise set.
         /// </summary>
         public T GetOption<T>(PerLanguageOption<T> option, string? language)
-            => OptionsHelpers.GetOption(option, language, GetOption);
+            => OptionsHelpers.GetOption(option, language, GetOptionCore);
 
         /// <summary>
         /// Gets the value of the option, or the default value if not otherwise set.
         /// </summary>
         internal T GetOption<T>(PerLanguageOption2<T> option, string? language)
-            => OptionsHelpers.GetOption(option, language, GetOption);
+            => OptionsHelpers.GetOption(option, language, GetOptionCore);
 
         /// <summary>
         /// Creates a new <see cref="OptionSet" /> that contains the changed value.

diff --git a/src/Workspaces/Core/Portable/Options/OptionsHelpers.cs b/src/Workspaces/Core/Portable/Options/OptionsHelpers.cs
@@ -36,5 +36,16 @@ public static T GetOption<T>(OptionKey optionKey, Func<OptionKey, object?> getOp
 
             return (T)value!;
         }
+
+        public static object? GetPublicOption(OptionKey optionKey, Func<OptionKey, object?> getOption)
+        {
+            var value = getOption(optionKey);
+            if (value is ICodeStyleOption codeStyleOption)
+            {
+                return codeStyleOption.AsPublicCodeStyleOption();
+            }
+
+            return value;
+        }
     }
 }
diff --git a/src/Workspaces/Core/Portable/Options/SerializableOptionSet.WorkspaceOptionSet.cs b/src/Workspaces/Core/Portable/Options/SerializableOptionSet.WorkspaceOptionSet.cs
@@ -37,7 +37,7 @@ private WorkspaceOptionSet(IOptionService service, ImmutableDictionary<OptionKey
             }
 
             [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/30819", AllowLocks = false)]
-            public override object? GetOption(OptionKey optionKey)
+            private protected override object? GetOptionCore(OptionKey optionKey)
             {
                 if (_values.TryGetValue(optionKey, out var value))
                 {

diff --git a/src/Workspaces/Core/Portable/Options/SerializableOptionSet.cs b/src/Workspaces/Core/Portable/Options/SerializableOptionSet.cs
@@ -106,7 +106,7 @@ public SerializableOptionSet WithLanguages(ImmutableHashSet<string> languages)
         }
 
         [PerformanceSensitive("https://github.com/dotnet/roslyn/issues/30819", AllowLocks = false)]
-        public override object? GetOption(OptionKey optionKey)
+        private protected override object? GetOptionCore(OptionKey optionKey)
         {
             if (_serializableOptionValues.TryGetValue(optionKey, out var value))
             {

diff --git a/src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt b/src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt
@@ -3,6 +3,11 @@ Microsoft.CodeAnalysis.Editing.DeclarationModifiers.IsVolatile.get -> bool
 Microsoft.CodeAnalysis.Editing.DeclarationModifiers.WithIsExtern(bool isExtern) -> Microsoft.CodeAnalysis.Editing.DeclarationModifiers
 Microsoft.CodeAnalysis.Editing.DeclarationModifiers.WithIsVolatile(bool isVolatile) -> Microsoft.CodeAnalysis.Editing.DeclarationModifiers
 Microsoft.CodeAnalysis.Editing.SyntaxGenerator.ElementBindingExpression(params Microsoft.CodeAnalysis.SyntaxNode[] arguments) -> Microsoft.CodeAnalysis.SyntaxNode
+Microsoft.CodeAnalysis.Options.DocumentOptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+*REMOVED*override Microsoft.CodeAnalysis.Options.DocumentOptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+Microsoft.CodeAnalysis.Options.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+*REMOVED*abstract Microsoft.CodeAnalysis.Options.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+Microsoft.CodeAnalysis.Options.OptionSet.GetOption<T>(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> T
 Microsoft.CodeAnalysis.Project.RemoveDocuments(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.DocumentId> documentIds) -> Microsoft.CodeAnalysis.Project
 Microsoft.CodeAnalysis.Solution.RemoveAdditionalDocuments(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.DocumentId> documentIds) -> Microsoft.CodeAnalysis.Solution
 Microsoft.CodeAnalysis.Solution.RemoveAnalyzerConfigDocuments(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.DocumentId> documentIds) -> Microsoft.CodeAnalysis.Solution

diff --git a/src/Workspaces/CoreTest/WorkspaceServiceTests/OptionServiceTests.cs b/src/Workspaces/CoreTest/WorkspaceServiceTests/OptionServiceTests.cs
@@ -316,25 +316,44 @@ private static void TestCodeStyleOptionsCommon<TCodeStyleOption>(IOption2 option
             var optionKey = new OptionKey(option, language);
             var optionKey2 = new OptionKey2(option, language);
 
-            //  1. WithChangedOption(OptionKey), GetOption(OptionKey)
-            var newOptionSet = originalOptionSet.WithChangedOption(optionKey, newValue);
-            Assert.Equal(newValue, newOptionSet.GetOption(optionKey));
+            // Value return from "object GetOption(OptionKey)" should always be public CodeStyleOption type.
+            var newPublicValue = newValue.AsPublicCodeStyleOption();
 
-            //  2. WithChangedOption(OptionKey), GetOption(OptionKey2)
+            //  1. WithChangedOption(OptionKey), GetOption(OptionKey)/GetOption<T>(OptionKey)
+            var newOptionSet = originalOptionSet.WithChangedOption(optionKey, newValue);
+            Assert.Equal(newPublicValue, newOptionSet.GetOption(optionKey));
+            // Value returned from public API should always be castable to public CodeStyleOption type.
+            Assert.NotNull((CodeStyleOption<bool>)newOptionSet.GetOption(optionKey)!);
+            // Verify "T GetOption<T>(OptionKey)" works for both cases of T being a public and internal code style option type.
+            Assert.Equal(newPublicValue, newOptionSet.GetOption<CodeStyleOption<bool>>(optionKey));
+            Assert.Equal(newValue, newOptionSet.GetOption<TCodeStyleOption>(optionKey));
+
+            //  2. WithChangedOption(OptionKey), GetOption(OptionKey2)/GetOption<T>(OptionKey2)
             newOptionSet = originalOptionSet.WithChangedOption(optionKey, newValue);
-            Assert.Equal(newValue, newOptionSet.GetOption(optionKey2));
+            Assert.Equal(newPublicValue, newOptionSet.GetOption(optionKey2));
+            // Verify "T GetOption<T>(OptionKey2)" works for both cases of T being a public and internal code style option type.
+            Assert.Equal(newPublicValue, newOptionSet.GetOption<CodeStyleOption<bool>>(optionKey2));
+            Assert.Equal(newValue, newOptionSet.GetOption<TCodeStyleOption>(optionKey2));
 
-            //  3. WithChangedOption(OptionKey2), GetOption(OptionKey)
+            //  3. WithChangedOption(OptionKey2), GetOption(OptionKey)/GetOption<T>(OptionKey)
             newOptionSet = originalOptionSet.WithChangedOption(optionKey2, newValue);
-            Assert.Equal(newValue, newOptionSet.GetOption(optionKey));
-
-            //  4. WithChangedOption(OptionKey2), GetOption(OptionKey2)
+            Assert.Equal(newPublicValue, newOptionSet.GetOption(optionKey));
+            // Value returned from public API should always be castable to public CodeStyleOption type.
+            Assert.NotNull((CodeStyleOption<bool>)newOptionSet.GetOption(optionKey)!);
+            // Verify "T GetOption<T>(OptionKey)" works for both cases of T being a public and internal code style option type.
+            Assert.Equal(newPublicValue, newOptionSet.GetOption<CodeStyleOption<bool>>(optionKey));
+            Assert.Equal(newValue, newOptionSet.GetOption<TCodeStyleOption>(optionKey));
+
+            //  4. WithChangedOption(OptionKey2), GetOption(OptionKey2)/GetOption<T>(OptionKey2)
             newOptionSet = originalOptionSet.WithChangedOption(optionKey2, newValue);
-            Assert.Equal(newValue, newOptionSet.GetOption(optionKey2));
+            Assert.Equal(newPublicValue, newOptionSet.GetOption(optionKey2));
+            // Verify "T GetOption<T>(OptionKey2)" works for both cases of T being a public and internal code style option type.
+            Assert.Equal(newPublicValue, newOptionSet.GetOption<CodeStyleOption<bool>>(optionKey2));
+            Assert.Equal(newValue, newOptionSet.GetOption<TCodeStyleOption>(optionKey2));
 
             //  5. IOptionService.GetOption(OptionKey)
             optionService.SetOptions(newOptionSet);
-            Assert.Equal(newValue, optionService.GetOption(optionKey));
+            Assert.Equal(newPublicValue, optionService.GetOption(optionKey));
         }
     }
 }
diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs
@@ -17,6 +17,9 @@ internal interface ICodeStyleOption
         ICodeStyleOption WithValue(object value);
         ICodeStyleOption WithNotification(NotificationOption2 notification);
         ICodeStyleOption AsCodeStyleOption<TCodeStyleOption>();
+#if !CODE_STYLE
+        ICodeStyleOption AsPublicCodeStyleOption();
+#endif
     }
 
     /// <summary>
@@ -53,11 +56,12 @@ public CodeStyleOption2(T value, NotificationOption2 notification)
         ICodeStyleOption ICodeStyleOption.WithValue(object value) => new CodeStyleOption2<T>((T)value, Notification);
         ICodeStyleOption ICodeStyleOption.WithNotification(NotificationOption2 notification) => new CodeStyleOption2<T>(Value, notification);
 
-        ICodeStyleOption ICodeStyleOption.AsCodeStyleOption<TCodeStyleOption>()
 #if CODE_STYLE
-            => this;
+        ICodeStyleOption ICodeStyleOption.AsCodeStyleOption<TCodeStyleOption>() => this;
 #else
+        ICodeStyleOption ICodeStyleOption.AsCodeStyleOption<TCodeStyleOption>()
             => this is TCodeStyleOption ? this : (ICodeStyleOption)new CodeStyleOption<T>(this);
+        ICodeStyleOption ICodeStyleOption.AsPublicCodeStyleOption() => new CodeStyleOption<T>(this);
 #endif
 
         private int EnumValueAsInt32 => (int)(object)Value;

diff --git a/...tilitiesAndExtensions/Compiler/Core/Options/EditorConfig/EditorConfigStorageLocation`1.cs b/...tilitiesAndExtensions/Compiler/Core/Options/EditorConfig/EditorConfigStorageLocation`1.cs
@@ -9,6 +9,7 @@
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
+using Microsoft.CodeAnalysis.CodeStyle;
 using Roslyn.Utilities;
 
 #if CODE_STYLE
@@ -104,6 +105,18 @@ string IEditorConfigStorageLocation2.GetEditorConfigString(object? value, Option
             => $"{KeyName} = {((IEditorConfigStorageLocation2)this).GetEditorConfigStringValue(value, optionSet)}";
 
         string IEditorConfigStorageLocation2.GetEditorConfigStringValue(object? value, OptionSet optionSet)
-            => GetEditorConfigStringValue((T)value!, optionSet);
+        {
+            T typedValue;
+            if (value is ICodeStyleOption codeStyleOption)
+            {
+                typedValue = (T)codeStyleOption.AsCodeStyleOption<T>();
+            }
+            else
+            {
+                typedValue = (T)value;
+            }
+
+            return GetEditorConfigStringValue(typedValue!, optionSet);
+        }
     }
 }