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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Apr 1, 2020
Merged

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

merged 8 commits into from
Apr 1, 2020

Commits on Mar 31, 2020

  1. Ensure that the public GetOption API returns the public CodeStyle opt… 

    …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.
    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    480288e View commit details
    Browse the repository at this point in the history

  2. Fix build

    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    f7c26a2 View commit details
    Browse the repository at this point in the history

  3. Fix TestOptionSet

    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    7517cce View commit details
    Browse the repository at this point in the history

  4. Add new API public T GetOption<T>(OptionKey optionKey) to OptionSet… 

    … 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
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    ea7e5e7 View commit details
    Browse the repository at this point in the history

  5. Update doc comment

    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    44c4de6 View commit details
    Browse the repository at this point in the history

  6. Fix one more location with an explicit cast

    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    ee7d534 View commit details
    Browse the repository at this point in the history

  7. Address feedback

    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    73e1eaf View commit details
    Browse the repository at this point in the history

  8. Fix build

    @mavasani
    mavasani committed Mar 31, 2020
    Configuration menu
    Copy the full SHA
    dea7a94 View commit details
    Browse the repository at this point in the history