Implement RS0042 (Do not copy value) #3420
Conversation
| helpLinkUri: null, | ||
| customTags: WellKnownDiagnosticTags.Telemetry); | ||
|
|
||
| internal static DiagnosticDescriptor NoBoxingRule = new DiagnosticDescriptor( |
There was a problem hiding this comment.
📝 Originally the analyzer reported all boxing of non-copyable types. This was particularly problematic for struct enumerators, so the rule was subsequently modified to allow moving a value into a box, and blocking the unboxing. Boxed non-copyable values may be used via Unsafe.Unbox<T>, which returns a reference to the boxed value without copying it out of the box.
| helpLinkUri: null, | ||
| customTags: WellKnownDiagnosticTags.Telemetry); | ||
|
|
||
| internal static DiagnosticDescriptor UnsupportedUseRule = new DiagnosticDescriptor( |
There was a problem hiding this comment.
📝 This rule is reported for cases where a non-copyable value type is used in a manner that the analyzer does not understand. Eventually, we want more specific diagnostics to be reported for invalid use cases, and no diagnostics to be reported for valid use cases.
There was a problem hiding this comment.
Will this rule be removed before you merge this PR?
There was a problem hiding this comment.
No. It can be removed if we ever get to a case where all the potential copies are covered by other messages.
Codecov Report
@@ Coverage Diff @@
## master #3420 +/- ##
==========================================
- Coverage 95.27% 95.08% -0.20%
==========================================
Files 1043 1045 +2
Lines 235509 237064 +1555
Branches 15244 15365 +121
==========================================
+ Hits 224374 225404 +1030
- Misses 9461 9962 +501
- Partials 1674 1698 +24 |
|
This looks very similar to https://github.com/ufcpp/NonCopyableAnalyzer . What do you think about using an attribute to mark the non-copyable structs? |
|
@jkotas The manner in which non-copyable types are identified is separate from how they are handled after identification. For now, I'm experimenting at a rather large scale in Roslyn.sln by treating all mutable value types as non-copyable. |
|
FYI: You will need to fix a new RS diagnostic once #3444 goes in. |
| context.RegisterCompilationStartAction(context => | ||
| { | ||
| var cache = new NonCopyableTypesCache(context.Compilation); | ||
| context.RegisterOperationBlockAction(context => AnalyzeOperationBlock(context, cache)); |
There was a problem hiding this comment.
📝 This needs to also have a symbol analyzer to ensure non-copyable types are not used incorrectly (e.g. the early checks would be for the use of a non-copyable type as a generic argument).
|
|
||
| public override void VisitAddressOf(IAddressOfOperation operation) | ||
| { | ||
| CheckTypeInUnsupportedContext(operation); |
There was a problem hiding this comment.
Do you override every single method on OperationWalker, with this being the default implementation?
There was a problem hiding this comment.
Yes. I do that until I manually update the method with a more complete implementation.
At a high level, this pull request approaches non-copyable value type restrictions by preventing all use of such types (similar to Banned API analyzer), followed by selectively allowing known safe patterns.
For the implementation, I have been iterating over Roslyn.sln using AnalyzerRunner to log all diagnostics produced by this analyzer.
Supersedes #2831