Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn if RequiresXXXCode is placed on an entrypoint #110185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ internal static MessageContainer CreateErrorMessage(MessageOrigin? origin, Diagn
if (context.IsWarningSubcategorySuppressed(subcategory))
return null;

// If the warning comes from compiler-generated code, it would not be actionable. The assumption is that
// compiler-generated code doesn't have issues.
if (origin.MemberDefinition is MethodDesc originMethod && originMethod.GetTypicalMethodDefinition() is not EcmaMethod)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ILC generates a method that calls user's main (after initializing the environment). It was triggering a warning if Main has RUC

return null;

if (TryLogSingleWarning(context, code, origin, subcategory))
return null;

Expand All @@ -145,6 +150,11 @@ internal static MessageContainer CreateErrorMessage(MessageOrigin? origin, Diagn
if (context.IsWarningSubcategorySuppressed(subcategory))
return null;

// If the warning comes from compiler-generated code, it would not be actionable. The assumption is that
// compiler-generated code doesn't have issues.
if (origin.MemberDefinition is MethodDesc originMethod && originMethod.GetTypicalMethodDefinition() is not EcmaMethod)
return null;

if (TryLogSingleWarning(context, (int)id, origin, subcategory))
return null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,19 @@ protected override void GetDependenciesDueToMethodCodePresenceInternal(ref Depen
if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresAssemblyFilesAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresAssemblyFilesOnStaticConstructor, method.GetDisplayName());
}

if (method is EcmaMethod maybeEntrypoint
&& (maybeEntrypoint.Module.EntryPoint == method || (method.IsUnmanagedCallersOnly && maybeEntrypoint.GetUnmanagedCallersOnlyExportName() != null)))
{
if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresUnreferencedCodeOnEntryPoint, method.GetDisplayName());

if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresDynamicCodeAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresDynamicCodeOnEntryPoint, method.GetDisplayName());

if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresAssemblyFilesAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresAssemblyFilesOnEntryPoint, method.GetDisplayName());
}
}

if (method.GetTypicalMethodDefinition() is Internal.TypeSystem.Ecma.EcmaMethod ecmaMethod)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Immutable;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;

Expand Down Expand Up @@ -160,5 +161,10 @@ public static SimpleNameSyntax GetUnqualifiedName (this NameSyntax name)
SimpleNameSyntax simple => simple,
_ => throw new InvalidOperationException ("Unreachable"),
};

public static INamedTypeSymbol? TaskType (this Compilation compilation)
=> compilation.GetTypeByMetadataName (typeof (Task).FullName!);
public static INamedTypeSymbol? TaskOfTType (this Compilation compilation)
=> compilation.GetTypeByMetadataName (typeof (Task<>).FullName!);
}
}
22 changes: 22 additions & 0 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -203,5 +203,27 @@ public static bool IsConstructor ([NotNullWhen (returnValue: true)] this ISymbol

public static bool IsStaticConstructor ([NotNullWhen (returnValue: true)] this ISymbol? symbol)
=> (symbol as IMethodSymbol)?.MethodKind == MethodKind.StaticConstructor;

public static bool IsEntryPoint (this IMethodSymbol methodSymbol, Compilation compilation)
=> methodSymbol.Name is WellKnownMemberNames.EntryPointMethodName or WellKnownMemberNames.TopLevelStatementsEntryPointMethodName &&
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how Roslyn does it in same named method. It might be overly broad in edge cases. Not sure how much we care, it's pretty niche.

methodSymbol.IsStatic &&
(methodSymbol.ReturnsVoid ||
methodSymbol.ReturnType.SpecialType == SpecialType.System_Int32 ||
SymbolEqualityComparer.Default.Equals(methodSymbol.ReturnType.OriginalDefinition, compilation.TaskType()) ||
SymbolEqualityComparer.Default.Equals(methodSymbol.ReturnType.OriginalDefinition, compilation.TaskOfTType()));

public static bool IsUnmanagedCallersOnlyEntryPoint (this IMethodSymbol methodSymbol)
{
foreach (var attr in methodSymbol.GetAttributes ()) {
if (attr.AttributeClass is { } attrClass && attrClass.HasName ("System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute")) {
foreach (var namedArgument in attr.NamedArguments) {
if (namedArgument.Key == "EntryPoint")
return true;
}
}
}

return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public abstract class RequiresAnalyzerBase : DiagnosticAnalyzer

private protected abstract DiagnosticDescriptor RequiresAttributeMismatch { get; }
private protected abstract DiagnosticDescriptor RequiresOnStaticCtor { get; }
private protected abstract DiagnosticDescriptor RequiresOnEntryPoint { get; }

private protected virtual ImmutableArray<(Action<SyntaxNodeAnalysisContext> Action, SyntaxKind[] SyntaxKind)> ExtraSyntaxNodeActions { get; } = ImmutableArray<(Action<SyntaxNodeAnalysisContext> Action, SyntaxKind[] SyntaxKind)>.Empty;
private protected virtual ImmutableArray<(Action<SymbolAnalysisContext> Action, SymbolKind[] SymbolKind)> ExtraSymbolActions { get; } = ImmutableArray<(Action<SymbolAnalysisContext> Action, SymbolKind[] SymbolKind)>.Empty;
Expand All @@ -51,8 +52,15 @@ public override void Initialize (AnalysisContext context)
var incompatibleMembers = GetSpecialIncompatibleMembers (compilation);
context.RegisterSymbolAction (symbolAnalysisContext => {
var methodSymbol = (IMethodSymbol) symbolAnalysisContext.Symbol;
if (methodSymbol.IsStaticConstructor () && methodSymbol.HasAttribute (RequiresAttributeName))
ReportRequiresOnStaticCtorDiagnostic (symbolAnalysisContext, methodSymbol);

if (methodSymbol.HasAttribute (RequiresAttributeName)) {
if (methodSymbol.IsStaticConstructor ())
ReportRequiresOnStaticCtorDiagnostic (symbolAnalysisContext, methodSymbol);

if (methodSymbol.IsEntryPoint (symbolAnalysisContext.Compilation) || methodSymbol.IsUnmanagedCallersOnlyEntryPoint ())
ReportRequiresOnEntryPointDiagnostic (symbolAnalysisContext, methodSymbol);
}

CheckMatchingAttributesInOverrides (symbolAnalysisContext, methodSymbol);
}, SymbolKind.Method);

Expand Down Expand Up @@ -238,6 +246,14 @@ private void ReportRequiresOnStaticCtorDiagnostic (SymbolAnalysisContext symbolA
ctor.GetDisplayName ()));
}

