Design issues for speeding up .editorconfig processing

[jasonmalinowski]

Background

When we implemented .editorconfig support in the compiler, a SyntaxTree was given a collection of DiagnosticOptions. Before we parsed syntax trees, we would parse and process all .editorconfig files to collect the dotnet_diagnostic severity settings. Then, when parsing a tree we'd compute the .editorconfig rules that applied to that tree and parse the tree. Those trees all got added to the Compilation like anything else, and the compiler would consume that information when computing effective diagnostic severities.

The Problem

First there was just a performance issue in the original implementation: if a change happens to an .editorconfig that didn't actually impact the dotnet_diagnostic rules, we'd still reparse trees regardless. We can fix that, but it doesn't address some of the original concerns. If you are editing inside of the diagnostic code, say the CS4242 below:

dotnet_diagnostic.CS4242.severity = error

any single keystroke in there was still going to reparse the entire project. If this is happening in an .editorconfig that shared across all your projects, a single keystroke invalidates all syntax trees, and in turn all compilations.

Possible Solution 1: Don't Reparse

The reparsing was done because of a bit of a misunderstanding: the belief was the compiler was going to potentially apply changes to compiler warnings during parsing at that time, but the parsing wasn't actually dependent on it at all. So we can avoid reparsing trees, but a change to the .editorconfig would still require us to go to each syntax tree and create new compilations replacing all the trees; since those would expose new syntax roots the decltree caches in the compilations would have to be tossed out regardless. This would be an improvement over the current situation, but still means a single keystroke is forcing us to recreate a lot of information in each Compilation.

We didn't find a good way to save this option, but it's possible we missed something.

Possible Solution 2: Move this all to CompilationOptions

This is the approach that Andy explored in #44331 and we want to discuss further. This design:

• Obsoletes SyntaxTree.DiagnosticOptions
• Introduces a SyntaxTreeOptionsProvider, effectively an API for a single method that takes a syntax tree and returns the diagnostics that apply to it. (There's a bit more but not relevant for this conversation.)
• Adds a property to a CompilationOptions to hold onto the provider.
• Replaces the compiler's use of the syntax tree data and only uses the CompilationOptions information when computing effective diagnostics.

The upside of this approach is syntax trees don't need to change at all; the addition or changing of an .editorconfig only changes the compilation options, and the compiler could see that only the SyntaxTreeOptionsProvider is changing and not have to drop decltrees or other related caches, and we can be much more efficient here.

This however creates strangess in the workspaces layer. Today the Workspace doesn't have to provide any of the values in a CompilationOptions from other state elsewhere: the workspace host just creates the CompilationOptions and that's passed through to the Compilation. Now, there is bit of a cycle we have to deal with. Generally it seems we have to either:

1. Ignore the SyntaxTreeOptionsProvider given by the host, and overwrite it with one that is backed by the Workspace. Any change to Workspace requires us to update it. This means the object the host passes in is never "visible" from the workspace in any way.
2. Keep Project.CompilationOptions lined up to the object the host passes in, and instead have a different CompilationOptions actually given to the Compilation. This will surprise anybody who is ever comparing Project.CompilationOptions against Project.GetCompilation().CompilationOptions.

We went with option 1 for Andy's experiment, and then ran into the following issues/surprises:

1. Taking a Solution, changing an .editorconfig, and then comparing it back to the original solution now shows a change to the CompilationOptions, since a new SyntaxTreeOptionsProvider was made. This broke some solution crawler tests because they saw unexpected project changes, but the resulting behavior would have been correct.
2. Workspace.TryApplyChanges was initially rejected because they didn't support CompilationOption changes. It seemed like we'd have to make a special exception to recognize this case to avoid that break.
3. If a workspace does opt into CompilationOptions changes, we'd need another compat fix. We offer a hook where a workspace can say "do I support applying this CompilationOptions change?" The trick we did was this:

roslyn/src/VisualStudio/CSharp/Impl/Utilities/CSharpCompilationOptionsChangingService.cs

Lines 29 to 30 in 2a58f73

	// Currently, only changes to AllowUnsafe of compilation options are supported.
	return oldCSharpOptions.WithAllowUnsafe(newCSharpOptions.AllowUnsafe) == newOptions;

Which was an easy way to say "is the only thing that changed between these two options the supported values". It seems we now need call this method with newOptions now being newOptions.WithSyntaxTreeOptionsProvider(oldOptions.SyntaxTreeOptionProvider) so that way the existing code and hooks would continue to work, but that seems surprising and odd in it's own right.

Possible Solution 3: Don't put it in CompilationOptions, but put it somewhere else

We quickly discussed a third option to avoid all of this by making the SyntaxTreeOptionsProvider not be in CompilationOptions, but is directly held by the Compilation. As it stands the Compilation's only inputs are trees, references, and options, so we'd either be making the AnalyzerConfig a top-level input, or have to make another "options" type which seemed odd.

[agocke]

Note: 3 is probably a dead end. It's a big layering violation and would probably have too many other impacts, including potential perf problems.

[CyrusNajmabadi]

I don't actually understand any of the issues/surprises section. Would be happy to chat tomorrow about it.

Taking a Solution, changing an .editorconfig, and then comparing it back to the original solution now shows a change to the CompilationOptions

Seems totally reasonable. A change to a .editorconfig ends up as a change to the CompilationOptions for affected projects.

Workspace.TryApplyChanges was initially rejected because they didn't support CompilationOption changes. It seemed like we'd have to make a special exception to recognize this case to avoid that break.

Seems fine. If we want to support Workspace.TryApplyChanges applying .editorconfig changes, then we support that. If we don't, then we don't support it.

If a workspace does opt into CompilationOptions changes, we'd need another compat fix. We offer a hook where a workspace can say "do I support applying this CompilationOptions change?" The trick we did was this:

Honestly, is it hard to just support changing all compilation options in Workspace.TryApplyChanges? I think we did the minimal amount before, which started with:

1. not doing anything.
2. adding 'unsafe' so that we could have code-actions enable 'unsafe-mode' on a project.

I don't honestly see why we wouldn't want to support roundtripping effectively everything we could here.

[jasonmalinowski]

Note: 3 is probably a dead end. It's a big layering violation and would probably have too many other impacts, including potential perf problems.

@agocke: how is it a layering violation? I can imagine many impacts but I wouldn't have imagined that one. 😄

Honestly, is it hard to just support changing all compilation options in Workspace.TryApplyChanges?

@CyrusNajmabadi: It's just the standard time/value tradeoff; it's also host specific in that it requires specific project system APIs to change things.

Seems fine. If we want to support Workspace.TryApplyChanges applying .editorconfig changes, then we support that. If we don't, then we don't support it.

Anybody trying to modify the .editorconfig by modifying the file will still work fine, and we support that today. It's just there's extra wrinkles to support it now.

[agocke]

@jasonmalinowski AnalyzerConfig splits into two sets of options: analyzer options and diagnostic configs. In order to make this work we would have to push the analyzer configs into the compilation, then have the compilation pass the analyzer options back up in some opaque fashion to the analyzer driver. I'm not sure how we would deal with AdditionalFiles, which aren't present in the Compilation, but could have analyzer options provided.

[jasonmalinowski]

@agocke I don't see any reason we can't just pass the object again to the analyzer driver if that's the problem there.

[agocke]

So if it's just providing DiangosticOptions, why wouldn't it go in the CompilationOptions, which is where we store all the other diagnostic options settings?

[jasonmalinowski]

We decided to continue with the approach in Possible Solution 2, and deal with the workspace changes that fell out from it.