Design concerns around work being done with IDE Analzyers/Fxers/Refactorings

Asked to post this by @sharwell .

I'm concerned with some of the recent work i've seen go in around IDE analzyers, fixers and refactorings. Specifically the work around:

the 'code cleanup' feature.
generating a .editorconfig from feature options
updating an .editorconfig to suppress a feature

All of these recent pieces of work have shared a similar design flaw that I strongly believe needs to be addressed. Specifically, all of them operate by literal hardcoding information about certain features and how they behave into a different layer in the stack. For example:

https://github.com/dotnet/roslyn/blob/0c884ee6dc6891c97da5fc5b2e591735294c4cde/src/Features/CSharp/Portable/CodeCleanup/CSharpCodeCleanupService.cs#L43-L48

Here, Code-Cleanup hardcodes in knowledge of how use implicit/explicit type works. Another example is here:

https://github.com/dotnet/roslyn/blob/0c884ee6dc6891c97da5fc5b2e591735294c4cde/src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs#L229-L231

Where we assume that the first fix offered is the right way to fix any issue.

These sorts of hardcoding make our system very brittle and hard to maintain. It means that one cannot just go update a single feature in its appropriate location. Instead, one has to know the entire set of points in the entire IDE that need to be updated to make the feature properly work.

Similarly, this also means that the only way to provide IDE features that work properly with these other systems is to ship them out of roslyn itself. Say, for example, a third party had written the "add accessibility modifiers" feature. There would have been no way for them to integrate properly with these IDE subsystems.

Finally, because of this hard coding, we've effectively made it much harder to do things like light-up VB support. Instead of VB just lighting up naturally (if this had only involved changes at the individual feature level), we now need to specifically redo all this hard code knowledge for VB as well. This leaves VB falling further behind and increases the likelihood of it not getting these features, whihc increases hte likelihood of future designs becoming even more brittle since we won't do the work to properly design them to work across languages.

I think these approaches are acceptable in feature-branches, but should not become part of the main project. We've seen far too often that once integrated, resources are rarely spent to address these issues. When we want to actually roll this into the product, we should go and produce a design without tight coupling and brittleness. For example, instead of the way we did things here, we could instead have a new interface that analyzers/fixers could implement. For example, something like ISupportCodeCleanup. We would encode into that interface all the information the underlying system needs to run properly, and we would then implement that interface where appropriate. This would now mean that we could keep all feature logic in a specific location, and we would not be shutting out third parties from participating. IT would also mean that VB woudl light up naturally with C# since they so often use the same Analyzers/Providers.

Tagging @kuhlenh @jinujoseph @sharwell @mavasani

I believe the root problem lies in the core design of how the Workspace options are tightly coupled to the IDE providers (analyzers/fixers/refactorings). From the beginning, the mapping from providers to the associated option name/setting that affect its behavior has been hard coded in the provider code and no other parts of the IDE knows or maintains this mapping. This tight coupling to workspace options has led to a lot of unwanted hacks (and bugs) in the IDE diagnostic service, leaking of options/severities/suppression etc. knowledge into providers, etc.

I feel the correct design should be to first make all of our existing IDE providers completely on par with third party providers in terms of the APIs that they have access to, especially the analyzers. To accomplish this, workspace options will have to be decoupled from providers and there needs to a central place to store the mappings from providers to the options of interest (your primary request here), so that correct subset of options get threaded to each provider through public APIs on the passed in analysis/fix/refactoring context. Once we have such a central mapping, features such as code cleanup and editorconfig configuration can use it for a much cleaner implementation without explicit knowledge of individual options.

Given there are bunch of refactoring items here, I feel the correct ordering for it would be:

@sharwell comes up with a high level design of system where IDE providers are completely "NuGet"izable.
His implementation moves the mappings from provider to option into a common IDE service.
We cleanup all the features you mentioned here to use this service so they have no knowledge of individual options/mappings.

Regardless of how much I dislike us introducing more hardcoded knowledge into the features you mention, attempting to do 3. first might mean spending extra cycles designing and implementing this service upfront, which might again have to be redesigned once 1. is taken up. The fact whether all the above work should block the editorconfig related work flowing from feature branch to the master branch depends on how soon we want these features to be delivered versus us wanting to have the ideal design at the cost of delaying shipping the feature - @kuhlenh @jinujoseph should make this call.

I believe the root problem lies in the core design of how the Workspace options are tightly coupled to the IDE providers (analyzers/fixers/refactorings).

This is definitely one concern. However, even if we decoupled this, i woudl still want to ensure we don't then write features that are then strongly coupled to whatever new 'mapping' system we provide. As an example, the core CodeCleanup system should never have any concept of "i know about features X, Y and Z". The core cleanup system should just be around concepts like:

i know when i should run cleanup. (i.e. save, format, etc.)
i know how to discover what cleanup operations should run at those times.
i know how to expose a UI to users to let them configure things.
i know how to run a bunch of stuff in bulk, while still being efficient.

etc. etc. CodeCleanup itself should have no concept of things like "removing and sorting usings" and whatnot. this is true, regardless of how options are properly surfaced and threaded through the stack.

The fact whether all the above work should block the editorconfig related work flowing from feature branch to the master branch depends on how soon we want these features to be delivered versus us wanting to have the ideal design at the cost of delaying shipping the feature - @kuhlenh @jinujoseph should make this call.

Note: i'm strongly against us taking on large technical debt to ship features quickly to customers. We've been down that route many times in the past, and it just leads us to an inevitable future where we have ot throw away everything and start over from scratch. That's not hyperbole. It's what happens :)

These areas are critical for the future health of the Roslyn IDE. I'd argue they're going to be one of the top deliverables that Roslyn will have and will have to maintain for 10+ years or more. Having lots of debt pile up here will severely hamper any and all future efforts in this space. I've noticed a lot more tendency recently to take on debt and open bugs to say we'll address the debt later. To my knowledge, almost none of these debt-related bugs have actually been worked on. That's pretty deeply concerning to me. A debt-based strategy can work if debt is being paid off. It doesn't work if it's just allowed to pile up as eventually things come due and you find yourself boxed into a place where getting out from underneath it now unfeasible.

dotnet / roslyn

Design concerns around work being done with IDE Analzyers/Fxers/Refactorings #29345