-
-
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). |
src/BenchmarkDotNet.Analyzers/General/BenchmarkClassAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/BenchmarkDotNet.Analyzers/General/BenchmarkClassAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/BenchmarkDotNet.Annotations/BenchmarkDotNet.Annotations.csproj
Outdated
Show resolved
Hide resolved
|
Is |
It's per category. |
src/BenchmarkDotNet.Analyzers/Attributes/ParamsAttributeAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/BenchmarkDotNet.Analyzers/Attributes/ArgumentsAttributeAnalyzer.cs
Outdated
Show resolved
Hide resolved
|
Please also test these cases: private const int x = 100;
[Params(x)]
public int num;
[Arguments(x)]
public void Benchmark(int i) { }[Params(DifferentType.SomeConst)]
public int num;
[Arguments(DifferentType.SomeConst)]
public void Benchmark(int i) { } |
I had that in the pipeline too. |
…arameter created by using a typeof expression
…ed with the Benchmark attribute when analyzing GenericTypeArguments attribute rules
…ot trigger mismatching type diagnostics * Test all valid attribute value types when performing type matching
…ArgumentsAttribute]" to Run analyzer and remove abstract modifier requirement
… said array for [Arguments] attribute values
… for [Params] and [Arguments] attribute values
…operty on BenchmarkAttribute * Split logic for baseline method analyzer into two rules, also verifying whether they're unique per category
… it works correctly with invalid string values
…r member" (CS1591)
Run analyzer tests in CI. Fix compile errors. Ensure analyzers run against test projects.
Add analyzers to samples.
…rule from warning to information * Move diagnostic "MethodWithoutAttributeMustHaveNoParameters" to its own analyzer and also take [ArgumentsSource] attribute into consideration * Suppress analyzer errors for incorrect BenchmarkDotNet usages in tests * Adjust wording of "ClassWithGenericTypeArgumentsAttributeMustBeGenericRule" diagnostic and move trigger location to the attribute level
fbb5d39 to
0af0204
Compare
|
Not sure if this is a valid issue, but locally I get lots of errors like Does the generated benchmark really need to reference the analyzer project? |
…sts.cs and TypeFilterTests.cs
|
Oh, that's probably from the Directory.build.props file. I didn't get that error until I cleaned my bin/obj dirs. It wouldn't be an issue if #2467 was done... we could just copy the project reference to each test project instead, not sure if there's a cleaner way. |
|
I pushed a fix for that. |
|
What's the next step in this PR process? |
|
Looks like some tests are still failing. |
|
I can't really see what's wrong besides the flaky tests |
|
Hm... I re-ran the tests twice and they keep failing. I'm not able to repro on my local machine. 🤔 |
|
It looks like updating the sdk to 9.0 caused some tests to fail even though the tests are using net8.0 tfm. The update was necessary because there were compile errors using Microsoft.CodeAnalysis.CSharp v4.14.0 otherwise. We can either revert the sdk update and downgrade the Microsoft.CodeAnalysis.CSharp version, or update all of the tests. It's probably easier to do the former. We should be updating all the tests to net10.0 soon after the GA release next week anyway. |
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 classBenchmarkClass(or any of its inherited classes) has no public methods marked with the[Benchmark]attribute[BenchmarkCategory]attribute results in unintended null arrayTODO
[Arguments]and[ArgumentsSource]attribute usages are mutually exclusive[ArgumentsSource]points to a valid method[ParamsSource]points to a valid methodSee #2666 for discussion as well as #389.