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

Jump to bottom

Ruc on type analyzer part 1 #2330

Merged
merged 10 commits into from
Oct 29, 2021
Merged

Ruc on type analyzer part 1 #2330

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fdef59a
First Ruc on type tests
tlakollo Oct 20, 2021
346a095
Change field and event signature to match linker and analyzer
tlakollo Oct 26, 2021
bcd7061
Enable concurrent ececution
tlakollo Oct 26, 2021
93430fa
Lint
tlakollo Oct 26, 2021
a102cb8
Bring IsContructor Method from roslyn source
tlakollo Oct 26, 2021
a699c28
PR feedback
tlakollo Oct 28, 2021
f66d64f
Delete test coverage files and additional dummy code
tlakollo Oct 28, 2021
c54c87e
Merge branch 'release/6.0.2xx' of https://github.com/mono/linker into…
tlakollo Oct 28, 2021
7b85f73
PR Feedback
tlakollo Oct 28, 2021
58b16d2
Remove special case for printing warnings in event instead of the cal…
tlakollo Oct 28, 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
14 changes: 6 additions & 8 deletions src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,6 @@ public static string GetDisplayName (this ISymbol symbol)
{
var sb = new StringBuilder ();
switch (symbol) {
case IFieldSymbol fieldSymbol:
sb.Append (fieldSymbol.Type);
sb.Append (" ");
sb.Append (fieldSymbol.ContainingSymbol.ToDisplayString ());
sb.Append ("::");
sb.Append (fieldSymbol.MetadataName);
break;

case IParameterSymbol parameterSymbol:
sb.Append (parameterSymbol.Name);
break;
Expand Down Expand Up @@ -93,5 +85,11 @@ public static bool IsSubclassOf (this ISymbol symbol, string ns, string type)

return false;
}

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

public static bool IsStaticConstructor ([NotNullWhen (returnValue: true)] this ISymbol? symbol)
=> (symbol as IMethodSymbol)?.MethodKind == MethodKind.StaticConstructor;
}
}
26 changes: 24 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 @@ -94,6 +94,16 @@ public override void Initialize (AnalysisContext context)
}
}, OperationKind.ObjectCreation);

context.RegisterOperationAction (operationContext => {
var fieldReference = (IFieldReferenceOperation) operationContext.Operation;
if (fieldReference.Field.IsStatic &&
tlakollo marked this conversation as resolved.
Show resolved Hide resolved
fieldReference.Syntax.IsKind (SyntaxKind.SimpleMemberAccessExpression) &&
fieldReference.Field.ContainingType is INamedTypeSymbol fieldDeclaringType &&
fieldDeclaringType.TryGetAttribute (RequiresAttributeName, out var requiresAttribute)) {
ReportRequiresDiagnostic (operationContext, fieldReference.Field, requiresAttribute);
}
}, OperationKind.FieldReference);

context.RegisterOperationAction (operationContext => {
var propAccess = (IPropertyReferenceOperation) operationContext.Operation;
var prop = propAccess.Property;
Expand All @@ -118,6 +128,9 @@ public override void Initialize (AnalysisContext context)

if (eventSymbol.RemoveMethod is IMethodSymbol eventRemoveMethod)
CheckCalledMember (operationContext, eventRemoveMethod, incompatibleMembers);

if (eventSymbol.RaiseMethod is IMethodSymbol eventRaiseMethod)
tlakollo marked this conversation as resolved.
Show resolved Hide resolved
CheckCalledMember (operationContext, eventRaiseMethod, incompatibleMembers);
}, OperationKind.EventReference);

