Add DoNotUseReferenceEqualsWithValueTypesAnalyzer #3368
Conversation
This analyzer detects when an argument into object.ReferenceEquals is a value type that gets boxed as part of the argument.
Codecov Report
@@ Coverage Diff @@
## master #3368 +/- ##
==========================================
+ Coverage 95.24% 95.25% +0.01%
==========================================
Files 989 991 +2
Lines 225299 226162 +863
Branches 14434 14503 +69
==========================================
+ Hits 214579 215441 +862
Misses 9093 9093
- Partials 1627 1628 +1 |
| internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create(RuleId, | ||
| s_localizableTitle, | ||
| s_localizableMessage, | ||
| DiagnosticCategory.MicrosoftCodeAnalysisCorrectness, |
There was a problem hiding this comment.
This category is actually meant for analyzers for correct usage of APIs from Microsoft.CodeAnalysis NuGet package. See https://github.com/dotnet/roslyn-analyzers/blob/master/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt. We also have an analyzer in the repo that would have flagged this category mismatch and enforces the ID range to category mapping, but that analyzer recently broke - let me resurrect that.
Meanwhile, you may want to follow up with @terrajobst on the category you guys would like to use. You can either re-use one of the FxCop categories from that file and pick the next available ID from that categories allocated ID range OR define a new category. If we decide to go with IDs that are not following the 2 letter prefix followed by an interger, then I can remove this ID-range mapping validation for the new category.
There was a problem hiding this comment.
#3370 - @bartonjs once that PR goes in, I'll have a follow-up PR to move the repo to consume the new analyzer package with that enhancement. At that point, you can pull in the latest sources into your branch and confirm that you get an RSxxxx warning here that your chosen rule ID does not fall in the allowed range ID for the provided rule category.
| <value>The argument for the '{0}' parameter to ReferenceEquals evaluates to the value type '{1}'. Due to value boxing, this call to ReferenceEquals will always return false.</value> | ||
| </data> | ||
| <data name="DoNotUseReferenceEqualsWithValueTypesTitle" xml:space="preserve"> | ||
| <value>Do not use ReferenceEquals with value types</value> |
There was a problem hiding this comment.
All the other messages end with a period.
There was a problem hiding this comment.
Yeah we are very inconsistent in whether or not periods are used in our title/message/description strings. I think we should follow @sharwell's proposal from #3183 (comment), which is summarized by @Evangelink in #1714 (comment)
There was a problem hiding this comment.
Another place where an analyzer/fixer to enforce this would be a much better route. @Evangelink given your initial interest in tackling #1714, would you be interested in writing this analyzer/fixer? If so, we can sync up offline on the design.
There was a problem hiding this comment.
My scan showed that things seemed to be title: no period, other: yes period.
There was a problem hiding this comment.
@mavasani Yep I'd be happy to help, let's have a chat later on.
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotUseReferenceEqualsWithValueTypes.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotUseReferenceEqualsWithValueTypes.cs
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotUseReferenceEqualsWithValueTypes.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotUseReferenceEqualsWithValueTypes.cs
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotUseReferenceEqualsWithValueTypes.cs
Outdated
Show resolved
Hide resolved
...tAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/DoNotUseReferenceEqualsWithValueTypes.cs
Show resolved
Hide resolved
src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| }", | ||
| GetCSharpResultAt(10, 36, "objA", "System.IntPtr")); |
There was a problem hiding this comment.
I believe the test framework also supports annotating the source such as return ReferenceEquals([|IntPtr.Zero|], test);, so you can avoid passing in this explicit expected diagnostic and the framework will decode the annotation and validate the diagnostic. I am fine with your current form as well, if that is by preference.
|
In a related vein, should there also be an analyzer warning when passing the newly added |
Seems reasonable. Should we just fold that case also in the analyzer you are adding, and just use a different message, but same ID? If so, you can create an additional descriptor in the analyzer with same arguments, but just a different messageFormat string, and it should work. |
| operationContext.ReportDiagnostic( | ||
| val.CreateDiagnostic( | ||
| Rule, | ||
| val.Type.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat))); |
There was a problem hiding this comment.
We cannot use this error message format because the analyzer is supported for both VB and C#. Generally we just use the simple type.Name in messages.
There was a problem hiding this comment.
I was concerned about that, but a lot of analyzers seem to use that format, none use the VB format, and the VB tests say that it says "Integer" instead of "int", so whatever the printer is it seems to understand which language applies.
src/Microsoft.NetCore.Analyzers/Microsoft.NetCore.Analyzers.sarif
Outdated
Show resolved
Hide resolved
|
I added the simple detection for ReferenceEqualityComparer's Equals method (I didn't do dataflow to find out if an I'm not sure how to add tests that involve types that were recently introduced... is there a best practice? (I did manually verify it with a project in dotnet/runtime that can see ReferenceEqualityComparer) |
|
|
@bartonjs The test would need to set the reference assemblies as appropriate for the API in question. Eventually this will be simple, such as the following: await new VerifyCS.Test
{
ReferenceAssemblies = ReferenceAssemblies.NetCore.NetCoreApp20, // netcoreapp2.0
}.RunAsync();Can you clarify exactly which version you want to test against? We can use that information to construct a custom |
.NET 5, Feb14 or newer 😄. |
|
Here's the definition for netcoreapp2.1: I'm guessing the new TFM is netcoreapp5.0? Assuming the reference assembly package for this TFM is still Microsoft.NETCore.App and the package still has the same internal structure as netcoreapp2.1, there are two changes that are needed:
|
|
I think we're in the process of prepping preview 1, so it's probably sensible to just wait for that to finish; but it doesn't look like 3.0/3.1/5.0 use the Microsoft.NETCore.App package anymore, it's just intrinsically there, or not (which may be part of why no NetCoreApp30 or NetCoreApp31 test target got defined?) |
|
@sharwell Yeah, pretty much that one. ::facepalm:: |
|
@bartonjs You'll be able to target this as soon as dotnet/roslyn-sdk#488 is available (it specifically needs the change to |
|
@bartonjs I just updated the SDK for dotnet/roslyn-analyzers. You can now use |
| @@ -51,5 +51,20 @@ public static async Task VerifyCodeFixAsync(string source, DiagnosticResult[] ex | |||
| test.ExpectedDiagnostics.AddRange(expected); | |||
| await test.RunAsync(); | |||
| } | |||
|
|
|||
| public static async Task VerifyAnalyzerAsync( | |||
There was a problem hiding this comment.
📝 New helper methods should not be added to this class. I will remove this in a follow-up PR.
|
@mavasani Would a fixer that replaces the triggering expressions with |
|
Yeah, I am not sure there is a good codefix that would be helpful here. |
This analyzer detects when an argument into object.ReferenceEquals
is a value type that gets boxed as part of the argument.
Contributes to dotnet/runtime#30740
cc @dotnet/dotnet-analyzers