From ea3d7050976899930e38c86d47f72e943ab2f704 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 28 Jul 2026 14:28:30 +0200 Subject: [PATCH 1/3] Add safety documentation analyzers Adds the safety documentation tier of the unsafe-v2 migration tooling. IL5009 reports an unsafe block or unsafe(...) expression that records no // SAFETY: comment explaining how its obligations are discharged, and AddSafetyCommentCodeFixProvider inserts a stub for review. IL5005 already covers the signature side of the contract; this covers the place where the obligations are actually met. IL5010 reports an explicit safe modifier with no documentation. It is the symmetric hole to IL5005: safe is a hand-written assertion the compiler cannot verify, on a native boundary or on a field whose overlap must not be used to type-pun. No fixer, since only a developer can write the justification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5 --- .../AddSafetyCommentCodeFixProvider.cs | 122 ++++++++++ .../illink/src/ILLink.CodeFix/Resources.resx | 3 + ...afeModifierMissingJustificationAnalyzer.cs | 87 +++++++ ...UnsafeBlockMissingSafetyCommentAnalyzer.cs | 123 ++++++++++ .../UnsafeMigrationSyntaxHelpers.cs | 19 ++ .../illink/src/ILLink.Shared/DiagnosticId.cs | 4 +- .../src/ILLink.Shared/SharedStrings.resx | 12 + .../AddSafetyCommentCodeFixTests.cs | 129 +++++++++++ ...difierMissingJustificationAnalyzerTests.cs | 112 +++++++++ ...eBlockMissingSafetyCommentAnalyzerTests.cs | 212 ++++++++++++++++++ .../UnsafeMigrationTestHelpers.cs | 4 +- 11 files changed, 825 insertions(+), 2 deletions(-) create mode 100644 src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs create mode 100644 src/tools/illink/src/ILLink.RoslynAnalyzer/SafeModifierMissingJustificationAnalyzer.cs create mode 100644 src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs create mode 100644 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs create mode 100644 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SafeModifierMissingJustificationAnalyzerTests.cs create mode 100644 src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs diff --git a/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs new file mode 100644 index 00000000000000..e66ae5ce89600f --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs @@ -0,0 +1,122 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using ILLink.CodeFixProvider; +using ILLink.RoslynAnalyzer; +using ILLink.Shared; +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 +{ + /// + /// Fixes analyzer diagnostic IL5009 by inserting a // SAFETY: TODO stub above an undocumented + /// unsafe region. + /// + /// + /// The stub only marks the region for review. It deliberately does not attempt to describe the reasoning, + /// which is what the developer has to supply. + /// + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AddSafetyCommentCodeFixProvider)), Shared] + public sealed class AddSafetyCommentCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider + { + private const string SafetyComment = "// SAFETY: TODO"; + + private static LocalizableString CodeFixTitle => + new LocalizableResourceString( + nameof(Resources.AddSafetyCommentCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => + [DiagnosticId.UnsafeBlockMissingSafetyComment.AsString()]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + Diagnostic diagnostic = context.Diagnostics[0]; + if (await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false) is not { } root) + return; + + SyntaxToken unsafeKeyword = root.FindToken(diagnostic.Location.SourceSpan.Start); + if (!unsafeKeyword.IsKind(SyntaxKind.UnsafeKeyword) + || GetCommentTarget(unsafeKeyword) is not { } target) + { + return; + } + + string title = CodeFixTitle.ToString(); + context.RegisterCodeFix( + CodeAction.Create( + title, + cancellationToken => AddSafetyCommentAsync(context.Document, target, cancellationToken), + title), + diagnostic); + } + + private static async Task AddSafetyCommentAsync( + Document document, + SyntaxNode target, + CancellationToken cancellationToken) + { + var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); + + // The comment goes on its own line directly above the target, keeping any existing leading trivia + // such as XML documentation or other comments in place. + SyntaxTriviaList leadingTrivia = target.GetLeadingTrivia(); + int insertionIndex = GetInsertionIndex(leadingTrivia); + SyntaxTriviaList updated = leadingTrivia.InsertRange( + insertionIndex, + new[] { SyntaxFactory.Comment(SafetyComment), SyntaxFactory.ElasticCarriageReturnLineFeed }); + + editor.ReplaceNode(target, target.WithLeadingTrivia(updated)); + return editor.GetChangedDocument(); + } + + /// + /// Inserts after any trivia that must stay attached to the line above, such as directives. + /// + private static int GetInsertionIndex(SyntaxTriviaList leadingTrivia) + { + int index = 0; + for (int i = 0; i < leadingTrivia.Count; i++) + { + if (leadingTrivia[i].IsDirective || leadingTrivia[i].IsKind(SyntaxKind.DisabledTextTrivia)) + index = i + 1; + } + + return index; + } + + /// + /// Finds the node the comment should be attached to. + /// + /// + /// An unsafe block is its own statement, but an unsafe expression sits inside a larger + /// statement, and a comment reads better above that statement than inline before the keyword. + /// + private static SyntaxNode? GetCommentTarget(SyntaxToken unsafeKeyword) + { + if (unsafeKeyword.Parent is null) + return null; + + if (unsafeKeyword.Parent.IsKind(SyntaxKind.UnsafeStatement)) + return unsafeKeyword.Parent; + + return unsafeKeyword.Parent.AncestorsAndSelf() + .FirstOrDefault(static ancestor => ancestor is StatementSyntax or MemberDeclarationSyntax); + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/Resources.resx b/src/tools/illink/src/ILLink.CodeFix/Resources.resx index 99b54822b57a9f..cf5eb96e980150 100644 --- a/src/tools/illink/src/ILLink.CodeFix/Resources.resx +++ b/src/tools/illink/src/ILLink.CodeFix/Resources.resx @@ -144,6 +144,9 @@ Mark field-like member 'unsafe' + + Add '// SAFETY:' comment + Add UnconditionalSuppressMessage attribute to parent method diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/SafeModifierMissingJustificationAnalyzer.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/SafeModifierMissingJustificationAnalyzer.cs new file mode 100644 index 00000000000000..4690553d488190 --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/SafeModifierMissingJustificationAnalyzer.cs @@ -0,0 +1,87 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +#if DEBUG +using System.Collections.Immutable; +using ILLink.Shared; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace ILLink.RoslynAnalyzer +{ + /// + /// Reports IL5010 for an explicit safe modifier that has no <safety> XML + /// documentation. + /// + /// + /// This is the symmetric hole to IL5005. safe is a hand-written assertion the compiler cannot + /// verify: on an extern member or a LibraryImport it claims the native boundary upholds its + /// contract, and on a field in an explicit or extended layout it claims the overlap cannot be used to + /// type-pun. Both deserve a recorded audit. No code fix is offered because only a developer can write the + /// justification. The diagnostic is disabled by default while this migration tooling remains experimental. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class SafeModifierMissingJustificationAnalyzer : DiagnosticAnalyzer + { + private static readonly DiagnosticDescriptor s_rule = + DiagnosticDescriptors.GetDiagnosticDescriptor( + DiagnosticId.SafeModifierMissingJustification, + isEnabledByDefault: false); + + public override ImmutableArray SupportedDiagnostics => [s_rule]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + if (!System.Diagnostics.Debugger.IsAttached) + context.EnableConcurrentExecution(); + + context.RegisterSymbolAction( + AnalyzeSymbol, + SymbolKind.Method, + SymbolKind.Property, + SymbolKind.Field, + SymbolKind.Event); + context.RegisterOperationAction(AnalyzeLocalFunction, OperationKind.LocalFunction); + } + + private static void AnalyzeSymbol(SymbolAnalysisContext context) => + AnalyzeSymbol(context.Symbol, context.CancellationToken, context.ReportDiagnostic); + + private static void AnalyzeLocalFunction(OperationAnalysisContext context) => + AnalyzeSymbol( + ((ILocalFunctionOperation)context.Operation).Symbol, + context.CancellationToken, + context.ReportDiagnostic); + + private static void AnalyzeSymbol( + ISymbol symbol, + System.Threading.CancellationToken cancellationToken, + System.Action reportDiagnostic) + { + // Accessors take their contract from the containing property or event, which is analyzed separately. + if (symbol is IMethodSymbol { AssociatedSymbol: not null }) + return; + + foreach (SyntaxNode declaration in UnsafeMigrationAnalyzerHelpers.GetDeclarations(symbol, cancellationToken)) + { + if (!UnsafeMigrationSyntaxHelpers.HasSafeModifier(declaration) + || UnsafeMigrationAnalyzerHelpers.HasSafetyDocumentation(declaration, symbol, cancellationToken)) + { + continue; + } + + SyntaxToken safeModifier = UnsafeMigrationSyntaxHelpers.GetModifier( + declaration, + UnsafeMigrationSyntaxHelpers.SafeKeywordKind); + if (safeModifier == default) + continue; + + reportDiagnostic(Diagnostic.Create(s_rule, safeModifier.GetLocation())); + } + } + } +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs new file mode 100644 index 00000000000000..227e4e4a73a000 --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs @@ -0,0 +1,123 @@ +// 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.Collections.Immutable; +using System.Linq; +using ILLink.Shared; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace ILLink.RoslynAnalyzer +{ + /// + /// Reports IL5009 for an unsafe region that has no // SAFETY: comment explaining how its + /// obligations are discharged. + /// + /// + /// IL5005 covers the signature side of the contract, where an unsafe member documents what it + /// asks of its callers. This covers the other side: an unsafe region is where those obligations are + /// actually discharged, and the reasoning is invisible unless it is written down. The convention follows + /// Rust's safety comments, + /// which the speclet recommends. The diagnostic is disabled by default while this migration tooling remains + /// experimental. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class UnsafeBlockMissingSafetyCommentAnalyzer : DiagnosticAnalyzer + { + internal const string SafetyCommentPrefix = "SAFETY"; + + private static readonly DiagnosticDescriptor s_rule = + DiagnosticDescriptors.GetDiagnosticDescriptor( + DiagnosticId.UnsafeBlockMissingSafetyComment, + isEnabledByDefault: false); + + public override ImmutableArray SupportedDiagnostics => [s_rule]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + if (!System.Diagnostics.Debugger.IsAttached) + context.EnableConcurrentExecution(); + + // 'unsafe' expressions are newer than the Roslyn this analyzer compiles against, so both forms are + // matched on the keyword token rather than on a node type. + context.RegisterSyntaxNodeAction(AnalyzeUnsafeStatement, SyntaxKind.UnsafeStatement); + context.RegisterSyntaxTreeAction(AnalyzeUnsafeExpressions); + } + + private static void AnalyzeUnsafeStatement(SyntaxNodeAnalysisContext context) + { + SyntaxNode unsafeStatement = context.Node; + + // A nested region inherits the reasoning of the one that already documents it. + if (IsNestedInUnsafeRegion(unsafeStatement) + || HasSafetyComment(unsafeStatement.GetFirstToken())) + { + return; + } + + context.ReportDiagnostic(Diagnostic.Create(s_rule, unsafeStatement.GetFirstToken().GetLocation())); + } + + private static void AnalyzeUnsafeExpressions(SyntaxTreeAnalysisContext context) + { + SyntaxNode root = context.Tree.GetRoot(context.CancellationToken); + foreach (SyntaxToken token in root.DescendantTokens()) + { + if (!UnsafeMigrationSyntaxHelpers.IsUnsafeExpressionKeyword(token) + || token.Parent is null + || IsNestedInUnsafeRegion(token.Parent) + || HasSafetyComment(token)) + { + continue; + } + + context.ReportDiagnostic(Diagnostic.Create(s_rule, token.GetLocation())); + } + } + + /// + /// Determines whether a region sits inside another unsafe region, whose comment already covers it. + /// + private static bool IsNestedInUnsafeRegion(SyntaxNode region) => + region.Ancestors().Any(static ancestor => + ancestor.IsKind(SyntaxKind.UnsafeStatement) + || UnsafeMigrationSyntaxHelpers.IsUnsafeExpressionKeyword(ancestor.GetFirstToken())); + + /// + /// Looks for a // SAFETY: comment attached to the region or to the statement that contains it. + /// + /// + /// An unsafe expression sits inside a larger statement, so its comment is normally written above + /// that statement rather than immediately before the keyword. + /// + private static bool HasSafetyComment(SyntaxToken unsafeKeyword) + { + if (ContainsSafetyComment(unsafeKeyword.LeadingTrivia)) + return true; + + for (SyntaxNode? node = unsafeKeyword.Parent; node is not null; node = node.Parent) + { + if (ContainsSafetyComment(node.GetLeadingTrivia())) + return true; + + // Stop at the statement or member that owns the region; trivia further out documents something else. + if (node is StatementSyntax or MemberDeclarationSyntax) + break; + } + + return false; + } + + private static bool ContainsSafetyComment(SyntaxTriviaList trivia) => + trivia.Any(static t => + (t.IsKind(SyntaxKind.SingleLineCommentTrivia) || t.IsKind(SyntaxKind.MultiLineCommentTrivia)) + && t.ToString().IndexOf(SafetyCommentPrefix, StringComparison.Ordinal) >= 0); + } +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs index 66fbe621f1628f..054993e964f6fc 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs @@ -17,6 +17,12 @@ internal static class UnsafeMigrationSyntaxHelpers // The analyzer builds against a Roslyn version that predates SyntaxKind.SafeKeyword. private static readonly SyntaxKind s_safeKeyword = SyntaxFacts.GetContextualKeywordKind("safe"); + /// + /// The kind of the safe contextual keyword, or when the hosting + /// compiler does not know it. + /// + internal static SyntaxKind SafeKeywordKind => s_safeKeyword; + internal static SyntaxTokenList GetModifiers(SyntaxNode declaration) => declaration switch { @@ -42,6 +48,19 @@ internal static SyntaxToken GetModifier(SyntaxNode declaration, SyntaxKind modif return default; } + + /// + /// Determines whether an unsafe keyword token introduces an unsafe(...) expression rather + /// than a modifier or a block. + /// + /// + /// UnsafeExpressionSyntax is newer than the Roslyn these analyzers compile against, but it derives + /// from , which is not, so the expression form is recognized by its parent's + /// base type. Matching on a following open parenthesis instead would misclassify the modifier on any + /// member whose type is a tuple, such as static unsafe (int, int) M(). + /// + internal static bool IsUnsafeExpressionKeyword(SyntaxToken token) => + token.IsKind(SyntaxKind.UnsafeKeyword) && token.Parent is ExpressionSyntax; } } #endif diff --git a/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs b/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs index 116954140fcd65..fe88cf20e08e38 100644 --- a/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs +++ b/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs @@ -223,6 +223,8 @@ public enum DiagnosticId // Memory safety migration diagnostic ids. UnsafeMemberMissingSafetyDocumentation = 5005, PointerSignatureRequiresUnsafe = 5006, + UnsafeBlockMissingSafetyComment = 5009, + SafeModifierMissingJustification = 5010, } public static class DiagnosticIdExtensions @@ -255,7 +257,7 @@ public static string GetDiagnosticCategory(this DiagnosticId diagnosticId) => { > 2000 and < 3000 => DiagnosticCategory.Trimming, >= 3000 and < 3050 => DiagnosticCategory.SingleFile, - 5005 or 5006 => DiagnosticCategory.Safety, + >= 5000 and < 5100 => DiagnosticCategory.Safety, >= 3050 and <= 6000 => DiagnosticCategory.AOT, _ => throw new ArgumentException($"The provided diagnostic id '{diagnosticId}' does not fall into the range of supported warning codes 2001 to 6000 (inclusive).") }; diff --git a/src/tools/illink/src/ILLink.Shared/SharedStrings.resx b/src/tools/illink/src/ILLink.Shared/SharedStrings.resx index dcb7b1bdc6cbab..e671b6599f6055 100644 --- a/src/tools/illink/src/ILLink.Shared/SharedStrings.resx +++ b/src/tools/illink/src/ILLink.Shared/SharedStrings.resx @@ -699,6 +699,18 @@ A member with a pointer or function-pointer signature must be marked 'unsafe' or include a '<safety>' XML documentation comment explaining why it is safe. + + Unsafe regions should document why they are safe + + + An 'unsafe' region should be preceded by a '// SAFETY:' comment explaining how its obligations are discharged. + + + Explicit 'safe' members should document why they are safe + + + A member marked 'safe' asserts a contract the compiler cannot verify and must include a '<safety>' XML documentation comment explaining why it holds. + An attribute element has property but this could not be found. diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs new file mode 100644 index 00000000000000..59257a945c9390 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs @@ -0,0 +1,129 @@ +// 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 Xunit; + +namespace ILLink.RoslynAnalyzer.Tests +{ + /// + /// Verifies the IL5009 code fix, which inserts a // SAFETY: TODO stub above an undocumented + /// unsafe region. + /// + public class AddSafetyCommentCodeFixTests + { + [Fact] + public async Task AddsCommentAboveUnsafeBlock() + { + string source = """ + class C + { + unsafe void M(int* p) + { + {|IL5009:unsafe|} + { + int x = *p; + } + } + } + """; + + string fixedSource = """ + class C + { + unsafe void M(int* p) + { + // SAFETY: TODO + unsafe + { + int x = *p; + } + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source, + fixedSource); + await test.RunAsync(); + } + + [Fact] + public async Task AddsCommentAboveStatementContainingUnsafeExpression() + { + string source = """ + class C + { + static unsafe int Read() => 0; + + void M() + { + int x = {|IL5009:unsafe|}(Read()); + } + } + """; + + string fixedSource = """ + class C + { + static unsafe int Read() => 0; + + void M() + { + // SAFETY: TODO + int x = unsafe(Read()); + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source, + fixedSource); + await test.RunAsync(); + } + + [Fact] + public async Task PreservesExistingLeadingComment() + { + string source = """ + class C + { + unsafe void M(int* p) + { + // Read the first element. + {|IL5009:unsafe|} + { + int x = *p; + } + } + } + """; + + string fixedSource = """ + class C + { + unsafe void M(int* p) + { + // SAFETY: TODO + // Read the first element. + unsafe + { + int x = *p; + } + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source, + fixedSource); + await test.RunAsync(); + } + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SafeModifierMissingJustificationAnalyzerTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SafeModifierMissingJustificationAnalyzerTests.cs new file mode 100644 index 00000000000000..cdfadb73bd6124 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SafeModifierMissingJustificationAnalyzerTests.cs @@ -0,0 +1,112 @@ +// 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 Xunit; + +namespace ILLink.RoslynAnalyzer.Tests +{ + /// + /// Verifies that IL5010 reports an explicit safe modifier that records no justification. + /// + public class SafeModifierMissingJustificationAnalyzerTests + { + [Fact] + public async Task ReportsUndocumentedSafeExternMethod() + { + string source = """ + using System.Runtime.InteropServices; + + class C + { + [DllImport("nativelib")] + public static {|IL5010:safe|} extern int M(int x); + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportDocumentedSafeExternMethod() + { + string source = """ + using System.Runtime.InteropServices; + + class C + { + /// The native entry point only reads the value it is given. + [DllImport("nativelib")] + public static safe extern int M(int x); + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task ReportsUndocumentedSafeFieldInExplicitLayout() + { + string source = """ + using System.Runtime.InteropServices; + + [StructLayout(LayoutKind.Explicit)] + struct S + { + [FieldOffset(0)] + public {|IL5010:safe|} int Value; + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportDocumentedSafeFieldInExplicitLayout() + { + string source = """ + using System.Runtime.InteropServices; + + [StructLayout(LayoutKind.Explicit)] + struct S + { + /// Both overlapping fields are unmanaged and the same size. + [FieldOffset(0)] + public safe int Value; + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportMembersWithoutSafeModifier() + { + string source = """ + using System.Runtime.InteropServices; + + class C + { + [DllImport("nativelib")] + public static unsafe extern int M(int x); + + public void Other() { } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + } +} +#endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs new file mode 100644 index 00000000000000..23865d12fcf5f5 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs @@ -0,0 +1,212 @@ +// 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 Xunit; + +namespace ILLink.RoslynAnalyzer.Tests +{ + /// + /// Verifies that IL5009 reports undocumented unsafe regions and respects an existing + /// // SAFETY: comment. + /// + public class UnsafeBlockMissingSafetyCommentAnalyzerTests + { + [Fact] + public async Task ReportsUndocumentedUnsafeBlock() + { + string source = """ + class C + { + unsafe void M(int* p) + { + {|IL5009:unsafe|} + { + int x = *p; + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Theory] + [InlineData("// SAFETY: p is validated by the caller")] + [InlineData("// SAFETY:")] + [InlineData("/* SAFETY: p is validated by the caller */")] + public async Task DoesNotReportDocumentedUnsafeBlock(string comment) + { + string source = $$""" + class C + { + unsafe void M(int* p) + { + {{comment}} + unsafe + { + int x = *p; + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportNestedUnsafeBlock() + { + // The nested region is covered by the reasoning recorded for the one that contains it. + string source = """ + class C + { + unsafe void M(int* p) + { + // SAFETY: p is validated by the caller + unsafe + { + unsafe + { + int x = *p; + } + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task ReportsUndocumentedUnsafeExpression() + { + string source = """ + class C + { + static unsafe int Read() => 0; + + void M() + { + int x = {|IL5009:unsafe|}(Read()); + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportDocumentedUnsafeExpression() + { + // An unsafe expression sits inside a larger statement, so its comment is written above that statement. + string source = """ + class C + { + static unsafe int Read() => 0; + + void M() + { + // SAFETY: Read only touches memory it owns + int x = unsafe(Read()); + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task ReportsUndocumentedUnsafeExpressionInFieldInitializer() + { + // A field initializer cannot contain an unsafe block, so the expression form is the only option. + string source = """ + class C + { + static unsafe int Read() => 0; + + static int Value = {|IL5009:unsafe|}(Read()); + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportUnsafeModifierOnTupleReturningMember() + { + // The 'unsafe' modifier is followed by '(' here, but it introduces no region. This shape is common, + // for example across the hardware intrinsics APIs. + string source = """ + class C + { + public static unsafe (int, int) GetPair() => default; + + unsafe (int, int) Prop { get; set; } + + unsafe (int, int) Field; + + unsafe (int, int) this[int i] => default; + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportUnsafeExpressionNestedInDocumentedBlock() + { + string source = """ + class C + { + static unsafe int Read() => 0; + + void M() + { + // SAFETY: Read only touches memory it owns + unsafe + { + int y = unsafe(Read()); + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportUnsafeModifier() + { + // The modifier is a contract, not a region. IL5005 covers its documentation. + string source = """ + class C + { + unsafe void M() { } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + } +} +#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..c3c983e8ce5c78 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs @@ -70,7 +70,9 @@ internal static Solution SetOptions(Solution solution, ProjectId projectId) var diagnosticOptions = compilationOptions.SpecificDiagnosticOptions .SetItems(CSharpVerifierHelper.NullableWarnings) .SetItem(DiagnosticId.UnsafeMemberMissingSafetyDocumentation.AsString(), ReportDiagnostic.Warn) - .SetItem(DiagnosticId.PointerSignatureRequiresUnsafe.AsString(), ReportDiagnostic.Warn); + .SetItem(DiagnosticId.PointerSignatureRequiresUnsafe.AsString(), ReportDiagnostic.Warn) + .SetItem(DiagnosticId.UnsafeBlockMissingSafetyComment.AsString(), ReportDiagnostic.Warn) + .SetItem(DiagnosticId.SafeModifierMissingJustification.AsString(), ReportDiagnostic.Warn); compilationOptions = compilationOptions .WithAllowUnsafe(true) .WithWarningLevel(999) From dd9665974b2264e9c54663edcf214ff314fd5e9c Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 28 Jul 2026 14:52:22 +0200 Subject: [PATCH 2/3] Match the SAFETY marker as a whole word Replace the substring check with a case-sensitive whole-word regex so that '// UNSAFETY:' and '// SAFETYNET' no longer count as safety comments, and a marker is only recognized when it is followed by a colon. Keeping the marker case-sensitive keeps the convention greppable across a code base. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5 --- ...UnsafeBlockMissingSafetyCommentAnalyzer.cs | 14 ++++- ...eBlockMissingSafetyCommentAnalyzerTests.cs | 56 +++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs index 227e4e4a73a000..7b185051e401ab 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs @@ -2,9 +2,9 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. #if DEBUG -using System; using System.Collections.Immutable; using System.Linq; +using System.Text.RegularExpressions; using ILLink.Shared; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -28,7 +28,15 @@ namespace ILLink.RoslynAnalyzer [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class UnsafeBlockMissingSafetyCommentAnalyzer : DiagnosticAnalyzer { - internal const string SafetyCommentPrefix = "SAFETY"; + /// + /// Matches the SAFETY: marker anywhere inside a comment. + /// + /// + /// The marker is matched as a whole word so that // UNSAFETY: or // SAFETYNET do not count, + /// and it is case-sensitive to keep the convention greppable across a code base. Matching anywhere in the + /// comment allows the marker to sit on an inner line of a block comment. + /// + private static readonly Regex s_safetyComment = new(@"\bSAFETY\s*:", RegexOptions.CultureInvariant); private static readonly DiagnosticDescriptor s_rule = DiagnosticDescriptors.GetDiagnosticDescriptor( @@ -117,7 +125,7 @@ private static bool HasSafetyComment(SyntaxToken unsafeKeyword) private static bool ContainsSafetyComment(SyntaxTriviaList trivia) => trivia.Any(static t => (t.IsKind(SyntaxKind.SingleLineCommentTrivia) || t.IsKind(SyntaxKind.MultiLineCommentTrivia)) - && t.ToString().IndexOf(SafetyCommentPrefix, StringComparison.Ordinal) >= 0); + && s_safetyComment.IsMatch(t.ToString())); } } #endif diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs index 23865d12fcf5f5..8634a195402ca3 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs @@ -37,6 +37,8 @@ await UnsafeMigrationTestHelpers [Theory] [InlineData("// SAFETY: p is validated by the caller")] [InlineData("// SAFETY:")] + [InlineData("//SAFETY: no space after the comment marker")] + [InlineData("// Reads one element. SAFETY: p is validated by the caller")] [InlineData("/* SAFETY: p is validated by the caller */")] public async Task DoesNotReportDocumentedUnsafeBlock(string comment) { @@ -59,6 +61,60 @@ await UnsafeMigrationTestHelpers .RunAsync(); } + [Theory] + // The marker is matched as a whole word, so these are not safety comments. + [InlineData("// UNSAFETY: this is a different word")] + [InlineData("// SAFETYNET: this is a different word")] + // It is case-sensitive so the convention stays greppable. + [InlineData("// safety: lowercase does not count")] + [InlineData("// Safety: mixed case does not count")] + // The marker has to be followed by a colon. + [InlineData("// SAFETY concerns are handled elsewhere")] + public async Task ReportsUnsafeBlockWithoutTheSafetyMarker(string comment) + { + string source = $$""" + class C + { + unsafe void M(int* p) + { + {{comment}} + {|IL5009:unsafe|} + { + int x = *p; + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + + [Fact] + public async Task DoesNotReportMarkerOnInnerLineOfBlockComment() + { + string source = """ + class C + { + unsafe void M(int* p) + { + /* + * SAFETY: p is validated by the caller + */ + unsafe + { + int x = *p; + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .RunAsync(); + } + [Fact] public async Task DoesNotReportNestedUnsafeBlock() { From 1ef48e93f8cbf2731f23b76dfb03cd55f5f01e2e Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 28 Jul 2026 21:14:34 +0200 Subject: [PATCH 3/3] Place the SAFETY comment next to the region it describes The fixer inserted the comment above all existing leading trivia, which contradicted its own doc comment and pushed the marker away from the code it explains. Insert it directly above the target instead, so XML documentation keeps its place on top and the marker stays adjacent to the region, matching the Rust convention the marker is modeled on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5 --- .../AddSafetyCommentCodeFixProvider.cs | 28 +++++++------- .../AddSafetyCommentCodeFixTests.cs | 37 ++++++++++++++++++- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs index e66ae5ce89600f..33aff5f5253163 100644 --- a/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs +++ b/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs @@ -72,31 +72,29 @@ private static async Task AddSafetyCommentAsync( { var editor = await DocumentEditor.CreateAsync(document, cancellationToken).ConfigureAwait(false); - // The comment goes on its own line directly above the target, keeping any existing leading trivia - // such as XML documentation or other comments in place. + // The comment goes on its own line immediately above the target, below any existing leading trivia, + // so that it stays adjacent to the code it describes and XML documentation keeps its place on top. SyntaxTriviaList leadingTrivia = target.GetLeadingTrivia(); - int insertionIndex = GetInsertionIndex(leadingTrivia); - SyntaxTriviaList updated = leadingTrivia.InsertRange( - insertionIndex, - new[] { SyntaxFactory.Comment(SafetyComment), SyntaxFactory.ElasticCarriageReturnLineFeed }); + SyntaxTriviaList indentation = GetIndentation(leadingTrivia); + SyntaxTriviaList updated = leadingTrivia + .Add(SyntaxFactory.Comment(SafetyComment)) + .Add(SyntaxFactory.ElasticCarriageReturnLineFeed) + .AddRange(indentation); editor.ReplaceNode(target, target.WithLeadingTrivia(updated)); return editor.GetChangedDocument(); } /// - /// Inserts after any trivia that must stay attached to the line above, such as directives. + /// Returns the whitespace that indents the target, so the inserted comment and the target line up. /// - private static int GetInsertionIndex(SyntaxTriviaList leadingTrivia) + private static SyntaxTriviaList GetIndentation(SyntaxTriviaList leadingTrivia) { - int index = 0; - for (int i = 0; i < leadingTrivia.Count; i++) - { - if (leadingTrivia[i].IsDirective || leadingTrivia[i].IsKind(SyntaxKind.DisabledTextTrivia)) - index = i + 1; - } + int start = leadingTrivia.Count; + while (start > 0 && leadingTrivia[start - 1].IsKind(SyntaxKind.WhitespaceTrivia)) + start--; - return index; + return SyntaxFactory.TriviaList(leadingTrivia.Skip(start)); } /// diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs index 59257a945c9390..da7a0641d73eb4 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs @@ -87,8 +87,9 @@ void M() } [Fact] - public async Task PreservesExistingLeadingComment() + public async Task PlacesCommentBelowExistingLeadingComment() { + // The safety comment belongs next to the region it describes, so it goes below an existing comment. string source = """ class C { @@ -108,8 +109,8 @@ class C { unsafe void M(int* p) { - // SAFETY: TODO // Read the first element. + // SAFETY: TODO unsafe { int x = *p; @@ -124,6 +125,38 @@ unsafe void M(int* p) fixedSource); await test.RunAsync(); } + + [Fact] + public async Task PlacesCommentBelowXmlDocumentation() + { + // XML documentation keeps its place directly above the declaration it documents. + string source = """ + class C + { + static unsafe int Read() => 0; + + /// The initial value. + static int Value = {|IL5009:unsafe|}(Read()); + } + """; + + string fixedSource = """ + class C + { + static unsafe int Read() => 0; + + /// The initial value. + // SAFETY: TODO + static int Value = unsafe(Read()); + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source, + fixedSource); + await test.RunAsync(); + } } } #endif