Roslyn: Add compiler command line switch to provide compilation level editorconfig files for severity configuration

[mavasani]

Extracted from discussions in https://github.com/dotnet/roslyn/issues/42198. The core compiler feature request here is:

1. C# and VB compilers should provide a new command line switch, say /globalAnalyzerConfig:<%path_to_editorconfig%>, to provide a global .editorconfig file with dotnet_diagnostic severity entries that applies to the entire compilation, similar to ruleset files.
2. The dotnet_diagnostic severity entries from the global .editorconfig file are directly reflected in CompilationOptions.SpecificDiagnosticOptions, again similar to ruleset files.
3. Design should allow for more then one global .editorconfig file for different clients that may want to supply such a global configuration file (any SDK, analyzer authors, or even end users who want to ship .editorconfig files as part of NuGet package). Severity settings from global .editorconfig files should be applied in the order in which they are provided on the command line.

Open questions:

1. Should the global editorconfig files be outside the directory cone of the passed in source files, or does this not matter at all?
2. Can the same editorconfig file be passed in as the argument to both /analyzerConfig and /globalAnalyzerConfig?
3. Future work: Should the non-severity based analyzer config options be also applied from the global editorconfig files? We certainly don't need this functionality in the original design.

[mavasani]

Tag @agocke

[agocke]

Right now my theory is that we could pretend like this is an editorconfig at the root of the directory tree. If there's one already there, we could pretend like the existing one is concatenated onto the "global" one.

No idea if this actually works, though.

[agocke]

Severity settings from global .editorconfig files should be applied in the order in which they are provided on the command line.

That's probably unacceptable unfortunately. For MSBuild+NuGet this is effectively undefined ordering.

One way to get around this is to make the feature even simpler:

1. global analyzerconfig files cannot have section names. It's just a universal set of key=value pairs. These settings are presumed to apply to every source file in the compilation.

2. Ordering is undefined among global analyzer config files. All global analyzer config files are evaluated before other analyzer config files.

3. If two global analyzer config files set conflicting values for the same key, no value is set, and a compiler warning is provided.

[mavasani]

@agocke I like the simplicity of your proposal!

[mavasani]

Tagging @jasonmalinowski @CyrusNajmabadi who were interested in this...

[mavasani]

@agocke @jaredpar @vatsalyaagrawal @mikadumont - do we know when we plan to take this up? Would the compiler team accept a PR that follow's Andy's proposal from https://github.com/dotnet/roslyn/issues/42219#issuecomment-595899565? This feature is important for .NET analyzers package.

One open question: Do we require the global analyzer configs to be named ".editorconfig", given these are not implicitly discovered? I am wondering if they should in fact _not_ be named ".editorconfig" but require a non-empty name prefix to ensure clear distinction between document/folder based editorconfig files and compilation level editorconfig files. This also allows shipping a NuGet package with multiple global analyzer config files in the same folder, as opposed to being forced to create a separate sub-folder per global analyzer config.

[jaredpar]

do we know when we plan to take this up

Plan was to take it up pretty much immediately. It's the foundation of our next iteration of source genearators

[mavasani]

@jaredpar Thanks for the update!

[AArnott]

I'm very interested in a slight variation of the use case in the description. Here is mine:

I have an SDK that comes with a few of its own analyzers. It also selects a handful of analyzers from other analyzer packages (including those defined in-box with .NET 5 SDK) that my SDK wants to be turned on. Folks who reference only my package should get all the analyzers that I selected and no others. The fact that several analyzer packages are brought in as transitive dependencies is by design, but I don't want to have to infuse many hundreds of analyzers into my user's project in order to just get a few of the relevant ones that I know are particularly important/relevant to my SDK.
But if that consuming project also references those other analyzer packages, or otherwise opts into seeing those rules, then they should be allowed to get the rest of the default set of analyzers from those packages.

So in other words, by virtue of an analyzer package being brought in transitively through my package, I should get the freedom to filter to a subset of rules to activate by default, and possibly adjust their severities. But in doing so, this should not disable rules that I didn't select if the consuming project has otherwise expressed interest in them.

[mavasani]

@agocke @AArnott I think we can address your scenario if we adjust Andy's proposal in https://github.com/dotnet/roslyn/issues/42219#issuecomment-595899565 slightly as follows:

Instead of:

1. If two global analyzer config files set conflicting values for the same key, no value is set, and a compiler warning is provided.

We use:
_If two global analyzer config files set conflicting values for the same key, and one of them is disabling the rule (i.e. severity = none) and another is enabling it (i.e. severity other then none), then the enabling severity wins and is preferred. Otherwise, no value is set, and a compiler warning is provided for an unresolvable conflict._

@AArnott do you think that would address your scenario? Your package can enable the ones you care about and disable the rest. They only stay disabled until the user explicitly pulls in the other package which comes with a global analyzer config that turns them on explicitly.

[AArnott]

I don't think that would work.

That would put a new requirement on all other analyzer packages retroactively. It would no longer be enough to ship a package with an analyzer assembly with default severities set within the DiagnosticDescriptor. Each analyzer package would then have to 're-enforce' those severities by also publishing a global analyzer config file. Otherwise all their severities would be set to None by my package when the two came together, without a conflict.

Once they create a conflict by introducing this global file, what prevents their global file from always preempting mine? After all: my package chains in their package, so their file would always be present when mine is, making mine unable to disable them.

I think we need a proper concept of recognizing whether an analyzer package was referenced transitively through a 'filtering' package to solve this problem.

[mavasani]

I think we need a proper concept of recognizing whether an analyzer package was referenced transitively through a 'filtering' package to solve this problem.

@AArnott I think it would be better to file a separate issue for your feature request.

[mavasani]

@agocke @chsienki @jaredpar - I was wondering if instead of adding a new command line switch, can we instead re-use the existing additional files switch for this purpose? Essentially, any additional file with a ".editorconfig" extension would be treated as a global compilation level editorconfig for severity and/or options configuration. This would drastically reduce the work involved in multiple areas:

1. Project system and IDE: We already flow the additional files from project system down the IDE, which populates the Project.AdditionalFiles in the workspace and also passes them down to the compiler.
2. MSBuild: We will not need a new, special item, similar to AdditionalFiles, to flow this file down to the compiler from MSBuild.
3. Compiler: We will not need any new command line compiler switch or parsing logic. Only work involved would be reading the editorconfig additional files and populating the special diagnostic options before reading the ruleset.

What do you think?

[jaredpar]

Essentially, any additional file with a ".editorconfig" extension would be treated as a global compilation level editorconfig for severity and/or options configuration.

That would be a breaking change to some extent. I agree the chances are low but at the same time I'd rather do a global switch and avoid the risk here. Using the name match is just having an implicit global switch that is harder to track

[agocke]

Agreed with Jared. I actually think there's a lot of reasons you may want to pass editorconfig files as additional files and not have them processed differently.

[chsienki]

@agocke To pass the configs with a separate switch is going to require project system work, which we decided we'd like to avoid. Instead we decided to go with a single property that marks an analyzer config as global (is_global=true) instead.

Re: your plan to not allow sections, this doesn't work for the source generators msbuild work, because we need to be able to pass in metadata on files, for which we need sections. I propose the following idea instead:

• ~global analyzerconfig files cannot have section names. It's just a universal set of key=value pairs. These settings are presumed to apply to every source file in the compilation.~

• Ordering is undefined among global analyzer config files. All global analyzer config files are merged, then evaluated before other analyzer config files.

• If two global analyzer config files set conflicting values for the same global or section key no value is set, and a compiler warning is provided.

• Global analyzer configs are presumed to be at the 'root' of the directory tree.

Basically we collect all the global configs up, and make one 'meta' merged config at the root of the directory tree; we still treat this independently of the other files. This means we don't need to worry about 'overlapping' configs: they apply differently depending on if is_global is set.

Objections / issues with this approach?

[agocke]

Re: your plan to not allow sections, this doesn't work for the source generators msbuild work, because we need to be able to pass in metadata on files, for which we need sections. I propose the following idea instead

I don't really understand what this constraint means. How would this allow you to provide metadata for specific files? If the analyzer config is treated as appearing at the root directory, and section names are resolved relative to that, how could you know what path specification to set, as a library?

Also, 'root' isn't particularly well-defined for Windows. What if your compilation has source files spread across two drive letters?

[agocke]

Also, what happens when you produce conflicting values for two global configs with different section names? No diagnostic? And since they're reordered randomly the values just switch back and forth every build?

How are you going to provide the user a way to diagnose what's happening?

[chsienki]

I was thinking that the filenames would have to be absolute. Not really any use to the static config provided by analyzers from a NuGet package, but works perfectly for configs generated as part of MSBuild (like source generators are). Then we don't really need to care 'where' the config file is conceptually, it's essentially out of the source tree.

I'm not sure I understand your second point. If you have two global configs with section [a.cs] we just merge them. Any keys that collide are unset and a warning is issued. Same as global sections.

[agocke]

I was thinking that the filenames would have to be absolute

I think that needs to be part of the design, then. Absolute paths in the section names only.

If you have two global configs with section [a.cs] we just merge them

The section name [*.cs] and the section name [a.*] would both match your file.

[chsienki]

This is supported now via the is_global mechanism as of #43889