EE should ignore ValueTuple from mscorlib type when duplicate found in app #17192

jcouv · 2017-02-16T23:09:30Z

Mitigates https://github.com/dotnet/corefx/issues/16195

Repro:

Make a trivial app targeting 4.6 involving tuples (you must add a reference to ValueTuple package)
On a machine with 4.7 installed, try to debug that program
The program runs fine, with modules listed below (ValueTuple.dll from the corefx package included in the app, mscorlib.dll from desktop 4.7)
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).

Fix:
The basic approach is to leverage the existing BinderFlags.IgnoreCorLibraryDuplicatedTypes flag from the Compilation and affect how ValueTuple types are loaded by GetWellKnownType.

I verified the fix in unittest and manually in VS.
VB has the same bug, so I will work on a fix for VB too.

cston · 2017-02-17T19:20:49Z

src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs

@@ -798,6 +822,11 @@ private NamedTypeSymbol ApplyGenericArguments(NamedTypeSymbol symbol, Type[] typ
            return result;
        }

+        private bool IsInCorLib(NamedTypeSymbol type)


jcouv · 2017-02-17T21:29:01Z

src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs

@@ -75,7 +75,7 @@ internal Symbol GetWellKnownTypeMember(WellKnownMember member)
            return _lazyWellKnownTypeMembers[(int)member];
        }

-        internal NamedTypeSymbol GetWellKnownType(WellKnownType type)
+        internal NamedTypeSymbol GetWellKnownType(WellKnownType type, bool ignoreCorLibraryDuplicatedTypes = false)


Aleksey pointed out that we are in a Compilation here, so no need to thread this through. We can just grab this option from the compilation.

… in mscorlib (C#)

jcouv · 2017-02-21T17:35:55Z

FYI @tmat

cston · 2017-02-21T23:07:46Z

src/Compilers/VisualBasic/Portable/Symbols/AssemblySymbol.vb

+                                    ' ignore candidate
+                                    Continue For
+                                End If
+                                If (IsInCorLib(result)) Then


Parentheses are not needed.

cston · 2017-02-21T23:17:32Z

src/ExpressionEvaluator/Core/Test/ResultProvider/Debugger/Engine/DkmClrRuntimeInstance.cs

@@ -89,6 +91,27 @@ internal DkmClrType GetType(string typeName, params System.Type[] typeArguments)
            return null;
        }

+        private IEnumerable<DkmClrModuleInstance> WithMscorlibLast(DkmClrModuleInstance[] list)


cston · 2017-02-21T23:18:35Z

src/ExpressionEvaluator/Core/Test/ResultProvider/Debugger/Engine/DkmClrRuntimeInstance.cs

+            DkmClrModuleInstance mscorlib = null;
+            foreach (var module in list)
+            {
+                if (module.Assembly.GetReferencedAssemblies().Length == 0)


TypeHelpers.IsMscorlib(module.Assembly)

I had tried that, but they are actually different Assembly types.
One is System.Reflection.Assembly and the other is Microsoft.VisualStudio.Debugger.Metadata.Assembly.

Ah, I see. In that case, can TypeHelpers.IsMscorlib be private again?

Oops. Thanks. Fixed now

jcouv · 2017-02-23T00:49:01Z

@cston @dotnet/roslyn-compiler This is ready for review when you have time. Thanks

cston · 2017-02-23T18:41:53Z

src/ExpressionEvaluator/Core/Test/ResultProvider/Debugger/Engine/DkmClrRuntimeInstance.cs

@@ -11,6 +11,8 @@
 using System.Linq;
 using System.Reflection;
 using Type = Microsoft.VisualStudio.Debugger.Metadata.Type;
+using System.Collections.Generic;
+using System.Diagnostics;


Sort usings.

cston · 2017-02-23T18:43:04Z

src/ExpressionEvaluator/VisualBasic/Test/ExpressionCompiler/TupleTests.vb

+            app.AssertTheseDiagnostics()
+
+            Dim runtime = CreateRuntimeInstance({app.ToModuleInstance(), corlibWithVT.ToModuleInstance()})
+            ' Create EE context with app assembly (including ValueTuple) And a more recent corlib (also including ValueTuple)


Typo: and rather than And.

cston · 2017-02-23T18:47:56Z

src/Scripting/VisualBasic/VisualBasicScriptCompiler.vb

@@ -86,7 +86,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Scripting
                    xmlReferenceResolver:=Nothing, ' don't support XML file references in interactive (permissions & doc comment includes)
                    sourceReferenceResolver:=SourceFileResolver.Default,
                    metadataReferenceResolver:=script.Options.MetadataResolver,
-                    assemblyIdentityComparer:=DesktopAssemblyIdentityComparer.Default),
+                    assemblyIdentityComparer:=DesktopAssemblyIdentityComparer.Default).
+                    WithIgnoreCorLibraryDuplicatedTypes(True),


