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

Add Type.GetMemberWithSameMetadataDefinitionAs #53704

Merged
merged 12 commits into from
Jun 15, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jun 3, 2021

This method allows for trim-compatible reflection over generic types. Developers can cache the open-generic MemberInfo statically, and then using the closed-generic Type, they can look up the corresponding MemberInfo. This results in a hard reference to the generic member, so the trimmer won't remove it or warn about the reflection usage.

Fix #45771

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

MemberTypes.Property => GetPropertyWithSameMetadataDefinitionAs(member),
MemberTypes.Field => GetFieldWithSameMetadataDefinitionAs(member),
MemberTypes.Event => GetEventWithSameMetadataDefinitionAs(member),
MemberTypes.NestedType => GetNestedTypeWithSameMetadataDefinitionAs(member),
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to do anything interesting for nested types? The nested types are not instantiated using the enclosed type instantiation, so I think this will always return the same type as that was passed in.

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably isn't going to do anything interesting, but I thought it would be incomplete without it, since GetMembers returns the nested types.

The nested types are not instantiated using the enclosed type instantiation,

I learned that the hard way in my tests 😄

if (openGenericMember is not Type)
{
Assert.True(closedGenericMember.DeclaringType == closedGenericType, $"'{closedGenericMember.Name}' doesn't have the right DeclaringType");
}

@eerhardt eerhardt marked this pull request as ready for review June 4, 2021 23:14
return result;
return (MethodInfo)taskType.GetMemberWithSameMetadataDefinitionAs(s_taskGetResultMethodInfo);
}
catch (ArgumentException)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to keep the original code? Is throwing and catching exception here going to be a performance problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

The caught exception should be rare - cases where you are returning a Task that isn't t.GetType() == typeof(Task) and isn't Task<T> or a derived type.

I'm not sure if just doing the try is going to be performance problem. This code is about interop'ing with JavaScript, which itself is fairly expensive.

I can revert and use the original code. But it is a pity that this was the first scenario discovered and the one written up in the API issue, and we decide that we can't use this API for this scenario.

Other options I've thought about:

  1. Trying to check if the type is Task<T> or a derived type before calling GetMemberWithSameMetadataDefinitionAs, but I'm not sure how to do this efficiently and without walking the base types manually.
  2. Reverting back to GetMemberWithSameMetadataDefinitionAs returning null if it isn't found. We wouldn't need to do anything special here then.

I think probably the best option in the short term is to just leave this code as it is in main. We can always change it later.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

It seems like we keep dancing around the scenario of "I have a Task (or something that derives from it), and I want to get its result, and I'm ok with boxing." This seems like it should be fairly straightforward: put a first-class API on Task or TaskExtensions to provide this information. For Task, it could return null. Involving reflection for this scenario seems awfully heavyweight.

Copy link
Member

@jkotas jkotas Jun 10, 2021

Choose a reason for hiding this comment

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

What is Task or TaskExtensions going to do differently? It will have to use reflection to do the job. (Or it will have to be non-pay-for-play.)

Copy link
Member

@jkotas jkotas Jun 10, 2021

Choose a reason for hiding this comment

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

without walking the base types manually.

Walking the base types manually would be like 5 lines and pretty efficient.

I am not sure whether the exceptional case is as rare as you may think - we have multiple types inheriting from Task in the framework itself. If it is really rare here, the try/catch should be ok.

It is interesting that the caller uses Array.Empty<object>() for results that are Task exactly, but this logic returns null for results that derive from Task. It suggests that the results that derive from Task may be broken. Have you actually seen a test case that hits the type deriving from Task, but not from Task<T> case?

Copy link
Member

@stephentoub stephentoub Jun 11, 2021

Choose a reason for hiding this comment

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

That's the non-pay-for-play concern. If we were to decide we're fine with the cost, then the approach we've previously discussed is similar to what you outline, e.g.

public async Task<object?> AsObjectTask()
{
    await this.ConfigureAwait(false);
    return this is ITaskWithResult t ? t.ResultAsObject : null;
}

so implementation is obviously far from hard. Not sure exactly what you'd want to call it, or whether it'd be better hidden away somewhere as a static (non-extension) method. Yes, this has been requested now and again, but it's not all that common a need.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I saw 13 matches for typeof(Task<>) in source.dot.net and believed this to be a more common pattern. (To be fair, I didn't trace back many of those calls.) If this isn't as common as I thought, no worries. :)

Copy link
Member

@stephentoub stephentoub Jun 11, 2021

Choose a reason for hiding this comment

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

For my edification, how do you search for typeof(Task<>) in source.dot.net? Or do you mean find all Task<TResult> usage via source.dot.net and then using the browser's search look for typeof(Task?

Copy link
Member

Choose a reason for hiding this comment

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

https://source.dot.net/System.Private.CoreLib/R/5ca7b77f3df89fc6.html

Because the matches in the left pane show the reference in context (the entire source line is there), you can CTRL-F in your browser and search "typeof(Task<>)". Kinda hokey, but if it works, it works.

Copy link
Member

@stephentoub stephentoub Jun 11, 2021

Choose a reason for hiding this comment

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

Thanks. Quickly skimmed the results I see:

  • Most of them are checking to see whether a type is a Task or Task<> in order to decide whether the method should be considered async / is task returning.
  • Another large chunk are various forms of code generators trying to determine what return type to use for the generated method (e.g. CodeDOM, F#'s conversion from tasks to its async model)
  • Some are looking to understand the type of the T in the Task<T> as part of API exploration / doc generation.
  • A few are trying to match up the T to see which of a few known types it might map to, e.g. for razor page handler execution
  • 1, from Microsoft.JSInterop, was about getting the object result from an arbitrary Task<T>

return result;
return (MethodInfo)taskType.GetMemberWithSameMetadataDefinitionAs(s_taskGetResultMethodInfo);
}
catch (ArgumentException)
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we keep dancing around the scenario of "I have a Task (or something that derives from it), and I want to get its result, and I'm ok with boxing." This seems like it should be fairly straightforward: put a first-class API on Task or TaskExtensions to provide this information. For Task, it could return null. Involving reflection for this scenario seems awfully heavyweight.

@eerhardt eerhardt force-pushed the GetMemberWithSameMetadataDefinitionAs branch from a1ca1c2 to a38c660 Compare June 10, 2021 03:10
@eerhardt
Copy link
Member Author

I believe this is ready for final review. PTAL.

@ghost
Copy link

ghost commented Jun 11, 2021

Tagging @dotnet/area-system-reflection for review.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost
Copy link

ghost commented Jun 15, 2021

Hello @eerhardt!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@eerhardt eerhardt merged commit cf7e7a4 into dotnet:main Jun 15, 2021
@eerhardt eerhardt deleted the GetMemberWithSameMetadataDefinitionAs branch June 15, 2021 21:14
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add API to find MethodInfo on instantiated generic type from generic type definition
6 participants