Treat files copied from a NuGet package content as generated

[Youssef1313]

When referencing a NuGet package with <IncludeContentInPack>true</IncludeContentInPack>, any warnings in that package can break the build. I think this should be treated in a special way (if possible).

There are two kind of issues here:

• Compiler warnings: I expect the warning to still exist as it's useful, but probably keep it as a warning even if TreatWarningsAsErrors is true? Not sure. I'd be okay with keeping the current behavior for this one.
• Analyzers: I'm expecting these files to be treated as generated files.

[jaredpar]

Not quite sure what the ask is here. The compiler considers code in a NuPkg file to be a part of the compilation, as if the code existed in the users project. This has been the design since basically the beginning.

What is the motivation for wanting to change this now? I'm very reluctant to introduce yet another type of code file over our current normal + generated code combination.

any warnings in that package can break the build

I don't see this as a particularly compelling argument. This has been true for many years now and it's failed to show up as a significant source of user feedback. It's also true for source generated files.

Customers can work around this by using a .editorconfig file to suppress the warnings in the NuPkg file. If this is not sufficient enough then I would push to making it easier to suppress warnings in NuPkg files vs. changing the way in which the compiler processes the files here.

Customers who distribute code via NuPkg are opting for a hard scenario here. It's expected that their code will need to be coded to a very high bar to be generally usable.

[Youssef1313]

Indeed, .editorconfig seems sufficient.

[Youssef1313]

I'm re-opening since .editorconfig was said to not work for dotnet/msbuild#7187

[elachlan]

Same with:

• SA1127 Generic type constraints should be on their own line msbuild#7267
• SA1505 An opening brace should not be followed by a blank line msbuild#7272

[elachlan]

Any movement on this? If not I might just continue forward with fixes upstream. The issue with this though is if the upstream package doesn't use the same analyzer rules and fail on warning. Then a build breaking change could happen at any time.

[jaredpar]

I'm still unclear on what the problem is here. The issue lacks specifics on a repro as do the linked issues. Think we need to have a concrete understanding of what the problem is to make any progress.

[elachlan]

I am enabling Code Analysis Analyzers on msbuild. MSBuild has a dependency in Microsoft.Build.Framework to the Microsoft.CodeAnalysis.Collections nuget package. That package adds code to the project which is then analyzed by the Code Analyzers.

Because MSBuild has TreatWarningsAsErrors set to true, this causes the build to fail. Since the code resides in the nuget package and hence is not editable, I cannot fix those errors.

So to fix those errors, we either fix the errors upstream and then update the nuget package; or find a way to supress the warnings for those files which are provided as a part of the nuget package.

[jaredpar]

Okay, I still don't see how this is a C# compiler bug though. The compiler is being passed files, an editor config, and is applying the rules as asked to those files. I don't understand the action being requested here.

[elachlan]

The fix suggested above was to apply a change to the .editorconfig to ignore those files.

[**/microsoft.codeanalysis.collections/**/*.cs]
# CA2208: Instantiate argument exceptions correctly
dotnet_diagnostic.CA2208.severity = none

This did not work.

I am unsure how we can exclude files provided from a nuget package from code analysis so our builds don't fail.

[jaredpar]

@mavasani

[mavasani]

@elachlan Just to confirm, the core issue you are facing is highlighted in dotnet/msbuild#7187 (comment), correct?

Few points to note:

1. You have couple of ways to configure this properly on your side:

   1. If you own the dependent NuGet package (microsoft.codeanalysis.collections in your case), then you should update the package to ship with an .editorconfig in it's source directory cone which has relevant entries to suppress the required analyzers. Note that you cannot suppress analyzer diagnostics in source files coming from a NuGet package with an .editorconfig present in your project's directory cone. Entries in .editorconfig files can only apply to source files within that directory cone, which is .editorconfig's core design principle.
   2. Your project can have a .globalconfig that turns off the required analyzers at project level, and then you can have a .editorconfig in your project or repo root that turn on these analyzers. By the precedence rules documented here, .editorconfig always overrides .globalconfig, so this will ensure analyzers are enabled for your sources in the repo, but disabled for all externally supplied sources from NuGet packages. I have attached a sample project that shows how you can use combination of .globalconfig + .editorconfig in your repo to serve your requirement: ClassLibrary54.zip

   IMO, approach ii is a better than i. Solution i above may not be feasible or scalable as your NuGet package might likely be included in different projects, each of which can have different 3rd party analyzer packages enabled, and the code supplied in the NuGet package is likely to have many violations for the analyzers in those packages.

2. As @jaredpar has stated in this issue and the related msbuild PR, all the .editorconfig and .globalconfig functioning in the compiler is as per design. I am converting this issue to a question, which can be closed once your queries have been satisfactorily answered.

[elachlan]

@mavasani thank you for the explanation an example project. It has been incredibly helpful!

It looks like I will need to rearchitect the analyzers in MSBuild to move back to .editorconfig from globalconfigs.

[Youssef1313]

Having to use both .editorconfig and .globalconfig is too much to suppress a warning IMO. I feel there should be an easier way.

