castleproject / Core

Castle Core, including Castle DynamicProxy, Logging Services and DictionaryAdapter
http://www.castleproject.org/
Other
2.2k stars 468 forks source link

System.Reflection.Emit package has been delisted #374

Closed jonorossi closed 5 years ago

jonorossi commented 6 years ago

See https://github.com/dotnet/corefx/issues/29365#issuecomment-385102487.

According to that thread the package was never cleanly implemented as a .NET Standard 1.x package and had a hack that it could see the internals of the runtime's classes, it was then recently decided for .NET Standard 2.0 not to continue that hack (because CoreRT doesn't have the hack). No .NET Standard 2.x package was published and recently on the 2018-04-28 System.Reflection.Emit and System.Reflection.Emit.Lightweight were delisted on NuGet.org.

That now leaves us using a delisted package while targeting .NET Standard 1.3 and 1.5. We need to follow this thread, however it likely means we'll need to drop our .NET Standard 1.3 and 1.5 targets and move to netcoreappx.x (.NET Core) targets, obviously resulting in our downstream users doing the same. The .NET Framework targets (net35;net40;net45) wouldn't change.

@stakx is this your understanding?

stakx commented 6 years ago

[It] likely means we'll need to drop our .NET Standard 1.3 and 1.5 targets and move to netcoreappx.x (.NET Core) targets, obviously resulting in our downstream users doing the same. The .NET Framework targets (net35;net40;net45) wouldn't change.

Yes, @jonorossi, I'm drawing the same conclusion.

Reflection.Emit has always been a problematic part of .NET Standard because some platforms don't actually implement that facility while still nominally declaring .NET Standard compatibility. So your .NET Standard library ends up getting bug reports saying that it doesn't work properly—but there's nothing you can do about it. (Been there.)

Reflection.Emit might still be brought back to .NET Standard. But even then, Reflection.Emit will simply throw NotSupportedException or NotImplementedException on those platforms. So we'd still be pretending to support platforms that we cannot in fact run on.

This is one of several ways in which .NET Standard is a fundamentally broken standard, and I'm personally much in favor of abandoning it and targeting instead the actual platforms that really do support Reflection.Emit. It's simply the most transparent and accurate thing to do, and also what Microsoft appears to recommend these days.

That said, it would be difficult to get downstream libraries to abandon .NET Standard because they'd likely have to carry the burden, i.e. to face a public outcry from their user base.

In closing, it might be best to simply keep using the delisted package until things start breaking because of that. Once that point is reached, we could still make the switch over to netcoreappX.Y.

zvirja commented 6 years ago

Just to clarify the things, Reflection.Emit support is not limited by .NET Framework and .NET Core:

So by targeting the platforms directly you might abandon a set of less popular platforms where everything works fine currently. Will you target the exotic runtimes like Xamarin.Android? ๐Ÿ˜‰

stakx commented 6 years ago

@zvirja - I've been thinking about the same thing, too. It's not just Mono, .NET Framework and .NET Core. And my (our?) relative lack of knowledge about these other runtimes becomes a problem if we choose to abandon .NET Standard and target the runtimes directly. We'd need to do some research and get to know those others better.

But, if it's true that the delisted System.Reflection.Emit package works only because runtime internals are made visible to it (by means of [InternalsVisibleTo] in the runtime), then doesn't that mean that the package is tied to a particular set of runtimes already? This warrants some more research, but it's seems quite probable that this package probably doesn't work on those other runtimes already, so we wouldn't lose anything by not targeting some of them.

But yeah. Basically, you're right.

zvirja commented 6 years ago

But, if it's true that the delisted System.Reflection.Emit package works only because runtime internals are made visible to it

Well, personally I don't care about that. From client perspective there is no difference whether package implements all the logic on its own or with cooperation with particular runtime - the result is that package works on all the possible runtimes. And it's much easier to support the code - target .NET Standard, rather than install a ton of different noname SDKs to support them too ๐Ÿ˜Ÿ

But let's see how it goes - definitely the package existence decision is not up to us.

ghost commented 6 years ago

/cc @terrajobst

stakx commented 6 years ago

(A small btw. David Fowler tweeted the following a little earlier today:)

I know we said target The lowest netstandard but we really want everyone to re-baseline at netstandard 2.0. At the very least add a netstandard 2.0 target. [...] The advantage is that the dependency graph is extremely simplified. The packages that all exist for netstandard 1.0 are effectively frozen and weโ€™d like to push the ecosystem off them

