Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,9 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.RemoveUnusedParametersA
CSharpRemoveUnusedValuesCodeFixProvider>;

[Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnusedValues)]
public class RemoveUnusedValueAssignmentTests : RemoveUnusedValuesTestsBase
public sealed class RemoveUnusedValueAssignmentTests(ITestOutputHelper logger)
: RemoveUnusedValuesTestsBase(logger)
{
public RemoveUnusedValueAssignmentTests(ITestOutputHelper logger)
: base(logger)
{
}

private protected override OptionsCollection PreferNone
=> Option(CSharpCodeStyleOptions.UnusedValueAssignment,
new CodeStyleOption2<UnusedValuePreference>(UnusedValuePreference.DiscardVariable, NotificationOption2.None));
Expand Down Expand Up @@ -10092,4 +10088,105 @@ void M()
}
""");
}

[Fact]
Copy link
Member

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

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

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

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

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

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

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

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

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 C
{
private static void M(ref int destinationIndex, Span<byte> buffer)
{
buffer[destinationIndex++] = (byte)0;
}
}
""",
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[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();
}

[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)
{
// 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;
Comment on lines +10150 to +10152
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
""",
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77258")]
public async Task TestWriteIntoStructIndexer()
{
await new VerifyCS.Test
{
TestCode = """
using System;

int[] a = new int[5];
MyStruct m = new MyStruct(a);
m[0] = 1;
Console.WriteLine(a[0]);

struct MyStruct(int[] a)
{
private int[] array = a;

public int this[int index]
{
get => array[index];
set => array[index] = value;
}
}
""",
ReferenceAssemblies = ReferenceAssemblies.Net.Net80,
LanguageVersion = LanguageVersion.CSharp12,
TestState =
{
OutputKind = OutputKind.ConsoleApplication,
}
}.RunAsync();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,26 @@ static bool IsDataContractSerializable(IFieldSymbol symbol, INamedTypeSymbol? da
private static Location GetDiagnosticLocation(IFieldSymbol field)
=> field.Locations[0];

private static bool IsWrittenTo(IOperation? operation, ISymbol owningSymbol)
{
if (operation is null)
return false;

var result = operation.GetValueUsageInfo(owningSymbol);
if (result.IsWrittenTo())
return true;

// Accessing an indexer/property off of a value type will read/write the value type depending on how the
// indexer/property itself is used.
return operation is { Type.IsValueType: true, Parent: IPropertyReferenceOperation }
? IsWrittenTo(operation.Parent, owningSymbol)
: false;
Comment on lines +248 to +250
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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.

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.

}

private static bool IsFieldWrite(IFieldReferenceOperation fieldReference, ISymbol owningSymbol)
{
// Check if the underlying member is being written or a writable reference to the member is taken.
var valueUsageInfo = fieldReference.GetValueUsageInfo(owningSymbol);
if (!valueUsageInfo.IsWrittenTo())
if (!IsWrittenTo(fieldReference, owningSymbol))
return false;

// Writes to fields inside constructor are ignored, except for the below cases:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ public static bool IsTargetOfObjectMemberInitializer(this IOperation operation)
/// Returns the <see cref="ValueUsageInfo"/> for the given operation.
/// This extension can be removed once https://github.com/dotnet/roslyn/issues/25057 is implemented.
/// </summary>
/// <remarks>
/// When referring to a variable, this method should only return a 'write' result if the variable is entirely
/// overwritten. Not if the variable is written <em>through</em>. For example, a write to a property on a struct
/// variable is not a write to the struct variable (though at runtime it might impact the value in some fashion).
/// <para/> Put another way, this only returns 'write' when certain that the entire value <em>is</em> absolutely
/// entirely overwritten.
/// </remarks>
public static ValueUsageInfo GetValueUsageInfo(this IOperation operation, ISymbol containingSymbol)
{
/*
Expand Down Expand Up @@ -205,13 +212,6 @@ where void Foo(in T v)
{
return ValueUsageInfo.Write;
}
else if (operation is { Type.IsValueType: true, Parent: IPropertyReferenceOperation })
{
// 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);
}
Copy link
Member

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)
{
if (variableInitializerOperation.Parent is IVariableDeclaratorOperation variableDeclaratorOperation)
Expand Down
Loading