Roslyn: make Solution.Options actually part of solution snapshot

Created on 5 May 2017 · 7Comments · Source: dotnet/roslyn

right now, Solution.Options is not actually part of snapshot but use Workspace.Options

we should make it as part of solution snapshot. which means when Workspace option is changed, new CurrentSolution should be created. and all normal process should begin from there such as workspace changed events and etc.

OptionSet should support checksum and support creating checksum without reading all options in to make creating checksum for solution efficient.

Area-IDE Concept-OOP Need Design Review

Source

heejaechang

Most helpful comment

Tagging @tmat @sharwell @jasonmalinowski @jinujoseph @vatsalyaagrawal - I would like to discuss the open design questions posted in https://github.com/dotnet/roslyn/issues/19284#issuecomment-566173145 in today's design meeting. I have a working implementation in https://github.com/dotnet/roslyn/pull/40289, but need a resolution on API design before proceeding.

mavasani on 16 Dec 2019

👍3

All 7 comments

this depends on how @jasonmalinowski decide on options.

heejaechang on 23 May 2017

if we can endure prefetch perf cost of options (assume we make option async API) as we did for file content hashing for OOP, then adding options in solution would be quite straight forward. after this 1 time cost, updating option as time go will be incremental, so won't be big issue I believe.

but it is not all cost since there are these benefits (probably more)

our data API will be truly data API. right now, it can return different result for same snapshot based on options.
we can remove bunch of special handling on options to reanalyze stuff to handle snapshot stay same but option changed case that causes changes in end result.

ex)
code related to this can all be removed since option change will just update snapshot
http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.cs,162
http://sourceroslyn.io/#Microsoft.CodeAnalysis.Remote.Workspaces/Diagnostics/DiagnosticComputer.cs,76

no need to do special option serialization anymore
http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs,164
cache based on snapshot now accommodate options as well. previously, we might have a bug due to cache being only based on snapshot when result could be changed due to options.

and etc.

heejaechang on 7 Dec 2019

@mavasani FYI

tmat on 7 Dec 2019

Yes, we should just snapshot the options. If we have multiple sources or groups of options and some of them might take longer to retrieve then we could model them as a tree and fetch nodes on demand. But I would just start with a single bunch.

tmat on 7 Dec 2019

API semantics

Current

Workspace.Options.get and Solution.Options.get are public APIs that fetch the global options from IOptionService. Both these getters always return the same (equivalent) option set.
Workspace.Options.set is the only public API that sets the global options in IOptionService, which is internal.

Workspace.cs

       /// <summary>
        /// Gets or sets the set of all global options.
        /// </summary>
        public OptionSet Options
        {
            get
            {
                return _services.GetRequiredService<IOptionService>().GetOptions();
            }

            set
            {
                _services.GetRequiredService<IOptionService>().SetOptions(value);
            }
        }

Solution.cs

        /// <summary>
        /// Returns the options that should be applied to this solution. This is equivalent to <see cref="Workspace.Options" /> when the <see cref="Solution"/> 
        /// instance was created.
        /// </summary>
        public OptionSet Options
        {
            get
            {
                // TODO: actually make this a snapshot
                return this.Workspace.Options;
            }
        }

New

Desired functionality changes:

Options are part of the _solution state_, such that they are created and saved on the Solution snapshot at the time the solution snapshot is created.
Currently, components running in OOP do not have access to latest options. They instead get the default options, unless the feature itself serializes/deserializes a hard coded list of option values. With this change, options automatically round trip to the RemoteWorkspace as they are part of the solution state and all features running OOP get correct option values.

API semantic changes:

Options fetched from the Workspace fetches options attached to CurrentSolution snapshot.
Options fetched from the Solution fetches options created at the time that solution snapshot was created.
Options fetched from any Solution instance can differ from the current global options or options fetch from a different solution snapshot.
Setting options on the Workspace updates the global option on IOptionService and then creates a new Solution instance with updated options, and updates Workspace.CurrentSolution to be this new solution instance.
Expose a public API on Solution to create a new solution snapshot with given options.

Open design question

How do we want to handle any semantic breaks for existing public APIs: Workspace.Options.get, Workspace.Options.set and Solution.Options.get?

