diff --git a/src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Shipped.md b/src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Shipped.md new file mode 100644 index 00000000000000..f50bb1fe2107bb --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Shipped.md @@ -0,0 +1,2 @@ +; Shipped analyzer releases +; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md diff --git a/src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Unshipped.md b/src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Unshipped.md new file mode 100644 index 00000000000000..f6ab3b228f6205 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Unshipped.md @@ -0,0 +1,9 @@ +; Unshipped analyzer release +; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +IL5005 | MemorySafety | Info | UnsafeEvolutionAnalyzer, [Documentation](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md) +IL5006 | MemorySafety | Info | UnsafeEvolutionAnalyzer, [Documentation](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md) diff --git a/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj b/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj index 5dd13cb7772158..c22fbe68f53024 100644 --- a/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj +++ b/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj @@ -21,4 +21,26 @@ + + + + + + + + + + + + diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/IntroduceUnsafeBlockCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/IntroduceUnsafeBlockCodeFixProvider.cs new file mode 100644 index 00000000000000..6366bbd5db5253 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/IntroduceUnsafeBlockCodeFixProvider.cs @@ -0,0 +1,684 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Formatting; + +namespace ILLink.CodeFix.UnsafeEvolution +{ + /// + /// Introduces unsafe { ... } blocks around code that the compiler now considers + /// to require an unsafe context under the updated memory-safety rules. + /// + /// + /// Handles: + /// + /// CS0214 - legacy pointer-in-non-unsafe-context error. + /// CS9360 - unsafe operation (pointer dereference, function-pointer invocation, etc.). + /// CS9361 - uninitialized stackalloc to Span<T> under SkipLocalsInit. + /// CS9362 - call of a requires-unsafe member outside an unsafe context. + /// CS9363 - compat-mode call of a member with pointers in its signature. + /// + /// Behavior: + /// + /// + /// If many unsafe diagnostics cluster inside a single member body + /// (see ), the whole body is wrapped once. + /// + /// + /// Otherwise the smallest containing statement is wrapped. Local declarations whose + /// initializer is unsafe but whose declared local is referenced later are split into + /// a forward declaration plus assignment so the wrap stays minimal (with scoped + /// added for ref-struct types). + /// + /// The first line of every generated block is // SAFETY-TODO: Audit. + /// + /// Statements that have preprocessor directives strictly BETWEEN their tokens are left + /// alone (handled manually). Enclosing #if/#endif blocks are fine - the wrap + /// simply lands inside them. + /// + /// + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(IntroduceUnsafeBlockCodeFixProvider)), Shared] + public sealed class IntroduceUnsafeBlockCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider + { + private const string WrapStatementTitle = "Wrap in 'unsafe' block (SAFETY-TODO)"; + private const string WrapBodyTitle = "Wrap entire member body in 'unsafe' block (SAFETY-TODO)"; + + /// + /// Compiler diagnostic IDs that this fixer can introduce an unsafe { } block to resolve. + /// + private static readonly ImmutableArray UnsafeDiagnosticIds = + [ + UnsafeEvolutionDescriptors.PointersAndFixedBuffersUnsafe, + UnsafeEvolutionDescriptors.UnsafeOperation, + UnsafeEvolutionDescriptors.UnsafeUninitializedStackAlloc, + UnsafeEvolutionDescriptors.UnsafeMemberOperation, + UnsafeEvolutionDescriptors.UnsafeMemberOperationCompat, + ]; + + public override ImmutableArray FixableDiagnosticIds => UnsafeDiagnosticIds; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var document = context.Document; + var diagnostic = context.Diagnostics.First(); + + if (await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false) is not { } root) + return; + + var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + + // Lambdas / anonymous functions do not inherit unsafe context from an enclosing + // member, so we cannot fix a diagnostic that lives in one by wrapping an outer + // statement. + var containingStatement = FindContainingStatement(node); + if (containingStatement is null) + { + // Expression-bodied members (e.g. 'int M() => Helper();') have no enclosing + // statement. If we can rewrite the arrow body into a block body, offer that fix. + var arrow = FindContainingArrowBody(node); + if (arrow is null) + return; + + var semanticModelForArrow = await document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + context.RegisterCodeFix( + CodeAction.Create( + title: WrapStatementTitle, + createChangedDocument: ct => WrapArrowBodyAsync(document, arrow, semanticModelForArrow, ct), + equivalenceKey: WrapStatementTitle), + diagnostic); + return; + } + + // Skip statements whose tokens enclose a preprocessor directive (e.g. an argument + // list with #if/#else/#endif between commas). Wrapping such a statement would + // corrupt the directive region. + if (UnsafeBlockHelpers.ContainsInternalDirectiveTrivia(containingStatement)) + return; + + var semanticModel = await document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + + // Decide the wrap strategy: split a local declaration into forward-decl + unsafe block + // when its declared locals (or pattern variables in its initializer) are used after the + // wrap, otherwise wrap the statement as-is. + var strategy = ChooseFixStrategy(containingStatement, semanticModel); + if (strategy is FixStrategy.Skip) + return; + + // If the diagnostic clusters densely inside a member body, prefer one big wrap. + var containingMember = FindContainingMember(node); + BlockSyntax? memberBody = GetMemberBody(containingMember); + if (memberBody is not null && ShouldWrapEntireBody(memberBody, semanticModel, context.CancellationToken)) + { + context.RegisterCodeFix( + CodeAction.Create( + title: WrapBodyTitle, + createChangedDocument: ct => WrapMemberBodyAsync(document, memberBody, ct), + equivalenceKey: WrapBodyTitle), + diagnostic); + return; + } + + context.RegisterCodeFix( + CodeAction.Create( + title: WrapStatementTitle, + createChangedDocument: ct => WrapSingleStatementAsync(document, containingStatement, strategy, ct), + equivalenceKey: WrapStatementTitle), + diagnostic); + } + + // ---- Diagnostic-ID classification ---- + + internal static bool IsUnsafeDiagnosticId(string id) => UnsafeDiagnosticIds.Contains(id); + + // ---- Boundary-aware ancestor walks ---- + + /// + /// Finds the smallest containing , but stops at lambda / + /// anonymous-function boundaries: diagnostics inside an expression-bodied lambda + /// produce no enclosing statement we are allowed to wrap. + /// + private static StatementSyntax? FindContainingStatement(SyntaxNode node) + { + for (var n = node; n is not null; n = n.Parent) + { + if (n is StatementSyntax statement) + return statement; + if (n is AnonymousFunctionExpressionSyntax) + return null; + } + return null; + } + + /// + /// Finds the smallest enclosing (the body + /// of an expression-bodied member or accessor), stopping at lambda / anonymous + /// function boundaries: an expression-bodied lambda is not a member body we can + /// rewrite. + /// + private static ArrowExpressionClauseSyntax? FindContainingArrowBody(SyntaxNode node) + { + for (var n = node; n is not null; n = n.Parent) + { + if (n is ArrowExpressionClauseSyntax arrow) + return arrow; + if (n is AnonymousFunctionExpressionSyntax) + return null; + } + return null; + } + + /// + /// Finds the smallest method-like declaration that contains the diagnostic, stopping + /// at lambda boundaries (unsafe context does not flow into lambdas, so wrapping the + /// outer member's body would not help a diagnostic that lives inside one). + /// + private static SyntaxNode? FindContainingMember(SyntaxNode node) + { + for (var n = node; n is not null; n = n.Parent) + { + if (n is LocalFunctionStatementSyntax lf) + return lf; + if (n is AnonymousFunctionExpressionSyntax) + return null; + if (IsMemberWithBlockBody(n)) + return n; + } + return null; + } + + private static bool IsMemberWithBlockBody(SyntaxNode n) => n is + MethodDeclarationSyntax + or ConstructorDeclarationSyntax + or DestructorDeclarationSyntax + or OperatorDeclarationSyntax + or ConversionOperatorDeclarationSyntax + or LocalFunctionStatementSyntax + or AccessorDeclarationSyntax; + + private static BlockSyntax? GetMemberBody(SyntaxNode? member) => member switch + { + MethodDeclarationSyntax m => m.Body, + ConstructorDeclarationSyntax c => c.Body, + DestructorDeclarationSyntax d => d.Body, + OperatorDeclarationSyntax op => op.Body, + ConversionOperatorDeclarationSyntax co => co.Body, + LocalFunctionStatementSyntax lf => lf.Body, + AccessorDeclarationSyntax a => a.Body, + _ => null, + }; + + // ---- Whole-body wrap decision ---- + + private static bool ShouldWrapEntireBody(BlockSyntax body, SemanticModel? semanticModel, CancellationToken ct) + { + if (semanticModel is null) + return false; + int unsafeCount = CountUnsafeDiagnosticsInBody(body, semanticModel, ct); + int statementCount = CountStatementsOutsideLambdas(body); + return UnsafeBlockHelpers.ShouldWrapEntireBody(unsafeCount, statementCount); + } + + private static int CountStatementsOutsideLambdas(BlockSyntax body) + { + int count = 0; + foreach (var s in body.DescendantNodes(descendIntoChildren: static n => !IsLambdaOrLocalFunction(n)) + .OfType()) + { + if (s is not BlockSyntax) + count++; + } + return count; + } + + private static int CountUnsafeDiagnosticsInBody(BlockSyntax body, SemanticModel semanticModel, CancellationToken ct) + { + int count = 0; + foreach (var d in semanticModel.GetDiagnostics(body.Span, ct)) + { + if (!IsUnsafeDiagnosticId(d.Id)) + continue; + // Wrapping the outer body doesn't establish unsafe context inside nested lambdas. + var diagNode = body.FindNode(d.Location.SourceSpan, getInnermostNodeForTie: true); + if (IsInsideLambdaWithinBody(diagNode, body)) + continue; + count++; + } + return count; + } + + private static bool IsLambdaOrLocalFunction(SyntaxNode n) => + n is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax; + + private static bool IsInsideLambdaWithinBody(SyntaxNode node, BlockSyntax body) + { + for (var n = node; n is not null && n != body; n = n.Parent) + { + if (IsLambdaOrLocalFunction(n)) + return true; + } + return false; + } + + // ---- Fix strategy: wrap as-is vs split into forward-decl + assignment ---- + + private enum FixStrategy + { + Skip, + WrapAsIs, + ForwardDeclare, + } + + private static FixStrategy ChooseFixStrategy(StatementSyntax statement, SemanticModel? semanticModel) + { + if (statement is not LocalDeclarationStatementSyntax local) + return FixStrategy.WrapAsIs; + + // Init-declared variables (out var, pattern variables) cannot follow the local out + // of the unsafe block; if any are referenced after, neither strategy is safe. + if (HasInitializerDeclaredVariablesUsedAfter(local, semanticModel)) + return FixStrategy.Skip; + + // If the declared locals themselves don't escape past this statement, we can safely + // wrap the whole declaration in unsafe. + if (!DeclaredLocalsUsedAfter(local, semanticModel)) + return FixStrategy.WrapAsIs; + + // Locals escape - we must keep them visible after the unsafe block. Try to split + // the declaration into a forward-decl + assignment; if we cannot, give up. + return TryRewriteAsForwardDeclaration(local, semanticModel) is not null + ? FixStrategy.ForwardDeclare + : FixStrategy.Skip; + } + + private static bool DeclaredLocalsUsedAfter(LocalDeclarationStatementSyntax local, SemanticModel? semanticModel) + { + if (semanticModel is null) + return true; // conservative: assume escape + + var declared = new HashSet(SymbolEqualityComparer.Default); + foreach (var v in local.Declaration.Variables) + { + if (semanticModel.GetDeclaredSymbol(v) is { } s) + declared.Add(s); + } + return declared.Count > 0 && AnyReferenceAfter(local, declared, semanticModel); + } + + private static bool HasInitializerDeclaredVariablesUsedAfter(LocalDeclarationStatementSyntax local, SemanticModel? semanticModel) + { + if (semanticModel is null || local.Declaration.Variables.Count != 1) + return false; + + var initializer = local.Declaration.Variables[0].Initializer; + if (initializer is null) + return false; + + var initVars = new HashSet(SymbolEqualityComparer.Default); + // Both 'M(out var x)' and 'switch (e) { _ when e is T t => ... }' surface as + // SingleVariableDesignationSyntax; one walk covers both. + foreach (var designation in initializer.DescendantNodes().OfType()) + { + if (semanticModel.GetDeclaredSymbol(designation) is { } s) + initVars.Add(s); + } + return initVars.Count > 0 && AnyReferenceAfter(local, initVars, semanticModel); + } + + private static bool AnyReferenceAfter(StatementSyntax statement, HashSet symbols, SemanticModel semanticModel) + { + // Look at the containing statement list. Both BlockSyntax and SwitchSectionSyntax + // hold sibling statements in which the declared local is in scope; embedded + // statements (if/while/etc.) have neither parent kind and locals don't escape. + SyntaxList siblings; + switch (statement.Parent) + { + case BlockSyntax block: + siblings = block.Statements; + break; + case SwitchSectionSyntax section: + siblings = section.Statements; + break; + default: + return false; + } + + int idx = siblings.IndexOf(statement); + for (int i = idx + 1; i < siblings.Count; i++) + { + foreach (var id in siblings[i].DescendantNodes().OfType()) + { + if (semanticModel.GetSymbolInfo(id).Symbol is { } sym && symbols.Contains(sym)) + return true; + } + } + return false; + } + + // ---- Wrapping the whole member body ---- + + private static async Task WrapMemberBodyAsync(Document document, BlockSyntax body, CancellationToken ct) + { + var root = await document.GetSyntaxRootAsync(ct).ConfigureAwait(false); + if (root is null || body.Statements.Count == 0) + return document; + + // Preserve each statement's existing leading trivia (comments, blank lines). + // BuildUnsafeBlock prepends the SAFETY-TODO comment to the first statement's + // existing trivia so user-authored comments survive. + var unsafeBlock = BuildUnsafeBlock([.. body.Statements]); + var newBody = SyntaxFactory.Block(unsafeBlock) + .WithOpenBraceToken(body.OpenBraceToken) + .WithCloseBraceToken(body.CloseBraceToken) + .WithAdditionalAnnotations(Formatter.Annotation); + + return document.WithSyntaxRoot(root.ReplaceNode(body, newBody)); + } + + // ---- Wrapping an expression-bodied member ---- + + private static async Task WrapArrowBodyAsync( + Document document, ArrowExpressionClauseSyntax arrow, SemanticModel? semanticModel, CancellationToken ct) + { + var root = await document.GetSyntaxRootAsync(ct).ConfigureAwait(false); + if (root is null || arrow.Parent is not { } member) + return document; + + bool requiresReturn = ArrowBodyRequiresReturn(member, semanticModel); + + // Build 'return ;' or ';' depending on the member's effective return type. + // Preserve the original expression's trivia inside the new statement so any inline + // comments authored on the arrow expression survive. + StatementSyntax inner = requiresReturn + ? SyntaxFactory.ReturnStatement(arrow.Expression.WithoutTrivia()) + : SyntaxFactory.ExpressionStatement(arrow.Expression.WithoutTrivia()); + var unsafeBlock = BuildUnsafeBlock([inner]); + var newBody = SyntaxFactory.Block(unsafeBlock).WithAdditionalAnnotations(Formatter.Annotation); + + // Replace the member's '=> expr;' with '{ unsafe { ... } }'. + var newMember = ReplaceArrowWithBlock(member, newBody); + if (newMember is null) + return document; + + return document.WithSyntaxRoot(root.ReplaceNode(member, newMember)); + } + + /// + /// True if rewriting 's arrow body into a block + /// body requires a return statement around the original expression. False when + /// the member produces no value (void method/local-function, set/init/add/remove + /// accessor, constructor/destructor, or an async method whose only result is the Task). + /// + private static bool ArrowBodyRequiresReturn(SyntaxNode memberWithArrowBody, SemanticModel? semanticModel) + { + switch (memberWithArrowBody) + { + case MethodDeclarationSyntax m: + return !IsVoidLikeMethod(m.ReturnType, m.Modifiers, semanticModel, m); + case LocalFunctionStatementSyntax lf: + return !IsVoidLikeMethod(lf.ReturnType, lf.Modifiers, semanticModel, lf); + case OperatorDeclarationSyntax: + case ConversionOperatorDeclarationSyntax: + case PropertyDeclarationSyntax: + case IndexerDeclarationSyntax: + return true; + case AccessorDeclarationSyntax a: + return a.Keyword.IsKind(SyntaxKind.GetKeyword); + case ConstructorDeclarationSyntax: + case DestructorDeclarationSyntax: + return false; + default: + // Unknown member kind - rewriting could produce broken syntax; assume no return. + return false; + } + } + + private static bool IsVoidLikeMethod(TypeSyntax returnType, SyntaxTokenList modifiers, SemanticModel? semanticModel, SyntaxNode declaration) + { + if (returnType is PredefinedTypeSyntax p && p.Keyword.IsKind(SyntaxKind.VoidKeyword)) + return true; + + // 'async Task' / 'async ValueTask' bodies do not return a value; the await/return + // expression is just an expression statement when in block form. + if (modifiers.Any(SyntaxKind.AsyncKeyword) && semanticModel is not null + && semanticModel.GetDeclaredSymbol(declaration) is IMethodSymbol m + && m.ReturnType is INamedTypeSymbol rt + && rt.TypeArguments.Length == 0 + && rt.Name is "Task" or "ValueTask") + { + return true; + } + + return false; + } + + private static SyntaxNode? ReplaceArrowWithBlock(SyntaxNode member, BlockSyntax newBody) => member switch + { + MethodDeclarationSyntax m => m.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + LocalFunctionStatementSyntax lf => lf.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + OperatorDeclarationSyntax op => op.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + ConversionOperatorDeclarationSyntax co => co.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + ConstructorDeclarationSyntax c => c.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + DestructorDeclarationSyntax d => d.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + AccessorDeclarationSyntax a => a.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default), + // For arrow-bodied property/indexer ('int P => expr;'), turn it into a block with a + // single get accessor whose body is the new unsafe block. + PropertyDeclarationSyntax prop => prop + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithAccessorList(SyntaxFactory.AccessorList(SyntaxFactory.SingletonList( + SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration).WithBody(newBody)))), + IndexerDeclarationSyntax idx => idx + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithAccessorList(SyntaxFactory.AccessorList(SyntaxFactory.SingletonList( + SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration).WithBody(newBody)))), + _ => null, + }; + + // ---- Wrapping a single statement ---- + + private static async Task WrapSingleStatementAsync( + Document document, StatementSyntax statement, FixStrategy strategy, CancellationToken ct) + { + var root = await document.GetSyntaxRootAsync(ct).ConfigureAwait(false); + if (root is null) + return document; + + SyntaxNode newRoot; + // Only attach the original statement's outer trivia to the inner pieces when we + // are going to splice them into an existing statement list. For embedded + // statements (parent is not Block/SwitchSection) ReplaceStatementWithStatements + // wraps the result in a fresh block and applies the trivia there - applying it + // here too would duplicate comments, blank lines, and indentation. + bool willSplice = statement.Parent is BlockSyntax or SwitchSectionSyntax; + if (strategy is FixStrategy.ForwardDeclare) + { + var semanticModel = await document.GetSemanticModelAsync(ct).ConfigureAwait(false); + var rewrite = TryRewriteAsForwardDeclaration((LocalDeclarationStatementSyntax)statement, semanticModel); + if (rewrite is null) + { + // Defensive: ChooseFixStrategy already verified the rewrite is possible by + // calling TryRewriteAsForwardDeclaration, so this branch should be unreachable. + // If it is reached (e.g. a future semantic-model change makes the rewrite fail + // here but not at strategy-decision time), bail out unchanged rather than + // producing a broken fix; wrap-as-is would not be safe because the local is + // referenced after the wrap point (that is why we picked ForwardDeclare). + return document; + } + + var forwardDecl = rewrite.Value.ForwardDeclaration; + var unsafeBlock = BuildUnsafeBlock([rewrite.Value.AssignmentStatement]) + .WithAdditionalAnnotations(Formatter.Annotation); + if (willSplice) + { + forwardDecl = forwardDecl.WithLeadingTrivia(statement.GetLeadingTrivia()); + unsafeBlock = unsafeBlock.WithTrailingTrivia(statement.GetTrailingTrivia()); + } + + newRoot = ReplaceStatementWithStatements(root, statement, [forwardDecl, unsafeBlock]); + } + else + { + var inner = statement.WithoutLeadingTrivia().WithoutTrailingTrivia(); + var unsafeBlock = BuildUnsafeBlock([inner]) + .WithAdditionalAnnotations(Formatter.Annotation); + if (willSplice) + { + unsafeBlock = unsafeBlock + .WithLeadingTrivia(statement.GetLeadingTrivia()) + .WithTrailingTrivia(statement.GetTrailingTrivia()); + } + + newRoot = ReplaceStatementWithStatements(root, statement, [unsafeBlock]); + } + + return document.WithSyntaxRoot(newRoot); + } + + // ---- Forward-declaration rewrite ---- + + private readonly record struct ForwardDeclarationRewrite( + LocalDeclarationStatementSyntax ForwardDeclaration, + StatementSyntax AssignmentStatement); + + /// + /// Attempts to split into a bare forward declaration + /// (scoped T x; or T x;) plus an assignment statement that can be + /// wrapped in an unsafe { } block. Returns null when the local + /// cannot be split without changing semantics or producing uncompilable syntax. + /// + private static ForwardDeclarationRewrite? TryRewriteAsForwardDeclaration( + LocalDeclarationStatementSyntax local, + SemanticModel? semanticModel) + { + if (local.IsConst) + return null; + if (!local.UsingKeyword.IsKind(SyntaxKind.None)) + return null; + if (local.Declaration.Variables.Count != 1) + return null; + if (local.Declaration.Type is RefTypeSyntax or ScopedTypeSyntax) + return null; + + var variable = local.Declaration.Variables[0]; + if (variable.Initializer is null) + return null; + + TypeSyntax typeSyntax = local.Declaration.Type; + ITypeSymbol? typeSymbol = null; + if (typeSyntax.IsVar) + { + if (semanticModel is null) + return null; + typeSymbol = semanticModel.GetTypeInfo(typeSyntax).Type; + if (typeSymbol is null or IErrorTypeSymbol) + return null; + // Anonymous / tuple-element types cannot be named in user code. + if (typeSymbol.IsAnonymousType || typeSymbol.Name.Length == 0) + return null; + + string typeName = typeSymbol.ToMinimalDisplayString(semanticModel, local.SpanStart); + typeSyntax = SyntaxFactory.ParseTypeName(typeName); + if (typeSyntax.ContainsDiagnostics) + return null; + } + else if (semanticModel is not null) + { + typeSymbol = semanticModel.GetTypeInfo(typeSyntax).Type; + } + + // ref structs (e.g. Span) need explicit 'scoped' so `x = stackalloc ...` compiles. + TypeSyntax bareType = typeSyntax.WithoutTrivia().WithTrailingTrivia(SyntaxFactory.Space); + TypeSyntax declType = typeSymbol is { IsRefLikeType: true } + ? SyntaxFactory.ScopedType( + SyntaxFactory.Token(SyntaxKind.ScopedKeyword).WithTrailingTrivia(SyntaxFactory.Space), + bareType) + : bareType; + + var forwardVarDecl = SyntaxFactory.VariableDeclaration( + declType, + [SyntaxFactory.VariableDeclarator(variable.Identifier.WithoutTrivia())]); + + var forwardDecl = SyntaxFactory.LocalDeclarationStatement(forwardVarDecl) + .WithTrailingTrivia(SyntaxFactory.ElasticCarriageReturnLineFeed); + + var assignment = SyntaxFactory.ExpressionStatement( + SyntaxFactory.AssignmentExpression( + SyntaxKind.SimpleAssignmentExpression, + SyntaxFactory.IdentifierName(variable.Identifier.WithoutTrivia()), + variable.Initializer.Value.WithoutTrivia())); + + return new ForwardDeclarationRewrite(forwardDecl, assignment); + } + + // ---- Block construction ---- + + private static UnsafeStatementSyntax BuildUnsafeBlock(IReadOnlyList innerStatements) + { + var safetyComment = SyntaxFactory.Comment(UnsafeEvolutionDescriptors.SafetyTodoCommentText); + var newline = SyntaxFactory.ElasticCarriageReturnLineFeed; + + // Attach the SAFETY-TODO comment as leading trivia of the first inner statement so + // it lands inside the braces of the produced block (and any pre-existing leading + // trivia of that statement, such as user comments, is preserved after the comment). + var first = innerStatements[0]; + var commentedFirst = first.WithLeadingTrivia(first.GetLeadingTrivia().InsertRange(0, [safetyComment, newline])); + + var newStatements = new List(innerStatements.Count) { commentedFirst }; + for (int i = 1; i < innerStatements.Count; i++) + newStatements.Add(innerStatements[i]); + + return SyntaxFactory.UnsafeStatement(SyntaxFactory.Block(newStatements)) + .WithAdditionalAnnotations(Formatter.Annotation); + } + + // ---- Replacement helpers ---- + + private static SyntaxNode ReplaceStatementWithStatements( + SyntaxNode root, StatementSyntax oldStatement, IReadOnlyList newStatements) + { + // Splice into the containing block or switch-section's statement list when possible, + // so we don't introduce an extra brace level. Embedded statements (e.g. the body of + // 'if (cond) M();') have neither parent kind, so we wrap them in a fresh block. + switch (oldStatement.Parent) + { + case BlockSyntax block: + return root.ReplaceNode(block, block.WithStatements(SpliceStatements(block.Statements, oldStatement, newStatements))); + case SwitchSectionSyntax section: + return root.ReplaceNode(section, section.WithStatements(SpliceStatements(section.Statements, oldStatement, newStatements))); + default: + var wrapping = SyntaxFactory.Block(newStatements) + .WithLeadingTrivia(oldStatement.GetLeadingTrivia()) + .WithTrailingTrivia(oldStatement.GetTrailingTrivia()) + .WithAdditionalAnnotations(Formatter.Annotation); + return root.ReplaceNode(oldStatement, wrapping); + } + } + + private static SyntaxList SpliceStatements( + SyntaxList existing, StatementSyntax target, IReadOnlyList replacement) + { + int idx = existing.IndexOf(target); + var updated = existing.RemoveAt(idx); + for (int i = replacement.Count - 1; i >= 0; i--) + updated = updated.Insert(idx, replacement[i]); + return updated; + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/RemoveUnsafeModifierCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/RemoveUnsafeModifierCodeFixProvider.cs new file mode 100644 index 00000000000000..6e06573ecfb260 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/RemoveUnsafeModifierCodeFixProvider.cs @@ -0,0 +1,98 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Editing; + +namespace ILLink.CodeFix.UnsafeEvolution +{ + /// + /// Removes the unsafe modifier from declarations where it is meaningless + /// (IL5005 / CS9377) or where it is probably unnecessary (IL5006). + /// + /// + /// The fixer is intentionally conservative: it only removes the modifier itself, + /// never the entire declaration, and the rules + /// already exclude cases that are not safe to rewrite (extern, partial, nested under + /// an unsafe type, etc). + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(RemoveUnsafeModifierCodeFixProvider)), Shared] + public sealed class RemoveUnsafeModifierCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider + { + private const string Title = "Remove 'unsafe' modifier"; + + public override ImmutableArray FixableDiagnosticIds => + [ + UnsafeEvolutionDescriptors.MeaninglessUnsafeModifierId, + UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifierId, + UnsafeEvolutionDescriptors.UnsafeMeaningless, + ]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var document = context.Document; + var diagnostic = context.Diagnostics.First(); + + if (await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false) is not { } root) + return; + + var declaration = FindDeclarationWithUnsafeModifier(root, diagnostic.Location.SourceSpan); + if (declaration is null) + return; + + context.RegisterCodeFix( + CodeAction.Create( + title: Title, + createChangedDocument: ct => RemoveUnsafeAsync(document, declaration, ct), + equivalenceKey: Title), + diagnostic); + } + + private static SyntaxNode? FindDeclarationWithUnsafeModifier(SyntaxNode root, Microsoft.CodeAnalysis.Text.TextSpan span) + { + var token = root.FindToken(span.Start); + for (var node = token.Parent; node is not null; node = node.Parent) + { + if (GetModifiers(node) is { } modifiers && modifiers.Any(SyntaxKind.UnsafeKeyword)) + return node; + } + return null; + } + + private static SyntaxTokenList? GetModifiers(SyntaxNode node) => node switch + { + BaseTypeDeclarationSyntax t => t.Modifiers, + DelegateDeclarationSyntax d => d.Modifiers, + BaseMethodDeclarationSyntax m => m.Modifiers, + LocalFunctionStatementSyntax lf => lf.Modifiers, + BasePropertyDeclarationSyntax p => p.Modifiers, + BaseFieldDeclarationSyntax f => f.Modifiers, + AccessorDeclarationSyntax a => a.Modifiers, + _ => null, + }; + + private static async Task RemoveUnsafeAsync(Document document, SyntaxNode declaration, CancellationToken ct) + { + // Use SyntaxGenerator to remove the modifier - this correctly transfers leading + // trivia (indentation, comments) from the removed token to whichever syntax now + // occupies the leading position of the declaration. + var editor = await DocumentEditor.CreateAsync(document, ct).ConfigureAwait(false); + var modifiers = editor.Generator.GetModifiers(declaration); + editor.SetModifiers(declaration, modifiers.WithIsUnsafe(false)); + return editor.GetChangedDocument(); + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeBlockHelpers.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeBlockHelpers.cs new file mode 100644 index 00000000000000..1c440a82803b9f --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeBlockHelpers.cs @@ -0,0 +1,100 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace ILLink.CodeFix.UnsafeEvolution +{ + /// + /// Helpers shared by the unsafe-evolution analyzer and code fixers. + /// + internal static class UnsafeBlockHelpers + { + /// + /// True if any pointer-typed or function-pointer-typed syntax appears anywhere + /// within (including nested generics, arrays, etc.). + /// + internal static bool ContainsPointerType(TypeSyntax? type) + { + if (type is null) + return false; + + return type is PointerTypeSyntax or FunctionPointerTypeSyntax + || type.DescendantNodes().Any(static n => n is PointerTypeSyntax or FunctionPointerTypeSyntax); + } + + /// + /// True if any parameter or return type of the given declaration mentions a pointer + /// or function-pointer type. Used as the heuristic gate for IL5006. + /// + internal static bool SignatureContainsPointer(SyntaxNode declaration) => declaration switch + { + MethodDeclarationSyntax m => SignatureHasPointer(m.ReturnType, m.ParameterList), + LocalFunctionStatementSyntax lf => SignatureHasPointer(lf.ReturnType, lf.ParameterList), + OperatorDeclarationSyntax op => SignatureHasPointer(op.ReturnType, op.ParameterList), + ConversionOperatorDeclarationSyntax co => SignatureHasPointer(co.Type, co.ParameterList), + DelegateDeclarationSyntax d => SignatureHasPointer(d.ReturnType, d.ParameterList), + ConstructorDeclarationSyntax c => SignatureHasPointer(returnType: null, c.ParameterList), + IndexerDeclarationSyntax idx => SignatureHasPointer(idx.Type, idx.ParameterList), + BasePropertyDeclarationSyntax bp => ContainsPointerType(bp.Type), // PropertyDeclaration, EventDeclaration + BaseFieldDeclarationSyntax bf => ContainsPointerType(bf.Declaration.Type), // FieldDeclaration, EventFieldDeclaration + AccessorDeclarationSyntax acc => acc.Parent?.Parent is { } owner && SignatureContainsPointer(owner), + _ => false, + }; + + private static bool SignatureHasPointer(TypeSyntax? returnType, BaseParameterListSyntax parameterList) + => ContainsPointerType(returnType) + || parameterList.Parameters.Any(static p => ContainsPointerType(p.Type)); + + /// + /// The first unsafe keyword in , or default if absent. + /// + internal static SyntaxToken FindUnsafeModifier(SyntaxTokenList modifiers) + => modifiers.FirstOrDefault(static t => t.IsKind(SyntaxKind.UnsafeKeyword)); + + /// + /// True when carries preprocessor directive trivia STRICTLY BETWEEN + /// its first and last tokens. Directives that sit in the leading trivia of the first token + /// or in the trailing trivia of the last token (i.e. an enclosing #if/#endif) are + /// excluded because preserving them across a rewrite is straightforward. + /// + internal static bool ContainsInternalDirectiveTrivia(SyntaxNode node) + { + int internalStart = node.GetFirstToken().Span.End; + int internalEnd = node.GetLastToken().Span.Start; + if (internalEnd <= internalStart) + return false; + + foreach (var trivia in node.DescendantTrivia(descendIntoTrivia: true)) + { + if (trivia.IsDirective + && trivia.SpanStart >= internalStart + && trivia.Span.End <= internalEnd) + { + return true; + } + } + return false; + } + + /// + /// Heuristic. Returns true if a member's body has enough unsafe diagnostics that + /// wrapping the whole body in one unsafe { } is preferable to wrapping each + /// statement individually. + /// + /// Number of CS9360/CS9361/CS9362/CS0214 diagnostics in the body. + /// Total number of statements in the body. + internal static bool ShouldWrapEntireBody(int unsafeDiagnosticCount, int totalStatementCount) + { + if (unsafeDiagnosticCount <= 0) + return false; + // Three or more unsafe operations and at least every 4th statement is unsafe. + return unsafeDiagnosticCount >= 3 && unsafeDiagnosticCount * 4 >= totalStatementCount; + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionAnalyzer.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionAnalyzer.cs new file mode 100644 index 00000000000000..ed28bd006ad818 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionAnalyzer.cs @@ -0,0 +1,152 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace ILLink.CodeFix.UnsafeEvolution +{ + /// + /// Reports IL5005 (meaningless unsafe modifier on a type / static ctor / destructor / delegate) + /// and IL5006 (probably-unnecessary unsafe modifier on a member whose signature has no pointer types). + /// + /// + /// These supplement compiler diagnostics so that dotnet format can drive the + /// on assemblies that have not yet opted into + /// the updated memory-safety rules. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class UnsafeEvolutionAnalyzer : DiagnosticAnalyzer + { + public override ImmutableArray SupportedDiagnostics => + [ + UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier, + UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier, + ]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + + // IL5005 - meaningless unsafe on types and special members. + context.RegisterSyntaxNodeAction(AnalyzeMeaninglessUnsafe, + SyntaxKind.ClassDeclaration, + SyntaxKind.StructDeclaration, + SyntaxKind.InterfaceDeclaration, + SyntaxKind.RecordDeclaration, + SyntaxKind.RecordStructDeclaration, + SyntaxKind.DelegateDeclaration, + SyntaxKind.ConstructorDeclaration, + SyntaxKind.DestructorDeclaration); + + // IL5006 - probably-unnecessary unsafe on a member whose signature has no pointer types. + context.RegisterSyntaxNodeAction(AnalyzeUnnecessaryUnsafe, + SyntaxKind.MethodDeclaration, + SyntaxKind.LocalFunctionStatement, + SyntaxKind.PropertyDeclaration, + SyntaxKind.IndexerDeclaration, + SyntaxKind.FieldDeclaration, + SyntaxKind.EventFieldDeclaration, + SyntaxKind.EventDeclaration); + } + + // ---- IL5005: meaningless 'unsafe' ---- + + private static void AnalyzeMeaninglessUnsafe(SyntaxNodeAnalysisContext context) + { + var node = context.Node; + var (modifiers, kind, name) = node switch + { + ClassDeclarationSyntax c => (c.Modifiers, "class", c.Identifier.ValueText), + StructDeclarationSyntax s => (s.Modifiers, "struct", s.Identifier.ValueText), + InterfaceDeclarationSyntax i => (i.Modifiers, "interface", i.Identifier.ValueText), + RecordDeclarationSyntax { ClassOrStructKeyword.RawKind: (int)SyntaxKind.StructKeyword } r + => (r.Modifiers, "record struct", r.Identifier.ValueText), + RecordDeclarationSyntax r => (r.Modifiers, "record", r.Identifier.ValueText), + DelegateDeclarationSyntax d => (d.Modifiers, "delegate", d.Identifier.ValueText), + ConstructorDeclarationSyntax c => (c.Modifiers, "static constructor", c.Identifier.ValueText), + DestructorDeclarationSyntax d => (d.Modifiers, "destructor", d.Identifier.ValueText), + _ => (default(SyntaxTokenList), "", ""), + }; + + // Instance constructors can legitimately be requires-unsafe; only static ones are meaningless. + if (node is ConstructorDeclarationSyntax && !modifiers.Any(SyntaxKind.StaticKeyword)) + return; + + var unsafeToken = UnsafeBlockHelpers.FindUnsafeModifier(modifiers); + if (unsafeToken == default) + return; + + context.ReportDiagnostic(Diagnostic.Create( + UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier, + unsafeToken.GetLocation(), + kind, + name)); + } + + // ---- IL5006: probably-unnecessary 'unsafe' on a signature without pointers ---- + + private static void AnalyzeUnnecessaryUnsafe(SyntaxNodeAnalysisContext context) + { + var node = context.Node; + var (modifiers, name) = node switch + { + MethodDeclarationSyntax m => (m.Modifiers, m.Identifier.ValueText), + LocalFunctionStatementSyntax lf => (lf.Modifiers, lf.Identifier.ValueText), + PropertyDeclarationSyntax p => (p.Modifiers, p.Identifier.ValueText), + IndexerDeclarationSyntax => (((IndexerDeclarationSyntax)node).Modifiers, "this[]"), + FieldDeclarationSyntax f => (f.Modifiers, FirstVariableName(f.Declaration)), + EventFieldDeclarationSyntax ef => (ef.Modifiers, FirstVariableName(ef.Declaration)), + EventDeclarationSyntax e => (e.Modifiers, e.Identifier.ValueText), + _ => (default(SyntaxTokenList), ""), + }; + + if (!HasRemovableUnsafe(node, modifiers)) + return; + + if (UnsafeBlockHelpers.SignatureContainsPointer(node)) + return; + + var unsafeToken = UnsafeBlockHelpers.FindUnsafeModifier(modifiers); + context.ReportDiagnostic(Diagnostic.Create( + UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier, + unsafeToken.GetLocation(), + name)); + } + + private static string FirstVariableName(VariableDeclarationSyntax decl) + => decl.Variables.Count > 0 ? decl.Variables[0].Identifier.ValueText : "?"; + + // ---- Common 'should we suggest removing the unsafe modifier?' predicate ---- + + private static bool HasRemovableUnsafe(SyntaxNode decl, SyntaxTokenList modifiers) + { + if (!modifiers.Any(SyntaxKind.UnsafeKeyword)) + return false; + + // 'extern' members must be explicitly marked unsafe or safe in the new rules; don't suggest removal. + if (modifiers.Any(SyntaxKind.ExternKeyword)) + return false; + + // Partial members require both halves to agree on 'unsafe'; we can't fix one safely. + if (modifiers.Any(SyntaxKind.PartialKeyword)) + return false; + + // Be conservative for members nested inside a type that also carries 'unsafe' - the + // type-level IL5005 will fire on the containing type, which is the better fix. + for (SyntaxNode? p = decl.Parent; p is not null; p = p.Parent) + { + if (p is TypeDeclarationSyntax td && td.Modifiers.Any(SyntaxKind.UnsafeKeyword)) + return false; + } + + return true; + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionDescriptors.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionDescriptors.cs new file mode 100644 index 00000000000000..3c6f6108182e72 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionDescriptors.cs @@ -0,0 +1,85 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using Microsoft.CodeAnalysis; + +namespace ILLink.CodeFix.UnsafeEvolution +{ + /// + /// Diagnostic identifiers consumed and produced by the unsafe-evolution analyzer and code fixers. + /// + /// + /// The CS**** identifiers come from the C# compiler when the + /// updated-memory-safety-rules feature is enabled. The IL5*** + /// identifiers are emitted by for cases the + /// compiler does not flag on its own (such as unsafe modifiers on signatures that + /// no longer need them). + /// + public static class UnsafeEvolutionDescriptors + { + public const string Category = "MemorySafety"; + + // ---- Compiler diagnostics consumed by IntroduceUnsafeBlockCodeFixProvider ---- + + /// Legacy: pointer used outside an unsafe context. + public const string PointersAndFixedBuffersUnsafe = "CS0214"; + + /// Operation requires unsafe context (e.g. pointer dereference). + public const string UnsafeOperation = "CS9360"; + + /// Uninitialized stackalloc -> Span<T> with SkipLocalsInit. + public const string UnsafeUninitializedStackAlloc = "CS9361"; + + /// Call of a requires-unsafe member outside an unsafe context. + public const string UnsafeMemberOperation = "CS9362"; + + /// Compat-mode call of a member that contains pointers in its signature. + public const string UnsafeMemberOperationCompat = "CS9363"; + + // ---- Compiler diagnostics consumed by RemoveUnsafeModifierCodeFixProvider ---- + + /// Warning emitted by the compiler for meaningless unsafe modifiers. + public const string UnsafeMeaningless = "CS9377"; + + // ---- ILLink analyzer diagnostics ---- + + /// + /// Meaningless unsafe modifier on a type, static constructor, destructor or delegate declaration. + /// + public const string MeaninglessUnsafeModifierId = "IL5005"; + + /// + /// Probably-unnecessary unsafe modifier on a member whose signature contains no pointer types. + /// + public const string UnnecessaryUnsafeModifierId = "IL5006"; + + public static readonly DiagnosticDescriptor MeaninglessUnsafeModifier = new( + id: MeaninglessUnsafeModifierId, + title: "The 'unsafe' modifier has no effect on this declaration", + messageFormat: "The 'unsafe' modifier has no effect on {0} '{1}' and should be removed", + category: Category, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: + "Under the updated memory-safety rules, the 'unsafe' modifier is meaningless on type, " + + "delegate, static constructor and destructor declarations. Remove it."); + + public static readonly DiagnosticDescriptor UnnecessaryUnsafeModifier = new( + id: UnnecessaryUnsafeModifierId, + title: "The 'unsafe' modifier on this signature is probably unnecessary", + messageFormat: "The 'unsafe' modifier on '{0}' is probably unnecessary; its signature contains no pointer types", + category: Category, + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: + "Under the updated memory-safety rules, marking a member as 'unsafe' makes it requires-unsafe; " + + "callers must use an 'unsafe' context. If the signature does not expose pointer types, the modifier " + + "is probably not needed. Review and remove it if appropriate."); + + // ---- Wording inserted by IntroduceUnsafeBlockCodeFixProvider ---- + + public const string SafetyTodoCommentText = "// SAFETY-TODO: Audit"; + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseCompilation.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseCompilation.cs index 5bfbc2ac5dcd6b..e7e03f21234f08 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseCompilation.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseCompilation.cs @@ -26,6 +26,9 @@ private static ImmutableArray CreateSupportedDiagnosticAnaly builder.Add(new RequiresAssemblyFilesAnalyzer()); builder.Add(new RequiresUnreferencedCodeAnalyzer()); builder.Add(new DynamicallyAccessedMembersAnalyzer()); +#if DEBUG + builder.Add(new ILLink.CodeFix.UnsafeEvolution.UnsafeEvolutionAnalyzer()); +#endif return builder.ToImmutable(); } diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/IntroduceUnsafeBlockCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/IntroduceUnsafeBlockCodeFixTests.cs new file mode 100644 index 00000000000000..5683fc65e6c06b --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/IntroduceUnsafeBlockCodeFixTests.cs @@ -0,0 +1,752 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System; +using System.Threading.Tasks; +using ILLink.CodeFix.UnsafeEvolution; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Testing; +using Xunit; +using VerifyCS = ILLink.RoslynAnalyzer.Tests.CSharpCodeFixVerifier< + ILLink.RoslynAnalyzer.DynamicallyAccessedMembersAnalyzer, + ILLink.CodeFix.UnsafeEvolution.IntroduceUnsafeBlockCodeFixProvider>; + +namespace ILLink.RoslynAnalyzer.Tests.UnsafeEvolution +{ + public class IntroduceUnsafeBlockCodeFixTests + { + private static Solution SetOptions(Solution solution, ProjectId projectId) + { + var project = solution.GetProject(projectId)!; + var parseOptions = (CSharpParseOptions)project.ParseOptions!; + parseOptions = parseOptions.WithLanguageVersion(LanguageVersion.Preview) + .WithFeatures([.. parseOptions.Features, new("updated-memory-safety-rules", "")]); + var compilationOptions = ((CSharpCompilationOptions)project.CompilationOptions!).WithAllowUnsafe(true); + return solution.WithProjectParseOptions(projectId, parseOptions) + .WithProjectCompilationOptions(projectId, compilationOptions); + } + + private static Task RunAsync( + string source, + string fixedSource, + DiagnosticResult[] baselineExpected, + DiagnosticResult[]? fixedExpected = null, + int? numberOfIterations = null) + { + var test = new VerifyCS.Test + { + TestCode = source, + FixedCode = fixedSource, + }; + test.ExpectedDiagnostics.AddRange(baselineExpected); + test.FixedState.ExpectedDiagnostics.AddRange(fixedExpected ?? []); + test.SolutionTransforms.Add(SetOptions); + if (numberOfIterations is not null) + { + test.NumberOfIncrementalIterations = numberOfIterations; + test.NumberOfFixAllIterations = numberOfIterations; + } + return test.RunAsync(); + } + + // ---- CS9362: requires-unsafe member call ---- + + [Fact] + public async Task CS9362_WrapsExpressionStatement() + { + string source = """ + public class C + { + public static unsafe void M1() { } + + public void M2() + { + M1(); + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe void M1() { } + + public void M2() + { + unsafe + { + // SAFETY-TODO: Audit + M1(); + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 9, 7, 13) + .WithArguments("C.M1()"), + ]); + } + + [Fact] + public async Task CS9362_WrapsReturnStatement() + { + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + return M1(); + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + unsafe + { + // SAFETY-TODO: Audit + return M1(); + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 16, 7, 20) + .WithArguments("C.M1()"), + ]); + } + + [Fact] + public async Task CS9362_LocalDeclaration_SplitsIntoForwardDecl() + { + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public void M2() + { + int x = M1(); + System.Console.WriteLine(x); + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public void M2() + { + int x; + unsafe + { + // SAFETY-TODO: Audit + x = M1(); + } + System.Console.WriteLine(x); + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 17, 7, 21) + .WithArguments("C.M1()"), + ]); + } + + // ---- CS9361: uninitialized stackalloc -> Span with SkipLocalsInit ---- + + [Fact] + public async Task CS9361_StackAllocToSpan_SplitsWithScopedForwardDecl() + { + string source = """ + using System; + using System.Runtime.CompilerServices; + + [module: SkipLocalsInit] + + public class C + { + public void M() + { + Span s = stackalloc byte[10]; + s[0] = 1; + } + } + """; + + string fixedSource = """ + using System; + using System.Runtime.CompilerServices; + + [module: SkipLocalsInit] + + public class C + { + public void M() + { + scoped Span s; + unsafe + { + // SAFETY-TODO: Audit + s = stackalloc byte[10]; + } + s[0] = 1; + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeUninitializedStackAlloc) + .WithSpan(10, 24, 10, 43), + ]); + } + + // ---- Wrap entire method body when many unsafe operations cluster ---- + + [Fact] + public async Task ManyUnsafeOps_WrapsEntireMemberBody() + { + // Five unsafe operations packed into a short method body: density triggers the + // ShouldWrapEntireBody heuristic (>=3 unsafe ops AND unsafe*4 >= statementCount). + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + int a = M1(); + int b = M1(); + int c = M1(); + int d = M1(); + int e = M1(); + return a + b + c + d + e; + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + unsafe + { + // SAFETY-TODO: Audit + int a = M1(); + int b = M1(); + int c = M1(); + int d = M1(); + int e = M1(); + return a + b + c + d + e; + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 17, 7, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 17, 8, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(9, 17, 9, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(10, 17, 10, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(11, 17, 11, 21).WithArguments("C.M1()"), + ]); + } + + // ---- Embedded statement (no surrounding block) ---- + + [Fact] + public async Task EmbeddedStatement_GetsWrappingBlock() + { + string source = """ + public class C + { + public static unsafe void M1() {} + + public void M2(bool cond) + { + if (cond) + M1(); + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe void M1() {} + + public void M2(bool cond) + { + if (cond) + { + unsafe + { + // SAFETY-TODO: Audit + M1(); + } + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 13, 8, 17) + .WithArguments("C.M1()"), + ]); + } + + // ---- Local declaration with 'var' resolves to explicit type for forward decl ---- + + [Fact] + public async Task CS9362_LocalDeclaration_VarResolvesToExplicitType() + { + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public void M2() + { + var x = M1(); + System.Console.WriteLine(x); + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public void M2() + { + int x; + unsafe + { + // SAFETY-TODO: Audit + x = M1(); + } + System.Console.WriteLine(x); + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 17, 7, 21) + .WithArguments("C.M1()"), + ]); + } + + // ---- Statement surrounded by #if directives IS wrapped (directives are preserved) ---- + + [Fact] + public async Task StatementInsideIfBranch_IsWrapped() + { + // The #if/#endif directives sit in the leading/trailing trivia AROUND the statement, + // not BETWEEN its tokens, so wrapping is safe - the unsafe block lands fully inside + // the conditional region. + string source = """ + public class C + { + public static unsafe void M1() {} + + public void M2() + { + #if true + M1(); + #endif + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe void M1() {} + + public void M2() + { + #if true + unsafe + { + // SAFETY-TODO: Audit + M1(); + } + #endif + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 9, 8, 13) + .WithArguments("C.M1()"), + ]); + } + + // ---- Statement with a directive BETWEEN its tokens is skipped ---- + + [Fact] + public async Task StatementWithInternalDirective_IsSkipped() + { + // The #if directive is between the call's arguments, i.e. INSIDE the statement. + // Splitting / wrapping such a statement would corrupt the directive region. + string source = """ + public class C + { + public static unsafe void M1(int a, int b) {} + + public void M2() + { + M1( + #if true + 1 + #else + 2 + #endif + , 3); + } + } + """; + + await RunAsync(source, source, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 9, 13, 17) + .WithArguments("C.M1(int, int)"), + ], [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 9, 13, 17) + .WithArguments("C.M1(int, int)"), + ]); + } + + // ---- Safety: 'using var' whose lifetime would be shortened by wrapping is skipped ---- + + [Fact] + public async Task UsingVarLocal_WhoseLifetimeWouldChange_IsSkipped() + { + // 'using var stream = M1();' followed by uses outside an unsafe block must not be + // wrapped: the wrap would shrink the 'using' lifetime to the block. + string source = """ + using System.IO; + public class C + { + public static unsafe Stream M1() => null; + + public void M2() + { + using var stream = M1(); + stream.Flush(); + } + } + """; + + await RunAsync(source, source, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 28, 8, 32) + .WithArguments("C.M1()"), + ], [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 28, 8, 32) + .WithArguments("C.M1()"), + ]); + } + + // ---- Safety: 'out var' pattern variable referenced after is skipped ---- + + [Fact] + public async Task LocalWithOutVarInitializer_PatternUsedAfter_IsSkipped() + { + // 'int ok = M1(out var value); Use(value);' - if we forward-declare 'ok', 'value' + // would be trapped inside the unsafe block. Bail out instead of producing bad code. + string source = """ + public class C + { + public static unsafe int M1(out int v) { v = 0; return 1; } + + public void M2() + { + int ok = M1(out var value); + System.Console.WriteLine(value); + System.Console.WriteLine(ok); + } + } + """; + + await RunAsync(source, source, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 18, 7, 35) + .WithArguments("C.M1(out int)"), + ], [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 18, 7, 35) + .WithArguments("C.M1(out int)"), + ]); + } + + // ---- Whole-body wrap preserves user comments inside the body ---- + + [Fact] + public async Task ManyUnsafeOps_WrappingBody_PreservesComments() + { + // Five unsafe operations + interleaved user comments. The comments must survive + // when we wrap the body in a single 'unsafe' block. + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + // First comment + int a = M1(); + int b = M1(); + // Second comment + int c = M1(); + int d = M1(); + int e = M1(); + return a + b + c + d + e; + } + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + unsafe + { + // SAFETY-TODO: Audit + // First comment + int a = M1(); + int b = M1(); + // Second comment + int c = M1(); + int d = M1(); + int e = M1(); + return a + b + c + d + e; + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 17, 8, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(9, 17, 9, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(11, 17, 11, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(12, 17, 12, 21).WithArguments("C.M1()"), + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(13, 17, 13, 21).WithArguments("C.M1()"), + ]); + } + + // ---- Safety: diagnostic inside an expression-bodied lambda is skipped ---- + + [Fact] + public async Task DiagnosticInsideExpressionBodiedLambda_IsSkipped() + { + // Wrapping the outer 'Action a = ...' in unsafe would not put the lambda's body + // in an unsafe context, so the diagnostic would persist. Bail entirely. + string source = """ + using System; + public class C + { + public static unsafe void M1() {} + + public void M2() + { + Action a = () => M1(); + a(); + } + } + """; + + await RunAsync(source, source, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 26, 8, 30) + .WithArguments("C.M1()"), + ], [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(8, 26, 8, 30) + .WithArguments("C.M1()"), + ]); + } + + // ---- Safety: 'var x = ...' that resolves to an unnameable type and escapes is skipped ---- + + [Fact] + public async Task LocalWithAnonymousVar_UsedAfter_IsSkipped() + { + // 'var x = new { A = M1() }' makes x an anonymous type. We cannot forward-declare + // such a local because the type cannot be named, and we cannot wrap-as-is because + // that would trap 'x' inside the unsafe block while 'Use(x)' is outside. + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public void M2() + { + var x = new { A = M1() }; + System.Console.WriteLine(x); + } + } + """; + + await RunAsync(source, source, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 27, 7, 31) + .WithArguments("C.M1()"), + ], [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(7, 27, 7, 31) + .WithArguments("C.M1()"), + ]); + } + + // ---- Expression-bodied members: arrow body -> block body with unsafe wrap ---- + + [Fact] + public async Task ExpressionBodiedMethod_NonVoid_IsRewrittenToBlockWithReturn() + { + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() => M1(); + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public int M2() + { + unsafe + { + // SAFETY-TODO: Audit + return M1(); + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(5, 24, 5, 28) + .WithArguments("C.M1()"), + ]); + } + + [Fact] + public async Task ExpressionBodiedMethod_Void_IsRewrittenToBlockWithExpressionStatement() + { + string source = """ + public class C + { + public static unsafe void M1() { } + + public void M2() => M1(); + } + """; + + string fixedSource = """ + public class C + { + public static unsafe void M1() { } + + public void M2() + { + unsafe + { + // SAFETY-TODO: Audit + M1(); + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(5, 25, 5, 29) + .WithArguments("C.M1()"), + ]); + } + + [Fact] + public async Task ExpressionBodiedProperty_IsRewrittenToGetAccessorWithUnsafeBlock() + { + string source = """ + public class C + { + public static unsafe int M1() => 0; + + public int P => M1(); + } + """; + + string fixedSource = """ + public class C + { + public static unsafe int M1() => 0; + + public int P + { + get + { + unsafe + { + // SAFETY-TODO: Audit + return M1(); + } + } + } + } + """; + + await RunAsync(source, fixedSource, [ + DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation) + .WithSpan(5, 21, 5, 25) + .WithArguments("C.M1()"), + ]); + } + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/RemoveUnsafeModifierCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/RemoveUnsafeModifierCodeFixTests.cs new file mode 100644 index 00000000000000..a94a53621b778e --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/RemoveUnsafeModifierCodeFixTests.cs @@ -0,0 +1,252 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Threading.Tasks; +using ILLink.CodeFix.UnsafeEvolution; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Testing; +using Xunit; +using VerifyCS = ILLink.RoslynAnalyzer.Tests.CSharpCodeFixVerifier< + ILLink.CodeFix.UnsafeEvolution.UnsafeEvolutionAnalyzer, + ILLink.CodeFix.UnsafeEvolution.RemoveUnsafeModifierCodeFixProvider>; + +namespace ILLink.RoslynAnalyzer.Tests.UnsafeEvolution +{ + public class RemoveUnsafeModifierCodeFixTests + { + private static Solution SetOptions(Solution solution, ProjectId projectId) + { + var project = solution.GetProject(projectId)!; + var compilationOptions = ((CSharpCompilationOptions)project.CompilationOptions!).WithAllowUnsafe(true); + return solution.WithProjectCompilationOptions(projectId, compilationOptions); + } + + private static Task RunAsync(string source, string fixedSource, params DiagnosticResult[] expected) + { + var test = new VerifyCS.Test + { + TestCode = source, + FixedCode = fixedSource, + }; + test.ExpectedDiagnostics.AddRange(expected); + test.SolutionTransforms.Add(SetOptions); + return test.RunAsync(); + } + + [Fact] + public async Task Removes_UnsafeOn_Class() + { + string source = """ + public unsafe class C + { + } + """; + + string fixedSource = """ + public class C + { + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(1, 8, 1, 14) + .WithArguments("class", "C")); + } + + [Fact] + public async Task Removes_UnsafeOn_Struct() + { + string source = """ + public unsafe struct S {} + """; + + string fixedSource = """ + public struct S {} + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(1, 8, 1, 14) + .WithArguments("struct", "S")); + } + + [Fact] + public async Task Removes_UnsafeOn_RecordStruct() + { + string source = """ + public unsafe record struct R(int X); + """; + + string fixedSource = """ + public record struct R(int X); + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(1, 8, 1, 14) + .WithArguments("record struct", "R")); + } + + [Fact] + public async Task Removes_UnsafeOn_Delegate() + { + string source = """ + public unsafe delegate void D(); + """; + + string fixedSource = """ + public delegate void D(); + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(1, 8, 1, 14) + .WithArguments("delegate", "D")); + } + + [Fact] + public async Task Removes_UnsafeOn_StaticConstructor() + { + string source = """ + public class C + { + static unsafe C() {} + } + """; + + string fixedSource = """ + public class C + { + static C() {} + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(3, 12, 3, 18) + .WithArguments("static constructor", "C")); + } + + [Fact] + public async Task Removes_UnsafeOn_Destructor() + { + string source = """ + public class C + { + unsafe ~C() {} + } + """; + + string fixedSource = """ + public class C + { + ~C() {} + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(3, 5, 3, 11) + .WithArguments("destructor", "C")); + } + + [Fact] + public async Task Removes_UnsafeOn_MethodWithoutPointerSignature() + { + string source = """ + public class C + { + public unsafe void M() {} + } + """; + + string fixedSource = """ + public class C + { + public void M() {} + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 12, 3, 18) + .WithArguments("M")); + } + + [Fact] + public async Task Removes_UnsafeOn_FieldWithoutPointer() + { + string source = """ + public class C + { + private unsafe int _x; + } + """; + + string fixedSource = """ + public class C + { + private int _x; + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 13, 3, 19) + .WithArguments("_x")); + } + + [Fact] + public async Task PreservesLeadingComment_WhenRemovingUnsafe() + { + string source = """ + public class C + { + // Important comment + public unsafe void M() {} + } + """; + + string fixedSource = """ + public class C + { + // Important comment + public void M() {} + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(4, 12, 4, 18) + .WithArguments("M")); + } + + [Fact] + public async Task Removes_UnsafeFirst_WhenAlsoPublic() + { + // When 'unsafe' is the very first modifier, ensure removal preserves the rest. + string source = """ + public class C + { + unsafe public void M() {} + } + """; + + string fixedSource = """ + public class C + { + public void M() {} + } + """; + + await RunAsync(source, fixedSource, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 5, 3, 11) + .WithArguments("M")); + } + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/UnsafeEvolutionAnalyzerTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/UnsafeEvolutionAnalyzerTests.cs new file mode 100644 index 00000000000000..2eb930c14e09c8 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/UnsafeEvolutionAnalyzerTests.cs @@ -0,0 +1,341 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Threading.Tasks; +using ILLink.CodeFix.UnsafeEvolution; +using Microsoft.CodeAnalysis.Testing; +using Xunit; +using VerifyCS = ILLink.RoslynAnalyzer.Tests.CSharpAnalyzerVerifier< + ILLink.CodeFix.UnsafeEvolution.UnsafeEvolutionAnalyzer>; + +namespace ILLink.RoslynAnalyzer.Tests.UnsafeEvolution +{ + public class UnsafeEvolutionAnalyzerTests + { + private static Task RunAsync(string source, params DiagnosticResult[] expected) => + VerifyCS.VerifyAnalyzerAsync( + src: source, + consoleApplication: false, + analyzerOptions: null, + additionalReferences: null, + allowUnsafe: true, + expected: expected); + + // ---- IL5005: meaningless unsafe on types and special members ---- + + [Theory] + [InlineData("class", "class")] + [InlineData("struct", "struct")] + [InlineData("interface", "interface")] + [InlineData("record", "record")] + [InlineData("record struct", "record struct")] + public async Task IL5005_FiresOn_UnsafeTypeDeclaration(string declKind, string expectedKindArg) + { + string source = $$""" + public unsafe {{declKind}} C {} + """; + + int unsafeColumn = "public ".Length + 1; // 1-based + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithLocation(1, unsafeColumn) + .WithArguments(expectedKindArg, "C")); + } + + [Fact] + public async Task IL5005_FiresOn_UnsafeDelegate() + { + string source = """ + public unsafe delegate void D(); + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithLocation(1, 8) + .WithArguments("delegate", "D")); + } + + [Fact] + public async Task IL5005_FiresOn_UnsafeStaticConstructor() + { + string source = """ + public class C + { + static unsafe C() {} + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(3, 12, 3, 18) + .WithArguments("static constructor", "C")); + } + + [Fact] + public async Task IL5005_DoesNotFireOn_UnsafeInstanceConstructor() + { + // Instance constructors can be requires-unsafe under the new rules. + string source = """ + public class C + { + public unsafe C() {} + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5005_FiresOn_UnsafeDestructor() + { + string source = """ + public class C + { + unsafe ~C() {} + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(3, 5, 3, 11) + .WithArguments("destructor", "C")); + } + + // ---- IL5006: unnecessary unsafe modifier on signatures with no pointer types ---- + + [Fact] + public async Task IL5006_FiresOn_MethodWithoutPointerSignature() + { + string source = """ + public class C + { + public unsafe void M() {} + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 12, 3, 18) + .WithArguments("M")); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_MethodWithPointerParameter() + { + string source = """ + public class C + { + public unsafe void M(int* p) {} + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_MethodWithPointerReturn() + { + string source = """ + public class C + { + public unsafe int* M() => null; + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_MethodWithPointerInNestedType() + { + // int*[] is allowed; nested pointer type counts as a pointer for our heuristic. + string source = """ + public class C + { + public unsafe int*[] M() => null; + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_MethodWithFunctionPointerParameter() + { + string source = """ + public class C + { + public unsafe void M(delegate* p) {} + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_ExternMethod() + { + string source = """ + using System.Runtime.InteropServices; + public class C + { + [DllImport("foo")] + public static extern unsafe int M(); + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_PartialMethod() + { + string source = """ + public partial class C + { + public unsafe partial void M(); + public unsafe partial void M() {} + } + """; + + await RunAsync(source); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_MemberInsideUnsafeType() + { + // The type itself triggers IL5005; the member is fixed transitively. + string source = """ + public unsafe class C + { + public unsafe void M() {} + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier) + .WithSpan(1, 8, 1, 14) + .WithArguments("class", "C")); + } + + [Fact] + public async Task IL5006_FiresOn_FieldWithoutPointerType() + { + string source = """ + public class C + { + private unsafe int _x; + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 13, 3, 19) + .WithArguments("_x"), + DiagnosticResult.CompilerWarning("CS0169").WithSpan(3, 24, 3, 26).WithArguments("C._x")); + } + + [Fact] + public async Task IL5006_DoesNotFireOn_FieldWithPointerType() + { + string source = """ + public class C + { + private unsafe int* _x; + } + """; + + await RunAsync(source, + DiagnosticResult.CompilerWarning("CS0169").WithSpan(3, 25, 3, 27).WithArguments("C._x")); + } + + [Fact] + public async Task IL5006_FiresOn_PropertyWithoutPointer() + { + string source = """ + public class C + { + public unsafe int P { get; set; } + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 12, 3, 18) + .WithArguments("P")); + } + + [Fact] + public async Task IL5006_FiresOn_IndexerWithoutPointer() + { + string source = """ + public class C + { + public unsafe int this[int i] => 0; + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(3, 12, 3, 18) + .WithArguments("this[]")); + } + + [Fact] + public async Task IL5006_FiresOn_LocalFunctionWithoutPointer() + { + string source = """ + public class C + { + public void M() + { + unsafe void Local() {} + Local(); + } + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(5, 9, 5, 15) + .WithArguments("Local")); + } + + [Fact] + public async Task IL5006_FiresOn_EventFieldWithoutPointer() + { + string source = """ + using System; + public class C + { + public unsafe event Action E; + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(4, 12, 4, 18) + .WithArguments("E"), + DiagnosticResult.CompilerWarning("CS0067").WithSpan(4, 32, 4, 33).WithArguments("C.E")); + } + + [Fact] + public async Task IL5006_FiresOn_EventWithExplicitAccessorsWithoutPointer() + { + string source = """ + using System; + public class C + { + public unsafe event Action E { add { } remove { } } + } + """; + + await RunAsync(source, + VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier) + .WithSpan(4, 12, 4, 18) + .WithArguments("E")); + } + } +} +#endif