Was LookupOptions.IgnoreCorLibraryDuplicatedTypes used before in scripting?

VB scripting didn't use that option previously (only the expression compiler). I was a bug.
In C#, both scripting and EE use BinderFlags.IgnoreCorLibraryDuplicatedTypes.

@tmat may want to comment.

Yes, both VB and C# should use it.

cston · 2017-02-23T19:14:51Z

LGTM

VSadov

LGTM

AlekseyTs · 2017-02-23T22:11:46Z

src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs

@@ -778,6 +787,21 @@ private NamedTypeSymbol ApplyGenericArguments(NamedTypeSymbol symbol, Type[] typ
                if ((object)result != null)
                {
                    // duplicate
+                    if (ignoreCorLibraryDuplicatedTypes)
+                    {
+                        if (IsInCorLib(candidate))


if (IsInCorLib(candidate)) [](start = 24, length = 26)

Are tests cover bodies of both if statements (this one and the next)? #Closed

AlekseyTs · 2017-02-23T22:15:17Z

src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs

@@ -798,6 +822,11 @@ private NamedTypeSymbol ApplyGenericArguments(NamedTypeSymbol symbol, Type[] typ
            return result;
        }

+        private static bool IsInCorLib(NamedTypeSymbol type)
+        {
+            return type.ContainingAssembly == type.ContainingAssembly.CorLibrary;


type.ContainingAssembly == type.ContainingAssembly.CorLibrary [](start = 19, length = 61)

(object)type.ContainingAssembly ==CorLibrary ? #Closed

AlekseyTs · 2017-02-23T22:18:11Z

    internal NamedTypeSymbol GetWellKnownType(WellKnownType type)

At this point it feels like we can benefit from a comment describing how exactly ambiguity is handled by this method #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs:78 in f33c1bc. [](commit_id = f33c1bc, deletion_comment = False)

AlekseyTs · 2017-02-23T22:24:14Z

src/Compilers/VisualBasic/Portable/Symbols/AssemblySymbol.vb

@@ -527,6 +541,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
            Return result
        End Function

+        Private Shared Function IsInCorLib(type As NamedTypeSymbol) As Boolean
+            Return type.ContainingAssembly = type.ContainingAssembly.CorLibrary


type.ContainingAssembly = type.ContainingAssembly.CorLibrary [](start = 19, length = 60)

type.ContainingAssembly Is CorLibrary ? #Closed

AlekseyTs · 2017-02-23T22:25:20Z

    Friend Function GetWellKnownType(type As WellKnownType) As NamedTypeSymbol

Consider adding a comment what effect does IgnoreCorLibraryDuplicatedTypes option have. #Closed

Refers to: src/Compilers/VisualBasic/Portable/Symbols/WellKnownMembers.vb:348 in f33c1bc. [](commit_id = f33c1bc, deletion_comment = False)

AlekseyTs · 2017-02-23T22:27:09Z

    Public Overloads Function Equals(other As VisualBasicCompilationOptions) As Boolean Implements IEquatable(Of VisualBasicCompilationOptions).Equals

It feels like an implementation of this function should be adjusted and relevant tests added. #Closed

Refers to: src/Compilers/VisualBasic/Portable/VisualBasicCompilationOptions.vb:964 in f33c1bc. [](commit_id = f33c1bc, deletion_comment = False)

AlekseyTs · 2017-02-23T22:31:13Z

src/ExpressionEvaluator/Core/Test/ResultProvider/Debugger/Engine/DkmClrRuntimeInstance.cs

+            DkmClrModuleInstance mscorlib = null;
+            foreach (var module in list)
+            {
+                if (module.Assembly.GetReferencedAssemblies().Length == 0)


if (module.Assembly.GetReferencedAssemblies().Length == 0) [](start = 16, length = 58)

This check is not accurate. I think it is possible to have an assembly without references that is not Core Library. #Closed

We should at least check whether it defines the object type. #Closed

AlekseyTs · 2017-02-23T22:37:29Z

It looks like we are missing direct tests for GetWellknownType API. #Closed

…7192

…17339)

jcouv added Area-Compilers New Language Feature - Tuples Tuples labels Feb 16, 2017

jcouv added this to the 15.1 milestone Feb 16, 2017

jcouv self-assigned this Feb 16, 2017

dnfclas added the cla-already-signed label Feb 16, 2017

cston reviewed Feb 17, 2017

View reviewed changes

jcouv commented Feb 17, 2017

View reviewed changes

jcouv force-pushed the tolerate-duplicates branch from fbdc730 to cf8b1fd Compare February 18, 2017 01:08

jcouv mentioned this pull request Feb 18, 2017

[Umbrella] ValueTuple library work #13177

Closed

50 tasks

jcouv force-pushed the tolerate-duplicates branch from cf8b1fd to 75a0a5c Compare February 18, 2017 02:39

jcouv added 9 commits February 17, 2017 18:50

Remove space from dotnet#17027

955002d

Add test and fix for EE finding duplicate ValueTuple types in app and…

10e5666

… in mscorlib (C#)

Adding repro for VB

64edd75

Fix for VB

f796199

PR feedback

10654cc

Use option from compilation instead of LookupOption in VB

16219be

Update test helper which is sensitive to host environment

ec78ce9

Update VB script compilation options

45092f8

Set default value

88197c4

jcouv force-pushed the tolerate-duplicates branch from 75a0a5c to 955002d Compare February 18, 2017 02:52

jcouv closed this Feb 21, 2017

jcouv reopened this Feb 21, 2017

dnfclas added the cla-already-signed label Feb 21, 2017

cston reviewed Feb 21, 2017

View reviewed changes

jcouv force-pushed the tolerate-duplicates branch 2 times, most recently from 609d211 to 28abce3 Compare February 22, 2017 18:50

PR feedback (1)

57b4858

jcouv force-pushed the tolerate-duplicates branch from 28abce3 to 57b4858 Compare February 22, 2017 18:53

cston reviewed Feb 23, 2017

View reviewed changes

Sorting usings and fixing typo

f33c1bc

VSadov approved these changes Feb 23, 2017

View reviewed changes

jcouv merged commit 8003aca into dotnet:master Feb 23, 2017

jcouv deleted the tolerate-duplicates branch February 23, 2017 22:07

AlekseyTs reviewed Feb 23, 2017

View reviewed changes

jcouv mentioned this pull request Feb 23, 2017

TypeHelpers.IsMscorlib should also check for object type #17338

Closed

jcouv added a commit to jcouv/roslyn that referenced this pull request Feb 23, 2017

Fix Equals on VB compilation options and other feedback from dotnet#1…

b740011

…7192

This was referenced Feb 23, 2017

Fix Equals on VB compilation options and other feedback from #17192 #17339

Merged

C# Interactive doesn't work with tuple syntax when desktop framework 4.7 is installed #17157

Closed

jcouv added a commit that referenced this pull request Feb 24, 2017

Fix Equals on VB compilation options and other feedback from #17192 (#…

32e8b61

…17339)

atifaziz mentioned this pull request May 2, 2017

Version 2.4 and .NET 4.7 morelinq/MoreLINQ#280

Closed

jcouv mentioned this pull request Jun 11, 2017

Inconsistent CS0433 and CS8179 #20135

Closed

This was referenced Jul 29, 2017

Cannot use C# 7 tuple literals in VS 2017 interactive window #20688

Closed

Scripting Tuples test has broken references between desktop/coreclr tests #18479

Open

csi.rsp includes System.ValueTuple reference #21096

Closed

This was referenced Aug 27, 2017

CSharpCompilation should be able to compile tuple code against Framework 4.6 when 4.7 is installed #21440

Closed

Interactive window to prefer tuple types from mscorlib #21851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EE should ignore ValueTuple from mscorlib type when duplicate found in app #17192

EE should ignore ValueTuple from mscorlib type when duplicate found in app #17192

jcouv commented Feb 16, 2017 •

edited

cston Feb 17, 2017

jcouv Feb 17, 2017

jcouv commented Feb 21, 2017

cston Feb 21, 2017

cston Feb 21, 2017

cston Feb 21, 2017

jcouv Feb 21, 2017

cston Feb 21, 2017

jcouv Feb 22, 2017

jcouv commented Feb 23, 2017

cston Feb 23, 2017

cston Feb 23, 2017

cston Feb 23, 2017

jcouv Feb 23, 2017

tmat Feb 23, 2017

cston commented Feb 23, 2017

VSadov left a comment

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

EE should ignore ValueTuple from mscorlib type when duplicate found in app #17192

EE should ignore ValueTuple from mscorlib type when duplicate found in app #17192

Conversation

jcouv commented Feb 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Feb 23, 2017

VSadov left a comment

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2017 • edited

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2017 • edited

Choose a reason for hiding this comment

AlekseyTs commented Feb 23, 2017 • edited

AlekseyTs Feb 23, 2017 • edited

Choose a reason for hiding this comment

AlekseyTs commented Feb 23, 2017 • edited

AlekseyTs commented Feb 23, 2017 • edited

AlekseyTs Feb 23, 2017 • edited

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2017 • edited

Choose a reason for hiding this comment

AlekseyTs commented Feb 23, 2017 • edited

jcouv commented Feb 16, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs Feb 23, 2017 •

edited

AlekseyTs commented Feb 23, 2017 •

edited