jbogard commented 6 years ago

@zvirja do you have a comprehensive list? I see the docs here:

https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder?view=netframework-4.7.2#applies-to

But there's a long list of other TFMs that Xamarin have and I really really don't want to check ALL of them.

FransBouma commented 6 years ago

@zvirja you're sure UWP supports Reflection.Emit? As UWP uses AOT, so it can't generate code at runtime. The 'best' way it can e.g. support lambda.compile is through interpreters, so I doubt it can support DynamicMethod/ILGenerator, simply because there's no clr/jit, things are compiled ahead of time. But perhaps I missed something ;)

Pinox commented 6 years ago

@FransBouma @zvirja

Reflection.Emit already throws errors in all my projects in Release Mode (UWP): https://github.com/dotnet/corefx/issues/30127

zvirja commented 6 years ago

@jbogard No, I don't ๐Ÿ˜Ÿ I briefly skimmed through the list here and tried to google which ones might support this feature.

@FransBouma No, I'm not sure about UWP support and my reply is mostly based on this SO question from which it looks like it works at least in Debug mode. Most likely it's indeed not supposed to work in Release mode as you correctly pointed out to. However, it's still a question whether this project should abandon UWP even if it's supported in Debug mode only. You might still want to use Moq or NSubstitute when writing tests for your UWP app and Debug mode is absolutely fine to you.

If Microsoft doesn't get back the package, then I'm 99% confident this project (and my other down the stream) will be forced to say sorry for the non-popular platforms (basically everything except .NET Framework and .NET Core) and ask clients to put their business logic into netstandard compliant assemblies. This way that can continue to use mocking libraries by targeting netcoreapp by their test project. I don't find it realistic to support Xamarin and UWP in debug mode - it's veery unlikely guys have any experience with them and their ecosystem ๐Ÿ˜– Also it will be a hell for contributors to setup the dev environment locally.

stakx commented 6 years ago

I can see why Microsoft wouldn't want Reflection.Emit in .NET Standard—it's for the same reason they haven't (yet?) added .NET Core's Span APIs. Once some API is in .NET Standard version X, all platforms will have to implement it, or otherwise they'll never be able to advance to .NET Standard version X+1. We know that there's several platforms that do in fact not support Reflection.Emit (UWP and mobile device platforms I think). So if MSFT wants those to be able to keep up with newer .NET Standard versions there's only two choices: (1) Keep roadblocks like Reflection.Emit out of the Standard; or (2) include it but make it crash with NotSupportedException. Solution (2) was previously used for a few singular methods, but AFAIK not for a whole facility encompassing dozens of types and hundreds of methods. So I guess they tend towards (1) for that reason.

A list of platforms having real, non-interpreted support for Reflection.Emit would really come in handy.

ghost commented 6 years ago

I am kind of hoping someone from Microsoft can help us clear this up. @terrajobst, @davidfowl?

stakx commented 6 years ago

News from https://github.com/dotnet/corefx/issues/29365#issuecomment-398130364:

Based on the overwhelming feedback we have relisted the latest version of the existing packages: [...] However as mentioned earlier in this thread these packages claim to be netstandard1.1 compatible but that is a lie. They will only work on .NET Core and .NET Framework.

(So, fundamentally, nothing really has changed. :wink:)

We still don't have a great solution for supporting these on netstandard2.0 but the owners (AtsushiKan joshfree) of System.Relfection.Emit will be trying to figure out the longer term solution.

That's probably as good as it's going to get at this point. Good to know someone's on it.

ghost commented 6 years ago

Sure but they probably have their hands full. We can't be blasting them with comms? Or should we?

davidfowl commented 6 years ago

We're aware and are actively working on a solution to the problem.

kzu commented 6 years ago

FWIW, Mono, Xamarin.Mac and Xamarin.Android should all work just fine if the package targets .net45 (or even .net461).

bloritsch commented 6 years ago

Any thoughts on replacing Reflection.Emit with Roslyn? Roslyn was designed for the Net Standard/Net Core world and is integral to a number of rendering engines (like Razor). I went that way with DHaven.Faux, and it works without problem. I did find it easier to generate the class in C# code and then have Roslyn parse that.

At any rate, my guess is that the dotnet team are pushing people in that direction.

ghost commented 6 years ago

@bloritsch

Can you "unpack" your statement. You are talking about a re-write of Castle.Core.

bloritsch commented 6 years ago

