BenchmarkDotNet
BenchmarkDotNet copied to clipboard
dotnet
Multi-target to .NET Standard 2.1, port JetBrains' nullability annotations et. al.
Note: with #2012 merged, the description is out of date.
Until now the BenchmarkDotNet library exclusively targets .NET Standard 2.0, which means that when installed on newer frameworks it references more packages than actually needed. Worse, some of these packages are of an older version that targeted only .NET Standard 1.x, and significantly bloat BenchmarkDotNet's transitive dependencies.
With this PR, I multi-targeted BenchmarkDotNet to .NET Standard 2.1, saving three packages (and one more from both frameworks), and updated some of them. The package's transitive dependencies have been drastically reduced. To illustrate this, the project's .deps.json file was 1921 lines long, and now has more than halved to 632 lines on .NET Standard 2.0 and 557 on .NET Standard 2.1.
An obstacle to this porting was JetBrains.Annotations' nullability annotations that predate C# 8.0's nullability annotations. On .NET Standard 2.1 there were two attributes named NotNullAttribute, and the compiler did not know which to pick. I enabled <Nullable>annotations</Nullable> on the project, removed [JetBrains.Annotations.NotNullAttribute], and replaced [JetBrains.Annotations.CanBeNull] with ?. Entirely annotating the project is a much bigger undertaking which is left for another time.
After this PR gets merged, I plan also multi-targeting for .NET 6, to take advantage of newer framework features. I already did in this PR with https://github.com/dotnet/BenchmarkDotNet/commit/d2bbe826c64ffd4f5f3ea4ea48780f9805fe8cbd for example.
Yeah, I noticed it after opening this PR, not the first time it happens. 😂
Since mine is bigger and more comprehensive, I recommend to merge it first, and leave the .NET 6 porting work to you.
@adamsitnik I synched the PR with the changes #2012 brought. Can you take a look?
Please wait before merging, I have to change the title and description, and add a couple more changes I thought.
EnableNETAnalyzers adds many new warnings, shouldn't the build fail because of TreatWarningsAsErrors?
common.props:
<EnableNETAnalyzers>true</EnableNETAnalyzers>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
@YegorStepanov these warnings were already enabled but with the obsolete Microsoft.CodeAnalysis.FxCopAnalyzers package. This PR does it the right way.
@teo-tsirpanis @adamsitnik all of our CI builds are red now. Here is the error:
/home/runner/work/BenchmarkDotNet/BenchmarkDotNet/tests/BenchmarkDotNet.IntegrationTests/BenchmarkDotNet.IntegrationTests.csproj : error NU1504: Duplicate 'PackageReference' items found. Remove the duplicate items or use the Update functionality to ensure a consistent restore behavior. The duplicate 'PackageReference' items are: System.Runtime.CompilerServices.Unsafe 6.0.0, System.Runtime.CompilerServices.Unsafe 6.0.0. [TargetFramework=net462]
@AndreyAkinshin @YegorStepanov appologies for breaking the build, thanks for fixing it!
The code looks pretty messy now:
I changed to <Nullable>enable</Nullable> to count NRT issues. It gives me 908 issues.
Is there a simple way to suppress them all? If not, maybe revert NRT until the whole library is annotated? Also, I think there are some public API that can accept/return nullable types. Wrong annotation can be annoying for users.
If @teo-tsirpanis isn't working on it, I will be happy to do it (~4 months).
I set it to annotations to minimize the work I had to do. Feel free to change it to enable @YegorStepanov, I don't have time for it.