Ensure that the public GetOption API returns the public CodeStyle opt… #42931

mavasani · 2020-03-31T02:13:50Z

…ion value

Fixes InvalidCastException from #42923
We already ensured that we return the correct CodeStyle option value for T OptionSet.GetOption<T> overloads to fetch option. However, for object? OptionSet.GetOption(OptionKey), we did not ensure that the public CodeStyleOption is returned, which could lead to a cast exception if an external consumer tries to cast the return value to CodeStyleOption.

NOTE: Even though theoretically this change introduces a breaking change for all public sub-types of OptionSet by making an abstract public method a non-abstract method, it is not really a breaking change as OptionSet already has internal abstract methods, hence cannot be sub-typed outside Roslyn. I also verified none of our IVT partners sub-type OptionSet. Another possible approach here is to revert the abstract to non-abstract breaking API change, and call out (T)GetOption(optionKey) as an unsupported/deprecated operation in favor of invoking GetOption<T>(optionKey), which provides type safe access to option value. Former was relying on internal implementation details, which were never part of the Options API contract.

Verified the repro case + also unit test failure for the modified test before the fix.

…ion value Fixes dotnet#42923 We already ensured that we return the correct CodeStyle option value for `T OptionSet.GetOption<T>` overloads to fetch option. However, for `object? OptionSet.GetOption(OptionKey)`, we did not ensure that the public CodeStyleOption is returned, which could lead to a cast exception if an external consumer tries to cast the return value to CodeStyleOption. NOTE: Even though theoretically this change introduces a breaking change for all public sub-types of `OptionSet` by making an abstract public method a non-abstract method, it is not really a breaking change as `OptionSet` already has internal abstract methods, hence cannot be sub-typed outside Roslyn. I also verified none of our IVT partners sub-type `OptionSet`. Verified the repro case + also unit test failure for the modified test before the fix.

… for scenarios where caller wants a typed option value. Using `(T)GetOption(optionKey)` still works for public CodeStyleOption type, but is fragile approach that relied on internal implementation details. It should be discouraged in favor of `GetOption<T>(optionKey)`.

mavasani · 2020-03-31T13:42:12Z

src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt

@@ -3,6 +3,10 @@ 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.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+*REMOVED*override Microsoft.CodeAnalysis.Options.DocumentOptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+*REMOVED*abstract Microsoft.CodeAnalysis.Options.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object


NOTE: Even though theoretically this change introduces a breaking change for all public sub-types of OptionSet by making an abstract public method a non-abstract method, it is not really a breaking change as OptionSet already has internal abstract methods, hence cannot be sub-typed outside Roslyn. I also verified none of our IVT partners sub-type OptionSet.

roslyn/src/Workspaces/Core/Portable/Options/OptionSet.cs

Line 104 in 75fb12b

internal abstract IEnumerable<OptionKey> GetChangedOptions(OptionSet optionSet);

Another possible option here is to revert the abstract to non-abstract breaking API change, and call out (T)GetOption(optionKey) as an unsupported/deprecated operation in favor of invoking GetOption<T>(optionKey), which provides type safe access to option value. Former was relying on internal implementation details, which were never part of the Options API contract.

Restricting overrides of this method is not a breaking change for this type, so if the approach works then the approach works.

Agreed no concern here since this was never practically inheritable.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOption2`1.cs

...edUtilitiesAndExtensions/Compiler/Core/Options/EditorConfig/EditorConfigStorageLocation`1.cs

src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt

jasonmalinowski · 2020-04-01T00:09:50Z

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

+        public new object? GetOption(OptionKey optionKey)
+            => base.GetOption(optionKey);


This may warrant a good comment here explaining why this is here as a binary back-compat member.

Sure, will add in a follow-up PR.

jasonmalinowski · 2020-04-01T00:13:41Z

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)


Should this be called GetOption to match the GetOption<> overload above? Or name the other one? The core code in OptionSet.cs threw me off in that one called "GetPublicOption" and the rest didn't and that seemed like an oversight...

