Runtime: API Change: Restore CompileToMethod in S.L.Expressions

[JonHanna]

At https://github.com/dotnet/corefx/issues/11408#issue-174928345 @bartdesmet notes:

I'm not clear why this method was removed. In fact, we tend to use it quite often (despite its limitations to emit only to static methods) in Bing, so I'd be interested in getting it back in .NET Core. All the code still seems to be there; just the entry-point was removed rather than conditionally excluded.

The entry-point came back conditionally excluded when that issue was closed. I also agree it can be useful, while I've only used it a handful of times myself, it was really useful those few times.

That Xamarin is now using this feature adds another advantage, in that we can't test it, and hence catch regressions that could break Xamarin, without it.

Mostly though I just think it can be useful, and is worth having.

CompileToMethod() has dependencies on most of what is covered by FEATURE_COMPILE so replacing FEATURE_COMPILE_TO_METHODBUILDER with FEATURE_COMPILE would bring it back in those builds that can feasibly use it (i.e. have support for Emit).

The mechanism for testing across both interpreter and compiler could be extended to test across all three possibities of interpreter, compiler and CompileToMethod() with dynamic methods, but other cases should probably be separate choices.

CompileToMethod() would require a separate set of tests.

• [ ] Approve change.
• [ ] Replace FEATURE_COMPILE_TO_METHODBUILDER with FEATURE_COMPILE
• [ ] Update TryEmitConstants to handle TypeBuilder constants when not emitting to DynamicMethod (may do this separately anyway if @marek-safar needs it).
• [ ] Update tests to include CompileToMethod() with DynamicMethods
• [ ] Add tests for compiling to static methods.

[stephentoub]

@danmosemsft, @weshaggard, @VSadov, any reason this hasn't come back for 2.0?

[weshaggard]

This method depends on types from Reflection.Emit which are not part of .NET Standard so it cannot be added to the standard. However it could be made .NET Core specific if we felt it was useful for those folks.

[stephentoub]

This method depends on types from Reflection.Emit which are not part of .NET Standard

In the method's signature?

[weshaggard]

Yes in the signature. See https://apisof.net/catalog/System.Linq.Expressions.LambdaExpression.CompileToMethod(MethodBuilder).

[marek-safar]

Right but it's available in .net core platform so it'd make sense to remove the ifdef

[stephentoub]

@danmosemsft, @VSadov, seems like this should come back for .NET Core 2.0?

[danmosemsft]

In general @ViktorHofer and I did not chase up missing members where they were specifically excluded from a type as part of defining .NET Standard, based on the assumption that bringing them back even for Core would cause a library in .NET Standard to take a reference outside of .NET Standard. However as I now understand, this was too strict as there's no reason why .NET Core can't have such a reference as it doesn't attempt to offer a standalone implementation for .NET Standard (nor one that could be trimmed to just that).

This is a list of many of the members we ignored for this reason. Perhaps others should be reexamined.

So I believe this could come back. Whether it should is up to @VSadov @OmarTawfik - of course I think it ideally should.

[danmosemsft]

Here is a better list -- this is everything (?) it is currently not planned to bring to Core, but exists in Desktop (by some definition). Of course it doesn't describe stuff that isn't implemented, but API is present.

https://github.com/dotnet/corefx/blob/master/src/shims/ApiCompatBaseline.netcoreapp.netfx461.ignore.txt

[ViktorHofer]

Nearly everything. There are still a few open issues where I'm awaiting responses if the members should make it in before 2.0 signoff.

[JonHanna]

The mechanism for testing across both interpreter and compiler could be extended to test across all three possibities of interpreter, compiler and CompileToMethod() with dynamic methods, but other cases should probably be separate choices.

I did some experimenting with this last night. A problem with this idea is that a very large number of the tests make use of a constant value of a type CompileToMethod() can't cope with, and rewriting the tests to cover the same cases but constructing the values within the expression tree would be both an arduous rewrite, and result in tests that took longer to execute.

Since the majority of paths are the same with IL compilation and CompileToMethod I think we would have to keep the majority of tests as they are, and add tests for those cases where CompileToMethod differs (mostly the very difference in constant handling that interferes with just replaying the bulk of the tests).

[JonHanna]

Interestingly, storing TypeBuilder constants with CompileToMethod() isn't quite as simple as it seemed when dealing with dotnet/runtime#20124.

To recap, interpretation can handle those fine, compiling to IL could not (and cannot on desktop, still) but now can because it does so as a bound constant (an approach unavailable to CompileToMethod()).

It had seemed at the time that CompileToMethod() could handle them, but that isn't quite the case.

CompileToMethod() can compile a TypeBuilder only if it's the TypeBuilder of the MethodBuilder it's writing (TypeLoadException otherwise). Also, unless the type of the ConstantExpression isn't TypeBuilder but a wider type (TypeInfo, Type, MemberInfo or object) it will fail on execution with an InvalidCastException because the constant becomes the RuntimeType that the TypeBuilder builds. If CompileToMethod() returns it should probably handle the failure cases better. In particular that InvalidCastException is one where a different error could solve a confused developer some time in getting to what they really need.

[chris-rogala]

Any status update on this?

[JonHanna]

I'm on a long-term hiatus from contributing, so I won't be experimenting further in the near future.

[chkn]

Is this something that should perhaps be revisited now that .NET Standard 2.1 will contain Reflection.Emit?

[aelij]

@danmosemsft Could it be added in .NET Core 3.x? I can submit a PR.

[danmosemsft]

I actually don't have context. My comments above were from large scale ports to .NEt Core. This component is owned by @cston @333fred @jaredpar (per https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/issue-guide.md)

Per https://github.com/dotnet/corefx/issues/33170 they are limiting changes to this library. However bringing back a method that was previously present in desktop may fall under the "regressions" category described in that issue. They will have to make this call.

[jaredpar]

Closing as we are not accepting new features, including bringing back old APIs, into the library at this time as it is effectively archived. More information is contained in the library README.md

[ltrzesniewski]

@jaredpar could you reconsider it for this particular feature, please?

All of the code is already there but disabled behind a FEATURE_COMPILE_TO_METHODBUILDER flag, and is missing when you migrate a project from .NET Framework to .NET Core. I think it wouldn't really be a risk to enable it: since it has been working on the .NET Framework, it's not exactly a "new" feature.

[EdinaLewis]

This missing feature has been the primary reason for my team not adopting .NET Core. Am I to understand then that this will not be part of .Net 5 either?

Please reconsider!