From 77372c7cf70223bfa88ee9237cdc9cfb2520e73c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 12 Jun 2026 14:06:22 -0700 Subject: [PATCH 01/11] Add CSWINRT2021 diagnostic descriptor for reading write-only 'Span' parameters Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AnalyzerReleases.Shipped.md | 3 ++- .../Diagnostics/DiagnosticDescriptors.cs | 13 +++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md b/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md index e1c6e605f..cd4010d2a 100644 --- a/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md +++ b/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md @@ -26,4 +26,5 @@ CSWINRT2016 | WindowsRuntime.SourceGenerator | Warning | Public authored type mi CSWINRT2017 | WindowsRuntime.SourceGenerator | Warning | Public authored type mixing '[ContractVersion]' and '[Version]' CSWINRT2018 | WindowsRuntime.SourceGenerator | Warning | '[WindowsRuntimeNativeExposedType]' target type cannot be instantiated CSWINRT2019 | WindowsRuntime.SourceGenerator | Warning | '[WindowsRuntimeNativeExposedType]' target type is not a projected class -CSWINRT2020 | WindowsRuntime.SourceGenerator | Warning | Duplicate '[WindowsRuntimeNativeExposedType]' target type \ No newline at end of file +CSWINRT2020 | WindowsRuntime.SourceGenerator | Warning | Duplicate '[WindowsRuntimeNativeExposedType]' target type +CSWINRT2021 | WindowsRuntime.SourceGenerator | Warning | Reading from a write-only 'Span' parameter diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs index 84567ce6c..908a3daea 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs @@ -286,4 +286,17 @@ internal static partial class DiagnosticDescriptors description: "A given type should only be used as the target of a single '[WindowsRuntimeNativeExposedType]' attribute in an assembly. CCW marshalling code is only generated once per type, so additional applications of the attribute for the same type have no effect.", helpLinkUri: "https://github.com/microsoft/CsWinRT", customTags: WellKnownDiagnosticTags.CompilationEnd); + + /// + /// Gets a for reading from a write-only parameter on a Windows Runtime method. + /// + public static readonly DiagnosticDescriptor WriteOnlySpanParameterRead = new( + id: "CSWINRT2021", + title: "Reading from a write-only 'Span' parameter", + messageFormat: """The 'Span' parameter '{0}' is projected as a fill array in the Windows Runtime ABI, which is write-only, so the method implementation should only write to it and not read from it""", + category: "WindowsRuntime.SourceGenerator", + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "A 'Span' parameter on a Windows Runtime method is projected as a fill array in the ABI, meaning the implementation is only allowed to write to it, not read from it. Reading from such a parameter (e.g. by indexing it for a value or a readonly reference, converting it to 'ReadOnlySpan', or iterating over it) is not supported. Use 'ReadOnlySpan' for a parameter that should be read from instead.", + helpLinkUri: "https://github.com/microsoft/CsWinRT"); } \ No newline at end of file From b4a7b730bdb9d7c4b2e76e7920c524108c152db5 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 12 Jun 2026 14:23:40 -0700 Subject: [PATCH 02/11] Add WriteOnlySpanParameterAnalyzer for CSWINRT2021 Detects reads from 'Span' parameters on Windows Runtime methods, which are projected as write-only fill arrays in the ABI. Covers indexer reads, conversions to 'ReadOnlySpan', and 'foreach' iteration, while skipping write-only usages (assignment targets, 'out'/'ref' arguments, writable 'ref' aliases and loop variables) to avoid false positives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../WriteOnlySpanParameterAnalyzer.cs | 238 ++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs new file mode 100644 index 000000000..e4e26a6fa --- /dev/null +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -0,0 +1,238 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace WindowsRuntime.SourceGenerator.Diagnostics; + +/// +/// A diagnostic analyzer that warns when the implementation of a Windows Runtime method reads from a +/// parameter, which is projected as a write-only fill array in the ABI. +/// +/// +/// In the Windows Runtime ABI, array parameters use one of three conventions: a parameter +/// is a "pass array" ([in], read-only), an out T[] parameter is a "receive array" ([out] with byref), and +/// a parameter is a "fill array" ([out] without byref). A fill array is write-only: the +/// implementation is given a buffer allocated by the caller that it is expected to fill, and reading from it is not supported. +/// This analyzer detects the most common ways a method might read from such a parameter, while avoiding false positives on +/// write-only usages. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class WriteOnlySpanParameterAnalyzer : DiagnosticAnalyzer +{ + /// + public override ImmutableArray SupportedDiagnostics { get; } = [DiagnosticDescriptors.WriteOnlySpanParameterRead]; + + /// + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + + context.RegisterCompilationStartAction(static context => + { + // 'Span' parameters are only projected as fill arrays when authoring a Windows Runtime component + if (!context.Options.AnalyzerConfigOptionsProvider.GlobalOptions.GetCsWinRTComponent()) + { + return; + } + + // Get the 'System.Span' symbol (the type that is projected as a write-only fill array) + if (context.Compilation.GetTypeByMetadataName("System.Span`1") is not { } spanType) + { + return; + } + + // Get the 'System.ReadOnlySpan' symbol (used to detect conversions that would allow reads) + if (context.Compilation.GetTypeByMetadataName("System.ReadOnlySpan`1") is not { } readOnlySpanType) + { + return; + } + + // This handles reading the value (or a readonly reference) of an element, such as: + // + // _ = span[i]; + // Foo(span[i]); + // ref readonly var x = ref span[i]; + // Foo(in span[i]); + // span[i]++; + // span[i] += 1; + context.RegisterOperationAction(context => + { + IPropertyReferenceOperation operation = (IPropertyReferenceOperation)context.Operation; + + // We only care about the 'Span' indexer (i.e. 'span[i]'), not other properties (e.g. 'Length') + if (!operation.Property.IsIndexer) + { + return; + } + + // The indexer must be invoked directly on a write-only 'Span' parameter + if (operation.Instance is not IParameterReferenceOperation { Parameter: { } parameter } || + !IsWindowsRuntimeFillArrayParameter(parameter, spanType)) + { + return; + } + + // Skip usages that only write to the element, which are valid for a fill array + if (IsWriteOnlyElementUsage(operation)) + { + return; + } + + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Syntax.GetLocation(), + parameter.Name)); + }, OperationKind.PropertyReference); + + // This handles converting the span to 'ReadOnlySpan', which would allow reads, such as: + // + // ReadOnlySpan readOnlySpan = span; + // Foo((ReadOnlySpan)span); + context.RegisterOperationAction(context => + { + IConversionOperation operation = (IConversionOperation)context.Operation; + + // The conversion must target 'ReadOnlySpan' (a read-only view over the span elements) + if (!SymbolEqualityComparer.Default.Equals(operation.Type?.OriginalDefinition, readOnlySpanType)) + { + return; + } + + // The operand must be a write-only 'Span' parameter + if (operation.Operand is not IParameterReferenceOperation { Parameter: { } parameter } || + !IsWindowsRuntimeFillArrayParameter(parameter, spanType)) + { + return; + } + + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Syntax.GetLocation(), + parameter.Name)); + }, OperationKind.Conversion); + + // This handles iterating over the span, which reads each element, such as: + // + // foreach (int x in span) + // { + // } + context.RegisterOperationAction(context => + { + if (context.Operation is not IForEachLoopOperation operation) + { + return; + } + + // The iterated collection is the span parameter, possibly wrapped in an identity conversion + IOperation collection = operation.Collection is IConversionOperation { Operand: { } operand } + ? operand + : operation.Collection; + + // The iterated collection must be a write-only 'Span' parameter + if (collection is not IParameterReferenceOperation { Parameter: { } parameter } || + !IsWindowsRuntimeFillArrayParameter(parameter, spanType)) + { + return; + } + + // Iterating with a writable 'ref' loop variable may be used to fill the span, so skip it to + // avoid false positives (by-value and 'ref readonly' loop variables can only read elements). + if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref }) + { + return; + } + + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Collection.Syntax.GetLocation(), + parameter.Name)); + }, OperationKind.Loop); + }); + } + + /// + /// Checks whether a given parameter is a write-only parameter on a Windows + /// Runtime method (i.e. a parameter that is projected as a fill array in the ABI). + /// + /// The parameter to check. + /// The for . + /// Whether is a write-only fill array parameter. + private static bool IsWindowsRuntimeFillArrayParameter(IParameterSymbol parameter, INamedTypeSymbol spanType) + { + // The parameter must be a by-value 'System.Span': only this is projected as a fill array (a 'ref', + // 'in' or 'out' variant is not a valid Windows Runtime parameter, and 'ReadOnlySpan' is a pass array). + if (parameter.RefKind is not RefKind.None || + !SymbolEqualityComparer.Default.Equals(parameter.Type.OriginalDefinition, spanType)) + { + return false; + } + + // The parameter must belong to an ordinary method, a constructor, or an explicit interface + // implementation (and not e.g. a local function, a lambda, an operator or a property accessor). + if (parameter.ContainingSymbol is not IMethodSymbol + { + MethodKind: MethodKind.Ordinary or MethodKind.Constructor or MethodKind.ExplicitInterfaceImplementation + } method) + { + return false; + } + + // The containing type must be a public, top-level class (i.e. an authored runtime class). Other type + // kinds can't have method bodies with fill array parameters, and nested types are never projected. + if (method.ContainingType is not { TypeKind: TypeKind.Class, DeclaredAccessibility: Accessibility.Public, ContainingType: null }) + { + return false; + } + + // Public methods (including overrides and implicit interface implementations) are part of the ABI surface + if (method.DeclaredAccessibility is Accessibility.Public) + { + return true; + } + + // Explicit interface implementations are also part of the ABI surface, through the interfaces they + // implement. Only public interfaces are considered, as those are the ones projected to the Windows Runtime. + foreach (IMethodSymbol implementedMethod in method.ExplicitInterfaceImplementations) + { + if (implementedMethod.ContainingType.DeclaredAccessibility is Accessibility.Public) + { + return true; + } + } + + return false; + } + + /// + /// Checks whether a given indexer reference is only used to write to the + /// target element (which is valid for a fill array), as opposed to also reading its current value. + /// + /// The for the indexer access. + /// Whether is a write-only usage of the element. + private static bool IsWriteOnlyElementUsage(IPropertyReferenceOperation operation) + { + return operation.Parent switch + { + // 'span[i] = value' (the element is the target of the assignment): this only writes to the element. + // Note that a compound assignment ('span[i] += value') is not handled here, as it also reads. + ISimpleAssignmentOperation simpleAssignment => ReferenceEquals(simpleAssignment.Target, operation), + + // 'Foo(out span[i])' or 'Foo(ref span[i])': the callee may write to the element, and we can't tell + // whether it also reads it, so we conservatively treat these as write-only to avoid false positives. + IArgumentOperation { Parameter.RefKind: RefKind.Out or RefKind.Ref } => true, + + // 'ref var x = ref span[i]' (a writable 'ref' alias to the element): the alias may be used to write + // to the element, so we also conservatively treat it as write-only (a 'ref readonly' alias cannot). + IVariableInitializerOperation { Parent: IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref } } => true, + + // Any other usage reads the value (e.g. as a value, a 'ref readonly' alias, an 'in' argument, a + // compound assignment, or an increment/decrement), which is not valid for a write-only fill array. + _ => false + }; + } +} From b870a933f8d8eb35d05a96dced2141ac3e821c23 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 12 Jun 2026 14:23:45 -0700 Subject: [PATCH 03/11] Add tests for WriteOnlySpanParameterAnalyzer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Test_WriteOnlySpanParameterAnalyzer.cs | 363 ++++++++++++++++++ 1 file changed, 363 insertions(+) create mode 100644 src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs new file mode 100644 index 000000000..5727454cc --- /dev/null +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -0,0 +1,363 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Threading.Tasks; +using WindowsRuntime.SourceGenerator.Diagnostics; +using WindowsRuntime.SourceGenerator.Tests.Helpers; + +namespace WindowsRuntime.SourceGenerator.Tests; + +using VerifyCS = CSharpAnalyzerTest; + +/// +/// Tests for . +/// +[TestClass] +public sealed class Test_WriteOnlySpanParameterAnalyzer +{ + // --- Tests where the analyzer should NOT warn --- + + [TestMethod] + public async Task WriteOnlyUsages_DoNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + span[0] = 1; // Writing to an element is valid + int length = span.Length; // Reading non-indexer properties is valid + bool empty = span.IsEmpty; + Out(out span[1]); // Writing through an 'out' argument is valid + Ref(ref span[2]); // Passing by 'ref' may write, so it is allowed + ref int slot = ref span[3]; // A writable 'ref' alias may write, so it is allowed + slot = 4; + + foreach (ref int x in span) // A writable 'ref' loop variable may write + { + x = 5; + } + } + + private static void Out(out int x) => x = 0; + + private static void Ref(ref int x) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ForEachWithWritableRefVariable_DoesNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + // A writable 'ref' loop variable can be used to write to all elements sequentially + foreach (ref var x in span) + { + } + + foreach (ref int y in span) + { + y = 1; + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadOnlySpanParameter_DoesNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Read(ReadOnlySpan span) + { + int x = span[0]; + ReadOnlySpan other = span; + + foreach (int y in span) + { + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task NotComponent_DoesNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + int x = span[0]; + ReadOnlySpan other = span; + + foreach (int y in span) + { + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [TestMethod] + public async Task PrivateMethod_DoesNotWarn() + { + const string source = """ + using System; + + public class Sample + { + private void Fill(Span span) + { + int x = span[0]; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task InternalClass_DoesNotWarn() + { + const string source = """ + using System; + + internal class Sample + { + public void Fill(Span span) + { + int x = span[0]; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task NestedClass_DoesNotWarn() + { + const string source = """ + using System; + + public class Outer + { + public class Inner + { + public void Fill(Span span) + { + int x = span[0]; + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task Struct_DoesNotWarn() + { + const string source = """ + using System; + + public struct Sample + { + public void Fill(Span span) + { + int x = span[0]; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task LocalFunction_DoesNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Method() + { + static void Fill(Span span) + { + int x = span[0]; + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + // --- Tests where the analyzer SHOULD warn --- + + [TestMethod] + public async Task IndexerReads_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + int value = {|CSWINRT2021:span[0]|}; + Value({|CSWINRT2021:span[1]|}); + ref readonly int readOnlyReference = ref {|CSWINRT2021:span[2]|}; + In(in {|CSWINRT2021:span[3]|}); + {|CSWINRT2021:span[4]|}++; + {|CSWINRT2021:span[5]|} += 1; + } + + private static void Value(int x) + { + } + + private static void In(in int x) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadOnlySpanConversions_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + ReadOnlySpan implicitConversion = {|CSWINRT2021:span|}; + ReadOnlySpan explicitConversion = {|CSWINRT2021:(ReadOnlySpan)span|}; + + Read({|CSWINRT2021:span|}); + } + + private static void Read(ReadOnlySpan span) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ForEachReads_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + foreach (int x in {|CSWINRT2021:span|}) + { + } + + foreach (ref readonly int y in {|CSWINRT2021:span|}) + { + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task StaticMethod_Warns() + { + const string source = """ + using System; + + public class Sample + { + public static void Fill(Span span) + { + int x = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task Constructor_Warns() + { + const string source = """ + using System; + + public class Sample + { + public Sample(Span span) + { + int x = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ExplicitInterfaceImplementation_Warns() + { + const string source = """ + using System; + + public interface IFillable + { + void Fill(Span span); + } + + public class Sample : IFillable + { + void IFillable.Fill(Span span) + { + int x = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } +} From 87a4ca00036a0219475fc20cb60eabfb4f43dd76 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 24 Jul 2026 11:50:39 -0700 Subject: [PATCH 04/11] Warn on reads through writable 'ref' foreach variables A 'foreach' loop with a writable 'ref' loop variable can legitimately be used to fill a write-only 'Span' parameter, so the loop itself is not reported. However, the loop variable aliases the current element, so reading through it is still a read from the fill array. The 'foreach' handler now walks the loop body and reports every read through such a loop variable, reusing the same write-only heuristics already used for indexer accesses. The loop variable can't be captured by a lambda or be ref reassigned, so all of its aliasing usages are guaranteed to be in the body. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 55ba2269-4b18-4924-895a-bd614f8ed6a8 --- .../WriteOnlySpanParameterAnalyzer.cs | 65 +++++++++++++++++-- .../Test_WriteOnlySpanParameterAnalyzer.cs | 56 ++++++++++++++++ 2 files changed, 114 insertions(+), 7 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index e4e26a6fa..c5b40494c 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -121,6 +121,13 @@ public override void Initialize(AnalysisContext context) // foreach (int x in span) // { // } + // + // It also handles reading the current element through a writable 'ref' loop variable, such as: + // + // foreach (ref int x in span) + // { + // int y = x; + // } context.RegisterOperationAction(context => { if (context.Operation is not IForEachLoopOperation operation) @@ -140,10 +147,14 @@ public override void Initialize(AnalysisContext context) return; } - // Iterating with a writable 'ref' loop variable may be used to fill the span, so skip it to - // avoid false positives (by-value and 'ref readonly' loop variables can only read elements). - if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref }) + // Iterating with a writable 'ref' loop variable may be used to fill the span, so the loop + // itself is valid. In that case, only the individual reads through the loop variable (which + // aliases the current element) are reported. By-value and 'ref readonly' loop variables can + // only ever read the elements, so for those the loop itself is reported instead. + if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol: { RefKind: RefKind.Ref } loopVariable }) { + ReportElementReadsThroughLoopVariable(context, operation.Body, loopVariable, parameter); + return; } @@ -209,13 +220,53 @@ private static bool IsWindowsRuntimeFillArrayParameter(IParameterSymbol paramete } /// - /// Checks whether a given indexer reference is only used to write to the - /// target element (which is valid for a fill array), as opposed to also reading its current value. + /// Reports all reads of the current element of a write-only parameter that are + /// performed through the writable ref loop variable of a foreach loop iterating over it. + /// + /// The context to report diagnostics to. + /// The body of the foreach loop declaring . + /// The writable ref loop variable, aliasing the current element. + /// The write-only parameter being iterated over. + private static void ReportElementReadsThroughLoopVariable( + OperationAnalysisContext context, + IOperation loopBody, + ILocalSymbol loopVariable, + IParameterSymbol parameter) + { + foreach (IOperation operation in loopBody.Descendants()) + { + // We only care about references to the loop variable, as those alias an element of the span. + // The loop variable can't be captured by a lambda or ref reassigned, so all aliasing usages + // of the current element are guaranteed to appear directly in the body of the loop. + if (operation is not ILocalReferenceOperation { Local: { } local } || + !SymbolEqualityComparer.Default.Equals(local, loopVariable)) + { + continue; + } + + // Skip usages that only write to the element, which are valid for a fill array + if (IsWriteOnlyElementUsage(operation)) + { + continue; + } + + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Syntax.GetLocation(), + parameter.Name)); + } + } + + /// + /// Checks whether a given reference to an element of a write-only parameter is + /// only used to write to that element (which is valid for a fill array), as opposed to also reading its value. /// - /// The for the indexer access. + /// The operation referencing the element (either an indexer access, or a ref alias). /// Whether is a write-only usage of the element. - private static bool IsWriteOnlyElementUsage(IPropertyReferenceOperation operation) + private static bool IsWriteOnlyElementUsage(IOperation operation) { + // The examples below use an indexer access for the element reference, but the same reasoning + // applies verbatim to a 'ref' alias to the element (e.g. a writable 'foreach' loop variable). return operation.Parent switch { // 'span[i] = value' (the element is the target of the assignment): this only writes to the element. diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs index 5727454cc..a54a9d25e 100644 --- a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -302,6 +302,62 @@ public void Fill(Span span) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } + [TestMethod] + public async Task ForEachWritableRefVariableReads_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + foreach (ref int x in span) + { + x = 0; // Writing through the alias is valid + Out(out x); // Writing through an 'out' argument is valid + Ref(ref x); // Passing by 'ref' may write, so it is allowed + ref int slot = ref x; // A writable 'ref' alias may write, so it is allowed + slot = 1; + + int value = {|CSWINRT2021:x|}; + Value({|CSWINRT2021:x|}); + In(in {|CSWINRT2021:x|}); + RefReadOnly(in {|CSWINRT2021:x|}); + ref readonly int readOnlyReference = ref {|CSWINRT2021:x|}; + {|CSWINRT2021:x|}++; + {|CSWINRT2021:x|} += 1; + + if (value > 0) + { + Value({|CSWINRT2021:x|}); // Nested reads are detected as well + } + } + } + + private static void Value(int x) + { + } + + private static void In(in int x) + { + } + + private static void RefReadOnly(ref readonly int x) + { + } + + private static void Out(out int x) => x = 0; + + private static void Ref(ref int x) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + [TestMethod] public async Task StaticMethod_Warns() { From 4132227aea330ae0dae3809342f04f2c465ea5d0 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 12:58:15 -0700 Subject: [PATCH 05/11] Analyze write-only 'Span' reads per operation block The analyzer used to register a separate operation action for each kind of read it detects, which means every candidate read was inspected in isolation. That makes it impossible to reason about the code that runs before a read, which is needed to tell apart a read of an uninitialized element from a read of one the method has already filled in. Switch to a single operation block action that walks the body of each method with a fill array parameter and dispatches to a dedicated helper per read kind. The method level checks now run once per body instead of once per candidate read, and the parameter checks make sure the referenced parameter belongs to the method being analyzed (and not to a nested lambda or local function). There is no change in behavior: this is only preparation for the next commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../WriteOnlySpanParameterAnalyzer.cs | 388 +++++++++++------- 1 file changed, 245 insertions(+), 143 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index c5b40494c..06506150b 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -52,188 +52,206 @@ public override void Initialize(AnalysisContext context) return; } - // This handles reading the value (or a readonly reference) of an element, such as: - // - // _ = span[i]; - // Foo(span[i]); - // ref readonly var x = ref span[i]; - // Foo(in span[i]); - // span[i]++; - // span[i] += 1; - context.RegisterOperationAction(context => + // The whole body of a method is analyzed at once, rather than each operation in isolation, so that + // each candidate read can also be inspected in the context of the code that necessarily runs before it + context.RegisterOperationBlockAction(context => { - IPropertyReferenceOperation operation = (IPropertyReferenceOperation)context.Operation; - - // We only care about the 'Span' indexer (i.e. 'span[i]'), not other properties (e.g. 'Length') - if (!operation.Property.IsIndexer) - { - return; - } - - // The indexer must be invoked directly on a write-only 'Span' parameter - if (operation.Instance is not IParameterReferenceOperation { Parameter: { } parameter } || - !IsWindowsRuntimeFillArrayParameter(parameter, spanType)) - { - return; - } - - // Skip usages that only write to the element, which are valid for a fill array - if (IsWriteOnlyElementUsage(operation)) - { - return; - } - - context.ReportDiagnostic(Diagnostic.Create( - DiagnosticDescriptors.WriteOnlySpanParameterRead, - operation.Syntax.GetLocation(), - parameter.Name)); - }, OperationKind.PropertyReference); - - // This handles converting the span to 'ReadOnlySpan', which would allow reads, such as: - // - // ReadOnlySpan readOnlySpan = span; - // Foo((ReadOnlySpan)span); - context.RegisterOperationAction(context => - { - IConversionOperation operation = (IConversionOperation)context.Operation; - - // The conversion must target 'ReadOnlySpan' (a read-only view over the span elements) - if (!SymbolEqualityComparer.Default.Equals(operation.Type?.OriginalDefinition, readOnlySpanType)) - { - return; - } - - // The operand must be a write-only 'Span' parameter - if (operation.Operand is not IParameterReferenceOperation { Parameter: { } parameter } || - !IsWindowsRuntimeFillArrayParameter(parameter, spanType)) + // Only look at methods that are part of the Windows Runtime ABI surface and that actually have + // at least one fill array parameter, which is the only thing this analyzer is concerned with + if (context.OwningSymbol is not IMethodSymbol method || + !HasFillArrayParameter(method, spanType) || + !IsWindowsRuntimeMethod(method)) { return; } - context.ReportDiagnostic(Diagnostic.Create( - DiagnosticDescriptors.WriteOnlySpanParameterRead, - operation.Syntax.GetLocation(), - parameter.Name)); - }, OperationKind.Conversion); - - // This handles iterating over the span, which reads each element, such as: - // - // foreach (int x in span) - // { - // } - // - // It also handles reading the current element through a writable 'ref' loop variable, such as: - // - // foreach (ref int x in span) - // { - // int y = x; - // } - context.RegisterOperationAction(context => - { - if (context.Operation is not IForEachLoopOperation operation) + foreach (IOperation operationBlock in context.OperationBlocks) { - return; + foreach (IOperation operation in operationBlock.DescendantsAndSelf()) + { + switch (operation) + { + case IPropertyReferenceOperation propertyReference: + AnalyzeElementRead(context, propertyReference, method, spanType); + break; + case IConversionOperation conversion: + AnalyzeSpanConversion(context, conversion, method, spanType, readOnlySpanType); + break; + case IForEachLoopOperation forEachLoop: + AnalyzeForEachLoop(context, forEachLoop, method, spanType); + break; + default: + break; + } + } } + }); + }); + } - // The iterated collection is the span parameter, possibly wrapped in an identity conversion - IOperation collection = operation.Collection is IConversionOperation { Operand: { } operand } - ? operand - : operation.Collection; - - // The iterated collection must be a write-only 'Span' parameter - if (collection is not IParameterReferenceOperation { Parameter: { } parameter } || - !IsWindowsRuntimeFillArrayParameter(parameter, spanType)) - { - return; - } + /// + /// Analyzes a property reference, to detect reads of an element of a write-only parameter. + /// + /// The context to report diagnostics to. + /// The property reference to analyze. + /// The method being analyzed. + /// The for . + /// + /// This handles reading the value (or a readonly reference) of an element, such as: + /// + /// _ = span[i]; + /// Foo(span[i]); + /// ref readonly var x = ref span[i]; + /// Foo(in span[i]); + /// span[i]++; + /// span[i] += 1; + /// + /// + private static void AnalyzeElementRead( + OperationBlockAnalysisContext context, + IPropertyReferenceOperation operation, + IMethodSymbol method, + INamedTypeSymbol spanType) + { + // We only care about the 'Span' indexer (i.e. 'span[i]'), not other properties (e.g. 'Length') + if (!operation.Property.IsIndexer) + { + return; + } - // Iterating with a writable 'ref' loop variable may be used to fill the span, so the loop - // itself is valid. In that case, only the individual reads through the loop variable (which - // aliases the current element) are reported. By-value and 'ref readonly' loop variables can - // only ever read the elements, so for those the loop itself is reported instead. - if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol: { RefKind: RefKind.Ref } loopVariable }) - { - ReportElementReadsThroughLoopVariable(context, operation.Body, loopVariable, parameter); + // The indexer must be invoked directly on a write-only 'Span' parameter + if (operation.Instance is not IParameterReferenceOperation { Parameter: { } parameter } || + !IsFillArrayParameter(parameter, method, spanType)) + { + return; + } - return; - } + // Skip usages that only write to the element, which are valid for a fill array + if (IsWriteOnlyElementUsage(operation)) + { + return; + } - context.ReportDiagnostic(Diagnostic.Create( - DiagnosticDescriptors.WriteOnlySpanParameterRead, - operation.Collection.Syntax.GetLocation(), - parameter.Name)); - }, OperationKind.Loop); - }); + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Syntax.GetLocation(), + parameter.Name)); } /// - /// Checks whether a given parameter is a write-only parameter on a Windows - /// Runtime method (i.e. a parameter that is projected as a fill array in the ABI). + /// Analyzes a conversion, to detect a write-only parameter being converted to + /// , which would allow reading all of its elements. /// - /// The parameter to check. + /// The context to report diagnostics to. + /// The conversion to analyze. + /// The method being analyzed. /// The for . - /// Whether is a write-only fill array parameter. - private static bool IsWindowsRuntimeFillArrayParameter(IParameterSymbol parameter, INamedTypeSymbol spanType) + /// The for . + /// + /// This handles converting the span to , such as: + /// + /// ReadOnlySpan<int> readOnlySpan = span; + /// Foo((ReadOnlySpan<int>)span); + /// + /// + private static void AnalyzeSpanConversion( + OperationBlockAnalysisContext context, + IConversionOperation operation, + IMethodSymbol method, + INamedTypeSymbol spanType, + INamedTypeSymbol readOnlySpanType) { - // The parameter must be a by-value 'System.Span': only this is projected as a fill array (a 'ref', - // 'in' or 'out' variant is not a valid Windows Runtime parameter, and 'ReadOnlySpan' is a pass array). - if (parameter.RefKind is not RefKind.None || - !SymbolEqualityComparer.Default.Equals(parameter.Type.OriginalDefinition, spanType)) + // The conversion must target 'ReadOnlySpan' (a read-only view over the span elements) + if (!SymbolEqualityComparer.Default.Equals(operation.Type?.OriginalDefinition, readOnlySpanType)) { - return false; + return; } - // The parameter must belong to an ordinary method, a constructor, or an explicit interface - // implementation (and not e.g. a local function, a lambda, an operator or a property accessor). - if (parameter.ContainingSymbol is not IMethodSymbol - { - MethodKind: MethodKind.Ordinary or MethodKind.Constructor or MethodKind.ExplicitInterfaceImplementation - } method) + // The operand must be a write-only 'Span' parameter + if (operation.Operand is not IParameterReferenceOperation { Parameter: { } parameter } || + !IsFillArrayParameter(parameter, method, spanType)) { - return false; + return; } - // The containing type must be a public, top-level class (i.e. an authored runtime class). Other type - // kinds can't have method bodies with fill array parameters, and nested types are never projected. - if (method.ContainingType is not { TypeKind: TypeKind.Class, DeclaredAccessibility: Accessibility.Public, ContainingType: null }) - { - return false; - } + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Syntax.GetLocation(), + parameter.Name)); + } - // Public methods (including overrides and implicit interface implementations) are part of the ABI surface - if (method.DeclaredAccessibility is Accessibility.Public) + /// + /// Analyzes a foreach loop, to detect iteration over a write-only parameter. + /// + /// The context to report diagnostics to. + /// The foreach loop to analyze. + /// The method being analyzed. + /// The for . + /// + /// This handles iterating over the span, which reads each element, such as: + /// + /// foreach (int x in span) + /// { + /// } + /// + /// It also handles reading the current element through a writable ref loop variable, such as: + /// + /// foreach (ref int x in span) + /// { + /// int y = x; + /// } + /// + /// + private static void AnalyzeForEachLoop( + OperationBlockAnalysisContext context, + IForEachLoopOperation operation, + IMethodSymbol method, + INamedTypeSymbol spanType) + { + // The iterated collection is the span parameter, possibly wrapped in an identity conversion + IOperation collection = operation.Collection is IConversionOperation { Operand: { } operand } + ? operand + : operation.Collection; + + // The iterated collection must be a write-only 'Span' parameter + if (collection is not IParameterReferenceOperation { Parameter: { } parameter } || + !IsFillArrayParameter(parameter, method, spanType)) { - return true; + return; } - // Explicit interface implementations are also part of the ABI surface, through the interfaces they - // implement. Only public interfaces are considered, as those are the ones projected to the Windows Runtime. - foreach (IMethodSymbol implementedMethod in method.ExplicitInterfaceImplementations) + // Iterating with a writable 'ref' loop variable may be used to fill the span, so the loop + // itself is valid. In that case, only the individual reads through the loop variable (which + // aliases the current element) are reported. By-value and 'ref readonly' loop variables can + // only ever read the elements, so for those the loop itself is reported instead. + if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol: { RefKind: RefKind.Ref } loopVariable }) { - if (implementedMethod.ContainingType.DeclaredAccessibility is Accessibility.Public) - { - return true; - } + AnalyzeLoopVariableReads(context, operation.Body, parameter, loopVariable); + + return; } - return false; + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.WriteOnlySpanParameterRead, + operation.Collection.Syntax.GetLocation(), + parameter.Name)); } /// - /// Reports all reads of the current element of a write-only parameter that are - /// performed through the writable ref loop variable of a foreach loop iterating over it. + /// Analyzes the body of a foreach loop iterating over a write-only parameter + /// with a writable ref loop variable, to detect reads of the current element through that variable. /// /// The context to report diagnostics to. /// The body of the foreach loop declaring . - /// The writable ref loop variable, aliasing the current element. /// The write-only parameter being iterated over. - private static void ReportElementReadsThroughLoopVariable( - OperationAnalysisContext context, + /// The writable ref loop variable, aliasing the current element. + private static void AnalyzeLoopVariableReads( + OperationBlockAnalysisContext context, IOperation loopBody, - ILocalSymbol loopVariable, - IParameterSymbol parameter) + IParameterSymbol parameter, + ILocalSymbol loopVariable) { - foreach (IOperation operation in loopBody.Descendants()) + foreach (IOperation operation in loopBody.DescendantsAndSelf()) { // We only care about references to the loop variable, as those alias an element of the span. // The loop variable can't be captured by a lambda or ref reassigned, so all aliasing usages @@ -257,6 +275,90 @@ private static void ReportElementReadsThroughLoopVariable( } } + /// + /// Checks whether a given method has at least one write-only parameter. + /// + /// The method to check. + /// The for . + /// Whether has at least one write-only fill array parameter. + private static bool HasFillArrayParameter(IMethodSymbol method, INamedTypeSymbol spanType) + { + foreach (IParameterSymbol parameter in method.Parameters) + { + if (IsFillArrayParameter(parameter, method, spanType)) + { + return true; + } + } + + return false; + } + + /// + /// Checks whether a given parameter is a write-only parameter of a given method + /// (i.e. a parameter that is projected as a fill array in the Windows Runtime ABI). + /// + /// The parameter to check. + /// The method being analyzed. + /// The for . + /// Whether is a write-only fill array parameter of . + private static bool IsFillArrayParameter(IParameterSymbol parameter, IMethodSymbol method, INamedTypeSymbol spanType) + { + // The parameter must belong to the method being analyzed, and not to a nested lambda or local function, + // which are never projected (and could not reference the parameters of the method anyway, as a span + // is a 'ref struct' type, and as such it cannot be captured by any of them). + if (!SymbolEqualityComparer.Default.Equals(parameter.ContainingSymbol, method)) + { + return false; + } + + // The parameter must be a by-value 'System.Span': only this is projected as a fill array (a 'ref', + // 'in' or 'out' variant is not a valid Windows Runtime parameter, and 'ReadOnlySpan' is a pass array). + return + parameter.RefKind is RefKind.None && + SymbolEqualityComparer.Default.Equals(parameter.Type.OriginalDefinition, spanType); + } + + /// + /// Checks whether a given method is part of the Windows Runtime ABI surface of an authored component. + /// + /// The method to check. + /// Whether is projected to the Windows Runtime. + private static bool IsWindowsRuntimeMethod(IMethodSymbol method) + { + // The method must be an ordinary method, a constructor, or an explicit interface + // implementation (and not e.g. a local function, a lambda, an operator or a property accessor). + if (method.MethodKind is not (MethodKind.Ordinary or MethodKind.Constructor or MethodKind.ExplicitInterfaceImplementation)) + { + return false; + } + + // The containing type must be a public, top-level class (i.e. an authored runtime class). Other type + // kinds can't have method bodies with fill array parameters, and nested types are never projected. + if (method.ContainingType is not { TypeKind: TypeKind.Class, DeclaredAccessibility: Accessibility.Public, ContainingType: null }) + { + return false; + } + + // Public methods (including overrides and implicit interface implementations) are part of the ABI surface + if (method.DeclaredAccessibility is Accessibility.Public) + { + return true; + } + + // Explicit interface implementations are also part of the ABI surface, through the interfaces they + // implement. Only public interfaces are considered, as those are the ones projected to the Windows Runtime. + foreach (IMethodSymbol implementedMethod in method.ExplicitInterfaceImplementations) + { + if (implementedMethod.ContainingType.DeclaredAccessibility is Accessibility.Public) + { + return true; + } + } + + return false; + } + /// /// Checks whether a given reference to an element of a write-only parameter is /// only used to write to that element (which is valid for a fill array), as opposed to also reading its value. From 03632605fe4b5695e12f8c452d2961e57b8f164e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 13:10:40 -0700 Subject: [PATCH 06/11] Don't warn for reads preceded by a write covering the same location Reading back a value the method itself has just written is perfectly valid: the element being read is no longer uninitialized, so there is nothing to warn about. The analyzer had no notion of that, which made it warn on a number of legitimate patterns, such as filling a span and then reading it back. A read is now skipped when a write covering the same location is guaranteed to run before it: - 'span.Clear()' and 'span.Fill(value)' initialize every element, so they cover any subsequent read, including 'foreach' loops and conversions to 'ReadOnlySpan'. - 'span[i] = value' and 'Foo(out span[i])' cover subsequent reads of that same element, including 'in' arguments and 'ref readonly' aliases. - The same two forms through a writable 'ref' 'foreach' loop variable cover subsequent reads of the element that variable aliases. The analysis is deliberately conservative, as missing a write only means an extra warning, whereas trusting one that doesn't actually happen would silently hide a real bug. A write is only ever considered when it appears in a statement that precedes (an ancestor of) the read within a common enclosing block, and when it is unconditionally reached within that statement: anything nested in a construct that might not run ('if', '?:', '??', '?.', 'switch', loops, 'try', lambdas, local functions, '&&' and '||') is ignored. Only definite writes count, so 'ref' arguments and writable 'ref' aliases are not treated as initializing. Finally, nothing is suppressed in a body containing a 'goto' (which could jump over the write), or when the span parameter or the index variable might have been reassigned in between. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../WriteOnlySpanParameterAnalyzer.cs | 457 +++++++++++++++++- 1 file changed, 443 insertions(+), 14 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index 06506150b..74d5130d2 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -19,6 +19,11 @@ namespace WindowsRuntime.SourceGenerator.Diagnostics; /// implementation is given a buffer allocated by the caller that it is expected to fill, and reading from it is not supported. /// This analyzer detects the most common ways a method might read from such a parameter, while avoiding false positives on /// write-only usages. +/// +/// Reading back a value the method itself has just written is valid, as the element being read is no longer uninitialized. +/// To account for that, a read is not reported when a write covering the same location is guaranteed to run before it, +/// such as a preceding span.Clear() or span.Fill(value) call, or a preceding assignment to that same element. +/// /// [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class WriteOnlySpanParameterAnalyzer : DiagnosticAnalyzer @@ -67,18 +72,22 @@ public override void Initialize(AnalysisContext context) foreach (IOperation operationBlock in context.OperationBlocks) { + // A 'goto' can jump over a write that would otherwise always run before a given read, so + // no read is ever suppressed in a body containing one (this is extremely rare in practice) + bool allowsSuppression = !ContainsGotoBranch(operationBlock); + foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { switch (operation) { case IPropertyReferenceOperation propertyReference: - AnalyzeElementRead(context, propertyReference, method, spanType); + AnalyzeElementRead(context, propertyReference, method, spanType, allowsSuppression); break; case IConversionOperation conversion: - AnalyzeSpanConversion(context, conversion, method, spanType, readOnlySpanType); + AnalyzeSpanConversion(context, conversion, method, spanType, readOnlySpanType, allowsSuppression); break; case IForEachLoopOperation forEachLoop: - AnalyzeForEachLoop(context, forEachLoop, method, spanType); + AnalyzeForEachLoop(context, forEachLoop, method, spanType, allowsSuppression); break; default: break; @@ -96,6 +105,7 @@ public override void Initialize(AnalysisContext context) /// The property reference to analyze. /// The method being analyzed. /// The for . + /// Whether reads preceded by a covering write can be suppressed. /// /// This handles reading the value (or a readonly reference) of an element, such as: /// @@ -111,7 +121,8 @@ private static void AnalyzeElementRead( OperationBlockAnalysisContext context, IPropertyReferenceOperation operation, IMethodSymbol method, - INamedTypeSymbol spanType) + INamedTypeSymbol spanType, + bool allowsSuppression) { // We only care about the 'Span' indexer (i.e. 'span[i]'), not other properties (e.g. 'Length') if (!operation.Property.IsIndexer) @@ -132,6 +143,14 @@ private static void AnalyzeElementRead( return; } + // Skip reads of an element that the method is guaranteed to have already written to + if (allowsSuppression && + operation.Arguments is [{ Value: { } index }] && + IsPrecededByCoveringWrite(operation, ReadTarget.ForElement(parameter, spanType, index))) + { + return; + } + context.ReportDiagnostic(Diagnostic.Create( DiagnosticDescriptors.WriteOnlySpanParameterRead, operation.Syntax.GetLocation(), @@ -147,6 +166,7 @@ private static void AnalyzeElementRead( /// The method being analyzed. /// The for . /// The for . + /// Whether reads preceded by a covering write can be suppressed. /// /// This handles converting the span to , such as: /// @@ -159,7 +179,8 @@ private static void AnalyzeSpanConversion( IConversionOperation operation, IMethodSymbol method, INamedTypeSymbol spanType, - INamedTypeSymbol readOnlySpanType) + INamedTypeSymbol readOnlySpanType, + bool allowsSuppression) { // The conversion must target 'ReadOnlySpan' (a read-only view over the span elements) if (!SymbolEqualityComparer.Default.Equals(operation.Type?.OriginalDefinition, readOnlySpanType)) @@ -174,6 +195,12 @@ private static void AnalyzeSpanConversion( return; } + // Skip conversions of a span that the method is guaranteed to have already filled entirely + if (allowsSuppression && IsPrecededByCoveringWrite(operation, ReadTarget.ForSpan(parameter, spanType))) + { + return; + } + context.ReportDiagnostic(Diagnostic.Create( DiagnosticDescriptors.WriteOnlySpanParameterRead, operation.Syntax.GetLocation(), @@ -187,6 +214,7 @@ private static void AnalyzeSpanConversion( /// The foreach loop to analyze. /// The method being analyzed. /// The for . + /// Whether reads preceded by a covering write can be suppressed. /// /// This handles iterating over the span, which reads each element, such as: /// @@ -206,7 +234,8 @@ private static void AnalyzeForEachLoop( OperationBlockAnalysisContext context, IForEachLoopOperation operation, IMethodSymbol method, - INamedTypeSymbol spanType) + INamedTypeSymbol spanType, + bool allowsSuppression) { // The iterated collection is the span parameter, possibly wrapped in an identity conversion IOperation collection = operation.Collection is IConversionOperation { Operand: { } operand } @@ -226,8 +255,14 @@ private static void AnalyzeForEachLoop( // only ever read the elements, so for those the loop itself is reported instead. if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol: { RefKind: RefKind.Ref } loopVariable }) { - AnalyzeLoopVariableReads(context, operation.Body, parameter, loopVariable); + AnalyzeLoopVariableReads(context, operation.Body, ReadTarget.ForAlias(parameter, spanType, loopVariable), allowsSuppression); + + return; + } + // Skip iterations over a span that the method is guaranteed to have already filled entirely + if (allowsSuppression && IsPrecededByCoveringWrite(operation.Collection, ReadTarget.ForSpan(parameter, spanType))) + { return; } @@ -242,14 +277,14 @@ private static void AnalyzeForEachLoop( /// with a writable ref loop variable, to detect reads of the current element through that variable. /// /// The context to report diagnostics to. - /// The body of the foreach loop declaring . - /// The write-only parameter being iterated over. - /// The writable ref loop variable, aliasing the current element. + /// The body of the foreach loop declaring the loop variable. + /// The element aliased by the writable ref loop variable of the loop. + /// Whether reads preceded by a covering write can be suppressed. private static void AnalyzeLoopVariableReads( OperationBlockAnalysisContext context, IOperation loopBody, - IParameterSymbol parameter, - ILocalSymbol loopVariable) + ReadTarget target, + bool allowsSuppression) { foreach (IOperation operation in loopBody.DescendantsAndSelf()) { @@ -257,7 +292,7 @@ private static void AnalyzeLoopVariableReads( // The loop variable can't be captured by a lambda or ref reassigned, so all aliasing usages // of the current element are guaranteed to appear directly in the body of the loop. if (operation is not ILocalReferenceOperation { Local: { } local } || - !SymbolEqualityComparer.Default.Equals(local, loopVariable)) + !SymbolEqualityComparer.Default.Equals(local, target.Alias)) { continue; } @@ -268,10 +303,16 @@ private static void AnalyzeLoopVariableReads( continue; } + // Skip reads of an element that the method is guaranteed to have already written to + if (allowsSuppression && IsPrecededByCoveringWrite(operation, target)) + { + continue; + } + context.ReportDiagnostic(Diagnostic.Create( DiagnosticDescriptors.WriteOnlySpanParameterRead, operation.Syntax.GetLocation(), - parameter.Name)); + target.Parameter.Name)); } } @@ -388,4 +429,392 @@ private static bool IsWriteOnlyElementUsage(IOperation operation) _ => false }; } + + /// + /// Checks whether a given read from a write-only parameter is preceded by a write + /// that is guaranteed to run before it, and that covers the same location. Such a read only ever observes values + /// that the method itself has just written, so it is valid even though the parameter is a fill array. + /// + /// The operation performing the read. + /// The location that accesses. + /// Whether is preceded by a write covering . + /// + /// Only writes appearing in a statement that precedes (an ancestor of) in some enclosing + /// block are considered, and only when they are unconditionally reached within that statement. This is meant to + /// be conservative: it is fine to miss a write that does happen, but a write must never be reported as guaranteed + /// unless it necessarily runs first, as that would silently hide an actual read of an uninitialized element. + /// + private static bool IsPrecededByCoveringWrite(IOperation read, ReadTarget target) + { + IOperation current = read; + IOperation? parent = read.Parent; + + while (parent is not null) + { + // Never look at the enclosing method when the read is inside a lambda or a local function, as those + // can be invoked at any point. This can't normally be reached, given that a span is a 'ref struct' + // type, and as such it cannot be captured by either of them, but it is cheap to guard against. + if (parent is IAnonymousFunctionOperation or ILocalFunctionOperation) + { + return false; + } + + // Only the statements within a block are ordered with respect to one another. Any other kind of parent + // is just skipped, meaning the search resumes from the closest enclosing statement in the parent block. + if (parent is IBlockOperation block) + { + int index = block.Operations.IndexOf(current); + + for (int i = index - 1; i >= 0; i--) + { + if (IsGuaranteedCoveringWrite(block.Operations[i], target)) + { + // A preceding write only covers the read if the operands it depends on can't have changed in + // between. If they can, no earlier write can be relied upon either, as it would span an even + // wider range of statements, so there is no point in continuing the search past this one. + return AreOperandsStable(block.Operations, i, index, target); + } + } + } + + current = parent; + parent = parent.Parent; + } + + return false; + } + + /// + /// Checks whether a given operation contains a write covering a target location that is guaranteed to be reached. + /// + /// The operation to inspect. + /// The location that the write should cover. + /// Whether necessarily performs a write covering . + private static bool IsGuaranteedCoveringWrite(IOperation operation, ReadTarget target) + { + // Constructs that might not run at all can never guarantee that a write nested in them will happen + if (IsConditionallyExecuted(operation)) + { + return false; + } + + if (IsCoveringWrite(operation, target)) + { + return true; + } + + foreach (IOperation child in operation.ChildOperations) + { + if (IsGuaranteedCoveringWrite(child, target)) + { + return true; + } + } + + return false; + } + + /// + /// Checks whether a given operation is a write covering a target location. + /// + /// The operation to inspect. + /// The location that the write should cover. + /// Whether writes to all of . + private static bool IsCoveringWrite(IOperation operation, ReadTarget target) + { + // 'span.Clear()' and 'span.Fill(value)' initialize every element of the span, so + // they cover any subsequent read from it, no matter which elements it looks at. + if (operation is IInvocationOperation { TargetMethod: { Name: "Clear", Parameters: [] } or { Name: "Fill", Parameters: [_] } } invocation && + SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType.OriginalDefinition, target.SpanType) && + invocation.Instance is { } instance && + IsReferenceTo(instance, target.Parameter)) + { + return true; + } + + // 'span[i] = value' and 'Foo(out span[i])' initialize a single element, so they only + // cover reads of that same element (i.e. reads through an equivalent index expression). + if (target.Index is { } index && + operation is IPropertyReferenceOperation { Property.IsIndexer: true, Instance: { } spanInstance, Arguments: [{ Value: { } writeIndex }] } && + IsReferenceTo(spanInstance, target.Parameter) && + IsDefiniteWrite(operation) && + AreIndexesEquivalent(writeIndex, index)) + { + return true; + } + + // 'x = value' and 'Foo(out x)' through a writable 'ref' 'foreach' loop variable initialize + // the element it aliases, so they cover reads of that element through the same variable. + return + target.Alias is { } alias && + IsReferenceTo(operation, alias) && + IsDefiniteWrite(operation); + } + + /// + /// Checks whether a given operation might not be reached during the execution of its parent operation. + /// + /// The operation to check. + /// Whether might be skipped, or run at some later point. + private static bool IsConditionallyExecuted(IOperation operation) + { + return operation is + IConditionalOperation or // 'if' statements and '?:' expressions + IConditionalAccessOperation or // '?.' and '?[]' accesses + ICoalesceOperation or // '??' expressions + ICoalesceAssignmentOperation or // '??=' expressions + ISwitchOperation or // 'switch' statements + ISwitchExpressionOperation or // 'switch' expressions + ILoopOperation or // all loops, as the body might never run + ITryOperation or // 'try' blocks, as an exception might skip the rest of the body + IAnonymousFunctionOperation or // lambdas and anonymous methods + ILocalFunctionOperation or // local functions + IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr }; + } + + /// + /// Checks whether the operands a covering write depends on (i.e. the span parameter itself, and the index + /// variable, if the target is a single element) can't have been modified between that write and a later read. + /// + /// The statements of the block containing both the write and the read. + /// The index of the statement containing the write. + /// The index of the statement containing the read. + /// The location that the write covers. + /// Whether the write still applies to when the read is reached. + private static bool AreOperandsStable(ImmutableArray statements, int writeIndex, int readIndex, ReadTarget target) + { + ISymbol? indexSymbol = target.IndexSymbol; + + for (int i = writeIndex; i <= readIndex; i++) + { + foreach (IOperation operation in statements[i].DescendantsAndSelf()) + { + // Reassigning the span parameter would make the write apply to an entirely different buffer + if (IsPotentialWrite(operation, target.Parameter)) + { + return false; + } + + // Likewise, modifying the index variable would make the two accesses target different elements + if (indexSymbol is not null && IsPotentialWrite(operation, indexSymbol)) + { + return false; + } + } + } + + return true; + } + + /// + /// Checks whether a given operation is a reference to a local or parameter that might modify its value. + /// + /// The operation to check. + /// The local or parameter to look for. + /// Whether might modify the value of . + private static bool IsPotentialWrite(IOperation operation, ISymbol symbol) + { + return IsReferenceTo(operation, symbol) && operation.Parent switch + { + // 'x = value', 'x += value' and 'x ??= value' + IAssignmentOperation assignment => ReferenceEquals(assignment.Target, operation), + + // 'x++' and 'x--' + IIncrementOrDecrementOperation incrementOrDecrement => ReferenceEquals(incrementOrDecrement.Target, operation), + + // 'Foo(out x)' and 'Foo(ref x)' + IArgumentOperation { Parameter.RefKind: RefKind.Out or RefKind.Ref } => true, + + // 'ref var alias = ref x', which can then be used to write to it + IVariableInitializerOperation { Parent: IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref } } => true, + + _ => false + }; + } + + /// + /// Checks whether a given reference to an element of a write-only parameter is + /// guaranteed to initialize that element, as opposed to just possibly writing to it. + /// + /// The operation referencing the element (either an indexer access, or a ref alias). + /// Whether necessarily writes to the element. + private static bool IsDefiniteWrite(IOperation operation) + { + return operation.Parent switch + { + // 'span[i] = value' or 'x = value': the element is definitely assigned + ISimpleAssignmentOperation simpleAssignment => ReferenceEquals(simpleAssignment.Target, operation), + + // 'Foo(out span[i])' or 'Foo(out x)': the callee has to assign the element before returning + IArgumentOperation { Parameter.RefKind: RefKind.Out } => true, + + // Note that 'ref' arguments and writable 'ref' aliases are not definite writes: the callee (or the + // code using the alias) might never actually write to the element, so they can't be relied upon. + _ => false + }; + } + + /// + /// Checks whether a given operation is a reference to a specific local or parameter. + /// + /// The operation to check. + /// The local or parameter to look for. + /// Whether is a reference to . + private static bool IsReferenceTo(IOperation operation, ISymbol symbol) + { + return operation switch + { + ILocalReferenceOperation localReference => SymbolEqualityComparer.Default.Equals(localReference.Local, symbol), + IParameterReferenceOperation parameterReference => SymbolEqualityComparer.Default.Equals(parameterReference.Parameter, symbol), + _ => false + }; + } + + /// + /// Checks whether two index expressions are guaranteed to produce the same value. + /// + /// The first index expression to compare. + /// The second index expression to compare. + /// Whether and necessarily refer to the same element. + /// + /// Two index expressions referring to the same variable are only equivalent as long as that variable is not + /// modified in between. It is up to callers to validate that (see ). + /// + private static bool AreIndexesEquivalent(IOperation left, IOperation right) + { + // Constant indices are equivalent when they have the same value (e.g. 'span[0]' and 'span[0]') + if (left.ConstantValue is { HasValue: true, Value: { } leftValue } && + right.ConstantValue is { HasValue: true, Value: { } rightValue }) + { + return leftValue.Equals(rightValue); + } + + // Otherwise, only a reference to the same local or parameter is recognized (e.g. 'span[i]' in a loop) + return (UnwrapImplicitConversions(left), UnwrapImplicitConversions(right)) switch + { + (ILocalReferenceOperation leftLocal, ILocalReferenceOperation rightLocal) => SymbolEqualityComparer.Default.Equals(leftLocal.Local, rightLocal.Local), + (IParameterReferenceOperation leftParameter, IParameterReferenceOperation rightParameter) => SymbolEqualityComparer.Default.Equals(leftParameter.Parameter, rightParameter.Parameter), + _ => false + }; + } + + /// + /// Unwraps all compiler inserted conversions from a given operation (e.g. the widening conversion in span[byteIndex]). + /// + /// The operation to unwrap. + /// The innermost operand of that is not an implicit conversion. + private static IOperation? UnwrapImplicitConversions(IOperation? operation) + { + while (operation is IConversionOperation { IsImplicit: true } conversion) + { + operation = conversion.Operand; + } + + return operation; + } + + /// + /// Checks whether a given operation block contains a goto branch. + /// + /// The operation block to inspect. + /// Whether contains a goto branch. + private static bool ContainsGotoBranch(IOperation operationBlock) + { + foreach (IOperation operation in operationBlock.DescendantsAndSelf()) + { + if (operation is IBranchOperation { BranchKind: BranchKind.GoTo }) + { + return true; + } + } + + return false; + } + + /// + /// Describes the location that a read from a write-only parameter accesses, so + /// that writes which are guaranteed to have already initialized that same location can be recognized. + /// + private readonly struct ReadTarget + { + /// + /// Creates a new instance with the specified parameters. + /// + /// The write-only parameter being read from. + /// The for . + /// The index of the element being read, if the read goes through the span indexer. + /// The writable ref foreach loop variable aliasing the element being read, if any. + private ReadTarget(IParameterSymbol parameter, INamedTypeSymbol spanType, IOperation? index, ILocalSymbol? alias) + { + Parameter = parameter; + SpanType = spanType; + Index = index; + Alias = alias; + } + + /// + /// Gets the write-only parameter being read from. + /// + public IParameterSymbol Parameter { get; } + + /// + /// Gets the for . + /// + public INamedTypeSymbol SpanType { get; } + + /// + /// Gets the index of the element being read, if the read goes through the span indexer. + /// + public IOperation? Index { get; } + + /// + /// Gets the writable ref foreach loop variable aliasing the element being read, if any. + /// + public ILocalSymbol? Alias { get; } + + /// + /// Gets the local or parameter that refers to, if it is a plain variable reference. + /// + public ISymbol? IndexSymbol => UnwrapImplicitConversions(Index) switch + { + ILocalReferenceOperation localReference => localReference.Local, + IParameterReferenceOperation parameterReference => parameterReference.Parameter, + _ => null + }; + + /// + /// Creates a for a read of all the elements of a span (e.g. a foreach loop). + /// + /// The write-only parameter being read from. + /// The for . + /// The resulting value. + public static ReadTarget ForSpan(IParameterSymbol parameter, INamedTypeSymbol spanType) + { + return new(parameter, spanType, index: null, alias: null); + } + + /// + /// Creates a for a read of a single element through the span indexer. + /// + /// The write-only parameter being read from. + /// The for . + /// The index of the element being read. + /// The resulting value. + public static ReadTarget ForElement(IParameterSymbol parameter, INamedTypeSymbol spanType, IOperation index) + { + return new(parameter, spanType, index, alias: null); + } + + /// + /// Creates a for a read of the element aliased by a writable ref loop variable. + /// + /// The write-only parameter being read from. + /// The for . + /// The writable ref foreach loop variable aliasing the element being read. + /// The resulting value. + public static ReadTarget ForAlias(IParameterSymbol parameter, INamedTypeSymbol spanType, ILocalSymbol alias) + { + return new(parameter, spanType, index: null, alias); + } + } } From b75c395f15dfb50b84e3a217f4fcce888582b988 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 13:10:41 -0700 Subject: [PATCH 07/11] Add tests for write-only 'Span' read suppression Cover the reads that are now recognized as valid, i.e. those preceded by a 'Clear()' or 'Fill(value)' call, by an assignment to the same element, or by an assignment through a writable 'ref' 'foreach' loop variable. Also cover the cases that must keep warning, to make sure the new logic can't hide an actual read of an uninitialized element: reads before the write, writes to a different element or to a different span, writes that are only reached conditionally (including in a 'switch' section or behind '&&'), writes that might not happen at all ('ref' arguments and writable 'ref' aliases), writes skipped by a 'goto', a same named extension method, and writes invalidated by the index variable or the span parameter being reassigned in between. The reads in 'ForEachWritableRefVariableReads_Warn' now come before the writes in the loop body, as the loop variable is otherwise initialized by the time they run, which is exactly the pattern that is no longer reported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../Test_WriteOnlySpanParameterAnalyzer.cs | 546 +++++++++++++++++- 1 file changed, 539 insertions(+), 7 deletions(-) diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs index a54a9d25e..c1d01a059 100644 --- a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -220,8 +220,187 @@ static void Fill(Span span) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } - // --- Tests where the analyzer SHOULD warn --- + [TestMethod] + public async Task ReadsAfterClear_DoNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, bool condition) + { + // Every element is initialized, so any subsequent read is fine + span.Clear(); + + int value = span[0]; + In(in span[1]); + ref readonly int readOnlyReference = ref span[2]; + ReadOnlySpan readOnlySpan = span; + + Read(span); + + foreach (int x in span) + { + } + + foreach (ref readonly int y in span) + { + } + + foreach (ref int z in span) + { + int nested = z; + } + + if (condition) + { + int inBranch = span[3]; + } + } + + private static void In(in int x) + { + } + + private static void Read(ReadOnlySpan span) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterFill_DoNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + span.Fill(42); + + int value = span[0]; + ReadOnlySpan readOnlySpan = span; + + foreach (int x in span) + { + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ElementReadsAfterWriteToSameIndex_DoNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + const int Index = 3; + + span[0] = 1; + int value = span[0]; // Reading back an element that was just written is fine + In(in span[0]); + RefReadOnly(in span[0]); + ref readonly int readOnlyReference = ref span[0]; + span[0]++; + span[0] += 1; + + Out(out span[1]); // An 'out' argument is also a definite write + int other = span[1]; + + span[Index] = 2; // Constant indices are compared by value + int constant = span[3]; + + for (int i = 0; i < span.Length; i++) + { + span[i] = i; + int inLoop = span[i]; + } + } + + private static void In(in int x) + { + } + + private static void RefReadOnly(ref readonly int x) + { + } + + private static void Out(out int x) => x = 0; + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task LoopVariableReadsAfterWrite_DoNotWarn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, bool condition) + { + foreach (ref int x in span) + { + x = 0; // The element is initialized through the alias + + int value = x; + Value(x); + In(in x); + RefReadOnly(in x); + ref readonly int readOnlyReference = ref x; + x++; + x += 1; + + if (condition) + { + Value(x); // Nested reads are fine as well + } + } + foreach (ref int y in span) + { + Out(out y); // An 'out' argument is also a definite write + + int value = y; + } + } + + private static void Value(int x) + { + } + + private static void In(in int x) + { + } + + private static void RefReadOnly(ref readonly int x) + { + } + + private static void Out(out int x) => x = 0; + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + // --- Tests where the analyzer SHOULD warn --- [TestMethod] public async Task IndexerReads_Warn() { @@ -314,12 +493,6 @@ public void Fill(Span span) { foreach (ref int x in span) { - x = 0; // Writing through the alias is valid - Out(out x); // Writing through an 'out' argument is valid - Ref(ref x); // Passing by 'ref' may write, so it is allowed - ref int slot = ref x; // A writable 'ref' alias may write, so it is allowed - slot = 1; - int value = {|CSWINRT2021:x|}; Value({|CSWINRT2021:x|}); In(in {|CSWINRT2021:x|}); @@ -332,6 +505,12 @@ public void Fill(Span span) { Value({|CSWINRT2021:x|}); // Nested reads are detected as well } + + x = 0; // Writing through the alias is valid + Out(out x); // Writing through an 'out' argument is valid + Ref(ref x); // Passing by 'ref' may write, so it is allowed + ref int slot = ref x; // A writable 'ref' alias may write, so it is allowed + slot = 1; } } @@ -416,4 +595,357 @@ void IFillable.Fill(Span span) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } + + [TestMethod] + public async Task ReadsBeforeWrite_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + // The write only covers reads that come after it + int value = {|CSWINRT2021:span[0]|}; + + span[0] = 1; + span.Clear(); + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ElementReadsAfterWriteToOtherIndex_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, int index) + { + span[0] = 1; + int value = {|CSWINRT2021:span[1]|}; // A different constant index + + span[index] = 2; + int other = {|CSWINRT2021:span[index + 1]|}; // Not provably the same index + + // A single element being initialized says nothing about all the other ones + ReadOnlySpan readOnlySpan = {|CSWINRT2021:span|}; + + foreach (int x in {|CSWINRT2021:span|}) + { + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ElementReadsAfterModifiedIndex_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + int i = 0; + + span[i] = 1; + i++; // The two accesses no longer target the same element + + int value = {|CSWINRT2021:span[i]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterReassignedSpan_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + span.Clear(); + span = default; // The write applies to a different buffer + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterConditionalWrite_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, bool condition) + { + if (condition) + { + span.Clear(); + } + + int value = {|CSWINRT2021:span[0]|}; + + while (condition) + { + span.Fill(1); // A loop body might never run + } + + int other = {|CSWINRT2021:span[1]|}; + + try + { + span.Clear(); // An exception might skip the rest of the block + } + catch (Exception) + { + } + + int last = {|CSWINRT2021:span[2]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterNonDefiniteWrite_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + Ref(ref span[0]); // The callee might never write to the element + int value = {|CSWINRT2021:span[0]|}; + + ref int slot = ref span[1]; // The alias might never be written to + int other = {|CSWINRT2021:span[1]|}; + + foreach (ref int x in span) + { + Ref(ref x); + + int nested = {|CSWINRT2021:x|}; + } + } + + private static void Ref(ref int x) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterWriteToOtherSpan_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, Span other) + { + other.Clear(); + other[0] = 1; + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterWriteSkippedByGoto_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, bool condition) + { + if (condition) + { + goto Read; + } + + span.Clear(); + + Read: + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task LoopVariableReadsBeforeWrite_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, bool condition) + { + foreach (ref int x in span) + { + int value = {|CSWINRT2021:x|}; // The write only covers later reads + + x = 0; + } + + foreach (ref int y in span) + { + if (condition) + { + y = 0; // The write might not happen + } + + int value = {|CSWINRT2021:y|}; + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterWriteInSwitchSection_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, int selector) + { + switch (selector) + { + case 0: + span.Clear(); + break; + default: + span.Fill(1); + break; + } + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterShortCircuitedWrite_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span, bool condition) + { + // The write is only reached when the left operand is 'true' + bool result = condition && Set(out span[0]); + + int value = {|CSWINRT2021:span[0]|}; + } + + private static bool Set(out int x) + { + x = 0; + + return true; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterExtensionMethodCall_Warn() + { + const string source = """ + using System; + + public static class SpanExtensions + { + public static void Clear(this Span span, bool flag) + { + } + } + + public class Sample + { + public void Fill(Span span) + { + span.Clear(true); // Not the 'Span.Clear()' method + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ForEachWithWriteInBody_Warns() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + // The write happens after each element has already been read + foreach (int x in {|CSWINRT2021:span|}) + { + span.Clear(); + } + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } } From c905bdc22a892919377840f40cd0a9ec3784cb04 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 13:55:07 -0700 Subject: [PATCH 08/11] Close the remaining holes in the covering write analysis Four cases were found where a read could be skipped even though the write it relies upon might not have happened, or applied to a different location: - Field and property initializers can reference the parameters of a primary constructor, but their owning symbol is the field or the property, not the constructor, so those blocks were not analyzed at all. Whether a parameter is a fill array is now decided from the parameter itself, as it was before the switch to an operation block action, and the owning symbol is only used to skip method bodies that can't possibly contain a read. - A deconstruction assignment ('(i, _) = ...') was not recognized as a write to the index variable, so a stale index was considered stable. - A writable 'ref' alias or a capture created before the write escaped the scan of the statements between the write and the read, which meant the index variable (or even the span parameter itself) could be modified without being noticed. Nothing is skipped anymore in a body that creates such an indirection, in the same way as for 'goto'. - A call to a method annotated with '[Conditional]' is removed entirely by the compiler, arguments included, when the associated preprocessor symbol is not defined, so a write nested in one is now treated as conditional. The well known symbols the analysis needs are bundled together while at it, as detecting '[Conditional]' calls requires one more of them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../WriteOnlySpanParameterAnalyzer.cs | 263 ++++++++++++------ .../Test_WriteOnlySpanParameterAnalyzer.cs | 130 +++++++++ 2 files changed, 306 insertions(+), 87 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index 74d5130d2..9f786978a 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -57,37 +57,46 @@ public override void Initialize(AnalysisContext context) return; } + // Also get the '[Conditional]' symbol, which is used to detect calls that might be removed entirely + INamedTypeSymbol? conditionalAttributeType = context.Compilation.GetTypeByMetadataName("System.Diagnostics.ConditionalAttribute"); + + WellKnownSymbols symbols = new(spanType, readOnlySpanType, conditionalAttributeType); + // The whole body of a method is analyzed at once, rather than each operation in isolation, so that // each candidate read can also be inspected in the context of the code that necessarily runs before it context.RegisterOperationBlockAction(context => { - // Only look at methods that are part of the Windows Runtime ABI surface and that actually have - // at least one fill array parameter, which is the only thing this analyzer is concerned with - if (context.OwningSymbol is not IMethodSymbol method || - !HasFillArrayParameter(method, spanType) || - !IsWindowsRuntimeMethod(method)) + // Fill array parameters can only be declared by a method on a public, top-level class (see + // 'IsWindowsRuntimeMethod'), which is a cheap way to skip most of the code being compiled + if (context.OwningSymbol.ContainingType is not { TypeKind: TypeKind.Class, DeclaredAccessibility: Accessibility.Public, ContainingType: null }) + { + return; + } + + // For a method, the parameters are known up front, so bodies that can't possibly read from a fill + // array parameter are skipped right away. That is not the case for field and property initializers, + // which can also reference the parameters of the primary constructor of their containing type. + if (context.OwningSymbol is IMethodSymbol method && !HasSpanParameter(method, spanType)) { return; } foreach (IOperation operationBlock in context.OperationBlocks) { - // A 'goto' can jump over a write that would otherwise always run before a given read, so - // no read is ever suppressed in a body containing one (this is extremely rare in practice) - bool allowsSuppression = !ContainsGotoBranch(operationBlock); + bool allowsSuppression = AllowsSuppression(operationBlock); foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { switch (operation) { case IPropertyReferenceOperation propertyReference: - AnalyzeElementRead(context, propertyReference, method, spanType, allowsSuppression); + AnalyzeElementRead(context, propertyReference, symbols, allowsSuppression); break; case IConversionOperation conversion: - AnalyzeSpanConversion(context, conversion, method, spanType, readOnlySpanType, allowsSuppression); + AnalyzeSpanConversion(context, conversion, symbols, allowsSuppression); break; case IForEachLoopOperation forEachLoop: - AnalyzeForEachLoop(context, forEachLoop, method, spanType, allowsSuppression); + AnalyzeForEachLoop(context, forEachLoop, symbols, allowsSuppression); break; default: break; @@ -103,8 +112,7 @@ public override void Initialize(AnalysisContext context) /// /// The context to report diagnostics to. /// The property reference to analyze. - /// The method being analyzed. - /// The for . + /// The well known symbols used by the analyzer. /// Whether reads preceded by a covering write can be suppressed. /// /// This handles reading the value (or a readonly reference) of an element, such as: @@ -120,8 +128,7 @@ public override void Initialize(AnalysisContext context) private static void AnalyzeElementRead( OperationBlockAnalysisContext context, IPropertyReferenceOperation operation, - IMethodSymbol method, - INamedTypeSymbol spanType, + WellKnownSymbols symbols, bool allowsSuppression) { // We only care about the 'Span' indexer (i.e. 'span[i]'), not other properties (e.g. 'Length') @@ -132,7 +139,7 @@ private static void AnalyzeElementRead( // The indexer must be invoked directly on a write-only 'Span' parameter if (operation.Instance is not IParameterReferenceOperation { Parameter: { } parameter } || - !IsFillArrayParameter(parameter, method, spanType)) + !IsFillArrayParameter(parameter, symbols)) { return; } @@ -146,7 +153,7 @@ private static void AnalyzeElementRead( // Skip reads of an element that the method is guaranteed to have already written to if (allowsSuppression && operation.Arguments is [{ Value: { } index }] && - IsPrecededByCoveringWrite(operation, ReadTarget.ForElement(parameter, spanType, index))) + IsPrecededByCoveringWrite(operation, ReadTarget.ForElement(parameter, symbols, index))) { return; } @@ -163,9 +170,7 @@ private static void AnalyzeElementRead( /// /// The context to report diagnostics to. /// The conversion to analyze. - /// The method being analyzed. - /// The for . - /// The for . + /// The well known symbols used by the analyzer. /// Whether reads preceded by a covering write can be suppressed. /// /// This handles converting the span to , such as: @@ -177,26 +182,24 @@ private static void AnalyzeElementRead( private static void AnalyzeSpanConversion( OperationBlockAnalysisContext context, IConversionOperation operation, - IMethodSymbol method, - INamedTypeSymbol spanType, - INamedTypeSymbol readOnlySpanType, + WellKnownSymbols symbols, bool allowsSuppression) { // The conversion must target 'ReadOnlySpan' (a read-only view over the span elements) - if (!SymbolEqualityComparer.Default.Equals(operation.Type?.OriginalDefinition, readOnlySpanType)) + if (!SymbolEqualityComparer.Default.Equals(operation.Type?.OriginalDefinition, symbols.ReadOnlySpanType)) { return; } // The operand must be a write-only 'Span' parameter if (operation.Operand is not IParameterReferenceOperation { Parameter: { } parameter } || - !IsFillArrayParameter(parameter, method, spanType)) + !IsFillArrayParameter(parameter, symbols)) { return; } // Skip conversions of a span that the method is guaranteed to have already filled entirely - if (allowsSuppression && IsPrecededByCoveringWrite(operation, ReadTarget.ForSpan(parameter, spanType))) + if (allowsSuppression && IsPrecededByCoveringWrite(operation, ReadTarget.ForSpan(parameter, symbols))) { return; } @@ -212,8 +215,7 @@ private static void AnalyzeSpanConversion( /// /// The context to report diagnostics to. /// The foreach loop to analyze. - /// The method being analyzed. - /// The for . + /// The well known symbols used by the analyzer. /// Whether reads preceded by a covering write can be suppressed. /// /// This handles iterating over the span, which reads each element, such as: @@ -233,8 +235,7 @@ private static void AnalyzeSpanConversion( private static void AnalyzeForEachLoop( OperationBlockAnalysisContext context, IForEachLoopOperation operation, - IMethodSymbol method, - INamedTypeSymbol spanType, + WellKnownSymbols symbols, bool allowsSuppression) { // The iterated collection is the span parameter, possibly wrapped in an identity conversion @@ -244,7 +245,7 @@ private static void AnalyzeForEachLoop( // The iterated collection must be a write-only 'Span' parameter if (collection is not IParameterReferenceOperation { Parameter: { } parameter } || - !IsFillArrayParameter(parameter, method, spanType)) + !IsFillArrayParameter(parameter, symbols)) { return; } @@ -255,13 +256,13 @@ private static void AnalyzeForEachLoop( // only ever read the elements, so for those the loop itself is reported instead. if (operation.LoopControlVariable is IVariableDeclaratorOperation { Symbol: { RefKind: RefKind.Ref } loopVariable }) { - AnalyzeLoopVariableReads(context, operation.Body, ReadTarget.ForAlias(parameter, spanType, loopVariable), allowsSuppression); + AnalyzeLoopVariableReads(context, operation.Body, ReadTarget.ForAlias(parameter, symbols, loopVariable), allowsSuppression); return; } // Skip iterations over a span that the method is guaranteed to have already filled entirely - if (allowsSuppression && IsPrecededByCoveringWrite(operation.Collection, ReadTarget.ForSpan(parameter, spanType))) + if (allowsSuppression && IsPrecededByCoveringWrite(operation.Collection, ReadTarget.ForSpan(parameter, symbols))) { return; } @@ -317,16 +318,16 @@ private static void AnalyzeLoopVariableReads( } /// - /// Checks whether a given method has at least one write-only parameter. + /// Checks whether a given method has at least one by-value parameter. /// /// The method to check. /// The for . - /// Whether has at least one write-only fill array parameter. - private static bool HasFillArrayParameter(IMethodSymbol method, INamedTypeSymbol spanType) + /// Whether has at least one by-value parameter. + private static bool HasSpanParameter(IMethodSymbol method, INamedTypeSymbol spanType) { foreach (IParameterSymbol parameter in method.Parameters) { - if (IsFillArrayParameter(parameter, method, spanType)) + if (IsSpanParameter(parameter, spanType)) { return true; } @@ -336,30 +337,38 @@ private static bool HasFillArrayParameter(IMethodSymbol method, INamedTypeSymbol } /// - /// Checks whether a given parameter is a write-only parameter of a given method - /// (i.e. a parameter that is projected as a fill array in the Windows Runtime ABI). + /// Checks whether a given parameter is a by-value parameter. /// /// The parameter to check. - /// The method being analyzed. /// The for . - /// Whether is a write-only fill array parameter of . - private static bool IsFillArrayParameter(IParameterSymbol parameter, IMethodSymbol method, INamedTypeSymbol spanType) + /// Whether is a by-value parameter. + private static bool IsSpanParameter(IParameterSymbol parameter, INamedTypeSymbol spanType) { - // The parameter must belong to the method being analyzed, and not to a nested lambda or local function, - // which are never projected (and could not reference the parameters of the method anyway, as a span - // is a 'ref struct' type, and as such it cannot be captured by any of them). - if (!SymbolEqualityComparer.Default.Equals(parameter.ContainingSymbol, method)) - { - return false; - } - - // The parameter must be a by-value 'System.Span': only this is projected as a fill array (a 'ref', - // 'in' or 'out' variant is not a valid Windows Runtime parameter, and 'ReadOnlySpan' is a pass array). + // Only a by-value 'System.Span' is projected as a fill array (a 'ref', 'in' or 'out' + // variant is not a valid Windows Runtime parameter, and 'ReadOnlySpan' is a pass array). return parameter.RefKind is RefKind.None && SymbolEqualityComparer.Default.Equals(parameter.Type.OriginalDefinition, spanType); } + /// + /// Checks whether a given parameter is a write-only parameter (i.e. a parameter + /// that is projected as a fill array in the Windows Runtime ABI). + /// + /// The parameter to check. + /// The well known symbols used by the analyzer. + /// Whether is a write-only fill array parameter. + private static bool IsFillArrayParameter(IParameterSymbol parameter, WellKnownSymbols symbols) + { + // The parameter must belong to a method that is projected to the Windows Runtime. This also filters + // out the parameters of nested lambdas and local functions, which are never projected (and could not + // reference the parameters of the enclosing method anyway, as a span cannot be captured by them). + return + IsSpanParameter(parameter, symbols.SpanType) && + parameter.ContainingSymbol is IMethodSymbol method && + IsWindowsRuntimeMethod(method); + } + /// /// Checks whether a given method is part of the Windows Runtime ABI surface of an authored component. /// @@ -493,7 +502,7 @@ private static bool IsPrecededByCoveringWrite(IOperation read, ReadTarget target private static bool IsGuaranteedCoveringWrite(IOperation operation, ReadTarget target) { // Constructs that might not run at all can never guarantee that a write nested in them will happen - if (IsConditionallyExecuted(operation)) + if (IsConditionallyExecuted(operation, target.Symbols)) { return false; } @@ -525,7 +534,7 @@ private static bool IsCoveringWrite(IOperation operation, ReadTarget target) // 'span.Clear()' and 'span.Fill(value)' initialize every element of the span, so // they cover any subsequent read from it, no matter which elements it looks at. if (operation is IInvocationOperation { TargetMethod: { Name: "Clear", Parameters: [] } or { Name: "Fill", Parameters: [_] } } invocation && - SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType.OriginalDefinition, target.SpanType) && + SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType.OriginalDefinition, target.Symbols.SpanType) && invocation.Instance is { } instance && IsReferenceTo(instance, target.Parameter)) { @@ -555,21 +564,27 @@ target.Alias is { } alias && /// Checks whether a given operation might not be reached during the execution of its parent operation. /// /// The operation to check. + /// The well known symbols used by the analyzer. /// Whether might be skipped, or run at some later point. - private static bool IsConditionallyExecuted(IOperation operation) + private static bool IsConditionallyExecuted(IOperation operation, WellKnownSymbols symbols) { - return operation is - IConditionalOperation or // 'if' statements and '?:' expressions - IConditionalAccessOperation or // '?.' and '?[]' accesses - ICoalesceOperation or // '??' expressions - ICoalesceAssignmentOperation or // '??=' expressions - ISwitchOperation or // 'switch' statements - ISwitchExpressionOperation or // 'switch' expressions - ILoopOperation or // all loops, as the body might never run - ITryOperation or // 'try' blocks, as an exception might skip the rest of the body - IAnonymousFunctionOperation or // lambdas and anonymous methods - ILocalFunctionOperation or // local functions - IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr }; + // A call to a method annotated with '[Conditional]' is removed entirely by the compiler, arguments + // included, when the associated preprocessor symbol is not defined for the current compilation + return (operation is IInvocationOperation { TargetMethod: { } targetMethod } && + symbols.ConditionalAttributeType is { } conditionalAttributeType && + targetMethod.HasAttributeWithType(conditionalAttributeType)) || + operation is + IConditionalOperation or // 'if' statements and '?:' expressions + IConditionalAccessOperation or // '?.' and '?[]' accesses + ICoalesceOperation or // '??' expressions + ICoalesceAssignmentOperation or // '??=' expressions + ISwitchOperation or // 'switch' statements + ISwitchExpressionOperation or // 'switch' expressions + ILoopOperation or // all loops, as the body might never run + ITryOperation or // 'try' blocks, as an exception might skip the rest of the body + IAnonymousFunctionOperation or // lambdas and anonymous methods + ILocalFunctionOperation or // local functions + IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr }; } /// @@ -628,10 +643,31 @@ private static bool IsPotentialWrite(IOperation operation, ISymbol symbol) // 'ref var alias = ref x', which can then be used to write to it IVariableInitializerOperation { Parent: IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref } } => true, + // '(x, y) = value' (a deconstruction assignment), where the reference is nested in the target tuple + ITupleOperation tuple => IsDeconstructionTarget(tuple), + _ => false }; } + /// + /// Checks whether a given tuple is (nested in) the target of a deconstruction assignment. + /// + /// The tuple to check. + /// Whether is being assigned to by a deconstruction. + private static bool IsDeconstructionTarget(ITupleOperation tuple) + { + IOperation current = tuple; + + // Deconstructions can be nested (e.g. '((x, y), z) = value'), so walk up all the enclosing tuples + while (current.Parent is ITupleOperation parent) + { + current = parent; + } + + return current.Parent is IDeconstructionAssignmentOperation deconstruction && ReferenceEquals(deconstruction.Target, current); + } + /// /// Checks whether a given reference to an element of a write-only parameter is /// guaranteed to initialize that element, as opposed to just possibly writing to it. @@ -714,21 +750,74 @@ private static bool AreIndexesEquivalent(IOperation left, IOperation right) } /// - /// Checks whether a given operation block contains a goto branch. + /// Checks whether reads in a given operation block can be skipped when they are preceded by a covering write. /// /// The operation block to inspect. - /// Whether contains a goto branch. - private static bool ContainsGotoBranch(IOperation operationBlock) + /// Whether preceding writes can be relied upon for reads in . + /// + /// Recognizing a preceding write relies on being able to see every write to the span parameter and to the + /// index variables in source order. That is not possible when the body can jump around, or when it creates + /// an indirection that could be used to modify a variable from a place the ordered scan can't account for. + /// + private static bool AllowsSuppression(IOperation operationBlock) { foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { - if (operation is IBranchOperation { BranchKind: BranchKind.GoTo }) + switch (operation) { - return true; + // A 'goto' can jump over a write that would otherwise always run before a given read + case IBranchOperation { BranchKind: BranchKind.GoTo }: + + // A lambda or a local function can capture a variable and modify it when invoked, which + // can happen at any point (a span itself can never be captured, as it is a 'ref struct') + case IAnonymousFunctionOperation or ILocalFunctionOperation: + + // A writable 'ref' alias to a local or parameter (e.g. the span itself, or an index + // variable) can be used to modify it from anywhere the alias is in scope. Note that an + // alias to an element of the span (e.g. 'ref var slot = ref span[i]') is not a concern + // here, as it can only ever be used to write to that element, never to invalidate it. + case IVariableDeclaratorOperation + { + Symbol.RefKind: RefKind.Ref, + Initializer.Value: ILocalReferenceOperation or IParameterReferenceOperation + }: + + // The same applies to a 'ref' reassignment of an existing alias + case ISimpleAssignmentOperation { IsRef: true, Value: ILocalReferenceOperation or IParameterReferenceOperation }: + return false; + default: + break; } } - return false; + return true; + } + + /// + /// The well known symbols used by the analyzer. + /// + /// The for . + /// The for . + /// The for [Conditional], if available. + private readonly struct WellKnownSymbols( + INamedTypeSymbol spanType, + INamedTypeSymbol readOnlySpanType, + INamedTypeSymbol? conditionalAttributeType) + { + /// + /// Gets the for . + /// + public INamedTypeSymbol SpanType => spanType; + + /// + /// Gets the for . + /// + public INamedTypeSymbol ReadOnlySpanType => readOnlySpanType; + + /// + /// Gets the for [Conditional], if available. + /// + public INamedTypeSymbol? ConditionalAttributeType => conditionalAttributeType; } /// @@ -741,13 +830,13 @@ private readonly struct ReadTarget /// Creates a new instance with the specified parameters. /// /// The write-only parameter being read from. - /// The for . + /// The well known symbols used by the analyzer. /// The index of the element being read, if the read goes through the span indexer. /// The writable ref foreach loop variable aliasing the element being read, if any. - private ReadTarget(IParameterSymbol parameter, INamedTypeSymbol spanType, IOperation? index, ILocalSymbol? alias) + private ReadTarget(IParameterSymbol parameter, WellKnownSymbols symbols, IOperation? index, ILocalSymbol? alias) { Parameter = parameter; - SpanType = spanType; + Symbols = symbols; Index = index; Alias = alias; } @@ -758,9 +847,9 @@ private ReadTarget(IParameterSymbol parameter, INamedTypeSymbol spanType, IOpera public IParameterSymbol Parameter { get; } /// - /// Gets the for . + /// Gets the well known symbols used by the analyzer. /// - public INamedTypeSymbol SpanType { get; } + public WellKnownSymbols Symbols { get; } /// /// Gets the index of the element being read, if the read goes through the span indexer. @@ -786,35 +875,35 @@ private ReadTarget(IParameterSymbol parameter, INamedTypeSymbol spanType, IOpera /// Creates a for a read of all the elements of a span (e.g. a foreach loop). /// /// The write-only parameter being read from. - /// The for . + /// The well known symbols used by the analyzer. /// The resulting value. - public static ReadTarget ForSpan(IParameterSymbol parameter, INamedTypeSymbol spanType) + public static ReadTarget ForSpan(IParameterSymbol parameter, WellKnownSymbols symbols) { - return new(parameter, spanType, index: null, alias: null); + return new(parameter, symbols, index: null, alias: null); } /// /// Creates a for a read of a single element through the span indexer. /// /// The write-only parameter being read from. - /// The for . + /// The well known symbols used by the analyzer. /// The index of the element being read. /// The resulting value. - public static ReadTarget ForElement(IParameterSymbol parameter, INamedTypeSymbol spanType, IOperation index) + public static ReadTarget ForElement(IParameterSymbol parameter, WellKnownSymbols symbols, IOperation index) { - return new(parameter, spanType, index, alias: null); + return new(parameter, symbols, index, alias: null); } /// /// Creates a for a read of the element aliased by a writable ref loop variable. /// /// The write-only parameter being read from. - /// The for . + /// The well known symbols used by the analyzer. /// The writable ref foreach loop variable aliasing the element being read. /// The resulting value. - public static ReadTarget ForAlias(IParameterSymbol parameter, INamedTypeSymbol spanType, ILocalSymbol alias) + public static ReadTarget ForAlias(IParameterSymbol parameter, WellKnownSymbols symbols, ILocalSymbol alias) { - return new(parameter, spanType, index: null, alias); + return new(parameter, symbols, index: null, alias); } } } diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs index c1d01a059..cb990f3ab 100644 --- a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -927,6 +927,136 @@ public void Fill(Span span) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } + [TestMethod] + public async Task PrimaryConstructorInitializers_Warn() + { + const string source = """ + using System; + + public class Sample(Span span) + { + private int _field = {|CSWINRT2021:span[0]|}; + + private int Property { get; } = {|CSWINRT2021:span[1]|}; + + private int _converted = Read({|CSWINRT2021:span|}); + + private static int Read(ReadOnlySpan span) => span.Length; + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterDeconstructedIndex_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + int i = 0; + + span[i] = 1; + (i, _) = (5, 0); // A deconstruction also modifies the index + + int value = {|CSWINRT2021:span[i]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsWithAliasedIndexOrSpan_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void FillWithAliasedIndex(Span span) + { + int i = 0; + ref int alias = ref i; // The alias can modify the index from anywhere + + span[i] = 1; + alias = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + + public void FillWithAliasedSpan(Span span, Span other) + { + ref Span alias = ref span; // The alias can redirect the span from anywhere + + span.Clear(); + alias = other; + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsWithCapturedIndex_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span span) + { + int i = 0; + + void Advance() => i++; // The capture can modify the index when invoked + + span[i] = 1; + Advance(); + + int value = {|CSWINRT2021:span[i]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterConditionalMethodWrite_Warn() + { + const string source = """ + using System; + using System.Diagnostics; + + public class Sample + { + public void Fill(Span span) + { + // The whole call, arguments included, is removed when 'DEBUG' is not defined + Log(span[0] = 1); + + int value = {|CSWINRT2021:span[0]|}; + } + + [Conditional("DEBUG")] + private static void Log(int x) + { + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + [TestMethod] public async Task ForEachWithWriteInBody_Warns() { From 0fb4aa8db846ed9232c31410e788035292eaa170 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 14:14:14 -0700 Subject: [PATCH 09/11] Harden the covering write analysis against indirect writes A second pass over the analysis found three more ways a write could be trusted when it might not have happened, or when it applied to a different element: - An override cannot carry '[Conditional]' itself, but it does inherit the conditional symbols of the method it overrides, so a call bound to it is erased just the same. The whole 'OverriddenMethod' chain is now inspected. - A writable 'ref' alias was only recognized when its referent was spelled out as a local or a parameter, which let a conditional 'ref' expression, a 'ref' returning call or a pointer sneak an indirection past the check. Any writable 'ref' alias now disables suppression, except for an alias to an element of a span (which can only ever write to that element, never redirect the span or change an index), and so does any use of a pointer. - An index that is itself a 'ref' local or parameter aliases some other storage, such as a field or an array element, so its value can change with no write to the index in sight. Such an index is no longer considered stable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../WriteOnlySpanParameterAnalyzer.cs | 70 ++++++++-- .../Test_WriteOnlySpanParameterAnalyzer.cs | 131 ++++++++++++++++++ 2 files changed, 188 insertions(+), 13 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index 9f786978a..bba112429 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -83,7 +83,7 @@ public override void Initialize(AnalysisContext context) foreach (IOperation operationBlock in context.OperationBlocks) { - bool allowsSuppression = AllowsSuppression(operationBlock); + bool allowsSuppression = AllowsSuppression(operationBlock, symbols); foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { @@ -572,7 +572,7 @@ private static bool IsConditionallyExecuted(IOperation operation, WellKnownSymbo // included, when the associated preprocessor symbol is not defined for the current compilation return (operation is IInvocationOperation { TargetMethod: { } targetMethod } && symbols.ConditionalAttributeType is { } conditionalAttributeType && - targetMethod.HasAttributeWithType(conditionalAttributeType)) || + IsConditionalMethod(targetMethod, conditionalAttributeType)) || operation is IConditionalOperation or // 'if' statements and '?:' expressions IConditionalAccessOperation or // '?.' and '?[]' accesses @@ -587,6 +587,27 @@ ILocalFunctionOperation or // local functions IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr }; } + /// + /// Checks whether calls to a given method are conditionally compiled. + /// + /// The method to check. + /// The for [Conditional]. + /// Whether calls to might be removed by the compiler. + private static bool IsConditionalMethod(IMethodSymbol method, INamedTypeSymbol conditionalAttributeType) + { + // An override cannot carry '[Conditional]' itself, but it does inherit the conditional + // symbols of the method it overrides, so the whole chain has to be inspected here + for (IMethodSymbol? currentMethod = method; currentMethod is not null; currentMethod = currentMethod.OverriddenMethod) + { + if (currentMethod.HasAttributeWithType(conditionalAttributeType)) + { + return true; + } + } + + return false; + } + /// /// Checks whether the operands a covering write depends on (i.e. the span parameter itself, and the index /// variable, if the target is a single element) can't have been modified between that write and a later read. @@ -600,6 +621,13 @@ private static bool AreOperandsStable(ImmutableArray statements, int { ISymbol? indexSymbol = target.IndexSymbol; + // An index that is itself a 'ref' local or parameter aliases some other storage (e.g. a field, or an + // array element), so its value can change without any write to the symbol itself being visible here + if (indexSymbol is ILocalSymbol { RefKind: not RefKind.None } or IParameterSymbol { RefKind: not RefKind.None }) + { + return false; + } + for (int i = writeIndex; i <= readIndex; i++) { foreach (IOperation operation in statements[i].DescendantsAndSelf()) @@ -753,13 +781,14 @@ private static bool AreIndexesEquivalent(IOperation left, IOperation right) /// Checks whether reads in a given operation block can be skipped when they are preceded by a covering write. /// /// The operation block to inspect. + /// The well known symbols used by the analyzer. /// Whether preceding writes can be relied upon for reads in . /// /// Recognizing a preceding write relies on being able to see every write to the span parameter and to the /// index variables in source order. That is not possible when the body can jump around, or when it creates /// an indirection that could be used to modify a variable from a place the ordered scan can't account for. /// - private static bool AllowsSuppression(IOperation operationBlock) + private static bool AllowsSuppression(IOperation operationBlock, WellKnownSymbols symbols) { foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { @@ -772,18 +801,20 @@ private static bool AllowsSuppression(IOperation operationBlock) // can happen at any point (a span itself can never be captured, as it is a 'ref struct') case IAnonymousFunctionOperation or ILocalFunctionOperation: - // A writable 'ref' alias to a local or parameter (e.g. the span itself, or an index - // variable) can be used to modify it from anywhere the alias is in scope. Note that an - // alias to an element of the span (e.g. 'ref var slot = ref span[i]') is not a concern - // here, as it can only ever be used to write to that element, never to invalidate it. - case IVariableDeclaratorOperation - { - Symbol.RefKind: RefKind.Ref, - Initializer.Value: ILocalReferenceOperation or IParameterReferenceOperation - }: + // A pointer can be used to modify a variable without any visible write to it + case { Type: IPointerTypeSymbol }: + return false; + + // A writable 'ref' alias can be used to modify whatever it points at from anywhere it is in + // scope. An alias to an element of a span is the one exception: it can only ever be used to + // write to that element, never to redirect the span itself or to change an index variable. + case IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref, Initializer.Value: { } aliasedValue } + when !IsSpanElementReference(aliasedValue, symbols): + return false; // The same applies to a 'ref' reassignment of an existing alias - case ISimpleAssignmentOperation { IsRef: true, Value: ILocalReferenceOperation or IParameterReferenceOperation }: + case ISimpleAssignmentOperation { IsRef: true, Value: { } reassignedValue } + when !IsSpanElementReference(reassignedValue, symbols): return false; default: break; @@ -793,6 +824,19 @@ private static bool AllowsSuppression(IOperation operationBlock) return true; } + /// + /// Checks whether a given operation references an element of a value. + /// + /// The operation to check. + /// The well known symbols used by the analyzer. + /// Whether is an access to an element of a . + private static bool IsSpanElementReference(IOperation operation, WellKnownSymbols symbols) + { + return + operation is IPropertyReferenceOperation { Property.IsIndexer: true, Instance.Type: { } instanceType } && + SymbolEqualityComparer.Default.Equals(instanceType.OriginalDefinition, symbols.SpanType); + } + /// /// The well known symbols used by the analyzer. /// diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs index cb990f3ab..e193326d8 100644 --- a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -1057,6 +1057,137 @@ private static void Log(int x) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } + [TestMethod] + public async Task ReadsAfterConditionalOverrideWrite_Warn() + { + const string source = """ + using System; + using System.Diagnostics; + + public class Logger + { + [Conditional("DEBUG")] + public virtual void Log(int x) + { + } + } + + public class Sample : Logger + { + // An override inherits the conditional symbols of the method it overrides + public override void Log(int x) + { + } + + public void Fill(Span span) + { + Log(span[0] = 1); + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsWithIndirectlyAliasedIndex_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void FillWithConditionalAlias(Span span, bool condition) + { + int i = 0; + int j = 0; + ref int alias = ref (condition ? ref i : ref j); + + span[i] = 1; + alias = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + + public void FillWithReturnedAlias(Span span) + { + int i = 0; + ref int alias = ref Identity(ref i); + + span[i] = 1; + alias = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + + public unsafe void FillWithPointer(Span span) + { + int i = 0; + int* pointer = &i; + + span[i] = 1; + *pointer = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + + private static ref int Identity(ref int x) => ref x; + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsWithRefLocalIndex_Warn() + { + const string source = """ + using System; + + public class Sample + { + private int _index; + + public void FillWithFieldIndex(Span span) + { + // The index aliases the field, so it changes with no write to 'i' in sight + ref int i = ref _index; + + span[i] = 1; + _index = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + + public void FillWithReadOnlyFieldIndex(Span span) + { + ref readonly int i = ref _index; + + span[i] = 1; + Reset(); + + int value = {|CSWINRT2021:span[i]|}; + } + + public void FillWithArrayIndex(Span span, int[] indices) + { + ref int i = ref indices[0]; + + span[i] = 1; + indices[0] = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + + private void Reset() => _index = 5; + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + [TestMethod] public async Task ForEachWithWriteInBody_Warns() { From 84955deed6f03dc56ce00f0d92c4a32aefb1f282 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 14:34:52 -0700 Subject: [PATCH 10/11] Also account for escaping references and erased partial calls Two more ways a write could be trusted when it might not apply, or might not have happened at all: - A reference to an operand can escape the call that receives it, either by being stored in a 'ref' field or by being wrapped in a span, and can then be used to modify that operand at any point. Scanning the statements between the write and the read is not enough for those, as a reference handed out before the write is still live afterwards, so the whole body is now checked for a reference to the span parameter or the index variable being passed by reference. - A call to a 'partial void' method that has no implementing declaration is removed by the compiler, the evaluation of its arguments included, exactly like a call to a '[Conditional]' method whose symbol is not defined. The carve-out that kept suppression enabled for an alias to a span element is dropped while at it. It was not sound (a span can be created over an index variable, e.g. with 'MemoryMarshal.CreateSpan'), and it also had no effect in practice: a write through such an alias is not recognized as covering anyway, and the loop variable of a 'foreach' loop is unaffected either way, as its declarator has no initializer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../WriteOnlySpanParameterAnalyzer.cs | 116 ++++++++++++------ .../Test_WriteOnlySpanParameterAnalyzer.cs | 72 +++++++++++ 2 files changed, 150 insertions(+), 38 deletions(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index bba112429..c180699a5 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -83,7 +83,7 @@ public override void Initialize(AnalysisContext context) foreach (IOperation operationBlock in context.OperationBlocks) { - bool allowsSuppression = AllowsSuppression(operationBlock, symbols); + bool allowsSuppression = AllowsSuppression(operationBlock); foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { @@ -568,11 +568,7 @@ target.Alias is { } alias && /// Whether might be skipped, or run at some later point. private static bool IsConditionallyExecuted(IOperation operation, WellKnownSymbols symbols) { - // A call to a method annotated with '[Conditional]' is removed entirely by the compiler, arguments - // included, when the associated preprocessor symbol is not defined for the current compilation - return (operation is IInvocationOperation { TargetMethod: { } targetMethod } && - symbols.ConditionalAttributeType is { } conditionalAttributeType && - IsConditionalMethod(targetMethod, conditionalAttributeType)) || + return (operation is IInvocationOperation { TargetMethod: { } targetMethod } && IsErasableCall(targetMethod, symbols)) || operation is IConditionalOperation or // 'if' statements and '?:' expressions IConditionalAccessOperation or // '?.' and '?[]' accesses @@ -588,15 +584,27 @@ ILocalFunctionOperation or // local functions } /// - /// Checks whether calls to a given method are conditionally compiled. + /// Checks whether calls to a given method might be removed entirely by the compiler, arguments included. /// /// The method to check. - /// The for [Conditional]. - /// Whether calls to might be removed by the compiler. - private static bool IsConditionalMethod(IMethodSymbol method, INamedTypeSymbol conditionalAttributeType) + /// The well known symbols used by the analyzer. + /// Whether calls to might not survive compilation. + private static bool IsErasableCall(IMethodSymbol method, WellKnownSymbols symbols) { - // An override cannot carry '[Conditional]' itself, but it does inherit the conditional - // symbols of the method it overrides, so the whole chain has to be inspected here + // A 'partial void' method that has no implementing declaration has all of its call sites removed + if (method is { IsPartialDefinition: true, PartialImplementationPart: null }) + { + return true; + } + + if (symbols.ConditionalAttributeType is not { } conditionalAttributeType) + { + return false; + } + + // A call to a method annotated with '[Conditional]' is removed when the associated preprocessor symbol + // is not defined. An override cannot carry the attribute itself, but it does inherit the conditional + // symbols of the method it overrides, so the whole chain of overridden methods is inspected here. for (IMethodSymbol? currentMethod = method; currentMethod is not null; currentMethod = currentMethod.OverriddenMethod) { if (currentMethod.HasAttributeWithType(conditionalAttributeType)) @@ -628,6 +636,13 @@ private static bool AreOperandsStable(ImmutableArray statements, int return false; } + // A reference to an operand handed out anywhere in the method (e.g. 'Foo(ref i)') can outlive the call + // that receives it, so it might be used to modify that operand from a place the scan below can't see + if (AreOperandsEscaping(GetOperationBlock(statements[readIndex]), target, indexSymbol)) + { + return false; + } + for (int i = writeIndex; i <= readIndex; i++) { foreach (IOperation operation in statements[i].DescendantsAndSelf()) @@ -649,6 +664,51 @@ private static bool AreOperandsStable(ImmutableArray statements, int return true; } + /// + /// Checks whether a reference to the operands a covering write depends on is handed out anywhere in an + /// operation block (e.g. as a ref argument), which could be used to modify them at any point. + /// + /// The operation block to inspect. + /// The location the write covers. + /// The local or parameter used as the index, if there is one. + /// Whether a reference to one of the operands might have escaped. + private static bool AreOperandsEscaping(IOperation operationBlock, ReadTarget target, ISymbol? indexSymbol) + { + foreach (IOperation operation in operationBlock.DescendantsAndSelf()) + { + // A reference passed to another method can outlive the call that receives it (e.g. by being stored + // in a 'ref' field, or by being wrapped in a span), so it is not enough to only look at the calls + // between the write and the read: one made before the write can still be used to modify an operand. + if (operation is not IArgumentOperation { Parameter.RefKind: not RefKind.None, Value: { } value }) + { + continue; + } + + if (IsReferenceTo(value, target.Parameter) || + (indexSymbol is not null && IsReferenceTo(value, indexSymbol))) + { + return true; + } + } + + return false; + } + + /// + /// Gets the operation block containing a given operation. + /// + /// The operation to get the operation block for. + /// The root operation containing . + private static IOperation GetOperationBlock(IOperation operation) + { + while (operation.Parent is { } parent) + { + operation = parent; + } + + return operation; + } + /// /// Checks whether a given operation is a reference to a local or parameter that might modify its value. /// @@ -781,14 +841,13 @@ private static bool AreIndexesEquivalent(IOperation left, IOperation right) /// Checks whether reads in a given operation block can be skipped when they are preceded by a covering write. /// /// The operation block to inspect. - /// The well known symbols used by the analyzer. /// Whether preceding writes can be relied upon for reads in . /// /// Recognizing a preceding write relies on being able to see every write to the span parameter and to the /// index variables in source order. That is not possible when the body can jump around, or when it creates /// an indirection that could be used to modify a variable from a place the ordered scan can't account for. /// - private static bool AllowsSuppression(IOperation operationBlock, WellKnownSymbols symbols) + private static bool AllowsSuppression(IOperation operationBlock) { foreach (IOperation operation in operationBlock.DescendantsAndSelf()) { @@ -803,18 +862,12 @@ private static bool AllowsSuppression(IOperation operationBlock, WellKnownSymbol // A pointer can be used to modify a variable without any visible write to it case { Type: IPointerTypeSymbol }: - return false; - // A writable 'ref' alias can be used to modify whatever it points at from anywhere it is in - // scope. An alias to an element of a span is the one exception: it can only ever be used to - // write to that element, never to redirect the span itself or to change an index variable. - case IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref, Initializer.Value: { } aliasedValue } - when !IsSpanElementReference(aliasedValue, symbols): - return false; - - // The same applies to a 'ref' reassignment of an existing alias - case ISimpleAssignmentOperation { IsRef: true, Value: { } reassignedValue } - when !IsSpanElementReference(reassignedValue, symbols): + // A writable 'ref' alias, whether declared or reassigned, can be used to modify whatever + // it points at from anywhere it is in scope. Note that the loop variable of a 'foreach' + // loop is not a concern here, as its declarator does not have an initializer. + case IVariableDeclaratorOperation { Symbol.RefKind: RefKind.Ref, Initializer: not null }: + case ISimpleAssignmentOperation { IsRef: true }: return false; default: break; @@ -824,19 +877,6 @@ private static bool AllowsSuppression(IOperation operationBlock, WellKnownSymbol return true; } - /// - /// Checks whether a given operation references an element of a value. - /// - /// The operation to check. - /// The well known symbols used by the analyzer. - /// Whether is an access to an element of a . - private static bool IsSpanElementReference(IOperation operation, WellKnownSymbols symbols) - { - return - operation is IPropertyReferenceOperation { Property.IsIndexer: true, Instance.Type: { } instanceType } && - SymbolEqualityComparer.Default.Equals(instanceType.OriginalDefinition, symbols.SpanType); - } - /// /// The well known symbols used by the analyzer. /// diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs index e193326d8..3da70a6e0 100644 --- a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -1188,6 +1188,78 @@ public void FillWithArrayIndex(Span span, int[] indices) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } + [TestMethod] + public async Task ReadsWithEscapingIndex_Warn() + { + const string source = """ + using System; + using System.Runtime.InteropServices; + + public ref struct Cursor + { + private ref int _index; + + public Cursor(ref int index) + { + _index = ref index; + } + + public void Advance() => _index++; + } + + public class Sample + { + public void FillWithStoredIndexReference(Span span) + { + int i = 0; + Cursor cursor = new(ref i); // The reference outlives the call + + span[i] = 1; + cursor.Advance(); + + int value = {|CSWINRT2021:span[i]|}; + } + + public void FillWithSpanOverIndex(Span span) + { + int i = 0; + Span window = MemoryMarshal.CreateSpan(ref i, 1); + + span[i] = 1; + window[0] = 5; + + int value = {|CSWINRT2021:span[i]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + + [TestMethod] + public async Task ReadsAfterUnimplementedPartialMethodWrite_Warn() + { + const string source = """ + using System; + + public partial class Sample + { + // Calls to a 'partial void' method with no implementing declaration are + // removed entirely by the compiler, the evaluation of arguments included + partial void Log(int x); + + public void Fill(Span span) + { + Log(span[0] = 1); + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + [TestMethod] public async Task ForEachWithWriteInBody_Warns() { From c2fe5d947b762ddf8d7850e58fc5b5f936624032 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 1 Aug 2026 14:46:33 -0700 Subject: [PATCH 11/11] Treat failing interpolated string handlers as conditional An interpolated string handler whose constructor has an 'out bool' success parameter, or whose append calls return 'bool', can report failure, in which case the compiler skips the evaluation of the remaining holes. A write inside one of them is therefore not guaranteed to run, in the same way as a write nested in any other construct that might be skipped. This is reachable with BCL types alone, e.g. through 'MemoryExtensions.TryWrite', whose handler reports failure when the destination is too small. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c86c0e5-5a20-48b7-b8c6-b04c963edef0 --- .../WriteOnlySpanParameterAnalyzer.cs | 6 +++++- .../Test_WriteOnlySpanParameterAnalyzer.cs | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs index c180699a5..8d1dd9961 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/Analyzers/WriteOnlySpanParameterAnalyzer.cs @@ -580,7 +580,11 @@ ISwitchExpressionOperation or // 'switch' expressions ITryOperation or // 'try' blocks, as an exception might skip the rest of the body IAnonymousFunctionOperation or // lambdas and anonymous methods ILocalFunctionOperation or // local functions - IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr }; + IBinaryOperation { OperatorKind: BinaryOperatorKind.ConditionalAnd or BinaryOperatorKind.ConditionalOr } or + + // Interpolated strings using a handler that can report failure, as the holes are not evaluated then + IInterpolatedStringHandlerCreationOperation { HandlerCreationHasSuccessParameter: true } or + IInterpolatedStringHandlerCreationOperation { HandlerAppendCallsReturnBool: true }; } /// diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs index 3da70a6e0..94407f511 100644 --- a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs +++ b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs @@ -1260,6 +1260,27 @@ public void Fill(Span span) await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); } + [TestMethod] + public async Task ReadsAfterInterpolatedStringHandlerWrite_Warn() + { + const string source = """ + using System; + + public class Sample + { + public void Fill(Span destination, Span span) + { + // The handler can report failure, in which case the holes are never evaluated + destination.TryWrite($"{span[0] = 1}", out int written); + + int value = {|CSWINRT2021:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } + [TestMethod] public async Task ForEachWithWriteInBody_Warns() {