ILLink: codefix for C# unsafe evolution

src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Shipped.md

@@ -0,0 +1,2 @@
+; Shipped analyzer releases
+; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

src/tools/illink/src/ILLink.CodeFix/AnalyzerReleases.Unshipped.md

@@ -0,0 +1,9 @@
+; Unshipped analyzer release
+; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md
+
+### New Rules
+
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+IL5005  | MemorySafety | Info | UnsafeEvolutionAnalyzer, [Documentation](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md)
+IL5006  | MemorySafety | Info | UnsafeEvolutionAnalyzer, [Documentation](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md)

src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj

@@ -21,4 +21,26 @@
     <EmbeddedResource Include="Resources.resx" GenerateSource="true" />
   </ItemGroup>
 
+  <!-- Keep this in sync with the '#if DEBUG' gate on UnsafeEvolutionAnalyzer and its
+       descriptors: AnalyzerReleases.Unshipped.md declares IL5005/IL5006, which are
+       only supported when the analyzer is compiled in Debug.
+       Microsoft.CodeAnalysis.Analyzers auto-includes AnalyzerReleases.*.md when the
+       file exists; we Remove the unshipped one in non-Debug builds inside a Target
+       so the Remove runs after the SDK package has done its auto-include, otherwise
+       RS2002 fires ("rule X is part of the next unshipped analyzer release, but is
+       not a supported diagnostic for any analyzer"). -->
+  <ItemGroup>
+    <AdditionalFiles Include="AnalyzerReleases.Shipped.md" />
+    <AdditionalFiles Include="AnalyzerReleases.Unshipped.md" Condition="'$(Configuration)' == 'Debug'" />
+  </ItemGroup>
+
+  <Target Name="_RemoveUnshippedAnalyzerReleasesOutsideDebug"
+          BeforeTargets="CoreCompile"
+          Condition="'$(Configuration)' != 'Debug'">
+    <ItemGroup>
+      <AdditionalFiles Remove="@(AdditionalFiles)"
+                       Condition="'%(Filename)%(Extension)' == 'AnalyzerReleases.Unshipped.md'" />
+    </ItemGroup>
+  </Target>
+
 </Project>

src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/RemoveUnsafeModifierCodeFixProvider.cs

@@ -0,0 +1,98 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#if DEBUG
+using System.Collections.Immutable;
+using System.Composition;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CodeActions;
+using Microsoft.CodeAnalysis.CodeFixes;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Editing;
+
+namespace ILLink.CodeFix.UnsafeEvolution
+{
+    /// <summary>
+    /// Removes the <c>unsafe</c> modifier from declarations where it is meaningless
+    /// (IL5005 / CS9377) or where it is probably unnecessary (IL5006).
+    /// </summary>
+    /// <remarks>
+    /// The fixer is intentionally conservative: it only removes the modifier itself,
+    /// never the entire declaration, and the <see cref="UnsafeEvolutionAnalyzer"/> rules
+    /// already exclude cases that are not safe to rewrite (extern, partial, nested under
+    /// an unsafe type, etc).
+    /// </remarks>
+    [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(RemoveUnsafeModifierCodeFixProvider)), Shared]
+    public sealed class RemoveUnsafeModifierCodeFixProvider : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider
+    {
+        private const string Title = "Remove 'unsafe' modifier";
+
+        public override ImmutableArray<string> FixableDiagnosticIds =>
+        [
+            UnsafeEvolutionDescriptors.MeaninglessUnsafeModifierId,
+            UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifierId,
+            UnsafeEvolutionDescriptors.UnsafeMeaningless,
+        ];
+
+        public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
+
+        public override async Task RegisterCodeFixesAsync(CodeFixContext context)
+        {
+            var document = context.Document;
+            var diagnostic = context.Diagnostics.First();
+
+            if (await document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false) is not { } root)
+                return;
+
+            var declaration = FindDeclarationWithUnsafeModifier(root, diagnostic.Location.SourceSpan);
+            if (declaration is null)
+                return;
+
+            context.RegisterCodeFix(
+                CodeAction.Create(
+                    title: Title,
+                    createChangedDocument: ct => RemoveUnsafeAsync(document, declaration, ct),
+                    equivalenceKey: Title),
+                diagnostic);
+        }
+
+        private static SyntaxNode? FindDeclarationWithUnsafeModifier(SyntaxNode root, Microsoft.CodeAnalysis.Text.TextSpan span)
+        {
+            var token = root.FindToken(span.Start);
+            for (var node = token.Parent; node is not null; node = node.Parent)
+            {
+                if (GetModifiers(node) is { } modifiers && modifiers.Any(SyntaxKind.UnsafeKeyword))
+                    return node;
+            }
+            return null;
+        }
+
+        private static SyntaxTokenList? GetModifiers(SyntaxNode node) => node switch
+        {
+            BaseTypeDeclarationSyntax t => t.Modifiers,
+            DelegateDeclarationSyntax d => d.Modifiers,
+            BaseMethodDeclarationSyntax m => m.Modifiers,
+            LocalFunctionStatementSyntax lf => lf.Modifiers,
+            BasePropertyDeclarationSyntax p => p.Modifiers,
+            BaseFieldDeclarationSyntax f => f.Modifiers,
+            AccessorDeclarationSyntax a => a.Modifiers,
+            _ => null,
+        };
+
+        private static async Task<Document> RemoveUnsafeAsync(Document document, SyntaxNode declaration, CancellationToken ct)
+        {
+            // Use SyntaxGenerator to remove the modifier - this correctly transfers leading
+            // trivia (indentation, comments) from the removed token to whichever syntax now
+            // occupies the leading position of the declaration.
+            var editor = await DocumentEditor.CreateAsync(document, ct).ConfigureAwait(false);
+            var modifiers = editor.Generator.GetModifiers(declaration);
+            editor.SetModifiers(declaration, modifiers.WithIsUnsafe(false));
+            return editor.GetChangedDocument();
+        }
+    }
+}
+#endif

