diff --git a/eng/liveILLink.targets b/eng/liveILLink.targets index d51194e4c25619..011394f1d2fd64 100644 --- a/eng/liveILLink.targets +++ b/eng/liveILLink.targets @@ -5,6 +5,7 @@ + true <_RequiresLiveILLink Condition="'$(_RequiresLiveILLink)' == '' And ( '$(PublishAot)' == 'true' Or '$(IsAotCompatible)' == 'true' Or '$(EnableAotAnalyzer)' == 'true' Or @@ -12,6 +13,7 @@ '$(IsTrimmable)' == 'true' Or '$(EnableTrimAnalyzer)' == 'true' Or '$(EnableSingleFileAnalyzer)' == 'true' Or '$(EnableUnsafeAnalyzer)' == 'true')">true + $(Features);updated-memory-safety-rules diff --git a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeToExternCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeToExternCodeFixProvider.cs new file mode 100644 index 00000000000000..c78bc353ce3e59 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeToExternCodeFixProvider.cs @@ -0,0 +1,33 @@ +// 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.Threading.Tasks; +using ILLink.CodeFixProvider; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeFixes; +using RoslynCodeFixProvider = Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider; + +namespace ILLink.CodeFix; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AddUnsafeToExternCodeFixProvider)), Shared] +public sealed class AddUnsafeToExternCodeFixProvider : RoslynCodeFixProvider +{ + private static LocalizableString CodeFixTitle => new LocalizableResourceString( + nameof(Resources.AddUnsafeModifierCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => ["CS9389"]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + => UnsafeModifierCodeFixHelpers.RegisterCodeFixAsync( + context, + CodeFixTitle.ToString(), + shouldHaveUnsafeModifier: true); +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/AddUnsafeToPointerSignatureCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeToPointerSignatureCodeFixProvider.cs new file mode 100644 index 00000000000000..01204839247560 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/AddUnsafeToPointerSignatureCodeFixProvider.cs @@ -0,0 +1,35 @@ +// 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.Threading.Tasks; +using ILLink.CodeFixProvider; +using ILLink.Shared; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeFixes; +using RoslynCodeFixProvider = Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider; + +namespace ILLink.CodeFix; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AddUnsafeToPointerSignatureCodeFixProvider)), Shared] +public sealed class AddUnsafeToPointerSignatureCodeFixProvider : RoslynCodeFixProvider +{ + private static LocalizableString CodeFixTitle => new LocalizableResourceString( + nameof(Resources.AddUnsafeModifierCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => + [DiagnosticId.PointerSignatureRequiresUnsafe.AsString()]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + => UnsafeModifierCodeFixHelpers.RegisterCodeFixAsync( + context, + CodeFixTitle.ToString(), + shouldHaveUnsafeModifier: true); +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs new file mode 100644 index 00000000000000..857d33df895fd6 --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs @@ -0,0 +1,43 @@ +// 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.Globalization; +using System.Threading.Tasks; +using ILLink.CodeFixProvider; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeFixes; +using RoslynCodeFixProvider = Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider; + +namespace ILLink.CodeFix; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(RemoveInvalidUnsafeCodeFixProvider)), Shared] +public sealed class RemoveInvalidUnsafeCodeFixProvider : RoslynCodeFixProvider +{ + private static LocalizableString CodeFixTitle => new LocalizableResourceString( + nameof(Resources.RemoveUnsafeModifierCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => ["CS0106", "CS9377"]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + { + if (context.Diagnostics[0] is { Id: "CS0106" } diagnostic && + !diagnostic.GetMessage(CultureInfo.InvariantCulture) + .Contains("'unsafe'")) + { + return Task.CompletedTask; + } + + return UnsafeModifierCodeFixHelpers.RegisterCodeFixAsync( + context, + CodeFixTitle.ToString(), + shouldHaveUnsafeModifier: false); + } +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/RemoveUndocumentedUnsafeCodeFixProvider.cs b/src/tools/illink/src/ILLink.CodeFix/RemoveUndocumentedUnsafeCodeFixProvider.cs new file mode 100644 index 00000000000000..12d90e238d394f --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/RemoveUndocumentedUnsafeCodeFixProvider.cs @@ -0,0 +1,35 @@ +// 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.Threading.Tasks; +using ILLink.CodeFixProvider; +using ILLink.Shared; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeFixes; +using RoslynCodeFixProvider = Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider; + +namespace ILLink.CodeFix; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(RemoveUndocumentedUnsafeCodeFixProvider)), Shared] +public sealed class RemoveUndocumentedUnsafeCodeFixProvider : RoslynCodeFixProvider +{ + private static LocalizableString CodeFixTitle => new LocalizableResourceString( + nameof(Resources.RemoveUnsafeModifierCodeFixTitle), + Resources.ResourceManager, + typeof(Resources)); + + public override ImmutableArray FixableDiagnosticIds => + [DiagnosticId.UnsafeMemberMissingSafetyDocumentation.AsString()]; + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + => UnsafeModifierCodeFixHelpers.RegisterCodeFixAsync( + context, + CodeFixTitle.ToString(), + shouldHaveUnsafeModifier: false); +} +#endif diff --git a/src/tools/illink/src/ILLink.CodeFix/Resources.resx b/src/tools/illink/src/ILLink.CodeFix/Resources.resx index d552d7228b3e62..fad29d76a80231 100644 --- a/src/tools/illink/src/ILLink.CodeFix/Resources.resx +++ b/src/tools/illink/src/ILLink.CodeFix/Resources.resx @@ -129,6 +129,12 @@ Add 'unsafe' to parent method + + Add 'unsafe' modifier + + + Remove 'unsafe' modifier + Add UnconditionalSuppressMessage attribute to parent method diff --git a/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs new file mode 100644 index 00000000000000..c3a3a836d574ac --- /dev/null +++ b/src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs @@ -0,0 +1,81 @@ +// 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; +using System.Threading.Tasks; +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.Editing; + +namespace ILLink.CodeFix; + +internal static class UnsafeModifierCodeFixHelpers +{ + public static Task RegisterCodeFixAsync( + CodeFixContext context, + string title, + bool shouldHaveUnsafeModifier) + { + if (!IsMigrationEnabled(context.Document)) + return Task.CompletedTask; + + Diagnostic diagnostic = context.Diagnostics[0]; + context.RegisterCodeFix( + CodeAction.Create( + title, + cancellationToken => SetUnsafeModifierAsync( + context.Document, + diagnostic, + shouldHaveUnsafeModifier, + cancellationToken), + title), + diagnostic); + + return Task.CompletedTask; + } + + private static async Task SetUnsafeModifierAsync( + Document document, + Diagnostic diagnostic, + bool shouldHaveUnsafeModifier, + CancellationToken cancellationToken) + { + if (await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) is not { } root || + UnsafeMemberAnalysis.FindDeclaration( + root, + diagnostic.Location.SourceSpan.Start) is not { } declaration) + { + return document; + } + + SyntaxGenerator generator = SyntaxGenerator.GetGenerator(document); + SyntaxNode replacement = declaration is AccessorDeclarationSyntax accessor && + shouldHaveUnsafeModifier && + !UnsafeMemberAnalysis.HasUnsafeModifier(accessor) + ? accessor + .WithModifiers(accessor.Modifiers.Add( + SyntaxFactory.Token(SyntaxKind.UnsafeKeyword) + .WithTrailingTrivia(SyntaxFactory.Space))) + .WithKeyword(accessor.Keyword.WithLeadingTrivia()) + .WithLeadingTrivia(accessor.GetLeadingTrivia()) + : generator.WithModifiers( + declaration, + generator.GetModifiers(declaration).WithIsUnsafe(shouldHaveUnsafeModifier)) + .WithLeadingTrivia(declaration.GetLeadingTrivia()); + + return document.WithSyntaxRoot(root.ReplaceNode(declaration, replacement)); + } + + private static bool IsMigrationEnabled(Document document) + => document.Project.AnalyzerOptions.AnalyzerConfigOptionsProvider.GlobalOptions.TryGetValue( + $"build_property.{MSBuildPropertyOptionNames.EnableUnsafeMigration}", + out string? value) && + string.Equals(value?.Trim(), "true", StringComparison.OrdinalIgnoreCase); +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs index 51a5645b4318c6..b884d829f07361 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs @@ -9,6 +9,9 @@ public static class MSBuildPropertyOptionNames public const string IncludeAllContentForSelfExtract = nameof(IncludeAllContentForSelfExtract); public const string EnableTrimAnalyzer = nameof(EnableTrimAnalyzer); public const string EnableAotAnalyzer = nameof(EnableAotAnalyzer); +#if DEBUG + public const string EnableUnsafeMigration = nameof(EnableUnsafeMigration); +#endif public const string VerifyReferenceAotCompatibility = nameof(VerifyReferenceAotCompatibility); public const string VerifyReferenceTrimCompatibility = nameof(VerifyReferenceTrimCompatibility); } diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/PointerSignatureRequiresUnsafeAnalyzer.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/PointerSignatureRequiresUnsafeAnalyzer.cs new file mode 100644 index 00000000000000..5b14eeef98765c --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/PointerSignatureRequiresUnsafeAnalyzer.cs @@ -0,0 +1,77 @@ +// 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 ILLink.Shared; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace ILLink.RoslynAnalyzer; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class PointerSignatureRequiresUnsafeAnalyzer : DiagnosticAnalyzer +{ + private static readonly DiagnosticDescriptor s_rule = + DiagnosticDescriptors.GetDiagnosticDescriptor( + DiagnosticId.PointerSignatureRequiresUnsafe, + diagnosticSeverity: DiagnosticSeverity.Info); + + public override ImmutableArray SupportedDiagnostics => [s_rule]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(static context => + { + if (!context.Options.IsMSBuildPropertyValueTrue(MSBuildPropertyOptionNames.EnableUnsafeMigration)) + return; + + context.RegisterSyntaxNodeAction(AnalyzeDeclaration, UnsafeMemberAnalysis.DeclarationKinds); + }); + } + + private static void AnalyzeDeclaration(SyntaxNodeAnalysisContext context) + { + if (!UnsafeMemberAnalysis.IsCandidate(context.Node) || + context.Node is AccessorDeclarationSyntax || + UnsafeMemberAnalysis.HasSafetyDocumentation(context.Node) || + !UnsafeMemberAnalysis.HasPointerInSignature( + context.Node, + context.SemanticModel, + context.CancellationToken) || + UnsafeMemberAnalysis.HasUnsafeModifier(context.Node)) + { + return; + } + + if (context.Node is BasePropertyDeclarationSyntax + { + AccessorList.Accessors: var accessors + } && + accessors.Any(UnsafeMemberAnalysis.HasUnsafeModifier)) + { + foreach (AccessorDeclarationSyntax accessor in accessors.Where(static accessor => + UnsafeMemberAnalysis.IsPropertyAccessor(accessor) && + !UnsafeMemberAnalysis.HasUnsafeModifier(accessor))) + { + ReportDiagnostic(context, accessor); + } + + return; + } + + ReportDiagnostic(context, context.Node); + } + + private static void ReportDiagnostic( + SyntaxNodeAnalysisContext context, + SyntaxNode declaration) + => context.ReportDiagnostic(Diagnostic.Create( + s_rule, + UnsafeMemberAnalysis.GetDeclarationLocation(declaration))); +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMemberAnalysis.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMemberAnalysis.cs new file mode 100644 index 00000000000000..854270bdf940fa --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMemberAnalysis.cs @@ -0,0 +1,193 @@ +// 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; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo( + "ILLink.CodeFixProvider, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")] + +namespace ILLink.RoslynAnalyzer; + +internal static class UnsafeMemberAnalysis +{ + public static SyntaxKind[] DeclarationKinds { get; } = + [ + SyntaxKind.MethodDeclaration, + SyntaxKind.ConstructorDeclaration, + SyntaxKind.OperatorDeclaration, + SyntaxKind.ConversionOperatorDeclaration, + SyntaxKind.LocalFunctionStatement, + SyntaxKind.PropertyDeclaration, + SyntaxKind.IndexerDeclaration, + SyntaxKind.GetAccessorDeclaration, + SyntaxKind.SetAccessorDeclaration, + SyntaxKind.InitAccessorDeclaration, + SyntaxKind.EventDeclaration, + SyntaxKind.EventFieldDeclaration + ]; + + public static bool IsCandidate(SyntaxNode declaration) + { + SyntaxNode member = GetContainingMember(declaration); + if (GetModifiers(member).Any(static modifier => modifier.IsKind(SyntaxKind.ExternKeyword))) + return false; + + return declaration switch + { + ConstructorDeclarationSyntax constructor => + !constructor.Modifiers.Any(static modifier => modifier.IsKind(SyntaxKind.StaticKeyword)), + MethodDeclarationSyntax or OperatorDeclarationSyntax or ConversionOperatorDeclarationSyntax => true, + LocalFunctionStatementSyntax => true, + BasePropertyDeclarationSyntax => true, + AccessorDeclarationSyntax accessor => IsPropertyAccessor(accessor), + EventDeclarationSyntax or EventFieldDeclarationSyntax => true, + _ => false + }; + } + + public static bool IsPropertyAccessor(AccessorDeclarationSyntax accessor) + => accessor.IsKind(SyntaxKind.GetAccessorDeclaration) || + accessor.IsKind(SyntaxKind.SetAccessorDeclaration) || + accessor.IsKind(SyntaxKind.InitAccessorDeclaration); + + public static SyntaxTokenList GetModifiers(SyntaxNode declaration) + => declaration switch + { + BaseTypeDeclarationSyntax type => type.Modifiers, + DelegateDeclarationSyntax @delegate => @delegate.Modifiers, + BaseMethodDeclarationSyntax method => method.Modifiers, + LocalFunctionStatementSyntax localFunction => localFunction.Modifiers, + BasePropertyDeclarationSyntax property => property.Modifiers, + EventFieldDeclarationSyntax @event => @event.Modifiers, + FieldDeclarationSyntax field => field.Modifiers, + AccessorDeclarationSyntax accessor => accessor.Modifiers, + AnonymousFunctionExpressionSyntax anonymousFunction => anonymousFunction.Modifiers, + _ => default + }; + + public static bool HasUnsafeModifier(SyntaxNode declaration) + => GetUnsafeModifier(declaration).RawKind != 0; + + public static SyntaxToken GetUnsafeModifier(SyntaxNode declaration) + => GetModifiers(declaration).FirstOrDefault( + static modifier => modifier.IsKind(SyntaxKind.UnsafeKeyword)); + + public static bool HasSafetyDocumentation(SyntaxNode declaration) + => GetContainingMember(declaration).GetLeadingTrivia() + .Select(static trivia => trivia.GetStructure()) + .OfType() + .SelectMany(static documentation => documentation.DescendantNodes()) + .Any(static node => node switch + { + XmlElementSyntax element => element.StartTag.Name.LocalName.ValueText == "safety", + XmlEmptyElementSyntax element => element.Name.LocalName.ValueText == "safety", + _ => false + }); + + public static bool HasPointerInSignature( + SyntaxNode declaration, + SemanticModel semanticModel, + CancellationToken cancellationToken) + { + declaration = GetContainingMember(declaration); + ISymbol? symbol = declaration switch + { + EventFieldDeclarationSyntax { Declaration.Variables: [var variable, ..] } => + semanticModel.GetDeclaredSymbol(variable, cancellationToken), + _ => semanticModel.GetDeclaredSymbol(declaration, cancellationToken) + }; + + return symbol switch + { + IMethodSymbol method => ContainsPointer(method.ReturnType) || + method.Parameters.Any(static parameter => ContainsPointer(parameter.Type)), + IPropertySymbol property => ContainsPointer(property.Type) || + property.Parameters.Any(static parameter => ContainsPointer(parameter.Type)), + IEventSymbol @event => ContainsPointer(@event.Type), + _ => GetSignatureTypes(declaration).Any(static type => + type.DescendantNodesAndSelf().Any( + static node => node is PointerTypeSyntax or FunctionPointerTypeSyntax)) + }; + } + + public static Location GetDeclarationLocation(SyntaxNode declaration) + => declaration switch + { + MethodDeclarationSyntax method => method.Identifier.GetLocation(), + ConstructorDeclarationSyntax constructor => constructor.Identifier.GetLocation(), + OperatorDeclarationSyntax @operator => @operator.OperatorToken.GetLocation(), + ConversionOperatorDeclarationSyntax conversion => conversion.ImplicitOrExplicitKeyword.GetLocation(), + LocalFunctionStatementSyntax localFunction => localFunction.Identifier.GetLocation(), + PropertyDeclarationSyntax property => property.Identifier.GetLocation(), + IndexerDeclarationSyntax indexer => indexer.ThisKeyword.GetLocation(), + EventDeclarationSyntax @event => @event.Identifier.GetLocation(), + EventFieldDeclarationSyntax { Declaration.Variables: [var variable, ..] } => + variable.Identifier.GetLocation(), + AccessorDeclarationSyntax accessor => accessor.Keyword.GetLocation(), + _ => declaration.GetFirstToken().GetLocation() + }; + + public static SyntaxNode? FindDeclaration(SyntaxNode root, int position) + => root.FindToken(position) + .Parent? + .AncestorsAndSelf() + .FirstOrDefault(static node => node is + BaseTypeDeclarationSyntax or + DelegateDeclarationSyntax or + BaseMethodDeclarationSyntax or + LocalFunctionStatementSyntax or + BasePropertyDeclarationSyntax or + EventFieldDeclarationSyntax or + FieldDeclarationSyntax or + AccessorDeclarationSyntax or + AnonymousFunctionExpressionSyntax); + + private static SyntaxNode GetContainingMember(SyntaxNode declaration) + => declaration is AccessorDeclarationSyntax + { + Parent.Parent: BasePropertyDeclarationSyntax property + } + ? property + : declaration; + + private static IEnumerable GetSignatureTypes(SyntaxNode declaration) + => declaration switch + { + MethodDeclarationSyntax method => + [method.ReturnType, .. GetParameterTypes(method.ParameterList)], + ConstructorDeclarationSyntax constructor => GetParameterTypes(constructor.ParameterList), + OperatorDeclarationSyntax @operator => + [@operator.ReturnType, .. GetParameterTypes(@operator.ParameterList)], + ConversionOperatorDeclarationSyntax conversion => + [conversion.Type, .. GetParameterTypes(conversion.ParameterList)], + LocalFunctionStatementSyntax localFunction => + [localFunction.ReturnType, .. GetParameterTypes(localFunction.ParameterList)], + PropertyDeclarationSyntax property => [property.Type], + IndexerDeclarationSyntax indexer => + [indexer.Type, .. GetParameterTypes(indexer.ParameterList)], + EventDeclarationSyntax @event => [@event.Type], + EventFieldDeclarationSyntax @event => [@event.Declaration.Type], + _ => [] + }; + + private static IEnumerable GetParameterTypes(BaseParameterListSyntax parameterList) + => parameterList.Parameters + .Select(static parameter => parameter.Type) + .OfType(); + + private static bool ContainsPointer(ITypeSymbol type) + => type switch + { + IPointerTypeSymbol or IFunctionPointerTypeSymbol => true, + IArrayTypeSymbol array => ContainsPointer(array.ElementType), + INamedTypeSymbol namedType => namedType.TypeArguments.Any(ContainsPointer), + _ => false + }; +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMemberMissingSafetyDocumentationAnalyzer.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMemberMissingSafetyDocumentationAnalyzer.cs new file mode 100644 index 00000000000000..db14bfa7601eba --- /dev/null +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMemberMissingSafetyDocumentationAnalyzer.cs @@ -0,0 +1,53 @@ +// 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; + +namespace ILLink.RoslynAnalyzer; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class UnsafeMemberMissingSafetyDocumentationAnalyzer : DiagnosticAnalyzer +{ + private static readonly DiagnosticDescriptor s_rule = + DiagnosticDescriptors.GetDiagnosticDescriptor( + DiagnosticId.UnsafeMemberMissingSafetyDocumentation, + diagnosticSeverity: DiagnosticSeverity.Info); + + public override ImmutableArray SupportedDiagnostics => [s_rule]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(static context => + { + if (!context.Options.IsMSBuildPropertyValueTrue(MSBuildPropertyOptionNames.EnableUnsafeMigration)) + return; + + context.RegisterSyntaxNodeAction(AnalyzeDeclaration, UnsafeMemberAnalysis.DeclarationKinds); + }); + } + + private static void AnalyzeDeclaration(SyntaxNodeAnalysisContext context) + { + if (!UnsafeMemberAnalysis.IsCandidate(context.Node) || + !UnsafeMemberAnalysis.HasUnsafeModifier(context.Node) || + UnsafeMemberAnalysis.HasSafetyDocumentation(context.Node) || + UnsafeMemberAnalysis.HasPointerInSignature( + context.Node, + context.SemanticModel, + context.CancellationToken)) + { + return; + } + + context.ReportDiagnostic(Diagnostic.Create( + s_rule, + UnsafeMemberAnalysis.GetUnsafeModifier(context.Node).GetLocation())); + } +} +#endif diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props b/src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props index 18ae6cb8fd5e8b..86173e57392d5e 100644 --- a/src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props +++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props @@ -3,6 +3,7 @@ + diff --git a/src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs b/src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs index 39626951acaaef..4a758bba0f30e4 100644 --- a/src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs +++ b/src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs @@ -11,5 +11,8 @@ internal static class DiagnosticCategory public const string SingleFile = nameof(SingleFile); public const string Trimming = nameof(Trimming); public const string AOT = nameof(AOT); +#if DEBUG + public const string Unsafe = nameof(Unsafe); +#endif } } diff --git a/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs b/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs index 997f3f232ae018..9c5d55bfc6fa14 100644 --- a/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs +++ b/src/tools/illink/src/ILLink.Shared/DiagnosticId.cs @@ -219,6 +219,12 @@ public enum DiagnosticId // Feature guard diagnostic ids. ReturnValueDoesNotMatchFeatureGuards = 4000, InvalidFeatureGuard = 4001, + +#if DEBUG + // Unsafe evolution migration diagnostic ids. + UnsafeMemberMissingSafetyDocumentation = 5005, + PointerSignatureRequiresUnsafe = 5006, +#endif } public static class DiagnosticIdExtensions @@ -251,6 +257,10 @@ public static string GetDiagnosticCategory(this DiagnosticId diagnosticId) => { > 2000 and < 3000 => DiagnosticCategory.Trimming, >= 3000 and < 3050 => DiagnosticCategory.SingleFile, +#if DEBUG + (int)DiagnosticId.UnsafeMemberMissingSafetyDocumentation or + (int)DiagnosticId.PointerSignatureRequiresUnsafe => DiagnosticCategory.Unsafe, +#endif >= 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 77eb755610ecb3..248ce46f94aebc 100644 --- a/src/tools/illink/src/ILLink.Shared/SharedStrings.resx +++ b/src/tools/illink/src/ILLink.Shared/SharedStrings.resx @@ -1269,4 +1269,16 @@ Trim dataflow analysis of member '{0}' took too long to complete. Trim safety cannot be guaranteed. + + Unsafe member is missing safety documentation + + + Remove the 'unsafe' modifier or document the caller's safety requirements with a <safety> XML comment. + + + Pointer signature requires an unsafe modifier + + + Add the 'unsafe' modifier or document why the pointer signature is safe with a <safety> XML comment. + diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs index aab096802e3cf5..4e165c7078c686 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs @@ -8,9 +8,12 @@ using ILLink.Shared; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing; using Microsoft.CodeAnalysis.Text; using Xunit; +using RoslynCodeFixProvider = Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider; using VerifyCS = ILLink.RoslynAnalyzer.Tests.CSharpCodeFixVerifier< ILLink.RoslynAnalyzer.DynamicallyAccessedMembersAnalyzer, ILLink.CodeFix.RequiresUnsafeCodeFixProvider>; @@ -56,6 +59,130 @@ static Task VerifyRequiresUnsafeCodeFix( return test.RunAsync(); } + static Task VerifyUnsafeMigrationCodeFix( + string source, + string fixedSource, + DiagnosticResult[] baselineExpected, + DiagnosticResult[] fixedExpected, + CompilerDiagnostics compilerDiagnostics = CompilerDiagnostics.Errors) + where TAnalyzer : DiagnosticAnalyzer, new() + where TCodeFix : RoslynCodeFixProvider, new() + { + var test = new CSharpCodeFixVerifier.Test + { + TestCode = source, + FixedCode = fixedSource, + CompilerDiagnostics = compilerDiagnostics + }; + test.ExpectedDiagnostics.AddRange(baselineExpected); + test.SolutionTransforms.Add(static (solution, projectId) => + { + solution = SetOptions(solution, projectId); + var project = solution.GetProject(projectId)!; + var compilationOptions = (CSharpCompilationOptions)project.CompilationOptions!; + return solution.WithProjectCompilationOptions( + projectId, + compilationOptions + .WithWarningLevel(9999) + .WithSpecificDiagnosticOptions( + compilationOptions.SpecificDiagnosticOptions + .SetItem("CS9377", ReportDiagnostic.Warn) + .SetItem("CS1591", ReportDiagnostic.Suppress) + .SetItem("CS8321", ReportDiagnostic.Suppress))); + }); + var config = ("/.editorconfig", SourceText.From(@$" +is_global = true +build_property.{MSBuildPropertyOptionNames.EnableUnsafeMigration} = true")); + test.TestState.AnalyzerConfigFiles.Add(config); + test.FixedState.AnalyzerConfigFiles.Add(config); + test.FixedState.ExpectedDiagnostics.AddRange(fixedExpected); + return test.RunAsync(); + } + + static DiagnosticResult MissingSafetyDocumentationDiagnostic() + => CSharpCodeFixVerifier< + UnsafeMemberMissingSafetyDocumentationAnalyzer, + RemoveUndocumentedUnsafeCodeFixProvider>.Diagnostic( + DiagnosticDescriptors.GetDiagnosticDescriptor( + DiagnosticId.UnsafeMemberMissingSafetyDocumentation, + diagnosticSeverity: DiagnosticSeverity.Info)); + + static DiagnosticResult PointerSignatureRequiresUnsafeDiagnostic() + => CSharpCodeFixVerifier< + PointerSignatureRequiresUnsafeAnalyzer, + AddUnsafeToPointerSignatureCodeFixProvider>.Diagnostic( + DiagnosticDescriptors.GetDiagnosticDescriptor( + DiagnosticId.PointerSignatureRequiresUnsafe, + diagnosticSeverity: DiagnosticSeverity.Info)); + + [Fact] + public void UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks() + { + foreach (DiagnosticId diagnosticId in new[] + { + DiagnosticId.UnsafeMemberMissingSafetyDocumentation, + DiagnosticId.PointerSignatureRequiresUnsafe + }) + { + DiagnosticDescriptor descriptor = DiagnosticDescriptors.GetDiagnosticDescriptor(diagnosticId); + Assert.Equal("Unsafe", descriptor.Category); + Assert.Empty(descriptor.HelpLinkUri); + } + } + + [Fact] + public async Task UnsafeMigrationCodeFixes_AreNotRegisteredWhenMigrationDisabled() + { + using var workspace = new AdhocWorkspace(); + ProjectId projectId = ProjectId.CreateNewId(); + DocumentId documentId = DocumentId.CreateNewId(projectId); + Solution solution = workspace.CurrentSolution + .AddProject(ProjectInfo.Create( + projectId, + VersionStamp.Default, + "Test", + "Test", + LanguageNames.CSharp, + compilationOptions: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary), + parseOptions: new CSharpParseOptions(LanguageVersion.Preview))) + .AddDocument(documentId, "Test.cs", SourceText.From("class C { }")); + + Assert.True(workspace.TryApplyChanges(solution)); + Document document = workspace.CurrentSolution.GetDocument(documentId)!; + SyntaxTree syntaxTree = (await document.GetSyntaxTreeAsync())!; + + foreach ((RoslynCodeFixProvider provider, string diagnosticId) in new[] + { + ((RoslynCodeFixProvider)new RemoveInvalidUnsafeCodeFixProvider(), "CS9377"), + (new AddUnsafeToExternCodeFixProvider(), "CS9389"), + (new RemoveUndocumentedUnsafeCodeFixProvider(), + DiagnosticId.UnsafeMemberMissingSafetyDocumentation.AsString()), + (new AddUnsafeToPointerSignatureCodeFixProvider(), + DiagnosticId.PointerSignatureRequiresUnsafe.AsString()) + }) + { + var descriptor = new DiagnosticDescriptor( + diagnosticId, + diagnosticId, + diagnosticId, + "Test", + DiagnosticSeverity.Warning, + isEnabledByDefault: true); + Diagnostic diagnostic = Diagnostic.Create( + descriptor, + Location.Create(syntaxTree, new TextSpan(0, 1))); + bool registered = false; + var context = new CodeFixContext( + document, + diagnostic, + (_, _) => registered = true, + default); + + await provider.RegisterCodeFixesAsync(context); + Assert.False(registered); + } + } + [Fact] public async Task CodeFix_WrapInUnsafeBlock_SimpleStatement() { @@ -1670,6 +1797,216 @@ await VerifyRequiresUnsafeCodeFix( }, fixedExpected: Array.Empty()); } + + [Fact] + public async Task RemoveInvalidUnsafeCodeFix_RemovesCompilerReportedModifiers() + { + var source = """ + unsafe class {|#0:Program|} + { + } + + unsafe enum {|#1:E|} + { + A + } + """; + + var fixedSource = """ + class Program + { + } + + enum E + { + A + } + """; + + await VerifyUnsafeMigrationCodeFix< + UnsafeMemberMissingSafetyDocumentationAnalyzer, + RemoveInvalidUnsafeCodeFixProvider>( + source, + fixedSource, + baselineExpected: [ + DiagnosticResult.CompilerWarning("CS9377").WithLocation(0), + DiagnosticResult.CompilerError("CS0106").WithLocation(1) + ], + fixedExpected: [], + compilerDiagnostics: CompilerDiagnostics.Warnings); + } + + [Fact] + public async Task AddUnsafeToExternCodeFix_AddsUnsafeByDefault() + { + var source = """ + using System.Runtime.InteropServices; + + class Program + { + [DllImport("user32.dll")] + [return: MarshalAs(UnmanagedType.Bool)] + static {|#0:extern|} bool MessageBeep(int uType); + } + """; + + var fixedSource = """ + using System.Runtime.InteropServices; + + class Program + { + [DllImport("user32.dll")] + [return: MarshalAs(UnmanagedType.Bool)] + static unsafe extern bool MessageBeep(int uType); + } + """; + + await VerifyUnsafeMigrationCodeFix< + UnsafeMemberMissingSafetyDocumentationAnalyzer, + AddUnsafeToExternCodeFixProvider>( + source, + fixedSource, + baselineExpected: [ + DiagnosticResult.CompilerError("CS9389").WithLocation(0) + ], + fixedExpected: []); + } + + [Fact] + public async Task RemoveUndocumentedUnsafeCodeFix_RemovesOnlyUndocumentedNonPointerMembers() + { + var source = """ + using System; + + class C + { + public {|#0:unsafe|} void Method() { } + + public void Host() + { + {|#1:unsafe|} void Local() { } + } + + public {|#2:unsafe|} int Property + { + {|#3:unsafe|} get => 0; + set { } + } + + public {|#4:unsafe|} event Action Event + { + add { } + remove { } + } + + /// The caller must validate the state. + public unsafe void Documented() { } + + public unsafe void* Pointer(void* value) => value; + + public static unsafe extern int Extern(); + } + """; + + var fixedSource = """ + using System; + + class C + { + public void Method() { } + + public void Host() + { + void Local() { } + } + + public int Property + { + get => 0; + set { } + } + + public event Action Event + { + add { } + remove { } + } + + /// The caller must validate the state. + public unsafe void Documented() { } + + public unsafe void* Pointer(void* value) => value; + + public static unsafe extern int Extern(); + } + """; + + await VerifyUnsafeMigrationCodeFix< + UnsafeMemberMissingSafetyDocumentationAnalyzer, + RemoveUndocumentedUnsafeCodeFixProvider>( + source, + fixedSource, + baselineExpected: [ + MissingSafetyDocumentationDiagnostic().WithLocation(0), + MissingSafetyDocumentationDiagnostic().WithLocation(1), + MissingSafetyDocumentationDiagnostic().WithLocation(2), + MissingSafetyDocumentationDiagnostic().WithLocation(3), + MissingSafetyDocumentationDiagnostic().WithLocation(4) + ], + fixedExpected: []); + } + + [Fact] + public async Task AddUnsafeToPointerSignatureCodeFix_AddsMissingModifiers() + { + var source = """ + class C + { + public void* {|#0:Method|}(void* value) => value; + + /// The pointer is not dereferenced. + public void* Documented(void* value) => value; + + public delegate* unmanaged {|#1:Property|} => default; + + public delegate* unmanaged Mixed + { + unsafe get => default; + {|#2:set|} { } + } + } + """; + + var fixedSource = """ + class C + { + public unsafe void* Method(void* value) => value; + + /// The pointer is not dereferenced. + public void* Documented(void* value) => value; + + public unsafe delegate* unmanaged Property => default; + + public delegate* unmanaged Mixed + { + unsafe get => default; + unsafe set { } + } + } + """; + + await VerifyUnsafeMigrationCodeFix< + PointerSignatureRequiresUnsafeAnalyzer, + AddUnsafeToPointerSignatureCodeFixProvider>( + source, + fixedSource, + baselineExpected: [ + PointerSignatureRequiresUnsafeDiagnostic().WithLocation(0), + PointerSignatureRequiresUnsafeDiagnostic().WithLocation(1), + PointerSignatureRequiresUnsafeDiagnostic().WithLocation(2) + ], + fixedExpected: []); + } } } #endif