dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

AnalyzerConfigFiles should be deduped, or else they cancel themselves out #60656

Open jimmylewis opened 2 years ago

jimmylewis commented 2 years ago

Version Used: VS Version 17.3.0 Preview 1.0 [32403.30.main]

Steps to Reproduce:

  1. Have a directory with a .globalconfiguration file, e.g. src/Languages/.globalconfig
  2. Have a separate directory for test code, e.g. test/Languages/TestProject/TestProject.csproj, test/Languages/TestProject2/TestProject2.csproj, etc.
  3. Include the globalconfig from point 1 in the test directory.build.props, e.g. in test/Langauges/Directory.Build.props
    <ItemGroup>
    <!-- test code should adhere to the same rules as product code -->
    <GlobalAnalyzerConfigFiles Include="$(ExtensionsSource)\Languages\.globalconfig" />
    </ItemGroup>
  4. Have one of the test project link a file from the src project tree

Expected Behavior: all test projects will build without issues

Actual Behavior: build fails on the project with the linked files, as the linked files bring in src\Languages.globalconfig on their own. This results in a duplicate entry passed to the compiler: image which in turn leads to all rules in that file being cancelled out by their own duplicate, e.g.

CSC warning MultipleGlobalAnalyzerKeys: Multiple global analyzer config files set the same key 'dotnet_diagnostic.sa1106.severity' in section 'Global Section'. It has been unset. Key was set by the following files: 'D:\src\WebTools\src\Languages.globalconfig, D:\src\WebTools\src\Languages.globalconfig' [D:\src\WebTools\test\Languages\TestServices\Microsoft.WebTools.Languages.TestServices.csproj]

(Note the file names are the same)

Workarounds: Option 1: Don't use Directory.Build.props to centralize this. The extra .globalconfig must be added to each test project individually that does not contain a linked file from the src tree. This is a pain as it has to be maintained on each project based on whether they contain linked files. Option 2: Don't use the file name .globalconfiguration to avoid auto-detection. Use a different name and set @(GlobalAnalyzerConfigFiles) in both of the Directory.Build.props (src\Languages and test\Languages). This will avoid picking up the originating config. This is less painful as it can still be accomplished via Directory.Build.props

sharwell commented 2 years ago

This appears to be an error in the build targets that include .editorconfig and .globalconfig files. While .editorconfig files should be included from the parents of all source files included in the compilation, .globalconfig should only be included from the ancestor directories of the project file location itself.

sharwell commented 2 years ago

@mavasani to coordinate the fix here

jimmylewis commented 2 years ago

It seems to me though that any means of including the same config file twice should not cause it to cancel itself out. This part specifically seems like an undesirable behavior in any circumstance.

Key was set by the following files: 'D:\src\WebTools\src\Languages.globalconfig, D:\src\WebTools\src\Languages.globalconfig'

sharwell commented 2 years ago

That question would be answered at the same time. However, note that if the issue I mentioned is fixed, the duplication problem never would have occurred, so it may not be a problem in the end.

mavasani commented 2 years ago

@chsienki