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

Consider allowing DynamicallyAccessedMembers on types #1196

Closed
MichalStrehovsky opened this issue May 15, 2020 · 13 comments
Closed

Consider allowing DynamicallyAccessedMembers on types #1196

MichalStrehovsky opened this issue May 15, 2020 · 13 comments

Comments

@MichalStrehovsky
Copy link
Member

We don't currently allow this. The semantic would be "keep the specified member types on this type and all derived types".

Blazor has a pattern where it does Object.GetType on an unsealed type and then goes over the properties on that type. This annotation would make it so that the result of Object.GetType is an annotated System.Type instance (the annotation would be an aggregate of all DynamicallyAccessedMemberKinds specified on the type of this for the GetType call and its base types).

This would make the GetProperties call provably linker-safe extra annotations/suppressions.

@vitek-karas
Copy link
Member

What about interfaces - the famous IControl in Blazor. That would be actually quite powerful - anything which implements the interface would get that.

@MichalStrehovsky
Copy link
Member Author

Can you remind me of the IControl pattern? Is this also in connection with Object.GetType, or is that the one where they scan everything in the assembly to find the things that implement IControl?

If it's the latter, it would still have the problem with:

  1. How do we connect this with the dataflow analysis? (How does linker know fooAssembly.GetTypes().Select(x => x.IsAssignableFrom(typeof(IControl)) is actually linker safe and produces annotated Type instances using the annotation from the interface). This is pretty hard to solve in general.
  2. How does markstep know about the type. We're slowly moving in a direction where MarkStep will only look at things that are reachable, but this would require us to look at everything again. This is solvable, but a bit annoying.

@vitek-karas
Copy link
Member

I don't know for sure, but I think it's the "DI-like" behavior - so this attribute alone would not solve the whole problem. But it could be a part of the solution:

  • Something which makes sure all the necessary types get marked (not this attribute)
  • Something which makes sure that all those types have the necessary members (this attribute)

I do agree that recognizing the general pattern would be hard, but we could possibly handle some special pattern which would help with that. In the end this is problematic because it's likely that those types are stored in a collection and we don't have an annotation for collection members really.

@MichalStrehovsky
Copy link
Member Author

This would also help analyzing EventSource because it has the same problem (calls GetType on itself and reflects on its methods and nested types) - let's go for this.

@vitek-karas
Copy link
Member

Two questions:

  • I assume this would mean (you already mentioned this, but to make it explicit) - keep the specified members only if the type itself is being kept for other reasons.
  • There would be a slight discrepancy in behavior: When used on a System.Type value, it currently only marks members on the known type and all its base types. It does NOT mark members on derived types. But when used on a type declaration it would include members on derived types. It would be really good for the behavior to be consistent in some way.

@MichalStrehovsky
Copy link
Member Author

I assume this would mean (you already mentioned this, but to make it explicit) - keep the specified members only if the type itself is being kept for other reasons.

Yep, the purpose is to make Object.GetType to work with this. If we have an instance of the type in the system, linker definitely knows about the type.

it currently only marks members on the known type and all its base types

I think you're looking at it from an implementation perspective. From a user perspective, this is doing the same thing - it ensures the System.Type instance produced from Object.GetType can be used with certain reflection APIs. The fact that member preservation also walks in a different direction of the inheritance chain is an implementation detail. The annotation still just means: I'll be able to dynamically access members on instances stored in the given location type.

@eerhardt
Copy link
Member

eerhardt commented Aug 3, 2020

Could this functionality be used to make ASP.NET MVC linker-safe? cc @davidfowl

In MVC, there is a base class ControllerBase that all controllers derive from. The MVC framework will look through all the Types, looking for Types derived from ControllerBase, and try to instantiate them and call methods on them.

@MichalStrehovsky
Copy link
Member Author

Could this functionality be used to make ASP.NET MVC linker-safe?

If the pattern is "dig though all types in an assembly and find the ones that derive from Base", this wouldn't help. The dataflow annotations are about making a connection from a place where System.Type is produced to a place where the System.Type is consumed by a reflection API.

What is being proposed here is to allow making a connection starting from Base someBase; .... someBase.GetType(): when Base was annotated with the annotation, and linker sees a GetType call on something statically typed as Base, it would propagate the annotation to the produced System.Type instance.

This wouldn't work so nicely for the patterns where someone goes over all types in an assembly, filters them with something horrible like a lambda with a capture that yield returns results. This would require a lot of analysis to ensure that:

  1. The loop over all types in an assembly is indeed safe because it's only looking at things that were preserved by some other rule.
  2. The resulting type gets annotation based on the matching preservation rule.

That kinds of patterns could probably only be made safe (with reasonable effort) by adding a dedicated new API (e.g. Assembly.GetTypesDerivingFrom(Type type)

@davidfowl
Copy link
Member

That kinds of patterns could probably only be made safe (with reasonable effort) by adding a dedicated new API (e.g. Assembly.GetTypesDerivingFrom(Type type)

This pattern is so common, it would be awesome to have an API that was declarative like this in the BCL.

@pranavkm
Copy link

pranavkm commented Feb 5, 2021

This might be one way for us to solve #1806 which I just filed today. Bringing this topic back up since we're trying to annotate ASP.NET Core and EF for 6.0.

@mateoatr
Copy link
Contributor

Can this be closed? Looks like dotnet/runtime#49778 and #1929 solve this.

@MichalStrehovsky
Copy link
Member Author

Yup.

@davidfowl
Copy link
Member

👍🏾

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

No branches or pull requests

7 participants