Skip to content

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

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.

Sign up for GitHub

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

Merged
CyrusNajmabadi merged 6 commits into from
Feb 21, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b24f62
Add test
CyrusNajmabadi Feb 20, 2025
8c00a94
Fix
Youssef1313 Feb 20, 2025
696f317
Expand doc comment
Youssef1313 Feb 20, 2025
3eb8e1b
Move check to 'make field readonly'
CyrusNajmabadi Feb 21, 2025
e96bc3a
Add test
CyrusNajmabadi Feb 21, 2025
7f67f6b
Update src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.Co…
CyrusNajmabadi Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix
  • Loading branch information
@Youssef1313
Youssef1313 committed Feb 20, 2025
commit 8c00a9464dade5bd3ceb29569cdf1c28b63b7a8a
68 changes: 50 additions & 18 deletions ...nalyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10089,34 +10089,66 @@ void M()
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72829")]
public async Task TestWriteIntoRefStructParameter()
[Fact]
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider work items

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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]);

Copy link
Member

@tannergooding tannergooding Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tannergooding tannergooding Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

public async Task TestWriteIntoPropertyOfRefStructParameter()
{
await new VerifyCS.Test
{
TestCode = """
using System;

internal sealed class UrlDecoder
internal sealed class C
{
private static bool DecodeCore(ref int destinationIndex, Span<byte> buffer)
private static void M(ref int destinationIndex, Span<byte> buffer)
{
// preserves the original head. if the percent-encodings cannot be interpreted as sequence of UTF-8 octets,
// bytes from this till the last scanned one will be copied to the memory pointed by writer.
var byte1 = 0;
if (byte1 == -1)
{
return false;
}
buffer[destinationIndex++] = (byte)0;
}
}
""",
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

if (byte1 <= 0x7F)
{
// first byte < U+007f, it is a single byte ASCII
buffer[destinationIndex++] = (byte)byte1;
return true;
}
[Fact]
public async Task TestWriteIntoPropertyOfRefStructParameterThenWriteTheParameter()
{
await new VerifyCS.Test
{
TestCode = """
using System;

internal sealed class C
{
private static void M(ref int destinationIndex, Span<byte> buffer)
{
buffer[destinationIndex++] = (byte)0;
// /0/Test0.cs(8,9): info IDE0059: Unnecessary assignment of a value to 'buffer'
{|IDE0059:buffer|} = default;
}
}
""",
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

return false;
[Fact]
public async Task TestWriteIntoPropertyOfStructParameter()
{
await new VerifyCS.Test
{
TestCode = """
using System;
internal struct S
{
public byte this[int index] { get => 0; set => _ = value; }
}

internal sealed class C
{
private static void M(ref int destinationIndex, S buffer)
{
// /0/Test0.cs(11,9): info IDE0059: Unnecessary assignment of a value to 'buffer'
{|IDE0059:buffer|}[destinationIndex++] = (byte)0;
}
}
""",
Expand Down
11 changes: 10 additions & 1 deletion src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation definitely helped!

// For example, if we have `s[0] = ...`, we will consider this as:
// 1. Read+Write: if s is value type and is not ref-like type.
// 2. Read only: if s is value type and is ref-like type.
// In the second case, it's a similar treatment as if it was a reference type.
// This is to help with unused assignment analysis where it's valid to have s[0] = ... without a subsequent read
// as the assignment can be observed by the caller.
return operation.Type.IsRefLikeType
? ValueUsageInfo.Read
: ValueUsageInfo.Read | GetValueUsageInfo(operation.Parent, containingSymbol);
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else if (operation.Parent is IVariableInitializerOperation variableInitializerOperation)
{
Expand Down