Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
8 changes: 7 additions & 1 deletion src/Features/Core/Portable/Diagnostics/AnalyzerHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions src/VisualStudio/Core/Impl/Options/OptionStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/VisualStudio/Core/Test.Next/Mocks/TestOptionSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down
1 change: 1 addition & 0 deletions src/Workspaces/Core/Portable/CodeStyle/CodeStyleOption.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
2 changes: 1 addition & 1 deletion src/Workspaces/Core/Portable/Options/DocumentOptionSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ 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 T GetOption<T>(PerLanguageOption<T> option)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
27 changes: 21 additions & 6 deletions src/Workspaces/Core/Portable/Options/OptionSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions src/Workspaces/Core/Portable/Options/OptionsHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var value = getOption(optionKey);
if (value is ICodeStyleOption codeStyleOption)
{
return codeStyleOption.AsPublicCodeStyleOption();
}

return value;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
4 changes: 4 additions & 0 deletions src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
mavasani marked this conversation as resolved.
Show resolved Hide resolved
*REMOVED*abstract Microsoft.CodeAnalysis.Options.OptionSet.GetOption(Microsoft.CodeAnalysis.Options.OptionKey optionKey) -> object
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Contributor Author

@mavasani mavasani Mar 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed no concern here since this was never practically inheritable.

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
Expand Down
41 changes: 30 additions & 11 deletions src/Workspaces/CoreTest/WorkspaceServiceTests/OptionServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -58,6 +61,8 @@ ICodeStyleOption ICodeStyleOption.AsCodeStyleOption<TCodeStyleOption>()
=> this;
#else
=> this is TCodeStyleOption ? this : (ICodeStyleOption)new CodeStyleOption<T>(this);

ICodeStyleOption ICodeStyleOption.AsPublicCodeStyleOption() => new CodeStyleOption<T>(this);
mavasani marked this conversation as resolved.
Show resolved Hide resolved
#endif

private int EnumValueAsInt32 => (int)(object)Value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.CodeStyle;
using Roslyn.Utilities;

#if CODE_STYLE
Expand Down Expand Up @@ -104,6 +105,18 @@ string IEditorConfigStorageLocation2.GetEditorConfigString(object? value, Option
=> $"{KeyName} = {((IEditorConfigStorageLocation2)this).GetEditorConfigStringValue(value, optionSet)}";

string IEditorConfigStorageLocation2.GetEditorConfigStringValue(object? value, OptionSet optionSet)
mavasani marked this conversation as resolved.
Show resolved Hide resolved
=> GetEditorConfigStringValue((T)value!, optionSet);
{
T typedValue;
if (value is ICodeStyleOption codeStyleOption)
{
typedValue = (T)codeStyleOption.AsCodeStyleOption<T>();
}
else
{
typedValue = (T)value;
}

return GetEditorConfigStringValue(typedValue!, optionSet);
}
}
}