private void ReportRequiresOnEntryPointDiagnostic (SymbolAnalysisContext symbolAnalysisContext, IMethodSymbol entryPoint)
{
symbolAnalysisContext.ReportDiagnostic (Diagnostic.Create (
RequiresOnEntryPoint,
entryPoint.Locations[0],
entryPoint.GetDisplayName ()));
}

private void ReportMismatchInAttributesDiagnostic (SymbolAnalysisContext symbolAnalysisContext, ISymbol member, ISymbol baseMember, bool isInterface = false, ISymbol? origin = null)
{
origin ??= member;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ public sealed class RequiresAssemblyFilesAnalyzer : RequiresAnalyzerBase

static readonly DiagnosticDescriptor s_requiresAssemblyFilesOnStaticCtor = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresAssemblyFilesOnStaticConstructor);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create (s_locationRule, s_getFilesRule, s_requiresAssemblyFilesRule, s_requiresAssemblyFilesAttributeMismatch, s_requiresAssemblyFilesOnStaticCtor);
static readonly DiagnosticDescriptor s_requiresAssemblyFilesOnEntryPoint = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresAssemblyFilesOnEntryPoint);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create (s_locationRule, s_getFilesRule, s_requiresAssemblyFilesRule, s_requiresAssemblyFilesAttributeMismatch, s_requiresAssemblyFilesOnStaticCtor, s_requiresAssemblyFilesOnEntryPoint);

private protected override string RequiresAttributeName => RequiresAssemblyFilesAttribute;

Expand All @@ -48,6 +50,8 @@ public sealed class RequiresAssemblyFilesAnalyzer : RequiresAnalyzerBase

private protected override DiagnosticDescriptor RequiresOnStaticCtor => s_requiresAssemblyFilesOnStaticCtor;

private protected override DiagnosticDescriptor RequiresOnEntryPoint => s_requiresAssemblyFilesOnEntryPoint;

internal override bool IsAnalyzerEnabled (AnalyzerOptions options)
{
var isSingleFileAnalyzerEnabled = options.GetMSBuildPropertyValue (MSBuildPropertyOptionNames.EnableSingleFileAnalyzer);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase
public const string FullyQualifiedRequiresDynamicCodeAttribute = "System.Diagnostics.CodeAnalysis." + RequiresDynamicCodeAttribute;

static readonly DiagnosticDescriptor s_requiresDynamicCodeOnStaticCtor = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCodeOnStaticConstructor);
static readonly DiagnosticDescriptor s_requiresDynamicCodeOnEntryPoint = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCodeOnEntryPoint);
static readonly DiagnosticDescriptor s_requiresDynamicCodeRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCode);
static readonly DiagnosticDescriptor s_requiresDynamicCodeAttributeMismatch = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCodeAttributeMismatch);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create (s_requiresDynamicCodeRule, s_requiresDynamicCodeAttributeMismatch, s_requiresDynamicCodeOnStaticCtor);
ImmutableArray.Create (s_requiresDynamicCodeRule, s_requiresDynamicCodeAttributeMismatch, s_requiresDynamicCodeOnStaticCtor, s_requiresDynamicCodeOnEntryPoint);

private protected override string RequiresAttributeName => RequiresDynamicCodeAttribute;

Expand All @@ -40,6 +41,8 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase

private protected override DiagnosticDescriptor RequiresOnStaticCtor => s_requiresDynamicCodeOnStaticCtor;

private protected override DiagnosticDescriptor RequiresOnEntryPoint => s_requiresDynamicCodeOnEntryPoint;

internal override bool IsAnalyzerEnabled (AnalyzerOptions options) =>
options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableAotAnalyzer);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public sealed class RequiresUnreferencedCodeAnalyzer : RequiresAnalyzerBase
static readonly DiagnosticDescriptor s_makeGenericTypeRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.MakeGenericType);
static readonly DiagnosticDescriptor s_makeGenericMethodRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.MakeGenericMethod);
static readonly DiagnosticDescriptor s_requiresUnreferencedCodeOnStaticCtor = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresUnreferencedCodeOnStaticConstructor);
static readonly DiagnosticDescriptor s_requiresUnreferencedCodeOnEntryPoint = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresUnreferencedCodeOnEntryPoint);

static readonly DiagnosticDescriptor s_typeDerivesFromRucClassRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresUnreferencedCodeOnBaseClass);

Expand All @@ -45,7 +46,7 @@ private Action<SymbolAnalysisContext> typeDerivesFromRucBase {
}

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create (s_makeGenericMethodRule, s_makeGenericTypeRule, s_requiresUnreferencedCodeRule, s_requiresUnreferencedCodeAttributeMismatch, s_typeDerivesFromRucClassRule, s_requiresUnreferencedCodeOnStaticCtor);
ImmutableArray.Create (s_makeGenericMethodRule, s_makeGenericTypeRule, s_requiresUnreferencedCodeRule, s_requiresUnreferencedCodeAttributeMismatch, s_typeDerivesFromRucClassRule, s_requiresUnreferencedCodeOnStaticCtor, s_requiresUnreferencedCodeOnEntryPoint);

private protected override string RequiresAttributeName => RequiresUnreferencedCodeAttribute;

Expand All @@ -61,6 +62,8 @@ private Action<SymbolAnalysisContext> typeDerivesFromRucBase {

private protected override DiagnosticDescriptor RequiresOnStaticCtor => s_requiresUnreferencedCodeOnStaticCtor;

private protected override DiagnosticDescriptor RequiresOnEntryPoint => s_requiresUnreferencedCodeOnEntryPoint;

internal override bool IsAnalyzerEnabled (AnalyzerOptions options) =>
options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer);

Expand Down
3 changes: 3 additions & 0 deletions src/tools/illink/src/ILLink.Shared/DiagnosticId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ public enum DiagnosticId
_unused_DynamicallyAccessedMembersOnTypeReferencesCompilerGeneratedMemberOnBase = 2120,
RedundantSuppression = 2121,
TypeNameIsNotAssemblyQualified = 2122,
RequiresUnreferencedCodeOnEntryPoint = 2123,
_EndTrimAnalysisWarningsSentinel,

// Single-file diagnostic ids.
Expand All @@ -195,6 +196,7 @@ public enum DiagnosticId
RequiresAssemblyFiles = 3002,
RequiresAssemblyFilesAttributeMismatch = 3003,
RequiresAssemblyFilesOnStaticConstructor = 3004,
RequiresAssemblyFilesOnEntryPoint = 3005,

// Dynamic code diagnostic ids.
RequiresDynamicCode = 3050,
Expand All @@ -204,6 +206,7 @@ public enum DiagnosticId
GenericRecursionCycle = 3054,
CorrectnessOfAbstractDelegatesCannotBeGuaranteed = 3055,
RequiresDynamicCodeOnStaticConstructor = 3056,
RequiresDynamicCodeOnEntryPoint = 3057,
_EndAotAnalysisWarningsSentinel,

// Feature guard diagnostic ids.
Expand Down
18 changes: 18 additions & 0 deletions src/tools/illink/src/ILLink.Shared/SharedStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -1179,6 +1179,24 @@
<data name="RequiresAssemblyFilesOnStaticConstructorTitle" xml:space="preserve">
<value>The use of 'RequiresAssemblyFilesAttribute' on static constructors is disallowed since is a method not callable by the user, is only called by the runtime. Placing the attribute directly on the static constructor will have no effect, instead use 'RequiresUnreferencedCodeAttribute' on the type which will handle warning and silencing from the static constructor.</value>
</data>
<data name="RequiresUnreferencedCodeOnEntryPointMessage" xml:space="preserve">
<value>'RequiresUnreferencedCodeAttribute' cannot be placed directly on application entry point '{0}'.</value>
</data>
<data name="RequiresUnreferencedCodeOnEntryPointTitle" xml:space="preserve">
<value>The use of 'RequiresUnreferencedCodeAttribute' on entry points is disallowed since the method will be called from outside the visible app.</value>
</data>
<data name="RequiresDynamicCodeOnEntryPointMessage" xml:space="preserve">
<value>'RequiresDynamicCodeAttribute' cannot be placed directly on application entry point '{0}'.</value>
</data>
<data name="RequiresDynamicCodeOnEntryPointTitle" xml:space="preserve">
<value>The use of 'RequiresDynamicCodeAttribute' on entry points is disallowed since the method will be called from outside the visible app.</value>
</data>
<data name="RequiresAssemblyFilesOnEntryPointMessage" xml:space="preserve">
<value>'RequiresAssemblyFilesAttribute' cannot be placed directly on application entry point '{0}'.</value>
</data>
<data name="RequiresAssemblyFilesOnEntryPointTitle" xml:space="preserve">
<value>The use of 'RequiresAssemblyFilesAttribute' on entry points is disallowed since the method will be called from outside the visible app.</value>
</data>
<data name="UnrecognizedInternalAttributeMessage" xml:space="preserve">
<value>The internal attribute name '{0}' being used in the xml is not supported by ILLink, check the spelling and the supported internal attributes.</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3088,6 +3088,8 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
Tracer.AddDirectDependency (method.DeclaringType, new DependencyInfo (DependencyKind.InstantiatedByCtor, method), marked: false);
} else if (method.IsStaticConstructor () && Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (method))
Context.LogWarning (methodOrigin, DiagnosticId.RequiresUnreferencedCodeOnStaticConstructor, method.GetDisplayName ());
else if (method == method.Module.EntryPoint && Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute>(method))
Context.LogWarning (methodOrigin, DiagnosticId.RequiresUnreferencedCodeOnEntryPoint, method.GetDisplayName ());

if (method.IsConstructor) {
if (!Annotations.ProcessSatelliteAssemblies && KnownMembers.IsSatelliteAssemblyMarker (method))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ public Task RequiresOnStaticConstructor ()
return RunTest (nameof (RequiresOnStaticConstructor));
}

[Fact]
public Task RequiresOnEntryPoint ()
{
return RunTest ();
}

[Fact]
public Task RequiresOnVirtualsAndInterfaces ()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ public class MemberTypes
// This is an easy way to suppress all trim related warnings in the Main method
// This test is about marking, not diagnostics and this Main will produce several warnings due to it accssing
// some problematic APIs (Delegate.Create for example) via reflection.
[UnconditionalSuppressMessage("Reflection", "IL2123", Justification = "The RUC suppresses warnings in an entrypoint, but we don't mind")]
[RequiresUnreferencedCode("test")]
[KeptAttributeAttribute(typeof(UnconditionalSuppressMessageAttribute))]
[KeptAttributeAttribute(typeof(RequiresUnreferencedCodeAttribute))]
public static void Main ()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ class RequiresOnAttribute
{
// Using these as a simple way to suppress all warning in Main
// it causes lot of warning due to DAM marking everything in the class.
[UnconditionalSuppressMessage ("Reflection", "IL2123", Justification = "The RUC suppresses warnings in an entrypoint, but we don't mind")]
[UnconditionalSuppressMessage ("Reflection", "IL3057", Justification = "The RDC suppresses warnings in an entrypoint, but we don't mind")]
[UnconditionalSuppressMessage ("Reflection", "IL3005", Justification = "The RAF suppresses warnings in an entrypoint, but we don't mind")]
[RequiresUnreferencedCode ("main")]
[RequiresDynamicCode ("main")]
[RequiresAssemblyFiles ("main")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace Mono.Linker.Tests.Cases.RequiresCapability
public class RequiresOnAttributeCtor
{
// Simple way to suppress all warnings in Main
[UnconditionalSuppressMessage ("Reflection", "IL2123", Justification = "The RUC suppresses warnings in an entrypoint, but we don't mind")]
[RequiresUnreferencedCode ("main")]
public static void Main ()
{
Expand Down
Loading