dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.03k stars 4.03k forks source link

GlobalOptionService.SetOptions doesn't support non-WorkspaceOptionSet #15122

Closed tmat closed 1 week ago

tmat commented 7 years ago

See check here: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Options/GlobalOptionService.cs,101

Roslyn provides convenient infrastructure for managing options with various persistence models. GlobalOpionService seems to be the right type for managing options that are not specific to a particular workspace. Currently it's limited to WorkspaceOptionSet. I think it should provide abstraction independent of a workspace.

Also the implementation of GetAccessedOptions is suspicious: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Options/WorkspaceOptionSet.cs,58

        internal IEnumerable<OptionKey> GetAccessedOptions()
        {
            var optionSet = _service.GetOptions();
            return GetChangedOptions(optionSet);
        }

Although _service is typed to IOptionsService it seems that it is always gonna be OptionServiceFactory.OptionsService. GetOptions() returns new WorkspaceOptionSet(this), whcih returns an empty option set tied to the workspace service. So basically the call to GetChangedOptions(optionSet) starts with empty cached options, populates them using the backing service and then throws them away. Not sure what the intent is exactly, but perhaps it could be implemented in more straightforward way.

tmat commented 7 years ago

@jmarolf @jasonmalinowski FYI

Pilchie commented 7 years ago

@tmat Is there something you are trying to accomplish here? I'm unable to prioritize any action here based on your description above.

tmat commented 7 years ago

Yes, I am trying to use GlobalOptionService, but as it currently is implemented I can't. Instead I need to get OptionService from a Workspace, even though the code doesn't really depend on Workspace otherwise it needs to import it just to get OptionService.

tmat commented 7 years ago

Also, as pointed out above, the design and implementation looks suspicious and could use some cleanup.

CyrusNajmabadi commented 1 week ago

Closing out. We redid all this. If there's still more work to do we should call it out specifically.