Can't we get [**/microsoft.codeanalysis.collections/**/*.cs] to work?

[elachlan]

I think that is being discussed in #47384 ?

[Youssef1313]

@elachlan There is no source generators involved for this issue I believe.

[mavasani]

Can't we get [**/microsoft.codeanalysis.collections/**/*.cs] to work?

.editorconfig cannot apply entries to files outside the directory cone it resides in. We shouldn't violate the core principles of .editorconfig for this requirement.

[Youssef1313]

@mavasani Would it be possible to get it via ../? e.g [../../**/microsoft.codeanalysis.collections/**/*.cs] (assuming they're two directories up)

That way we're not violating how .editorconfig works currently (ie, it doesn't apply to parent directories), unless it was explicitly specified (considered as a new non-breaking feature)

[mavasani]

It looks like I will need to rearchitect the analyzers in MSBuild to move back to .editorconfig from globalconfigs.

@elachlan One way to avoid repetitive entries for each rule ID in both your .editorconfig and .globalconfig would be to specify the below entry in .globalconfig, which disables all analyzers by default in the project:

dotnet_analyzer_diagnostic.severity = none

You can then specify dotnet_diagnostic.RuleId.severity = <%severity%> in your .editorconfig for each rule you want to enable for your repo.

[mavasani]

Would it be possible to get it via ../

I don't think so. The moment we allow .editorconfig to specify sections that apply to files outside its directory cone with ../, every .editorconfig file on your file system can theoretically be applicable to every source file on your file system. This breaks project system's implicit discovery of applicable .editorconfig files by walking up the directory tree in a linear fashion. Also how do you resolve conflicts when there are conflicting entries in two editorconfigs in sibling directories of the repo root directory?

[elachlan]

Ideally, I'd like to exclude the source files provided via the nuget package from the global rules. That way the rules are kept centralised in the globalconfig (the file which is designed for analyzer rules). If there were some sort of syntax like the filter for editorconfig, it would help achieve that. Alternatively, just don't apply code analysis rules form globalconfigs to source files which are provided by nuget.

I do understand that the documentation says that AnalyzerConfig files are "options that apply to all the source files in a project, regardless of their file names or file paths". but my feeling is that if the file is essentially "locked" then I cannot change it, so the analyzer becomes a bit of a hindrance. I can see how it would be helpful for projects that would run security based code analysis, so that any code that is included is scanned before build.

An escape hatch for this would be incredibly useful, especially if it was targeted.

[Youssef1313]

@elachlan This is the ideal behavior I'd want, but as far as I understand, the issue here is that the compiler is passed source files, and it doesn't know whether it comes from NuGet or not.

For the security analyzers, it's not an issue at all if the compiler was able to determine if the file passed comes from NuGet. All security analyzers are configured to analyze generated code, while most of non-security analyzers doesn't analyze generated code.

https://github.com/dotnet/roslyn-analyzers/blob/f471d3381584f10f9908432e0b2b2b8ef07a0aa6/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Security/DoNotUseXslTransform.cs#L35-L36

https://github.com/dotnet/roslyn-analyzers/blob/f471d3381584f10f9908432e0b2b2b8ef07a0aa6/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/ImplementIDisposableCorrectly.cs#L128

[Youssef1313]

An idea (which could be bad but may inspire others to come up with a better resolution):

<PackageReference Include="PackageName" CopyContentToRoot="true" />

This will cause content files to be copied in whatever directory msbuild is invoked from, and these files will now be applicable to .editorconfig

(this won't be a roslyn thing)

[mavasani]

the issue here is that the compiler is passed source files, and it doesn't know whether it comes from NuGet or not.

Yes, this is the core issue here. You want to apply certain configuration to a group of files, that are no different to compiler than files coming from random directories in your file system. I don't believe we should introduce any notion in the compiler or editorconfig/globalconfig files to specially treat source files coming from NuGet package.

[mavasani]

Adding @chsienki, who might have more context on whether the feature being requested in #55992 (comment) is even technically possible. My gut feel says it is not, but Chris is the expert here.

[jaredpar]

as far as I understand, the issue here is that the compiler is passed source files, and it doesn't know whether it comes from NuGet or not.

Correct.

There was a lot of discussion early on about how to handle files in NuPgk and the decision was it is just source on disk. It is nothing special (even if we wanted it to be). If the developer puts NuPkg root such that an .editorconfig is above it on the directory tree then it must follow .editorconfig rules.

[chsienki]

Closing this as by design.

From the compiler point of view, nuget source files are just source files and should be treated the same way. I'm not sure how we'd even determine if a source file did come from a nuget package, as at the point of compilation they're effectively just regular files on disk.

We have existing mechanisms that allow you to address the issues raised here. .globalconfig can be used to configure source files outside of the directory cone, and .editorconfig can be used to override those preferences for files within the directory. It was previously problematic if a nuget package shipped a .globalconfig as you couldn't override it, but this was addressed by #49834

An alternative to using globalconfigs is to configure nuget to restore within your project directory. That way you can configure it via regular .editorconfig syntax.