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

[Umbrella] ValueTuple library work #13177

Closed
jcouv opened this issue Aug 15, 2016 · 14 comments
Closed

[Umbrella] ValueTuple library work #13177

jcouv opened this issue Aug 15, 2016 · 14 comments

Comments

@jcouv
Copy link
Member

@jcouv jcouv commented Aug 15, 2016

Documentation of the various slices in the ValueTuple package:

Targeting pack Contents
netcoreapp1.1 Ref and lib assemblies type-forwards to System.Runtime ref assembly, which type-forwards to System.Private.Corelib (from CoreCLR or CoreRT).
netstandard1.0 and up Assembly implements ValueTuple and depends on System.Runtime
netstandard2.0 and up Relies on netstandard package (requires binding redirect)
PCL Assembly implements ValueTuple, but depends on mscorlib. Note that the precedence rules on targets means that PCL is picked when targeting desktop 4.5.
desktop 4.7 type-forwards ValueTuple to mscorlib

Known issues

ValueTuple nupkg and net47

Most prominent issue is when targeting desktop 4.6 (or lower) and referencing the ValueTuple package, then running on machine with desktop 4.7 installed.
The symptom are that tuples don't work in EE, interactive and possibly ASP.NET.
Workaround options: (1) target 4.7, or (2) remove ValueTuple dependency, or (3) run without 4.7 installed.
See https://github.com/dotnet/corefx/issues/16195 for more details.

ValueTuple in netstandard and net47

ValueTuple in netstandard and net471

Applications targeting .NET 4.6.1 that use .NET Standard libraries might be broken when running on .NET 4.7.1

microsoft/dotnet-framework-early-access#9


Reported issues


Work items

Adding System.ValueTuple to various corlib implementations:

Add ITuple to System.Tuple and System.ValueTuple to those corlibs:

After that:

