Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorconfig vs Global analyzerconfig #47707

Closed
4 tasks done
mavasani opened this issue Sep 15, 2020 · 12 comments
Closed
4 tasks done

Editorconfig vs Global analyzerconfig #47707

mavasani opened this issue Sep 15, 2020 · 12 comments

Comments

@mavasani
Copy link
Contributor

mavasani commented Sep 15, 2020

Editorconfig

  • Starting VS2019 16.3, Roslyn compiler and IDE supports analyzer and source generator configuration via regular .editorconfig files for configuration that applies to specific source files and/or folders within a project, applicable files and/or folders are selected by section headers.

  • Designed to work well will existing .editorconfig files used to configure editor style settings for different IDEs.

  • File/directory based discovery and configuration:

    • Implicitly discovered by the project system by walking the file system and passed to the compiler
    • All supported analyzer configuration entries must be under section headers to select applicable files and/or folders. Configuration entries not under a section header are ignored by the compiler.
  • Core scenarios: Support end user analyzer and source generator configuration via below entries:

    • Options: Key-value pair entries of the form key = value for analyzer config options passed to analyzers and source generators
    • Diagnostic severity settings: Certain well-known key value pairs as per the syntax here and here to configure diagnostic severity of analyzer diagnostics and compiler warnings.
  • Conflict resolution:

    • Regular editorconfig conflict resolution mechanism is applied so that editorconfig files deeper in the directory structure override the settings from editorconfig files that are higher up.
    • Highest precedence for diagnostic severity configuration entries: Overrides diagnostic severity settings that come from command line options (/nowarn, /warnaserror) and entries coming from ruleset or global analyzerconfig files.
  • Example:

# Remove the line below if you want to inherit .editorconfig settings from higher directories
root = true

# C# files
[*.cs]

#### Core EditorConfig Options ####

# Indentation and spacing
indent_size = 4
indent_style = space
tab_width = 4

#### .NET Coding Conventions ####

# this. and Me. preferences
dotnet_style_qualification_for_method = true:warning

#### Diagnostic configuration ####

# CA1000: Do not declare static members on generic types
dotnet_diagnostic.CA1000.severity = warning

Global analyzerconfig

  • Starting VS2019 16.7, Roslyn compiler and IDE supports special global, compilation level analyzer/source generator configuration files for specifying configuration that applies to all the source files in the compilation/project, regardless of the actual file names or file paths of these source files.

  • Unlike editorconfig files, these are not designed to configure editor style settings for different IDEs.

    • File name: No restrictions or requirements
    • File format:
      • Simple key-value pair configuration entries of the form key = value
      • Must contain a top level entry of the form is_global = true
      • Can contain section headers, as long as the headers specify an absolute file path. Section headers based on globbing or relative file paths are ignored.
  • File/directory agnostic configuration:

    • No implicit discovery: Need to be explicitly specified in the MSBuild targets/project file to be passed to the compiler:
      <ItemGroup>
        <EditorConfgFiles Include="<%path_to_global_analyzer_config%>" />
      </ItemGroup>
    • No globbing or relative file path based section headers allowed: All supported analyzer configuration entries must either be top level without section header OR section header must have an absolute file path. Entries under any section headers that are based on globs, say [*.cs] or [*], or section headers that use relative file paths are ignored.
  • Core scenarios: Support identical analyzer configuration entries as editorconfig files, but for different core scenarios:

    • Allow NuGet packages and SDKs to ship compilation level analyzer configuration files that apply to all source files in the project, regardless of the actual file names or file paths of these source files.
    • Allow passing in project-level MSBuild property values and item metadata to analyzers and source generators: Details here
    • Allow configuration of no-location analyzer diagnostics that do not have a file path and hence cannot be configured via regular editorconfig files.
  • Conflict resolution:

    • Entries with same key coming from different global analyzerconfig files are ignored and compiler generates warnings about each of these conflicts.
    • Lowest precedence for option configuration entries: For entries with same key coming from a global analyzerconfig and editorconfig file, the editorconfig file entry always overrides the global analyzerconfig entry
    • Lowest precedence for diagnostic severity configuration entries: Diagnostic severity settings that come from editorconfig files and those that come from command line options (/nowarn, /warnaserror) always override the entries from global analyzerconfig files.
  • Example:

# Top level entry required to mark this as a global analyzer config file
is_global = true

# NOTE: No section headers for configuration entries

#### .NET Coding Conventions ####

# this. and Me. preferences
dotnet_style_qualification_for_method = true:warning

#### Diagnostic configuration ####

# CA1000: Do not declare static members on generic types
dotnet_diagnostic.CA1000.severity = warning

Confusion points

The core points of confusion are:

  • Both file kinds have a similar end user goal of configuring analyzers and/or source generators, even though they have completely orthogonal purposes - path based vs path agnostic configuration for source files in the compilation.
  • Both files have similar format for the core configuration entries: key-value pair based entries, including support for the well-known diagnostic severity configuration entries
  • Use of same compiler switch for both files + Use of the editorconfig terminology in the MSBuild item name to specify global analyzerconfig files, i.e. <EditorConfgFiles Include="path_to_global_analyzer_config"/>. This was done to reduce the engineering cost on project system side, especially legacy project system. However, this makes the user feel the file name and format must match regular .editorconfig files, including use of globbing based section headers. For example, classic user mistake highlighted in the issue here.
  • Global analyzer config files can be named .editorconfig and things work just fine as long as entries abide by the global analyzerconfig's required format.
  • Global analyzer config files silently ignore analyzer configuration entries under section headers
  • Lack of tooling support in VS to create/edit global config files, so user has to manually do all the steps, making it hard to reason about the subtle differences between the two file formats.

Action items for future work

There are few action items that we plan to take up to reduce the above confusion:

@mavasani
Copy link
Contributor Author

Tagging @chsienki @jasonmalinowski

@jasonmalinowski
Copy link
Member

Awesome writeup, thanks!

mavasani added a commit to mavasani/docs that referenced this issue Sep 29, 2020
Global AnalyzerConfig is a new configuration file format for providing project level analyzer options. It works orthogonally with EditorConfig, which provides file/directory based options. See dotnet/roslyn#47707 for more detailed description. This PR primarily adds documentation from the same issue.

Additionally, it also adds documentation for precedence between options (severity options and non-severity options) coming from different configuration sources. This addresses dotnet/roslyn#47154.
mavasani added a commit to mavasani/docs that referenced this issue Sep 29, 2020
Global AnalyzerConfig is a new configuration file format for providing project level analyzer options. It works orthogonally with EditorConfig, which provides file/directory based options. See dotnet/roslyn#47707 for more detailed description. This PR primarily adds documentation from the same issue.

Additionally, it also adds documentation for precedence between options (severity options and non-severity options) coming from different configuration sources. This addresses dotnet/roslyn#47154.
@mavasani
Copy link
Contributor Author

@chsienki The only pending item here is Bug fix: We should update the GeneratedCodeUtilities heuristic to consider source files without a file path as generated files, currently they are considered as non-generated files: http://sourceroslyn.io/#Microsoft.CodeAnalysis/InternalUtilities/GeneratedCodeUtilities.cs,63. Feel free to close this issue once that is done.

@mavasani
Copy link
Contributor Author

mavasani commented Oct 9, 2020

I am going to close this issue now.

@paulomorgado
Copy link

How do I resolve MultipleGlobalAnalyzerKeys warnings? Do I have to explicitly ignore them?

@mavasani
Copy link
Contributor Author

@paulomorgado This requires @chsienki to implement #48634. Chris, this is now the third customer request for the same feature. The global configs that ship with the SDK are conflicting with end user's global config and we need precedence feature to allow end users to override the settings from the SDK.

@cmenzi
Copy link

