Skip to content

Fix warnings for DAM.All #2191

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

Merged
merged 5 commits into from
Aug 10, 2021
Merged

Fix warnings for DAM.All #2191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff9216e
Fix warnings for DAM.All
sbomer Aug 5, 2021
7692811
Add tests
sbomer Aug 5, 2021
f97b935
PR feedback
sbomer Aug 5, 2021
b1abc94
Avoid redundant DAM warnings for base members
sbomer Aug 9, 2021
9a1aa77
PR feedback
sbomer Aug 10, 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
128 changes: 105 additions & 23 deletions src/linker/Linker.Dataflow/DynamicallyAccessedMembersBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,18 @@ internal static class DynamicallyAccessedMemberTypesOverlay

internal static class DynamicallyAccessedMembersBinder
{
// Returns the members of the type bound by memberTypes. For DynamicallyAccessedMemberTypes.All, this returns a single null result.
// This sentinel value allows callers to handle the case where DynamicallyAccessedMemberTypes.All conceptually binds to the entire type
// including all recursive nested members.
public static IEnumerable<IMetadataTokenProvider> GetDynamicallyAccessedMembers (this TypeDefinition typeDefinition, LinkContext context, DynamicallyAccessedMemberTypes memberTypes)
// Returns the members of the type bound by memberTypes. For DynamicallyAccessedMemberTypes.All, this returns all members of the type and its
// nested types, including interface implementations, plus the same or any base types or implemented interfaces.
// DynamicallyAccessedMemberTypes.PublicNestedTypes and NonPublicNestedTypes do the same for members of the selected nested types.
public static IEnumerable<IMetadataTokenProvider> GetDynamicallyAccessedMembers (this TypeDefinition typeDefinition, LinkContext context, DynamicallyAccessedMemberTypes memberTypes, bool declaredOnly = false)
{
var declaredOnlyFlags = declaredOnly ? BindingFlags.DeclaredOnly : BindingFlags.Default;

if (memberTypes == DynamicallyAccessedMemberTypes.All) {
yield return null;
var members = new List<IMetadataTokenProvider> ();
typeDefinition.GetAllOnType (context, declaredOnly, members);
foreach (var m in members)
yield return m;
yield break;
}

Expand All @@ -45,57 +50,67 @@ public static IEnumerable<IMetadataTokenProvider> GetDynamicallyAccessedMembers
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicMethods)) {
foreach (var m in typeDefinition.GetMethodsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic))
foreach (var m in typeDefinition.GetMethodsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic | declaredOnlyFlags))
yield return m;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicMethods)) {
foreach (var m in typeDefinition.GetMethodsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public))
foreach (var m in typeDefinition.GetMethodsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public | declaredOnlyFlags))
yield return m;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicFields)) {
foreach (var f in typeDefinition.GetFieldsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic))
foreach (var f in typeDefinition.GetFieldsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic | declaredOnlyFlags))
yield return f;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicFields)) {
foreach (var f in typeDefinition.GetFieldsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public))
foreach (var f in typeDefinition.GetFieldsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public | declaredOnlyFlags))
yield return f;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicNestedTypes)) {
foreach (var t in typeDefinition.GetNestedTypesOnType (filter: null, bindingFlags: BindingFlags.NonPublic))
yield return t;
foreach (var nested in typeDefinition.GetNestedTypesOnType (filter: null, bindingFlags: BindingFlags.NonPublic)) {
yield return nested;
var members = new List<IMetadataTokenProvider> ();
nested.GetAllOnType (context, declaredOnly: false, members);
foreach (var m in members)
yield return m;
}
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicNestedTypes)) {
foreach (var t in typeDefinition.GetNestedTypesOnType (filter: null, bindingFlags: BindingFlags.Public))
yield return t;
foreach (var nested in typeDefinition.GetNestedTypesOnType (filter: null, bindingFlags: BindingFlags.Public)) {
yield return nested;
var members = new List<IMetadataTokenProvider> ();
nested.GetAllOnType (context, declaredOnly: false, members);
foreach (var m in members)
yield return m;
}
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicProperties)) {
foreach (var p in typeDefinition.GetPropertiesOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic))
foreach (var p in typeDefinition.GetPropertiesOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic | declaredOnlyFlags))
yield return p;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicProperties)) {
foreach (var p in typeDefinition.GetPropertiesOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public))
foreach (var p in typeDefinition.GetPropertiesOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public | declaredOnlyFlags))
yield return p;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicEvents)) {
foreach (var e in typeDefinition.GetEventsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic))
foreach (var e in typeDefinition.GetEventsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.NonPublic | declaredOnlyFlags))
yield return e;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicEvents)) {
foreach (var e in typeDefinition.GetEventsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public))
foreach (var e in typeDefinition.GetEventsOnTypeHierarchy (context, filter: null, bindingFlags: BindingFlags.Public | declaredOnlyFlags))
yield return e;
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypesOverlay.Interfaces)) {
foreach (var i in typeDefinition.GetAllInterfaceImplementations (context))
foreach (var i in typeDefinition.GetAllInterfaceImplementations (context, declaredOnly))
yield return i;
}
}
Expand Down Expand Up @@ -160,6 +175,9 @@ public static IEnumerable<MethodDefinition> GetMethodsOnTypeHierarchy (this Type
yield return method;
}

if ((bindingFlags & BindingFlags.DeclaredOnly) == BindingFlags.DeclaredOnly)
yield break;

type = context.TryResolve (type.BaseType);
onBaseType = true;
}
Expand Down Expand Up @@ -196,6 +214,9 @@ public static IEnumerable<FieldDefinition> GetFieldsOnTypeHierarchy (this TypeDe
yield return field;
}

if ((bindingFlags & BindingFlags.DeclaredOnly) == BindingFlags.DeclaredOnly)
yield break;

type = context.TryResolve (type.BaseType);
onBaseType = true;
}
Expand Down Expand Up @@ -261,6 +282,9 @@ public static IEnumerable<PropertyDefinition> GetPropertiesOnTypeHierarchy (this
yield return property;
}

if ((bindingFlags & BindingFlags.DeclaredOnly) == BindingFlags.DeclaredOnly)
yield break;

type = context.TryResolve (type.BaseType);
onBaseType = true;
}
Expand Down Expand Up @@ -306,26 +330,84 @@ public static IEnumerable<EventDefinition> GetEventsOnTypeHierarchy (this TypeDe
yield return @event;
}

if ((bindingFlags & BindingFlags.DeclaredOnly) == BindingFlags.DeclaredOnly)
yield break;

type = context.TryResolve (type.BaseType);
onBaseType = true;
}
}

