Remove dedicated tuple type symbol #39370

jcouv · 2019-10-17T21:38:24Z

Fixes #20648

Makes tuples be generic types and removes the TupleTypeSymbol wrapping.
Instead, we're adding tuple element names and virtual fields directly onto NamedTypeSymbol (storing them in an optional TupleUncommonData) with AddOrWrapTupleMembers.

Diagnostics now always prefer tuple syntax, while IL verification always uses System.ValueTuple<...>.

Impacted APIs are Arity, TypeArguments and OriginalDefinition.

VB will be handled separately, but following the same model.

The IDE code was updated:

ValueTupleRef was added to the tests, which caused some refactorings to generate better GetHashCode methods
some special handling was added so that we find the definition of tuple fields on the user syntax, not on the System.ValueTuple definition
some tweaks to QuickInfo displaying tuples

Status: a couple of IDE tests are still failing.

CyrusNajmabadi · 2019-10-30T22:46:21Z

Any implication on hte public roslyn surface area here? would this be observable to clients of the API? i.e. would someone now see Generate TypeArguments on a tuple symbol where they didn't before? #Resolved

jcouv · 2019-10-30T23:26:34Z

@CyrusNajmabadi Yes, there is some impact. It is detailed here. For example, Arity and TypeArguments. #Resolved

CyrusNajmabadi · 2019-10-31T07:11:19Z

@CyrusNajmabadi Yes, there is some impact. It is detailed here. For example, Arity and TypeArguments.

interesting! when you make the change LMK if there is any ide fallout. i'd be curious to see what sort of impact this has on consumptive code.

Note: IDE has special serialization/deserializtion logic here for Tuples: https://github.com/dotnet/roslyn/blob/master/src/Workspaces/Core/Portable/SymbolKey/SymbolKey.TupleTypeSymbolKey.cs

My hope is that you will be able to get rid of this almost entirely and just use the named-type-symbolkey serialization logic. Though... perhaps taht won't be possible since there still needs to be some way to funnel the element-names around. But perhaps we can unify this in some cleaner way versus what we have right now.

Thanks! #Resolved

gafter · 2019-12-12T21:09:00Z

            default:

What about GetType and ReferenceEquals? #Resolved

Refers to: src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs:547 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

gafter · 2019-12-12T21:19:55Z

src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedNamedTypeSymbol.cs

@@ -123,6 +124,10 @@ public override ImmutableArray<Location> Locations
 {
 get
 {
+ if (IsTupleType)
+ {
+ return TupleData.Locations;


Locations [](start = 37, length = 9)

I assume this is an improvement in the user experience. Nit: add an empty line after the }, and in the next method too. #Resolved

Yes, we need to keep locations pointing to where various tuples and tuple element names are defined in source.
Without this, GoToDefinition and FindAllReferences works badly.

In reply to: 357376280 [](ancestors = 357376280)

gafter · 2019-12-13T23:13:20Z

        // Note: we emitted the missing fields

This is confusing. We didn't even give an error about it, we just emitted them? What if source had contained a private Item1? Would we then have emitted a synthesized one also? #WontFix

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs:5878 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

jcouv · 2019-12-16T17:35:15Z

    /// For instance, ValueTuple&lt;..., ValueTuple&lt; int >> (the underlying type for an 8-tuple)

No can do. This results in bad XML:

In reply to: 565184831 [](ancestors = 565184831)

Refers to: src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs:229 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

jcouv · 2019-12-16T17:37:36Z

            default:

Unsure (they are currently allowed, but I'm not sure if that's dangerous). I'd rather separate this question from this PR, as it is existing behavior. File an issue?

In reply to: 565187780 [](ancestors = 565187780)

Refers to: src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs:547 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

jcouv · 2019-12-16T17:40:59Z

        // Note: we emitted the missing fields

I'm open to making this scenario an error, but I'm not sure how much I care.
The scenario where Item1 is defined but is private is covered in BadValueTupleByItself.

In reply to: 565645020 [](ancestors = 565645020)

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs:5878 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

jcouv · 2019-12-16T17:42:15Z

            if (currentType.IsTupleType)

I'll remove this unreferenced method :)

In reply to: 565185455 [](ancestors = 565185455)

Refers to: src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs:260 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

jcouv · 2019-12-16T20:23:03Z

            default:

Filed #40417 for "GetType"

I think "ReferenceEquals" is okay, but we can discuss that in that issue. Thanks

In reply to: 566164166 [](ancestors = 566164166,565187780)

Refers to: src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs:547 in e8059fb. [](commit_id = e8059fb, deletion_comment = False)

gafter · 2019-12-16T21:22:16Z

Done reviewing (Iteration 18). Only two remaining substantive comments: one in NamedTypeSymbol.cs and one in UnsupportedMetadataTypeSymbol.cs. #Resolved

mavasani

IDE changes LGTM.

mavasani · 2019-12-17T18:47:57Z

src/VisualStudio/Core/Test/CodeModel/AbstractRootCodeModelTests.vb

 Dim actualNames = rootCodeModel.CodeElements.OfType(Of EnvDTE.CodeElement).Select(Function(e) e.Name).ToArray()
+ Assert.Equal(names.Length, rootCodeModel.CodeElements.Count)


unintentional change? #Resolved

This made debugging failures easier (you get to access the actualNames local) #Resolved

mavasani · 2019-12-17T18:52:03Z

src/Workspaces/Core/Portable/SymbolKey/SymbolKey.SymbolKeyWriter.cs

@@ -421,7 +421,7 @@ public override object VisitNamedType(INamedTypeSymbol namedTypeSymbol)
 WriteType(SymbolKeyType.ErrorType);
 ErrorTypeSymbolKey.Create(namedTypeSymbol, this);
 }
- else if (namedTypeSymbol.IsTupleType)
+ else if (namedTypeSymbol.IsTupleType && namedTypeSymbol.TupleUnderlyingType != namedTypeSymbol)


Should we be using the symbol comparer for comparing symbols? #Resolved

gafter

JoeRobich · 2019-12-17T21:58:30Z

All CI checks are passing. Overriding to merge.

jcouv · 2019-12-17T21:58:54Z

Yay, thanks!

This lowers the minimum required VS version. Note: 3.4 introduces test failures related to tuple underlying type resolution - probably related to this change: dotnet/roslyn#39370

jcouv added the Area-Compilers label Oct 17, 2019

jcouv added this to the 16.5 milestone Oct 17, 2019

jcouv self-assigned this Oct 17, 2019

jcouv force-pushed the tuple-symbols branch 4 times, most recently from 15cb3e3 to c517383 Compare October 22, 2019 00:26

jcouv force-pushed the tuple-symbols branch from e0e235c to 4a83224 Compare October 30, 2019 23:29

jcouv force-pushed the tuple-symbols branch from 51292ec to e3f0123 Compare November 1, 2019 21:33

jcouv closed this Nov 4, 2019

jcouv reopened this Nov 4, 2019

jcouv force-pushed the tuple-symbols branch 3 times, most recently from ebe37eb to 28692cc Compare November 5, 2019 01:36

jaredpar closed this Nov 5, 2019

jaredpar reopened this Nov 5, 2019

jcouv force-pushed the tuple-symbols branch 6 times, most recently from bd53057 to 49f3922 Compare November 8, 2019 21:58

jcouv force-pushed the tuple-symbols branch 2 times, most recently from 8b5d7ff to a485e8a Compare December 3, 2019 00:04

jcouv force-pushed the tuple-symbols branch 2 times, most recently from 88a8e27 to 16197a2 Compare December 4, 2019 00:25

Refactor tuple implementation

468d190

gafter reviewed Dec 12, 2019

View reviewed changes

PR feedback from Neal

d4342ba

jcouv added 2 commits December 16, 2019 14:17

Restore an IDE test without ValueTuple

8b9c722

Rename constants

b25d1ec

jcouv requested a review from CyrusNajmabadi December 16, 2019 23:03

jcouv added 2 commits December 16, 2019 16:20

Merge remote-tracking branch 'dotnet/master' into tuple-symbols

e686f4e

Restore formatting

c935653

jcouv requested review from gafter and mavasani December 17, 2019 18:33

mavasani approved these changes Dec 17, 2019

View reviewed changes

gafter approved these changes Dec 17, 2019

View reviewed changes

JoeRobich merged commit cc30674 into dotnet:master Dec 17, 2019

jcouv deleted the tuple-symbols branch December 17, 2019 21:58

cston mentioned this pull request Feb 14, 2020

INamedTypeSymbol.TupleUnderlyingType should return null rather than this #41702

Closed

gafter mentioned this pull request Mar 30, 2020

VB symbolic API incorrect for tuple types. #42860

Open

AlekseyTs mentioned this pull request Apr 24, 2020

Invalid metadata emitted for a System.ValueTuple type #43595

Closed

vweijsters mentioned this pull request Jul 27, 2020

Fixed issues DotNetAnalyzers/StyleCopAnalyzers#3180

Merged

CyrusNajmabadi mentioned this pull request Mar 2, 2021

Stack overflow in SymbolEquivalenceComparer #50800

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dedicated tuple type symbol #39370

Remove dedicated tuple type symbol #39370

jcouv commented Oct 17, 2019 •

edited

Loading

CyrusNajmabadi commented Oct 30, 2019 •

edited by jcouv

Loading

jcouv commented Oct 30, 2019 •

edited

Loading

CyrusNajmabadi commented Oct 31, 2019 •

edited by jcouv

Loading

gafter commented Dec 12, 2019 •

edited by jcouv

Loading

gafter Dec 12, 2019 •

edited

Loading

jcouv Dec 16, 2019

gafter commented Dec 13, 2019 •

edited by jcouv

Loading

jcouv commented Dec 16, 2019 •

edited

Loading

jcouv commented Dec 16, 2019 •

edited

Loading

jcouv commented Dec 16, 2019

jcouv commented Dec 16, 2019

jcouv commented Dec 16, 2019

gafter commented Dec 16, 2019 •

edited by jcouv

Loading

mavasani left a comment

mavasani Dec 17, 2019 •

edited by jcouv

Loading

jcouv Dec 17, 2019 •

edited

Loading

mavasani Dec 17, 2019 •

edited by jcouv

Loading

gafter left a comment

JoeRobich commented Dec 17, 2019

jcouv commented Dec 17, 2019

		Dim actualNames = rootCodeModel.CodeElements.OfType(Of EnvDTE.CodeElement).Select(Function(e) e.Name).ToArray()
		Assert.Equal(names.Length, rootCodeModel.CodeElements.Count)

Remove dedicated tuple type symbol #39370

Remove dedicated tuple type symbol #39370

Conversation

jcouv commented Oct 17, 2019 • edited Loading

CyrusNajmabadi commented Oct 30, 2019 • edited by jcouv Loading

jcouv commented Oct 30, 2019 • edited Loading

CyrusNajmabadi commented Oct 31, 2019 • edited by jcouv Loading

gafter commented Dec 12, 2019 • edited by jcouv Loading

gafter Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 16, 2019

Choose a reason for hiding this comment

gafter commented Dec 13, 2019 • edited by jcouv Loading

jcouv commented Dec 16, 2019 • edited Loading

jcouv commented Dec 16, 2019 • edited Loading

jcouv commented Dec 16, 2019

jcouv commented Dec 16, 2019

jcouv commented Dec 16, 2019

gafter commented Dec 16, 2019 • edited by jcouv Loading

mavasani left a comment

Choose a reason for hiding this comment

mavasani Dec 17, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

mavasani Dec 17, 2019 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

JoeRobich commented Dec 17, 2019

jcouv commented Dec 17, 2019

jcouv commented Oct 17, 2019 •

edited

Loading

CyrusNajmabadi commented Oct 30, 2019 •

edited by jcouv

Loading

jcouv commented Oct 30, 2019 •

edited

Loading

CyrusNajmabadi commented Oct 31, 2019 •

edited by jcouv

Loading

gafter commented Dec 12, 2019 •

edited by jcouv

Loading

gafter Dec 12, 2019 •

edited

Loading

gafter commented Dec 13, 2019 •

edited by jcouv

Loading

jcouv commented Dec 16, 2019 •

edited

Loading

jcouv commented Dec 16, 2019 •

edited

Loading

gafter commented Dec 16, 2019 •

edited by jcouv

Loading

mavasani Dec 17, 2019 •

edited by jcouv

Loading

jcouv Dec 17, 2019 •

edited

Loading

mavasani Dec 17, 2019 •

edited by jcouv

Loading