.editorconfig diagnostic suppressions don't apply to source-generated files

[jasonmalinowski]

The failures in #47252 demonstrate this, I think. We have this set in our .editorconfig:

roslyn/.editorconfig

Lines 221 to 222 in 4602d5d

	# warning RS0037: PublicAPI.txt is missing '#nullable enable'
	dotnet_diagnostic.RS0037.severity = none

When we had the file checked in explicitly, then I'm guessing this took effect; when it's converted to source generator output, we're now getting warnings from the source-generated file.

It's not clear to me if the real fix here is we need to switch some of those settings to be global suppressions so they apply everywhere.

[sharwell]

This particular rule should be in a global .editorconfig. Most rules are excluded from source generators on the grounds that they are considered generated code. I'm not sure we need to support more than this, but if there is a specific proposal we could review it.

[jaredpar]

Can you elaborate on the context in which you see this rule: when building on the command line, when opening the synthensized file in the IDE, etc ...?

[chsienki]

I think this is essentially a dupe of #44223

We're not parsing the generated files according the the editorconfig settings

[chsienki]

Oh, hang on. Yeah this is sort of by design I think. The generated files essentially live outside of the source tree completely (I mean, in memory they might not even have a path), so normal editor config rules are never going to match them.

We could make [*] match them, but that'd technically be against the editoroconfig spec, becuase that really means 'everything in this dir, recursively' essentially [<pathToConfig>/*] so matching them would be incorrect.

Global .editorconfig is the right mechanism here, it says 'here are the rules that apply to things without any path', which in this case includes the generated files.

Now, that being said, is that a good experience for a customer? I don't know; essentially you're only allowed a single set of rules for generated files, even in totally different parts of your source tree, which is maybe not ideal.

I suspect one option here would be to give the generated files a 'virtual' path of whatever csproj they're being generated into. That gives us a known, stable, location on disk (that we can say to customers, 'oh they match here') and allows you to customize the rules based on file path, without breaking the spirit of editorconfig.

[jasonmalinowski]

So what happens if we change the behavior and say that a global .editorconfig has rules outside a glob that are global, but the globs still otherwise apply normally? In other words, it only controls the interpretation of the stuff outside a glob. I recognize there's an intent here to somehow let the global ones also apply globs to absolute files, but is that a common scenario? or, at least more common than what I'm trying to to do here?

[chsienki]

I think the design of global editor configs is sound, and makes sense for what it was designed for. I think that generated files not matching regular editorconfigs is the real issue here.

Global configs get merged at read-time. We take all the global configs and merge them into a single 'root-less' config. How would globs work then? We'd need to somehow know the roots of the constituent parts. Absolute paths are the only thing that really makes sense in that cases (and we already have a dependency on it for the MSBuild -> analyzerOptions stuff).

[jasonmalinowski]

How would globs work then?

I guess my thought was the globs are just still treated as globs relative to the position of the .editorconfig; basically the only thing is is_global impacts the interpretation of stuff prior to the globs and that's it.

[mavasani]

@jasonmalinowski just to confirm:

1. Did you make sure to add is_global = true to the config file?
2. Are you using the latest compiler bits after Modify global analyzer config precedence: #45871 was merged? That PR fixed a bunch of issues related to respecting diagnostic configurations in global config.

[jasonmalinowski]

@mavasani you can take a look at the PR here. It's upgrading the toolset to be much newer bits, like the last week or two, so it'd have your PR. Basically, adding is_global works, but seems to implicitly change the globbing behavior of the globs so they no longer match regular files for the stuff outside the global section, and I don't understand what scenario we're trying to enable with that.

[mavasani]

@jasonmalinowski I believe you will need to create a separate configuration file for global config - I don’t believe a single file can have both global entries and also serve as directory based editorconfig @chsienki to confirm. I would instead suggest the following:

1. Create a separate file, say “global.editorconfig”, either at repo root or somewhere within eng directory.
2. Move all global entries to this file. Make sure there are no section headers in this file, entries under sections will be ignored.
3. Explicitly add this as an Editorconfig item for the entire repo in a targets file.

[jasonmalinowski]

Yeah, that seems really clumsy. I wasn't sure how many are really taking advantage of globbing support in global files to make this design really make sense -- it means anybody who is consuming generators and wants to do what I'm trying to do doesn't have a sane path forward.

Proposal: if we do have enough users for the global globbing needs, should we split out a second flag? is_global means the properties outside a glob section applies to all files, but how the globs apply to files don't change. is_global_file_names or something is a second flag to opt into the change of behavior around files?

[jaredpar]

This issue has gotten a bit stale. Is the status quo suffecient (seems arguable given we've not had any discussion in 1+ years). If so should we close this out? Or do we need to push for a solution here?

[sharwell]

We have a big education gap, and a gap in the EditorConfig UI here. Situations where .ruleset applied previously need to be implemented using .globalconfig (see e.g. dotnet/msbuild#7192), but currently people are generally trying to implement them with .editorconfig. Even in this repository we've failed to leverage global_level to reduce duplication in our .globalconfig files.