From 8a76e1ec79a896b4933c9ec99ef778963b6e0535 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 24 Jul 2026 19:53:15 +0200 Subject: [PATCH 1/4] Add unsafe context migration code fixes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86742a9-ab20-4209-83f8-d7074a99b968 --- .../src/ILLink.CodeFix/AddUnsafeContext.cs | 1405 ++++++++++++++ .../ILLink.CodeFixProvider.csproj | 1 + .../RequiresUnsafeCodeFixProvider.cs | 574 ------ .../illink/src/ILLink.CodeFix/Resources.resx | 7 +- ...ynchronizeUnsafeContractCodeFixProvider.cs | 699 +++++++ .../UnsafeModifierCodeFixHelpers.cs | 35 +- .../UnsafeContractHelpers.cs | 219 +++ .../UnsafeMigrationSyntaxHelpers.cs | 3 + .../AddUnsafeContextCodeFixTests.cs | 1466 +++++++++++++++ .../RequiresUnsafeCodeFixTests.cs | 1675 ----------------- .../SynchronizeUnsafeContractCodeFixTests.cs | 476 +++++ .../UnsafeMigrationTestHelpers.cs | 1 + 12 files changed, 4307 insertions(+), 2254 deletions(-) create mode 100644 src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs delete mode 100644 src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs create mode 100644 src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs create mode 100644 src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs create mode 100644 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs delete mode 100644 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs create mode 100644 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs diff --git a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs new file mode 100644 index 00000000000000..e72aa24db2292c --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs @@ -0,0 +1,1405 @@ +// 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 ILLink.CodeFixProvider; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Formatting; +using Microsoft.CodeAnalysis.Text; + +namespace ILLink.CodeFix +{ + /// + /// Fixes unsafe-context diagnostics by introducing the narrowest context that preserves source semantics. + /// Unsafe statements are preferred so the generated audit comment can be expanded during review. + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AddUnsafeContextCodeFixProvider)), Shared] + public sealed class AddUnsafeContextCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider + { + public const string UnsafeOperationDiagnosticId = "CS9360"; + public const string UnsafeUninitializedStackAllocDiagnosticId = "CS9361"; + public const string UnsafeMemberOperationDiagnosticId = "CS9362"; + public const string UnsafeMemberOperationCompatDiagnosticId = "CS9363"; + public const string UnsafeConstructorConstraintDiagnosticId = "CS9376"; + + private const string UnsafeExpressionPlaceholder = "__unsafeExpression"; + + private static readonly SymbolDisplayFormat s_localTypeDisplayFormat = + SymbolDisplayFormat.MinimallyQualifiedFormat.WithMiscellaneousOptions( + SymbolDisplayFormat.MinimallyQualifiedFormat.MiscellaneousOptions + | SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers + | SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier + | SymbolDisplayMiscellaneousOptions.UseSpecialTypes); + + private static LocalizableString CodeFixTitle => + new LocalizableResourceString( + nameof(Resources.AddUnsafeContextCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => + [ + UnsafeOperationDiagnosticId, + UnsafeUninitializedStackAllocDiagnosticId, + UnsafeMemberOperationDiagnosticId, + UnsafeMemberOperationCompatDiagnosticId, + UnsafeConstructorConstraintDiagnosticId, + ]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + Diagnostic diagnostic = context.Diagnostics[0]; + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) + return; + + SyntaxNode targetNode = root.FindNode( + diagnostic.Location.SourceSpan, + getInnermostNodeForTie: true); + string title = CodeFixTitle.ToString(); + + // Attribute applications are deliberately not suppressible under the updated language rules. + if (targetNode.AncestorsAndSelf().OfType().Any()) + return; + + if (TryGetConstructorRequiringUnsafe(diagnostic, targetNode) is { } constructor) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + cancellationToken => UnsafeModifierCodeFixHelpers.SetUnsafeModifierAsync( + context.Document, + constructor, + cancellationToken), + title), + diagnostic); + return; + } + + if (diagnostic.Id == UnsafeConstructorConstraintDiagnosticId + && targetNode.AncestorsAndSelf().OfType().FirstOrDefault() is { } usingDirective) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(AddUnsafeToUsingDirective(context.Document, root, usingDirective)), + title), + diagnostic); + return; + } + + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + if (semanticModel is null) + return; + + if (IsExpressionOnlyContext(targetNode, diagnostic.Location.SourceSpan) + && TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } expressionOnly) + { + RegisterExpressionFix( + context, + diagnostic, + root, + semanticModel, + expressionOnly, + title); + return; + } + + ArrowExpressionClauseSyntax? arrowExpression = targetNode.AncestorsAndSelf() + .OfType() + .FirstOrDefault(arrow => arrow.Expression.FullSpan.Contains(diagnostic.Location.SourceSpan)); + if (arrowExpression is not null) + { + if (arrowExpression.Parent is AnonymousFunctionExpressionSyntax + || arrowExpression.Expression.DescendantNodesAndSelf().OfType().Any() + || HasDirectiveWithinSpan(arrowExpression)) + { + if (TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } expression) + { + RegisterExpressionFix( + context, + diagnostic, + root, + semanticModel, + expression, + title); + } + } + else + { + context.RegisterCodeFix( + CodeAction.Create( + title, + cancellationToken => ConvertExpressionBodyAsync( + context.Document, + root, + semanticModel, + arrowExpression, + cancellationToken), + title), + diagnostic); + } + return; + } + + StatementSyntax? containingStatement = targetNode.AncestorsAndSelf() + .OfType() + .FirstOrDefault(static statement => statement is not BlockSyntax); + while (containingStatement?.Parent is LabeledStatementSyntax labeledStatement) + containingStatement = labeledStatement; + + if (containingStatement is null) + { + if (TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } expression) + { + RegisterExpressionFix( + context, + diagnostic, + root, + semanticModel, + expression, + title); + } + return; + } + + if (containingStatement is LocalDeclarationStatementSyntax localDeclaration) + { + if (TryCreateSplitLocalDeclaration( + diagnostic, + localDeclaration, + semanticModel, + context.CancellationToken, + out LocalDeclarationStatementSyntax forwardDeclaration, + out StatementSyntax assignmentStatement)) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + localDeclaration, + [forwardDeclaration, CreateUnsafeStatement(assignmentStatement)])), + title), + diagnostic); + return; + } + + if (TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } localExpression + && CanUseUnsafeExpression(localExpression)) + { + RegisterExpressionFix( + context, + diagnostic, + root, + semanticModel, + localExpression, + title); + return; + } + + if (!localDeclaration.AwaitKeyword.IsKind(SyntaxKind.None)) + return; + + if (TryGetEnclosingSwitch(localDeclaration) is { } localSwitch) + { + if (!ContainsAwaitOrYield(localSwitch)) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + localSwitch, + [CreateUnsafeStatement(localSwitch)])), + title), + diagnostic); + } + return; + } + + if (TryGetStatementRangeToWrap( + localDeclaration, + semanticModel, + context.CancellationToken, + forceThroughContainerEnd: IsUsingDeclaration(localDeclaration), + out SyntaxNode localContainer, + out int localStart, + out int localEnd) + && CanWrapStatementRange( + localContainer, + localStart, + localEnd, + semanticModel, + context.CancellationToken)) + { + RegisterStatementRangeFix( + context, + diagnostic, + root, + localContainer, + localStart, + localEnd, + title); + } + return; + } + + bool cannotContainUnsafeStatement = containingStatement.DescendantNodesAndSelf() + .OfType() + .Any(); + bool topLevelScopeSensitive = containingStatement.Parent is GlobalStatementSyntax + && GetDeclaredNames(containingStatement).Count > 0; + bool containsAwaitStatement = containingStatement switch + { + CommonForEachStatementSyntax forEach => + !forEach.AwaitKeyword.IsKind(SyntaxKind.None), + UsingStatementSyntax usingStatement => + !usingStatement.AwaitKeyword.IsKind(SyntaxKind.None), + _ => false, + }; + bool preferExpression = containingStatement is YieldStatementSyntax + || containingStatement.DescendantNodesAndSelf().OfType().Any() + || containsAwaitStatement + || topLevelScopeSensitive + || HasDirectiveWithinSpan(containingStatement) + || WouldNarrowDeclaredVariableScope( + containingStatement, + semanticModel, + context.CancellationToken); + + if ((cannotContainUnsafeStatement || preferExpression) + && TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } statementExpression + && CanUseUnsafeExpression(statementExpression)) + { + RegisterExpressionFix( + context, + diagnostic, + root, + semanticModel, + statementExpression, + title); + return; + } + + if (topLevelScopeSensitive + && containingStatement.Parent is GlobalStatementSyntax + { + Parent: CompilationUnitSyntax compilationUnit, + } globalStatement) + { + GlobalStatementSyntax[] globalStatements = compilationUnit.Members + .OfType() + .ToArray(); + int start = System.Array.IndexOf(globalStatements, globalStatement); + int end = globalStatements.Length - 1; + if (start >= 0 + && CanWrapStatementRange( + compilationUnit, + start, + end, + semanticModel, + context.CancellationToken)) + { + RegisterStatementRangeFix( + context, + diagnostic, + root, + compilationUnit, + start, + end, + title); + } + return; + } + + if (cannotContainUnsafeStatement || containsAwaitStatement) + return; + + if (preferExpression && TryGetEnclosingSwitch(containingStatement) is { } containingSwitch) + { + if (!ContainsAwaitOrYield(containingSwitch)) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + containingSwitch, + [CreateUnsafeStatement(containingSwitch)])), + title), + diagnostic); + } + return; + } + + if (preferExpression) + { + if (TryGetStatementRangeToWrap( + containingStatement, + semanticModel, + context.CancellationToken, + forceThroughContainerEnd: topLevelScopeSensitive, + out SyntaxNode container, + out int start, + out int end) + && CanWrapStatementRange( + container, + start, + end, + semanticModel, + context.CancellationToken)) + { + RegisterStatementRangeFix(context, diagnostic, root, container, start, end, title); + } + return; + } + + if (TryGetStatementList( + containingStatement, + out SyntaxNode singleContainer, + out _, + out int singleIndex) + && !CanWrapStatementRange( + singleContainer, + singleIndex, + singleIndex, + semanticModel, + context.CancellationToken)) + { + return; + } + + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + containingStatement, + [CreateUnsafeStatement(containingStatement)])), + title), + diagnostic); + } + + private static void RegisterExpressionFix( + CodeFixContext context, + Diagnostic diagnostic, + SyntaxNode root, + SemanticModel semanticModel, + ExpressionSyntax expression, + string title) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(ReplaceWithUnsafeExpression( + context.Document, + root, + semanticModel, + expression, + context.CancellationToken)), + title), + diagnostic); + } + + private static void RegisterStatementRangeFix( + CodeFixContext context, + Diagnostic diagnostic, + SyntaxNode root, + SyntaxNode container, + int start, + int end, + string title) + { + context.RegisterCodeFix( + CodeAction.Create( + title, + _ => Task.FromResult(WrapStatementRange( + context.Document, + root, + container, + start, + end)), + title), + diagnostic); + } + + private static ConstructorDeclarationSyntax? TryGetConstructorRequiringUnsafe( + Diagnostic diagnostic, + SyntaxNode targetNode) + { + if (diagnostic.Id is not (UnsafeMemberOperationDiagnosticId or UnsafeMemberOperationCompatDiagnosticId)) + return null; + + if (targetNode.AncestorsAndSelf().OfType().FirstOrDefault() is { } initializer + && diagnostic.Location.SourceSpan.Contains(initializer.Span)) + { + return initializer.Parent as ConstructorDeclarationSyntax; + } + + if (targetNode.AncestorsAndSelf().OfType().FirstOrDefault() is { } constructor + && constructor.Initializer is null + && diagnostic.Location.SourceSpan.Contains(constructor.Span)) + { + return constructor; + } + + return null; + } + + private static bool IsExpressionOnlyContext(SyntaxNode targetNode, TextSpan diagnosticSpan) + { + if (targetNode.AncestorsAndSelf().OfType() + .Any(filter => filter.FilterExpression.FullSpan.Contains(diagnosticSpan))) + { + return true; + } + + if (targetNode.AncestorsAndSelf().OfType() + .Any(initializer => initializer.ArgumentList.FullSpan.Contains(diagnosticSpan))) + { + return true; + } + + return false; + } + + private static ExpressionSyntax? TryGetUnsafeExpression(SyntaxNode targetNode, TextSpan diagnosticSpan) => + PromoteUnsafeExpression( + targetNode.AncestorsAndSelf() + .OfType() + .FirstOrDefault(expression => expression.FullSpan.Contains(diagnosticSpan)), + diagnosticSpan); + + private static ExpressionSyntax? PromoteUnsafeExpression( + ExpressionSyntax? expression, + TextSpan diagnosticSpan) + { + if (expression is null) + return null; + + foreach (SyntaxNode ancestor in expression.AncestorsAndSelf()) + { + switch (ancestor) + { + case ConditionalAccessExpressionSyntax conditionalAccess + when conditionalAccess.WhenNotNull.FullSpan.Contains(diagnosticSpan): + expression = conditionalAccess; + break; + + case BaseObjectCreationExpressionSyntax objectCreation + when objectCreation.Initializer?.FullSpan.Contains(diagnosticSpan) == true: + expression = objectCreation; + break; + + case WithExpressionSyntax withExpression + when withExpression.Initializer.FullSpan.Contains(diagnosticSpan): + expression = withExpression; + break; + + case CollectionExpressionSyntax collectionExpression + when collectionExpression.FullSpan.Contains(diagnosticSpan): + expression = collectionExpression; + break; + } + } + + return expression; + } + + private static bool CanUseUnsafeExpression(ExpressionSyntax expression) => + !expression.DescendantNodesAndSelf().OfType().Any() + && (expression.Parent switch + { + ExpressionStatementSyntax { Expression: var statementExpression } + when statementExpression == expression => false, + ForStatementSyntax forStatement + when forStatement.Initializers.Contains(expression) + || forStatement.Incrementors.Contains(expression) => false, + AttributeArgumentSyntax => false, + _ => true, + }); + + private static bool TryCreateSplitLocalDeclaration( + Diagnostic diagnostic, + LocalDeclarationStatementSyntax localDeclaration, + SemanticModel semanticModel, + CancellationToken cancellationToken, + out LocalDeclarationStatementSyntax forwardDeclaration, + out StatementSyntax assignmentStatement) + { + forwardDeclaration = null!; + assignmentStatement = null!; + + if (diagnostic.Id == UnsafeConstructorConstraintDiagnosticId + || localDeclaration.IsConst + || IsUsingDeclaration(localDeclaration) + || localDeclaration.Parent is not BlockSyntax and not SwitchSectionSyntax + || HasDirectiveWithinSpan(localDeclaration) + || localDeclaration.Declaration.Variables.Count != 1 + || IsRefType(localDeclaration.Declaration.Type)) + { + return false; + } + + VariableDeclaratorSyntax variable = localDeclaration.Declaration.Variables[0]; + if (variable.Initializer is not { } initializer + || !initializer.Value.FullSpan.Contains(diagnostic.Location.SourceSpan) + || initializer.Value.DescendantNodesAndSelf().OfType().Any()) + { + return false; + } + + var localSymbol = semanticModel.GetDeclaredSymbol(variable, cancellationToken) as ILocalSymbol; + if (localSymbol is null || localSymbol.Type is IErrorTypeSymbol || ContainsAnonymousType(localSymbol.Type)) + return false; + + if (InitializerVariablesEscape( + localDeclaration, + initializer.Value, + localSymbol, + semanticModel, + cancellationToken)) + { + return false; + } + + TypeSyntax type = localDeclaration.Declaration.Type; + if (type.IsVar) + { + type = SyntaxFactory.ParseTypeName( + localSymbol.Type.ToMinimalDisplayString( + semanticModel, + localDeclaration.SpanStart, + s_localTypeDisplayFormat)); + } + + if (localSymbol.Type.IsRefLikeType) + { + if (!ContainsStackAllocThatFlowsToLocal(initializer.Value)) + return false; + + if (type is not ScopedTypeSyntax) + { + type = SyntaxFactory.ScopedType( + SyntaxFactory.Token(SyntaxKind.ScopedKeyword) + .WithTrailingTrivia(SyntaxFactory.Space), + type.WithoutLeadingTrivia()); + } + } + + VariableDeclaratorSyntax declarationVariable = variable.WithInitializer(null); + VariableDeclarationSyntax declaration = localDeclaration.Declaration + .WithType(type.WithTriviaFrom(localDeclaration.Declaration.Type)) + .WithVariables([declarationVariable]); + + forwardDeclaration = localDeclaration + .WithDeclaration(declaration) + .WithLeadingTrivia(localDeclaration.GetLeadingTrivia()) + .WithTrailingTrivia(SyntaxFactory.ElasticCarriageReturnLineFeed) + .WithAdditionalAnnotations(Formatter.Annotation); + + AssignmentExpressionSyntax assignment = SyntaxFactory.AssignmentExpression( + SyntaxKind.SimpleAssignmentExpression, + SyntaxFactory.IdentifierName(variable.Identifier.WithoutTrivia()), + initializer.EqualsToken, + initializer.Value.WithoutLeadingTrivia()); + assignmentStatement = SyntaxFactory.ExpressionStatement(assignment) + .WithTrailingTrivia(localDeclaration.GetTrailingTrivia()) + .WithAdditionalAnnotations(Formatter.Annotation); + return true; + } + + private static bool InitializerVariablesEscape( + LocalDeclarationStatementSyntax localDeclaration, + ExpressionSyntax initializer, + ILocalSymbol declaredLocal, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + DataFlowAnalysis? analysis = semanticModel.AnalyzeDataFlow(initializer); + if (analysis is null || !analysis.Succeeded) + return true; + + ISymbol[] initializerVariables = analysis.VariablesDeclared + .Where(symbol => !SymbolEqualityComparer.Default.Equals(symbol, declaredLocal)) + .ToArray(); + if (initializerVariables.Length == 0 + || !TryGetStatementList( + localDeclaration, + out SyntaxNode container, + out SyntaxList statements, + out int statementIndex)) + { + return false; + } + + IEnumerable laterStatements = Enumerable + .Range(statementIndex + 1, statements.Count - statementIndex - 1) + .Select(index => GetStatement(container, statements, index)); + if (localDeclaration.Parent is SwitchSectionSyntax switchSection + && switchSection.Parent is SwitchStatementSyntax switchStatement) + { + int sectionIndex = switchStatement.Sections.IndexOf(switchSection); + laterStatements = laterStatements.Concat( + switchStatement.Sections + .Skip(sectionIndex + 1) + .SelectMany(static section => section.Statements)); + } + + return laterStatements.Any(statement => ReferencesAnySymbol( + statement, + initializerVariables, + semanticModel, + cancellationToken)); + } + + private static bool IsRefType(TypeSyntax type) => + type is RefTypeSyntax + or ScopedTypeSyntax { Type: RefTypeSyntax }; + + private static bool ContainsAnonymousType(ITypeSymbol type) => + type switch + { + INamedTypeSymbol { IsAnonymousType: true } => true, + IArrayTypeSymbol array => ContainsAnonymousType(array.ElementType), + INamedTypeSymbol namedType => namedType.TypeArguments.Any(ContainsAnonymousType), + _ => false, + }; + + private static bool ContainsStackAllocThatFlowsToLocal(ExpressionSyntax expression) => + expression switch + { + StackAllocArrayCreationExpressionSyntax or ImplicitStackAllocArrayCreationExpressionSyntax => true, + ParenthesizedExpressionSyntax parenthesized => + ContainsStackAllocThatFlowsToLocal(parenthesized.Expression), + CastExpressionSyntax cast => + ContainsStackAllocThatFlowsToLocal(cast.Expression), + ConditionalExpressionSyntax conditional => + ContainsStackAllocThatFlowsToLocal(conditional.WhenTrue) + || ContainsStackAllocThatFlowsToLocal(conditional.WhenFalse), + SwitchExpressionSyntax switchExpression => + switchExpression.Arms.Any(static arm => ContainsStackAllocThatFlowsToLocal(arm.Expression)), + _ => false, + }; + + private static bool IsUsingDeclaration(LocalDeclarationStatementSyntax declaration) => + !declaration.UsingKeyword.IsKind(SyntaxKind.None); + + private static bool WouldNarrowDeclaredVariableScope( + StatementSyntax statement, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + if (!TryGetStatementList(statement, out _, out SyntaxList statements, out int statementIndex)) + return false; + + DataFlowAnalysis? analysis = semanticModel.AnalyzeDataFlow(statement); + if (analysis is null || !analysis.Succeeded) + return false; + + var declaredSymbols = new HashSet( + analysis.VariablesDeclared, + SymbolEqualityComparer.Default); + AddSyntacticallyDeclaredSymbols( + statement, + declaredSymbols, + semanticModel, + cancellationToken); + HashSet declaredNames = GetDeclaredNames(statement); + if (declaredSymbols.Count == 0 && declaredNames.Count == 0) + return false; + + IEnumerable laterStatements = statements.Skip(statementIndex + 1); + if (statement.Parent is SwitchSectionSyntax switchSection + && switchSection.Parent is SwitchStatementSyntax switchStatement) + { + int sectionIndex = switchStatement.Sections.IndexOf(switchSection); + laterStatements = laterStatements.Concat( + switchStatement.Sections + .Skip(sectionIndex + 1) + .SelectMany(static section => section.Statements)); + } + + return laterStatements.Any(laterStatement => + ReferencesAnySymbol( + laterStatement, + declaredSymbols, + semanticModel, + cancellationToken) + || ReferencesAnyName(laterStatement, declaredNames)); + } + + private static bool TryGetStatementRangeToWrap( + StatementSyntax triggerStatement, + SemanticModel semanticModel, + CancellationToken cancellationToken, + bool forceThroughContainerEnd, + out SyntaxNode container, + out int start, + out int end) + { + if (!TryGetStatementList(triggerStatement, out container, out SyntaxList statements, out start)) + { + end = -1; + return false; + } + + end = forceThroughContainerEnd ? statements.Count - 1 : start; + if (forceThroughContainerEnd) + return true; + + while (true) + { + HashSet? declaredSymbols = GetDeclaredSymbols( + container, + statements, + start, + end, + semanticModel, + cancellationToken); + if (declaredSymbols is null) + { + end = statements.Count - 1; + return true; + } + + if (statements.Skip(start).Take(end - start + 1).OfType() + .Any(IsUsingDeclaration)) + { + end = statements.Count - 1; + return true; + } + + int expandedEnd = end; + var selectedStatements = new List(end - start + 1); + for (int i = start; i <= end; i++) + selectedStatements.Add(GetStatement(container, statements, i)); + HashSet declaredNames = GetDeclaredNames(selectedStatements); + for (int i = end + 1; i < statements.Count; i++) + { + if (ReferencesAnySymbol( + GetStatement(container, statements, i), + declaredSymbols, + semanticModel, + cancellationToken) + || ReferencesAnyName( + GetStatement(container, statements, i), + declaredNames)) + { + expandedEnd = i; + } + } + + if (expandedEnd == end) + return true; + + end = expandedEnd; + } + } + + private static bool TryGetStatementList( + StatementSyntax statement, + out SyntaxNode container, + out SyntaxList statements, + out int statementIndex) + { + switch (statement.Parent) + { + case BlockSyntax block: + container = block; + statements = block.Statements; + statementIndex = statements.IndexOf(statement); + return statementIndex >= 0; + + case SwitchSectionSyntax switchSection: + container = switchSection; + statements = switchSection.Statements; + statementIndex = statements.IndexOf(statement); + return statementIndex >= 0; + + case GlobalStatementSyntax + { + Parent: CompilationUnitSyntax compilationUnit, + }: + container = compilationUnit; + statements = SyntaxFactory.List( + compilationUnit.Members + .OfType() + .Select(static globalStatement => globalStatement.Statement)); + statementIndex = statements.IndexOf(statement); + return statementIndex >= 0; + + default: + container = null!; + statements = default; + statementIndex = -1; + return false; + } + } + + private static HashSet? GetDeclaredSymbols( + SyntaxNode container, + SyntaxList statements, + int start, + int end, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + var symbols = new HashSet(SymbolEqualityComparer.Default); + if (container is CompilationUnitSyntax) + { + for (int i = start; i <= end; i++) + { + StatementSyntax statement = GetStatement(container, statements, i); + DataFlowAnalysis? statementAnalysis = semanticModel.AnalyzeDataFlow(statement); + if (statementAnalysis is null || !statementAnalysis.Succeeded) + return null; + + symbols.UnionWith(statementAnalysis.VariablesDeclared); + AddSyntacticallyDeclaredSymbols( + statement, + symbols, + semanticModel, + cancellationToken); + } + + return symbols; + } + + DataFlowAnalysis? analysis = semanticModel.AnalyzeDataFlow(statements[start], statements[end]); + if (analysis is null || !analysis.Succeeded) + return null; + + symbols.UnionWith(analysis.VariablesDeclared); + for (int i = start; i <= end; i++) + { + AddSyntacticallyDeclaredSymbols( + GetStatement(container, statements, i), + symbols, + semanticModel, + cancellationToken); + } + return symbols; + } + + private static void AddSyntacticallyDeclaredSymbols( + SyntaxNode node, + HashSet symbols, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + foreach (VariableDeclaratorSyntax variable in + node.DescendantNodesAndSelf().OfType()) + { + if (semanticModel.GetDeclaredSymbol(variable, cancellationToken) is { } symbol) + symbols.Add(symbol); + } + + foreach (SingleVariableDesignationSyntax designation in + node.DescendantNodesAndSelf().OfType()) + { + if (semanticModel.GetDeclaredSymbol(designation, cancellationToken) is { } symbol) + symbols.Add(symbol); + } + } + + private static bool ReferencesAnySymbol( + SyntaxNode node, + IEnumerable symbols, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + var symbolSet = new HashSet(symbols, SymbolEqualityComparer.Default); + if (symbolSet.Count == 0) + return false; + + return node.DescendantNodesAndSelf() + .OfType() + .Any(name => + semanticModel.GetSymbolInfo(name, cancellationToken).Symbol is { } symbol + && symbolSet.Contains(symbol)); + } + + private static HashSet GetDeclaredNames(SyntaxNode node) => + GetDeclaredNames([node]); + + private static HashSet GetDeclaredNames(IEnumerable nodes) + { + var names = new HashSet(); + foreach (SyntaxNode node in nodes) + { + foreach (VariableDeclaratorSyntax variable in + node.DescendantNodesAndSelf().OfType()) + { + names.Add(variable.Identifier.ValueText); + } + + foreach (SingleVariableDesignationSyntax designation in + node.DescendantNodesAndSelf().OfType()) + { + names.Add(designation.Identifier.ValueText); + } + } + + return names; + } + + private static bool ReferencesAnyName( + SyntaxNode node, + HashSet names) => + names.Count > 0 + && node.DescendantNodesAndSelf() + .OfType() + .Any(name => names.Contains(name.Identifier.ValueText)); + + private static bool HasDirectiveWithinSpan(SyntaxNode node) => + node.DescendantTrivia(descendIntoTrivia: true) + .Any(trivia => trivia.IsDirective + && node.Span.Contains(trivia.SpanStart)); + + private static Document AddUnsafeToUsingDirective( + Document document, + SyntaxNode root, + UsingDirectiveSyntax usingDirective) + { + SyntaxToken unsafeKeyword = SyntaxFactory.Token(SyntaxKind.UnsafeKeyword) + .WithTrailingTrivia(SyntaxFactory.Space); + UsingDirectiveSyntax replacement = usingDirective + .WithUnsafeKeyword(unsafeKeyword) + .WithAdditionalAnnotations(Formatter.Annotation); + return document.WithSyntaxRoot(root.ReplaceNode(usingDirective, replacement)); + } + + private static Document ReplaceWithUnsafeExpression( + Document document, + SyntaxNode root, + SemanticModel semanticModel, + ExpressionSyntax expression, + CancellationToken cancellationToken) + { + ExpressionSyntax operand = expression.WithoutLeadingTrivia().WithoutTrailingTrivia(); + Conversion conversion = semanticModel.GetConversion(expression, cancellationToken); + if (conversion.IsUserDefined + && conversion.IsImplicit + && conversion.MethodSymbol is { } conversionOperator) + { + ITypeSymbol convertedType = GetUserDefinedConversionResultType( + semanticModel, + expression, + conversionOperator, + cancellationToken); + TypeSyntax convertedTypeSyntax = SyntaxFactory.ParseTypeName( + convertedType.ToMinimalDisplayString( + semanticModel, + expression.SpanStart, + s_localTypeDisplayFormat)); + operand = SyntaxFactory.CastExpression(convertedTypeSyntax, operand); + } + + ExpressionSyntax template = SyntaxFactory.ParseExpression( + $"unsafe(/* SAFETY: Audit */ {UnsafeExpressionPlaceholder})"); + IdentifierNameSyntax placeholder = template.DescendantNodesAndSelf() + .OfType() + .Single(identifier => identifier.Identifier.ValueText == UnsafeExpressionPlaceholder); + + ExpressionSyntax replacement = template + .ReplaceNode( + placeholder, + operand) + .WithLeadingTrivia(expression.GetLeadingTrivia()) + .WithTrailingTrivia(expression.GetTrailingTrivia()); + return document.WithSyntaxRoot(root.ReplaceNode(expression, replacement)); + } + + private static ITypeSymbol GetUserDefinedConversionResultType( + SemanticModel semanticModel, + ExpressionSyntax expression, + IMethodSymbol conversionOperator, + CancellationToken cancellationToken) + { + ITypeSymbol resultType = conversionOperator.ReturnType; + ITypeSymbol? sourceType = semanticModel.GetTypeInfo(expression, cancellationToken).Type; + if (sourceType is INamedTypeSymbol sourceNamedType + && sourceNamedType.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T + && sourceNamedType.TypeArguments.Length == 1 + && conversionOperator.Parameters.Length == 1 + && SymbolEqualityComparer.Default.Equals( + sourceNamedType.TypeArguments[0], + conversionOperator.Parameters[0].Type) + && resultType.IsValueType + && resultType.OriginalDefinition.SpecialType != SpecialType.System_Nullable_T) + { + INamedTypeSymbol nullableType = semanticModel.Compilation.GetSpecialType( + SpecialType.System_Nullable_T); + if (nullableType.TypeKind != TypeKind.Error) + resultType = nullableType.Construct(resultType); + } + + return resultType; + } + + private static bool CanWrapStatementRange( + SyntaxNode container, + int start, + int end, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + SyntaxList statements = container switch + { + BlockSyntax block => block.Statements, + SwitchSectionSyntax switchSection => switchSection.Statements, + CompilationUnitSyntax compilationUnit => SyntaxFactory.List( + compilationUnit.Members + .OfType() + .Select(static globalStatement => globalStatement.Statement)), + _ => default, + }; + IEnumerable selectedStatements = statements + .Select((statement, index) => GetStatement(container, statements, index)) + .Skip(start) + .Take(end - start + 1) + .ToArray(); + + if (selectedStatements.Any(ContainsAwaitOrYield)) + return false; + + if (end > start && selectedStatements.Any(static statement => statement.ContainsDirectives)) + return false; + + ISymbol[] localFunctions = selectedStatements + .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) + .Select(localFunction => semanticModel.GetDeclaredSymbol(localFunction, cancellationToken)) + .OfType() + .ToArray(); + if (localFunctions.Length > 0 + && statements.Where((_, index) => index < start || index > end) + .Any(statement => ReferencesAnySymbol( + statement, + localFunctions, + semanticModel, + cancellationToken))) + { + return false; + } + + string[] labels = selectedStatements + .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) + .Select(static label => label.Identifier.ValueText) + .Distinct() + .ToArray(); + if (labels.Length > 0 + && statements.Where((_, index) => index < start || index > end) + .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) + .Any(gotoStatement => + gotoStatement.Expression is IdentifierNameSyntax identifier + && labels.Contains(identifier.Identifier.ValueText))) + { + return false; + } + + return true; + } + + private static StatementSyntax GetStatement( + SyntaxNode container, + SyntaxList statements, + int index) => + container is CompilationUnitSyntax compilationUnit + ? compilationUnit.Members + .OfType() + .ElementAt(index) + .Statement + : statements[index]; + + private static SwitchStatementSyntax? TryGetEnclosingSwitch(StatementSyntax statement) => + statement.AncestorsAndSelf() + .OfType() + .Select(static section => section.Parent) + .OfType() + .FirstOrDefault(); + + private static bool ContainsAwaitOrYield(SyntaxNode node) => + node.DescendantNodesAndSelf().Any(static descendant => + descendant is AwaitExpressionSyntax + or YieldStatementSyntax + or CommonForEachStatementSyntax { AwaitKeyword.RawKind: not 0 } + or UsingStatementSyntax { AwaitKeyword.RawKind: not 0 } + or LocalDeclarationStatementSyntax { AwaitKeyword.RawKind: not 0 }); + + private static Task ConvertExpressionBodyAsync( + Document document, + SyntaxNode root, + SemanticModel semanticModel, + ArrowExpressionClauseSyntax arrowExpression, + CancellationToken cancellationToken) + { + SyntaxNode declaration = arrowExpression.Parent!; + ExpressionSyntax expression = arrowExpression.Expression; + StatementSyntax statement; + + if (expression is ThrowExpressionSyntax throwExpression) + { + statement = SyntaxFactory.ThrowStatement(throwExpression.Expression); + } + else + { + ISymbol? symbol = declaration switch + { + MemberDeclarationSyntax member => semanticModel.GetDeclaredSymbol(member, cancellationToken), + LocalFunctionStatementSyntax localFunction => semanticModel.GetDeclaredSymbol(localFunction, cancellationToken), + AccessorDeclarationSyntax accessor => semanticModel.GetDeclaredSymbol(accessor, cancellationToken), + _ => null, + }; + ITypeSymbol? expressionType = semanticModel.GetTypeInfo(expression, cancellationToken).Type; + bool isVoidExpression = symbol is IMethodSymbol { ReturnsVoid: true } + || expressionType?.SpecialType == SpecialType.System_Void + || symbol is IMethodSymbol asyncMethod + && IsResultlessAsyncMethod(asyncMethod); + statement = isVoidExpression + ? SyntaxFactory.ExpressionStatement(expression) + : SyntaxFactory.ReturnStatement(expression); + } + + UnsafeStatementSyntax unsafeStatement = CreateUnsafeStatement(statement); + BlockSyntax body = SyntaxFactory.Block(unsafeStatement) + .WithAdditionalAnnotations(Formatter.Annotation); + + SyntaxNode replacement = declaration switch + { + MethodDeclarationSyntax method => method + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + ConstructorDeclarationSyntax constructor => constructor + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + DestructorDeclarationSyntax destructor => destructor + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + OperatorDeclarationSyntax @operator => @operator + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + ConversionOperatorDeclarationSyntax conversion => conversion + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + LocalFunctionStatementSyntax localFunction => localFunction + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + PropertyDeclarationSyntax property => property + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithAccessorList(CreateGetterAccessorList(unsafeStatement)), + IndexerDeclarationSyntax indexer => indexer + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithAccessorList(CreateGetterAccessorList(unsafeStatement)), + AccessorDeclarationSyntax accessor => accessor + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithBody(body), + _ => declaration, + }; + + return Task.FromResult(document.WithSyntaxRoot(root.ReplaceNode( + declaration, + replacement.WithAdditionalAnnotations(Formatter.Annotation)))); + } + + private static bool IsResultlessAsyncMethod(IMethodSymbol method) + { + if (!method.IsAsync) + return false; + + if (method.ReturnsVoid) + return true; + + if (method.ReturnType is INamedTypeSymbol + { + Arity: 0, + Name: "Task" or "ValueTask", + } taskType + && taskType.ContainingNamespace.ToDisplayString() == "System.Threading.Tasks") + { + return true; + } + + foreach (AttributeData attribute in method.ReturnType.GetAttributes()) + { + if (attribute.AttributeClass?.ToDisplayString() + != "System.Runtime.CompilerServices.AsyncMethodBuilderAttribute" + || attribute.ConstructorArguments.Length != 1 + || attribute.ConstructorArguments[0].Value is not INamedTypeSymbol builderType) + { + continue; + } + + return builderType.GetMembers("SetResult") + .OfType() + .Any(static setResult => setResult.Parameters.IsEmpty); + } + + return false; + } + + private static AccessorListSyntax CreateGetterAccessorList(UnsafeStatementSyntax unsafeStatement) => + SyntaxFactory.AccessorList( + SyntaxFactory.SingletonList( + SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration) + .WithBody(SyntaxFactory.Block(unsafeStatement)))); + + private static UnsafeStatementSyntax CreateUnsafeStatement(StatementSyntax statement) + { + StatementSyntax innerStatement = statement + .WithoutLeadingTrivia() + .WithoutTrailingTrivia() + .WithLeadingTrivia( + SyntaxFactory.Comment("// SAFETY: Audit"), + SyntaxFactory.ElasticCarriageReturnLineFeed); + return SyntaxFactory.UnsafeStatement(SyntaxFactory.Block(innerStatement)) + .WithLeadingTrivia(statement.GetLeadingTrivia()) + .WithTrailingTrivia(statement.GetTrailingTrivia()) + .WithAdditionalAnnotations(Formatter.Annotation); + } + + private static Document ReplaceStatementWithStatements( + Document document, + SyntaxNode root, + StatementSyntax oldStatement, + IReadOnlyList replacements) + { + switch (oldStatement.Parent) + { + case BlockSyntax block: + { + int index = block.Statements.IndexOf(oldStatement); + SyntaxList statements = block.Statements + .RemoveAt(index) + .InsertRange(index, replacements); + return document.WithSyntaxRoot(root.ReplaceNode( + block, + block.WithStatements(statements))); + } + + case SwitchSectionSyntax switchSection: + { + int index = switchSection.Statements.IndexOf(oldStatement); + SyntaxList statements = switchSection.Statements + .RemoveAt(index) + .InsertRange(index, replacements); + return document.WithSyntaxRoot(root.ReplaceNode( + switchSection, + switchSection.WithStatements(statements))); + } + + default: + return document.WithSyntaxRoot(root.ReplaceNode( + oldStatement, + replacements.Count == 1 + ? replacements[0] + : SyntaxFactory.Block(replacements) + .WithAdditionalAnnotations(Formatter.Annotation))); + } + } + + private static Document WrapStatementRange( + Document document, + SyntaxNode root, + SyntaxNode container, + int start, + int end) + { + SyntaxList statements = container switch + { + BlockSyntax block => block.Statements, + SwitchSectionSyntax switchSection => switchSection.Statements, + CompilationUnitSyntax compilationUnit => SyntaxFactory.List( + compilationUnit.Members + .OfType() + .Select(static globalStatement => globalStatement.Statement)), + _ => default, + }; + StatementSyntax[] statementsToWrap = statements + .Skip(start) + .Take(end - start + 1) + .ToArray(); + + StatementSyntax firstStatement = statementsToWrap[0]; + StatementSyntax firstInnerStatement = firstStatement + .WithoutLeadingTrivia() + .WithLeadingTrivia( + SyntaxFactory.Comment("// SAFETY: Audit"), + SyntaxFactory.ElasticCarriageReturnLineFeed); + statementsToWrap[0] = firstInnerStatement; + + UnsafeStatementSyntax unsafeStatement = SyntaxFactory.UnsafeStatement( + SyntaxFactory.Block(statementsToWrap)) + .WithLeadingTrivia(firstStatement.GetLeadingTrivia()) + .WithAdditionalAnnotations(Formatter.Annotation); + + var replacementList = new List(statements.Count - (end - start)); + for (int i = 0; i < statements.Count; i++) + { + if (i == start) + replacementList.Add(unsafeStatement); + if (i < start || i > end) + replacementList.Add(statements[i]); + } + SyntaxList replacementStatements = SyntaxFactory.List(replacementList); + SyntaxNode replacementContainer = container switch + { + BlockSyntax block => block.WithStatements(replacementStatements), + SwitchSectionSyntax switchSection => switchSection.WithStatements(replacementStatements), + CompilationUnitSyntax compilationUnit => ReplaceGlobalStatements( + compilationUnit, + start, + end, + unsafeStatement), + _ => container, + }; + return document.WithSyntaxRoot(root.ReplaceNode(container, replacementContainer)); + } + + private static CompilationUnitSyntax ReplaceGlobalStatements( + CompilationUnitSyntax compilationUnit, + int start, + int end, + UnsafeStatementSyntax unsafeStatement) + { + GlobalStatementSyntax[] globalStatements = compilationUnit.Members + .OfType() + .ToArray(); + int memberIndex = compilationUnit.Members.IndexOf(globalStatements[start]); + SyntaxList members = compilationUnit.Members; + for (int i = start; i <= end; i++) + members = members.RemoveAt(memberIndex); + + return compilationUnit.WithMembers(members.Insert( + memberIndex, + SyntaxFactory.GlobalStatement(unsafeStatement))); + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj b/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj index 441acde0ff83a8..1556cc90ef1c8d 100644 --- a/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj +++ b/src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj @@ -14,6 +14,7 @@ + diff --git a/src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs deleted file mode 100644 index fd9c7d22ad55cd..00000000000000 --- a/src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs +++ /dev/null @@ -1,574 +0,0 @@ -// 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 ILLink.CodeFixProvider; -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 -{ - [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(RequiresUnsafeCodeFixProvider)), Shared] - public sealed class RequiresUnsafeCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider - { - private const string WrapInUnsafeBlockTitle = "Wrap in unsafe block"; - - public const string UnsafeMemberOperationDiagnosticId = "CS9362"; - - public sealed override ImmutableArray FixableDiagnosticIds => [UnsafeMemberOperationDiagnosticId]; - - private static LocalizableString CodeFixTitle => new LocalizableResourceString(nameof(Resources.RequiresUnsafeCodeFixTitle), Resources.ResourceManager, typeof(Resources)); - - public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) - { - var document = context.Document; - var diagnostic = context.Diagnostics.First(); - var codeFixTitle = CodeFixTitle.ToString(); - - if (await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false) is not { } root) - return; - - SyntaxNode targetNode = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); - - // Register "add unsafe modifier" code fix - var unsafeModifierTarget = GetUnsafeModifierTarget(targetNode); - if (unsafeModifierTarget is not null && !HasUnsafeModifier(unsafeModifierTarget)) - { - context.RegisterCodeFix(CodeAction.Create( - title: codeFixTitle, - createChangedDocument: ct => AddUnsafeModifierAsync(document, unsafeModifierTarget, ct), - equivalenceKey: codeFixTitle), diagnostic); - } - - // Register the "wrap in unsafe block" code fix - - // Find the statement containing the unsafe call - var containingStatement = targetNode.AncestorsAndSelf().OfType().FirstOrDefault(); - - // Check if this is a local function with expression body - treat it like expression-bodied member - if (containingStatement is LocalFunctionStatementSyntax localFunc && localFunc.ExpressionBody != null) - { - if (!HasDirectiveTrivia(localFunc.ExpressionBody)) - { - context.RegisterCodeFix(CodeAction.Create( - title: WrapInUnsafeBlockTitle, - createChangedDocument: ct => ConvertExpressionBodyToUnsafeBlockAsync(document, localFunc.ExpressionBody, ct), - equivalenceKey: WrapInUnsafeBlockTitle), diagnostic); - } - return; - } - - if (containingStatement is null || containingStatement is BlockSyntax) - { - // Try expression-bodied member - var arrowExpr = targetNode.AncestorsAndSelf().OfType().FirstOrDefault(); - if (arrowExpr != null && !HasDirectiveTrivia(arrowExpr)) - { - context.RegisterCodeFix(CodeAction.Create( - title: WrapInUnsafeBlockTitle, - createChangedDocument: ct => ConvertExpressionBodyToUnsafeBlockAsync(document, arrowExpr, ct), - equivalenceKey: WrapInUnsafeBlockTitle), diagnostic); - } - return; - } - - // Find the parent block containing this statement - var parentBlock = containingStatement.Parent as BlockSyntax; - if (parentBlock != null) - { - context.RegisterCodeFix(CodeAction.Create( - title: WrapInUnsafeBlockTitle, - createChangedDocument: ct => WrapStatementsInUnsafeBlockAsync(document, parentBlock, containingStatement, ct), - equivalenceKey: WrapInUnsafeBlockTitle), diagnostic); - return; - } - - // Handle switch case sections - var switchSection = containingStatement.Parent as SwitchSectionSyntax; - if (switchSection != null) - { - context.RegisterCodeFix(CodeAction.Create( - title: WrapInUnsafeBlockTitle, - createChangedDocument: ct => WrapSwitchSectionStatementInUnsafeBlockAsync(document, switchSection, containingStatement, ct), - equivalenceKey: WrapInUnsafeBlockTitle), diagnostic); - return; - } - - // Handle embedded statements (if/else/while/for without braces) - if (IsEmbeddedStatement(containingStatement)) - { - context.RegisterCodeFix(CodeAction.Create( - title: WrapInUnsafeBlockTitle, - createChangedDocument: ct => WrapEmbeddedStatementInUnsafeBlockAsync(document, containingStatement, ct), - equivalenceKey: WrapInUnsafeBlockTitle), diagnostic); - return; - } - } - - private static bool IsEmbeddedStatement(StatementSyntax statement) - { - // An embedded statement is a statement that is the direct child of a control flow statement - // without being wrapped in a block (e.g., "if (x) return;" instead of "if (x) { return; }") - return statement.Parent is IfStatementSyntax - || statement.Parent is ElseClauseSyntax - || statement.Parent is WhileStatementSyntax - || statement.Parent is ForStatementSyntax - || statement.Parent is ForEachStatementSyntax - || statement.Parent is DoStatementSyntax - || statement.Parent is UsingStatementSyntax - || statement.Parent is LockStatementSyntax - || statement.Parent is FixedStatementSyntax; - } - - private static async Task AddUnsafeModifierAsync( - Document document, - SyntaxNode declaration, - CancellationToken cancellationToken) - { - var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); - var modifiers = editor.Generator.GetModifiers(declaration); - editor.SetModifiers(declaration, modifiers.WithIsUnsafe(true)); - return editor.GetChangedDocument(); - } - - private static SyntaxNode? GetUnsafeModifierTarget(SyntaxNode targetNode) - => targetNode.AncestorsAndSelf().FirstOrDefault(static node => node is MethodDeclarationSyntax - or ConstructorDeclarationSyntax - or DestructorDeclarationSyntax - or LocalFunctionStatementSyntax - or PropertyDeclarationSyntax - or IndexerDeclarationSyntax - or AccessorDeclarationSyntax); - - private static bool HasUnsafeModifier(SyntaxNode declaration) - => declaration switch - { - MethodDeclarationSyntax method => method.Modifiers.Any(SyntaxKind.UnsafeKeyword), - ConstructorDeclarationSyntax constructor => constructor.Modifiers.Any(SyntaxKind.UnsafeKeyword), - DestructorDeclarationSyntax destructor => destructor.Modifiers.Any(SyntaxKind.UnsafeKeyword), - LocalFunctionStatementSyntax localFunction => localFunction.Modifiers.Any(SyntaxKind.UnsafeKeyword), - PropertyDeclarationSyntax property => property.Modifiers.Any(SyntaxKind.UnsafeKeyword), - IndexerDeclarationSyntax indexer => indexer.Modifiers.Any(SyntaxKind.UnsafeKeyword), - AccessorDeclarationSyntax accessor => accessor.Modifiers.Any(SyntaxKind.UnsafeKeyword), - _ => false - }; - - private static async Task WrapStatementsInUnsafeBlockAsync( - Document document, - BlockSyntax parentBlock, - StatementSyntax triggerStatement, - CancellationToken cancellationToken) - { - var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); - if (semanticModel == null) - return document; - - // Find the trigger statement in the block - var statements = parentBlock.Statements; - int triggerIndex = statements.IndexOf(triggerStatement); - if (triggerIndex < 0) - return document; - - // Check if any statement has directive trivia that would be lost or mangled - var leadingTrivia = triggerStatement.GetLeadingTrivia(); - if (leadingTrivia.Any(t => t.IsDirective)) - { - // Skip the fix - directives in statement trivia would be lost or malformed - return document; - } - - var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); - - // Create the TODO comment - var todoComment = SyntaxFactory.Comment("// TODO(unsafe): Baselining unsafe usage"); - var newLine = SyntaxFactory.ElasticCarriageReturnLineFeed; - - // Check if we can use forward declaration strategy - // This applies when the trigger is a local declaration with a single variable - // Ref locals (including ref readonly and scoped ref) cannot be forward-declared - LocalDeclarationStatementSyntax? forwardDecl = null; - StatementSyntax statementToWrap = triggerStatement; - int endIndex = triggerIndex; // For block expansion when forward decl not possible - - bool isRefOrScopedLocal = triggerStatement is LocalDeclarationStatementSyntax localDeclCheck && - (localDeclCheck.Declaration.Type is RefTypeSyntax || localDeclCheck.Declaration.Type is ScopedTypeSyntax); - - if (triggerStatement is LocalDeclarationStatementSyntax localDecl && - !localDecl.IsConst && - !isRefOrScopedLocal && - localDecl.Declaration.Variables.Count == 1 && - localDecl.Declaration.Variables[0].Initializer != null) - { - var variable = localDecl.Declaration.Variables[0]; - TypeSyntax? typeSyntax = localDecl.Declaration.Type; - - // If using 'var', resolve to explicit type - if (typeSyntax.IsVar) - { - var typeInfo = semanticModel.GetTypeInfo(typeSyntax, cancellationToken); - if (typeInfo.Type is not null and not IErrorTypeSymbol) - { - typeSyntax = SyntaxFactory.ParseTypeName(typeInfo.Type.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat)) - .WithTrailingTrivia(SyntaxFactory.Space); - } - else - { - // Can't resolve type, fall back to wrapping the whole declaration - typeSyntax = null; - } - } - - if (typeSyntax != null) - { - // Create forward declaration: Type varName; - var forwardDeclVariable = SyntaxFactory.VariableDeclarator(variable.Identifier); - var forwardDeclDeclaration = SyntaxFactory.VariableDeclaration(typeSyntax) - .AddVariables(forwardDeclVariable); - forwardDecl = SyntaxFactory.LocalDeclarationStatement(forwardDeclDeclaration) - .WithLeadingTrivia(triggerStatement.GetLeadingTrivia()) - .WithTrailingTrivia(SyntaxFactory.TriviaList(SyntaxFactory.ElasticCarriageReturnLineFeed)); - - // Create assignment: varName = initializer; - var assignment = SyntaxFactory.AssignmentExpression( - SyntaxKind.SimpleAssignmentExpression, - SyntaxFactory.IdentifierName(variable.Identifier), - variable.Initializer!.Value); - statementToWrap = SyntaxFactory.ExpressionStatement(assignment) - .WithTrailingTrivia(SyntaxFactory.TriviaList(SyntaxFactory.ElasticCarriageReturnLineFeed)); - } - } - else if (isRefOrScopedLocal) - { - // For ref/scoped locals, we must expand the block until no variables - // declared inside are used outside. This handles chains of dependencies - // where ref locals lead to regular locals that are also used later. - var localDeclStmt = (LocalDeclarationStatementSyntax)triggerStatement; - if (localDeclStmt.Declaration.Variables.Count == 1) - { - // Iteratively expand until no variables declared inside escape outside - bool expanded = true; - while (expanded && endIndex < statements.Count - 1) - { - expanded = false; - - // Collect all variables declared in the current range (using symbols for correct scoping) - var declaredVariableSymbols = new HashSet(SymbolEqualityComparer.Default); - for (int i = triggerIndex; i <= endIndex; i++) - { - if (statements[i] is LocalDeclarationStatementSyntax rangeLocalDecl) - { - foreach (var variable in rangeLocalDecl.Declaration.Variables) - { - var symbol = semanticModel.GetDeclaredSymbol(variable, cancellationToken); - if (symbol is not null) - declaredVariableSymbols.Add(symbol); - } - } - } - - // Check ALL remaining statements - does any use a declared variable? - for (int nextIndex = endIndex + 1; nextIndex < statements.Count; nextIndex++) - { - var stmt = statements[nextIndex]; - - bool usesAnyDeclaredVariable = stmt.DescendantNodes() - .OfType() - .Any(id => - { - var symbolInfo = semanticModel.GetSymbolInfo(id, cancellationToken); - return symbolInfo.Symbol is not null && declaredVariableSymbols.Contains(symbolInfo.Symbol); - }); - - if (usesAnyDeclaredVariable) - { - // Check for directive trivia that would be mangled - if (stmt.GetLeadingTrivia().Any(t => t.IsDirective)) - { - // Stop expansion here - don't include statements with directives - break; - } - - // Expand to include this statement - endIndex = nextIndex; - expanded = true; - break; // Restart the outer loop to recollect variables - } - } - } - } - } - - // Build the statements to wrap - List statementsToWrap; - if (forwardDecl != null) - { - statementsToWrap = new List { statementToWrap }; - } - else - { - statementsToWrap = statements.Skip(triggerIndex).Take(endIndex - triggerIndex + 1).ToList(); - } - - // Create the unsafe block - var wrappedStatements = statementsToWrap.Select(s => - s.WithoutTrivia() - .WithTrailingTrivia(SyntaxFactory.TriviaList(SyntaxFactory.ElasticCarriageReturnLineFeed))); - var unsafeBlock = SyntaxFactory.UnsafeStatement( - SyntaxFactory.Block(wrappedStatements)) - .WithLeadingTrivia(forwardDecl != null - ? SyntaxFactory.TriviaList(todoComment, newLine) - : triggerStatement.GetLeadingTrivia().InsertRange(0, new[] { todoComment, newLine })) - .WithTrailingTrivia(forwardDecl != null - ? triggerStatement.GetTrailingTrivia() - : statementsToWrap.Last().GetTrailingTrivia()); - - // Build the new list of statements - var newStatements = new List(); - for (int i = 0; i < statements.Count; i++) - { - if (i == triggerIndex) - { - if (forwardDecl != null) - { - newStatements.Add(forwardDecl); - } - newStatements.Add(unsafeBlock); - } - else if (i > triggerIndex && i <= endIndex) - { - // Skip - already included in unsafe block - } - else - { - newStatements.Add(statements[i]); - } - } - - var newBlock = parentBlock.WithStatements(SyntaxFactory.List(newStatements)); - editor.ReplaceNode(parentBlock, newBlock); - - return editor.GetChangedDocument(); - } - - private static async Task WrapSwitchSectionStatementInUnsafeBlockAsync( - Document document, - SwitchSectionSyntax _, - StatementSyntax triggerStatement, - CancellationToken cancellationToken) - { - var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); - - // Create the TODO comment - var todoComment = SyntaxFactory.Comment("// TODO(unsafe): Baselining unsafe usage"); - var newLine = SyntaxFactory.CarriageReturnLineFeed; - - // Create the unsafe block wrapping just the trigger statement - var unsafeBlock = SyntaxFactory.UnsafeStatement( - SyntaxFactory.Block(triggerStatement.WithoutTrivia())) - .WithLeadingTrivia(triggerStatement.GetLeadingTrivia().InsertRange(0, new[] { todoComment, newLine })) - .WithTrailingTrivia(triggerStatement.GetTrailingTrivia()); - - // Replace the trigger statement with the unsafe block - editor.ReplaceNode(triggerStatement, unsafeBlock); - - return editor.GetChangedDocument(); - } - - private static async Task WrapEmbeddedStatementInUnsafeBlockAsync( - Document document, - StatementSyntax embeddedStatement, - CancellationToken cancellationToken) - { - var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); - - // Create the TODO comment - var todoComment = SyntaxFactory.Comment("// TODO(unsafe): Baselining unsafe usage"); - var newLine = SyntaxFactory.CarriageReturnLineFeed; - - // Create the unsafe block wrapping the statement - var unsafeBlock = SyntaxFactory.UnsafeStatement( - SyntaxFactory.Block(embeddedStatement.WithoutTrivia())) - .WithLeadingTrivia(todoComment, newLine); - - // Wrap in a block to replace the embedded statement - var wrappingBlock = SyntaxFactory.Block(unsafeBlock) - .WithLeadingTrivia(embeddedStatement.GetLeadingTrivia()) - .WithTrailingTrivia(embeddedStatement.GetTrailingTrivia()); - - editor.ReplaceNode(embeddedStatement, wrappingBlock); - - return editor.GetChangedDocument(); - } - - private static async Task ConvertExpressionBodyToUnsafeBlockAsync( - Document document, - ArrowExpressionClauseSyntax arrowExpr, - CancellationToken cancellationToken) - { - var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); - - // Create the TODO comment - var todoComment = SyntaxFactory.Comment("// TODO(unsafe): Baselining unsafe usage"); - var newLine = SyntaxFactory.CarriageReturnLineFeed; - - // Get the parent member to determine the return type - var parent = arrowExpr.Parent; - bool isVoid = false; - - if (parent is MethodDeclarationSyntax method) - { - isVoid = method.ReturnType is PredefinedTypeSyntax pts && pts.Keyword.IsKind(SyntaxKind.VoidKeyword); - } - else if (parent is LocalFunctionStatementSyntax localFunc) - { - isVoid = localFunc.ReturnType is PredefinedTypeSyntax pts && pts.Keyword.IsKind(SyntaxKind.VoidKeyword); - } - else if (parent is DestructorDeclarationSyntax) - { - isVoid = true; - } - else if (parent is AccessorDeclarationSyntax or PropertyDeclarationSyntax or IndexerDeclarationSyntax) - { - isVoid = false; - } - - // Create the statement that goes inside the unsafe block - StatementSyntax innerStatement = isVoid - ? SyntaxFactory.ExpressionStatement(arrowExpr.Expression.WithoutTrivia()) - : SyntaxFactory.ReturnStatement(arrowExpr.Expression.WithoutTrivia()); - - // Create the unsafe block - var unsafeBlock = SyntaxFactory.UnsafeStatement( - SyntaxFactory.Block(innerStatement)) - .WithLeadingTrivia(todoComment, newLine); - - // Create the block body - var blockBody = SyntaxFactory.Block(unsafeBlock); - - // Replace based on parent type - switch (parent) - { - case MethodDeclarationSyntax methodDecl: - var newMethod = methodDecl - .WithExpressionBody(null) - .WithSemicolonToken(default) - .WithBody(blockBody); - editor.ReplaceNode(methodDecl, newMethod); - break; - - case LocalFunctionStatementSyntax localFunc: - var newLocalFunc = localFunc - .WithExpressionBody(null) - .WithSemicolonToken(default) - .WithBody(blockBody); - editor.ReplaceNode(localFunc, newLocalFunc); - break; - - case DestructorDeclarationSyntax destructorDecl: - var newDestructor = destructorDecl - .WithExpressionBody(null) - .WithSemicolonToken(default) - .WithBody(blockBody); - editor.ReplaceNode(destructorDecl, newDestructor); - break; - - case PropertyDeclarationSyntax propDecl: - var getter = SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration) - .WithBody(SyntaxFactory.Block(unsafeBlock)); - var accessorList = SyntaxFactory.AccessorList(SyntaxFactory.SingletonList(getter)); - var newProp = propDecl - .WithExpressionBody(null) - .WithSemicolonToken(default) - .WithAccessorList(accessorList); - editor.ReplaceNode(propDecl, newProp); - break; - - case AccessorDeclarationSyntax accessor: - var newAccessor = accessor - .WithExpressionBody(null) - .WithSemicolonToken(default) - .WithBody(SyntaxFactory.Block(unsafeBlock)); - editor.ReplaceNode(accessor, newAccessor); - break; - - case IndexerDeclarationSyntax indexerDecl: - var indexerGetter = SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration) - .WithBody(SyntaxFactory.Block(unsafeBlock)); - var indexerAccessorList = SyntaxFactory.AccessorList(SyntaxFactory.SingletonList(indexerGetter)); - var newIndexer = indexerDecl - .WithExpressionBody(null) - .WithSemicolonToken(default) - .WithAccessorList(indexerAccessorList); - editor.ReplaceNode(indexerDecl, newIndexer); - break; - } - - return editor.GetChangedDocument(); - } - - /// - /// Checks if the arrow expression clause or its expression has preprocessor directive trivia. - /// Converting expression bodies with directives to block bodies is error-prone, so we skip the fix. - /// - private static bool HasDirectiveTrivia(ArrowExpressionClauseSyntax arrowExpr) - { - // Check the arrow expression clause's leading trivia (e.g., #if or #else before =>) - if (arrowExpr.GetLeadingTrivia().Any(t => t.IsDirective)) - return true; - - // Check the arrow token's trailing trivia (e.g., #if right after =>) - if (arrowExpr.ArrowToken.TrailingTrivia.Any(t => t.IsDirective)) - return true; - - // Check the expression's leading trivia (e.g., #if before the expression) - if (arrowExpr.Expression.GetLeadingTrivia().Any(t => t.IsDirective)) - return true; - - // Check the expression's trailing trivia (e.g., #endif after the expression) - if (arrowExpr.Expression.GetTrailingTrivia().Any(t => t.IsDirective)) - return true; - - // Check the arrow expression clause's trailing trivia - if (arrowExpr.GetTrailingTrivia().Any(t => t.IsDirective)) - return true; - - // Check the parent member for directives between the signature and the arrow - // e.g., method() #if FOO => expr1; #else => expr2; #endif - if (arrowExpr.Parent is MethodDeclarationSyntax method) - { - // Check trivia after the parameter list (where #if might appear) - if (method.ParameterList.GetTrailingTrivia().Any(t => t.IsDirective)) - return true; - // Check constraint clauses if present - foreach (var constraint in method.ConstraintClauses) - { - if (constraint.GetTrailingTrivia().Any(t => t.IsDirective)) - return true; - } - } - else if (arrowExpr.Parent is LocalFunctionStatementSyntax localFunc) - { - if (localFunc.ParameterList.GetTrailingTrivia().Any(t => t.IsDirective)) - return true; - } - else if (arrowExpr.Parent is PropertyDeclarationSyntax prop) - { - if (prop.Identifier.TrailingTrivia.Any(t => t.IsDirective)) - return true; - } - - return false; - } - } -} -#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/Resources.resx b/src/tools/illink/src/ILLink.CodeFix/Resources.resx index 99b54822b57a9f..ebcab064ea3dee 100644 --- a/src/tools/illink/src/ILLink.CodeFix/Resources.resx +++ b/src/tools/illink/src/ILLink.CodeFix/Resources.resx @@ -126,8 +126,8 @@ Add RequiresDynamicCode attribute to parent method - - Add 'unsafe' to parent method + + Add unsafe context Mark extern member 'unsafe' @@ -144,6 +144,9 @@ Mark field-like member 'unsafe' + + Propagate unsafe contract + Add UnconditionalSuppressMessage attribute to parent method diff --git a/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs new file mode 100644 index 00000000000000..6e87ba68523a0d --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs @@ -0,0 +1,699 @@ +// 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 ILLink.CodeFixProvider; +using ILLink.RoslynAnalyzer; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.FindSymbols; +using Microsoft.CodeAnalysis.Formatting; +using Microsoft.CodeAnalysis.Text; + +namespace ILLink.CodeFix +{ + /// + /// Propagates intentional caller-unsafe contracts through partials, overrides, and interface implementations. + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(SynchronizeUnsafeContractCodeFixProvider)), Shared] + public sealed class SynchronizeUnsafeContractCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider + { + public const string UnsafeOverrideDiagnosticId = "CS9364"; + public const string UnsafeImplicitImplementationDiagnosticId = "CS9365"; + public const string UnsafeExplicitImplementationDiagnosticId = "CS9366"; + public const string PartialUnsafeMismatchDiagnosticId = "CS0764"; + public const string PartialSafeMismatchDiagnosticId = "CS9390"; + + private static LocalizableString UnsafeCodeFixTitle => + new LocalizableResourceString( + nameof(Resources.SynchronizeUnsafeContractCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => + [ + UnsafeOverrideDiagnosticId, + UnsafeImplicitImplementationDiagnosticId, + UnsafeExplicitImplementationDiagnosticId, + PartialUnsafeMismatchDiagnosticId, + PartialSafeMismatchDiagnosticId, + ]; + + public override FixAllProvider GetFixAllProvider() => ContractFixAllProvider.Instance; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + Diagnostic diagnostic = context.Diagnostics[0]; + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null || semanticModel is null) + return; + + SyntaxNode targetNode = root.FindNode( + diagnostic.Location.SourceSpan, + getInnermostNodeForTie: true); + if (targetNode.AncestorsAndSelf().OfType() + .FirstOrDefault(static variable => + variable.Parent?.Parent is EventFieldDeclarationSyntax) is { } eventVariable + && eventVariable.Parent?.Parent is EventFieldDeclarationSyntax + { + Declaration.Variables.Count: > 1, + } eventField + && eventField.ContainsDirectives) + { + return; + } + + if (UnsafeModifierCodeFixHelpers.FindDeclaration(targetNode) is not { } declaration + || GetDeclaredContractSymbol( + semanticModel, + targetNode, + declaration, + context.CancellationToken) is not { } declaredSymbol) + { + return; + } + + ISymbol symbol = GetContractSymbolForDiagnostic( + diagnostic, + declaredSymbol, + context.CancellationToken); + if (!IsSupportedContractSymbol(symbol)) + return; + + HashSet closure = await GetContractClosureAsync( + context.Document.Project.Solution, + symbol, + context.CancellationToken).ConfigureAwait(false); + if (!CanPropagateUnsafeContract( + context.Document.Project.Solution, + closure, + context.CancellationToken)) + { + return; + } + + string unsafeTitle = UnsafeCodeFixTitle.ToString(); + context.RegisterCodeFix( + CodeAction.Create( + unsafeTitle, + cancellationToken => ApplyModifierAsync( + context.Document.Project.Solution, + closure, + cancellationToken), + unsafeTitle), + diagnostic); + } + + private sealed class ContractFixAllProvider : FixAllProvider + { + internal static ContractFixAllProvider Instance { get; } = new(); + + public override async Task GetFixAsync(FixAllContext fixAllContext) + { + ImmutableArray diagnostics = + await GetDiagnosticsInScopeAsync(fixAllContext).ConfigureAwait(false); + if (diagnostics.IsEmpty) + return null; + + string title = UnsafeCodeFixTitle.ToString(); + return CodeAction.Create( + title, + cancellationToken => PropagateDiagnosticsAsync( + fixAllContext.Solution, + diagnostics, + cancellationToken), + fixAllContext.CodeActionEquivalenceKey ?? title); + } + + private static async Task> GetDiagnosticsInScopeAsync( + FixAllContext context) => + context.Scope switch + { + FixAllScope.Document => await context + .GetDocumentDiagnosticsAsync(context.Document!) + .ConfigureAwait(false), + FixAllScope.Project => await context + .GetAllDiagnosticsAsync(context.Project!) + .ConfigureAwait(false), + FixAllScope.Solution => ImmutableArray.CreateRange( + (await Task.WhenAll( + context.Solution.Projects.Select(context.GetAllDiagnosticsAsync)) + .ConfigureAwait(false)) + .SelectMany(static diagnostics => diagnostics)), + _ => [], + }; + } + + private static async Task PropagateDiagnosticsAsync( + Solution solution, + ImmutableArray diagnostics, + CancellationToken cancellationToken) + { + var closure = new HashSet(SymbolEqualityComparer.Default); + foreach (Diagnostic diagnostic in diagnostics) + { + if (await GetDiagnosticSymbolAsync( + solution, + diagnostic, + cancellationToken).ConfigureAwait(false) is not { } symbol) + { + continue; + } + + HashSet diagnosticClosure = await GetContractClosureAsync( + solution, + symbol, + cancellationToken).ConfigureAwait(false); + if (CanPropagateUnsafeContract( + solution, + diagnosticClosure, + cancellationToken)) + { + closure.UnionWith(diagnosticClosure); + } + } + + return await ApplyModifierAsync( + solution, + closure, + cancellationToken).ConfigureAwait(false); + } + + private static async Task GetDiagnosticSymbolAsync( + Solution solution, + Diagnostic diagnostic, + CancellationToken cancellationToken) + { + Document? document = solution.GetDocument(diagnostic.Location.SourceTree); + if (document is null) + return null; + + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + if (root is null || semanticModel is null) + return null; + + SyntaxNode targetNode = root.FindNode( + diagnostic.Location.SourceSpan, + getInnermostNodeForTie: true); + if (targetNode.AncestorsAndSelf().OfType() + .FirstOrDefault(static variable => + variable.Parent?.Parent is EventFieldDeclarationSyntax) is { } eventVariable + && eventVariable.Parent?.Parent is EventFieldDeclarationSyntax + { + Declaration.Variables.Count: > 1, + } eventField + && eventField.ContainsDirectives) + { + return null; + } + + if (UnsafeModifierCodeFixHelpers.FindDeclaration(targetNode) is not { } declaration + || GetDeclaredContractSymbol( + semanticModel, + targetNode, + declaration, + cancellationToken) is not { } declaredSymbol) + { + return null; + } + + ISymbol symbol = GetContractSymbolForDiagnostic( + diagnostic, + declaredSymbol, + cancellationToken); + return IsSupportedContractSymbol(symbol) ? symbol : null; + } + + private static ISymbol GetContractSymbolForDiagnostic( + Diagnostic diagnostic, + ISymbol declaredSymbol, + CancellationToken cancellationToken) + { + ISymbol symbol = UnsafeContractHelpers.NormalizeContractSymbol(declaredSymbol); + if (diagnostic.Id is not (PartialUnsafeMismatchDiagnosticId or PartialSafeMismatchDiagnosticId) + || symbol is not IPropertySymbol property) + { + return symbol; + } + + IPropertySymbol[] propertyParts = UnsafeContractHelpers.GetPartialParts(property) + .OfType() + .ToArray(); + if (propertyParts.Length < 2) + return symbol; + + if (HasMismatch(propertyParts, HasPropertyUnsafe)) + return symbol; + + if (property.GetMethod is not null + && HasMismatch(propertyParts, propertyPart => + HasAccessorUnsafe(propertyPart, MethodKind.PropertyGet))) + { + return property.GetMethod; + } + + if (property.SetMethod is not null + && HasMismatch(propertyParts, propertyPart => + HasAccessorUnsafe(propertyPart, MethodKind.PropertySet))) + { + return property.SetMethod; + } + + return symbol; + + bool HasPropertyUnsafe(IPropertySymbol propertyPart) => + UnsafeContractHelpers.GetDirectDeclarations(propertyPart, cancellationToken) + .Any(declaration => + UnsafeMigrationSyntaxHelpers.HasModifier( + declaration, + SyntaxKind.UnsafeKeyword)); + + bool HasAccessorUnsafe(IPropertySymbol propertyPart, MethodKind accessorKind) + { + IMethodSymbol? accessor = accessorKind == MethodKind.PropertyGet + ? propertyPart.GetMethod + : propertyPart.SetMethod; + return accessor is not null + && UnsafeContractHelpers.GetDirectDeclarations(accessor, cancellationToken) + .Any(declaration => + declaration is AccessorDeclarationSyntax + && UnsafeMigrationSyntaxHelpers.HasModifier( + declaration, + SyntaxKind.UnsafeKeyword)); + } + + static bool HasMismatch( + IEnumerable parts, + System.Func hasModifier) + { + bool? first = null; + foreach (IPropertySymbol part in parts) + { + bool current = hasModifier(part); + if (first is null) + { + first = current; + } + else if (first.Value != current) + { + return true; + } + } + + return false; + } + } + + private static ISymbol? GetDeclaredContractSymbol( + SemanticModel semanticModel, + SyntaxNode targetNode, + SyntaxNode declaration, + CancellationToken cancellationToken) + { + VariableDeclaratorSyntax? eventVariable = targetNode.AncestorsAndSelf() + .OfType() + .FirstOrDefault(static variable => + variable.Parent?.Parent is EventFieldDeclarationSyntax); + return eventVariable is not null + ? semanticModel.GetDeclaredSymbol(eventVariable, cancellationToken) + : semanticModel.GetDeclaredSymbol(declaration, cancellationToken); + } + + private static bool IsSupportedContractSymbol(ISymbol symbol) => + symbol is IMethodSymbol or IPropertySymbol or IEventSymbol; + + private static bool CanPropagateUnsafeContract( + Solution solution, + IEnumerable symbols, + CancellationToken cancellationToken) + { + foreach (ISymbol symbol in symbols) + { + bool hasEditableDeclaration = false; + foreach (SyntaxNode declaration in UnsafeContractHelpers.GetDeclarations( + symbol, + cancellationToken)) + { + if (solution.GetDocument(declaration.SyntaxTree) is null) + continue; + + if (declaration is VariableDeclaratorSyntax + { + Parent.Parent: EventFieldDeclarationSyntax eventField, + }) + { + if (eventField.Declaration.Variables.Count > 1 + && eventField.ContainsDirectives) + { + return false; + } + + hasEditableDeclaration = true; + continue; + } + + if (declaration is ArrowExpressionClauseSyntax + || UnsafeModifierCodeFixHelpers.FindDeclaration(declaration) is not null) + { + hasEditableDeclaration = true; + } + } + + if (!hasEditableDeclaration) + return false; + } + + return true; + } + + private static async Task> GetContractClosureAsync( + Solution solution, + ISymbol seed, + CancellationToken cancellationToken) + { + var closure = new HashSet(SymbolEqualityComparer.Default); + var queue = new Queue(); + + Enqueue(seed); + while (queue.Count > 0) + { + ISymbol symbol = queue.Dequeue(); + + foreach (ISymbol part in UnsafeContractHelpers.GetPartialParts(symbol)) + Enqueue(part); + + if (UnsafeContractHelpers.GetOverriddenMember(symbol) is { } overriddenMember) + Enqueue(overriddenMember); + + foreach (ISymbol interfaceMember in UnsafeContractHelpers.GetImplementedInterfaceMembers(symbol)) + Enqueue(interfaceMember); + + foreach (ISymbol overridingMember in await SymbolFinder.FindOverridesAsync( + symbol, + solution, + cancellationToken: cancellationToken).ConfigureAwait(false)) + { + Enqueue(overridingMember); + } + + if (symbol.ContainingType?.TypeKind == TypeKind.Interface) + { + foreach (ISymbol implementation in await SymbolFinder.FindImplementationsAsync( + symbol, + solution, + cancellationToken: cancellationToken).ConfigureAwait(false)) + { + Enqueue(implementation); + } + } + } + + return closure; + + void Enqueue(ISymbol symbol) + { + symbol = UnsafeContractHelpers.NormalizeContractSymbol(symbol); + if (closure.Add(symbol)) + queue.Enqueue(symbol); + } + } + + private static async Task ApplyModifierAsync( + Solution solution, + IEnumerable symbols, + CancellationToken cancellationToken) + { + var targetsByDocument = new Dictionary>(); + var eventTargetsByDocument = + new Dictionary>>(); + foreach (ISymbol symbol in symbols) + { + foreach (SyntaxNode declaration in UnsafeContractHelpers.GetDeclarations(symbol, cancellationToken)) + { + Document? document = solution.GetDocument(declaration.SyntaxTree); + if (document is null) + continue; + + if (declaration is VariableDeclaratorSyntax + { + Parent.Parent: EventFieldDeclarationSyntax eventField, + } eventVariable) + { + if (!eventTargetsByDocument.TryGetValue( + document.Id, + out Dictionary>? eventFields)) + { + eventFields = new Dictionary>(); + eventTargetsByDocument.Add(document.Id, eventFields); + } + + if (!eventFields.TryGetValue( + eventField.Span, + out HashSet? eventNames)) + { + eventNames = new HashSet(); + eventFields.Add(eventField.Span, eventNames); + } + + eventNames.Add(eventVariable.Identifier.ValueText); + continue; + } + + if (!targetsByDocument.TryGetValue(document.Id, out HashSet? spans)) + { + spans = new HashSet(); + targetsByDocument.Add(document.Id, spans); + } + + spans.Add(declaration.Span); + } + } + + var documentIds = new HashSet(targetsByDocument.Keys); + documentIds.UnionWith(eventTargetsByDocument.Keys); + foreach (DocumentId documentId in documentIds) + { + Document document = solution.GetDocument(documentId)!; + var targets = new List(); + if (targetsByDocument.TryGetValue(documentId, out HashSet? spans)) + { + targets.AddRange(spans.Select(static span => + new ModifierTarget(span))); + } + if (eventTargetsByDocument.TryGetValue( + documentId, + out Dictionary>? eventFields)) + { + targets.AddRange(eventFields.Select(static eventField => + new ModifierTarget(eventField.Key) + { + EventNames = eventField.Value, + })); + } + foreach (ModifierTarget target in + targets.OrderByDescending(static target => target.Span.Start)) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + continue; + + SyntaxNode node = root.FindNode(target.Span, getInnermostNodeForTie: true); + if (target.EventNames is not null) + { + EventFieldDeclarationSyntax? eventField = node.AncestorsAndSelf() + .OfType() + .FirstOrDefault(); + if (eventField is not null) + { + document = await SetEventVariablesUnsafeAsync( + document, + root, + eventField, + target.EventNames, + cancellationToken).ConfigureAwait(false); + } + continue; + } + + ArrowExpressionClauseSyntax? expressionBody = node.AncestorsAndSelf() + .OfType() + .FirstOrDefault(); + if (expressionBody is not null) + { + document = SetExpressionBodiedGetterUnsafe( + document, + root, + expressionBody); + continue; + } + + if (UnsafeModifierCodeFixHelpers.FindDeclaration(node) is not { } declaration) + continue; + + document = await UnsafeModifierCodeFixHelpers.SetUnsafeModifierAsync( + document, + declaration, + cancellationToken).ConfigureAwait(false); + } + + solution = document.Project.Solution; + } + + return solution; + } + + private static Document SetExpressionBodiedGetterUnsafe( + Document document, + SyntaxNode root, + ArrowExpressionClauseSyntax expressionBody) + { + SyntaxToken unsafeModifier = SyntaxFactory.Token(SyntaxKind.UnsafeKeyword) + .WithTrailingTrivia(SyntaxFactory.ElasticSpace); + AccessorDeclarationSyntax getter = SyntaxFactory.AccessorDeclaration( + SyntaxKind.GetAccessorDeclaration) + .WithModifiers([unsafeModifier]) + .WithExpressionBody(expressionBody) + .WithSemicolonToken(expressionBody.Parent switch + { + PropertyDeclarationSyntax property => property.SemicolonToken, + IndexerDeclarationSyntax indexer => indexer.SemicolonToken, + _ => default, + }); + + SyntaxNode replacement = expressionBody.Parent switch + { + PropertyDeclarationSyntax property => property + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithAccessorList(SyntaxFactory.AccessorList( + SyntaxFactory.SingletonList(getter))), + IndexerDeclarationSyntax indexer => indexer + .WithExpressionBody(null) + .WithSemicolonToken(default) + .WithAccessorList(SyntaxFactory.AccessorList( + SyntaxFactory.SingletonList(getter))), + _ => expressionBody.Parent!, + }; + return document.WithSyntaxRoot(root.ReplaceNode( + expressionBody.Parent!, + replacement.WithAdditionalAnnotations(Formatter.Annotation))); + } + + private static async Task SetEventVariablesUnsafeAsync( + Document document, + SyntaxNode root, + EventFieldDeclarationSyntax eventField, + HashSet eventNames, + CancellationToken cancellationToken) + { + SeparatedSyntaxList variables = eventField.Declaration.Variables; + if (variables.Count > 1 && eventField.ContainsDirectives) + return document; + + if (variables.Count == 1 + || variables.All(variable => eventNames.Contains(variable.Identifier.ValueText))) + { + return await UnsafeModifierCodeFixHelpers.SetUnsafeModifierAsync( + document, + eventField, + cancellationToken).ConfigureAwait(false); + } + + if (eventField.Parent is not TypeDeclarationSyntax containingType) + return document; + + var splitEvents = new List(variables.Count); + for (int i = 0; i < variables.Count; i++) + { + VariableDeclaratorSyntax variable = variables[i]; + EventFieldDeclarationSyntax splitEvent = eventField + .WithDeclaration(eventField.Declaration.WithVariables( + SyntaxFactory.SingletonSeparatedList(variable))) + .WithLeadingTrivia(i == 0 + ? eventField.GetLeadingTrivia() + : SyntaxFactory.TriviaList(SyntaxFactory.ElasticCarriageReturnLineFeed)) + .WithTrailingTrivia(i == variables.Count - 1 + ? eventField.GetTrailingTrivia() + : variables.GetSeparator(i).LeadingTrivia + .AddRange(variables.GetSeparator(i).TrailingTrivia) + .Add(SyntaxFactory.ElasticCarriageReturnLineFeed)); + + if (eventNames.Contains(variable.Identifier.ValueText)) + splitEvent = SetEventFieldUnsafe(splitEvent); + + splitEvents.Add(splitEvent.WithAdditionalAnnotations(Formatter.Annotation)); + } + + int memberIndex = containingType.Members.IndexOf(eventField); + SyntaxList members = containingType.Members + .RemoveAt(memberIndex) + .InsertRange(memberIndex, splitEvents); + TypeDeclarationSyntax replacement = containingType.WithMembers(members); + return document.WithSyntaxRoot(root.ReplaceNode(containingType, replacement)); + } + + private static EventFieldDeclarationSyntax SetEventFieldUnsafe( + EventFieldDeclarationSyntax eventField) + { + SyntaxToken safeModifier = UnsafeMigrationSyntaxHelpers.GetSafeModifier(eventField); + if (safeModifier != default) + { + return eventField.ReplaceToken( + safeModifier, + SyntaxFactory.Token(SyntaxKind.UnsafeKeyword).WithTriviaFrom(safeModifier)); + } + + if (UnsafeMigrationSyntaxHelpers.HasModifier(eventField, SyntaxKind.UnsafeKeyword)) + return eventField; + + SyntaxTokenList modifiers = eventField.Modifiers; + int insertionIndex = modifiers.IndexOf(SyntaxKind.ExternKeyword); + int partialIndex = modifiers.IndexOf(SyntaxKind.PartialKeyword); + if (partialIndex >= 0 && (insertionIndex < 0 || partialIndex < insertionIndex)) + insertionIndex = partialIndex; + if (insertionIndex < 0) + insertionIndex = modifiers.Count; + + SyntaxToken unsafeModifier = SyntaxFactory.Token(SyntaxKind.UnsafeKeyword) + .WithTrailingTrivia(SyntaxFactory.ElasticSpace); + if (modifiers.Count == 0) + { + unsafeModifier = unsafeModifier.WithLeadingTrivia(eventField.EventKeyword.LeadingTrivia); + eventField = eventField.WithEventKeyword( + eventField.EventKeyword.WithLeadingTrivia(default(SyntaxTriviaList))); + } + else if (insertionIndex == 0) + { + unsafeModifier = unsafeModifier.WithLeadingTrivia(modifiers[0].LeadingTrivia); + modifiers = modifiers.Replace( + modifiers[0], + modifiers[0].WithLeadingTrivia(default(SyntaxTriviaList))); + } + + return eventField.WithModifiers(modifiers.Insert(insertionIndex, unsafeModifier)); + } + + private sealed class ModifierTarget + { + internal ModifierTarget(TextSpan span) + { + Span = span; + } + + internal TextSpan Span { get; } + internal HashSet? EventNames { get; init; } + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs index 2813ff19d1402d..2b8e53a70e7b0f 100644 --- a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs @@ -18,7 +18,7 @@ namespace ILLink.CodeFix { /// /// Centralizes declaration discovery and trivia-preserving modifier edits for the unsafe-v2 code-fix providers. - /// It is shared by fixes for CS9389, CS9377/CS0106, IL5005, IL5006, and CS9392. + /// It is shared by unsafe migration fixes for compiler diagnostics, IL5005, and IL5006. /// internal static class UnsafeModifierCodeFixHelpers { @@ -96,6 +96,32 @@ internal static async Task AddUnsafeModifierAsync( return editor.GetChangedDocument(); } + /// + /// Replaces an explicit safe contract with unsafe, or adds unsafe when no safety modifier is present. + /// + internal static async Task SetUnsafeModifierAsync( + Document document, + SyntaxNode declaration, + CancellationToken cancellationToken) + { + SyntaxToken safeModifier = UnsafeMigrationSyntaxHelpers.GetSafeModifier(declaration); + if (safeModifier != default) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + return document; + + SyntaxToken unsafeModifier = SyntaxFactory.Token(SyntaxKind.UnsafeKeyword) + .WithTriviaFrom(safeModifier); + return document.WithSyntaxRoot(root.ReplaceToken(safeModifier, unsafeModifier)); + } + + if (UnsafeMigrationSyntaxHelpers.HasModifier(declaration, SyntaxKind.UnsafeKeyword)) + return document; + + return await AddUnsafeModifierAsync(document, declaration, cancellationToken).ConfigureAwait(false); + } + /// /// Removes unsafe without disturbing modifiers that the current SyntaxGenerator does not model. /// @@ -168,8 +194,11 @@ private static AccessorDeclarationSyntax RemoveUnsafeModifier(AccessorDeclaratio private static SyntaxTokenList AddUnsafeModifier(SyntaxTokenList modifiers) { - // Place unsafe before extern while preserving the existing modifier order. + // Place unsafe before syntax-sensitive extern/partial modifiers while preserving other modifier order. int insertionIndex = modifiers.IndexOf(SyntaxKind.ExternKeyword); + int partialIndex = modifiers.IndexOf(SyntaxKind.PartialKeyword); + if (partialIndex >= 0 && (insertionIndex < 0 || partialIndex < insertionIndex)) + insertionIndex = partialIndex; if (insertionIndex < 0) insertionIndex = modifiers.Count; @@ -203,7 +232,7 @@ private static SyntaxTokenList RemoveUnsafeModifier(SyntaxTokenList modifiers) return modifiers; } - private static SyntaxNode WithModifiers(SyntaxNode declaration, SyntaxTokenList modifiers) => + internal static SyntaxNode WithModifiers(SyntaxNode declaration, SyntaxTokenList modifiers) => declaration switch { BaseTypeDeclarationSyntax type => type.WithModifiers(modifiers), diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs new file mode 100644 index 00000000000000..8eddf0defb38dd --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs @@ -0,0 +1,219 @@ +// 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.Linq; +using System.Threading; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Text; + +namespace ILLink.RoslynAnalyzer +{ + /// + /// Provides source and symbol helpers shared by unsafe-contract analyzers and code fixes. + /// + internal static class UnsafeContractHelpers + { + internal static ISymbol NormalizeContractSymbol(ISymbol symbol) => + symbol is IMethodSymbol + { + MethodKind: MethodKind.EventAdd or MethodKind.EventRemove, + AssociatedSymbol: IEventSymbol @event, + } + ? @event + : symbol; + + internal static IEnumerable GetPartialParts(ISymbol symbol) + { + symbol = NormalizeContractSymbol(symbol); + var seen = new HashSet(SymbolEqualityComparer.Default); + + foreach (ISymbol part in GetPartialPartsCore(symbol)) + { + if (seen.Add(part)) + yield return part; + } + } + + internal static IEnumerable GetDeclarations( + ISymbol symbol, + CancellationToken cancellationToken) + { + var seen = new HashSet<(SyntaxTree Tree, TextSpan Span)>(); + foreach (ISymbol part in GetPartialParts(symbol)) + { + foreach (SyntaxReference reference in part.DeclaringSyntaxReferences) + { + SyntaxNode declaration = GetDeclarationForSymbol( + part, + reference.GetSyntax(cancellationToken)); + if (seen.Add((declaration.SyntaxTree, declaration.Span))) + yield return declaration; + } + } + } + + internal static IEnumerable GetDirectDeclarations( + ISymbol symbol, + CancellationToken cancellationToken) + { + symbol = NormalizeContractSymbol(symbol); + foreach (SyntaxReference reference in symbol.DeclaringSyntaxReferences) + { + yield return GetDeclarationForSymbol( + symbol, + reference.GetSyntax(cancellationToken)); + } + } + + internal static ISymbol? GetOverriddenMember(ISymbol symbol) => + NormalizeContractSymbol(symbol) switch + { + IMethodSymbol method => method.OverriddenMethod, + IPropertySymbol property => property.OverriddenProperty, + IEventSymbol @event => @event.OverriddenEvent, + _ => null, + }; + + internal static IEnumerable GetImplementedInterfaceMembers(ISymbol symbol) + { + symbol = NormalizeContractSymbol(symbol); + INamedTypeSymbol? containingType = symbol.ContainingType; + if (containingType is null) + yield break; + + var seen = new HashSet(SymbolEqualityComparer.Default); + foreach (INamedTypeSymbol @interface in containingType.AllInterfaces) + { + foreach (ISymbol interfaceMember in GetInterfaceContractSlots(@interface)) + { + ISymbol? implementation = containingType.FindImplementationForInterfaceMember(interfaceMember); + if (implementation is not null + && ContractSymbolsMatch(implementation, symbol) + && seen.Add(interfaceMember)) + { + yield return interfaceMember; + } + } + } + } + + internal static IEnumerable GetInterfaceContractSlots(INamedTypeSymbol @interface) + { + foreach (ISymbol member in @interface.GetMembers()) + { + if (member is IMethodSymbol { AssociatedSymbol: not null }) + continue; + + if (member is IPropertySymbol property) + { + if (property.GetMethod is not null) + yield return property.GetMethod; + if (property.SetMethod is not null) + yield return property.SetMethod; + continue; + } + + yield return member; + } + } + + private static IEnumerable GetPartialPartsCore(ISymbol symbol) + { + yield return symbol; + + switch (symbol) + { + case IMethodSymbol { AssociatedSymbol: IPropertySymbol property } accessor: + foreach (IPropertySymbol propertyPart in GetPropertyParts(property)) + { + IMethodSymbol? accessorPart = accessor.MethodKind == MethodKind.PropertyGet + ? propertyPart.GetMethod + : propertyPart.SetMethod; + if (accessorPart is not null) + yield return accessorPart; + } + break; + + case IMethodSymbol method: + if (method.PartialDefinitionPart is { } methodDefinition) + yield return methodDefinition; + if (method.PartialImplementationPart is { } methodImplementation) + yield return methodImplementation; + break; + + case IPropertySymbol property: + foreach (IPropertySymbol propertyPart in GetPropertyParts(property)) + yield return propertyPart; + break; + + case IEventSymbol @event: + if (@event.PartialDefinitionPart is { } eventDefinition) + yield return eventDefinition; + if (@event.PartialImplementationPart is { } eventImplementation) + yield return eventImplementation; + break; + } + } + + private static IEnumerable GetPropertyParts(IPropertySymbol property) + { + yield return property; + if (property.PartialDefinitionPart is { } propertyDefinition) + yield return propertyDefinition; + if (property.PartialImplementationPart is { } propertyImplementation) + yield return propertyImplementation; + } + + private static bool ContractSymbolsMatch(ISymbol left, ISymbol right) + { + foreach (ISymbol leftPart in GetPartialParts(NormalizeContractSymbol(left))) + { + foreach (ISymbol rightPart in GetPartialParts(NormalizeContractSymbol(right))) + { + if (SymbolEqualityComparer.Default.Equals(leftPart, rightPart) + || SymbolEqualityComparer.Default.Equals( + leftPart.OriginalDefinition, + rightPart.OriginalDefinition)) + { + return true; + } + } + } + + return false; + } + + private static SyntaxNode FindDeclaration(SyntaxNode node) => + node.AncestorsAndSelf().FirstOrDefault(static ancestor => + ancestor is VariableDeclaratorSyntax + { + Parent.Parent: EventFieldDeclarationSyntax, + } + or BaseMethodDeclarationSyntax + or BasePropertyDeclarationSyntax + or EventDeclarationSyntax + or EventFieldDeclarationSyntax + or AccessorDeclarationSyntax) + ?? node; + + private static SyntaxNode GetDeclarationForSymbol(ISymbol symbol, SyntaxNode syntax) + { + SyntaxNode declaration = FindDeclaration(syntax); + if (symbol is IMethodSymbol { MethodKind: MethodKind.PropertyGet }) + { + return declaration switch + { + PropertyDeclarationSyntax { ExpressionBody: { } expressionBody } => expressionBody, + IndexerDeclarationSyntax { ExpressionBody: { } expressionBody } => expressionBody, + _ => declaration, + }; + } + + return declaration; + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs index 66fbe621f1628f..e04d58c47c7309 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs @@ -32,6 +32,9 @@ internal static bool HasModifier(SyntaxNode declaration, SyntaxKind modifier) => internal static bool HasSafeModifier(SyntaxNode declaration) => s_safeKeyword != SyntaxKind.None && GetModifiers(declaration).Any(s_safeKeyword); + internal static SyntaxToken GetSafeModifier(SyntaxNode declaration) => + s_safeKeyword == SyntaxKind.None ? default : GetModifier(declaration, s_safeKeyword); + internal static SyntaxToken GetModifier(SyntaxNode declaration, SyntaxKind modifier) { foreach (SyntaxToken token in GetModifiers(declaration)) diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs new file mode 100644 index 00000000000000..d15e0be2c49a3c --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs @@ -0,0 +1,1466 @@ +// 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; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Xunit; + +namespace ILLink.RoslynAnalyzer.Tests +{ + /// + /// Verifies unsafe-context migration across compiler diagnostics and syntax positions. + /// + public class AddUnsafeContextCodeFixTests + { + [Fact] + public async Task SplitsLocalDeclarationAndAddsSafetyComment() + { + var source = """ + class C + { + static unsafe int Read() => 0; + + static int M() + { + int value = {|CS9362:Read()|}; + return value; + } + } + """; + var fixedSource = """ + class C + { + static unsafe int Read() => 0; + + static int M() + { + int value; + unsafe + { + // SAFETY: Audit + value = Read(); + } + return value; + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task InitializerOutVariableKeepsItsScope() + { + var source = """ + class C + { + static unsafe int Get(out int other) + { + other = 0; + return 0; + } + + static void M() + { + int value = {|CS9362:Get(out int other)|}; + System.Console.WriteLine(value + other); + } + } + """; + var fixedSource = """ + class C + { + static unsafe int Get(out int other) + { + other = 0; + return 0; + } + + static void M() + { + int value = unsafe(/* SAFETY: Audit */ Get(out int other)); + System.Console.WriteLine(value + other); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UnsafeImplicitConversionPreservesSelectedOperator() + { + var source = """ + struct Convertible + { + public static unsafe implicit operator short(Convertible value) => 1; + public static explicit operator int(Convertible value) => 2; + } + + class C + { + static int Value = {|CS9362:new Convertible()|}; + } + """; + var fixedSource = """ + struct Convertible + { + public static unsafe implicit operator short(Convertible value) => 1; + public static explicit operator int(Convertible value) => 2; + } + + class C + { + static int Value = unsafe(/* SAFETY: Audit */ (short)new Convertible()); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task StackAllocForwardDeclarationIsScoped() + { + var source = """ + [module: System.Runtime.CompilerServices.SkipLocalsInit] + + class C + { + static void M() + { + System.Span bytes = {|CS9361:stackalloc byte[10]|}; + bytes.Clear(); + } + } + """; + var fixedSource = """ + [module: System.Runtime.CompilerServices.SkipLocalsInit] + + class C + { + static void M() + { + scoped System.Span bytes; + unsafe + { + // SAFETY: Audit + bytes = stackalloc byte[10]; + } + bytes.Clear(); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ExtendsUnsafeStatementAroundLabelForOutVariable() + { + var source = """ + class C + { + static unsafe void Get(out int value) => value = 0; + static void Use(int value) { } + + static void M() + { + Label: + {|CS9362:Get(out int value)|}; + Use(value); + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Get(out int value) => value = 0; + static void Use(int value) { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Label: + Get(out int value); + Use(value); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task NoFixWhenGotoWouldEnterUnsafeStatement() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + goto Label; + Label: + {|CS9362:Work()|}; + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task WrapsPointerDereferenceStatement() + { + var source = """ + class C + { + static void M(int* pointer) + { + System.Console.WriteLine({|CS9360:*|}pointer); + } + } + """; + var fixedSource = """ + class C + { + static void M(int* pointer) + { + unsafe + { + // SAFETY: Audit + System.Console.WriteLine(*pointer); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UsesUnsafeExpressionForCatchFilter() + { + var source = """ + class C + { + static unsafe bool Filter(System.Exception exception) => true; + + static void M() + { + try + { + } + catch (System.Exception exception) when ({|CS9362:Filter(exception)|}) + { + } + } + } + """; + var fixedSource = """ + class C + { + static unsafe bool Filter(System.Exception exception) => true; + + static void M() + { + try + { + } + catch (System.Exception exception) when (unsafe(/* SAFETY: Audit */ Filter(exception))) + { + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UsesUnsafeExpressionForFieldInitializer() + { + var source = """ + class C + { + static unsafe int Read() => 0; + static int Value = {|CS9362:Read()|}; + } + """; + var fixedSource = """ + class C + { + static unsafe int Read() => 0; + static int Value = unsafe(/* SAFETY: Audit */ Read()); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UnsafeImplicitConversionStaysInsideExpression() + { + var source = """ + class Convertible + { + public static unsafe implicit operator int(Convertible value) => 0; + } + + class C + { + static int Value = {|CS9362:new Convertible()|}; + } + """; + var fixedSource = """ + class Convertible + { + public static unsafe implicit operator int(Convertible value) => 0; + } + + class C + { + static int Value = unsafe(/* SAFETY: Audit */ (int)new Convertible()); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ConditionalAccessWrapsTheCompleteExpression() + { + var source = """ + class Data + { + public unsafe int Value { get; } + } + + class C + { + static Data s_data = new(); + static int? Value = s_data?{|CS9362:.Value|}; + } + """; + var fixedSource = """ + class Data + { + public unsafe int Value { get; } + } + + class C + { + static Data s_data = new(); + static int? Value = unsafe(/* SAFETY: Audit */ s_data?.Value); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ObjectInitializerWrapsTheCompleteCreation() + { + var source = """ + class Data + { + public unsafe int Value { get; set; } + } + + class C + { + static Data Value = new() { {|CS9362:Value|} = 1 }; + } + """; + var fixedSource = """ + class Data + { + public unsafe int Value { get; set; } + } + + class C + { + static Data Value = unsafe(/* SAFETY: Audit */ new() { Value = 1 }); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task CollectionInitializerWrapsTheCompleteCreation() + { + var source = """ + class Collection : System.Collections.Generic.IEnumerable + { + public unsafe void Add(int value) { } + public System.Collections.Generic.IEnumerator GetEnumerator() => null; + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => GetEnumerator(); + } + + class C + { + static Collection Value = new() { {|CS9362:1|} }; + } + """; + var fixedSource = """ + class Collection : System.Collections.Generic.IEnumerable + { + public unsafe void Add(int value) { } + public System.Collections.Generic.IEnumerator GetEnumerator() => null; + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => GetEnumerator(); + } + + class C + { + static Collection Value = unsafe(/* SAFETY: Audit */ new() { 1 }); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UsesUnsafeExpressionForUsingDeclarationInitializer() + { + var source = """ + class Resource : System.IDisposable + { + public void Dispose() { } + } + + class C + { + static unsafe Resource Create() => new(); + + static void M() + { + using Resource resource = {|CS9362:Create()|}; + System.Console.WriteLine(resource); + } + } + """; + var fixedSource = """ + class Resource : System.IDisposable + { + public void Dispose() { } + } + + class C + { + static unsafe Resource Create() => new(); + + static void M() + { + using Resource resource = unsafe(/* SAFETY: Audit */ Create()); + System.Console.WriteLine(resource); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UsesUnsafeExpressionForScopedRefInitializer() + { + var source = """ + class C + { + static int s_value; + static unsafe ref int GetReference() => ref s_value; + + static void M() + { + scoped ref int value = ref {|CS9362:GetReference()|}; + value++; + } + } + """; + var fixedSource = """ + class C + { + static int s_value; + static unsafe ref int GetReference() => ref s_value; + + static void M() + { + scoped ref int value = ref unsafe(/* SAFETY: Audit */ GetReference()); + value++; + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task AwaitUsesUnsafeExpression() + { + var source = """ + class C + { + static unsafe System.Threading.Tasks.Task WorkAsync() => System.Threading.Tasks.Task.CompletedTask; + + static async System.Threading.Tasks.Task M() + { + await {|CS9362:WorkAsync()|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe System.Threading.Tasks.Task WorkAsync() => System.Threading.Tasks.Task.CompletedTask; + + static async System.Threading.Tasks.Task M() + { + await unsafe(/* SAFETY: Audit */ WorkAsync()); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ExpressionBodiedAwaitUsesUnsafeExpression() + { + var source = """ + class C + { + static unsafe System.Threading.Tasks.Task WorkAsync() => System.Threading.Tasks.Task.CompletedTask; + static async System.Threading.Tasks.Task M() => await {|CS9362:WorkAsync()|}; + } + """; + var fixedSource = """ + class C + { + static unsafe System.Threading.Tasks.Task WorkAsync() => System.Threading.Tasks.Task.CompletedTask; + static async System.Threading.Tasks.Task M() => await unsafe(/* SAFETY: Audit */ WorkAsync()); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task AwaitUsingInitializerUsesUnsafeExpression() + { + var source = """ + class Resource : System.IAsyncDisposable + { + public System.Threading.Tasks.ValueTask DisposeAsync() => default; + } + + class C + { + static unsafe Resource Create() => new(); + + static async System.Threading.Tasks.Task M() + { + await using Resource resource = {|CS9362:Create()|}; + await System.Threading.Tasks.Task.Yield(); + } + } + """; + var fixedSource = """ + class Resource : System.IAsyncDisposable + { + public System.Threading.Tasks.ValueTask DisposeAsync() => default; + } + + class C + { + static unsafe Resource Create() => new(); + + static async System.Threading.Tasks.Task M() + { + await using Resource resource = unsafe(/* SAFETY: Audit */ Create()); + await System.Threading.Tasks.Task.Yield(); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task AwaitedLocalInitializerUsesUnsafeExpression() + { + var source = """ + class C + { + static unsafe System.Threading.Tasks.Task GetAsync() => System.Threading.Tasks.Task.FromResult(0); + + static async System.Threading.Tasks.Task M() + { + int value = await {|CS9362:GetAsync()|}; + return value; + } + } + """; + var fixedSource = """ + class C + { + static unsafe System.Threading.Tasks.Task GetAsync() => System.Threading.Tasks.Task.FromResult(0); + + static async System.Threading.Tasks.Task M() + { + int value = await unsafe(/* SAFETY: Audit */ GetAsync()); + return value; + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task NoFixWhenUnsafeAwaitPatternSpansAwaitExpression() + { + var source = """ + class Awaitable + { + public unsafe Awaiter GetAwaiter() => default; + } + + struct Awaiter : System.Runtime.CompilerServices.INotifyCompletion + { + public bool IsCompleted => true; + public void OnCompleted(System.Action continuation) { } + public void GetResult() { } + } + + class C + { + static async System.Threading.Tasks.Task M() + { + {|CS9362:await new Awaitable()|}; + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixForExpressionBodiedUnsafeAwaitPattern() + { + var source = """ + class Awaitable + { + public unsafe Awaiter GetAwaiter() => default; + } + + struct Awaiter : System.Runtime.CompilerServices.INotifyCompletion + { + public bool IsCompleted => true; + public void OnCompleted(System.Action continuation) { } + public void GetResult() { } + } + + class C + { + static async System.Threading.Tasks.Task M() => + {|CS9362:await new Awaitable()|}; + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixForAwaitUsingWithUnsafeDispose() + { + var source = """ + class Resource + { + public unsafe System.Threading.Tasks.ValueTask DisposeAsync() => default; + } + + class C + { + static async System.Threading.Tasks.Task M() + { + {|CS9362:await using Resource resource = new();|} + await System.Threading.Tasks.Task.Yield(); + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixWhenUsingRangeWouldHideGenericLocalFunction() + { + var source = """ + ref struct Resource + { + public unsafe void Dispose() { } + } + + class C + { + static void M() + { + Local(); + {|CS9362:using Resource resource = new();|} + void Local() { } + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixWhenRangeCrossesConditionalDirective() + { + var source = """ + #define A + + class C + { + static unsafe void Get(out int value) => value = 0; + + static void M() + { + #if A + {|CS9362:Get(out int value)|}; + #endif + System.Console.WriteLine(value); + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixWhenUsingRangeWouldHideLocalFunction() + { + var source = """ + ref struct Resource + { + public unsafe void Dispose() { } + } + + class C + { + static void M() + { + Local(); + {|CS9362:using Resource resource = new();|} + void Local() { } + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixForUnsafeForeachContainingYield() + { + var source = """ + class Enumerable + { + public unsafe Enumerator GetEnumerator() => default; + + public struct Enumerator + { + public int Current => 0; + public bool MoveNext() => false; + } + } + + class C + { + static System.Collections.Generic.IEnumerable M() + { + {|CS9362:foreach|} (int value in new Enumerable()) + { + yield return value; + } + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task YieldUsesUnsafeExpression() + { + var source = """ + class C + { + static unsafe int Read() => 0; + + static System.Collections.Generic.IEnumerable M() + { + yield return {|CS9362:Read()|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe int Read() => 0; + + static System.Collections.Generic.IEnumerable M() + { + yield return unsafe(/* SAFETY: Audit */ Read()); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ConvertsExpressionBodiedMemberToUnsafeStatement() + { + var source = """ + class C + { + static unsafe int Read() => 0; + static int M() => {|CS9362:Read()|}; + } + """; + var fixedSource = """ + class C + { + static unsafe int Read() => 0; + static int M() + { + unsafe + { + // SAFETY: Audit + return Read(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ConvertsAsyncTaskExpressionBodyToExpressionStatement() + { + var source = """ + class C + { + static unsafe System.Threading.Tasks.Task WorkAsync() => + System.Threading.Tasks.Task.CompletedTask; + + static async System.Threading.Tasks.Task M() => {|CS9362:WorkAsync()|}; + } + """; + var fixedSource = """ + class C + { + static unsafe System.Threading.Tasks.Task WorkAsync() => + System.Threading.Tasks.Task.CompletedTask; + + static async System.Threading.Tasks.Task M() + { + unsafe + { + // SAFETY: Audit + WorkAsync(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ConvertsAsyncTaskValueExpressionBodyToExpressionStatement() + { + var source = """ + class C + { + static unsafe int Work() => 0; + static async System.Threading.Tasks.Task M() => {|CS9362:Work()|}; + } + """; + var fixedSource = """ + class C + { + static unsafe int Work() => 0; + static async System.Threading.Tasks.Task M() + { + unsafe + { + // SAFETY: Audit + Work(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task PreservesOutVariableScope() + { + var source = """ + class C + { + static unsafe bool TryGet(out int value) + { + value = 0; + return true; + } + + static void M() + { + if ({|CS9362:TryGet(out int value)|}) + { + } + + System.Console.WriteLine(value); + } + } + """; + var fixedSource = """ + class C + { + static unsafe bool TryGet(out int value) + { + value = 0; + return true; + } + + static void M() + { + if (unsafe(/* SAFETY: Audit */ TryGet(out int value))) + { + } + + System.Console.WriteLine(value); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ExtendsUnsafeStatementForOutVariableInExpressionStatement() + { + var source = """ + class C + { + static unsafe void Get(out int value) => value = 0; + + static void M() + { + {|CS9362:Get(out int value)|}; + System.Console.WriteLine(value); + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Get(out int value) => value = 0; + + static void M() + { + unsafe + { + // SAFETY: Audit + Get(out int value); + System.Console.WriteLine(value); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ExtendsUnsafeStatementForUsingDeclarationDispose() + { + var source = """ + ref struct Resource + { + public unsafe void Dispose() { } + public void Use() { } + } + + class C + { + static void M() + { + {|CS9362:using Resource resource = new();|} + resource.Use(); + } + } + """; + var fixedSource = """ + ref struct Resource + { + public unsafe void Dispose() { } + public void Use() { } + } + + class C + { + static void M() + { + unsafe + { + // SAFETY: Audit + using Resource resource = new(); + resource.Use(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UsesUnsafeExpressionInsideDirective() + { + var source = """ + class C + { + static unsafe int Read() => 0; + + static int M() + { + int value = + #if true + {|CS9362:Read()|} + #else + 0 + #endif + ; + return value; + } + } + """; + var fixedSource = """ + class C + { + static unsafe int Read() => 0; + + static int M() + { + int value = + #if true + unsafe(/* SAFETY: Audit */ Read()) + #else + 0 + #endif + ; + return value; + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UnsafeStatementStaysInsideConditionalDirective() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + #if true + {|CS9362:Work()|}; + #endif + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + #if true + unsafe + { + // SAFETY: Audit + Work(); + } + #endif + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task TopLevelLocalKeepsItsScope() + { + var source = """ + int value = {|CS9362:Read()|}; + System.Console.WriteLine(value); + + static unsafe int Read() => 0; + """; + var fixedSource = """ + int value = unsafe(/* SAFETY: Audit */ Read()); + System.Console.WriteLine(value); + + static unsafe int Read() => 0; + """; + + var test = CreateTest(source, fixedSource); + test.TestState.OutputKind = OutputKind.ConsoleApplication; + await test.RunAsync(); + } + + [Fact] + public async Task TopLevelOutVariableKeepsItsScope() + { + var source = """ + {|CS9362:Get(out int value)|}; + System.Console.WriteLine(value); + + static unsafe void Get(out int value) => value = 0; + """; + var fixedSource = """ + unsafe + { + // SAFETY: Audit + Get(out int value); + System.Console.WriteLine(value); + + static unsafe void Get(out int value) => value = 0; + } + """; + + var test = CreateTest(source, fixedSource); + test.TestState.OutputKind = OutputKind.ConsoleApplication; + await test.RunAsync(); + } + + [Fact] + public async Task NoFixWhenTopLevelGotoWouldEnterUnsafeStatement() + { + var source = """ + goto Label; + Label: + {|CS9362:Work()|}; + + static unsafe void Work() { } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + test.TestState.OutputKind = OutputKind.ConsoleApplication; + await test.RunAsync(); + } + + [Fact] + public async Task UsesUnsafeExpressionInConstructorArgument() + { + var source = """ + class B + { + protected B(int value) { } + } + + class C : B + { + static unsafe int Read() => 0; + C() : base({|CS9362:Read()|}) { } + } + """; + var fixedSource = """ + class B + { + protected B(int value) { } + } + + class C : B + { + static unsafe int Read() => 0; + C() : base(unsafe(/* SAFETY: Audit */ Read())) { } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task MarksConstructorUnsafeForUnsafeBaseCall() + { + var source = """ + class B + { + protected unsafe B() { } + } + + class C : B + { + {|CS9362:public C() { }|} + } + """; + var fixedSource = """ + class B + { + protected unsafe B() { } + } + + class C : B + { + public unsafe C() { } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task MarksConstructorUnsafeForLegacyPointerBaseCall() + { + var source = """ + class C : LegacyBase + { + public C(int* pointer) {|CS9363:: base(pointer)|} { } + } + """; + var fixedSource = """ + class C : LegacyBase + { + public unsafe C(int* pointer) : base(pointer) { } + } + """; + + var test = CreateTest(source, fixedSource); + test.TestState.AdditionalReferences.Add(CreateLegacyReference()); + await test.RunAsync(); + } + + [Fact] + public async Task AddsUnsafeToUsingAliasForConstructorConstraint() + { + var source = """ + using {|CS9376:Alias|} = Generic; + + class UnsafeConstructor + { + public unsafe UnsafeConstructor() { } + } + + class Generic where T : new() { } + """; + var fixedSource = """ + using unsafe Alias = Generic; + + class UnsafeConstructor + { + public unsafe UnsafeConstructor() { } + } + + class Generic where T : new() { } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task WrapsConstructorConstraintInvocation() + { + var source = """ + class UnsafeConstructor + { + public unsafe UnsafeConstructor() { } + } + + class C + { + static void Create() where T : new() { } + + static void M() + { + {|CS9376:Create()|}; + } + } + """; + var fixedSource = """ + class UnsafeConstructor + { + public unsafe UnsafeConstructor() { } + } + + class C + { + static void Create() where T : new() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Create(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task FixesLegacyPointerSignatureDiagnostic() + { + var source = """ + class C + { + static void M() + { + {|CS9363:Legacy.GetPointer()|}; + } + } + """; + var fixedSource = """ + class C + { + static void M() + { + unsafe + { + // SAFETY: Audit + Legacy.GetPointer(); + } + } + } + """; + + var test = CreateTest(source, fixedSource); + test.TestState.AdditionalReferences.Add(CreateLegacyReference()); + await test.RunAsync(); + } + + [Fact] + public async Task OneUnsafeStatementFixesOverlappingDiagnostics() + { + var source = """ + class C + { + static void M() + { + Legacy.Callback(); + } + } + """; + var fixedSource = """ + class C + { + static void M() + { + unsafe + { + // SAFETY: Audit + Legacy.Callback(); + } + } + } + """; + + var test = CreateTest(source, fixedSource); + test.TestState.AdditionalReferences.Add(CreateLegacyReference()); + test.TestState.ExpectedDiagnostics.Add( + Microsoft.CodeAnalysis.Testing.DiagnosticResult + .CompilerError(AddUnsafeContextCodeFixProvider.UnsafeMemberOperationCompatDiagnosticId) + .WithSpan(5, 9, 5, 24) + .WithArguments("Legacy.Callback")); + test.TestState.ExpectedDiagnostics.Add( + Microsoft.CodeAnalysis.Testing.DiagnosticResult + .CompilerError(AddUnsafeContextCodeFixProvider.UnsafeOperationDiagnosticId) + .WithSpan(5, 9, 5, 26)); + await test.RunAsync(); + } + + private static CSharpCodeFixVerifier.Test CreateTest( + string source, + string fixedSource) => + UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source, + fixedSource); + + private static MetadataReference CreateLegacyReference() + { + var syntaxTree = SyntaxFactory.ParseSyntaxTree( + """ + public static class Legacy + { + public static unsafe int* GetPointer() => null; + public static unsafe delegate* Callback; + } + + public class LegacyBase + { + public unsafe LegacyBase(int* pointer) { } + } + """, + new CSharpParseOptions(LanguageVersion.Preview)); + CSharpCompilation compilation = CSharpCompilation.Create( + "Legacy", + [syntaxTree], + SourceGenerators.Tests.LiveReferencePack.GetMetadataReferences(), + new CSharpCompilationOptions( + OutputKind.DynamicallyLinkedLibrary, + allowUnsafe: true)); + return compilation.EmitToImageReference(); + } + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs deleted file mode 100644 index aab096802e3cf5..00000000000000 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs +++ /dev/null @@ -1,1675 +0,0 @@ -// 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; -using ILLink.Shared; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.Testing; -using Microsoft.CodeAnalysis.Text; -using Xunit; -using VerifyCS = ILLink.RoslynAnalyzer.Tests.CSharpCodeFixVerifier< - ILLink.RoslynAnalyzer.DynamicallyAccessedMembersAnalyzer, - ILLink.CodeFix.RequiresUnsafeCodeFixProvider>; - -namespace ILLink.RoslynAnalyzer.Tests -{ - public class RequiresUnsafeCodeFixTests - { - 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!; - compilationOptions = compilationOptions.WithAllowUnsafe(true); - return solution.WithProjectParseOptions(projectId, parseOptions) - .WithProjectCompilationOptions(projectId, compilationOptions); - } - - static Task VerifyRequiresUnsafeCodeFix( - string source, - string fixedSource, - DiagnosticResult[] baselineExpected, - DiagnosticResult[] fixedExpected, - int? numberOfIterations = null, - int codeActionIndex = 1) - { - var test = new VerifyCS.Test - { - TestCode = source, - FixedCode = fixedSource, - CodeActionIndex = codeActionIndex - }; - test.ExpectedDiagnostics.AddRange(baselineExpected); - test.SolutionTransforms.Add(SetOptions); - if (numberOfIterations != null) - { - test.NumberOfIncrementalIterations = numberOfIterations; - test.NumberOfFixAllIterations = numberOfIterations; - } - test.FixedState.ExpectedDiagnostics.AddRange(fixedExpected); - return test.RunAsync(); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_SimpleStatement() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x = M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionStatement() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - public void M2() - { - M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - public void M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 9, 9, 13) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ReturnStatement() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - return M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 16, 9, 20) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_IfStatement() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe bool M1() => true; - - public void M2() - { - if (M1()) - { - System.Console.WriteLine("yes"); - } - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe bool M1() => true; - - public void M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - if (M1()) - { - System.Console.WriteLine("yes"); - } - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 13, 9, 17) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task NoWarning_InsideUnsafeBlock() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - unsafe - { - int x = M1(); - } - } - } - """; - - // No diagnostics expected - already in unsafe block - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: test, // No change expected - baselineExpected: Array.Empty(), - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_InsideUnsafeMethod() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe void M2() - { - int x = M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty(), - codeActionIndex: 0); - } - - [Fact] - public async Task CodeFix_InsideUnsafeClass() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public unsafe class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x = M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public unsafe class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_InsideUnsafeProperty() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe int P - { - get => M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe int P - { - get - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 16, 9, 20) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty(), - codeActionIndex: 1); - } - - [Fact] - public async Task CodeFix_InsideUnsafeLocalFunction() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - unsafe int Local() => M1(); - - _ = Local(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe void M2() - { - unsafe int Local() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - // TODO(unsafe): Baselining unsafe usage - - unsafe - { - _ = Local(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 31, 9, 35) - .WithArguments("C.M1()"), - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(11, 13, 11, 20) - .WithArguments("Local()") - }, - fixedExpected: Array.Empty(), - numberOfIterations: 3, - codeActionIndex: 0); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionBodiedMethod() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() => M1(); - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(7, 24, 7, 28) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionBodiedVoidMethod() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - public void M2() => M1(); - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - public void M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(7, 25, 7, 29) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionBodiedDestructor() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - ~C() => M1(); - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - ~C() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - M1(); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(7, 13, 7, 17) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionBodiedProperty() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int P => M1(); - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int P - { - get - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(7, 21, 7, 25) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty(), - codeActionIndex: 1); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionBodiedAccessor() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int P - { - get => M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int P - { - get - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 16, 9, 20) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty(), - codeActionIndex: 1); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_LocalFunction_ConvertsExpressionBody() - { - // Local functions with expression bodies are converted to block bodies - // with the unsafe block inside, preserving their scope. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int Local() => M1(); - _ = Local(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int Local() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - - _ = Local(); - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 24, 9, 28) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ForwardDeclaration() - { - // When a variable is declared with unsafe code and used later, - // use forward declaration instead of expanding the unsafe block. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x = M1(); - int y = x + 1; - System.Console.WriteLine(y); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - int y = x + 1; - System.Console.WriteLine(y); - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_UnusedVariable() - { - // When a variable declared with unsafe code is not used after, - // forward declaration still applies consistently. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x = M1(); - int y = 42; - System.Console.WriteLine(y); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - int y = 42; - System.Console.WriteLine(y); - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ForwardDeclaration_VarType() - { - // When using 'var', the forward declaration should use the explicit type. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - var x = M1(); - System.Console.WriteLine(x); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - System.Console.WriteLine(x); - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_RefLocal_NoForwardDeclaration() - { - // Ref locals cannot use forward declaration (can't declare without initializer), - // so wrap the entire declaration in the unsafe block. - // Note: If the ref local is used after the declaration, those uses will be broken - // and need manual fixing. This test has no usage after the declaration. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - private int _field; - - public static unsafe ref int M1(ref int x) => ref x; - - public void M2() - { - ref int x = ref M1(ref _field); - } - } - """; - - // ref locals can't be forward-declared, so wrap the whole declaration - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - private int _field; - - public static unsafe ref int M1(ref int x) => ref x; - - public void M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - ref int x = ref M1(ref _field); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(11, 25, 11, 39) - .WithArguments("C.M1(ref int)") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_RefLocalFromUnsafeAs_NoForwardDeclaration() - { - // Pattern matching real-world usage: ref byte x = ref Unsafe.As(ref source) - // The Unsafe.As call is unsafe, and the result is assigned to a ref local. - // When the ref local is used after the declaration, those statements must be included - // in the unsafe block since ref locals can't be forward-declared. - var test = """ - using System.Diagnostics.CodeAnalysis; - using System.Runtime.CompilerServices; - - public class C - { - public void M2() - { - char c = 'x'; - ref byte x = ref Unsafe.As(ref c); - int y = x + 1; - } - } - - namespace System.Runtime.CompilerServices - { - public static class Unsafe - { - public static unsafe ref TTo As(ref TFrom source) => throw null!; - } - } - """; - - // ref locals can't be forward-declared, so must expand block to include usages - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - using System.Runtime.CompilerServices; - - public class C - { - public void M2() - { - char c = 'x'; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - ref byte x = ref Unsafe.As(ref c); - int y = x + 1; - } - } - } - - namespace System.Runtime.CompilerServices - { - public static class Unsafe - { - public static unsafe ref TTo As(ref TFrom source) => throw null!; - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 26, 9, 54) - .WithArguments("System.Runtime.CompilerServices.Unsafe.As(ref char)") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ChainedRefLocals_ExpandsToIncludeAll() - { - // When a ref local is used to create another ref local, and that second ref local - // is used later, the unsafe block must expand to include ALL usages. - // This matches the pattern in CharUnicodeInfo.cs where rsStart is used to create rsDelta. - var test = """ - using System.Diagnostics.CodeAnalysis; - using System.Runtime.CompilerServices; - - public class C - { - public void M2() - { - byte b = 0; - ref ushort rsStart = ref Unsafe.As(ref b); - ref ushort rsDelta = ref Unsafe.Add(ref rsStart, 1); - int delta = rsDelta; - } - } - - namespace System.Runtime.CompilerServices - { - public static class Unsafe - { - public static unsafe ref TTo As(ref TFrom source) => throw null!; - public static ref T Add(ref T source, int elementOffset) => throw null!; - } - } - """; - - // Both ref locals and the usage of rsDelta must be inside the unsafe block - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - using System.Runtime.CompilerServices; - - public class C - { - public void M2() - { - byte b = 0; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - ref ushort rsStart = ref Unsafe.As(ref b); - ref ushort rsDelta = ref Unsafe.Add(ref rsStart, 1); - int delta = rsDelta; - } - } - } - - namespace System.Runtime.CompilerServices - { - public static class Unsafe - { - public static unsafe ref TTo As(ref TFrom source) => throw null!; - public static ref T Add(ref T source, int elementOffset) => throw null!; - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 34, 9, 64) - .WithArguments("System.Runtime.CompilerServices.Unsafe.As(ref byte)") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_RefLocal_RegularVarEscapes_ExpandsToIncludeUsage() - { - // When a ref local block expansion pulls in a regular variable declaration, - // and that regular variable is used after the block, we must expand further. - // This matches the CharUnicodeInfo.cs pattern. - var test = """ - using System.Diagnostics.CodeAnalysis; - using System.Runtime.CompilerServices; - - public class C - { - public int M2() - { - byte b = 0; - ref ushort rsStart = ref Unsafe.As(ref b); - ref ushort rsDelta = ref Unsafe.Add(ref rsStart, 1); - int delta = rsDelta; - return delta + 1; - } - } - - namespace System.Runtime.CompilerServices - { - public static class Unsafe - { - public static unsafe ref TTo As(ref TFrom source) => throw null!; - public static ref T Add(ref T source, int elementOffset) => throw null!; - } - } - """; - - // Block must expand to include return statement since delta is used there - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - using System.Runtime.CompilerServices; - - public class C - { - public int M2() - { - byte b = 0; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - ref ushort rsStart = ref Unsafe.As(ref b); - ref ushort rsDelta = ref Unsafe.Add(ref rsStart, 1); - int delta = rsDelta; - return delta + 1; - } - } - } - - namespace System.Runtime.CompilerServices - { - public static class Unsafe - { - public static unsafe ref TTo As(ref TFrom source) => throw null!; - public static ref T Add(ref T source, int elementOffset) => throw null!; - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 34, 9, 64) - .WithArguments("System.Runtime.CompilerServices.Unsafe.As(ref byte)") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_RefReadonlyLocal_NoForwardDeclaration() - { - // Ref readonly locals cannot use forward declaration, so wrap the declaration. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe ref readonly int M1(in int x) => ref x; - - public void M2() - { - int value = 42; - ref readonly int x = ref M1(in value); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe ref readonly int M1(in int x) => ref x; - - public void M2() - { - int value = 42; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - ref readonly int x = ref M1(in value); - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(10, 34, 10, 46) - .WithArguments("C.M1(in int)") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_NotOfferedForStatementsWithPragmaDirectives() - { - // When statements to wrap have #pragma directives after the diagnostic statement, - // the unsafe block should not include the directive trivia. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public void M2() - { - int x = M1(); - #pragma warning disable CS0168 - if (x > 0) - #pragma warning restore CS0168 - { - } - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe void M2() - { - int x; - // TODO(unsafe): Baselining unsafe usage - unsafe - { - x = M1(); - } - #pragma warning disable CS0168 - if (x > 0) - #pragma warning restore CS0168 - { - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 17, 9, 21) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty(), - numberOfIterations: 2, - codeActionIndex: 0); - } - - [Fact] - public async Task CodeFix_AddUnsafeModifier_Method() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - return M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe int M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - """; - - var addUnsafeTest = new VerifyCS.Test - { - TestCode = test, - FixedCode = fixedSource, - CodeActionIndex = 0, - NumberOfIncrementalIterations = 2, - NumberOfFixAllIterations = 2 - }; - addUnsafeTest.ExpectedDiagnostics.Add( - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 16, 9, 20) - .WithArguments("C.M1()")); - addUnsafeTest.SolutionTransforms.Add(SetOptions); - await addUnsafeTest.RunAsync(); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_Method() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - return M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - } - """; - - var addAttributeTest = new VerifyCS.Test - { - TestCode = test, - FixedCode = fixedSource, - CodeActionIndex = 1 - }; - addAttributeTest.ExpectedDiagnostics.Add( - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 16, 9, 20) - .WithArguments("C.M1()")); - addAttributeTest.SolutionTransforms.Add(SetOptions); - await addAttributeTest.RunAsync(); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_Constructor() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - public C() - { - M1(); - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe void M1() { } - - public C() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - M1(); - } - } - } - """; - - var addAttributeTest = new VerifyCS.Test - { - TestCode = test, - FixedCode = fixedSource, - CodeActionIndex = 1 - }; - addAttributeTest.ExpectedDiagnostics.Add( - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(9, 9, 9, 13) - .WithArguments("C.M1()")); - addAttributeTest.SolutionTransforms.Add(SetOptions); - await addAttributeTest.RunAsync(); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_NotOfferedForExpressionBodyWithPreprocessorDirectives() - { - // When an expression-bodied member has preprocessor directives (#if/#else/#endif), - // the "Wrap in unsafe block" fix should NOT be offered because it would destroy - // the conditional compilation structure. - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - #if SOME_DEFINE - => 42; - #else - => M1(); - #endif - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public unsafe int M2() - #if SOME_DEFINE - => 42; - #else - => M1(); - #endif - } - """; - - // The "Wrap in unsafe block" fix should NOT be available. - - var addAttributeTest = new VerifyCS.Test - { - TestCode = test, - FixedCode = fixedSource, - CodeActionIndex = 0, - NumberOfIncrementalIterations = 1, - NumberOfFixAllIterations = 1 - }; - addAttributeTest.ExpectedDiagnostics.Add( - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(11, 12, 11, 16) - .WithArguments("C.M1()")); - addAttributeTest.FixedState.ExpectedDiagnostics.Add( - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(11, 12, 11, 16) - .WithArguments("C.M1()")); - addAttributeTest.SolutionTransforms.Add(SetOptions); - await addAttributeTest.RunAsync(); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_SwitchCaseSection() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2(int x) - { - switch (x) - { - case 1: - return M1(); - default: - return 0; - } - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2(int x) - { - switch (x) - { - case 1: - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - default: - return 0; - } - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(12, 24, 12, 28) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_IfStatementWithoutBraces() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2(bool condition) - { - if (condition) - return M1(); - return 0; - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2(bool condition) - { - if (condition) - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - return 0; - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(10, 20, 10, 24) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - - [Fact] - public async Task CodeFix_WrapInUnsafeBlock_ExpressionBodiedLocalFunction() - { - var test = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - int x = 1; - static int LocalFunc() => M1(); - return LocalFunc() + x; - } - } - """; - - var fixedSource = """ - using System.Diagnostics.CodeAnalysis; - - public class C - { - public static unsafe int M1() => 0; - - public int M2() - { - int x = 1; - static int LocalFunc() - { - // TODO(unsafe): Baselining unsafe usage - unsafe - { - return M1(); - } - } - - return LocalFunc() + x; - } - } - """; - - await VerifyRequiresUnsafeCodeFix( - source: test, - fixedSource: fixedSource, - baselineExpected: new[] { - DiagnosticResult.CompilerError(RequiresUnsafeCodeFixProvider.UnsafeMemberOperationDiagnosticId) - .WithSpan(10, 35, 10, 39) - .WithArguments("C.M1()") - }, - fixedExpected: Array.Empty()); - } - } -} -#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs new file mode 100644 index 00000000000000..52e7205fa0a571 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs @@ -0,0 +1,476 @@ +// 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; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Xunit; + +namespace ILLink.RoslynAnalyzer.Tests +{ + /// + /// Verifies one-way unsafe propagation for compiler-reported contract mismatches. + /// + public class SynchronizeUnsafeContractCodeFixTests + { + [Fact] + public async Task CompilerOverrideFixPropagatesUpAndDown() + { + var source = """ + class Base + { + public virtual void Method() { } + } + + class First : Base + { + public override unsafe void {|CS9364:Method|}() { } + } + + class Second : Base + { + public override void Method() { } + } + """; + var fixedSource = """ + class Base + { + public virtual unsafe void Method() { } + } + + class First : Base + { + public override unsafe void Method() { } + } + + class Second : Base + { + public override unsafe void Method() { } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task CompilerInterfaceFixPropagatesToEveryImplementation() + { + var source = """ + interface I + { + void Method(); + } + + class Implicit : I + { + public unsafe void {|CS9365:Method|}() { } + } + + class Explicit : I + { + unsafe void I.{|CS9366:Method|}() { } + } + + class Other : I + { + public void Method() { } + } + """; + var fixedSource = """ + interface I + { + unsafe void Method(); + } + + class Implicit : I + { + public unsafe void Method() { } + } + + class Explicit : I + { + unsafe void I.Method() { } + } + + class Other : I + { + public unsafe void Method() { } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task CompilerDefaultInterfaceFixPropagatesToBaseContract() + { + var source = """ + interface IBase + { + void Method(); + } + + interface IDerived : IBase + { + unsafe void IBase.{|CS9366:Method|}() { } + } + """; + var fixedSource = """ + interface IBase + { + unsafe void Method(); + } + + interface IDerived : IBase + { + unsafe void IBase.Method() { } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task NoFixForInheritedImplementationDiagnosticOnType() + { + var source = """ + interface I + { + void Method(); + } + + class Base + { + public unsafe void Method() { } + } + + class Derived : Base, {|CS9365:I|} + { + } + """; + + await CreateCompilerTest(source).RunAsync(); + } + + [Fact] + public async Task NoFixForUneditableMetadataContract() + { + var source = """ + class UnsafeImplementation : I + { + public unsafe void {|CS9365:Method|}() { } + } + + class SafeImplementation : I + { + public void Method() { } + } + """; + + var test = CreateCompilerTest(source); + test.TestState.AdditionalReferences.Add(CreateReference( + """ + public interface I + { + void Method(); + } + """)); + await test.RunAsync(); + } + + [Fact] + public async Task PartialUnsafeMismatchAddsUnsafeToEveryPart() + { + var source = """ + partial class C + { + public partial void Method(); + public unsafe partial void {|CS0764:Method|}() { } + } + """; + var fixedSource = """ + partial class C + { + public unsafe partial void Method(); + public unsafe partial void Method() { } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UnsafeWinsOverSafePartialContract() + { + var source = """ + partial class C + { + public unsafe partial void Method(); + public safe extern partial void {|CS0764:{|CS9390:Method|}|}(); + } + """; + var fixedSource = """ + partial class C + { + public unsafe partial void Method(); + public unsafe extern partial void Method(); + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task PureSafePartialMismatchDefaultsToUnsafe() + { + var source = """ + partial class C + { + public partial void {|CS9389:Method|}(); + public safe extern partial void {|CS9390:Method|}(); + } + """; + var fixedSource = """ + partial class C + { + public unsafe partial void Method(); + public unsafe extern partial void Method(); + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task AccessorContractIsPropagatedIndependently() + { + var source = """ + class Base + { + public virtual int Property { get; set; } + } + + class Derived : Base + { + public override int Property + { + unsafe {|CS9364:get|} => 0; + set { } + } + } + """; + var fixedSource = """ + class Base + { + public virtual int Property { unsafe get; set; } + } + + class Derived : Base + { + public override int Property + { + unsafe get => 0; + set { } + } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task PropagatesAcrossDocuments() + { + var test = new CSharpCodeFixVerifier< + DynamicallyAccessedMembersAnalyzer, + SynchronizeUnsafeContractCodeFixProvider>.Test(); + test.TestState.Sources.Add(( + "Base.cs", + """ + class Base + { + public virtual void Method() { } + } + """)); + test.TestState.Sources.Add(( + "First.cs", + """ + class First : Base + { + public override unsafe void {|CS9364:Method|}() { } + } + """)); + test.TestState.Sources.Add(( + "Second.cs", + """ + class Second : Base + { + public override void Method() { } + } + """)); + test.FixedState.Sources.Add(( + "Base.cs", + """ + class Base + { + public virtual unsafe void Method() { } + } + """)); + test.FixedState.Sources.Add(( + "First.cs", + """ + class First : Base + { + public override unsafe void Method() { } + } + """)); + test.FixedState.Sources.Add(( + "Second.cs", + """ + class Second : Base + { + public override unsafe void Method() { } + } + """)); + test.SolutionTransforms.Add(UnsafeMigrationTestHelpers.SetOptions); + + await test.RunAsync(); + } + + [Fact] + public async Task MixedPartialPropertyFormsUseAccessorModifiers() + { + var source = """ + partial class C + { + public partial int Property { unsafe get; } + public partial int Property => {|CS0764:0|}; + } + """; + var fixedSource = """ + partial class C + { + public partial int Property { unsafe get; } + public partial int Property { unsafe get => 0; } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task CompilerEventFixSplitsSharedInterfaceDeclaration() + { + var source = """ + #pragma warning disable CS0067 + + interface I + { + event System.Action First, Safe; + } + + class C : I + { + public unsafe event System.Action {|CS9365:First|}; + public event System.Action Safe; + } + """; + var fixedSource = """ + #pragma warning disable CS0067 + + interface I + { + unsafe event System.Action First; + event System.Action Safe; + } + + class C : I + { + public unsafe event System.Action First; + public event System.Action Safe; + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task FixAllAggregatesMultipleEventsFromOneInterfaceDeclaration() + { + var source = """ + #pragma warning disable CS0067 + + interface I + { + event System.Action First, Second, Safe; + } + + class C : I + { + public unsafe event System.Action {|CS9365:First|}; + public unsafe event System.Action {|CS9365:Second|}; + public event System.Action Safe; + } + """; + var fixedSource = """ + #pragma warning disable CS0067 + + interface I + { + unsafe event System.Action First; + unsafe event System.Action Second; + event System.Action Safe; + } + + class C : I + { + public unsafe event System.Action First; + public unsafe event System.Action Second; + public event System.Action Safe; + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + private static CSharpCodeFixVerifier< + DynamicallyAccessedMembersAnalyzer, + SynchronizeUnsafeContractCodeFixProvider>.Test CreateCompilerTest( + string source, + string fixedSource) => + UnsafeMigrationTestHelpers + .CreateCodeFixTest< + DynamicallyAccessedMembersAnalyzer, + SynchronizeUnsafeContractCodeFixProvider>( + source, + fixedSource); + + private static CSharpCodeFixVerifier< + DynamicallyAccessedMembersAnalyzer, + SynchronizeUnsafeContractCodeFixProvider>.Test CreateCompilerTest( + string source) => + UnsafeMigrationTestHelpers + .CreateCodeFixTest< + DynamicallyAccessedMembersAnalyzer, + SynchronizeUnsafeContractCodeFixProvider>( + source); + + private static MetadataReference CreateReference(string source) + { + SyntaxTree syntaxTree = SyntaxFactory.ParseSyntaxTree( + source, + new CSharpParseOptions(LanguageVersion.Preview)); + CSharpCompilation compilation = CSharpCompilation.Create( + "ContractReference", + [syntaxTree], + SourceGenerators.Tests.LiveReferencePack.GetMetadataReferences(), + new CSharpCompilationOptions( + OutputKind.DynamicallyLinkedLibrary, + allowUnsafe: true)); + return compilation.EmitToImageReference(); + } + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs index 63ac6c37f9fca7..32d3cb0c5f3ad6 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs @@ -79,6 +79,7 @@ internal static Solution SetOptions(Solution solution, ProjectId projectId) return solution.WithProjectParseOptions(projectId, parseOptions) .WithProjectCompilationOptions(projectId, compilationOptions); } + } } #endif From d86aacccdf225356f50909ec171ecb5f66532c79 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 24 Jul 2026 20:25:22 +0200 Subject: [PATCH 2/4] Address unsafe contract propagation feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86742a9-ab20-4209-83f8-d7074a99b968 --- ...ynchronizeUnsafeContractCodeFixProvider.cs | 18 +++++- .../UnsafeContractHelpers.cs | 1 + .../SynchronizeUnsafeContractCodeFixTests.cs | 59 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs index 6e87ba68523a0d..aaf9597f0a5d37 100644 --- a/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs +++ b/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs @@ -348,6 +348,13 @@ private static bool CanPropagateUnsafeContract( if (solution.GetDocument(declaration.SyntaxTree) is null) continue; + if (declaration.AncestorsAndSelf().OfType() + .Any(static parameter => + parameter.Parent?.Parent is RecordDeclarationSyntax)) + { + return false; + } + if (declaration is VariableDeclaratorSyntax { Parent.Parent: EventFieldDeclarationSyntax eventField, @@ -363,8 +370,15 @@ private static bool CanPropagateUnsafeContract( continue; } - if (declaration is ArrowExpressionClauseSyntax - || UnsafeModifierCodeFixHelpers.FindDeclaration(declaration) is not null) + if (declaration is ArrowExpressionClauseSyntax) + { + hasEditableDeclaration = true; + continue; + } + + if (UnsafeModifierCodeFixHelpers.FindDeclaration(declaration) + is { } editableDeclaration + && editableDeclaration is not BaseTypeDeclarationSyntax) { hasEditableDeclaration = true; } diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs index 8eddf0defb38dd..2a0cca88c7cf59 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs @@ -109,6 +109,7 @@ internal static IEnumerable GetInterfaceContractSlots(INamedTypeSymbol if (member is IPropertySymbol property) { + yield return property; if (property.GetMethod is not null) yield return property.GetMethod; if (property.SetMethod is not null) diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs index 52e7205fa0a571..cbc42f201f0009 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs @@ -103,6 +103,65 @@ public unsafe void Method() { } await CreateCompilerTest(source, fixedSource).RunAsync(); } + [Fact] + public async Task CompilerPropertyFixPropagatesPropertyContract() + { + var source = """ + interface I + { + int Property { get; } + } + + class UnsafeImplementation : I + { + public unsafe int Property => {|CS9365:0|}; + } + + class OtherImplementation : I + { + public int Property => 0; + } + """; + var fixedSource = """ + interface I + { + unsafe int Property { get; } + } + + class UnsafeImplementation : I + { + public unsafe int Property => 0; + } + + class OtherImplementation : I + { + public unsafe int Property => 0; + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task NoFixWhenPropertyImplementationIsSynthesized() + { + var source = """ + interface I + { + int Property { get; } + } + + class UnsafeImplementation : I + { + public unsafe int Property => {|CS9365:0|}; + } + + record OtherImplementation(int Property) : I; + """; + + await CreateCompilerTest(source).RunAsync(); + } + [Fact] public async Task CompilerDefaultInterfaceFixPropagatesToBaseContract() { From 1201bbc1ba29f28116761ca689c32b6144551d62 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 24 Jul 2026 22:22:23 +0200 Subject: [PATCH 3/4] Address review feedback on unsafe migration code fixes Roslyn does not treat an unsafe expression as an unsafe context for property and indexer accessors or for method group conversions, because those are bound by the enclosing binder. The fixer used to wrap such operations anyway, which left the diagnostic in place and nested another unsafe expression on every later pass. It now writes an explicit cast inside the unsafe expression for those operations, declines the fix when no cast can express it (ref-returning accessors, unspeakable types), and never re-wraps an expression that is already inside an unsafe expression. Assignment targets, increments, and deconstruction right sides are handled as statements instead. Modifiers that the fixers add now come with a TODO: Audit stub. Without it IL5005 removed the modifier that had just been added, so a second migration pass undid the first one. Contract propagation no longer marks sibling overrides and implementations: a safe member may override or implement a caller-unsafe one, so widening them only grew the audit surface. Replacing an explicit safe modifier is offered under its own title because it discards a deliberate audit. Also merges a new unsafe region with adjacent generated regions, registers the fixes for every diagnostic in the context, and hardens top level statement replacement, statement list access, and the identifier fallback used when expanding a statement range. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c3f7fe4-85f4-40f3-9ccc-e6113369014b --- ....cs => AddUnsafeContextCodeFixProvider.cs} | 1219 ++++++++++------- .../illink/src/ILLink.CodeFix/Resources.resx | 3 + ...ynchronizeUnsafeContractCodeFixProvider.cs | 104 +- .../UnsafeModifierCodeFixHelpers.cs | 118 +- .../UnsafeMigrationAnalyzerHelpers.cs | 6 +- .../UnsafeMigrationSyntaxHelpers.cs | 16 +- .../AddUnsafeContextCodeFixTests.cs | 534 ++++++++ .../SynchronizeUnsafeContractCodeFixTests.cs | 111 +- ...MissingSafetyDocumentationAnalyzerTests.cs | 29 + 9 files changed, 1568 insertions(+), 572 deletions(-) rename src/tools/illink/src/ILLink.CodeFix/{AddUnsafeContext.cs => AddUnsafeContextCodeFixProvider.cs} (57%) diff --git a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs similarity index 57% rename from src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs rename to src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs index e72aa24db2292c..1faac65563d826 100644 --- a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs +++ b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs @@ -32,8 +32,25 @@ public sealed class AddUnsafeContextCodeFixProvider : Microsoft.CodeAnalysis.Cod public const string UnsafeMemberOperationCompatDiagnosticId = "CS9363"; public const string UnsafeConstructorConstraintDiagnosticId = "CS9376"; + internal const string SafetyAuditComment = "// SAFETY: Audit"; + private const string UnsafeExpressionPlaceholder = "__unsafeExpression"; + /// + /// How many audited-but-safe statements may be absorbed when merging a new unsafe region with an adjacent + /// generated one. Merging trades a slightly wider audit scope for far fewer unsafe blocks per member; keep + /// this small so that migration does not silently blanket unrelated code. + /// + private const int MaxSafeStatementsBetweenMergedRegions = 1; + + private static readonly CSharpParseOptions s_unsafeExpressionParseOptions = + new(LanguageVersion.Preview); + + // The code fix compiles against a Roslyn version that predates unsafe expressions but runs inside whichever + // compiler the host loaded. Discover the syntax kind at run time and fall back to statement contexts when the + // host cannot parse 'unsafe(...)'. + private static readonly int s_unsafeExpressionRawKind = GetUnsafeExpressionRawKind(); + private static readonly SymbolDisplayFormat s_localTypeDisplayFormat = SymbolDisplayFormat.MinimallyQualifiedFormat.WithMiscellaneousOptions( SymbolDisplayFormat.MinimallyQualifiedFormat.MiscellaneousOptions @@ -65,9 +82,8 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (root is null) return; - SyntaxNode targetNode = root.FindNode( - diagnostic.Location.SourceSpan, - getInnermostNodeForTie: true); + TextSpan diagnosticSpan = diagnostic.Location.SourceSpan; + SyntaxNode targetNode = root.FindNode(diagnosticSpan, getInnermostNodeForTie: true); string title = CodeFixTitle.ToString(); // Attribute applications are deliberately not suppressible under the updated language rules. @@ -76,27 +92,23 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (TryGetConstructorRequiringUnsafe(diagnostic, targetNode) is { } constructor) { - context.RegisterCodeFix( - CodeAction.Create( - title, - cancellationToken => UnsafeModifierCodeFixHelpers.SetUnsafeModifierAsync( - context.Document, - constructor, - cancellationToken), - title), - diagnostic); + RegisterFix( + context, + title, + cancellationToken => MarkConstructorUnsafeAsync( + context.Document, + constructor, + cancellationToken)); return; } if (diagnostic.Id == UnsafeConstructorConstraintDiagnosticId && targetNode.AncestorsAndSelf().OfType().FirstOrDefault() is { } usingDirective) { - context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(AddUnsafeToUsingDirective(context.Document, root, usingDirective)), - title), - diagnostic); + RegisterFix( + context, + title, + _ => Task.FromResult(AddUnsafeToUsingDirective(context.Document, root, usingDirective))); return; } @@ -104,52 +116,34 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (semanticModel is null) return; - if (IsExpressionOnlyContext(targetNode, diagnostic.Location.SourceSpan) - && TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } expressionOnly) + if (IsExpressionOnlyContext(targetNode, diagnosticSpan)) { - RegisterExpressionFix( - context, - diagnostic, - root, - semanticModel, - expressionOnly, - title); + TryRegisterExpressionFix(context, root, semanticModel, targetNode, diagnosticSpan, title); return; } ArrowExpressionClauseSyntax? arrowExpression = targetNode.AncestorsAndSelf() .OfType() - .FirstOrDefault(arrow => arrow.Expression.FullSpan.Contains(diagnostic.Location.SourceSpan)); + .FirstOrDefault(arrow => arrow.Expression.FullSpan.Contains(diagnosticSpan)); if (arrowExpression is not null) { if (arrowExpression.Parent is AnonymousFunctionExpressionSyntax || arrowExpression.Expression.DescendantNodesAndSelf().OfType().Any() || HasDirectiveWithinSpan(arrowExpression)) { - if (TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } expression) - { - RegisterExpressionFix( - context, - diagnostic, - root, - semanticModel, - expression, - title); - } + TryRegisterExpressionFix(context, root, semanticModel, targetNode, diagnosticSpan, title); } else { - context.RegisterCodeFix( - CodeAction.Create( - title, - cancellationToken => ConvertExpressionBodyAsync( - context.Document, - root, - semanticModel, - arrowExpression, - cancellationToken), - title), - diagnostic); + RegisterFix( + context, + title, + cancellationToken => ConvertExpressionBodyAsync( + context.Document, + root, + semanticModel, + arrowExpression, + cancellationToken)); } return; } @@ -162,16 +156,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (containingStatement is null) { - if (TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } expression) - { - RegisterExpressionFix( - context, - diagnostic, - root, - semanticModel, - expression, - title); - } + TryRegisterExpressionFix(context, root, semanticModel, targetNode, diagnosticSpan, title); return; } @@ -185,75 +170,38 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) out LocalDeclarationStatementSyntax forwardDeclaration, out StatementSyntax assignmentStatement)) { - context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(ReplaceStatementWithStatements( - context.Document, - root, - localDeclaration, - [forwardDeclaration, CreateUnsafeStatement(assignmentStatement)])), - title), - diagnostic); + RegisterFix( + context, + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + localDeclaration, + [forwardDeclaration, CreateUnsafeStatement(assignmentStatement)]))); return; } - if (TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } localExpression - && CanUseUnsafeExpression(localExpression)) - { - RegisterExpressionFix( - context, - diagnostic, - root, - semanticModel, - localExpression, - title); + if (TryRegisterExpressionFix(context, root, semanticModel, targetNode, diagnosticSpan, title)) return; - } if (!localDeclaration.AwaitKeyword.IsKind(SyntaxKind.None)) return; if (TryGetEnclosingSwitch(localDeclaration) is { } localSwitch) { - if (!ContainsAwaitOrYield(localSwitch)) - { - context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(ReplaceStatementWithStatements( - context.Document, - root, - localSwitch, - [CreateUnsafeStatement(localSwitch)])), - title), - diagnostic); - } + RegisterEnclosingSwitchFix(context, root, localSwitch, title); return; } - if (TryGetStatementRangeToWrap( - localDeclaration, - semanticModel, - context.CancellationToken, - forceThroughContainerEnd: IsUsingDeclaration(localDeclaration), - out SyntaxNode localContainer, - out int localStart, - out int localEnd) - && CanWrapStatementRange( - localContainer, - localStart, - localEnd, - semanticModel, - context.CancellationToken)) + if (StatementRange.TryCreateForStatement(localDeclaration, out StatementRange localRange, out int localIndex)) { - RegisterStatementRangeFix( + TryRegisterStatementRangeFix( context, - diagnostic, root, - localContainer, - localStart, - localEnd, + semanticModel, + localRange, + localIndex, + forceThroughContainerEnd: IsUsingDeclaration(localDeclaration), title); } return; @@ -283,47 +231,26 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) context.CancellationToken); if ((cannotContainUnsafeStatement || preferExpression) - && TryGetUnsafeExpression(targetNode, diagnostic.Location.SourceSpan) is { } statementExpression - && CanUseUnsafeExpression(statementExpression)) + && TryRegisterExpressionFix(context, root, semanticModel, targetNode, diagnosticSpan, title)) { - RegisterExpressionFix( - context, - diagnostic, - root, - semanticModel, - statementExpression, - title); return; } - if (topLevelScopeSensitive - && containingStatement.Parent is GlobalStatementSyntax - { - Parent: CompilationUnitSyntax compilationUnit, - } globalStatement) + bool hasStatementList = StatementRange.TryCreateForStatement( + containingStatement, + out StatementRange statements, + out int statementIndex); + + if (topLevelScopeSensitive && hasStatementList) { - GlobalStatementSyntax[] globalStatements = compilationUnit.Members - .OfType() - .ToArray(); - int start = System.Array.IndexOf(globalStatements, globalStatement); - int end = globalStatements.Length - 1; - if (start >= 0 - && CanWrapStatementRange( - compilationUnit, - start, - end, - semanticModel, - context.CancellationToken)) - { - RegisterStatementRangeFix( - context, - diagnostic, - root, - compilationUnit, - start, - end, - title); - } + TryRegisterStatementRangeFix( + context, + root, + semanticModel, + statements, + statementIndex, + forceThroughContainerEnd: true, + title); return; } @@ -332,112 +259,128 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (preferExpression && TryGetEnclosingSwitch(containingStatement) is { } containingSwitch) { - if (!ContainsAwaitOrYield(containingSwitch)) - { - context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(ReplaceStatementWithStatements( - context.Document, - root, - containingSwitch, - [CreateUnsafeStatement(containingSwitch)])), - title), - diagnostic); - } + RegisterEnclosingSwitchFix(context, root, containingSwitch, title); return; } - if (preferExpression) + if (hasStatementList) { - if (TryGetStatementRangeToWrap( - containingStatement, - semanticModel, - context.CancellationToken, - forceThroughContainerEnd: topLevelScopeSensitive, - out SyntaxNode container, - out int start, - out int end) - && CanWrapStatementRange( - container, - start, - end, - semanticModel, - context.CancellationToken)) - { - RegisterStatementRangeFix(context, diagnostic, root, container, start, end, title); - } + TryRegisterStatementRangeFix( + context, + root, + semanticModel, + statements, + statementIndex, + forceThroughContainerEnd: false, + title); return; } - if (TryGetStatementList( - containingStatement, - out SyntaxNode singleContainer, - out _, - out int singleIndex) - && !CanWrapStatementRange( - singleContainer, - singleIndex, - singleIndex, - semanticModel, - context.CancellationToken)) - { + if (preferExpression) return; - } + // Embedded statements (such as the body of an 'if' without braces) have no statement list to extend. + RegisterFix( + context, + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + containingStatement, + [CreateUnsafeStatement(containingStatement)]))); + } + + private static void RegisterFix( + CodeFixContext context, + string title, + System.Func> createChangedDocument) => context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(ReplaceStatementWithStatements( - context.Document, - root, - containingStatement, - [CreateUnsafeStatement(containingStatement)])), - title), - diagnostic); + CodeAction.Create(title, createChangedDocument, title), + context.Diagnostics); + + private static void RegisterEnclosingSwitchFix( + CodeFixContext context, + SyntaxNode root, + SwitchStatementSyntax switchStatement, + string title) + { + if (ContainsAwaitOrYield(switchStatement)) + return; + + RegisterFix( + context, + title, + _ => Task.FromResult(ReplaceStatementWithStatements( + context.Document, + root, + switchStatement, + [CreateUnsafeStatement(switchStatement)]))); } - private static void RegisterExpressionFix( + private static bool TryRegisterExpressionFix( CodeFixContext context, - Diagnostic diagnostic, SyntaxNode root, SemanticModel semanticModel, - ExpressionSyntax expression, + SyntaxNode targetNode, + TextSpan diagnosticSpan, string title) { - context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(ReplaceWithUnsafeExpression( - context.Document, - root, - semanticModel, - expression, - context.CancellationToken)), - title), - diagnostic); + if (TryGetUnsafeExpressionFix( + targetNode, + diagnosticSpan, + semanticModel, + context.CancellationToken) is not { } fix) + { + return false; + } + + (ExpressionSyntax expression, ExpressionSyntax operand) = fix; + RegisterFix( + context, + title, + _ => Task.FromResult(ReplaceWithUnsafeExpression(context.Document, root, expression, operand))); + return true; } - private static void RegisterStatementRangeFix( + private static bool TryRegisterStatementRangeFix( CodeFixContext context, - Diagnostic diagnostic, SyntaxNode root, - SyntaxNode container, - int start, - int end, + SemanticModel semanticModel, + in StatementRange statements, + int statementIndex, + bool forceThroughContainerEnd, string title) { - context.RegisterCodeFix( - CodeAction.Create( - title, - _ => Task.FromResult(WrapStatementRange( - context.Document, - root, - container, - start, - end)), - title), - diagnostic); + if (!TryGetStatementRangeToWrap( + statements, + statementIndex, + semanticModel, + context.CancellationToken, + forceThroughContainerEnd, + out int start, + out int end)) + { + return false; + } + + (int mergedStart, int mergedEnd) = ExtendRangeOverGeneratedUnsafeStatements(statements, start, end); + if (mergedStart != start || mergedEnd != end) + { + if (CanWrapStatementRange(statements, mergedStart, mergedEnd, semanticModel, context.CancellationToken)) + { + (start, end) = (mergedStart, mergedEnd); + } + } + + if (!CanWrapStatementRange(statements, start, end, semanticModel, context.CancellationToken)) + return false; + + StatementRange range = statements; + RegisterFix( + context, + title, + _ => Task.FromResult(WrapStatementRange(context.Document, root, range, start, end))); + return true; } private static ConstructorDeclarationSyntax? TryGetConstructorRequiringUnsafe( @@ -463,6 +406,24 @@ private static void RegisterStatementRangeFix( return null; } + /// + /// An unsafe constructor is the only way to establish an unsafe context for its initializer, so this widens + /// the constructor's own contract and is documented like any other caller-unsafe declaration. + /// + private static async Task MarkConstructorUnsafeAsync( + Document document, + ConstructorDeclarationSyntax constructor, + CancellationToken cancellationToken) + { + document = await UnsafeModifierCodeFixHelpers.SetUnsafeModifierAsync( + document, + constructor, + cancellationToken).ConfigureAwait(false); + return await UnsafeModifierCodeFixHelpers.AddPendingSafetyDocumentationAsync( + document, + cancellationToken).ConfigureAwait(false); + } + private static bool IsExpressionOnlyContext(SyntaxNode targetNode, TextSpan diagnosticSpan) { if (targetNode.AncestorsAndSelf().OfType() @@ -480,6 +441,57 @@ private static bool IsExpressionOnlyContext(SyntaxNode targetNode, TextSpan diag return false; } + private static bool SupportsUnsafeExpressions => s_unsafeExpressionRawKind >= 0; + + private static bool IsUnsafeExpression(SyntaxNode node) => + SupportsUnsafeExpressions && node.RawKind == s_unsafeExpressionRawKind; + + private static int GetUnsafeExpressionRawKind() + { + ExpressionSyntax probe = ParseUnsafeExpression(UnsafeExpressionPlaceholder); + return !probe.IsKind(SyntaxKind.IdentifierName) && TryFindPlaceholder(probe) is not null + ? probe.RawKind + : -1; + } + + private static ExpressionSyntax ParseUnsafeExpression(string inner) => + SyntaxFactory.ParseExpression( + $"unsafe(/* SAFETY: Audit */ {inner})", + options: s_unsafeExpressionParseOptions); + + private static IdentifierNameSyntax? TryFindPlaceholder(ExpressionSyntax expression) => + expression.DescendantNodesAndSelf() + .OfType() + .FirstOrDefault(static identifier => identifier.Identifier.ValueText == UnsafeExpressionPlaceholder); + + /// + /// Builds the expression-level fix, or returns when no semantics-preserving unsafe + /// expression can be written for the reported operation. + /// + private static (ExpressionSyntax Expression, ExpressionSyntax Operand)? TryGetUnsafeExpressionFix( + SyntaxNode targetNode, + TextSpan diagnosticSpan, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + if (!SupportsUnsafeExpressions + || TryGetUnsafeExpression(targetNode, diagnosticSpan) is not { } expression + || !CanUseUnsafeExpression(expression)) + { + return null; + } + + // Reaching here from inside an unsafe expression means the compiler does not treat that expression as an + // unsafe context for this operation, so wrapping it again would only nest without fixing anything. + if (expression.AncestorsAndSelf().Any(IsUnsafeExpression)) + return null; + + if (TryGetUnsafeExpressionOperand(semanticModel, expression, cancellationToken) is not { } operand) + return null; + + return (expression, operand); + } + private static ExpressionSyntax? TryGetUnsafeExpression(SyntaxNode targetNode, TextSpan diagnosticSpan) => PromoteUnsafeExpression( targetNode.AncestorsAndSelf() @@ -533,9 +545,151 @@ ForStatementSyntax forStatement when forStatement.Initializers.Contains(expression) || forStatement.Incrementors.Contains(expression) => false, AttributeArgumentSyntax => false, + // An unsafe expression cannot be the target of an assignment or an increment, and wrapping the right + // side of a deconstruction does not put the generated Deconstruct call in an unsafe context. + AssignmentExpressionSyntax assignment => + assignment.Left != expression + && assignment.Left is not (DeclarationExpressionSyntax or TupleExpressionSyntax), + PrefixUnaryExpressionSyntax prefix => + !prefix.IsKind(SyntaxKind.PreIncrementExpression) + && !prefix.IsKind(SyntaxKind.PreDecrementExpression), + PostfixUnaryExpressionSyntax => false, _ => true, }); + /// + /// Produces the expression to place inside unsafe(...). Conversions, property and indexer accessors, + /// and method group conversions are bound by the enclosing binder, so they only enter the unsafe context when + /// an explicit cast forces them inside it. + /// + private static ExpressionSyntax? TryGetUnsafeExpressionOperand( + SemanticModel semanticModel, + ExpressionSyntax expression, + CancellationToken cancellationToken) + { + ExpressionSyntax operand = expression.WithoutLeadingTrivia().WithoutTrailingTrivia(); + if (TryGetRequiredCastType(semanticModel, expression, cancellationToken, out bool requiresCast) is { } castType) + { + if (TryParseTypeName(semanticModel, castType, expression.SpanStart) is not { } castTypeSyntax) + return null; + + if (NeedsParenthesesForCast(operand)) + operand = SyntaxFactory.ParenthesizedExpression(operand); + + return SyntaxFactory.CastExpression(castTypeSyntax, operand); + } + + return requiresCast ? null : operand; + } + + private static ITypeSymbol? TryGetRequiredCastType( + SemanticModel semanticModel, + ExpressionSyntax expression, + CancellationToken cancellationToken, + out bool requiresCast) + { + requiresCast = true; + TypeInfo typeInfo = semanticModel.GetTypeInfo(expression, cancellationToken); + + // A user-defined implicit conversion is applied to the result of the unsafe expression; casting to its + // result type also pulls any receiver evaluation inside, so it is checked first. + Conversion conversion = semanticModel.GetConversion(expression, cancellationToken); + if (conversion.IsUserDefined + && conversion.IsImplicit + && conversion.MethodSymbol is { } conversionOperator) + { + return GetUserDefinedConversionResultType( + semanticModel, + expression, + conversionOperator, + cancellationToken); + } + + ISymbol? symbol = semanticModel.GetSymbolInfo(expression, cancellationToken).Symbol; + if (typeInfo.Type is null && symbol is IMethodSymbol) + { + return typeInfo.ConvertedType is INamedTypeSymbol { TypeKind: TypeKind.Delegate } delegateType + ? delegateType + : null; + } + + if (symbol is IPropertySymbol property) + { + // A cast would drop the reference, so a ref-returning accessor cannot be fixed this way. + return property.RefKind == RefKind.None + ? WithFlowNullability(typeInfo, property.Type) + : null; + } + + requiresCast = false; + return null; + } + + private static ITypeSymbol WithFlowNullability(TypeInfo typeInfo, ITypeSymbol type) => + typeInfo.Nullability.FlowState == NullableFlowState.NotNull && type.IsReferenceType + ? type.WithNullableAnnotation(NullableAnnotation.NotAnnotated) + : type; + + private static bool NeedsParenthesesForCast(ExpressionSyntax expression) => + expression is BinaryExpressionSyntax + or AssignmentExpressionSyntax + or ConditionalExpressionSyntax + or IsPatternExpressionSyntax + or SwitchExpressionSyntax + or AnonymousFunctionExpressionSyntax + or QueryExpressionSyntax + or RangeExpressionSyntax + or WithExpressionSyntax + or ThrowExpressionSyntax + // A cast followed by '&', '*', '+' or '-' is parsed as a binary operator when the cast type is also a + // valid expression, so unary operands always get parentheses. + or PrefixUnaryExpressionSyntax; + + /// + /// Renders a type as source, returning for types that cannot be named at that position. + /// + private static TypeSyntax? TryParseTypeName( + SemanticModel semanticModel, + ITypeSymbol type, + int position) + { + if (type is IErrorTypeSymbol || type.SpecialType == SpecialType.System_Void || ContainsAnonymousType(type)) + return null; + + string displayString = type.ToMinimalDisplayString(semanticModel, position, s_localTypeDisplayFormat); + TypeSyntax parsedType = SyntaxFactory.ParseTypeName(displayString); + return parsedType.ContainsDiagnostics || parsedType.ToFullString() != displayString + ? null + : parsedType; + } + + private static ITypeSymbol GetUserDefinedConversionResultType( + SemanticModel semanticModel, + ExpressionSyntax expression, + IMethodSymbol conversionOperator, + CancellationToken cancellationToken) + { + ITypeSymbol resultType = conversionOperator.ReturnType; + ITypeSymbol? sourceType = semanticModel.GetTypeInfo(expression, cancellationToken).Type; + if (sourceType is INamedTypeSymbol sourceNamedType + && sourceNamedType.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T + && sourceNamedType.TypeArguments.Length == 1 + && conversionOperator.Parameters.Length == 1 + && SymbolEqualityComparer.Default.Equals( + sourceNamedType.TypeArguments[0], + conversionOperator.Parameters[0].Type) + && resultType.IsValueType + && resultType.OriginalDefinition.SpecialType != SpecialType.System_Nullable_T) + { + INamedTypeSymbol nullableType = semanticModel.Compilation.GetSpecialType( + SpecialType.System_Nullable_T); + if (nullableType.TypeKind != TypeKind.Error) + resultType = nullableType.Construct(resultType); + } + + return resultType; + } + private static bool TryCreateSplitLocalDeclaration( Diagnostic diagnostic, LocalDeclarationStatementSyntax localDeclaration, @@ -583,11 +737,10 @@ private static bool TryCreateSplitLocalDeclaration( TypeSyntax type = localDeclaration.Declaration.Type; if (type.IsVar) { - type = SyntaxFactory.ParseTypeName( - localSymbol.Type.ToMinimalDisplayString( - semanticModel, - localDeclaration.SpanStart, - s_localTypeDisplayFormat)); + if (TryParseTypeName(semanticModel, localSymbol.Type, localDeclaration.SpanStart) is not { } inferredType) + return false; + + type = inferredType; } if (localSymbol.Type.IsRefLikeType) @@ -637,32 +790,22 @@ private static bool InitializerVariablesEscape( if (analysis is null || !analysis.Succeeded) return true; - ISymbol[] initializerVariables = analysis.VariablesDeclared - .Where(symbol => !SymbolEqualityComparer.Default.Equals(symbol, declaredLocal)) - .ToArray(); - if (initializerVariables.Length == 0 - || !TryGetStatementList( + var initializerVariables = new HashSet( + analysis.VariablesDeclared.Where(symbol => !SymbolEqualityComparer.Default.Equals(symbol, declaredLocal)), + SymbolEqualityComparer.Default); + if (initializerVariables.Count == 0 + || !StatementRange.TryCreateForStatement( localDeclaration, - out SyntaxNode container, - out SyntaxList statements, + out StatementRange statements, out int statementIndex)) { return false; } - IEnumerable laterStatements = Enumerable - .Range(statementIndex + 1, statements.Count - statementIndex - 1) - .Select(index => GetStatement(container, statements, index)); - if (localDeclaration.Parent is SwitchSectionSyntax switchSection - && switchSection.Parent is SwitchStatementSyntax switchStatement) - { - int sectionIndex = switchStatement.Sections.IndexOf(switchSection); - laterStatements = laterStatements.Concat( - switchStatement.Sections - .Skip(sectionIndex + 1) - .SelectMany(static section => section.Statements)); - } - + IEnumerable laterStatements = GetLaterStatements( + statements, + statementIndex, + localDeclaration); return laterStatements.Any(statement => ReferencesAnySymbol( statement, initializerVariables, @@ -670,6 +813,30 @@ private static bool InitializerVariablesEscape( cancellationToken)); } + /// + /// Enumerates the statements that could observe declarations made by , including + /// the remaining sections of an enclosing switch statement. + /// + private static IEnumerable GetLaterStatements( + StatementRange statements, + int statementIndex, + StatementSyntax statement) + { + for (int index = statementIndex + 1; index < statements.Count; index++) + yield return statements[index]; + + if (statement.Parent is SwitchSectionSyntax switchSection + && switchSection.Parent is SwitchStatementSyntax switchStatement) + { + int sectionIndex = switchStatement.Sections.IndexOf(switchSection); + foreach (SwitchSectionSyntax laterSection in switchStatement.Sections.Skip(sectionIndex + 1)) + { + foreach (StatementSyntax laterStatement in laterSection.Statements) + yield return laterStatement; + } + } + } + private static bool IsRefType(TypeSyntax type) => type is RefTypeSyntax or ScopedTypeSyntax { Type: RefTypeSyntax }; @@ -707,7 +874,7 @@ private static bool WouldNarrowDeclaredVariableScope( SemanticModel semanticModel, CancellationToken cancellationToken) { - if (!TryGetStatementList(statement, out _, out SyntaxList statements, out int statementIndex)) + if (!StatementRange.TryCreateForStatement(statement, out StatementRange statements, out int statementIndex)) return false; DataFlowAnalysis? analysis = semanticModel.AnalyzeDataFlow(statement); @@ -726,49 +893,32 @@ private static bool WouldNarrowDeclaredVariableScope( if (declaredSymbols.Count == 0 && declaredNames.Count == 0) return false; - IEnumerable laterStatements = statements.Skip(statementIndex + 1); - if (statement.Parent is SwitchSectionSyntax switchSection - && switchSection.Parent is SwitchStatementSyntax switchStatement) - { - int sectionIndex = switchStatement.Sections.IndexOf(switchSection); - laterStatements = laterStatements.Concat( - switchStatement.Sections - .Skip(sectionIndex + 1) - .SelectMany(static section => section.Statements)); - } - - return laterStatements.Any(laterStatement => + return GetLaterStatements(statements, statementIndex, statement).Any(laterStatement => ReferencesAnySymbol( laterStatement, declaredSymbols, semanticModel, cancellationToken) - || ReferencesAnyName(laterStatement, declaredNames)); + || ReferencesAnyUnresolvedName(laterStatement, declaredNames, semanticModel, cancellationToken)); } private static bool TryGetStatementRangeToWrap( - StatementSyntax triggerStatement, + in StatementRange statements, + int triggerIndex, SemanticModel semanticModel, CancellationToken cancellationToken, bool forceThroughContainerEnd, - out SyntaxNode container, out int start, out int end) { - if (!TryGetStatementList(triggerStatement, out container, out SyntaxList statements, out start)) - { - end = -1; - return false; - } - - end = forceThroughContainerEnd ? statements.Count - 1 : start; + start = triggerIndex; + end = forceThroughContainerEnd ? statements.Count - 1 : triggerIndex; if (forceThroughContainerEnd) return true; while (true) { HashSet? declaredSymbols = GetDeclaredSymbols( - container, statements, start, end, @@ -780,28 +930,26 @@ private static bool TryGetStatementRangeToWrap( return true; } - if (statements.Skip(start).Take(end - start + 1).OfType() - .Any(IsUsingDeclaration)) + // A using declaration disposes at the end of its container, so its unsafe region must reach there. + for (int i = start; i <= end; i++) { - end = statements.Count - 1; - return true; + if (statements[i] is LocalDeclarationStatementSyntax declaration && IsUsingDeclaration(declaration)) + { + end = statements.Count - 1; + return true; + } } - int expandedEnd = end; var selectedStatements = new List(end - start + 1); for (int i = start; i <= end; i++) - selectedStatements.Add(GetStatement(container, statements, i)); + selectedStatements.Add(statements[i]); HashSet declaredNames = GetDeclaredNames(selectedStatements); + + int expandedEnd = end; for (int i = end + 1; i < statements.Count; i++) { - if (ReferencesAnySymbol( - GetStatement(container, statements, i), - declaredSymbols, - semanticModel, - cancellationToken) - || ReferencesAnyName( - GetStatement(container, statements, i), - declaredNames)) + if (ReferencesAnySymbol(statements[i], declaredSymbols, semanticModel, cancellationToken) + || ReferencesAnyUnresolvedName(statements[i], declaredNames, semanticModel, cancellationToken)) { expandedEnd = i; } @@ -814,70 +962,122 @@ private static bool TryGetStatementRangeToWrap( } } - private static bool TryGetStatementList( - StatementSyntax statement, - out SyntaxNode container, - out SyntaxList statements, - out int statementIndex) + /// + /// Grows a range so that it absorbs neighbouring unsafe regions that this fixer generated and has not yet been + /// audited, which keeps a migrated member from accumulating a long run of tiny unsafe blocks. + /// Statements between those regions are only absorbed when they declare nothing, because a declaration moved + /// into the merged block would no longer be visible to the statements that follow it. + /// + private static (int Start, int End) ExtendRangeOverGeneratedUnsafeStatements( + in StatementRange statements, + int start, + int end) { - switch (statement.Parent) + int gap = 0; + for (int index = start - 1; index >= 0; index--) { - case BlockSyntax block: - container = block; - statements = block.Statements; - statementIndex = statements.IndexOf(statement); - return statementIndex >= 0; + if (IsGeneratedUnsafeStatement(statements[index])) + { + start = index; + gap = 0; + continue; + } - case SwitchSectionSyntax switchSection: - container = switchSection; - statements = switchSection.Statements; - statementIndex = statements.IndexOf(statement); - return statementIndex >= 0; + if (gap == MaxSafeStatementsBetweenMergedRegions || GetDeclaredNames(statements[index]).Count > 0) + break; - case GlobalStatementSyntax - { - Parent: CompilationUnitSyntax compilationUnit, - }: - container = compilationUnit; - statements = SyntaxFactory.List( - compilationUnit.Members - .OfType() - .Select(static globalStatement => globalStatement.Statement)); - statementIndex = statements.IndexOf(statement); - return statementIndex >= 0; + gap++; + } - default: - container = null!; - statements = default; - statementIndex = -1; - return false; + gap = 0; + for (int index = end + 1; index < statements.Count; index++) + { + if (IsGeneratedUnsafeStatement(statements[index])) + { + end = index; + gap = 0; + continue; + } + + if (gap == MaxSafeStatementsBetweenMergedRegions || GetDeclaredNames(statements[index]).Count > 0) + break; + + gap++; + } + + return (start, end); + } + + /// + /// Recognizes an unsafe statement that this fixer produced and that still carries the unaudited marker. + /// Blocks whose marker was replaced during review are left alone. + /// + private static bool IsGeneratedUnsafeStatement(StatementSyntax statement) => + statement is UnsafeStatementSyntax unsafeStatement + && unsafeStatement.Block.Statements.Count > 0 + && HasSafetyAuditMarker(unsafeStatement.Block.Statements[0]); + + private static bool HasSafetyAuditMarker(SyntaxNode node) => + node.GetLeadingTrivia().Any(static trivia => + trivia.IsKind(SyntaxKind.SingleLineCommentTrivia) + && trivia.ToString() == SafetyAuditComment); + + private static StatementSyntax RemoveSafetyAuditMarker(StatementSyntax statement) + { + SyntaxTriviaList leadingTrivia = statement.GetLeadingTrivia(); + for (int index = 0; index < leadingTrivia.Count; index++) + { + if (!leadingTrivia[index].IsKind(SyntaxKind.SingleLineCommentTrivia) + || leadingTrivia[index].ToString() != SafetyAuditComment) + { + continue; + } + + int count = index + 1 < leadingTrivia.Count && leadingTrivia[index + 1].IsKind(SyntaxKind.EndOfLineTrivia) + ? 2 + : 1; + return statement.WithLeadingTrivia( + leadingTrivia.Take(index).Concat(leadingTrivia.Skip(index + count))); } + + return statement; + } + + /// + /// Replaces the line break that separated a flattened statement from its former closing brace with an elastic + /// one, so the formatter lays the merged block out instead of inheriting the nested indentation. + /// + private static StatementSyntax NormalizeTrailingEndOfLine(StatementSyntax statement) + { + SyntaxTriviaList trailingTrivia = statement.GetTrailingTrivia(); + int count = trailingTrivia.Count; + while (count > 0 && trailingTrivia[count - 1].IsKind(SyntaxKind.EndOfLineTrivia)) + count--; + + return statement.WithTrailingTrivia( + trailingTrivia.Take(count).Concat([SyntaxFactory.ElasticCarriageReturnLineFeed])); } private static HashSet? GetDeclaredSymbols( - SyntaxNode container, - SyntaxList statements, + in StatementRange statements, int start, int end, SemanticModel semanticModel, CancellationToken cancellationToken) { var symbols = new HashSet(SymbolEqualityComparer.Default); - if (container is CompilationUnitSyntax) + + // Top-level statements are separate members, so they can only be analyzed one at a time. + if (statements.IsTopLevel) { for (int i = start; i <= end; i++) { - StatementSyntax statement = GetStatement(container, statements, i); - DataFlowAnalysis? statementAnalysis = semanticModel.AnalyzeDataFlow(statement); + DataFlowAnalysis? statementAnalysis = semanticModel.AnalyzeDataFlow(statements[i]); if (statementAnalysis is null || !statementAnalysis.Succeeded) return null; symbols.UnionWith(statementAnalysis.VariablesDeclared); - AddSyntacticallyDeclaredSymbols( - statement, - symbols, - semanticModel, - cancellationToken); + AddSyntacticallyDeclaredSymbols(statements[i], symbols, semanticModel, cancellationToken); } return symbols; @@ -889,13 +1089,8 @@ private static bool TryGetStatementList( symbols.UnionWith(analysis.VariablesDeclared); for (int i = start; i <= end; i++) - { - AddSyntacticallyDeclaredSymbols( - GetStatement(container, statements, i), - symbols, - semanticModel, - cancellationToken); - } + AddSyntacticallyDeclaredSymbols(statements[i], symbols, semanticModel, cancellationToken); + return symbols; } @@ -922,20 +1117,15 @@ private static void AddSyntacticallyDeclaredSymbols( private static bool ReferencesAnySymbol( SyntaxNode node, - IEnumerable symbols, + HashSet symbols, SemanticModel semanticModel, - CancellationToken cancellationToken) - { - var symbolSet = new HashSet(symbols, SymbolEqualityComparer.Default); - if (symbolSet.Count == 0) - return false; - - return node.DescendantNodesAndSelf() + CancellationToken cancellationToken) => + symbols.Count > 0 + && node.DescendantNodesAndSelf() .OfType() .Any(name => semanticModel.GetSymbolInfo(name, cancellationToken).Symbol is { } symbol - && symbolSet.Contains(symbol)); - } + && symbols.Contains(symbol)); private static HashSet GetDeclaredNames(SyntaxNode node) => GetDeclaredNames([node]); @@ -961,13 +1151,22 @@ private static HashSet GetDeclaredNames(IEnumerable nodes) return names; } - private static bool ReferencesAnyName( + /// + /// Falls back to matching identifier text for names the semantic model cannot resolve. The document contains + /// compiler errors by definition here, so this keeps ranges conservative without widening them whenever an + /// unrelated member happens to share a local's name. + /// + private static bool ReferencesAnyUnresolvedName( SyntaxNode node, - HashSet names) => + HashSet names, + SemanticModel semanticModel, + CancellationToken cancellationToken) => names.Count > 0 && node.DescendantNodesAndSelf() .OfType() - .Any(name => names.Contains(name.Identifier.ValueText)); + .Any(name => + names.Contains(name.Identifier.ValueText) + && semanticModel.GetSymbolInfo(name, cancellationToken).Symbol is null); private static bool HasDirectiveWithinSpan(SyntaxNode node) => node.DescendantTrivia(descendIntoTrivia: true) @@ -990,93 +1189,28 @@ private static Document AddUnsafeToUsingDirective( private static Document ReplaceWithUnsafeExpression( Document document, SyntaxNode root, - SemanticModel semanticModel, ExpressionSyntax expression, - CancellationToken cancellationToken) + ExpressionSyntax operand) { - ExpressionSyntax operand = expression.WithoutLeadingTrivia().WithoutTrailingTrivia(); - Conversion conversion = semanticModel.GetConversion(expression, cancellationToken); - if (conversion.IsUserDefined - && conversion.IsImplicit - && conversion.MethodSymbol is { } conversionOperator) - { - ITypeSymbol convertedType = GetUserDefinedConversionResultType( - semanticModel, - expression, - conversionOperator, - cancellationToken); - TypeSyntax convertedTypeSyntax = SyntaxFactory.ParseTypeName( - convertedType.ToMinimalDisplayString( - semanticModel, - expression.SpanStart, - s_localTypeDisplayFormat)); - operand = SyntaxFactory.CastExpression(convertedTypeSyntax, operand); - } - - ExpressionSyntax template = SyntaxFactory.ParseExpression( - $"unsafe(/* SAFETY: Audit */ {UnsafeExpressionPlaceholder})"); - IdentifierNameSyntax placeholder = template.DescendantNodesAndSelf() - .OfType() - .Single(identifier => identifier.Identifier.ValueText == UnsafeExpressionPlaceholder); - + ExpressionSyntax template = ParseUnsafeExpression(UnsafeExpressionPlaceholder); + IdentifierNameSyntax placeholder = TryFindPlaceholder(template)!; ExpressionSyntax replacement = template - .ReplaceNode( - placeholder, - operand) + .ReplaceNode(placeholder, operand) .WithLeadingTrivia(expression.GetLeadingTrivia()) .WithTrailingTrivia(expression.GetTrailingTrivia()); return document.WithSyntaxRoot(root.ReplaceNode(expression, replacement)); } - private static ITypeSymbol GetUserDefinedConversionResultType( - SemanticModel semanticModel, - ExpressionSyntax expression, - IMethodSymbol conversionOperator, - CancellationToken cancellationToken) - { - ITypeSymbol resultType = conversionOperator.ReturnType; - ITypeSymbol? sourceType = semanticModel.GetTypeInfo(expression, cancellationToken).Type; - if (sourceType is INamedTypeSymbol sourceNamedType - && sourceNamedType.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T - && sourceNamedType.TypeArguments.Length == 1 - && conversionOperator.Parameters.Length == 1 - && SymbolEqualityComparer.Default.Equals( - sourceNamedType.TypeArguments[0], - conversionOperator.Parameters[0].Type) - && resultType.IsValueType - && resultType.OriginalDefinition.SpecialType != SpecialType.System_Nullable_T) - { - INamedTypeSymbol nullableType = semanticModel.Compilation.GetSpecialType( - SpecialType.System_Nullable_T); - if (nullableType.TypeKind != TypeKind.Error) - resultType = nullableType.Construct(resultType); - } - - return resultType; - } - private static bool CanWrapStatementRange( - SyntaxNode container, + in StatementRange statements, int start, int end, SemanticModel semanticModel, CancellationToken cancellationToken) { - SyntaxList statements = container switch - { - BlockSyntax block => block.Statements, - SwitchSectionSyntax switchSection => switchSection.Statements, - CompilationUnitSyntax compilationUnit => SyntaxFactory.List( - compilationUnit.Members - .OfType() - .Select(static globalStatement => globalStatement.Statement)), - _ => default, - }; - IEnumerable selectedStatements = statements - .Select((statement, index) => GetStatement(container, statements, index)) - .Skip(start) - .Take(end - start + 1) - .ToArray(); + var selectedStatements = new List(end - start + 1); + for (int i = start; i <= end; i++) + selectedStatements.Add(statements[i]); if (selectedStatements.Any(ContainsAwaitOrYield)) return false; @@ -1084,51 +1218,44 @@ private static bool CanWrapStatementRange( if (end > start && selectedStatements.Any(static statement => statement.ContainsDirectives)) return false; - ISymbol[] localFunctions = selectedStatements - .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) - .Select(localFunction => semanticModel.GetDeclaredSymbol(localFunction, cancellationToken)) - .OfType() - .ToArray(); - if (localFunctions.Length > 0 - && statements.Where((_, index) => index < start || index > end) - .Any(statement => ReferencesAnySymbol( - statement, - localFunctions, - semanticModel, - cancellationToken))) - { - return false; - } - + var localFunctions = new HashSet( + selectedStatements + .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) + .Select(localFunction => semanticModel.GetDeclaredSymbol(localFunction, cancellationToken)) + .OfType(), + SymbolEqualityComparer.Default); string[] labels = selectedStatements .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) .Select(static label => label.Identifier.ValueText) .Distinct() .ToArray(); - if (labels.Length > 0 - && statements.Where((_, index) => index < start || index > end) - .SelectMany(static statement => statement.DescendantNodesAndSelf().OfType()) - .Any(gotoStatement => + if (localFunctions.Count == 0 && labels.Length == 0) + return true; + + for (int i = 0; i < statements.Count; i++) + { + if (i >= start && i <= end) + continue; + + StatementSyntax statement = statements[i]; + + // Moving a local function into a nested block hides it from the rest of the container. + if (ReferencesAnySymbol(statement, localFunctions, semanticModel, cancellationToken)) + return false; + + // A goto cannot jump into an unsafe block. + if (labels.Length > 0 + && statement.DescendantNodesAndSelf().OfType().Any(gotoStatement => gotoStatement.Expression is IdentifierNameSyntax identifier && labels.Contains(identifier.Identifier.ValueText))) - { - return false; + { + return false; + } } return true; } - private static StatementSyntax GetStatement( - SyntaxNode container, - SyntaxList statements, - int index) => - container is CompilationUnitSyntax compilationUnit - ? compilationUnit.Members - .OfType() - .ElementAt(index) - .Statement - : statements[index]; - private static SwitchStatementSyntax? TryGetEnclosingSwitch(StatementSyntax statement) => statement.AncestorsAndSelf() .OfType() @@ -1276,7 +1403,7 @@ private static UnsafeStatementSyntax CreateUnsafeStatement(StatementSyntax state .WithoutLeadingTrivia() .WithoutTrailingTrivia() .WithLeadingTrivia( - SyntaxFactory.Comment("// SAFETY: Audit"), + SyntaxFactory.Comment(SafetyAuditComment), SyntaxFactory.ElasticCarriageReturnLineFeed); return SyntaxFactory.UnsafeStatement(SyntaxFactory.Block(innerStatement)) .WithLeadingTrivia(statement.GetLeadingTrivia()) @@ -1327,78 +1454,140 @@ private static Document ReplaceStatementWithStatements( private static Document WrapStatementRange( Document document, SyntaxNode root, - SyntaxNode container, + in StatementRange statements, int start, int end) { - SyntaxList statements = container switch + var statementsToWrap = new List(end - start + 1); + for (int i = start; i <= end; i++) { - BlockSyntax block => block.Statements, - SwitchSectionSyntax switchSection => switchSection.Statements, - CompilationUnitSyntax compilationUnit => SyntaxFactory.List( - compilationUnit.Members - .OfType() - .Select(static globalStatement => globalStatement.Statement)), - _ => default, - }; - StatementSyntax[] statementsToWrap = statements - .Skip(start) - .Take(end - start + 1) - .ToArray(); + StatementSyntax statement = statements[i]; + + // Merged regions are flattened so the result is one audit scope instead of nested unsafe blocks. + if (IsGeneratedUnsafeStatement(statement)) + { + UnsafeStatementSyntax generated = (UnsafeStatementSyntax)statement; + StatementSyntax firstInnerStatement = RemoveSafetyAuditMarker(generated.Block.Statements[0]); + statementsToWrap.Add( + NormalizeTrailingEndOfLine(firstInnerStatement) + .WithLeadingTrivia(generated.GetLeadingTrivia() + .AddRange(firstInnerStatement.GetLeadingTrivia()))); + statementsToWrap.AddRange(generated.Block.Statements.Skip(1).Select(NormalizeTrailingEndOfLine)); + continue; + } + + statementsToWrap.Add(statement); + } StatementSyntax firstStatement = statementsToWrap[0]; - StatementSyntax firstInnerStatement = firstStatement + statementsToWrap[0] = firstStatement .WithoutLeadingTrivia() .WithLeadingTrivia( - SyntaxFactory.Comment("// SAFETY: Audit"), + SyntaxFactory.Comment(SafetyAuditComment), SyntaxFactory.ElasticCarriageReturnLineFeed); - statementsToWrap[0] = firstInnerStatement; UnsafeStatementSyntax unsafeStatement = SyntaxFactory.UnsafeStatement( SyntaxFactory.Block(statementsToWrap)) .WithLeadingTrivia(firstStatement.GetLeadingTrivia()) .WithAdditionalAnnotations(Formatter.Annotation); - var replacementList = new List(statements.Count - (end - start)); - for (int i = 0; i < statements.Count; i++) + SyntaxNode replacementContainer = statements.ReplaceRange(start, end, unsafeStatement); + return document.WithSyntaxRoot(root.ReplaceNode(statements.Container, replacementContainer)); + } + + /// + /// Provides uniform access to the statement list that contains a statement, including the top-level statement + /// list of a compilation unit, whose statements are wrapped in members. + /// + private readonly struct StatementRange + { + private readonly ImmutableArray _statements; + + private StatementRange(SyntaxNode container, ImmutableArray statements) { - if (i == start) - replacementList.Add(unsafeStatement); - if (i < start || i > end) - replacementList.Add(statements[i]); + Container = container; + _statements = statements; } - SyntaxList replacementStatements = SyntaxFactory.List(replacementList); - SyntaxNode replacementContainer = container switch + + internal SyntaxNode Container { get; } + + internal int Count => _statements.Length; + + internal StatementSyntax this[int index] => _statements[index]; + + internal bool IsTopLevel => Container is CompilationUnitSyntax; + + internal static bool TryCreateForStatement( + StatementSyntax statement, + out StatementRange statements, + out int statementIndex) { - BlockSyntax block => block.WithStatements(replacementStatements), - SwitchSectionSyntax switchSection => switchSection.WithStatements(replacementStatements), - CompilationUnitSyntax compilationUnit => ReplaceGlobalStatements( - compilationUnit, - start, - end, - unsafeStatement), - _ => container, - }; - return document.WithSyntaxRoot(root.ReplaceNode(container, replacementContainer)); - } + switch (statement.Parent) + { + case BlockSyntax block: + statements = new StatementRange(block, [.. block.Statements]); + break; - private static CompilationUnitSyntax ReplaceGlobalStatements( - CompilationUnitSyntax compilationUnit, - int start, - int end, - UnsafeStatementSyntax unsafeStatement) - { - GlobalStatementSyntax[] globalStatements = compilationUnit.Members - .OfType() - .ToArray(); - int memberIndex = compilationUnit.Members.IndexOf(globalStatements[start]); - SyntaxList members = compilationUnit.Members; - for (int i = start; i <= end; i++) - members = members.RemoveAt(memberIndex); + case SwitchSectionSyntax switchSection: + statements = new StatementRange(switchSection, [.. switchSection.Statements]); + break; + + case GlobalStatementSyntax { Parent: CompilationUnitSyntax compilationUnit }: + statements = new StatementRange( + compilationUnit, + [.. compilationUnit.Members.OfType().Select(static global => global.Statement)]); + break; + + default: + statements = default; + statementIndex = -1; + return false; + } + + statementIndex = statements._statements.IndexOf(statement); + return statementIndex >= 0; + } - return compilationUnit.WithMembers(members.Insert( - memberIndex, - SyntaxFactory.GlobalStatement(unsafeStatement))); + /// + /// Replaces the statements in [start, end] with a single statement. + /// + internal SyntaxNode ReplaceRange(int start, int end, StatementSyntax replacement) + { + if (Container is CompilationUnitSyntax compilationUnit) + { + SyntaxList members = compilationUnit.Members; + GlobalStatementSyntax[] globalStatements = members.OfType().ToArray(); + + // Resolve every member index before editing: removing a member rebuilds the list, and top-level + // statements are not guaranteed to be adjacent members. + int[] memberIndices = new int[end - start + 1]; + for (int i = start; i <= end; i++) + memberIndices[i - start] = members.IndexOf(globalStatements[i]); + + for (int i = memberIndices.Length - 1; i >= 0; i--) + members = members.RemoveAt(memberIndices[i]); + + return compilationUnit.WithMembers( + members.Insert(memberIndices[0], SyntaxFactory.GlobalStatement(replacement))); + } + + var replacementStatements = new List(Count - (end - start)); + for (int i = 0; i < Count; i++) + { + if (i == start) + replacementStatements.Add(replacement); + if (i < start || i > end) + replacementStatements.Add(_statements[i]); + } + + SyntaxList statements = SyntaxFactory.List(replacementStatements); + return Container switch + { + BlockSyntax block => block.WithStatements(statements), + SwitchSectionSyntax switchSection => switchSection.WithStatements(statements), + _ => Container, + }; + } } } } diff --git a/src/tools/illink/src/ILLink.CodeFix/Resources.resx b/src/tools/illink/src/ILLink.CodeFix/Resources.resx index ebcab064ea3dee..70bf39f119c41d 100644 --- a/src/tools/illink/src/ILLink.CodeFix/Resources.resx +++ b/src/tools/illink/src/ILLink.CodeFix/Resources.resx @@ -147,6 +147,9 @@ Propagate unsafe contract + + Propagate unsafe contract (discards explicit 'safe') + Add UnconditionalSuppressMessage attribute to parent method diff --git a/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs index aaf9597f0a5d37..a14303fd69b921 100644 --- a/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs +++ b/src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs @@ -15,7 +15,6 @@ using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.FindSymbols; using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Text; @@ -39,6 +38,12 @@ public sealed class SynchronizeUnsafeContractCodeFixProvider : Microsoft.CodeAna Resources.ResourceManager, typeof(Resources)); + private static LocalizableString DiscardSafeCodeFixTitle => + new LocalizableResourceString( + nameof(Resources.SynchronizeUnsafeContractDiscardingSafeCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + public override ImmutableArray FixableDiagnosticIds => [ UnsafeOverrideDiagnosticId, @@ -90,28 +95,29 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (!IsSupportedContractSymbol(symbol)) return; - HashSet closure = await GetContractClosureAsync( - context.Document.Project.Solution, - symbol, - context.CancellationToken).ConfigureAwait(false); + HashSet closure = GetContractClosure(symbol, context.CancellationToken); + Solution solution = context.Document.Project.Solution; if (!CanPropagateUnsafeContract( - context.Document.Project.Solution, + solution, closure, context.CancellationToken)) { return; } - string unsafeTitle = UnsafeCodeFixTitle.ToString(); + // Replacing an explicit safe contract discards a deliberate audit, so it is offered under its own title. + string title = DiscardsExplicitSafeContract(solution, closure, context.CancellationToken) + ? DiscardSafeCodeFixTitle.ToString() + : UnsafeCodeFixTitle.ToString(); context.RegisterCodeFix( CodeAction.Create( - unsafeTitle, + title, cancellationToken => ApplyModifierAsync( - context.Document.Project.Solution, + solution, closure, cancellationToken), - unsafeTitle), - diagnostic); + title), + context.Diagnostics); } private sealed class ContractFixAllProvider : FixAllProvider @@ -170,10 +176,7 @@ private static async Task PropagateDiagnosticsAsync( continue; } - HashSet diagnosticClosure = await GetContractClosureAsync( - solution, - symbol, - cancellationToken).ConfigureAwait(false); + HashSet diagnosticClosure = GetContractClosure(symbol, cancellationToken); if (CanPropagateUnsafeContract( solution, diagnosticClosure, @@ -391,10 +394,14 @@ private static bool CanPropagateUnsafeContract( return true; } - private static async Task> GetContractClosureAsync( - Solution solution, - ISymbol seed, - CancellationToken cancellationToken) + /// + /// Collects the declarations that must become caller-unsafe: the partial parts of the reported member and the + /// base and interface slots it overrides or implements, transitively. + /// Overrides and implementations that are not themselves marked unsafe are deliberately left alone; the + /// language permits a safe member to override or implement a caller-unsafe one, so widening them would grow + /// the audit surface and cascade an unsafe contract onto their callers for no compiler-mandated reason. + /// + private static HashSet GetContractClosure(ISymbol seed, CancellationToken cancellationToken) { var closure = new HashSet(SymbolEqualityComparer.Default); var queue = new Queue(); @@ -402,6 +409,7 @@ private static async Task> GetContractClosureAsync( Enqueue(seed); while (queue.Count > 0) { + cancellationToken.ThrowIfCancellationRequested(); ISymbol symbol = queue.Dequeue(); foreach (ISymbol part in UnsafeContractHelpers.GetPartialParts(symbol)) @@ -412,25 +420,6 @@ private static async Task> GetContractClosureAsync( foreach (ISymbol interfaceMember in UnsafeContractHelpers.GetImplementedInterfaceMembers(symbol)) Enqueue(interfaceMember); - - foreach (ISymbol overridingMember in await SymbolFinder.FindOverridesAsync( - symbol, - solution, - cancellationToken: cancellationToken).ConfigureAwait(false)) - { - Enqueue(overridingMember); - } - - if (symbol.ContainingType?.TypeKind == TypeKind.Interface) - { - foreach (ISymbol implementation in await SymbolFinder.FindImplementationsAsync( - symbol, - solution, - cancellationToken: cancellationToken).ConfigureAwait(false)) - { - Enqueue(implementation); - } - } } return closure; @@ -443,6 +432,29 @@ void Enqueue(ISymbol symbol) } } + /// + /// Determines whether propagation would replace a deliberate safe audit with an unsafe contract. + /// + private static bool DiscardsExplicitSafeContract( + Solution solution, + IEnumerable symbols, + CancellationToken cancellationToken) + { + foreach (ISymbol symbol in symbols) + { + foreach (SyntaxNode declaration in UnsafeContractHelpers.GetDeclarations(symbol, cancellationToken)) + { + if (solution.GetDocument(declaration.SyntaxTree) is not null + && UnsafeMigrationSyntaxHelpers.HasSafeModifier(declaration)) + { + return true; + } + } + } + + return false; + } + private static async Task ApplyModifierAsync( Solution solution, IEnumerable symbols, @@ -561,6 +573,9 @@ private static async Task ApplyModifierAsync( cancellationToken).ConfigureAwait(false); } + document = await UnsafeModifierCodeFixHelpers.AddPendingSafetyDocumentationAsync( + document, + cancellationToken).ConfigureAwait(false); solution = document.Project.Solution; } @@ -584,6 +599,7 @@ private static Document SetExpressionBodiedGetterUnsafe( IndexerDeclarationSyntax indexer => indexer.SemicolonToken, _ => default, }); + getter = (AccessorDeclarationSyntax)UnsafeModifierCodeFixHelpers.MarkForSafetyDocumentation(getter); SyntaxNode replacement = expressionBody.Parent switch { @@ -634,9 +650,11 @@ private static async Task SetEventVariablesUnsafeAsync( EventFieldDeclarationSyntax splitEvent = eventField .WithDeclaration(eventField.Declaration.WithVariables( SyntaxFactory.SingletonSeparatedList(variable))) + // Later declarations rely on the line break that the preceding one already carries, so that + // generated documentation does not introduce a blank line between them. .WithLeadingTrivia(i == 0 ? eventField.GetLeadingTrivia() - : SyntaxFactory.TriviaList(SyntaxFactory.ElasticCarriageReturnLineFeed)) + : default(SyntaxTriviaList)) .WithTrailingTrivia(i == variables.Count - 1 ? eventField.GetTrailingTrivia() : variables.GetSeparator(i).LeadingTrivia @@ -644,7 +662,15 @@ private static async Task SetEventVariablesUnsafeAsync( .Add(SyntaxFactory.ElasticCarriageReturnLineFeed)); if (eventNames.Contains(variable.Identifier.ValueText)) + { + bool wasUnsafe = UnsafeMigrationSyntaxHelpers.HasModifier(splitEvent, SyntaxKind.UnsafeKeyword); splitEvent = SetEventFieldUnsafe(splitEvent); + if (!wasUnsafe) + { + splitEvent = (EventFieldDeclarationSyntax)UnsafeModifierCodeFixHelpers + .MarkForSafetyDocumentation(splitEvent); + } + } splitEvents.Add(splitEvent.WithAdditionalAnnotations(Formatter.Annotation)); } diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs index 2b8e53a70e7b0f..5c496ef740aed2 100644 --- a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs @@ -22,6 +22,11 @@ namespace ILLink.CodeFix /// internal static class UnsafeModifierCodeFixHelpers { + internal const string SafetyDocumentationText = "TODO: Audit"; + + private static readonly SyntaxAnnotation s_safetyDocumentationAnnotation = + new(nameof(s_safetyDocumentationAnnotation)); + /// /// Registers an add-unsafe action for a supported declaration that has no existing safety modifier. /// @@ -98,17 +103,26 @@ internal static async Task AddUnsafeModifierAsync( /// /// Replaces an explicit safe contract with unsafe, or adds unsafe when no safety modifier is present. + /// The declaration is annotated so that can document the + /// new caller-unsafe contract, which also keeps IL5005 from immediately removing the modifier again. /// internal static async Task SetUnsafeModifierAsync( Document document, SyntaxNode declaration, CancellationToken cancellationToken) { - SyntaxToken safeModifier = UnsafeMigrationSyntaxHelpers.GetSafeModifier(declaration); - if (safeModifier != default) + // Only contracts that this fix introduces are documented; a modifier the developer already wrote stays + // subject to the IL5005 rules. + if (UnsafeMigrationSyntaxHelpers.GetSafeModifier(declaration) != default) { + (document, declaration) = await AnnotateForSafetyDocumentationAsync( + document, + declaration, + cancellationToken).ConfigureAwait(false); + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); - if (root is null) + SyntaxToken safeModifier = UnsafeMigrationSyntaxHelpers.GetSafeModifier(declaration); + if (root is null || safeModifier == default) return document; SyntaxToken unsafeModifier = SyntaxFactory.Token(SyntaxKind.UnsafeKeyword) @@ -119,9 +133,107 @@ internal static async Task SetUnsafeModifierAsync( if (UnsafeMigrationSyntaxHelpers.HasModifier(declaration, SyntaxKind.UnsafeKeyword)) return document; + (document, declaration) = await AnnotateForSafetyDocumentationAsync( + document, + declaration, + cancellationToken).ConfigureAwait(false); return await AddUnsafeModifierAsync(document, declaration, cancellationToken).ConfigureAwait(false); } + /// + /// Marks a declaration so that documents its new contract. + /// + internal static SyntaxNode MarkForSafetyDocumentation(SyntaxNode declaration) => + declaration.WithAdditionalAnnotations(s_safetyDocumentationAnnotation); + + /// + /// Documents every declaration that marked as caller-unsafe. + /// + internal static async Task AddPendingSafetyDocumentationAsync( + Document document, + CancellationToken cancellationToken) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + return document; + + SyntaxNode[] documentationTargets = root.GetAnnotatedNodes(s_safetyDocumentationAnnotation) + .Select(GetSafetyDocumentationTarget) + .Distinct() + .Where(static target => !UnsafeMigrationSyntaxHelpers.HasSafetyDocumentation(target)) + .ToArray(); + if (documentationTargets.Length > 0) + { + root = root.ReplaceNodes( + documentationTargets, + static (_, target) => AddSafetyDocumentation(target)); + } + + SyntaxNode[] annotated = root.GetAnnotatedNodes(s_safetyDocumentationAnnotation).ToArray(); + if (annotated.Length > 0) + { + root = root.ReplaceNodes( + annotated, + static (_, node) => node.WithoutAnnotations(s_safetyDocumentationAnnotation)); + } + + return document.WithSyntaxRoot(root); + } + + private static async Task<(Document Document, SyntaxNode Declaration)> AnnotateForSafetyDocumentationAsync( + Document document, + SyntaxNode declaration, + CancellationToken cancellationToken) + { + if (declaration.HasAnnotation(s_safetyDocumentationAnnotation)) + return (document, declaration); + + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + return (document, declaration); + + var annotation = new SyntaxAnnotation(); + SyntaxNode annotatedDeclaration = declaration + .WithAdditionalAnnotations(s_safetyDocumentationAnnotation, annotation); + document = document.WithSyntaxRoot(root.ReplaceNode(declaration, annotatedDeclaration)); + + root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + return (document, root?.GetAnnotatedNodes(annotation).FirstOrDefault() ?? annotatedDeclaration); + } + + /// + /// Finds the declaration that owns the documentation for a caller-unsafe contract. + /// Accessors cannot carry XML documentation, so their contract is documented on the containing member. + /// + private static SyntaxNode GetSafetyDocumentationTarget(SyntaxNode declaration) => + declaration is AccessorDeclarationSyntax + ? declaration.Ancestors().OfType().FirstOrDefault() ?? declaration + : declaration; + + private static SyntaxNode AddSafetyDocumentation(SyntaxNode declaration) + { + SyntaxTriviaList leadingTrivia = declaration.GetLeadingTrivia(); + string indentation = leadingTrivia.LastOrDefault(static trivia => + trivia.IsKind(SyntaxKind.WhitespaceTrivia)).ToFullString(); + SyntaxTriviaList documentation = SyntaxFactory.ParseLeadingTrivia( + $"/// <{UnsafeMigrationSyntaxHelpers.SafetyDocumentationElement}>{SafetyDocumentationText}" + + $"" + + $"{GetEndOfLine(declaration)}{indentation}"); + return declaration.WithLeadingTrivia(leadingTrivia.AddRange(documentation)); + } + + /// + /// Reuses the line ending already present around a declaration so generated documentation matches the file. + /// + private static string GetEndOfLine(SyntaxNode declaration) + { + SyntaxTrivia endOfLine = declaration.GetLeadingTrivia() + .FirstOrDefault(static trivia => trivia.IsKind(SyntaxKind.EndOfLineTrivia)); + return endOfLine == default + ? SyntaxFactory.CarriageReturnLineFeed.ToFullString() + : endOfLine.ToFullString(); + } + /// /// Removes unsafe without disturbing modifiers that the current SyntaxGenerator does not model. /// diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzerHelpers.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzerHelpers.cs index 144e9fd98942f6..d81219354cfc5f 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzerHelpers.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzerHelpers.cs @@ -188,11 +188,7 @@ private static IEnumerable GetDocumentationSymbols(ISymbol symbol) } private static bool HasSafetyDocumentation(SyntaxNode declaration) => - declaration.GetLeadingTrivia().Any(static trivia => - trivia.GetStructure() is DocumentationCommentTriviaSyntax documentationComment - && documentationComment.DescendantNodes().Any(static node => - node is XmlElementSyntax { StartTag.Name.LocalName.ValueText: "safety" } - or XmlEmptyElementSyntax { Name.LocalName.ValueText: "safety" })); + UnsafeMigrationSyntaxHelpers.HasSafetyDocumentation(declaration); private static bool HasInstanceBackingField(ISymbol associatedSymbol) => associatedSymbol.ContainingType.GetMembers() diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs index e04d58c47c7309..27c06d03c98c03 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs @@ -14,7 +14,11 @@ namespace ILLink.RoslynAnalyzer /// internal static class UnsafeMigrationSyntaxHelpers { - // The analyzer builds against a Roslyn version that predates SyntaxKind.SafeKeyword. + internal const string SafetyDocumentationElement = "safety"; + + // This project compiles against MicrosoftCodeAnalysisVersion_LatestVS, which predates SyntaxKind.SafeKeyword, + // but it runs inside whichever compiler the host loaded, which may be newer. Resolve the kind at run time so + // the migration tooling works on both, and treats 'safe' as unsupported when the host cannot parse it. private static readonly SyntaxKind s_safeKeyword = SyntaxFacts.GetContextualKeywordKind("safe"); internal static SyntaxTokenList GetModifiers(SyntaxNode declaration) => @@ -45,6 +49,16 @@ internal static SyntaxToken GetModifier(SyntaxNode declaration, SyntaxKind modif return default; } + + /// + /// Determines whether a declaration carries a <safety> documentation element. + /// + internal static bool HasSafetyDocumentation(SyntaxNode declaration) => + declaration.GetLeadingTrivia().Any(static trivia => + trivia.GetStructure() is DocumentationCommentTriviaSyntax documentationComment + && documentationComment.DescendantNodes().Any(static node => + node is XmlElementSyntax { StartTag.Name.LocalName.ValueText: SafetyDocumentationElement } + or XmlEmptyElementSyntax { Name.LocalName.ValueText: SafetyDocumentationElement })); } } #endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs index d15e0be2c49a3c..85fd9ff4ad1808 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs @@ -1259,6 +1259,7 @@ protected unsafe B() { } class C : B { + /// TODO: Audit public unsafe C() { } } """; @@ -1278,6 +1279,7 @@ public C(int* pointer) {|CS9363:: base(pointer)|} { } var fixedSource = """ class C : LegacyBase { + /// TODO: Audit public unsafe C(int* pointer) : base(pointer) { } } """; @@ -1428,6 +1430,538 @@ static void M() await test.RunAsync(); } + [Fact] + public async Task UnsafePropertyReadUsesCastInsideUnsafeExpression() + { + var source = """ + class Data + { + public unsafe int Value { get; } + } + + class C + { + static Data s_data = new(); + static int Value = {|CS9362:s_data.Value|}; + } + """; + var fixedSource = """ + class Data + { + public unsafe int Value { get; } + } + + class C + { + static Data s_data = new(); + static int Value = unsafe(/* SAFETY: Audit */ (int)s_data.Value); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task UnsafeIndexerReadUsesCastInsideUnsafeExpression() + { + var source = """ + class Data + { + public unsafe bool this[int index] => true; + } + + class C + { + static Data s_data = new(); + + static void M() + { + try + { + } + catch (System.Exception) when ({|CS9362:s_data[0]|}) + { + } + } + } + """; + var fixedSource = """ + class Data + { + public unsafe bool this[int index] => true; + } + + class C + { + static Data s_data = new(); + + static void M() + { + try + { + } + catch (System.Exception) when (unsafe(/* SAFETY: Audit */ (bool)s_data[0])) + { + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task MethodGroupConversionUsesCastInsideUnsafeExpression() + { + var source = """ + class C + { + static unsafe void Work() { } + static System.Action s_action = {|CS9362:Work|}; + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + static System.Action s_action = unsafe(/* SAFETY: Audit */ (System.Action)Work); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task MethodGroupConversionInYieldReturnUsesCast() + { + var source = """ + class C + { + static unsafe void Work() { } + + static System.Collections.Generic.IEnumerable M() + { + yield return {|CS9362:Work|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static System.Collections.Generic.IEnumerable M() + { + yield return unsafe(/* SAFETY: Audit */ (System.Action)Work); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task DeconstructionUsesUnsafeStatement() + { + var source = """ + class D + { + public unsafe void Deconstruct(out int first, out int second) + { + first = 0; + second = 0; + } + } + + class C + { + static void M() + { + var (first, second) = {|CS9362:new D()|}; + System.Console.WriteLine(first + second); + } + } + """; + var fixedSource = """ + class D + { + public unsafe void Deconstruct(out int first, out int second) + { + first = 0; + second = 0; + } + } + + class C + { + static void M() + { + unsafe + { + // SAFETY: Audit + var (first, second) = new D(); + System.Console.WriteLine(first + second); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task NoFixWhenUnsafeExpressionIsAlreadyPresent() + { + var source = """ + class Data + { + public unsafe int Value { get; } + } + + class C + { + static Data s_data = new(); + static int Value = unsafe(/* SAFETY: Audit */ {|CS9362:s_data.Value|}); + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixForUnsafePropertyAssignmentInAwaitStatement() + { + var source = """ + class Data + { + public unsafe int Value { get; set; } + } + + class C + { + static Data s_data = new(); + static async System.Threading.Tasks.Task GetAsync() => 1; + + static async System.Threading.Tasks.Task M() + { + {|CS9362:{|CS9362:s_data.Value|}|} += await GetAsync(); + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task NoFixForRefReturningUnsafeProperty() + { + var source = """ + class Data + { + static int s_value; + public unsafe ref int Value => ref s_value; + } + + class C + { + static Data s_data = new(); + static int Value = {|CS9362:s_data.Value|}; + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source); + await test.RunAsync(); + } + + [Fact] + public async Task MergesAdjacentGeneratedUnsafeStatements() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + {|CS9362:Work()|}; + {|CS9362:Work()|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Work(); + Work(); + } + } + } + """; + // Fix all evaluates every diagnostic against the original document, so it cannot merge the regions that + // the individual fixes produce. + var batchFixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Work(); + } + + unsafe + { + // SAFETY: Audit + Work(); + } + } + } + """; + + var test = CreateTest(source, fixedSource); + test.BatchFixedCode = batchFixedSource; + await test.RunAsync(); + } + + [Fact] + public async Task MergesGeneratedUnsafeStatementsSeparatedBySafeStatement() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + {|CS9362:Work()|}; + System.Console.WriteLine(); + {|CS9362:Work()|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Work(); + System.Console.WriteLine(); + Work(); + } + } + } + """; + var batchFixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Work(); + } + + System.Console.WriteLine(); + unsafe + { + // SAFETY: Audit + Work(); + } + } + } + """; + + var test = CreateTest(source, fixedSource); + test.BatchFixedCode = batchFixedSource; + await test.RunAsync(); + } + + [Fact] + public async Task DoesNotMergeIntoAuditedUnsafeStatement() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Reviewed by the runtime team. + Work(); + } + {|CS9362:Work()|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Reviewed by the runtime team. + Work(); + } + unsafe + { + // SAFETY: Audit + Work(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task DoesNotMergeAcrossADeclaringStatement() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + {|CS9362:Work()|}; + int value = 5; + {|CS9362:Work()|}; + System.Console.WriteLine(value); + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + Work(); + } + + int value = 5; + unsafe + { + // SAFETY: Audit + Work(); + } + + System.Console.WriteLine(value); + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task MergingKeepsCommentsFromTheAbsorbedRegion() + { + var source = """ + class C + { + static unsafe void Work() { } + + static void M() + { + unsafe + { + // SAFETY: Audit + // Only the first call needs the pointer. + Work(); + } + {|CS9362:Work()|}; + } + } + """; + var fixedSource = """ + class C + { + static unsafe void Work() { } + + static void M() + { + // Only the first call needs the pointer. + unsafe + { + // SAFETY: Audit + Work(); + Work(); + } + } + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + + [Fact] + public async Task ParenthesizesUnaryOperandOfInsertedCast() + { + var source = """ + struct Ptr + { + public static implicit operator Ptr(int value) => default; + } + + class C + { + static int* GetPointer() => null; + static Ptr s_ptr = {|CS9360:*|}GetPointer(); + } + """; + var fixedSource = """ + struct Ptr + { + public static implicit operator Ptr(int value) => default; + } + + class C + { + static int* GetPointer() => null; + static Ptr s_ptr = unsafe(/* SAFETY: Audit */ (Ptr)(*GetPointer())); + } + """; + + await CreateTest(source, fixedSource).RunAsync(); + } + private static CSharpCodeFixVerifier.Test CreateTest( string source, string fixedSource) => diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs index cbc42f201f0009..ba7b4c725213d4 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs @@ -37,6 +37,7 @@ public override void Method() { } var fixedSource = """ class Base { + /// TODO: Audit public virtual unsafe void Method() { } } @@ -47,7 +48,7 @@ public override unsafe void Method() { } class Second : Base { - public override unsafe void Method() { } + public override void Method() { } } """; @@ -81,6 +82,7 @@ public void Method() { } var fixedSource = """ interface I { + /// TODO: Audit unsafe void Method(); } @@ -96,7 +98,7 @@ unsafe void I.Method() { } class Other : I { - public unsafe void Method() { } + public void Method() { } } """; @@ -125,6 +127,7 @@ class OtherImplementation : I var fixedSource = """ interface I { + /// TODO: Audit unsafe int Property { get; } } @@ -135,7 +138,7 @@ class UnsafeImplementation : I class OtherImplementation : I { - public unsafe int Property => 0; + public int Property => 0; } """; @@ -143,7 +146,7 @@ class OtherImplementation : I } [Fact] - public async Task NoFixWhenPropertyImplementationIsSynthesized() + public async Task PropagatesToInterfaceEvenWhenOtherImplementationIsSynthesized() { var source = """ interface I @@ -158,8 +161,22 @@ class UnsafeImplementation : I record OtherImplementation(int Property) : I; """; + var fixedSource = """ + interface I + { + /// TODO: Audit + unsafe int Property { get; } + } - await CreateCompilerTest(source).RunAsync(); + class UnsafeImplementation : I + { + public unsafe int Property => 0; + } + + record OtherImplementation(int Property) : I; + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); } [Fact] @@ -179,6 +196,7 @@ interface IDerived : IBase var fixedSource = """ interface IBase { + /// TODO: Audit unsafe void Method(); } @@ -252,12 +270,15 @@ partial class C var fixedSource = """ partial class C { + /// TODO: Audit public unsafe partial void Method(); public unsafe partial void Method() { } } """; - await CreateCompilerTest(source, fixedSource).RunAsync(); + var test = CreateCompilerTest(source, fixedSource); + test.CodeActionEquivalenceKey = "Propagate unsafe contract"; + await test.RunAsync(); } [Fact] @@ -274,6 +295,7 @@ partial class C partial class C { public unsafe partial void Method(); + /// TODO: Audit public unsafe extern partial void Method(); } """; @@ -294,12 +316,17 @@ partial class C var fixedSource = """ partial class C { + /// TODO: Audit public unsafe partial void Method(); + /// TODO: Audit public unsafe extern partial void Method(); } """; - await CreateCompilerTest(source, fixedSource).RunAsync(); + var test = CreateCompilerTest(source, fixedSource); + // Replacing a deliberate audit is offered under a title that says so. + test.CodeActionEquivalenceKey = "Propagate unsafe contract (discards explicit 'safe')"; + await test.RunAsync(); } [Fact] @@ -323,6 +350,7 @@ public override int Property var fixedSource = """ class Base { + /// TODO: Audit public virtual int Property { unsafe get; set; } } @@ -339,6 +367,42 @@ public override int Property await CreateCompilerTest(source, fixedSource).RunAsync(); } + [Fact] + public async Task ExpressionBodiedBaseGetterBecomesUnsafeAccessor() + { + var source = """ + class Base + { + public virtual int Property => 0; + } + + class Derived : Base + { + public override int Property + { + unsafe {|CS9364:get|} => 1; + } + } + """; + var fixedSource = """ + class Base + { + /// TODO: Audit + public virtual int Property { unsafe get => 0; } + } + + class Derived : Base + { + public override int Property + { + unsafe get => 1; + } + } + """; + + await CreateCompilerTest(source, fixedSource).RunAsync(); + } + [Fact] public async Task PropagatesAcrossDocuments() { @@ -374,6 +438,7 @@ public override void Method() { } """ class Base { + /// TODO: Audit public virtual unsafe void Method() { } } """)); @@ -390,7 +455,7 @@ public override unsafe void Method() { } """ class Second : Base { - public override unsafe void Method() { } + public override void Method() { } } """)); test.SolutionTransforms.Add(UnsafeMigrationTestHelpers.SetOptions); @@ -412,6 +477,7 @@ partial class C partial class C { public partial int Property { unsafe get; } + /// TODO: Audit public partial int Property { unsafe get => 0; } } """; @@ -441,6 +507,7 @@ class C : I interface I { + /// TODO: Audit unsafe event System.Action First; event System.Action Safe; } @@ -478,7 +545,9 @@ class C : I interface I { + /// TODO: Audit unsafe event System.Action First; + /// TODO: Audit unsafe event System.Action Second; event System.Action Safe; } @@ -490,8 +559,32 @@ class C : I public event System.Action Safe; } """; + // Fix all computes both fixes against the original shared declaration, so the merged text keeps a blank + // line between the two declarations it split apart. + var batchFixedSource = """ + #pragma warning disable CS0067 - await CreateCompilerTest(source, fixedSource).RunAsync(); + interface I + { + /// TODO: Audit + unsafe event System.Action First; + + /// TODO: Audit + unsafe event System.Action Second; + event System.Action Safe; + } + + class C : I + { + public unsafe event System.Action First; + public unsafe event System.Action Second; + public event System.Action Safe; + } + """; + + var test = CreateCompilerTest(source, fixedSource); + test.BatchFixedCode = batchFixedSource; + await test.RunAsync(); } private static CSharpCodeFixVerifier< diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMemberMissingSafetyDocumentationAnalyzerTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMemberMissingSafetyDocumentationAnalyzerTests.cs index 35559d26bab9e7..16e84af47675a4 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMemberMissingSafetyDocumentationAnalyzerTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMemberMissingSafetyDocumentationAnalyzerTests.cs @@ -108,6 +108,35 @@ unsafe void Local() { } await test.RunAsync(); } + [Fact] + public async Task GeneratedSafetyDocumentationSuppressesTheDiagnostic() + { + // Contracts introduced by the migration fixers carry this stub, so a later analysis pass does not remove + // the modifier they just added. + var source = """ + class Base + { + /// TODO: Audit + public virtual unsafe void Method() { } + } + + class B + { + protected {|IL5005:unsafe|} B() { } + } + + class C : B + { + /// TODO: Audit + public unsafe C() { } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source); + await test.RunAsync(); + } + [Fact] public async Task IgnoresDeclarationsThatCannotExposeUnsafeContracts() { From da45c48b1fb8c3b3ef0e40366b93749c269fff40 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 24 Jul 2026 23:51:02 +0200 Subject: [PATCH 4/4] Address PR feedback on unsafe expression parsing and line endings The unsafe expression template is now parsed with the parse options of the document being fixed instead of a fixed set, so the generated syntax always matches the language mode the project compiles with, and the fix is declined when that mode cannot express an unsafe expression. The template also carries the syntax kind used to detect an enclosing unsafe expression, which removes the static probe that guessed it. Generated safety documentation reuses the line ending the file already uses. Member declarations carry their indentation but not the preceding line break, so the previous fallback emitted a carriage return into files that use line feeds only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c3f7fe4-85f4-40f3-9ccc-e6113369014b --- .../AddUnsafeContextCodeFixProvider.cs | 60 +++++++++---------- .../UnsafeModifierCodeFixHelpers.cs | 26 +++++--- .../AddUnsafeContextCodeFixTests.cs | 11 ++++ 3 files changed, 56 insertions(+), 41 deletions(-) diff --git a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs index 1faac65563d826..5b277f7902950e 100644 --- a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs +++ b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeContextCodeFixProvider.cs @@ -43,14 +43,6 @@ public sealed class AddUnsafeContextCodeFixProvider : Microsoft.CodeAnalysis.Cod /// private const int MaxSafeStatementsBetweenMergedRegions = 1; - private static readonly CSharpParseOptions s_unsafeExpressionParseOptions = - new(LanguageVersion.Preview); - - // The code fix compiles against a Roslyn version that predates unsafe expressions but runs inside whichever - // compiler the host loaded. Discover the syntax kind at run time and fall back to statement contexts when the - // host cannot parse 'unsafe(...)'. - private static readonly int s_unsafeExpressionRawKind = GetUnsafeExpressionRawKind(); - private static readonly SymbolDisplayFormat s_localTypeDisplayFormat = SymbolDisplayFormat.MinimallyQualifiedFormat.WithMiscellaneousOptions( SymbolDisplayFormat.MinimallyQualifiedFormat.MiscellaneousOptions @@ -334,11 +326,16 @@ private static bool TryRegisterExpressionFix( return false; } - (ExpressionSyntax expression, ExpressionSyntax operand) = fix; + (ExpressionSyntax expression, ExpressionSyntax operand, ExpressionSyntax template) = fix; RegisterFix( context, title, - _ => Task.FromResult(ReplaceWithUnsafeExpression(context.Document, root, expression, operand))); + _ => Task.FromResult(ReplaceWithUnsafeExpression( + context.Document, + root, + expression, + operand, + template))); return true; } @@ -441,24 +438,21 @@ private static bool IsExpressionOnlyContext(SyntaxNode targetNode, TextSpan diag return false; } - private static bool SupportsUnsafeExpressions => s_unsafeExpressionRawKind >= 0; - - private static bool IsUnsafeExpression(SyntaxNode node) => - SupportsUnsafeExpressions && node.RawKind == s_unsafeExpressionRawKind; - - private static int GetUnsafeExpressionRawKind() + /// + /// Builds the unsafe expression template with the parse options of the document being fixed, so the generated + /// syntax matches the language mode the project compiles with. Returns when that mode + /// cannot express an unsafe expression, or when the loaded compiler predates the syntax. + /// + private static ExpressionSyntax? TryParseUnsafeExpressionTemplate(SyntaxNode node) { - ExpressionSyntax probe = ParseUnsafeExpression(UnsafeExpressionPlaceholder); - return !probe.IsKind(SyntaxKind.IdentifierName) && TryFindPlaceholder(probe) is not null - ? probe.RawKind - : -1; + ExpressionSyntax template = SyntaxFactory.ParseExpression( + $"unsafe(/* SAFETY: Audit */ {UnsafeExpressionPlaceholder})", + options: node.SyntaxTree.Options as CSharpParseOptions); + return !template.ContainsDiagnostics && TryFindPlaceholder(template) is not null + ? template + : null; } - private static ExpressionSyntax ParseUnsafeExpression(string inner) => - SyntaxFactory.ParseExpression( - $"unsafe(/* SAFETY: Audit */ {inner})", - options: s_unsafeExpressionParseOptions); - private static IdentifierNameSyntax? TryFindPlaceholder(ExpressionSyntax expression) => expression.DescendantNodesAndSelf() .OfType() @@ -468,28 +462,28 @@ private static ExpressionSyntax ParseUnsafeExpression(string inner) => /// Builds the expression-level fix, or returns when no semantics-preserving unsafe /// expression can be written for the reported operation. /// - private static (ExpressionSyntax Expression, ExpressionSyntax Operand)? TryGetUnsafeExpressionFix( + private static (ExpressionSyntax Expression, ExpressionSyntax Operand, ExpressionSyntax Template)? TryGetUnsafeExpressionFix( SyntaxNode targetNode, TextSpan diagnosticSpan, SemanticModel semanticModel, CancellationToken cancellationToken) { - if (!SupportsUnsafeExpressions - || TryGetUnsafeExpression(targetNode, diagnosticSpan) is not { } expression - || !CanUseUnsafeExpression(expression)) + if (TryGetUnsafeExpression(targetNode, diagnosticSpan) is not { } expression + || !CanUseUnsafeExpression(expression) + || TryParseUnsafeExpressionTemplate(expression) is not { } template) { return null; } // Reaching here from inside an unsafe expression means the compiler does not treat that expression as an // unsafe context for this operation, so wrapping it again would only nest without fixing anything. - if (expression.AncestorsAndSelf().Any(IsUnsafeExpression)) + if (expression.AncestorsAndSelf().Any(node => node.RawKind == template.RawKind)) return null; if (TryGetUnsafeExpressionOperand(semanticModel, expression, cancellationToken) is not { } operand) return null; - return (expression, operand); + return (expression, operand, template); } private static ExpressionSyntax? TryGetUnsafeExpression(SyntaxNode targetNode, TextSpan diagnosticSpan) => @@ -1190,9 +1184,9 @@ private static Document ReplaceWithUnsafeExpression( Document document, SyntaxNode root, ExpressionSyntax expression, - ExpressionSyntax operand) + ExpressionSyntax operand, + ExpressionSyntax template) { - ExpressionSyntax template = ParseUnsafeExpression(UnsafeExpressionPlaceholder); IdentifierNameSyntax placeholder = TryFindPlaceholder(template)!; ExpressionSyntax replacement = template .ReplaceNode(placeholder, operand) diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs index 5c496ef740aed2..1c29de6ab7d17f 100644 --- a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs @@ -13,6 +13,7 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Text; namespace ILLink.CodeFix { @@ -166,7 +167,7 @@ internal static async Task AddPendingSafetyDocumentationAsync( { root = root.ReplaceNodes( documentationTargets, - static (_, target) => AddSafetyDocumentation(target)); + (_, target) => AddSafetyDocumentation(target, cancellationToken)); } SyntaxNode[] annotated = root.GetAnnotatedNodes(s_safetyDocumentationAnnotation).ToArray(); @@ -210,7 +211,7 @@ declaration is AccessorDeclarationSyntax ? declaration.Ancestors().OfType().FirstOrDefault() ?? declaration : declaration; - private static SyntaxNode AddSafetyDocumentation(SyntaxNode declaration) + private static SyntaxNode AddSafetyDocumentation(SyntaxNode declaration, CancellationToken cancellationToken) { SyntaxTriviaList leadingTrivia = declaration.GetLeadingTrivia(); string indentation = leadingTrivia.LastOrDefault(static trivia => @@ -218,20 +219,28 @@ private static SyntaxNode AddSafetyDocumentation(SyntaxNode declaration) SyntaxTriviaList documentation = SyntaxFactory.ParseLeadingTrivia( $"/// <{UnsafeMigrationSyntaxHelpers.SafetyDocumentationElement}>{SafetyDocumentationText}" + $"" - + $"{GetEndOfLine(declaration)}{indentation}"); + + $"{GetEndOfLine(declaration, cancellationToken)}{indentation}"); return declaration.WithLeadingTrivia(leadingTrivia.AddRange(documentation)); } /// - /// Reuses the line ending already present around a declaration so generated documentation matches the file. + /// Reuses the line ending the file already uses so generated documentation does not mix line endings. /// - private static string GetEndOfLine(SyntaxNode declaration) + private static string GetEndOfLine(SyntaxNode declaration, CancellationToken cancellationToken) { SyntaxTrivia endOfLine = declaration.GetLeadingTrivia() .FirstOrDefault(static trivia => trivia.IsKind(SyntaxKind.EndOfLineTrivia)); - return endOfLine == default - ? SyntaxFactory.CarriageReturnLineFeed.ToFullString() - : endOfLine.ToFullString(); + if (endOfLine != default) + return endOfLine.ToFullString(); + + SourceText text = declaration.SyntaxTree.GetText(cancellationToken); + foreach (TextLine line in text.Lines) + { + if (line.EndIncludingLineBreak > line.End) + return text.ToString(TextSpan.FromBounds(line.End, line.EndIncludingLineBreak)); + } + + return SyntaxFactory.CarriageReturnLineFeed.ToFullString(); } /// @@ -362,3 +371,4 @@ private static int GetUnsafeModifierIndex(SyntaxTokenList modifiers) => } } #endif + diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs index 85fd9ff4ad1808..9add823f6b5698 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs @@ -1962,6 +1962,17 @@ class C await CreateTest(source, fixedSource).RunAsync(); } + [Fact] + public async Task GeneratedDocumentationMatchesFileLineEndings() + { + // Written with explicit line feeds: the generated documentation must not introduce carriage returns. + var source = "class B\n{\n protected unsafe B() { }\n}\n\nclass C : B\n{\n {|CS9362:public C() { }|}\n}\n"; + var fixedSource = "class B\n{\n protected unsafe B() { }\n}\n\nclass C : B\n{\n" + + " /// TODO: Audit\n public unsafe C() { }\n}\n"; + + await CreateTest(source, fixedSource).RunAsync(); + } + private static CSharpCodeFixVerifier.Test CreateTest( string source, string fixedSource) =>