Runtime: Fix Ref.Emit contract to throw PNSE on AOT

[joshfree]

Currently the System.Reflection.Emit package that shipped with .NET Core 1.0 incorrectly states it supports NETStandard 1.1+ when in fact it does not work on AOT platforms (it only works on .NET Core and .NET Framework platforms) https://github.com/dotnet/corefx/issues/29365#issuecomment-398130364. We need to fix the following in the package so that it's no longer a lie and so developers who use the package understand where the sharp edges are.

• [x] 1. Build as a netstandard 2.0 package. (reference assembly will build against netstandard.dll)
• [x] 2. Ensure TypeBuilder inherits from Type. This is a small breaking change that allows us to build the reference assembly using the normal build path despite TypeInfo's constructor not being public out of netstandard.dll.
• [x] 3. Because of this breaking change, we must also bump the contract's major version number.
• [x] 4. Make sure APIs correctly throw PNSE for AOT platforms that do not support Emit such as Unity, Xamarin/mono, UWP, UWP-AOT, etc
• [ ] 5. Update the package description to document the PNSEs on AOT platforms
• [ ] 6. While we're working on this contract, diff the existing contracts surface area with .NET Framework Ref.Emit to see if additional APIs should be added to the contract (ex https://github.com/dotnet/corefx/issues/30355)

/cc @weshaggard @terrajobst @jkotas @AtsushiKan @joperezr

[joperezr]

Unassigning from me since my part of the work was finished by @ericstj here: https://github.com/dotnet/corefx/pull/30741

[ghost]

Paging this issue back into memory: we're building a PNSE assembly for AOT now there's nothing else that has to be done.

Given that System.Private.CoreLib also includes these same types (that throw PNSE), we could opportunistically turn the AOT configuration back into a façade (just to avoid having two copies of these types.) But I don't think there's any urgency on this.

[joshfree]

@steveharter we should get this into 3.0 soon to ensure there's no unintended fallout from this change.

[steveharter]

I added checkboxes to the original description.

Ensure TypeBuilder inherits from Type. This is a small breaking change that allows us to build the reference assembly using the normal build path despite TypeInfo's constructor not being public out of netstandard.dll.

Currently TypeBuilder does inherit from Type in the contract\ref, but not the implementation (it inherits from TypeInfo).

Because of this breaking change, we must also bump the contract's major version number.

There is no breaking change (TypeBuilder inherits from Type)

Make sure APIs correctly throw PNSE for AOT platforms that do not support Emit such as Unity, Xamarin/mono, UWP, UWP-AOT, etc

Over time any dependent frameworks\platforms will take on the newer bits and should have the correct semantics. However, for the shorter term and\or for those that are using older or servicing branches we need to determine if we need to update the servicing branch(es).

[steveharter]

@zamont @tommcdon from UWP side, do you want an updated service branch(es) that would contain the "correct" semantics (throwing PNSE) for System.Reflection.Emit?

[tommcdon]

@steveharter For UWP no need to backport the correctness fix.

[steveharter]

@tommcdon I assume we also want to throw PNSE for System.Reflection.Emit.dll in both UWP debug mode and release (UWP-AOT) mode. Note that today debug mode works.

[tommcdon]

@steveharter Correct - we should try to provide a consistent experience between Debug/Release mode debugging

[jkotas]

throw PNSE for System.Reflection.Emit.dll in both UWP debug mode

That does not work. Number of other libraries (like serializers) are implemented differently for regular (debug) vs. aot (release) flavor. The regular debug flavor uses Reflection.Emit. If you start throwing from Reflection.Emit contracts in debug, these libraries will break.

[steveharter]

It is unfortunate that there are dependencies on the debug\non-aot semantics. I was planning to add TargetsUap to the S.R.Emit.csproj and S.R.Emit.Lightweight.csproj to generate the PNSE, but now will hold off unless I hear otherwise.

[jkotas]

I think this issue can be closed.

[steveharter]

I think this issue can be closed.

Closing. One of the other items listed was to "Update the package description to document the PNSEs on AOT platforms" however we don't do that on any other packages that I saw.

[stakx]

@steveharter: Sorry for disturbing the peace of this closed issue, but could you please clarify something about the following:

[X] 1. Build as a netstandard 2.0 package. (reference assembly will build against netstandard.dll)

When I install the S.R.E package (4.6.0-preview3.19128.7) into a a netstandard2.0 library project, one of the things that get installed along with it is Microsoft.NETCore.Targets. I'm a little worried about seeing the ".NET Core" bit there: There's no guarantee that users of my library are going to target .NET Core. What is the meaning and what are the implications of that dependency?

[steveharter]

@stakx I was unable to repro the Microsoft.NETCore.Targets file getting created. Where was it created?

However I do see a hidden [myprojectname].csproj.nuget.g.targets get created in the \obj folder. I'm using preview version of SDK (3.0.100-preview-009812)

[stakx]

@steveharter: The name I mentioned doesn't refer to an MSBuild .targets file, but to a NuGet package dependency.

[ericstj]

Microsoft.NETCore.Targets is a transitive dependency of Ref.Emit It's actually not for .NETCore, it was just the common naming we were using back in 1.0. Back then it contained the "targets" supported by nuget-based portable libraries. These implemented the "supports" checks done by nuget, buy describing what tfm and RID set a support clause mapped to. You can see that content here: https://github.com/dotnet/corefx/blob/master/pkg/Microsoft.NETCore.Targets/runtime.json.

It's harmless and can be ignored. It contributes nothing to the build and is only used by nuget during restore if you happen to use supports clauses in a project.json. I don't even think that concept survived to the MSBuild representation of packagereference.

[stakx]

@ericstj: Thank you for the explanation!

(It might be good to rename that package then, so things look a little less suspicious from the end user's perspective.)

[ericstj]

I wish we could. Unfortunately NuGet doesn't provide a way to rename a package (you cannot declare one package as replacing the old package) otherwise we would have done this. Here's an ask for this feature from NuGet: https://github.com/aspnet/AspNetCore/issues/1222,

We're really only bringing the reference along to replace the old 1.0 version should it be pulled into the dependency graph. Back in 1.0 this package also contained the runtime-package-associations for our RID split packages, those are problematic since they would bring in the 1.0 runtime packages. More info here: https://github.com/dotnet/corefx/issues/18088.