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

Track MethodInfo, PropertyInfo, and FieldInfo through reflection uses #3037

Open
jtschuster opened this issue Sep 16, 2022 · 4 comments
Open
Labels

Comments

@jtschuster
Copy link
Member

For codebases that use reflection extensively, this could simplify transitioning to trim compatibility.

For example, EFCore.Sqlite has this code:

var currentAppData = Type.GetType("Windows.Storage.ApplicationData, Windows, ContentType=WindowsRuntime")
    ?? Type.GetType("Windows.Storage.ApplicationData, Microsoft.Windows.SDK.NET")
    ?.GetRuntimeProperty("Current")?.GetValue(null);
var localFolder = currentAppData?.GetType()
    .GetRuntimeProperty("LocalFolder")?.GetValue(currentAppData);
var localFolderPath = (string?)localFolder?.GetType()
    .GetRuntimeProperty("Path")?.GetValue(localFolder);

This can technically be statically analyzed, and we know the exact types we need: Windows.Storage.ApplicationData, Windows.Storage.ApplicationData.Current, the type of Windows.Storage.ApplicationData.Current, and its property LocalFolder, the type of LocalFolder, and its property Path.

In addition, we may be able to make MethodInfo.MakeGenericMethod AOT-Friendly if we know to keep the method in its generic form and the type being made generic.

Related to #2482

@jtschuster jtschuster changed the title We could track MethodInfo, PropertyInfo, and FieldInfo through reflection uses Track MethodInfo, PropertyInfo, and FieldInfo through reflection uses Sep 16, 2022
@MichalStrehovsky
Copy link
Member

We would also need a "typed stack" because of the PropertyInfo.GetValue() and object.GetType() methods in-between (we need to be able to figure out the type of the thing stored in the property - we're lucky because it's a sealed type).

We did this kind of analysis in .NET Native (where illinker dataflow analysis originally comes from). The logic wasn't ported over to illink because it doesn't fundamentally move the needle. We kept adding patterns for all sorts of things (.NET Native could also model System.Reflection.Assembly, return types of MethodInfo's, some of dynamic keyword usage etc.) We were in this mode where whenever we found a pattern that could be analyzed, we built the analysis for it. It had very diminishing returns - it only very rarely helped outside of the one place that we built it for, even though it looked general purpose.

At the time .NET Native was getting defunded, we were thinking about modeling string concatenation operations because there was an app that concatenated type name and namespace and fed it to Type.GetType. Once you start adding these, it's really hard to find the place where to stop doing it.

My 2 cents is that I think it would be better use of time to just rewrite the code in EF to use the primitives we already support.

@vitek-karas
Copy link
Member

I agree with Michal about the pattern above. That said we have other reasons to support "typed stack":
#1218
dotnet/runtime#93720

If/when we do that, then adding this is not such a stretch. But I do agree that we should first see how common this is. I remember there was a more appealing case for something like this which was about the ability to store MethodInfo in a static/readonly field for perf reasons - and linker being able to recognize it (currently such pattern has no way to be annotated without warning).

I think a more likely thing we would support is a pattern like:

Type type = typeof(TestType).GetField("TestField").FieldType;
type.GetMethods(); // No warning - currently this will warn

That is more of a "static" approach. The object.GetType is just problematic all way round. For example, it can only reliably work if the static type of the variable is sealed. If it's not sealed we can't really predict the result of GetType() and thus can't predict the result of GetFiled() on it. (the above use case has all types sealed, so it could work)

The above code can be rewritten with the FieldType approach - still not analyzable currently though. The only currently analyzable pattern would be to call Type.GetType on all of the involved types, which is not pretty and definitely rather ineffective.

@vitek-karas
Copy link
Member

Found the issue discussing ability to store MethodInfo in a static readonly field without breaking data flow:
#2482

@MichalStrehovsky
Copy link
Member

If we want to take on this work, my suggestion would be to be very crisp about scoping. Otherwise it's easy to spend several man-months doing tiny features that we wouldn't have done if we know it's a several months big workitem (i.e. this is a rabbit hole and it's very easy to spend unjustifiable amounts of time on it because it's all broken into increments so small that they're not part of planning).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants