-
Notifications
You must be signed in to change notification settings - Fork 127
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
Mark generic arguments of dynamically accessed types passed as string #1566
Mark generic arguments of dynamically accessed types passed as string #1566
Conversation
// If this is a generic type, mark all its arguments. | ||
if (typeReference is GenericInstanceType genericType) { | ||
foreach (var arg in genericType.GenericArguments) { | ||
MarkTypeForDynamicallyAccessedMembers (ref reflectionContext, arg, requiredMemberTypes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excessive and sort of wrong.
Take for example:
class MyType
{
public void UnusedMethod() {}
}
void Test()
{
Type listType = Type.GetType("System.Collections.Generic.List`1[MyType]");
MethodInfo addMethod = listType.GetMethod("Add");
}
The call to GetMethod
is annotated with DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)
. So with your code it would mark List
1.Addbut also
MyType.UnusedMethod`.
I think the easiest way to fix this would be to simply call MarkType
on the TypeReference
and then apply additional marking based on the annotations. MarkType
on its own should not include more then necessary for the type (on simply cases it will not mark any methods and so on).
For non-generic cases this is already happening, since for example marking a method on the type (due to annotations) will also mark the type (MarkMethod does that). The problem is with generics since by calling .Resolve
we lose the generic instantiation part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a way this is a very similar problem to #1559.
Can you please validate if your change also affects the behavior of arrays as noted in #1559?
Either make sure to note the differences in the other issue, or maybe try to fix both at once - I would expect the fixes to be relatively similar.
Basically we need to pass around TypeReference
instead of TypeDefinition
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I wasn't sure how much to keep of a type that is passed as generic argument in a dynamically accessed member - I changed things so that we use MarkTypeVisibleToReflection
to mark the type references of these.
Based on the discussion in #1559 I changed ProcessGenericArgumentDataFlow
so that we mark the element type of the generic argument that is passed. This way, passing T[]
will result in the linker marking T
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion in #1559 is at best inconclusive. I think we definitely need to mark the array type itself (based on the annotations). Right now we're also marking the element type based on the annotations, which is technically incorrect. I'm starting to agree with @MichalStrehovsky that we can stop doing that - the cases where this would matter should be very few.
Note that the original repro with JSON serializing T[]
is sort of "wrong" anyway. We don't have an annotation which would work for serializers - it would have to be something like RecursivePublicProperties
. Once we have that, it could also handle arrays (as in mark the element types recursively). Something like this has been proposed a while ago: #1087.
if (typeRef is GenericInstanceType genericType) { | ||
foreach (var arg in genericType.GenericArguments) | ||
MarkType (ref reflectionContext, arg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary and in also not complete. The issue at hand is about generics, but the same problem applies to T[]
and/or T*
and so on. All of this is already handled by MarkType
- which calls GetOriginalType
which does the recursive walk of the element types and/or generic arguments.
So all we need to do here is call MarkType(typeRef)
. It should not include "more" as marking a type really only guarantees presence of the type, but none of its members.
var resolvedReference = typeReference is ArrayType ? | ||
typeReference.Module.ImportReference (typeof (Array))?.Resolve () : typeReference.Resolve (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please introduce a helper method for this? Maybe an extension method on TypeReference
- something like ResolveToMainTypeDefinition
... I can't come up with a better name - maybe @MichalStrehovsky will know. It should:
- For normal types it does what
.Resolve
does today - For generics it resolves to the open generic type (which is what
.Resolve
does today as well) - For arrays it resolves to
System.Array
type def. - The question is what should happen for other "weird" cases like - multi-dim arrays, pointers, by-refs, ... we should probably look into this some more if it can every happen and what does it mean when it does happen.
Eventually (We should either fix this right away or file an issue) this should handle not just arrays, but pointers, by-refs and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this as an extension method as you suggested. Note that for multidim arrays it will behave the same way as for unidimensional arrays, since Cecil does not differentiate between the two (both are ArrayType
). Not sure if we want different "resolve semantics" for pointers or by-refs though.
Mostly array resolution issue related to dotnet#1566
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are more places where we call .Resolve and end up with the wrong thing. Simply search for .Resolve () in the ReflectionMethodBodyScanner.
I wrote tests for the cases which I don't think are covered by this change: #1580
Mostly array resolution issue related to #1566
Enable ComplexTypeHandling tests
Whitespace
5ed1c7d
to
6bcf9a2
Compare
Nice 👍 |
…dotnet#1566) * Mark dynamically accessed generic arguments * Clean test * Use MarkType for keeping generic args Enable ComplexTypeHandling tests * Use MarkTypeVisibleToReflection for generic arguments Clean tests * MarkType always * Check for array types in TypeNameResolver * Mark System.Array instead of its element type * Whitespace * PR feedback * Pattern match Whitespace * Add extension method ResolveToMainTypeDefinition * Add comment * Use ResolveToMainTypeDefinition
Mostly array resolution issue related to dotnet/linker#1566 Commit migrated from dotnet/linker@2699ffa
…dotnet/linker#1566) * Mark dynamically accessed generic arguments * Clean test * Use MarkType for keeping generic args Enable ComplexTypeHandling tests * Use MarkTypeVisibleToReflection for generic arguments Clean tests * MarkType always * Check for array types in TypeNameResolver * Mark System.Array instead of its element type * Whitespace * PR feedback * Pattern match Whitespace * Add extension method ResolveToMainTypeDefinition * Add comment * Use ResolveToMainTypeDefinition Commit migrated from dotnet/linker@2a4eb66
Currently, the linker won't mark the generic arguments of a dynamically accessed type that is passed as a string. For an example, see #1537.