ValueTuple in desktop 4.6 app clashes with that of mscorlib 4.7 #16195

Closed
jcouv opened this Issue Feb 15, 2017 · 12 comments

Comments

Projects
None yet
7 participants
@jcouv
Member

jcouv commented Feb 15, 2017

Repro:

  1. Make a trivial app targeting 4.6 involving tuples (you must add a reference to ValueTuple package)
  2. On a machine with 4.7 installed, try to debug that program
  3. The program runs fine, with modules listed below (ValueTuple.dll from the corefx package included in the app, mscorlib.dll from desktop 4.7)
  4. In the EE (Immediate Window, Watch, QuickWatch) type something that involves tuple syntax
    This will give an error CS8179, because the compiler found two instances of the well-known type (that is ambiguity).

image

(1, 2)
error CS8179: Predefined type 'System.ValueTuple`2' is not defined or imported
t = (1, 2)
error CS8179: Predefined type 'System.ValueTuple`2' is not defined or imported
        static void Main(string[] args)
        {
            (int first, int second) t = (1, 1);
            while(true)
            {
                Thread.Sleep(1000);
                Console.WriteLine(t.first);
                t.first++;
            }
        }

FYI @tmat @VSadov @cston @AlekseyTs

@jcouv jcouv referenced this issue in dotnet/roslyn Feb 15, 2017

Closed

[Umbrella] ValueTuple library work #13177

42 of 49 tasks complete
@tmat

This comment has been minimized.

Show comment
Hide comment
@tmat

tmat Feb 15, 2017

Member

A fix would be to include System.ValueTuple.dll in .NET Framework 4.7 that type-forwards all types to mscorlib and add it to GAC. When loading System.ValueTuple the loader would prefer this library over the one in the app dir. So there would be no duplicates loaded.

Member

tmat commented Feb 15, 2017

A fix would be to include System.ValueTuple.dll in .NET Framework 4.7 that type-forwards all types to mscorlib and add it to GAC. When loading System.ValueTuple the loader would prefer this library over the one in the app dir. So there would be no duplicates loaded.

@tmat

This comment has been minimized.

Show comment
Hide comment
@tmat

tmat Feb 15, 2017

Member

Issue dotnet/roslyn#17157 has the same root cause.

Member

tmat commented Feb 15, 2017

Issue dotnet/roslyn#17157 has the same root cause.

@jcouv

This comment has been minimized.

Show comment
Hide comment
@jcouv

jcouv Feb 15, 2017

Member

Discussed with @weshaggard
If we made a change to desktop 4.7 to have type unification to System.ValueTuple.dll version 4.0 (type unification only cares about major and minor version), then we'd introduce another problem.
System.ValueTuple.dll 4.0.1.1 has ITuple, but desktop 4.7 doesn't.
An app that used ITuple on desktop 4.6 would lose it when loaded on a machine with 4.7. (Maybe that isn't so bad?)
I'm going to explore an EE/compiler solution and I'll get back with some findings.

Member

jcouv commented Feb 15, 2017

Discussed with @weshaggard
If we made a change to desktop 4.7 to have type unification to System.ValueTuple.dll version 4.0 (type unification only cares about major and minor version), then we'd introduce another problem.
System.ValueTuple.dll 4.0.1.1 has ITuple, but desktop 4.7 doesn't.
An app that used ITuple on desktop 4.6 would lose it when loaded on a machine with 4.7. (Maybe that isn't so bad?)
I'm going to explore an EE/compiler solution and I'll get back with some findings.

@tmat

This comment has been minimized.

Show comment
Hide comment
@tmat

tmat Feb 15, 2017

Member

Why doesn't 4.7 have ITuple? Can we add it?

Member

tmat commented Feb 15, 2017

Why doesn't 4.7 have ITuple? Can we add it?

@jcouv

This comment has been minimized.

Show comment
Hide comment
@jcouv

jcouv Feb 15, 2017

Member

I shiproom rejected the ITuple PR when I made it (a few days past code complete alas), so I doubt they would take it now.
@weshaggard Let me know if I'm being too pessimistic.

Member

jcouv commented Feb 15, 2017

I shiproom rejected the ITuple PR when I made it (a few days past code complete alas), so I doubt they would take it now.
@weshaggard Let me know if I'm being too pessimistic.

@tmat

This comment has been minimized.

Show comment
Hide comment
@tmat

tmat Feb 15, 2017

Member

The shiproom didn't know we have a bigger problem.

Member

tmat commented Feb 15, 2017

The shiproom didn't know we have a bigger problem.

@tmat

This comment has been minimized.

Show comment
Hide comment
@tmat

tmat Feb 15, 2017

Member

If the compiler resolved the ambiguity by picking the type from mscorlib (as we do in EEs for other corlib ambiguities), wouldn't we have the same problem with ITuple? That EE wouldn't see ITuple, so expression evaluation would behave differently than the application code. That would be confusing for the user.

Member

tmat commented Feb 15, 2017

If the compiler resolved the ambiguity by picking the type from mscorlib (as we do in EEs for other corlib ambiguities), wouldn't we have the same problem with ITuple? That EE wouldn't see ITuple, so expression evaluation would behave differently than the application code. That would be confusing for the user.

@jcouv

This comment has been minimized.

Show comment
Hide comment
@jcouv

jcouv Mar 30, 2017

Member

Ping @weshaggard This is the issue I mentioned, which will require work in desktop 4.7.1 (to the GAC and type unification), but not to corefx itself.

Member

jcouv commented Mar 30, 2017

Ping @weshaggard This is the issue I mentioned, which will require work in desktop 4.7.1 (to the GAC and type unification), but not to corefx itself.

@jcouv

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Apr 18, 2017

Member

@AlexGhiondea is this going to be part of what you're doing?
Also should you move it to the desktop bug tracker?

Member

danmosemsft commented Apr 18, 2017

@AlexGhiondea is this going to be part of what you're doing?
Also should you move it to the desktop bug tracker?

@AlexGhiondea

This comment has been minimized.

Show comment
Hide comment
@AlexGhiondea

AlexGhiondea Apr 19, 2017

Member

Yes it is. And there is no need for an additional issue -- i am handling this as part of a larger work item.

Member

AlexGhiondea commented Apr 19, 2017

Yes it is. And there is no need for an additional issue -- i am handling this as part of a larger work item.

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Apr 25, 2017

Member

I believe I've also addressed this in the NETStandard.Library.NETFramework support package. Closing this.

Member

ericstj commented Apr 25, 2017

I believe I've also addressed this in the NETStandard.Library.NETFramework support package. Closing this.

@ericstj ericstj closed this Apr 25, 2017

@atifaziz atifaziz referenced this issue in morelinq/MoreLINQ May 2, 2017

Closed

Version 2.4 and .NET 4.7 #280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment