-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Add Roslyn analyzers to detect incorrect usage of BenchmarkDotNet #2837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@dotnet-policy-service agree |
08ffcd6
to
e0bd1d6
Compare
...markDotNet.Analyzers.Tests/AnalyzerTests/Attributes/ParamsAllValuesAttributeAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...markDotNet.Analyzers.Tests/AnalyzerTests/Attributes/ParamsAllValuesAttributeAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
...hmarkDotNet.Analyzers/BenchmarkDotNet.Analyzers.Tests/BenchmarkDotNet.Analyzers.Tests.csproj
Outdated
Show resolved
Hide resolved
src/BenchmarkDotNet.Disassembler.x64/BenchmarkDotNet.Disassembler.x64.csproj
Outdated
Show resolved
Hide resolved
I'm not sure whether the analyzers should be automatically enabled with the base BenchmarkDotNet package or be opt-in via its own NuGet package, what do you think? |
They should be enabled by default. |
So maybe the VSIX package project can be removed then as the analyzer can be referenced through an analyzer project reference. |
I actually think the analyzer should be included directly into the annotations package. Otherwise, it was found that a separate analyzer package pulls in too many unnecessary dependencies. It's a bit complicated to set up the build to do it, though, so I can do it separately after this is merged if you want. [Edit] Or I can push to your branch after your changes are complete. |
I'm getting |
4cec602
to
de94c5b
Compare
You need to import common.props in the analyzer project, too. |
de94c5b
to
c18a417
Compare
Solved it. I also needed to add the public key of the assembly to the InternalsVisibleTo attribute. |
Do I just reference the analyzer project from the annotations project? Or do I need to do something special for the analyzers to activate for the user? Mind that we of course don't want the analyzers to activate for the annotations project, but transitively for the user. |
Yes that's sufficient for now. |
Also you should move the analyzers test project to under the tests/ directory (and move the analyzers project up 1 level). |
BDN1003 | Usage | Error | BDN1003_General_BenchmarkClass_ClassMustBePublic | ||
BDN1004 | Usage | Error | BDN1004_General_BenchmarkClass_ClassMustBeNonStatic | ||
BDN1005 | Usage | Error | BDN1005_General_BenchmarkClass_ClassMustBeNonAbstract | ||
BDN1006 | Usage | Error | BDN1006_General_BenchmarkClass_ClassMustBeNonGeneric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an incorrect error (or a misleading description). Benchmark classes may be generic, but if they are, they must have [GenericTypeArguments]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the attribute might not be required if it's abstract and a derived class supplies the generic types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a non-generic class is annotated with [GenericTypeArguments]
, should I loop infinitely over its ancestor classes until I find one that is generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, non-generic classes should not have [GenericTypeArguments]
.
BDN1002 | Usage | Error | BDN1002_General_BenchmarkClass_MethodMustBeNonGeneric | ||
BDN1003 | Usage | Error | BDN1003_General_BenchmarkClass_ClassMustBePublic | ||
BDN1004 | Usage | Error | BDN1004_General_BenchmarkClass_ClassMustBeNonStatic | ||
BDN1005 | Usage | Error | BDN1005_General_BenchmarkClass_ClassMustBeNonAbstract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this rule is entirely correct. I think users may create an abstract base class with benchmarks, then create derived concrete classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not entirely sure where I got this rule from. Maybe we should just remove it completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it is only types with [GenericTypeArguments]
that must be concrete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps? :)
I think I can add a rule that the referenced benchmark class in BenchmarkRunner.Run<BenchmarkClass>
cannot be abstract.
context.ReportDiagnostic(Diagnostic.Create(ClassMustBeNonGenericRule, classDeclarationSyntax.TypeParameterList.GetLocation(), classDeclarationSyntax.Identifier.ToString())); | ||
} | ||
} | ||
else if (genericTypeArgumentsAttributes.Length == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple GenericTypeArgumentsAttributes are allowed. You should iterate over them all.
} | ||
|
||
var benchmarkMethods = benchmarkMethodsBuilder.ToImmutable(); | ||
if (benchmarkMethods.Length == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base/derived classes without benchmarks may still apply other attributes like [GlobalSetup]
, [Params]
, etc. These types should be analyzed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would split each attribute analysis to a separate method and call them all.
src/BenchmarkDotNet.Annotations/BenchmarkDotNet.Annotations.csproj
Outdated
Show resolved
Hide resolved
… from BenchmarkDotNet.Annotations
Is |
It's per category. |
This PR introduces an extensive set of analyzers that warns the user of incorrect usage of BenchmarkDotNet. This is something that has been asked since 2017 but has yet to be included as of this date. BDN has a set of validators that use reflection to detect errors but they are only triggered after the benchmark code has been compiled and is about to run.
I had the idea to implement this in 2022 but the testing framework back then wasn't trivial to use so I gave up in the end. Today, the Roslyn analyzer testing is completely testing framework-agnostic, making things considerably easier. It's also trivial to add multiple source files, references and framework assemblies in order to test your analyzer precisely the way you want.
All unit tests are implemented using xUnit v2.
With these analyzers, developers can detect errors early and solve them immediately. The descriptions are very clear and succinct, guiding the user and explaining the reasoning behind the specific rule.
Here's a list of currently implemented analyzers. There are still some remaining but I believe this is a good start and covers most common usage errors. The rest is up for grabs and can be added along the way.
BenchmarkRunner.Run<BenchmarkClass>()
and the benchmark class (BenchmarkClass
) has no public methods marked with the[Benchmark]
attributeTODO
[ArgumentsSource]
points to a valid method[ParamsSource]
points to a valid method[GenericTypeArguments]
attribute matches the type of the type parameterSee #2666 for discussion as well as #389.