Deprecate all these 3 APIs, while retaining their current behavior + Introduce completely new APIs , say GetOptions and SetOptions pairs and both Workspace and Solution, that are as per the new snapshot based semantics. I have an API proposal below.
Deprecate only the Workspace.Options.set API, as that is the only public setter API, in favor of new Workspace.SetOptions API that documents the new semantics, especially that underlying CurrentSolution will be been updated. Workspace.Options.get and Solution.Options.get are re-purposed to returns snapshot based options. If we take this route, there can still be silent semantic breaks:
1. Workspace.Options.get and Solution.Options.get now no longer return equivalent options, so if user attempts to to fetch options from older solution snapshot, they get previous options.
2. Workspace.TryApplyChanges might fail if the caller had done following:
  1. Fetch current solution instance and create a new solution from it with some document/project changes.
  2. Update workspace options (and hence the underlying CurrentSolution) so that Solution.WorkspaceVersion is bumped up for the current solution.
  3. Invoke Workspace.TryApplyChanges with new solution created in first step above. This solution will have a lesser workspace version due to second step, hence TryApplyChanges will fail.
Don't deprecate any existing APIs, but repurpose them, while preventing breaking side effects:
1. Update Workspace.TryApplyChanges to be tolerant to option changes. This can be done by either of the below ways:
  1. Do not update the solution's WorkspaceVersion when changing options. Q: Can this have other bad side effects?
  2. Or, update TryApplyChanges to ignore only option changes between solution, and allow updates even if WorkspaceVersion goes back in this case. This might be tricky to implement.
2. Documented breaking changes due to bad assumptions by the API users:
  1. Workspace.Options.get and Solution.Options.get may no longer return equivalent options and
  2. Solution.Options.get is repurposed for the new functionality.
    
    Given that solution should always be treated as immutable, any caller that is broken by the above changes was already going against the solution immutability principle due to this bug, and ought to be fixed.

Proposal (based on approach 1. above)

Workspace.cs

        /// <summary>
        /// Gets or sets the set of all global options.
        /// </summary>
        [Obsolete("Use GetOptions/SetOptions APIs")]
        public OptionSet Options
        {
            get
            {
                return _services.GetRequiredService<IOptionService>().GetOptions();
            }

            set
            {
                _services.GetRequiredService<IOptionService>().SetOptions(value);
            }
        }

        /// <summary>
        /// Gets the options for the `CurrentSolution`s
        /// </summary>
        public OptionSet GetOptions() => this.CurrentSolution.GetOptions();

        /// <summary>
        /// Sets the global options and updates the underlying `CurrentSolution` to be a new `Solution` instance with the updated options.
        /// </summary>
        public OptionSet SetOptions()
        {
            if (_optionService.SetOptions(options))
            {
                UpdateCurrentSolutionOnOptionsChanged();
            }
        }

Solution.cs

        /// <summary>
        /// Returns the options that should be applied to this solution. This is equivalent to <see cref="Workspace.Options" /> when the <see cref="Solution"/> 
        /// instance was created.
        /// </summary>
        [Obsolete("Use GetOptions/WithOptions APIs")]
        public OptionSet Options
        {
            get
            {
                // TODO: actually make this a snapshot
                return this.Workspace.Options;
            }
        }

        /// <summary>
        /// Gets the options attached to this Solution instance at the time this Solution was created.
        /// NOTE: The options returned may not be equivalent to `Workspace.Options`, which returns the options attached to `Workspace.CurrentSolution`
        /// </summary>
        public OptionSet GetOptions() => _state.Options;

        /// <summary>
        /// Creates a new solution instance with the specified <paramref name="options"/>.
        /// </summary>
        public Solution WithOptions(OptionSet options)
        {
            var newState = _state.WithOptions(options: options);
            if (newState == _state)
            {
                return this;
            }

            return new Solution(newState);
        }

mavasani on 16 Dec 2019

👍3

@mavasani One question I do have (although you can save the answer for the design meeting rather than furiously typing it in the next 10 minutes): which compat issues have you observed while working on this? Is this a situation where we're mostly trying to predict compat issues or do we know of some breaking problems already in the wild?