I understand that. However it seems perfectly clear that Reflection.Emit is on it's way out. The only other option I know of is Roslyn which is natively NetStandard. As some point APIs die (i.e. are no longer supported).

Microsoft has been known to not finish APIs when they have something to replace them. You can see that with the transition from WinForms to WPF to Silverlight to UWP. The same thing is happening with APIs from the standard framework to net core and net standard. For better or worse the decisions on what APIs are being used in Net Core and Net Standard are driven by UWP and ASP.Net MVC. Both of those have moved toward using Roslyn for their dynamic content needs.

Unfortunately I don't know how pervasive Reflection.Emit is used within Castle.Core. I do know that with Castle.Core being a library it would be a shame if everything that uses Castle.Core had to explicitly be netcore because that's what this library was compiled against. I routinely have to support code that is shared between legacy framework and netcore.

So my question is really one of if anyone even considered the possibility of moving to a new way of generating your dynamic proxies as the existing API being used is languishing in support?

ghost commented 6 years ago

Yes, this is one way.

We could also use:

jbogard commented 6 years ago

Thatโ€™s not at all clear. Itโ€™s on the list for the next version of netstandard.

On Wed, Aug 1, 2018 at 7:55 PM Berin Loritsch notifications@github.com wrote:

I understand that. However it seems perfectly clear that Reflection.Emit is on it's way out. The only other option I know of is Roslyn which is natively NetStandard. As some point APIs die (i.e. are no longer supported).

Microsoft has been known to not finish APIs when they have something to replace them. You can see that with the transition from WinForms to WPF to Silverlight to UWP. The same thing is happening with APIs from the standard framework to net core and net standard. For better or worse the decisions on what APIs are being used in Net Core and Net Standard are driven by UWP and ASP.Net MVC. Both of those have moved toward using Roslyn for their dynamic content needs.

Unfortunately I don't know how pervasive Reflection.Emit is used within Castle.Core. I do know that with Castle.Core being a library it would be a shame if everything that uses Castle.Core had to explicitly be netcore because that's what this library was compiled against. I routinely have to support code that is shared between legacy framework and netcore.

So my question is really one of if anyone even considered the possibility of moving to a new way of generating your dynamic proxies as the existing API being used is languishing in support?

โ€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/castleproject/Core/issues/374#issuecomment-409770836, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMmEBaPr6-m-xlWESmAl_tcImArBvks5uMk3ngaJpZM4UmY_l .

jonorossi commented 6 years ago

Any thoughts on replacing Reflection.Emit with Roslyn? Roslyn was designed for the Net Standard/Net Core world and is integral to a number of rendering engines (like Razor). I went that way with DHaven.Faux, and it works without problem. I did find it easier to generate the class in C# code and then have Roslyn parse that.

At any rate, my guess is that the dotnet team are pushing people in that direction.

I understand that. However it seems perfectly clear that Reflection.Emit is on it's way out. The only other option I know of is Roslyn which is natively NetStandard. As some point APIs die (i.e. are no longer supported). [..] Unfortunately I don't know how pervasive Reflection.Emit is used within Castle.Core. I do know that with Castle.Core being a library it would be a shame if everything that uses Castle.Core had to explicitly be netcore because that's what this library was compiled against. I routinely have to support code that is shared between legacy framework and netcore.

So my question is really one of if anyone even considered the possibility of moving to a new way of generating your dynamic proxies as the existing API being used is languishing in support?

@bloritsch As I recall Roslyn actually predates .NET Core/Standard by many years, it wasn't designed for it, that aside yes at least I have thought about it but Castle DynamicProxy's API is designed so you don't need to know upfront which types you want to proxy, and it would be a completely different library without the ability to add new proxy types to the loaded assembly, something I'm not interested in pursuing at the moment.

I suggest you take a read of https://github.com/dotnet/standard/pull/829, Microsoft has made no indication of deprecating reflection emit and I've never got the feeling that it is on the way out, they are discussing whether it goes into .NET Standard which is a completely separate beast to runtime support as .NET Standard becomes the minimum all .NET runtimes must implement.

For everyone if it wasn't obvious, my plan here is to wait to see the results of the .NET Standard proposal and make our decision after that decision has been made by the .NET Standard Board. If it goes in, great we can continuing using .NET Standard, otherwise we'll just have to target each runtime directly.

jonorossi commented 6 years ago

https://docs.microsoft.com/en-us/dotnet/framework/unmanaged-api/profiling/icorprofilerfunctioncontrol-setilfunctionbody-method. Admittedly the hard way, but the way all tooling has gone before us visa vi RedGate, JetBrains, New Relic et al.

