search

AArnott / Nerdbank.MessagePack

A .NET MessagePack serialization library with great performance and simplicity.
https://aarnott.github.io/Nerdbank.MessagePack/
MIT License
38 stars 1 forks source link

Microsoft.VisualStudio.Threading analyzers leaking into consuming project #101

Closed joelverhagen closed 1 week ago

joelverhagen commented 1 week ago

A bunch of VSTHRD warnings and errors appear in my project when I reference Nerdbank.MessagePack 0.2.34-alpha. I believe this is due to VS threading analyzers you use in the project.

I was able to suppress them by doing <ExcludeAssets>analyzers</ExcludeAssets> on the Nerdbank.MessagePack <ProjectReference> but this forced me to lift the dependency to all of my projects that transitively reference Nerdbank.MessagePack.

I saw this approach in your code but this seems like a pretty extreme measure since I just want to suppress the VS Threading analyzer coming from Nerdbank.MessagePack. https://github.com/AArnott/Nerdbank.MessagePack/blob/ad761abcda5c8951be7863fecd05567b24cf2d9f/src/Analyzers.props#L11-L17

Is this expected? Personally, I would expect MessagePack-related analyzers to run but not Visual Studio ones.

Right now, I preferred working around the problem by simply suppressing the message codes I didn't want, e.g.

dotnet_diagnostic.VSTHRD002.severity = none
dotnet_diagnostic.VSTHRD003.severity = none
dotnet_diagnostic.VSTHRD011.severity = none
dotnet_diagnostic.VSTHRD103.severity = none
dotnet_diagnostic.VSTHRD200.severity = none
AArnott commented 1 week ago

I saw this approach in your code but this seems like a pretty extreme measure

That doesn't address the same problem you're seeing and isn't extreme. It's necessary to prevent any dependencies in the analyzers from being exposed as nuget dependencies. Analyzers must be entirely self-contained, so any dependencies they have that aren't provided by the compiler must be bundled in the nuget package itself, and thus there is no need or desire to have the package itself express dependencies to support analyzers. The xml snippet you found prevents these dependencies from showing up in the package.

It's entirely separate the problem you're facing, which is totally a legit complaint. Sadly, nuget doesn't support PrivateAssets="analyzers" or I would very happily do that, which would prevent analyzers associated with upstream runtime dependency packages from activating for your project. This has been a long outstanding feature request against nuget itself, as you can see at https://github.com/microsoft/vs-streamjsonrpc/issues/195, which is the same as this one except it was filed against the StreamJsonRpc library. The request on nuget is here: https://github.com/NuGet/Home/issues/6279 (please 👍🏻 vote it up!)

In the meantime, there are only two options:

  1. Suppress the analyzers on your consuming side (as you've already done). This is the approach I recommend.
  2. Nerdbank.MessagePack drop its dependencies on other packages that bring in analyzers. This would require that we drop Nerdbank.Streams as a dependency, which is conceivable but would be a loss in some ways.
AArnott commented 1 week ago

I would expect MessagePack-related analyzers to run but not Visual Studio ones

FYI these "Visual Studio ones" are not specific to Visual Studio, and in fact many of them may be useful for most apps to have. In fact some of them are so generally interesting that the .NET SDK started porting them from the vs-threading analyzer library to the SDK a while back. That work was never finished, unfortunately.