src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeBlockHelpers.cs

@@ -0,0 +1,100 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#if DEBUG
+using System.Linq;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace ILLink.CodeFix.UnsafeEvolution
+{
+    /// <summary>
+    /// Helpers shared by the unsafe-evolution analyzer and code fixers.
+    /// </summary>
+    internal static class UnsafeBlockHelpers
+    {
+        /// <summary>
+        /// True if any pointer-typed or function-pointer-typed syntax appears anywhere
+        /// within <paramref name="type"/> (including nested generics, arrays, etc.).
+        /// </summary>
+        internal static bool ContainsPointerType(TypeSyntax? type)
+        {
+            if (type is null)
+                return false;
+
+            return type is PointerTypeSyntax or FunctionPointerTypeSyntax
+                || type.DescendantNodes().Any(static n => n is PointerTypeSyntax or FunctionPointerTypeSyntax);
+        }
+
+        /// <summary>
+        /// True if any parameter or return type of the given declaration mentions a pointer
+        /// or function-pointer type. Used as the heuristic gate for IL5006.
+        /// </summary>
+        internal static bool SignatureContainsPointer(SyntaxNode declaration) => declaration switch
+        {
+            MethodDeclarationSyntax m => SignatureHasPointer(m.ReturnType, m.ParameterList),
+            LocalFunctionStatementSyntax lf => SignatureHasPointer(lf.ReturnType, lf.ParameterList),
+            OperatorDeclarationSyntax op => SignatureHasPointer(op.ReturnType, op.ParameterList),
+            ConversionOperatorDeclarationSyntax co => SignatureHasPointer(co.Type, co.ParameterList),
+            DelegateDeclarationSyntax d => SignatureHasPointer(d.ReturnType, d.ParameterList),
+            ConstructorDeclarationSyntax c => SignatureHasPointer(returnType: null, c.ParameterList),
+            IndexerDeclarationSyntax idx => SignatureHasPointer(idx.Type, idx.ParameterList),
+            BasePropertyDeclarationSyntax bp => ContainsPointerType(bp.Type),    // PropertyDeclaration, EventDeclaration
+            BaseFieldDeclarationSyntax bf => ContainsPointerType(bf.Declaration.Type), // FieldDeclaration, EventFieldDeclaration
+            AccessorDeclarationSyntax acc => acc.Parent?.Parent is { } owner && SignatureContainsPointer(owner),
+            _ => false,
+        };
+
+        private static bool SignatureHasPointer(TypeSyntax? returnType, BaseParameterListSyntax parameterList)
+            => ContainsPointerType(returnType)
+                || parameterList.Parameters.Any(static p => ContainsPointerType(p.Type));
+
+        /// <summary>
+        /// The first <c>unsafe</c> keyword in <paramref name="modifiers"/>, or <c>default</c> if absent.
+        /// </summary>
+        internal static SyntaxToken FindUnsafeModifier(SyntaxTokenList modifiers)
+            => modifiers.FirstOrDefault(static t => t.IsKind(SyntaxKind.UnsafeKeyword));
+
+        /// <summary>
+        /// True when <paramref name="node"/> carries preprocessor directive trivia STRICTLY BETWEEN
+        /// its first and last tokens. Directives that sit in the leading trivia of the first token
+        /// or in the trailing trivia of the last token (i.e. an enclosing <c>#if/#endif</c>) are
+        /// excluded because preserving them across a rewrite is straightforward.
+        /// </summary>
+        internal static bool ContainsInternalDirectiveTrivia(SyntaxNode node)
+        {
+            int internalStart = node.GetFirstToken().Span.End;
+            int internalEnd = node.GetLastToken().Span.Start;
+            if (internalEnd <= internalStart)
+                return false;
+
+            foreach (var trivia in node.DescendantTrivia(descendIntoTrivia: true))
+            {
+                if (trivia.IsDirective
+                    && trivia.SpanStart >= internalStart
+                    && trivia.Span.End <= internalEnd)
+                {
+                    return true;
+                }
+            }
+            return false;
+        }
+
+        /// <summary>
+        /// Heuristic. Returns true if a member's body has enough unsafe diagnostics that
+        /// wrapping the whole body in one <c>unsafe { }</c> is preferable to wrapping each
+        /// statement individually.
+        /// </summary>
+        /// <param name="unsafeDiagnosticCount">Number of CS9360/CS9361/CS9362/CS0214 diagnostics in the body.</param>
+        /// <param name="totalStatementCount">Total number of statements in the body.</param>
+        internal static bool ShouldWrapEntireBody(int unsafeDiagnosticCount, int totalStatementCount)
+        {
+            if (unsafeDiagnosticCount <= 0)
+                return false;
+            // Three or more unsafe operations and at least every 4th statement is unsafe.
+            return unsafeDiagnosticCount >= 3 && unsafeDiagnosticCount * 4 >= totalStatementCount;
+        }
+    }
+}
+#endif

src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/UnsafeEvolutionAnalyzer.cs

@@ -0,0 +1,152 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#if DEBUG
+using System.Collections.Immutable;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace ILLink.CodeFix.UnsafeEvolution
+{
+    /// <summary>
+    /// Reports IL5005 (meaningless <c>unsafe</c> modifier on a type / static ctor / destructor / delegate)
+    /// and IL5006 (probably-unnecessary <c>unsafe</c> modifier on a member whose signature has no pointer types).
+    /// </summary>
+    /// <remarks>
+    /// These supplement compiler diagnostics so that <c>dotnet format</c> can drive the
+    /// <see cref="RemoveUnsafeModifierCodeFixProvider"/> on assemblies that have not yet opted into
+    /// the updated memory-safety rules.
+    /// </remarks>
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class UnsafeEvolutionAnalyzer : DiagnosticAnalyzer
+    {
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
+        [
+            UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier,
+            UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier,
+        ];
+
+        public override void Initialize(AnalysisContext context)
+        {
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+            context.EnableConcurrentExecution();
+
+            // IL5005 - meaningless unsafe on types and special members.
+            context.RegisterSyntaxNodeAction(AnalyzeMeaninglessUnsafe,
+                SyntaxKind.ClassDeclaration,
+                SyntaxKind.StructDeclaration,
+                SyntaxKind.InterfaceDeclaration,
+                SyntaxKind.RecordDeclaration,
+                SyntaxKind.RecordStructDeclaration,
+                SyntaxKind.DelegateDeclaration,
+                SyntaxKind.ConstructorDeclaration,
+                SyntaxKind.DestructorDeclaration);
+
+            // IL5006 - probably-unnecessary unsafe on a member whose signature has no pointer types.
+            context.RegisterSyntaxNodeAction(AnalyzeUnnecessaryUnsafe,
+                SyntaxKind.MethodDeclaration,
+                SyntaxKind.LocalFunctionStatement,
+                SyntaxKind.PropertyDeclaration,
+                SyntaxKind.IndexerDeclaration,
+                SyntaxKind.FieldDeclaration,
+                SyntaxKind.EventFieldDeclaration,
+                SyntaxKind.EventDeclaration);
+        }
+
+        // ---- IL5005: meaningless 'unsafe' ----
+
+        private static void AnalyzeMeaninglessUnsafe(SyntaxNodeAnalysisContext context)
+        {
+            var node = context.Node;
+            var (modifiers, kind, name) = node switch
+            {
+                ClassDeclarationSyntax c => (c.Modifiers, "class", c.Identifier.ValueText),
+                StructDeclarationSyntax s => (s.Modifiers, "struct", s.Identifier.ValueText),
+                InterfaceDeclarationSyntax i => (i.Modifiers, "interface", i.Identifier.ValueText),
+                RecordDeclarationSyntax { ClassOrStructKeyword.RawKind: (int)SyntaxKind.StructKeyword } r
+                    => (r.Modifiers, "record struct", r.Identifier.ValueText),
+                RecordDeclarationSyntax r => (r.Modifiers, "record", r.Identifier.ValueText),
+                DelegateDeclarationSyntax d => (d.Modifiers, "delegate", d.Identifier.ValueText),
+                ConstructorDeclarationSyntax c => (c.Modifiers, "static constructor", c.Identifier.ValueText),
+                DestructorDeclarationSyntax d => (d.Modifiers, "destructor", d.Identifier.ValueText),
+                _ => (default(SyntaxTokenList), "", ""),
+            };
+
+            // Instance constructors can legitimately be requires-unsafe; only static ones are meaningless.
+            if (node is ConstructorDeclarationSyntax && !modifiers.Any(SyntaxKind.StaticKeyword))
+                return;
+
+            var unsafeToken = UnsafeBlockHelpers.FindUnsafeModifier(modifiers);
+            if (unsafeToken == default)
+                return;
+
+            context.ReportDiagnostic(Diagnostic.Create(
+                UnsafeEvolutionDescriptors.MeaninglessUnsafeModifier,
+                unsafeToken.GetLocation(),
+                kind,
+                name));
+        }
+
+        // ---- IL5006: probably-unnecessary 'unsafe' on a signature without pointers ----
+
+        private static void AnalyzeUnnecessaryUnsafe(SyntaxNodeAnalysisContext context)
+        {
+            var node = context.Node;
+            var (modifiers, name) = node switch
+            {
+                MethodDeclarationSyntax m => (m.Modifiers, m.Identifier.ValueText),
+                LocalFunctionStatementSyntax lf => (lf.Modifiers, lf.Identifier.ValueText),
+                PropertyDeclarationSyntax p => (p.Modifiers, p.Identifier.ValueText),
+                IndexerDeclarationSyntax => (((IndexerDeclarationSyntax)node).Modifiers, "this[]"),
+                FieldDeclarationSyntax f => (f.Modifiers, FirstVariableName(f.Declaration)),
+                EventFieldDeclarationSyntax ef => (ef.Modifiers, FirstVariableName(ef.Declaration)),
+                EventDeclarationSyntax e => (e.Modifiers, e.Identifier.ValueText),
+                _ => (default(SyntaxTokenList), ""),
+            };
+
+            if (!HasRemovableUnsafe(node, modifiers))
+                return;
+
+            if (UnsafeBlockHelpers.SignatureContainsPointer(node))
+                return;
+
+            var unsafeToken = UnsafeBlockHelpers.FindUnsafeModifier(modifiers);
+            context.ReportDiagnostic(Diagnostic.Create(
+                UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier,
+                unsafeToken.GetLocation(),
+                name));
+        }
+
+        private static string FirstVariableName(VariableDeclarationSyntax decl)
+            => decl.Variables.Count > 0 ? decl.Variables[0].Identifier.ValueText : "?";
+
+        // ---- Common 'should we suggest removing the unsafe modifier?' predicate ----
+
+        private static bool HasRemovableUnsafe(SyntaxNode decl, SyntaxTokenList modifiers)
+        {
+            if (!modifiers.Any(SyntaxKind.UnsafeKeyword))
+                return false;
+
+            // 'extern' members must be explicitly marked unsafe or safe in the new rules; don't suggest removal.
+            if (modifiers.Any(SyntaxKind.ExternKeyword))
+                return false;
+
+            // Partial members require both halves to agree on 'unsafe'; we can't fix one safely.
+            if (modifiers.Any(SyntaxKind.PartialKeyword))
+                return false;
+
+            // Be conservative for members nested inside a type that also carries 'unsafe' - the
+            // type-level IL5005 will fire on the containing type, which is the better fix.
+            for (SyntaxNode? p = decl.Parent; p is not null; p = p.Parent)
+            {
+                if (p is TypeDeclarationSyntax td && td.Modifiers.Any(SyntaxKind.UnsafeKeyword))
+                    return false;
+            }
+
+            return true;
+        }
+    }
+}
+#endif