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..33aff5f5253163 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs @@ -0,0 +1,120 @@ +// 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 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(); + 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(); + } + + /// + /// Returns the whitespace that indents the target, so the inserted comment and the target line up. + /// + private static SyntaxTriviaList GetIndentation(SyntaxTriviaList leadingTrivia) + { + int start = leadingTrivia.Count; + while (start > 0 && leadingTrivia[start - 1].IsKind(SyntaxKind.WhitespaceTrivia)) + start--; + + return SyntaxFactory.TriviaList(leadingTrivia.Skip(start)); + } + + /// + /// 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..7b185051e401ab --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs @@ -0,0 +1,131 @@ +// 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.Linq; +using System.Text.RegularExpressions; +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 + { + /// + /// 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( + 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)) + && s_safetyComment.IsMatch(t.ToString())); + } +} +#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..da7a0641d73eb4 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs @@ -0,0 +1,162 @@ +// 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 PlacesCommentBelowExistingLeadingComment() + { + // The safety comment belongs next to the region it describes, so it goes below an existing comment. + 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) + { + // Read the first element. + // SAFETY: TODO + unsafe + { + int x = *p; + } + } + } + """; + + var test = UnsafeMigrationTestHelpers + .CreateCodeFixTest( + source, + 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 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..8634a195402ca3 --- /dev/null +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs @@ -0,0 +1,268 @@ +// 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: 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) + { + string source = $$""" + class C + { + unsafe void M(int* p) + { + {{comment}} + unsafe + { + int x = *p; + } + } + } + """; + + await UnsafeMigrationTestHelpers + .CreateAnalyzerTest(source) + .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() + { + // 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)