From 9b24f628f83fe7a985a609687ac6dba617b1a21a Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 20 Feb 2025 01:50:56 -0800 Subject: [PATCH 1/6] Add test --- .../RemoveUnusedValueAssignmentTests.cs | 43 ++++++++++++++++--- .../Apis/Microsoft.CodeAnalysis.CSharp.txt | 1 + 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs index 48b3eab5b066c..09bd17cbd8476 100644 --- a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs +++ b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs @@ -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.DiscardVariable, NotificationOption2.None)); @@ -10092,4 +10088,39 @@ void M() } """); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72829")] + public async Task TestWriteIntoRefStructParameter() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + + internal sealed class UrlDecoder + { + private static bool DecodeCore(ref int destinationIndex, Span 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; + } + + if (byte1 <= 0x7F) + { + // first byte < U+007f, it is a single byte ASCII + buffer[destinationIndex++] = (byte)byte1; + return true; + } + + return false; + } + } + """, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + }.RunAsync(); + } } diff --git a/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt b/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt index 3075a24a86c70..2bdd939afa897 100644 --- a/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt +++ b/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt @@ -1177,6 +1177,7 @@ Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.get_GetEnumeratorMethod Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.get_IsAsynchronous Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.get_MoveNextMethod Microsoft.CodeAnalysis.CSharp.InterceptableLocation +Microsoft.CodeAnalysis.CSharp.InterceptableLocation.Equals(Microsoft.CodeAnalysis.CSharp.InterceptableLocation) Microsoft.CodeAnalysis.CSharp.InterceptableLocation.Equals(System.Object) Microsoft.CodeAnalysis.CSharp.InterceptableLocation.GetDisplayLocation Microsoft.CodeAnalysis.CSharp.InterceptableLocation.GetHashCode From 8c00a9464dade5bd3ceb29569cdf1c28b63b7a8a Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 20 Feb 2025 20:00:10 +0100 Subject: [PATCH 2/6] Fix --- .../RemoveUnusedValueAssignmentTests.cs | 68 ++++++++++++++----- .../Core/Extensions/OperationExtensions.cs | 11 ++- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs index 09bd17cbd8476..ee78cee868bdb 100644 --- a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs +++ b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs @@ -10089,34 +10089,66 @@ void M() """); } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72829")] - public async Task TestWriteIntoRefStructParameter() + [Fact] + 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 buffer) + private static void M(ref int destinationIndex, Span 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 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; } } """, diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs index cedc0a32aab41..ab18e27cd975f 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs +++ b/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. + // 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); } else if (operation.Parent is IVariableInitializerOperation variableInitializerOperation) { From 696f317dee4e0c4d2752c2c96d639fa83ed0ba4f Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Thu, 20 Feb 2025 20:38:44 +0100 Subject: [PATCH 3/6] Expand doc comment --- .../Compiler/Core/Extensions/OperationExtensions.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs index ab18e27cd975f..50ebf574b4e1d 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs @@ -217,6 +217,12 @@ where void Foo(in T v) // 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. + // Put in other words, we think of "write" in this context as "changing the value of the value type" + // The "value" for value types is just the value itself, except for ref-like types where we think of + // the "value" more as the "reference" itself (think of it as a pointer). + // s[0] = ... where s is value type and not ref-like type: Read+Write (the conceptual value of s changes) + // s[0] = ... where s is value type and ref-like type: Read (the ceonceptual value of s (i.e, the reference) doesn't change) + // s = ... is always considered a write regardless of what "s" is (this is handled by the IAssignmentOperation condition) return operation.Type.IsRefLikeType ? ValueUsageInfo.Read : ValueUsageInfo.Read | GetValueUsageInfo(operation.Parent, containingSymbol); From 3eb8e1b7b9a83dc064517736027e759694f85ded Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 20 Feb 2025 23:54:43 -0800 Subject: [PATCH 4/6] Move check to 'make field readonly' --- .../RemoveUnusedValueAssignmentTests.cs | 33 +++++++++++++++++-- ...ractMakeFieldReadonlyDiagnosticAnalyzer.cs | 19 +++++++++-- .../Core/Extensions/OperationExtensions.cs | 22 ------------- 3 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs index ee78cee868bdb..8a44864938647 100644 --- a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs +++ b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs @@ -10147,8 +10147,37 @@ 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; + // 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; + } + } + """, + 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; } } """, diff --git a/src/Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs index b1e5e48045c6f..22979e5ea92f6 100644 --- a/src/Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs @@ -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; + } + 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: diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs index 50ebf574b4e1d..3cc761fd828bd 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs @@ -205,28 +205,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. - // We consider a potential write only if the type is not a ref-like type. - // 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. - // Put in other words, we think of "write" in this context as "changing the value of the value type" - // The "value" for value types is just the value itself, except for ref-like types where we think of - // the "value" more as the "reference" itself (think of it as a pointer). - // s[0] = ... where s is value type and not ref-like type: Read+Write (the conceptual value of s changes) - // s[0] = ... where s is value type and ref-like type: Read (the ceonceptual value of s (i.e, the reference) doesn't change) - // s = ... is always considered a write regardless of what "s" is (this is handled by the IAssignmentOperation condition) - return operation.Type.IsRefLikeType - ? ValueUsageInfo.Read - : ValueUsageInfo.Read | GetValueUsageInfo(operation.Parent, containingSymbol); - } else if (operation.Parent is IVariableInitializerOperation variableInitializerOperation) { if (variableInitializerOperation.Parent is IVariableDeclaratorOperation variableDeclaratorOperation) From e96bc3a1621c7bf91077cfdf2db5bb138a5000c5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 20 Feb 2025 23:59:33 -0800 Subject: [PATCH 5/6] Add test --- .../RemoveUnusedValueAssignmentTests.cs | 5 +++++ .../Compiler/Core/Extensions/OperationExtensions.cs | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs index 8a44864938647..a0cd8ae3d72fe 100644 --- a/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs +++ b/src/Analyzers/CSharp/Tests/RemoveUnusedParametersAndValues/RemoveUnusedValueAssignmentTests.cs @@ -10182,6 +10182,11 @@ public int this[int index] } """, ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + LanguageVersion = LanguageVersion.CSharp12, + TestState = + { + OutputKind = OutputKind.ConsoleApplication, + } }.RunAsync(); } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs index 3cc761fd828bd..0ae859bd6b33a 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/OperationExtensions.cs @@ -24,6 +24,13 @@ public static bool IsTargetOfObjectMemberInitializer(this IOperation operation) /// Returns the for the given operation. /// This extension can be removed once https://github.com/dotnet/roslyn/issues/25057 is implemented. /// + /// + /// 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 through. 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). + /// Put another way, this only returns 'write' when certain that the entire value is absolutely + /// entirely overwritten. + /// public static ValueUsageInfo GetValueUsageInfo(this IOperation operation, ISymbol containingSymbol) { /* From 7f67f6b661a8eb6b2ee2ccc9db636cd84d042e94 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 21 Feb 2025 00:00:46 -0800 Subject: [PATCH 6/6] Update src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt --- .../ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt b/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt index 2bdd939afa897..3075a24a86c70 100644 --- a/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt +++ b/src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.CSharp.txt @@ -1177,7 +1177,6 @@ Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.get_GetEnumeratorMethod Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.get_IsAsynchronous Microsoft.CodeAnalysis.CSharp.ForEachStatementInfo.get_MoveNextMethod Microsoft.CodeAnalysis.CSharp.InterceptableLocation -Microsoft.CodeAnalysis.CSharp.InterceptableLocation.Equals(Microsoft.CodeAnalysis.CSharp.InterceptableLocation) Microsoft.CodeAnalysis.CSharp.InterceptableLocation.Equals(System.Object) Microsoft.CodeAnalysis.CSharp.InterceptableLocation.GetDisplayLocation Microsoft.CodeAnalysis.CSharp.InterceptableLocation.GetHashCode