Fix false positive 'Unnecessary assignment of a value' #77297

Youssef1313 · 2025-02-20T19:01:04Z

CyrusNajmabadi · 2025-02-20T19:14:10Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs

@@ -210,7 +210,16 @@ where void Foo(in T v)
            // Accessing an indexer/property off of a value type will read/write the value type depending on how the
            // indexer/property itself is used. We add ValueUsageInfo.Read to the result since the value on which we are
            // accessing a property is always read.
-            return ValueUsageInfo.Read | GetValueUsageInfo(operation.Parent, containingSymbol);
+            // We consider a potential write only if the type is not a ref-like type.


I think it would be good to explain why being a ref-like type matters. Like why a write through it is still not considered a write. I'm genuinely not clear on why that's the right way to think about things (even if it helps this bug).

I expanded the doc comment on how I'm thinking about it. I can't say I'm 100% confident though.

The explanation definitely helped!

CyrusNajmabadi · 2025-02-20T19:40:27Z

src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs

@@ -10092,4 +10088,71 @@ void M()
            }
            """);
    }
+
+    [Fact]


Consider work items

Not sure where the related issue on GitHub is. The one I could find is #77258 but it's unrelated to ref-structs and won't be fixed by this PR.

Not a problem! Btw, do you know what would fix 77258?

#77258 feels tricky. If accurate results are needed then I'm guessing some sort of interprocedural analysis may be needed?

int[] a = new int[5]; MyStruct m = new MyStruct(a); // Just by looking at this, we can't tell whether or not this assignment is unnecessary // So, either we go with false positives, or false negatives. // Or, we go with interprocedural analysis that tries to understand exactly the indexer logic does and tries to relate it to the surrounding code, which seems very complicated (not even sure if possible to achieve). m[0] = 1; Console.WriteLine(a[0]);

If accurate results are needed then I'm guessing some sort of interprocedural analysis may be needed?

Intraprocedural analysis isn't safe to do and would represent at best a point in time snapshot. It's notably also "impossible" to do for cross assembly due to the existence of reference assemblies.

We would require some new attribute or construct that represents the necessary concept (which is most functionally similar to a Pure like annotation) and have it documented as a breaking change to remove from a method once it's been placed on it.

In general any property, indexer, or method may have side effects or involve indirect mutations; even if these members are annotated readonly. The implementation of such members may change over time or differ at runtime as compared to compile time.

The question here is if we can make the field read-only. The field can be readonly if the user is only access read-only members off of it, as those won't cause copies to happen.

or involve indirect mutations; even if these members are annotated readonly.

An indirect mutation is fine. This analyzer is just asking if the field can be readonly. It can be, while still making indirect mutations.

Note: the fail case here is to not offer anything at all. So it's a narrow whitelist of cases we offer the feature for. And it's a very popular and widely used analyzer/fixer with widespread usage. We want it to still be useful in th those whitelisted cases, without accidentally having false positives.

Ah, gotcha. The comment was confusing to me and I understood it as relating to IDE0059 (unnecessary assignment of a value). It sounds like you're saying its rather in relation to IDE0044 (add readonly modifier), IDE0250 (struct can be made readonly), or IDE0251 (member can be made readonly)

Yup! There are multiple features affected here, all with different desired behaviors :-)

"Make member read-only" wants to offer in all cases where it is totally safe to make readonly (so where it would not cause errors, or different runtime behavior (like a copy)).

"Remove unnecessary assignment" wants to inform the user of a full overwrite of a variable (local, parameter) that won't be observed (either because there no following reads, or because there is another full write that follows).

Different features. Different goals. Subtle interactions since they use some common helpers which we need to be more vigilant about :-)

src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs

src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt

…deAnalysis.CSharp.txt

CyrusNajmabadi · 2025-02-21T08:01:11Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs

-            // indexer/property itself is used. We add ValueUsageInfo.Read to the result since the value on which we are
-            // accessing a property is always read.
-            return ValueUsageInfo.Read | GetValueUsageInfo(operation.Parent, containingSymbol);
-        }


remove both the recent changes made to this file. moving to MakeFieldReadonly.

CyrusNajmabadi · 2025-02-21T08:02:20Z

@Youssef1313 I changed how this works a bit after the internal convo.

Youssef1313 · 2025-02-21T08:08:18Z

src/Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs

+        return operation is { Type.IsValueType: true, Parent: IPropertyReferenceOperation }
+            ? IsWrittenTo(operation.Parent, owningSymbol)
+            : false;


I guess an extra check here that the property reference is not readonly may make sense.

public struct S { public string P1 { get; set; } public readonly string P2 { get => ""; set { } } } public class C { private readonly S _s; public void M() { _s.P1 = ""; // not valid as _s is read-only and this is considered a write _s.P2 = ""; // perfectly valid as the property is readonly. } }

I guess an extra check here that the property reference is not readonly may make sense.

readonly does not in any way indicate "unnecessary" or "non-side effecting". In general you cannot assume that any property, indexer, or method usage for a user-defined type is "unnecessary" as there is no such way to define such an annotation in the language today.

@tannergooding This comment is on the "make field readonly" analyzer, unrelated to the "unnecessary assignment".

Currently there is a small false negative where we won't suggest making a field readonly if a readonly property setter is accessed. Based on internal discussion with Cyrus, it's not an important scenario and we are okay with the false negative.

and we are okay with the false negative.

Yup. False negatives are pretty much always ok (for roslyn ide analyzers). These are not security ah analyzers, so it's ok to miss some cases.

True negatives/positives are what we desire all the time. Lots, but not all, of our analyzers have this.

False positives are what we try hard to avoid. But occasionally will accept if the overall value to the user is high, and the chance of causing an issue is low. For example offering to change x.Prop != null ? x.Prop : y.Prop into x.Prop ?? y.Prop. This is technically a false positive as the semantics do change here. But the overall value in detecting this, and the extremely low rate of this changing semantics in a negative way for a user outweighs the feature to avoid the false positive. This team decision has been based on huge customer feedback that they do want these cases found.

tannergooding · 2025-02-21T11:13:20Z

src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs

+                        // Don't want to report IDE0059 here.  This write might be necessary as the struct might be wrapping
+                        // memory visible elsewhere.
+                        buffer[destinationIndex++] = (byte)0;


Even beyond wrapping memory, it could be doing something like Console.WriteLine(value) which constitutes an observable side effect.

Property getters/setters can execute arbitrary code and their implementations can change over time.

All if that is fine. This feature isn't making it so that the property isn't called.

[main] Update dependencies from dotnet/arcade - Fixes #12881 - Fixed instances of IDE0059 - False positive case of IDE0059, fix is on the way dotnet/roslyn#77297

[main] Update dependencies from dotnet/arcade - Fixes dotnet#12881 - Fixed instances of IDE0059 - False positive case of IDE0059, fix is on the way dotnet/roslyn#77297

* Don't use an implicit-object if the lang version doesn't support it (PR: [#77437](dotnet/roslyn#77437)) * Fix issue where we were changing semantics when converting to a collection expr. (PR: [#77417](dotnet/roslyn#77417)) * Detect and emit more idiomatic null check patterns (PR: [#77412](dotnet/roslyn#77412)) * Partial events and constructors: IDE (PR: [#77337](dotnet/roslyn#77337)) * Simplify keyword recommenders. (PR: [#77396](dotnet/roslyn#77396)) * Remove async/await (PR: [#77397](dotnet/roslyn#77397)) * Preserve encoding during DocumentState updates (PR: [#77354](dotnet/roslyn#77354)) * Don't realize the SourceText in SyntaxTree.OverlapsHiddenPosition if not needed (PR: [#77334](dotnet/roslyn#77334)) * Handle ModuleCancellationTokenExpression in AbstractFlow visitor (PR: [#77310](dotnet/roslyn#77310)) * PERF: Reduce the number of nodes walked during import completion commit. (PR: [#77305](dotnet/roslyn#77305)) * Allow expression body refactorings on non empty selections (PR: [#76969](dotnet/roslyn#76969)) * Partial events and constructors: public API (PR: [#77202](dotnet/roslyn#77202)) * Refresh diagnostics when fading options change (PR: [#77322](dotnet/roslyn#77322)) * Reduce allocations during completion in FilterToVisibleAndBrowsableSymbols (PR: [#77315](dotnet/roslyn#77315)) * Change override completion to select text after updating the buffer. (PR: [#76983](dotnet/roslyn#76983)) * Fix false positive 'Unnecessary assignment of a value' (PR: [#77297](dotnet/roslyn#77297))

CyrusNajmabadi and others added 2 commits February 20, 2025 01:50

Add test

9b24f62

Fix

8c00a94

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 20, 2025

dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Feb 20, 2025

CyrusNajmabadi reviewed Feb 20, 2025

View reviewed changes

Expand doc comment

696f317

CyrusNajmabadi approved these changes Feb 20, 2025

View reviewed changes

jaredpar reviewed Feb 20, 2025

View reviewed changes

src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Feb 20, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Tanya-Solyanik mentioned this pull request Feb 21, 2025

[Tracking] Reenable IDE0059 when the fix flows to VMR repo dotnet/winforms#12997

Closed

Tanya-Solyanik added a commit to dotnet/winforms that referenced this pull request Feb 21, 2025

False positive case of IDE0059, fix is on the way dotnet/roslyn#77297

58ba846

CyrusNajmabadi added 2 commits February 20, 2025 23:54

Move check to 'make field readonly'

3eb8e1b

Add test

e96bc3a

CyrusNajmabadi marked this pull request as ready for review February 21, 2025 08:00

CyrusNajmabadi requested a review from a team as a code owner February 21, 2025 08:00

CyrusNajmabadi reviewed Feb 21, 2025

View reviewed changes

src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt Outdated Show resolved Hide resolved

Update src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.Co…

7f67f6b

…deAnalysis.CSharp.txt

CyrusNajmabadi reviewed Feb 21, 2025

View reviewed changes

CyrusNajmabadi enabled auto-merge February 21, 2025 08:02

Youssef1313 commented Feb 21, 2025

View reviewed changes

CyrusNajmabadi merged commit aebe202 into dotnet:main Feb 21, 2025
25 checks passed

dotnet-policy-service bot added this to the Next milestone Feb 21, 2025

Youssef1313 deleted the unnecessaryWriteRefStruct branch February 21, 2025 09:51

tannergooding reviewed Feb 21, 2025

View reviewed changes

CyrusNajmabadi mentioned this pull request Feb 24, 2025

IDE0058 and IDE060 is incorrectly reported #77323

Closed

akhera99 removed this from the Next milestone Feb 25, 2025

akhera99 added this to the 17.14 P2 milestone Feb 25, 2025

This was referenced Feb 25, 2025

[Automated] PRs inserted in VS build main-35824.58 #77333

Closed

[Automated] PRs inserted in VS build feature.d18initial-10325.04 #77339

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35825.320 #77347

Closed

CyrusNajmabadi mentioned this pull request Mar 3, 2025

IDE0059 erroneously raised for write into span #77390

Closed

JoeRobich mentioned this pull request Mar 8, 2025

Update Roslyn to 4.14.0-3.25156.1 dotnet/vscode-csharp#8049

Merged

@@ @@ -10092,4 +10088,71 @@ void M() @@
                           }
                           """);
                   }
+                  [Fact]

Fix false positive 'Unnecessary assignment of a value' #77297

Fix false positive 'Unnecessary assignment of a value' #77297

Uh oh!

Conversation

Youssef1313 commented Feb 20, 2025 • edited by CyrusNajmabadi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Youssef1313 commented Feb 20, 2025 •

edited by CyrusNajmabadi

Loading

tannergooding Feb 21, 2025 •

edited

Loading

CyrusNajmabadi Feb 21, 2025 •

edited

Loading