public static IEnumerable<InterfaceImplementation> GetAllInterfaceImplementations (this TypeDefinition type, LinkContext context)
public static IEnumerable<InterfaceImplementation> GetAllInterfaceImplementations (this TypeDefinition type, LinkContext context, bool declaredOnly)
{
while (type != null) {
foreach (var i in type.Interfaces) {
yield return i;

TypeDefinition interfaceType = context.TryResolve (i.InterfaceType);
if (interfaceType != null) {
foreach (var innerInterface in interfaceType.GetAllInterfaceImplementations (context))
yield return innerInterface;
if (!declaredOnly) {
TypeDefinition interfaceType = context.TryResolve (i.InterfaceType);
if (interfaceType != null) {
foreach (var innerInterface in interfaceType.GetAllInterfaceImplementations (context, declaredOnly: false))
yield return innerInterface;
}
}
}

if (declaredOnly)
yield break;

type = context.TryResolve (type.BaseType);
}
}

public static void GetAllOnType (this TypeDefinition type, LinkContext context, bool declaredOnly, List<IMetadataTokenProvider> members) => GetAllOnType (type, context, declaredOnly, members, new HashSet<TypeDefinition> ());

static void GetAllOnType (TypeDefinition type, LinkContext context, bool declaredOnly, List<IMetadataTokenProvider> members, HashSet<TypeDefinition> types)
{
if (!types.Add (type))
return;

if (type.HasNestedTypes) {
foreach (var nested in type.NestedTypes) {
members.Add (nested);
// Base types and interfaces of nested types are always included.
GetAllOnType (nested, context, declaredOnly: false, members, types);
}
}

if (!declaredOnly) {
var baseType = context.TryResolve (type.BaseType);
if (baseType != null)
GetAllOnType (baseType, context, declaredOnly: false, members, types);
}

if (!declaredOnly && type.HasInterfaces) {
foreach (var iface in type.Interfaces) {
members.Add (iface);
var interfaceType = context.TryResolve (iface.InterfaceType);
GetAllOnType (interfaceType, context, declaredOnly: false, members, types);
}
}

if (type.HasFields) {
foreach (var f in type.Fields)
members.Add (f);
}

if (type.HasMethods) {
foreach (var m in type.Methods)
members.Add (m);
}

if (type.HasProperties) {
foreach (var p in type.Properties)
members.Add (p);
}

if (type.HasEvents) {
foreach (var e in type.Events)
members.Add (e);
}
}
}
}
23 changes: 14 additions & 9 deletions src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,17 @@ public void ApplyDynamicallyAccessedMembersToType (ref ReflectionPatternContext
// Handle cases where a type has no members but annotations are to be applied to derived type members
reflectionPatternContext.RecordHandledPattern ();

MarkTypeForDynamicallyAccessedMembers (ref reflectionPatternContext, type, annotation, DependencyKind.DynamicallyAccessedMemberOnType);
// The annotations this type inherited from its base types should not produce
// warnings on the base members, since those are covered by warnings in the base type.
// For now, annotations that are inherited from base types but also declared explicitly on this type
// still warn about base members since the cache doesn't track enough information to tell the difference.
Comment on lines +132 to +133
Copy link
Member

@vitek-karas vitek-karas Aug 10, 2021

Choose a reason for hiding this comment

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

I think this is fine - the only case where I can think of this making sense is if the derived type adds more annotations (base has all methods and derived type has both methods and fields) - but that is SO unlikely and rare - I don't care :-).

var declaredAnnotation = _context.Annotations.FlowAnnotations.GetTypeAnnotation (type);
var inheritedOnlyAnnotation = annotation & ~declaredAnnotation;

// Warn about all members accessed by the annotation on this type (including members of base types/interfaces)
MarkTypeForDynamicallyAccessedMembers (ref reflectionPatternContext, type, declaredAnnotation, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
// Warn about members accessed by inherited annotations (not including members of base types/interfaces)
MarkTypeForDynamicallyAccessedMembers (ref reflectionPatternContext, type, inheritedOnlyAnnotation, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: true);
}

ValueNode GetValueNodeForCustomAttributeArgument (CustomAttributeArgument argument)
Expand Down Expand Up @@ -2285,9 +2295,9 @@ void RequireDynamicallyAccessedMembers (ref ReflectionPatternContext reflectionC

static bool HasBindingFlag (BindingFlags? bindingFlags, BindingFlags? search) => bindingFlags != null && (bindingFlags & search) == search;

void MarkTypeForDynamicallyAccessedMembers (ref ReflectionPatternContext reflectionContext, TypeDefinition typeDefinition, DynamicallyAccessedMemberTypes requiredMemberTypes, DependencyKind dependencyKind)
void MarkTypeForDynamicallyAccessedMembers (ref ReflectionPatternContext reflectionContext, TypeDefinition typeDefinition, DynamicallyAccessedMemberTypes requiredMemberTypes, DependencyKind dependencyKind, bool declaredOnly = false)
{
foreach (var member in typeDefinition.GetDynamicallyAccessedMembers (_context, requiredMemberTypes)) {
foreach (var member in typeDefinition.GetDynamicallyAccessedMembers (_context, requiredMemberTypes, declaredOnly)) {
switch (member) {
case MethodDefinition method:
MarkMethod (ref reflectionContext, method, dependencyKind);
Expand All @@ -2296,8 +2306,7 @@ void MarkTypeForDynamicallyAccessedMembers (ref ReflectionPatternContext reflect
MarkField (ref reflectionContext, field, dependencyKind);
break;
case TypeDefinition nestedType:
DependencyInfo nestedDependencyInfo = new DependencyInfo (dependencyKind, reflectionContext.Source);
reflectionContext.RecordRecognizedPattern (nestedType, () => _markStep.MarkEntireType (nestedType, includeBaseAndInterfaceTypes: true, nestedDependencyInfo));
MarkType (ref reflectionContext, nestedType, dependencyKind);
Copy link
Member

@vitek-karas vitek-karas Aug 9, 2021

Choose a reason for hiding this comment

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

I must admit I'm a bit nervous about potential endless recursion. After all that's why MarkEntireType has the hashset - to avoid that. This is basically trying to do the same thing - without it.

If I remember correctly the counter example for MarkEntireType was something like this:

class MyAttribute : Attribute
{
    MyAttribute([DAM(All)] Type type) {}
}

[MyAttribute(typeof(TestType))]
class TestType
{
}

This would be endless since MarkEntireType calls MarkCustomAttributes which will in turn eventually call MarkEntireType (due to the All annotation) and so on.

I think this new code will work, since MarkType effectively has a hashset in the CheckProcessed which happens before the custom attribute marking. (Similar repro can be made via base types or implemented interfaces with generics - but the solution is the same I hope).

So I guess there's no problem in the end, I hope...

Copy link
Member

@vitek-karas vitek-karas Aug 9, 2021

Choose a reason for hiding this comment

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

Can you add a test case along these lines. The base type one would look like:

class Base<[DAM(All)] T> {}

class TestType : Base<TestType> {}

Similarly for interfaces.

Another one is attributes on members: We already have an issue with this on properties and other members - see #2196 (I just created this). The All is basically just another case of the same problem for these members.

Copy link
Member Author

@sbomer sbomer Aug 10, 2021

Choose a reason for hiding this comment

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

Yup, the first example is already covered by this test: https://github.com/mono/linker/blob/e21bb44ca909d33c18acf3ed0b68f94a413351a5/test/Mono.Linker.Tests.Cases/DataFlow/AttributeConstructorDataflow.cs#L96-L100.

I added the second testcase you suggested - I think generic type parameters shouldn't be an issue since the DAM binder only gives back members and doesn't look at the instantiated base generic type at all (instead it relies on normal MarkType logic to mark generic arguments).

Good find about attributes on properties/events.

Copy link
Member

@vitek-karas vitek-karas Aug 10, 2021

Choose a reason for hiding this comment

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

(instead it relies on normal MarkType logic to mark generic arguments).

That was the point - I don't think MarkType was written with reentrancy in mind - it seems to handle it, which is good, but I thought it would be good to have tests for it. Before data flow, there were only a few ways to get to the reentrancy, now there are many more.

But I'm fine if we try to cover this later on.

Copy link
Member Author

@sbomer sbomer Aug 10, 2021

Choose a reason for hiding this comment

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

Yeah, it looks like it was at least given some thought but it's good to have more tests as we add more ways to get to MarkType. I did add the generic tests you suggested above so I think that is covered.

break;
case PropertyDefinition property:
MarkProperty (ref reflectionContext, property, dependencyKind);
Expand All @@ -2308,10 +2317,6 @@ void MarkTypeForDynamicallyAccessedMembers (ref ReflectionPatternContext reflect
case InterfaceImplementation interfaceImplementation:
MarkInterfaceImplementation (ref reflectionContext, interfaceImplementation, dependencyKind);
break;
case null:
DependencyInfo dependencyInfo = new DependencyInfo (dependencyKind, reflectionContext.Source);
reflectionContext.RecordRecognizedPattern (typeDefinition, () => _markStep.MarkEntireType (typeDefinition, includeBaseAndInterfaceTypes: true, dependencyInfo));
break;
}
}
}
Expand Down
Loading