Make System.ValueTuple binary serializable (https://github.com/dotnet/corefx/issues/15229) (which will ship together with ITuple above):

  • mscorlib/desktop with tests (work item, changeset)
  • corlib/coreCLR (dotnet/coreclr#9271)
  • in coreFX, update ref assembly (add serializable attribute) and add tests to cover coreCLR implementation
  • corlib/coreRT (dotnet/corert#2644)
  • mscorlib/xamarin (I think nothing to be done)
  • Add compiler end-to-end tests (with tuple syntax)

Hash seed randomization:

  • desktop (could be considered)
  • coreFX (done with Guid)
  • coreCLR (done with Random)
  • coreRT (done with Random)

EqualityComparer optimization:

  • desktop (N/A, the underlying runtime does not have the optimization)
  • coreFX (N/A, cannot be done because that ValueTuple code can run on existing runtimes that don't have the underlying optimization)
  • coreCLR (done)
  • coreRT (dotnet/corert#3021)

Various BCL improvements:

Downgrade warnings issue:
- [ ] Change dependencies of ValueTuple package to reference 1.0.0 baseline (issue, fix) (closed "by design", customer should upgrade their 1.0 projects to 1.1 to use ValueTuple).

EE bug with not loading mscorlib

  • Fix in 15.3 and ported to 15.2 (#18552)

Clash between ValueTuple.dll and mscorlib.dll from new desktop framework (in short, a workaround was added to EE and a facade assembly was added to mscorlib in desktop 4.7.1 that type unifies with ValueTuple assembly version 4.1.*):

Binding redirects required for EF tests (julielerman/EFCore2NightlyNet461#1, ErikEJ/EntityFramework.SqlServerCompact#463)

But auto-generated binding redirects break type unification (microsoft/dotnet-framework-early-access#9)

Various bugs/proposals:

Change behavior when comparing with null (https://github.com/dotnet/corefx/issues/18528) (we ended up cancelling to keep Tuple and ValueTuple APIs more stable)

  • In Core 2.0 (PR dotnet/coreclr#11345)
  • In desktop (version after 4.7.1, to give more bake time and not require quirking)
  • ValueTuple package (it will be left as-is)
  • Mono?
@jcouv jcouv added this to the 2.0 (Preview 5) milestone Aug 15, 2016
@jcouv jcouv self-assigned this Aug 15, 2016
@jcouv jcouv modified the milestones: 2.0 (RC), 2.0 (Preview 5) Sep 8, 2016
@jcouv
Copy link
Member Author

@jcouv jcouv commented Sep 16, 2016

Added ValueTuple code and test code for mscorlib/desktop (changeset, bug)
Still need to enable maddog

@jcouv
Copy link
Member Author

@jcouv jcouv commented Oct 12, 2016

Marek added code and test for mscorlib/xamarin (mono/mono@0a515cf)

It will be part of Xamarin Cycle10 release which is expected to be in line with VS Jan/Feb release.

@jcouv jcouv modified the milestones: 2.0 (RTM), 2.0 (RC) Oct 13, 2016
@jcouv jcouv modified the milestones: 2.0 (RC.3), 2.0 (RTM) Nov 23, 2016
@jcouv jcouv added the 3 - Working label Nov 24, 2016
@jcouv
Copy link
Member Author

@jcouv jcouv commented Dec 6, 2016

Added ITuple to mscorlib/desktop (workitem, changeset before temporary rollback, final changeset)

@jamesqo
Copy link
Contributor

@jamesqo jamesqo commented Dec 7, 2016

When porting the change to coreclr/corert, consider cherry-picking the changes from dotnet/corefx#14187 which should substantially speed up GetHashCode.

edit: Also, dotnet/corefx#14141 may need to be ported to the version of ValueTuple already present in desktop.

@jcouv
Copy link
Member Author

@jcouv jcouv commented Dec 8, 2016

@jamesqo Makes sense. When I port ValueTuple to coreclr/corert, I will use the latest code from corefx at that time (so it will include the hashcode changes).

@jcouv
Copy link
Member Author

@jcouv jcouv commented Dec 20, 2016

Added ValueTuple and ITuple to corlib in CoreCLR (dotnet/coreclr#8695) and CoreRT (dotnet/corert#2399).

@jcouv
Copy link
Member Author

@jcouv jcouv commented Jan 3, 2017

Removed ITuple constraint from TRest
Desktop: workitem, changeset
CoreCLR: dotnet/coreclr#8793
CoreRT: dotnet/corert#2434

@jcouv jcouv changed the title Port ValueTuple library code into mscorlib [Umbrella] Port ValueTuple library code into mscorlib Jan 5, 2017
@jcouv
Copy link
Member Author

@jcouv jcouv commented Jan 6, 2017

Added type forwards from CoreFx (System.ValueTuple and System.Runtime) to CoreCLR's implmentation in System.Private.CoreLib (dotnet/corefx#14786)

@jcouv jcouv modified the milestones: 2.0 (RTM), 2.0 (RC.3) Jan 6, 2017
@jcouv
Copy link
Member Author

@jcouv jcouv commented Jan 11, 2017

Mono now re-uses the code from CoreRT, which means it has ITuple too (mono/mono@ddb72c4)

@jcouv jcouv modified the milestones: 2.1, 2.0 (RTM) Jan 11, 2017
@jcouv jcouv changed the title [Umbrella] Port ValueTuple library code into mscorlib [Umbrella] Make ValueTuple library available Jan 18, 2017
@jcouv jcouv changed the title [Umbrella] Make ValueTuple library available and further revisions [Umbrella] ValueTuple library work Feb 9, 2017
@gafter gafter added this to Backlog in Compiler: Tuples Feb 20, 2017
@jcouv jcouv modified the milestones: 2.1, 2.2 Mar 3, 2017
@jcouv jcouv modified the milestones: 15.2, 15.3 Mar 11, 2017
@jaredpar jaredpar modified the milestones: 15.later, 15.3 May 15, 2017
@jcouv
Copy link
Member Author

@jcouv jcouv commented Jun 27, 2017

The remaining bullets from the list above will now be tracked by individual issues. None of them is essential at this point. I'll close this umbrella.

@jcouv jcouv closed this Jun 27, 2017
@jcouv jcouv moved this from Active to Done in Compiler: Julien's umbrellas Jun 27, 2017
@jcouv jcouv moved this from Backlog to Closed in Compiler: Tuples Jun 27, 2017
@jcouv
Copy link
Member Author

@jcouv jcouv commented Oct 18, 2017

An issue was reported with 15.5, possibly related to ASP.Net, but not yet diagnosed.

@Tornhoof
Copy link

@Tornhoof Tornhoof commented Oct 18, 2017

Same issue happens on 15.4 after installing .NET 4.7.1 DevPack and changing the framework version to .NET 4.7.1.
I had to add the ValueTuple Nuget package (4.4) to the projects otherwise it wouldn't compile.
Error is: Error CS8137 Cannot define a class or member that utilizes tuples because the compiler required type 'System.Runtime.CompilerServices.TupleElementNamesAttribute' cannot be found. Are you missing a reference?

@jcouv
Copy link
Member Author

@jcouv jcouv commented Oct 18, 2017

@Tornhoof I think you're describing a problem that has been identified and acknowledged with 4.7.1.. Can you try and see if the mitigation described in this thread (manually editing auto-generated binding redirects) works for the issue you're describing?

@Tornhoof
Copy link

@Tornhoof Tornhoof commented Oct 18, 2017

Yes, manually removing the value tuple redirect fixes it. Unfortunately manually removing something from the auto-generated redirects in the new SDK Project system is a bit of work, for now I prefer my NuGet workaround as it fixes the bindings automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Compiler: Pattern-Matching
Future Subfeatures
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.