Sure, will rename and doc in a follow-up PR.

jasonmalinowski · 2020-04-01T00:16:55Z

src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt

@@ -3,6 +3,10 @@ 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.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+*REMOVED*override Microsoft.CodeAnalysis.Options.DocumentOptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
+*REMOVED*abstract Microsoft.CodeAnalysis.Options.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object


Agreed no concern here since this was never practically inheritable.

sandyarmstrong · 2020-04-08T18:05:06Z

@mavasani you said "I also verified none of our IVT partners sub-type OptionSet", but somehow VSmac got missed in this verification. Is there a list we need to get on?

mavasani · 2020-04-08T18:08:55Z

I looked at the internal VSO sources. Where is your repo? So you guys sub-type OptionSet? I am not sure that was intended to be a supported scenario...

sandyarmstrong · 2020-04-08T18:11:29Z

I'll ping you with the URL. I understand that internal API usage is not supported but when I saw the comment about verification I just wanted to make sure VSmac was taken into account in the future. This API break is not a big deal for us, so no worries on that account.

dotnet#42931 added this API as we thought converting the base `OptionSet` type's abstract method to non-abstract and removing the override on `DocumentOptionSet` would be a binary breaking change for callers. However, based on the public API review meeting, it was identified that it would indeed not be a breaking change as the compiler would have emitted a callvirt to `OptionSet.GetOption`.

mavasani added the Area-IDE label Mar 31, 2020

mavasani added this to the 16.6 milestone Mar 31, 2020

mavasani requested review from jasonmalinowski, sharwell and CyrusNajmabadi March 31, 2020 02:13

mavasani added 4 commits March 30, 2020 19:58

Fix build

f7c26a2

Fix TestOptionSet

7517cce

Update doc comment

44c4de6

mavasani commented Mar 31, 2020

View reviewed changes

Fix one more location with an explicit cast

ee7d534

mavasani marked this pull request as ready for review March 31, 2020 17:27

mavasani requested a review from a team as a code owner March 31, 2020 17:27

sharwell approved these changes Mar 31, 2020

View reviewed changes

mavasani added 2 commits March 31, 2020 11:15

Address feedback

73e1eaf

Fix build

dea7a94

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

jasonmalinowski approved these changes Apr 1, 2020

View reviewed changes

mavasani merged commit a376b3e into dotnet:master Apr 1, 2020

ghost modified the milestones: 16.6.P3, Next Apr 1, 2020

mavasani deleted the FixOptionCastException branch April 1, 2020 00:19

sharwell modified the milestones: Next, temp, 16.6.P3 Apr 6, 2020

mavasani mentioned this pull request Apr 16, 2020

Remove public API on DocumentOptionSet #43428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that the public GetOption API returns the public CodeStyle opt… #42931

Ensure that the public GetOption API returns the public CodeStyle opt… #42931

mavasani commented Mar 31, 2020 •

edited

Loading

mavasani Mar 31, 2020

mavasani Mar 31, 2020 •

edited

Loading

sharwell Mar 31, 2020

jasonmalinowski Apr 1, 2020

jasonmalinowski Apr 1, 2020

mavasani Apr 1, 2020

jasonmalinowski Apr 1, 2020

mavasani Apr 1, 2020

jasonmalinowski Apr 1, 2020

sandyarmstrong commented Apr 8, 2020

mavasani commented Apr 8, 2020

sandyarmstrong commented Apr 8, 2020

		public new object? GetOption(OptionKey optionKey)
		=> base.GetOption(optionKey);

Ensure that the public GetOption API returns the public CodeStyle opt… #42931

Ensure that the public GetOption API returns the public CodeStyle opt… #42931

Conversation

mavasani commented Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

mavasani Mar 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandyarmstrong commented Apr 8, 2020

mavasani commented Apr 8, 2020

sandyarmstrong commented Apr 8, 2020

mavasani commented Mar 31, 2020 •

edited

Loading

mavasani Mar 31, 2020 •

edited

Loading