Merge pull request #4119 from icsharpcode/fix/2372-address-taken-by

Fix #2372: report a field's address loads on their own

ICSharpCode.ILSpyX/Analyzers/Builtin/FieldAccessAnalyzer.cs

@@ -33,42 +33,61 @@ using ILOpCode = System.Reflection.Metadata.ILOpCode;
 namespace ICSharpCode.ILSpyX.Analyzers.Builtin
 {
 	/// <summary>
-	/// Finds methods where this field is read.
+	/// Finds methods where this field is written.
 	/// </summary>
 	[ExportAnalyzer(Header = "Assigned By", Order = 20)]
 	[Shared]
 	class AssignedByFieldAccessAnalyzer : FieldAccessAnalyzer
 	{
-		public AssignedByFieldAccessAnalyzer() : base(true) { }
+		public AssignedByFieldAccessAnalyzer() : base(FieldAccessKind.Write) { }
 	}
 
 	/// <summary>
-	/// Finds methods where this field is written.
+	/// Finds methods where this field is read.
 	/// </summary>
 	[ExportAnalyzer(Header = "Read By", Order = 10)]
 	[Shared]
 	class ReadByFieldAccessAnalyzer : FieldAccessAnalyzer
 	{
-		public ReadByFieldAccessAnalyzer() : base(false) { }
+		public ReadByFieldAccessAnalyzer() : base(FieldAccessKind.Read) { }
 	}
 
 	/// <summary>
-	/// Finds methods where this field is read or written.
+	/// Finds methods that load this field's address.
+	/// </summary>
+	[ExportAnalyzer(Header = "Address Taken By", Order = 30)]
+	[Shared]
+	class AddressTakenByFieldAccessAnalyzer : FieldAccessAnalyzer
+	{
+		public AddressTakenByFieldAccessAnalyzer() : base(FieldAccessKind.AddressOf) { }
+	}
+
+	enum FieldAccessKind
+	{
+		Read,
+		Write,
+		AddressOf
+	}
+
+	/// <summary>
+	/// Finds methods that access this field in one particular way.
 	/// </summary>
 	class FieldAccessAnalyzer : IAnalyzer
 	{
 		const GetMemberOptions Options = GetMemberOptions.IgnoreInheritedMembers | GetMemberOptions.ReturnMemberDefinitions;
 
-		readonly bool showWrites; // true: show writes; false: show read access
+		readonly FieldAccessKind kind;
 
-		public FieldAccessAnalyzer(bool showWrites)
+		public FieldAccessAnalyzer(FieldAccessKind kind)
 		{
-			this.showWrites = showWrites;
+			this.kind = kind;
 		}
 
 		public bool Show(ISymbol? symbol)
 		{
-			return symbol is IField field && (!showWrites || !field.IsConst);
+			// A constant is inlined at every use: there is nothing to assign to and no address
+			// to take.
+			return symbol is IField field && (kind == FieldAccessKind.Read || !field.IsConst);
 		}
 
 		public IEnumerable<ISymbol> Analyze(ISymbol analyzedSymbol, AnalyzerContext context)
@@ -201,13 +220,18 @@ namespace ICSharpCode.ILSpyX.Analyzers.Builtin
 			{
 				case ILOpCode.Ldfld:
 				case ILOpCode.Ldsfld:
-					return !showWrites;
+					return kind == FieldAccessKind.Read;
 				case ILOpCode.Stfld:
 				case ILOpCode.Stsfld:
-					return showWrites;
+					return kind == FieldAccessKind.Write;
 				case ILOpCode.Ldflda:
 				case ILOpCode.Ldsflda:
-					return true; // always show address-loading
+					// An address load says only that something needed a reference to the field.
+					// What happens through that reference is decided by the consumer - calling a
+					// method on a value-type field reads it, passing it as a ref argument may
+					// write it - and the IL scan here does not look at the consumer, so it is
+					// neither a read nor a write.
+					return kind == FieldAccessKind.AddressOf;
 				default:
 					return false;
 			}

ILSpy.Tests/Analyzers/Library/FieldAccessAnalyzerTests.cs

@@ -0,0 +1,98 @@
+// Copyright (c) 2026 Siegfried Pammer
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy of this
+// software and associated documentation files (the "Software"), to deal in the Software
+// without restriction, including without limitation the rights to use, copy, modify, merge,
+// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons
+// to whom the Software is furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all copies or
+// substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
+// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
+// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
+// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+// DEALINGS IN THE SOFTWARE.
+
+using System.Linq;
+
+using AwesomeAssertions;
+
+using ICSharpCode.Decompiler.TypeSystem;
+using ICSharpCode.ILSpy.Languages;
+using ICSharpCode.ILSpyX;
+using ICSharpCode.ILSpyX.Analyzers;
+using ICSharpCode.ILSpyX.Analyzers.Builtin;
+
+using NUnit.Framework;
+
+namespace ICSharpCode.ILSpy.Tests.Analyzers.Library;
+
+/// <summary>
+/// Loading a field's address says only that something needed a reference to it, not what was
+/// done through that reference: `flag.ToString()` on a value-type field emits ldflda and writes
+/// nothing. Counting it as an assignment put read-only uses under "Assigned By" (issue #2372),
+/// so it is reported on its own instead.
+/// </summary>
+[TestFixture]
+public class FieldAccessAnalyzerTests
+{
+	AssemblyList assemblyList = null!;
+	CSharpLanguage language = null!;
+	ITypeDefinition typeDefinition = null!;
+
+	[OneTimeSetUp]
+	public void Setup()
+	{
+		assemblyList = new AssemblyList();
+		var testAssembly = assemblyList.OpenAssembly(typeof(FieldAccessAnalyzerTests).Assembly.Location);
+		assemblyList.OpenAssembly(typeof(void).Assembly.Location);
+		language = new CSharpLanguage();
+		typeDefinition = testAssembly.GetTypeSystemOrNull()!
+			.FindType(typeof(TestCases.Main.FieldAccess))
+			.GetDefinition()!;
+	}
+
+	string[] Analyze(IAnalyzer analyzer, string fieldName)
+	{
+		var context = new AnalyzerContext { AssemblyList = assemblyList, Language = language };
+		var field = typeDefinition.Fields.Single(f => f.Name == fieldName);
+		return analyzer.Analyze(field, context).OfType<IEntity>().Select(e => e.Name).ToArray();
+	}
+
+	[TestCase("instanceFlag", "ReadsInstanceFlagByAddress")]
+	[TestCase("staticFlag", "ReadsStaticFlagByAddress")]
+	public void An_Address_Load_Is_Not_An_Assignment(string fieldName, string addressUser)
+	{
+		Analyze(new AssignedByFieldAccessAnalyzer(), fieldName)
+			.Should().NotContain(addressUser, "taking the address is not a write");
+	}
+
+	[TestCase("instanceFlag", "ReadsInstanceFlagByAddress")]
+	[TestCase("staticFlag", "ReadsStaticFlagByAddress")]
+	public void An_Address_Load_Is_Not_A_Read_Either(string fieldName, string addressUser)
+	{
+		Analyze(new ReadByFieldAccessAnalyzer(), fieldName)
+			.Should().NotContain(addressUser, "what the address is used for is not known here");
+	}
+
+	[TestCase("instanceFlag", "ReadsInstanceFlagByAddress")]
+	[TestCase("staticFlag", "ReadsStaticFlagByAddress")]
+	public void An_Address_Load_Is_Reported_On_Its_Own(string fieldName, string addressUser)
+	{
+		Analyze(new AddressTakenByFieldAccessAnalyzer(), fieldName)
+			.Should().Contain(addressUser);
+	}
+
+	[Test]
+	public void Plain_Reads_And_Writes_Are_Unaffected()
+	{
+		Analyze(new ReadByFieldAccessAnalyzer(), "instanceFlag").Should().Contain("ReadsInstanceFlag");
+		Analyze(new AssignedByFieldAccessAnalyzer(), "instanceFlag").Should().Contain("WritesInstanceFlag");
+		Analyze(new AssignedByFieldAccessAnalyzer(), "staticFlag").Should().Contain("WritesStaticFlag");
+		Analyze(new AddressTakenByFieldAccessAnalyzer(), "instanceFlag")
+			.Should().NotContain("WritesInstanceFlag", "a plain stfld takes no address");
+	}
+}

ILSpy.Tests/Analyzers/Library/TestCases/MainAssembly.cs

@@ -33,4 +33,40 @@ namespace ICSharpCode.ILSpy.Tests.Analyzers.Library.TestCases.Main
 			return int.Parse("1234");
 		}
 	}
+
+	// Fixture for the field-access analysers. A field of a value type reached through a
+	// method call is loaded by address (ldflda/ldsflda), which says nothing about whether
+	// the call writes to it - issue #2372.
+	class FieldAccess
+	{
+		public bool instanceFlag;
+		public static bool staticFlag;
+
+		public string ReadsInstanceFlagByAddress()
+		{
+			// callvirt Boolean::ToString(ldflda instanceFlag)
+			return instanceFlag.ToString();
+		}
+
+		public static string ReadsStaticFlagByAddress()
+		{
+			// call Boolean::ToString(ldsflda staticFlag)
+			return staticFlag.ToString();
+		}
+
+		public bool ReadsInstanceFlag()
+		{
+			return instanceFlag;
+		}
+
+		public void WritesInstanceFlag()
+		{
+			instanceFlag = true;
+		}
+
+		public static void WritesStaticFlag()
+		{
+			staticFlag = true;
+		}
+	}
 }