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

API Change: Restore CompileToMethod in S.L.Expressions #20270

Closed
5 tasks
JonHanna opened this issue Feb 18, 2017 · 19 comments
Closed
5 tasks

API Change: Restore CompileToMethod in S.L.Expressions #20270

JonHanna opened this issue Feb 18, 2017 · 19 comments
Labels
area-System.Linq.Expressions untriaged New issue has not been triaged by the area owner
Milestone

Comments

@JonHanna
Copy link
Contributor

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
Copy link
Member

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

@weshaggard
Copy link
Member

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
Copy link
Member

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

In the method's signature?

@weshaggard
Copy link
Member

@marek-safar
Copy link
Contributor

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

@stephentoub
Copy link
Member

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

@danmoseley
Copy link
Member

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.

@danmoseley
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Interestingly, storing TypeBuilder constants with CompileToMethod() isn't quite as simple as it seemed when dealing with #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
Copy link

Any status update on this?

@JonHanna
Copy link
Contributor Author

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

@chkn
Copy link
Contributor

chkn commented Feb 26, 2019

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

@aelij
Copy link
Contributor

aelij commented Apr 9, 2019

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

@danmoseley
Copy link
Member

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.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@jaredpar
Copy link
Member

jaredpar commented Jul 8, 2020

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

@jaredpar jaredpar closed this as completed Jul 8, 2020
@ltrzesniewski
Copy link
Contributor

@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
Copy link

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!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Linq.Expressions untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests