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.

Sign up for GitHub

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 on RUC annotated ctors invoked through new() constraint in analyzer #2254

Merged
merged 5 commits into from
Sep 8, 2021
Merged

Warn on RUC annotated ctors invoked through new() constraint in analyzer #2254

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
839a712
Add support in RUC analyzer for new() constraint on generics
Sep 3, 2021
d1e2468
Support `new()` constraint on types
Sep 3, 2021
a5af97c
Use SyntaxNodeAnalysisContext for constructor constraints
Sep 8, 2021
90cf840
Lint
Sep 8, 2021
03dfeb0
Update justification
Sep 8, 2021
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
13 changes: 13 additions & 0 deletions src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ internal static bool HasAttribute (this ISymbol symbol, string attributeName)
return false;
}

internal static bool TryGetAttribute (this ISymbol member, string attributeName, [NotNullWhen (returnValue: true)] out AttributeData? attribute)
{
attribute = null;
foreach (var attr in member.GetAttributes ()) {
if (attr.AttributeClass is { } attrClass && attrClass.HasName (attributeName)) {
attribute = attr;
return true;
}
}

return false;
}

internal static bool TryGetOverriddenMember (this ISymbol? symbol, [NotNullWhen (returnValue: true)] out ISymbol? overridenMember)
{
overridenMember = symbol switch {
Expand Down
4 changes: 2 additions & 2 deletions src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ public override void Initialize (AnalysisContext context)
var compilation = context.Compilation;
if (!IsAnalyzerEnabled (context.Options, compilation))
return;
var incompatibleMembers = GetSpecialIncompatibleMembers (compilation);

var incompatibleMembers = GetSpecialIncompatibleMembers (compilation);
context.RegisterSymbolAction (symbolAnalysisContext => {
var methodSymbol = (IMethodSymbol) symbolAnalysisContext.Symbol;
CheckMatchingAttributesInOverrides (symbolAnalysisContext, methodSymbol);
Expand Down Expand Up @@ -263,7 +263,7 @@ private void ReportMismatchInAttributesDiagnostic (SymbolAnalysisContext symbolA

protected abstract string GetMessageFromAttribute (AttributeData requiresAttribute);

private string GetUrlFromAttribute (AttributeData? requiresAttribute)
public static string GetUrlFromAttribute (AttributeData? requiresAttribute)
{
var url = requiresAttribute?.NamedArguments.FirstOrDefault (na => na.Key == "Url").Value.Value?.ToString ();
return MessageFormat.FormatRequiresAttributeUrlArg (url);
Expand Down
56 changes: 54 additions & 2 deletions src/ILLink.RoslynAnalyzer/RequiresUnreferencedCodeAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@

using System;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using ILLink.Shared;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

namespace ILLink.RoslynAnalyzer
{
Expand All @@ -31,6 +33,49 @@ public sealed class RequiresUnreferencedCodeAnalyzer : RequiresAnalyzerBase
operationContext.Operation.Syntax.GetLocation ()));
};

[SuppressMessage ("MicrosoftCodeAnalysisPerformance", "RS1008",
Justification = "This action is registered through a compilation start action, so that the instances which " +
"can register this operation action will not outlive a compilation's lifetime, avoiding the possibility of " +
"this data causing stale compilations to remain in memory.")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain why this action causes the warning and the one above doesn't? I don't see how they're really different...

static readonly Action<OperationAnalysisContext> s_constructorConstraint = operationContext => {
if (FindContainingSymbol (operationContext, DiagnosticTargets.All) is not ISymbol containingSymbol ||
containingSymbol.HasAttribute (RequiresUnreferencedCodeAttribute))
return;

var typeParams = ImmutableArray<ITypeParameterSymbol>.Empty;
var typeArgs = ImmutableArray<ITypeSymbol>.Empty;
if (operationContext.Operation is IObjectCreationOperation objectCreationOperation &&
objectCreationOperation.Type is INamedTypeSymbol objectType && objectType.IsGenericType) {
typeParams = objectType.TypeParameters;
typeArgs = objectType.TypeArguments;
Copy link
Member

Choose a reason for hiding this comment

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

This only reacts to calling a constructor on a generic type.
Code like this should still warn though:

static class MyTest<T> where T : new ()
{
    static void DoNothing() {}
}

void Test()
{
    // IL2026
    MyTest.DoNothing<ClassWithRUCOnCtor>();
}

I must admit that I don't know enough about analyzers to tell what type of solution would be the best here. Linker will warn anywhere the generic is instantiated. Even cases like typeof(MyType<ClassWithRUCOnCtor>) will generate the warning.

This is obviously not that common, so it's OK to solve this later...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the analysis context used here to a SyntaxNodeAnalysisContext; this way we look for syntax nodes that represent generic names and check, for their type parameters, if these have any constructor constraints. I think this would cover up all cases. I've added the scenario you outline above (and the analyzer warns now).

} else if (operationContext.Operation is IInvocationOperation invocationOperation &&
invocationOperation.TargetMethod is IMethodSymbol targetMethod && targetMethod.IsGenericMethod) {
typeParams = targetMethod.TypeParameters;
typeArgs = targetMethod.TypeArguments;
}
mateoatr marked this conversation as resolved.
Show resolved Hide resolved

for (int i = 0; i < typeParams.Length; i++) {
var typeParameter = typeParams[i];
var typeArgument = typeArgs[i];
if (!typeParameter.HasConstructorConstraint)
continue;

var typeArgCtors = ((INamedTypeSymbol) typeArgument).InstanceConstructors;
foreach (var instanceCtor in typeArgCtors) {
if (instanceCtor.Arity > 0)
continue;

if (instanceCtor.TryGetAttribute (RequiresUnreferencedCodeAttribute, out var requiresUnreferencedCodeAttribute)) {
operationContext.ReportDiagnostic (Diagnostic.Create (s_requiresUnreferencedCodeRule,
operationContext.Operation.Syntax.GetLocation (),
containingSymbol.GetDisplayName (),
(string) requiresUnreferencedCodeAttribute.ConstructorArguments[0].Value!,
GetUrlFromAttribute (requiresUnreferencedCodeAttribute)));
}
}
}
};

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create (s_dynamicTypeInvocationRule, s_requiresUnreferencedCodeRule, s_requiresUnreferencedCodeAttributeMismatch);

Expand All @@ -47,8 +92,15 @@ public sealed class RequiresUnreferencedCodeAnalyzer : RequiresAnalyzerBase
protected override bool IsAnalyzerEnabled (AnalyzerOptions options, Compilation compilation) =>
options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer, compilation);

private protected override ImmutableArray<(Action<OperationAnalysisContext> Action, OperationKind[] OperationKind)> ExtraOperationActions =>
ImmutableArray.Create ((s_dynamicTypeInvocation, new OperationKind[] { OperationKind.DynamicInvocation }));
private protected override ImmutableArray<(Action<OperationAnalysisContext> Action, OperationKind[] OperationKind)> ExtraOperationActions {
get {
var diagsBuilder = ImmutableArray.CreateBuilder<(Action<OperationAnalysisContext>, OperationKind[])> ();
diagsBuilder.Add ((s_dynamicTypeInvocation, new OperationKind[] { OperationKind.DynamicInvocation }));
diagsBuilder.Add ((s_constructorConstraint, new OperationKind[] { OperationKind.Invocation, OperationKind.ObjectCreation }));

return diagsBuilder.ToImmutable ();
}
}

protected override bool VerifyAttributeArguments (AttributeData attribute) =>
attribute.ConstructorArguments.Length >= 1 && attribute.ConstructorArguments[0] is { Type: { SpecialType: SpecialType.System_String } } ctorArg;
Expand Down
22 changes: 16 additions & 6 deletions test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public static void Main ()
AccessThroughPInvoke.Test ();
OnEventMethod.Test ();
AccessThroughNewConstraint.Test ();
AccessThroughNewConstraint.TestNewConstraintOnTypeParameter ();
AccessThroughLdToken.Test ();
RequiresOnClass.Test ();
}
Expand Down Expand Up @@ -770,19 +771,28 @@ public static void Test ()

class AccessThroughNewConstraint
{
class NewConstrainTestType
class NewConstraintTestType
{
[RequiresUnreferencedCode ("Message for --NewConstrainTestType.ctor--")]
public NewConstrainTestType () { }
[RequiresUnreferencedCode ("Message for --NewConstraintTestType.ctor--")]
public NewConstraintTestType () { }
}

static void GenericMethod<T> () where T : new() { }

// https://github.com/mono/linker/issues/2117
[ExpectedWarning ("IL2026", "--NewConstrainTestType.ctor--", GlobalAnalysisOnly = true)]
[ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--")]
public static void Test ()
{
GenericMethod<NewConstrainTestType> ();
GenericMethod<NewConstraintTestType> ();
}

class NewConstaintOnTypeParameter<T> where T : new()
{
}

[ExpectedWarning ("IL2026", "--NewConstraintTestType.ctor--")]
public static void TestNewConstraintOnTypeParameter ()
{
_ = new NewConstaintOnTypeParameter<NewConstraintTestType> ();
}
}

Expand Down