Do not pass mutable value types by value #5454
Conversation
- Add CA2019 files - Add CA2019 resources - Suppress IDE0130 "Namespace does not match folder structure" warning for unit test project, since it flags literally every unit test file.
- Analyzer tests for known problematic types
Add tests ensuring fixer updates lvalue callsites.
- Minor adjustment to the span that is underlined for visual basic return value errors. - Run msbuild
…s-mutable-value-types-by-value
|
|
||
| using System.Diagnostics.CodeAnalysis; | ||
|
|
||
| [assembly: SuppressMessage("Style", "IDE0130:Namespace does not match folder structure", Justification = "<Pending>", Scope = "namespace", Target = "~N:Microsoft.NetCore.CSharp.Analyzers.Runtime")] |
There was a problem hiding this comment.
❗ This repository doesn't use global suppressions. The rule can be configured in the appropriate .globalconfig file.
|
I strongly believe that RS0042 (Do not copy value) is superior to this proposed rule in all aspects. |
|
This is a really complicated analysis scenario. I ran into a large number of issues working in this space in the past and I'm not sure a pull request review is the right place to try and cover them all. |
Is there a slack or discord channel where we could discuss this further? |
Codecov Report
@@ Coverage Diff @@
## main #5454 +/- ##
==========================================
+ Coverage 95.49% 95.53% +0.04%
==========================================
Files 1217 1276 +59
Lines 277298 289016 +11718
Branches 16741 17384 +643
==========================================
+ Hits 264817 276125 +11308
- Misses 10280 10547 +267
- Partials 2201 2344 +143 |
@sharwell It looks like RS0042 does much more than what was specified here. |
|
Yes, I'm still discussing this with the runtime team. It looks like some incorrect assumptions were made regarding |
| @@ -1620,6 +1620,30 @@ Number of parameters supplied in the logging message template do not match the n | |||
| |CodeFix|False| | |||
| --- | |||
|
|
|||
| ## [CA2019](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2019): Do not pass mutable value types by value | |||
There was a problem hiding this comment.
For both of these, what is the plan for handling and/or annotating value types which explicitly should be passed and returned by value?
For example, this is going to have tons of false positives in interop code. It's likewise not going to be taking SIMD (Vector2, Vecto3, or Vector4) or ABI (Unix can pass a struct in multiple registers sometimes) into account where passing by ref or in is generally inappropriate and may hurt perf or cause other regressions. This may likewise flag things like Guid incorrectly on older TFMs given that it wasn't updated to be readonly until newer releases (even though it was "functionally readonly" already).
There was a problem hiding this comment.
@tannergooding As per this post, this analyzer only flags value types that are one of the following:
- On a built-in list of problematic types (the list currently contains
SpinLockandUtf8JsonReader) - Added as problematic mutable value types via an .editorconfig option,
- Types that it determines to be struct-enumerator types (must be a nested value type who's name ends with "Enumerator").
Edit: also, for struct-enumerator types, we suppress the return-by-value warning if returned from a method named "GetEnumerator".
Fix dotnet/runtime#33772.
As of right now, the analyzer reports all parameters that are typed as mutable value types and are not marked as
reforout. We also report all methods who's return type is a mutable value type that do not return by reference, unless the return type is a struct-enumerator type and the method's name is "GetEnumerator".We report all properties of a mutable value type that do not return by reference.
I ran the analyzer against dotnet/runtime and found multiple false-positives in JsonElement.cs (
EnumerateArray()andEnumerateObject()) and ThrowHelper.Serialization.cs.There were false positive violations for both the return-values rule and the pass-by-value rule. All the return-value rule violations were factory methods. I think we should consider not reporting violations for method return values, as that would raise a false positive for any factory method, which seems like a fairly common scenario.
In the meantime, I've added an exception in the analyzer for methods named "GetEnumerator" that return a struct-enumerator type, as otherwise we'd get a huge number of false positives throughout dotnet/runtime.
All of the pass-by-value violations were
inparameters, and I was able to determine that the methods do not call any mutating members on the arguments in question. We may want to consider not flagginginparameters. Unlike passing by-value, which is the default, it seems likely that any parameter marked asinwas done so with the intention that the method will not be modifying it, and therefore is likely safe.