Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Remove ArgIterator/TypedReference #20

Closed
weshaggard opened this issue Sep 14, 2016 · 21 comments
Closed

Remove ArgIterator/TypedReference #20

weshaggard opened this issue Sep 14, 2016 · 21 comments
Assignees
Labels
netstandard-api This tracks requests for standardizing APIs.

Comments

@weshaggard
Copy link
Member

Remove ArgIterator and TypedReference along with any __arglist overloads as they aren't easily supported on some platforms and there mostly only for managed c++ support which isn't really supported by our newer platforms.

@weshaggard weshaggard self-assigned this Sep 14, 2016
@weshaggard
Copy link
Member Author

Removed with 6e3ad13.

@terrajobst
Copy link
Member

terrajobst commented Jan 10, 2017

It looks like the managed linker might depend on TypedReference outside the C++/CLI and __arglist scenario.

@russellhadley @erozenfeld @swaroop-sridhar can you elaborate your dependency on TypedReference?

@terrajobst terrajobst reopened this Jan 10, 2017
@weshaggard weshaggard added the netstandard-api This tracks requests for standardizing APIs. label Jan 10, 2017
@weshaggard
Copy link
Member Author

I assume this related to https://github.com/dotnet/corefx/issues/14088. Is it only TypedReference that is needed?

cc @jkotas

@erozenfeld
Copy link
Member

It turns out that the managed linker doesn't have to depend on TypedReference so that shouldn't be considered a reason to bring TypedReference back. dotnet/corefx#14088 is a different motivation to bring it back.

@terrajobst
Copy link
Member

terrajobst commented Jan 11, 2017

I'd say that given we originally removed it due to dubious use we should now bring it back. First, API Port telemetry shows it's used in 2 percent of our reports, which is quite a bit considering the low-level nature of this type. Also, considering dotnet/corefx#14088 it seems to be general useful.

And on top, .NET Framework and Xamarin/Mono already have the type. I believe this is also true for CoreCLR. Not sure about CoreRT/MRT, but that's a different issue.

@weshaggard, any objections exposing this type?

@danmoseley
Copy link
Member

There is still time to do this for NS2.0 if it's mainly just exposing something. As Immo mentions of course it needs to work on MRT.

@jkotas
Copy link
Member

jkotas commented Jan 11, 2017

It looks like the managed linker might depend on TypedReference

We should fix the linker to not depend on it.

@jkotas jkotas closed this as completed Jan 11, 2017
@jkotas
Copy link
Member

jkotas commented Jan 11, 2017

First, API Port telemetry shows it's used in 2 percent of our reports, which is quite a bit considering

I missed this part. Yes - if you see this type being used a lot, I agree we should bring it back.

it needs to work on MRT

There is nothing fundamental that prevents it work on MRT. It is just some extra work in the NS2.0 bucket.

@jkotas jkotas reopened this Jan 11, 2017
@terrajobst
Copy link
Member

Excellent. So it seems we should expose it then.

@weshaggard, press the magic button 😄

@weshaggard
Copy link
Member Author

I will add both TypedReference and ArgIterator back to the standard.

@jkotas
Copy link
Member

jkotas commented Jan 16, 2017

TypedReference and ArgIterator back to the standard.

Sounds reasonable to me.

ArgIterator implementation in .NET Core will need codegen and runtime work, on Unix at least. The codegen teams will need to be involved in the implementation. It should throw PNSE until this work gets done.

@Thaina
Copy link

Thaina commented Jan 16, 2017

I have curios for such times that, how many platform derived from dotnet could use TypedReference and ArgIterator

Such as xamarin that compile to mobile platform, especially iOS which is prohibited runtime code generation. It can use Reflection but cannot use Reflection.Emit. Can it use TypedReference?

There are also webassembly that would parsed from llvm compiled C#. Are there limitation that make it not compatible?

I don't know where to ask this question, sorry if this is wrong place

@jkotas
Copy link
Member

jkotas commented Jan 16, 2017

There is nothing about TypedReference or ArgIterator that requires runtime code generation.

@Thaina
Copy link

Thaina commented Jan 16, 2017

@jkotas I mean there are many reason each platform cannot implement some feature. TypedReference used to be not supported on some platform in the past. I just curious that, even it was brought into standard. Are there platform that would not be able to use this feature in the future

@terrajobst
Copy link
Member

terrajobst commented Jan 16, 2017

Are there platform that would not be able to use this feature in the future

Potentially. We're walking a tight rope here. On the one hand side, we don't want to include large amounts of API into the .NET Standard that aren't/cannot be implemented everywhere. However, we also have to consider cases where if the producer of the .NET Standard library doesn't care about all the platforms but wants to share with a specific set of platforms that have this functionality. For larger components, such as registry, it makes sense to make the APIs available as additional packages that those projects can consume. But for tiny APIs like TypedReference and ArgIterator it's just not worth it.

Future platforms are likely forks of the existing open source stacks. Mono/Xamarin support both concepts. As @jkotas said, .NET Core's implementation for ArgIterator needs fixes to work on Unix. It's possible that we never add it, but I doubt that we'd go out of our way to remove these smaller features, unless we run into real issues.

@weshaggard
Copy link
Member Author

reverted with #162.

@weshaggard
Copy link
Member Author

After further discussion we decided to remove ArgIterator and the __arglist overloads from the standard because the amount of work to even make them throw PNSE isn't worth any tiny value it might provide. Removed with 52651e7.

@EgorBo
Copy link
Member

EgorBo commented Sep 3, 2018

@weshaggard EntityFramework6 uses String.Concat with __arglist overload:
https://github.com/aspnet/EntityFramework6/blob/master/src/EntityFramework/Core/Objects/ELinq/MethodCallTranslator.cs#L1493

if you run this on .NET Core you will get null

@jkotas
Copy link
Member

jkotas commented Sep 3, 2018

@EgorBo Is there a bug on this in EF6? We do not have plans to add cross-plat vararg support to .NET Core in near future. It would be best for EF6 to fix this.

@EgorBo
Copy link
Member

EgorBo commented Sep 3, 2018

@jkotas I don't think so, I am here just to leave a note 🙂 some of our users hit this issue with CoreFX String and EF6 packages on Mono: mono/mono#9996 (and the fix is mono/mono#10452 (review))

@jkotas
Copy link
Member

jkotas commented Sep 4, 2018

Opened dotnet/ef6#619

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
netstandard-api This tracks requests for standardizing APIs.
Projects
None yet
Development

No branches or pull requests

7 participants