Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Emit metadata for type forwarders - #5054

Merged
MichalStrehovsky merged 3 commits into
dotnet:masterfrom
MichalStrehovsky:typeForwarders
Dec 7, 2017
Merged

Emit metadata for type forwarders#5054
MichalStrehovsky merged 3 commits into
dotnet:masterfrom
MichalStrehovsky:typeForwarders

Conversation

@MichalStrehovsky

@MichalStrehovsky MichalStrehovsky commented Dec 2, 2017

Copy link
Copy Markdown
Member

Fixes #2279, probably also #5051. I haven't checked.

I'm marking it as WIP, because I'm not too happy with how this turned out - emitting the metadata is easy; deciding what to emit is hard. Seeking some feedback.

I chose to use RD.XML as the mechanism to include the (otherwise unused) assemblies into the metadata. So the user has to put <Assembly Name="mscorlib" /> in their RD.XML to get forwarders from mscorlib. It's really not great.

The other problem is that mscorlib has a lot of garbage in it (like references to System.Security.Permissions). This code throws a lot.

@jkotas

jkotas commented Dec 2, 2017

Copy link
Copy Markdown
Member

like references to System.Security.Permissions

Why is System.Security.Permissions a problem? It should be in the framework set. Edit: I see that it is not there for some reason. It should be fine to add more package references next to https://github.com/dotnet/corert/blob/master/src/Framework/Framework.depproj#L28 if it helps.

probably also #5051

Would a better fix for this be to not throw from GetType when mscorlib does not exist? It should be just a matter of calling TryGetRuntimeAssembly instead of GetRuntimeAssembly.

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

Would a better fix for this be to not throw from GetType when mscorlib does not exist?

That depends. Would it be better if Type.GetType("System.Object") returns null, or throws an exception complaining about missing mscorlib if metadata for mscorlib wasn't included? Neither is great for System.Object, but the current behavior is a bit more diagnosable.

The problem is that Type.GetType needs to search current assembly, then the "core assembly", and only if the type can't be found in either, we return null. We were forced to use mscorlib facade as the core assembly for compat reasons (it used to be System.Private.CoreLib, but that didn't have things in it that people Type.GetType for).

@jkotas

jkotas commented Dec 2, 2017

Copy link
Copy Markdown
Member

All non-throwing metadata APIs return null for the tree-shaken types or methods. They do not try to "help" by throwing exceptions. I agree that it is not always easy to diagnose these. I do not see a good reason why this API should be on a different plan and try to "help".

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

I do not see a good reason why this API should be on a different plan and try to "help"

Typically if one gets this null returning behavior, the fix is to put the thing they're looking for in RD.XML (so in this case, as a user, I would expect adding <Type Name="System.Object" Dynamic="Required Public" /> will fix it). It will not fix this though because of obscure backcompat implementation details within the reflection stack.

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

As for the mechanism that decides what type forwards to include metadata for - how consistent are we with TypeForwardedFrom? Are we only doing it for types relevant to binary serialization? We could use that to include the type forward if metadata for the type itself is generated.

.NET Native for UWP apps currently uses similar heuristic ("if we're generating metadata for something, and there's a type forward for it somewhere within the inputs, generate the type forward too"). This has it's own set of problems too though (e.g. with this you can't really have a type forward within the app that forwards to a type implemented in the shared assembly, because shared assembly has the metadata and we don't look at type forwards for those types during app build). We've been getting away without it, but it's not great.

@jkotas

jkotas commented Dec 2, 2017

Copy link
Copy Markdown
Member

We've been getting away without it, but it's not great.

Agree. I think it is good enough to stick with it, at least for now.

For GetType, I think there are two case:

  1. The more compatible case: mscorlib is pulled in via rd.xml or other means. Adding "System.RandomTypeInMsCorlib" to rd.xml is alway going to work, regardless of whether the API throws or not. mscorlib is always going to be there, so it is never going to throw.
  2. The leaner case: mscorlib is not pulled in via rd.xml or other means. Adding "System.RandomTypeInMsCorLib" to rd.xml is not going to work regardless of whether the API throws or not. mscorlib is never going to be there, so it is always going to throw.

So I think changing the API to no throw for case 2 is more to enable us to experiment with leaner modes than to have much effect on self-diagnosability.

Manually parse the AssemblyReference record.
@MichalStrehovsky MichalStrehovsky changed the title [WIP] Emit metadata for type forwarders Emit metadata for type forwarders Dec 5, 2017
{
throw new NotSupportedException("Multi-module assemblies");
string simpleName = obj.Name;
return simpleName != null ? simpleName.GetHashCode() : 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: This could be written as return obj.Name?.GetHashCode() ?? 0.

@jkotas

jkotas commented Dec 7, 2017

Copy link
Copy Markdown
Member

@dotnet-bot test Windows_NT Release please

@MichalStrehovsky
MichalStrehovsky merged commit 7ee9fb3 into dotnet:master Dec 7, 2017
@MichalStrehovsky
MichalStrehovsky deleted the typeForwarders branch December 7, 2017 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants