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.05k stars 4.04k forks source link

Analyzers should ignore content files added by nuget packages #6736

Open yishaigalatzer opened 9 years ago

yishaigalatzer commented 9 years ago

NuGet 3.3 re-introduces support for content files. In this version they can be added to the project dynamically and are never supposed to change.

Analyzers should treat these files the same as generated files and not suggest fixes.

bording commented 3 years ago

@mavasani Your work on adding analyzer configuration to EditorConfig files seems to have missed this scenario, unless I'm overlooking something.

I can't find a way to make the configuration rules apply to source files that are being brought into my projects via NuGet content files. These files live outside the project folder structure because they are references directly from the extracted NuGet cache folder.

mavasani commented 3 years ago

@bording Editorconfig files are only meant to work when they are in project's directory cone. If you prefer shipping analyzer configuration via NuGet packages, you should look at shipping them as Global config files, which are directory/location agnostic and apply to the entire project: https://docs.microsoft.com/dotnet/fundamentals/code-analysis/configuration-files

bording commented 3 years ago

@mavasani I wasn't aware of the global config files feature, so thanks for pointing that out.

However, I'm not sure I see how they would solve the problem I'm having.

The scenario is that I have a project has various analyzer diagnostics enabled that I want to enforce on that project. I also need that project to reference a NuGet package that brings in source code via content files, and for the purposes of this discussion assume that I cannot make changes to that source code or the package itself.

I need a way to continue to enforce the analyzer rules in the project, but ignore them for the package source files by adding some configuration to the consuming project.

I can't do this with .editorconfig because the package source files aren't in the project's directory cone. It appears that I also can't add a .globalconfig to the project and use that to scope the configuration to the package source files in some way because of the limitations of that file format (no globs).

If the compiler was treating content files from NuGet packages as something to to exclude from analyzers by default as this issue is suggesting, then I wouldn't have to do anything to get the behavior I need.

Is there some other way with the currently implemented configuration support to solve my problem?

yishaigalatzer commented 3 years ago

@mavasani I would like to refine my proposal.

I think analyzers should inspect files in your project and provide feedback, because they could have significant effects on the result of the build. A user could fix it (maybe) either by a. Update the package version to one that doesn't have an issue b. File an issue with the package owner in the hopes that they would fix it, or submit a PR if it is OSS c. If they are the owners of the package, they can fix it themselves.

However, as @bording says, often this would not be possible at least not immediately. In that case the user has to turn off all the analyzers, and would not be able to benefit from them at all.

I could think of alternative configuration options, so just floating strawman ideas for discussion, the right solution is probably different:

Could a solution like "disable content files from package (optional package id):(optional version range)" address the issue? Or alternatively ignore files from {NUGET_HOME}/* ?

CC @rrelyea

mavasani commented 3 years ago

I need a way to continue to enforce the analyzer rules in the project, but ignore them for the package source files by adding some configuration to the consuming project.

You can drop in an editorconfig file in the NuGet package folder along with the sources. I believe the editorconfig will apply to all files under that directory in the NuGet package, so should apply to sources that ship in the NuGet package and nothing else. Can you try that out? If that doesn't work, this is likely a compiler/MSBuild issue as this scenario is supposed to work.

bording commented 3 years ago

You can drop in an editorconfig file in the NuGet package folder along with the sources.

That really isn't a viable solution since it would require each developer interacting with the repo to manually place this file in their local package folder.

I need something that I can commit into the project repo so that anyone using the project gets the correct behavior.

bording commented 3 years ago

I've been experimenting a bit more today, and for my exact scenario, I seem to have come up with something that works:

This means that all the analyzers are disabled for the NuGet package content source files, but still configured properly for the source files actually in the project.

However, if I try to use the same approach to selectively enable analyzers that I want to run against the NuGet package content source files, I seem to be running into problems.

With the following .globalconfig:

is_global = true
dotnet_analyzer_diagnostic.severity = none
dotnet_diagnostic.CA2007.severity = error

I am not seeing any errors for CA2007 violations in the NuGet package content source files, but I do see them if I remove those entries from the .globalconfig file. Based on my understanding of the severity precedence rules, I would expect to see them with those two lines in the file.


Despite all of this, I still feel that this is more of a workaround than an actual valid way to make all of this work. There should be an intentionally designed way to configure rules in the consuming project for source files coming in from NuGet packages.

adamralph commented 3 years ago

Is there anything that I (or anyone else) can do to help move this forward?

Youssef1313 commented 1 year ago

@jaredpar Is this by-design? Looks like the same I was asking for in https://github.com/dotnet/roslyn/issues/55992 if I understand correctly?