-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Should there be a .Net Standard 2.0 version of Reflection.Emit? #26007
Comments
@ericstj @weshaggard do you know if it is on purpose this way, or an oversight? |
It was intentional. For libraries like this that have jagged support and no portable implementation we stopped shipping packages. The expectation is that you target a specific framework if you need it. For some we have brought back packages with land mines (throwing implementations on some platforms). Folks could propose we add it back but this one in particular has had some stong debate in the past due to lack of support on all AOT based .net frameworks. /cc @terrajobst |
How am I supposed to learn about that? As far as I can tell, Reflection.Emit is not reported by platform-compat. And when I google "reflection emit .Net Standard", the only result on the first page talking about whether you should use it that way is a tweet from July 2017 by @terrajobst suggesting that you should do it (maybe the situation changed since then?). |
I’ll let @terrajobst respond to that, perhaps he was thinking about the nuget gaurdrails feature we had in 1.x and the 1.x packages. With those we had jagged implementations and build time support for messaging that things aren’t supported. I’m not particularly leaning one way or the other here just explaining the way things are working at the moment. I think if we can get sign off from folks on adding back a package with a platform not supported implementation we could do so. FWIW anything that’s available in a framework but isn’t in netstandard or in a library/package with a netstandard asset means you need to target the framework to get it. That’s all I meant with my expectation comment. |
But here the package (with .Net Standard 1.x assets) does exist. I don't think you can just stop updating a package and assume people won't use it anymore. At the very least, you should have documentation explaining the situation. |
The biggest trouble with System.Reflection.Emit is that we don't have a way to provide the library in a netstandard way because of TypeInfo (see https://github.com/dotnet/corefx/issues/14334). The existing package is targeting netstandard1.1 but it is a lie and will only work on .NET Framework and .NET Core because of the tight coupling we have. We hacked it to work by playing InternalsVisibleTo tricks to give it access to TypeInfo's constructors, but that same trick is what makes it not work on a general .NET Standard platform. We decided to not propagate the error further by hacking netstandard2.0 in the same way and instead stopped producing the package and made it platform specific. Thanks for raising the issue @svick and we should use this issue to document the trouble with the package. |
What about unlisting the no longer updated packages on NuGet so it's clear they are not recommended? |
That is a reasonable suggestion. I just unlisted all the versions of https://www.nuget.org/packages/System.Reflection.Emit/ and https://www.nuget.org/packages/System.Reflection.Emit.Lightweight |
This is very needed. Not being able to build and save assemblies is currently my dotnet/corefx#1 biggest blocker for updating to .NET Core, and I'd really like to see getting this very fundamental API up to speed made a higher priority than working on new features. |
@masonwheeler just to call out you can use Reflection.Emit if you are writing .NET Core app or library you just cannot use it while writing .NET Standard library currently. |
@weshaggard are you sure? I just checked the repo, and it appears that not only is |
@masonwheeler you're correct that we don't support AssemblyBuilder.Save in .NET Core that is being tracked by another issue https://github.com/dotnet/corefx/issues/4491. |
There it is! I thought I had seen that issue before, but searching only turned up this one. :P But yeah. Without the ability to actually emit your results, you can't really say that you can use Reflection.Emit on Core. |
@weshaggard before the System.Reflection.Emit.Lightweight package being unlisted on nuget.org it was possible to get Selenium.WebDriver package in PowerShell 5 (for .Net Framework) and PowerShell Core 6 (for .Net Core 2.0) on Windows 10 using the following command: Install-Package Selenium.WebDriver -Destination $PSScriptRoot -Force -ForceBootstrap Now this command cannot download all the Selenium package dependencies for both PowerShell versions. It fails because System.Reflection.Emit.Lightweight.4.3.0 package is unlisted on nuget.org. Could you advise on how to fix this issue? |
@SergeyKhutornoy are you calling "Install-Package" in VS from the Package Manager Console? If so that correctly installs the package for me. If you are calling Install-Package from Powershell that is a different kind of package management. I actually just tried that locally and I get a different error (Install-Package : No match was found for the specified search criteria and package name 'Selenium.WebDriver'.) I'm not familiar with that package management system so I'm not sure how to work around it. One thing you could try is to explicitly install System.Reflection.Emit.Lightweight 4.3.0 first and then see if it works. If that doesn't work is there a reason you cannot use VS or nuget tools to install the package? |
Unlisting this package is a mistake, and not providing a .NET Standard 2.0 version is a mistake. We're about to ship a major new version of our product, NServiceBus, and we're targeting |
I don't mind not being able to save dynamic assemblies so much - I can always test the logic in a .NET Framework TFM - but |
Please bring back a way to reference |
How do we use DynamicMethod in .NET Core now that it is unlisted as a package? |
@danielcrenna You don't need the package to use |
I could build my runtime (which is released already) on .netstandard 2.0 using System.Reflection.Emit.Lightweight 4.3 as I use DynamicMethod. I now see the build process pulls the package from offline cache and not from nuget. Looking here: https://apisof.net/catalog/System.Reflection.Emit.DynamicMethod what's the conclusion I have to take? That it's in NS1.6? Or in some in-between dimension where 'platform extensions' are valid? So iow: As I target .netstandard2.0 and use DynamicMethod, I can make a reference to System.Reflection.Emit.Lightweight although it's unlisted, and supported in ns1.6 (?), but this feels really off: it feels as a liability as I now depend on a package that's unlisted (and I then have to hope unlisted packages are kept there till the end of time. ) Sad thing is: there's no alternative other than depending on an unlisted package that is only possible because one knows the exact name. // @terrajobst |
To add: EF Core 2.1 has a dependency on Castle.Core (https://www.nuget.org/packages/Castle.Core/) for its proxies, which depends on System.Reflection.Emit. Isn't it wise for all involved to have this package (and Emit.Lightweight) to simply be enlisted again? |
@FransBouma strictly speaking EF Core Proxies has the dependency, not EF Core itself. But it's a mess anyway. In fact FirebirdClient has dependency on System.Reflection.Emit. And what to do now, right? |
While it is understandable that full AOT platforms can't allow new types to be generated at runtime, it is more of a surprise that we can't support I am the author of the LightInject DI library and that is using Take a look here for the implementation. Note that I don't implement all the Opcodes. Only the ones I need. My point is that it should be possible to do this for all OpCodes and then enable A LOT of libraries to still target netstandard2.0. Most libraries that uses Reflection.Emit does not generate new types. They simply generate code through the DynamicMethod Also note that DynamicProxy, Moq and other libraries that generates types at runtime cannot target My two cents |
Is there a list somewhere of the TFMs that do support |
@jbogard All full framework TFM's should be good in addition to the netcoreapp TFM's. Is this in AutoMapper? |
This seems like a poorly thought out and communicated decision with far reaching consequences which appears to have been made with very little involvement from the community it impacts, surprising given the packages have 22M downloads. Doesn't that warrant some kind of analysis on the potential impact on everyone currently depending on it? ServiceStack references the now delisted Reflection.Emit packages in ServiceStack.Text which is the base dependency package used in effectively every ServiceStack NuGet package as well as several other NuGet packages using it as a dependency. We only provide So what's the recommendation now for packages using Reflection.Emit? Stop publishing .NET Standard 2.0 builds and tell everyone they can no longer create .NET Standard 2.0 builds for their libraries and projects? The previous solution for using .NET Standard APIs that don't implement the API was to throw |
@seesharper yes, I added those but that might leave out others. The API docs list more but what others might that be missing? Is that a comprehensive list of possible TFMs that support the API? What about, say, |
I've refactored my DynamicMethod/ILGenerator code which I used to emit setter methods for properties at runtime with a Lambda.Compile() solution so the dependency on Reflection.Emit is now gone, however I had to spend 3-4 hours on it which I would have liked to have spend on other things. But alas, that's life when things 'move fast and break often' I guess? Anyway, what I find a bit disturbing is the deafening silence from Microsoft personnel in this thread for the past weeks. Feels like debating things in a room when the people who actually can change things are not there. I totally agree with @mythz and others here, it's poorly communicated, and with 22M downloads this was a silly decision with far-reaching consequences. |
@mythz I don't know how you're using it, but for my stuff, I had to revert back to #if feature flags to remove functionality in platforms that don't support building types on the fly (in my library, being able to map to an interface and I create a proxy on the fly). |
Disagree. The major .NET Core and .NET Framework platforms support it, just because AOT environments don't shouldn't preclude it being able to support the major .NET Platforms targeting Has this now become the strategy for .NET Standard going forward? APIs not supported in AOT environments are now going to be removed? So there's going to be no more Supporting the intersection of the lowest common denominator APIs was what PCL did and resulted in a horrible development experience which forced the PCL bait and switch technique and delegating to multiple platform-specific implementations, which is all the investment we threw away when we moved to .NET Standard. The best solution was the existing one (esp. as removing it now is a disruptive and breaking change to its transitive deps) which lets us use Reflection.Emit in all platforms that supported it whilst needing to implement a fallback for those that don't. Removing the ability to use Reflection.Emit in .NET Standard means libraries that use it can no longer target .NET Standard. What is even the point if we can't use features that .NET Core and .NET Framework share in a platform-agnostic abstraction? It would just be adding unnecessary abstractions/confusion/complexity to the .NET ecosystem with no benefits. |
Thanks for the feedback everyone. Based on the overwhelming feedback we have relisted the latest version of the existing packages: https://www.nuget.org/packages/System.Reflection.Emit/4.3.0 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 if you are consuming them from a netstandard library expect that library to fail if it is ran on any other .NET Standard implementation. 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. |
It is my understanding that
IMHO, throwing This figure as it stands today is kind of a lie as I see it. Xamarin claims to support Continuing to publish |
They're not equivalent at all. The only reason why .NET Standard is useful is due to its vastly greater API surface which allows us to create a single build to run on multiple platforms. PCL's are far less useful as its reduced API intersection subset forced creation of multiple platform specific builds for non-trivial functionality. Their capabilities are not equivalent and the solutions they lead to are vastly different in complexity for both library authors and consumers, with .NET Standard we have a single build which runs on all platforms as we're able to check at runtime if the platform supports Reflection.Emit, if it doesn't we fallback to Lambda Expressions and Reflection. If these API's wasn't available to .NET Standard libraries we couldn't have a single build and would need to go back to developing and maintaining non-portable platform-specific builds.
Forcing the creation and dependence of platform-specific builds is a far worse outcome, esp. with so many transitive dependencies already dependent on it. The primary benefit of .NET Standard is to be able to create libraries and projects to target a single useful abstraction, if we can't use it to access mainstream features available in .NET Core and .NET Framework it fails to resolve its primary use-case and may as well not exist since it's just adding more abstractions/confusion to the .NET ecosystem without any benefits over PCLs. PCL's already tried binding to abstractions which only expose the intersection of API features available in each platform, which is effectively what you're asking to back to, since you'll only have access to APIs available in each platform and by extension only contain APIs available in the most restricted platform. Having a broader API Surface in .NET Standard is far more useful and gives library authors the freedom of being able to choose how they want to handle support for different platforms, if these APIs weren't available that option wouldn't exist, it would force platform-specific builds and force all its dependent libraries and projects to also maintain platform specific builds, adding friction, fragmentation, reducing portability and limiting support to only the platforms authors choose to maintain builds for vs all the platforms supporting .NET Standard that support that feature. It's not just a matter of preference, removing mainstream API support has far reaching disruptive impact on what solutions are available, forces unnecessary platform specific builds impacting the utility of libraries whilst breaking every .NET Standard library and project using them. |
This comment has been minimized.
This comment has been minimized.
@mythz you’re absolutely right. |
I completely understand that hiding the ref emit package is frustrating, considering the fall out that it caused. I also understand that us optimizing for execution environments that you don't care about can be upsetting, especially when this includes trade-offs that affect scenarios that you care about negatively. That being said, I've marked your comment as "abusive" as it's not constructive. Insulting people isn't going to get these issues addressed. Also consider that this isn't a policy we control. The fact that you cannot generate and execute code at runtime on iOS is a policy decision by Apple. Also, in some cases it's not a policy restriction but a technological one. There as three options with reflection emit:
Right now, we're leaning towards the first option but the second option is also on the table. |
I’m in favor of 2. It ubiquitous enough that it should be there. We need the capability check anyways for other reasons. |
So am I but it won't help anyone who is currently relying on .NET Standard 2.0 as this requires a new version of the standard. However, the APIs in question already exist. So I'm thinking about a minimal fix to make this packages work as-is but also fix .NET Standard 2.0 vNext. |
@terrajobst Is there something about option 1 that makes option 2 harder to do? It seems to me that getting a version of the packages that target So, unless there's a really good reason not to, I vote for both 1 and 2. |
Looks like we posted virtually at the same time. Hopefully my comment addresses your question :-) |
Please note that (Sorry, I don't know how many platforms support |
@glassgrass what frameworks do you imagine a I can certainly imagine some implementation that would write out the IL and produce an assembly like an API-driven ILASM, but I don't see current frameworks where I would want to use that. Frameworks that have a JIT (and support Assembly.Load) could also support Ref.Emit directly, and provide their Ref.Emit implementation instead of a netstandard2.0 version. I know this is the case at least desktop/.netcore/.netnative. The first two support ref.emit and provide an implementation, the later does not, nor does it have a JIT nor support dynamic assembly loading. I do think it would be an interesting project for someone to play around in corefxlab to write something on top of |
That's on my plate: (https://github.com/dotnet/corefx/issues/4491 and https://github.com/dotnet/corefx/issues/2800) https://github.com/dotnet/corefx/issues/2800 will come first as the ref emit stuff is likely to be an extension of it. I started prototyping 2800 this week and will be continuing that work in the months ahead. I'm not moving it to corefxlab until I'm much further along. Workflow is too inefficient there for my tastes. |
@atsushikan: Glad to see that there is still ongoing work on this issue. Thanks for tackling this. I've mentioned the following elsewhere previously: One thing that Of course one could only ever produce single-type assemblies, but this becomes potentially inefficient & difficult when making internals visible to the dynamically generated assemblies via |
@stakx You can emit |
@tmat: This is interesting. It's a pity this special attribute isn't officially documented, sort of makes it a bit of a gamble to use it outside the runtime. |
Would this be added to |
@karelz https://github.com/dotnet/corefx/issues/30654 tracks the work for 3.0 |
@joshfree is this one a dupe, or does it cover more? |
@karelz it covered the issue with the Ref.Emit package being incorrectly delisted from nuget and then the package being re-listed by @weshaggard. I think this issue can now be closed as the remaining work is tracked with dotnet/corefx#30654. |
OK, closing then :) ... the remaining work is tracked in dotnet/corefx#30654 as mentioned above. |
System.Reflection.Emit is not part of .Net Standard, but there are packages that let you use it from a .Net Standard library (specifically, System.Reflection.Emit and System.Reflection.Emit.Lightweight). But those packages do not have a .Net Standard 2.0 version, only .Net Standard 1.x versions.
This has some implications:
typeBuilder.CreateType()
(even though this code works both on .Net Framework 4.6.1 and .Net Core 2.0) and have to usetypeBuilder.CreateTypeInfo().AsType()
instead. It's possible there are also other APIs like this.These issues would be solved if a .Net Standard 2.0 version was added to Reflection.Emit packages. Is that something that would be worth doing?
Though both of these are fairly small quibbles, so I'm not sure how much value would doing this add.
The text was updated successfully, but these errors were encountered: