diff --git a/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md b/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md index 8acb41578..96901163e 100644 --- a/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md +++ b/src/Authoring/WinRT.SourceGenerator2/AnalyzerReleases.Shipped.md @@ -23,4 +23,5 @@ CSWINRT2013 | WindowsRuntime.SourceGenerator | Warning | Invalid 'ContractVersio CSWINRT2014 | WindowsRuntime.SourceGenerator | Warning | API contract type missing 'ContractVersionAttribute' CSWINRT2015 | WindowsRuntime.SourceGenerator | Info | Public authored type missing version metadata CSWINRT2016 | WindowsRuntime.SourceGenerator | Warning | Public authored type missing 'ContractVersionAttribute' -CSWINRT2017 | WindowsRuntime.SourceGenerator | Warning | Public authored type mixing '[ContractVersion]' and '[Version]' \ No newline at end of file +CSWINRT2017 | WindowsRuntime.SourceGenerator | Warning | Public authored type mixing '[ContractVersion]' and '[Version]' +CSWINRT2018 | WindowsRuntime.SourceGenerator | Warning | Reading from a write-only 'Span' parameter \ No newline at end of file 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 + }; + } +} diff --git a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs index 65b19e356..488c5668f 100644 --- a/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs +++ b/src/Authoring/WinRT.SourceGenerator2/Diagnostics/DiagnosticDescriptors.cs @@ -244,4 +244,17 @@ internal static partial class DiagnosticDescriptors isEnabledByDefault: true, description: "Public types in a Windows Runtime component should not mix '[ContractVersion]' and '[Version]', as these represent two different versioning schemes. Pick one and apply it consistently across the public API surface of the component.", helpLinkUri: "https://github.com/microsoft/CsWinRT"); + + /// + /// Gets a for reading from a write-only parameter on a Windows Runtime method. + /// + public static readonly DiagnosticDescriptor WriteOnlySpanParameterRead = new( + id: "CSWINRT2018", + 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 diff --git a/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs b/src/Tests/SourceGenerator2Test/Test_WriteOnlySpanParameterAnalyzer.cs new file mode 100644 index 000000000..8267241dc --- /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 = {|CSWINRT2018:span[0]|}; + Value({|CSWINRT2018:span[1]|}); + ref readonly int readOnlyReference = ref {|CSWINRT2018:span[2]|}; + In(in {|CSWINRT2018:span[3]|}); + {|CSWINRT2018:span[4]|}++; + {|CSWINRT2018: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 = {|CSWINRT2018:span|}; + ReadOnlySpan explicitConversion = {|CSWINRT2018:(ReadOnlySpan)span|}; + + Read({|CSWINRT2018: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 {|CSWINRT2018:span|}) + { + } + + foreach (ref readonly int y in {|CSWINRT2018: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 = {|CSWINRT2018: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 = {|CSWINRT2018: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 = {|CSWINRT2018:span[0]|}; + } + } + """; + + await VerifyCS.VerifyAnalyzerAsync(source, isCsWinRTComponent: true); + } +}