From 7572b57eeebf1ce66c44a2f469de0c438bb58c87 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Mon, 7 Sep 2026 23:00:53 +0200 Subject: [PATCH] Fix #2372: report a field's address loads on their own Loading a field's address was counted as both a read and a write, so a use that only reads - calling a method on a value-type field emits ldflda and writes nothing - was listed under "Assigned By". An address load says only that something needed a reference to the field. What happens through that reference is up to the consumer, and the IL scan here does not look at it, so the load is neither a read nor a write and gets a category of its own. Special-casing the methods known not to mutate their receiver would answer this one screenshot and leave every ref argument still guessing. Assisted-by: Claude:claude-opus-5:Claude Code --- .../Analyzers/Builtin/FieldAccessAnalyzer.cs | 48 ++++++--- .../Library/FieldAccessAnalyzerTests.cs | 98 +++++++++++++++++++ .../Library/TestCases/MainAssembly.cs | 36 +++++++ 3 files changed, 170 insertions(+), 12 deletions(-) create mode 100644 ILSpy.Tests/Analyzers/Library/FieldAccessAnalyzerTests.cs diff --git a/ICSharpCode.ILSpyX/Analyzers/Builtin/FieldAccessAnalyzer.cs b/ICSharpCode.ILSpyX/Analyzers/Builtin/FieldAccessAnalyzer.cs index 1cad39d0d..9c5203ae0 100644 --- a/ICSharpCode.ILSpyX/Analyzers/Builtin/FieldAccessAnalyzer.cs +++ b/ICSharpCode.ILSpyX/Analyzers/Builtin/FieldAccessAnalyzer.cs @@ -33,42 +33,61 @@ using ILOpCode = System.Reflection.Metadata.ILOpCode; namespace ICSharpCode.ILSpyX.Analyzers.Builtin { /// - /// Finds methods where this field is read. + /// Finds methods where this field is written. /// [ExportAnalyzer(Header = "Assigned By", Order = 20)] [Shared] class AssignedByFieldAccessAnalyzer : FieldAccessAnalyzer { - public AssignedByFieldAccessAnalyzer() : base(true) { } + public AssignedByFieldAccessAnalyzer() : base(FieldAccessKind.Write) { } } /// - /// Finds methods where this field is written. + /// Finds methods where this field is read. /// [ExportAnalyzer(Header = "Read By", Order = 10)] [Shared] class ReadByFieldAccessAnalyzer : FieldAccessAnalyzer { - public ReadByFieldAccessAnalyzer() : base(false) { } + public ReadByFieldAccessAnalyzer() : base(FieldAccessKind.Read) { } + } + + /// + /// Finds methods that load this field's address. + /// + [ExportAnalyzer(Header = "Address Taken By", Order = 30)] + [Shared] + class AddressTakenByFieldAccessAnalyzer : FieldAccessAnalyzer + { + public AddressTakenByFieldAccessAnalyzer() : base(FieldAccessKind.AddressOf) { } + } + + enum FieldAccessKind + { + Read, + Write, + AddressOf } /// - /// Finds methods where this field is read or written. + /// Finds methods that access this field in one particular way. /// 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 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; } diff --git a/ILSpy.Tests/Analyzers/Library/FieldAccessAnalyzerTests.cs b/ILSpy.Tests/Analyzers/Library/FieldAccessAnalyzerTests.cs new file mode 100644 index 000000000..32214a490 --- /dev/null +++ b/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; + +/// +/// 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. +/// +[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().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"); + } +} diff --git a/ILSpy.Tests/Analyzers/Library/TestCases/MainAssembly.cs b/ILSpy.Tests/Analyzers/Library/TestCases/MainAssembly.cs index 655625135..7d6754b0f 100644 --- a/ILSpy.Tests/Analyzers/Library/TestCases/MainAssembly.cs +++ b/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; + } + } }