@fir3pho3nixx really not sure how this is an option, if you meant the whole Profiling API fair enough. In my opinion this isn't really an option because a runtime instrumenter won't fly with users as I know people have a hard enough time getting PostSharp added to a build process, something hooking in the runtime at that level won't be fun for us or users.

Mono cecil(debugger for mono), if only that was runtime based.

Cecil isn't a debugger, just a library for reading and writing PE files containing IL, others have existed before it like PERWAPI. There is also System.Reflection.Metadata which is part of corefx that is aiming to do the same, however as you know doesn't currently work with the runtime internals, but Microsoft is keen for this to be a supported scenerio, however just not with the current .NET Framework implementation. My understanding of comments from the Microsoft team in https://github.com/dotnet/corefx/issues/4491 is that corefx they want to be able to do reflection emit without the implementation living in coreclr, i.e. expose the right entry points to ask the runtime to update an assembly it has loaded. This obviously isn't trivial and will take time to mature bits and pieces.

stakx commented 6 years ago

@bloritsch:

Any thoughts on replacing Reflection.Emit with Roslyn?

Like it has been said above, this would mean a complete rewrite. Another project that's going this way is Moq. The upcoming next major version of Moq will no longer be based on Castle DynamicProxy because of its heavy reliance on Reflection Emit, which limits adoption on e.g. mobile platforms. Moq 5 includes a library called "Stunts", which is essentially the Roslyn-based replacement for DynamicProxy.

AFAIK, the split of Moq into Moq and such an "SDK" library was made with the thought that other mocking libraries might also want to use it.

However it seems perfectly clear that Reflection.Emit is on it's way out.

Not so clear at all. I do agree though that the Reflection Emit API increasingly feels as if it were being left behind, similar to LINQ expression trees. Perhaps understandable since every innovation in the C# language and/or the CLR has to be reflected in this API, which cannot be easily changed due to its complexity (at least that's my impression, take a look at its source code), so there's this time lag during which Reflection Emit feels outdated.

Note also that the CLR machinery on which Reflection Emit is based will actually play a fairly important part for a feature that's currently in the making: collectible assemblies (issue here, PR here).

My personal conclusion at this time is this:

ghost commented 6 years ago

@fir3pho3nixx really not sure how this is an option, if you meant the whole Profiling API fair enough. In my opinion this isn't really an option because a runtime instrumenter won't fly with users as I know people have a hard enough time getting PostSharp added to a build process, something hooking in the runtime at that level won't be fun for us or users.

@jonorossi Yup ๐Ÿ˜ธ. Why I said " Admittedly the hard way". I have written one of those before. Not pretty.

ghost commented 6 years ago

Cecil isn't a debugger, just a library for reading and writing PE files containing IL, others have existed before it like PERWAPI. There is also System.Reflection.Metadata which is part of corefx that is aiming to do the same, however as you know doesn't currently work with the runtime internals, but Microsoft is keen for this to be a supported scenerio, however just not with the current .NET Framework implementation. My understanding of comments from the Microsoft team in dotnet/corefx#4491 is that corefx they want to be able to do reflection emit without the implementation living in coreclr, i.e. expose the right entry points to ask the runtime to update an assembly it has loaded. This obviously isn't trivial and will take time to mature bits and pieces.

@jonorossi

Correct, however if you dig deeper into Mono.Cecil usage you will notice it is part of the debug stack for Mono. Anyway, whatever the hook is, where it is, MS have to figure this out. I think we all agree that something in dotnet would be nicer.

@davidfowl

If a rewrite of DynamicProxy were to occur, it should perhaps be on top of System.Reflection.Metadata

How are you guys going with this? Is this assumption correct?

stakx commented 5 years ago

my plan here is to wait to see the results of the .NET Standard proposal and make our decision after that decision has been made by the .NET Standard Board. If it goes in, great we can continuing using .NET Standard, otherwise we'll just have to target each runtime directly.

@jonorossi, I suppose we can close this issue, then? By now, it seems safe to say that System.Reflection.Emit will be part of netstandard2.1 (and that we will keep using it).

Additionally, we have some overlap between the present issue and #407, it would probably be easier to limit the discussion about future supported target frameworks to a single issue. #407 seems like the better place for that, judging by the two issues' titles.

jonorossi commented 5 years ago

Yep, #407 sounds like the place to continue discussion, and the issue here was just about .NET Standard 1.x support being broken by the package being delisted.