context.RegisterOperationAction (operationContext => {
Expand Down Expand Up @@ -161,9 +174,17 @@ void CheckCalledMember (
ImmutableArray<ISymbol> incompatibleMembers)
{
ISymbol containingSymbol = FindContainingSymbol (operationContext, AnalyzerDiagnosticTargets);
INamedTypeSymbol memberDeclaringType = member.ContainingType;
INamedTypeSymbol containingSymbolDeclaringType = containingSymbol.ContainingType;

if ((!member.IsConstructor () && member.IsStatic || member.IsConstructor ()) &&
memberDeclaringType.TryGetAttribute (RequiresAttributeName, out var typeRequires) &&
!containingSymbolDeclaringType.HasAttribute (RequiresAttributeName))
ReportRequiresDiagnostic (operationContext, member, typeRequires);

// Do not emit any diagnostic if caller is annotated with the attribute too.
if (containingSymbol.HasAttribute (RequiresAttributeName))
if (containingSymbol.HasAttribute (RequiresAttributeName) ||
containingSymbolDeclaringType.HasAttribute (RequiresAttributeName))
return;

// Check also for RequiresAttribute in the associated symbol
Expand Down Expand Up @@ -228,7 +249,8 @@ protected enum DiagnosticTargets
Property = 0x0002,
Field = 0x0004,
Event = 0x0008,
All = MethodOrConstructor | Property | Field | Event
Class = 0x0010,
All = MethodOrConstructor | Property | Field | Event | Class
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/ILLink.RoslynAnalyzer/RequiresUnreferencedCodeAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public sealed class RequiresUnreferencedCodeAnalyzer : RequiresAnalyzerBase

private protected override string RequiresAttributeFullyQualifiedName => FullyQualifiedRequiresUnreferencedCodeAttribute;

private protected override DiagnosticTargets AnalyzerDiagnosticTargets => DiagnosticTargets.MethodOrConstructor;
private protected override DiagnosticTargets AnalyzerDiagnosticTargets => DiagnosticTargets.MethodOrConstructor | DiagnosticTargets.Class;

private protected override DiagnosticDescriptor RequiresDiagnosticRule => s_requiresUnreferencedCodeRule;

Expand Down
14 changes: 14 additions & 0 deletions src/linker/Linker/MethodReferenceExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,20 @@ public static string GetDisplayName (this MethodReference method)
return sb.ToString ();
}

if (methodDefinition != null && methodDefinition.IsEventMethod ()) {
// Append event name
string name = methodDefinition.SemanticsAttributes switch {
MethodSemanticsAttributes.AddOn => string.Concat (methodDefinition.Name.AsSpan (4), ".add"),
MethodSemanticsAttributes.RemoveOn => string.Concat (methodDefinition.Name.AsSpan (7), ".remove"),
MethodSemanticsAttributes.Fire => string.Concat (methodDefinition.Name.AsSpan (6), ".raise"),
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'm not 100% sure if the AsSpan should be 6 (for raise) or 5 (for fire) but seems like documentation mentions more raise than fire

_ => throw new NotSupportedException (),
};
sb.Append (name);
// Insert declaring type name and namespace
sb.Insert (0, '.').Insert (0, method.DeclaringType.GetDisplayName ());
return sb.ToString ();
}

// Append parameters
sb.Append ("(");
if (method.HasParameters) {
Expand Down
63 changes: 35 additions & 28 deletions test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresCapability.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,7 @@ static StaticCtor ()
}
}

[ExpectedWarning ("IL2026", "RequiresOnClass.StaticCtor.StaticCtor()", "Message for --StaticCtor--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.StaticCtor.StaticCtor()", "Message for --StaticCtor--")]
static void TestStaticCctorRequires ()
{
_ = new StaticCtor ();
Expand All @@ -935,13 +935,13 @@ static StaticCtorTriggeredByFieldAccess ()
public static int field;
}

[ExpectedWarning ("IL2026", "StaticCtorTriggeredByFieldAccess.field", "Message for --StaticCtorTriggeredByFieldAccess--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticCtorTriggeredByFieldAccess.field", "Message for --StaticCtorTriggeredByFieldAccess--")]
static void TestStaticCtorMarkingIsTriggeredByFieldAccessWrite ()
{
StaticCtorTriggeredByFieldAccess.field = 1;
}

[ExpectedWarning ("IL2026", "StaticCtorTriggeredByFieldAccess.field", "Message for --StaticCtorTriggeredByFieldAccess--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticCtorTriggeredByFieldAccess.field", "Message for --StaticCtorTriggeredByFieldAccess--")]
static void TestStaticCtorMarkingTriggeredOnSecondAccessWrite ()
{
StaticCtorTriggeredByFieldAccess.field = 2;
Expand All @@ -965,7 +965,7 @@ class StaticCCtorTriggeredByFieldAccessRead
public static int field = 42;
}

[ExpectedWarning ("IL2026", "StaticCCtorTriggeredByFieldAccessRead.field", "Message for --StaticCCtorTriggeredByFieldAccessRead--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticCCtorTriggeredByFieldAccessRead.field", "Message for --StaticCCtorTriggeredByFieldAccessRead--")]
static void TestStaticCtorMarkingIsTriggeredByFieldAccessRead ()
{
var _ = StaticCCtorTriggeredByFieldAccessRead.field;
Expand All @@ -983,7 +983,7 @@ public void TriggerStaticCtorMarking ()
}
}

[ExpectedWarning ("IL2026", "StaticCtorTriggeredByCtorCalls.StaticCtorTriggeredByCtorCalls()", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticCtorTriggeredByCtorCalls.StaticCtorTriggeredByCtorCalls()")]
static void TestStaticCtorTriggeredByCtorCall ()
{
new StaticCtorTriggeredByCtorCalls ();
Expand All @@ -995,7 +995,7 @@ class ClassWithInstanceField
public int field = 42;
}

[ExpectedWarning ("IL2026", "ClassWithInstanceField.ClassWithInstanceField()", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "ClassWithInstanceField.ClassWithInstanceField()")]
static void TestInstanceFieldCallDontWarn ()
{
ClassWithInstanceField instance = new ClassWithInstanceField ();
Expand Down Expand Up @@ -1095,13 +1095,13 @@ public int Method (int a)
}
}

[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.StaticMethod()", "--ClassWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.StaticMethod()", "--ClassWithRequires--")]
static void TestRequiresInClassAccessedByStaticMethod ()
{
ClassWithRequires.StaticMethod ();
}

[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires", "--ClassWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires", "--ClassWithRequires--")]
static void TestRequiresInClassAccessedByCctor ()
{
var classObject = new ClassWithRequires ();
Expand All @@ -1112,10 +1112,10 @@ static void TestRequiresInParentClassAccesedByStaticMethod ()
ClassWithRequires.NestedClass.NestedStaticMethod ();
}

[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.StaticMethod()", "--ClassWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.StaticMethod()", "--ClassWithRequires--")]
// Although we suppress the warning from RequiresOnMethod.MethodWithRequires () we still get a warning because we call CallRequiresMethod() which is an static method on a type with RUC
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.CallMethodWithRequires()", "--ClassWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "ClassWithRequires.Instance", "--ClassWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.CallMethodWithRequires()", "--ClassWithRequires--")]
[ExpectedWarning ("IL2026", "ClassWithRequires.Instance", "--ClassWithRequires--")]
static void TestRequiresOnBaseButNotOnDerived ()
{
DerivedWithoutRequires.StaticMethodInInheritedClass ();
Expand All @@ -1129,7 +1129,7 @@ static void TestRequiresOnBaseButNotOnDerived ()
DerivedWithoutRequires2.StaticMethod ();
}

[ExpectedWarning ("IL2026", "RequiresOnClass.DerivedWithRequires.StaticMethodInInheritedClass()", "--DerivedWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.DerivedWithRequires.StaticMethodInInheritedClass()", "--DerivedWithRequires--")]
static void TestRequiresOnDerivedButNotOnBase ()
{
DerivedWithRequires.StaticMethodInInheritedClass ();
Expand All @@ -1138,8 +1138,8 @@ static void TestRequiresOnDerivedButNotOnBase ()
DerivedWithRequires.NestedClass.NestedStaticMethod ();
}

[ExpectedWarning ("IL2026", "RequiresOnClass.DerivedWithRequires2.StaticMethodInInheritedClass()", "--DerivedWithRequires2--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.StaticMethod()", "--ClassWithRequires--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "RequiresOnClass.DerivedWithRequires2.StaticMethodInInheritedClass()", "--DerivedWithRequires2--")]
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.StaticMethod()", "--ClassWithRequires--")]
static void TestRequiresOnBaseAndDerived ()
{
DerivedWithRequires2.StaticMethodInInheritedClass ();
Expand All @@ -1148,7 +1148,8 @@ static void TestRequiresOnBaseAndDerived ()
DerivedWithRequires2.NestedClass.NestedStaticMethod ();
}

[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.TestSuppressions(Type[])", ProducedBy = ProducedBy.Trimmer)]
// TODO: Parameter signature differs between linker and analyzer
[ExpectedWarning ("IL2026", "RequiresOnClass.ClassWithRequires.TestSuppressions(", "Type[])")]
static void TestSuppressionsOnClass ()
{
ClassWithRequires.TestSuppressions (new[] { typeof (ClassWithRequires) });
Expand Down Expand Up @@ -1186,14 +1187,16 @@ class MemberTypesWithRequires
public static int Property { get; set; }

// These should not be reported https://github.com/mono/linker/issues/2218
[ExpectedWarning ("IL2026", "add_Event", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "remove_Event", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Event.add", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Event.remove", ProducedBy = ProducedBy.Trimmer)]
public static event EventHandler Event;
}

[ExpectedWarning ("IL2026", "MemberTypesWithRequires.field", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Property.set", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.remove_Event", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.field")]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Property.set")]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Event", ProducedBy = ProducedBy.Analyzer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Event.add", ProducedBy = ProducedBy.Analyzer)]
[ExpectedWarning ("IL2026", "MemberTypesWithRequires.Event.remove")]
static void TestOtherMemberTypesWithRequires ()
{
MemberTypesWithRequires.field = 1;
Expand Down Expand Up @@ -1334,7 +1337,7 @@ class WithRequires
[RequiresUnreferencedCode ("--WithRequiresOnlyInstanceFields--")]
class WithRequiresOnlyInstanceFields
{
public int InstnaceField;
public int InstanceField;
}

[ExpectedWarning ("IL2109", "ReflectionAccessOnField/DerivedWithoutRequires", "ReflectionAccessOnField.WithRequires", ProducedBy = ProducedBy.Trimmer)]
Expand All @@ -1361,20 +1364,22 @@ static void TestDAMAccess ()
typeof (DerivedWithRequires).RequiresPublicFields ();
}

[ExpectedWarning ("IL2026", "WithRequires.StaticField", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "WithRequires.StaticField")]
// Analyzer does not recognize the binding flags
Copy link
Member

Choose a reason for hiding this comment

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

Can we note the remaining pieces that won't be finished in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like adding:

// This attribute doesn't work because the analyzer still doesn't understand the binding flags
// [ExpectedWarning ("IL2026", "WithRequires.PrivateField", ProducedBy = ProducedBy.Analyzer)]
// This test needs support for DAM Analyzer
// [ExpectedWarning ("IL2026", "SomeDAMTest", ProducedBy = ProducedBy.Analyzer)]
// This test uses interfaces and inheritance which is still not supported by the analyzer
// [ExpectedWarning ("IL2026, "SomeInterfaceTest", ProducedBy = ProducedBy.Analyzer)]

Copy link
Member

Choose a reason for hiding this comment

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

I meant in the PR summary, just listing the stuff that doesn't work yet

[ExpectedWarning ("IL2026", "WithRequires.PrivateStaticField", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "DerivedWithRequires.DerivedStaticField", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "DerivedWithRequires.DerivedStaticField")]
[ExpectedWarning ("IL2026", "DerivedWithRequires.DerivedStaticField", ProducedBy = ProducedBy.Analyzer)]
static void TestDirectReflectionAccess ()
{
typeof (WithRequires).GetField (nameof (WithRequires.StaticField));
typeof (WithRequires).GetField (nameof (WithRequires.InstanceField)); // Doesn't warn
typeof (WithRequires).GetField ("PrivateStaticField", BindingFlags.NonPublic);
typeof (WithRequiresOnlyInstanceFields).GetField (nameof (WithRequiresOnlyInstanceFields.InstnaceField)); // Doesn't warn
typeof (WithRequiresOnlyInstanceFields).GetField (nameof (WithRequiresOnlyInstanceFields.InstanceField)); // Doesn't warn
typeof (DerivedWithoutRequires).GetField (nameof (DerivedWithRequires.DerivedStaticField)); // Doesn't warn
typeof (DerivedWithRequires).GetField (nameof (DerivedWithRequires.DerivedStaticField));
}

[ExpectedWarning ("IL2026", "WithRequires.StaticField", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "WithRequires.StaticField")]
[DynamicDependency (nameof (WithRequires.StaticField), typeof (WithRequires))]
[DynamicDependency (nameof (WithRequires.InstanceField), typeof (WithRequires))] // Doesn't warn
[DynamicDependency (DynamicallyAccessedMemberTypes.PublicFields, typeof (DerivedWithoutRequires))] // Doesn't warn
Expand Down Expand Up @@ -1426,12 +1431,14 @@ class WithRequires
{
// These should be reported only in TestDirectReflectionAccess
// https://github.com/mono/linker/issues/2218
[ExpectedWarning ("IL2026", "add_StaticEvent", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "remove_StaticEvent", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticEvent.add", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticEvent.remove", ProducedBy = ProducedBy.Trimmer)]
public static event EventHandler StaticEvent;
}

[ExpectedWarning ("IL2026", "add_StaticEvent", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "StaticEvent", ProducedBy = ProducedBy.Analyzer)]
[ExpectedWarning ("IL2026", "StaticEvent.remove", ProducedBy = ProducedBy.Analyzer)]
[ExpectedWarning ("IL2026", "StaticEvent.add")]
static void TestDirectReflectionAccess ()
{
typeof (WithRequires).GetEvent (nameof (WithRequires.StaticEvent));
Expand Down
2 changes: 1 addition & 1 deletion test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ void VerifyLoggedMessages (AssemblyDefinition original, LinkerTestLogger logger,
bool foundReflectionAccessPatternAttributesToVerify = false;
foreach (var attr in attrProvider.CustomAttributes) {
if (!IsProducedByLinker (attr))
break;
continue;

switch (attr.AttributeType.Name) {

Expand Down