cmenzi commented Nov 30, 2020

Hello

I've still some "confusion points":

  • Is the new <GlobalAnalyzerConfigFiles /> only working, if I use .NET 5 SDK or is it bound to VS 16.8+?
    • For example: Can I use it, if I build netstandard2.0 library when using sdk version 3.1.404 in global.json
      • Can I use Microsoft.CodeAnalysis.NetAnalyzers in this case?
  • The <EditorConfgFiles /> is equal to <GlobalAnalyzerConfigFiles /> and <GlobalAnalyzerConfigFiles /> is the way to go?
  • If I have .NET Framework projects, we should still use .ruleset files or does GlobalAnalyzerConfigFiles also works ?
    • Can I use Microsoft.CodeAnalysis.NetAnalyzers package or do I need to use Microsoft.CodeAnalysis.FxCopAnalyzers in this case?
  • Is there a plan that I can share .editorconfig (indent-size, indent-style, ...) also via a NuGet package?

@mavasani
Copy link
Contributor Author

Is the new <GlobalAnalyzerConfigFiles /> only working, if I use .NET 5 SDK or is it bound to VS 16.8+?

This is a compiler feature, not tied to any specific .NET SDK. It should work on any target framework as long as you have the required VS/compiler that supports this feature (VS2019 16.8 should have the required compiler).

For example: Can I use it, if I build netstandard2.0 library when using sdk version 3.1.404 in global.json

Depends on which compiler version you are using, it needs compiler version 3.7 or later.

Can I use Microsoft.CodeAnalysis.NetAnalyzers in this case?

Yes, this analyzer package is not tied to a specific target platform or .NET SDK.

Is there a plan that I can share .editorconfig (indent-size, indent-style, ...) also via a NuGet package?

No, by definition .editorconfig files are dependent on the folder location so shipping them inside a NuGet package does not make sense. This was one of the primary use cases for introducing folder agnostic global config files.

@Flash0ver
Copy link

  • Allow NuGet packages and SDKs to ship compilation level analyzer configuration files that apply to all source files in the project, regardless of the actual file names or file paths of these source files.

@mavasani is it possible to ship a .globalconfig file via a NuGet package?
I tried adding both a .globalconfig and a <PackageId>.globalconfig to the build folder of a NuGet package, but it didn't seem to be picked up by the consuming project.
Am I doing something wrong?

@mavasani
Copy link
Contributor Author

is it possible to ship a .globalconfig file via a NuGet package?
I tried adding both a .globalconfig and a .globalconfig to the build folder of a NuGet package, but it didn't seem to be picked up by the consuming project.

Tagging @jmarolf. I don't think we have logic in the SDK targets to automatically append globalconfig files in NuGet package's build folder to GlobalAnalyzerConfigFiles item group, which would resolve your scenario. I think we should, otherwise there is no good way to address this scenario other than using explicit paths to NuGet package cache on the machine. Maybe Jon has better suggestions for you.

@jmarolf
Copy link
Contributor

jmarolf commented Mar 22, 2022

correct @mavasani, unless you have explicitly set DiscoverGlobalAnalyzerConfigFiles to false in your project we include any msbuild item that is included in GlobalAnalyzerConfigFiles

<EditorConfigFiles Include="@(GlobalAnalyzerConfigFiles->Exists())" Condition="'$(DiscoverGlobalAnalyzerConfigFiles)' != 'false'" />

So in your nuget package you just need to include a props or targets file that looks like this

<Project>

  <ItemGroup>
    <GlobalAnalyzerConfigFiles Include="Relative/path/.globalconfig" />
  </ItemGroup>

</Project>

@fdalsotto
Copy link

I wonder why when enabling a rule like IDE0017, nothing comes up. Then, I open a file which I now the rule will be fired and that file starts getting itself tracked. I have StyleCop enabled but IDE entries don't have much to do with StyleCop, correct? Wonder what am I missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants