Don't wrap fields in ValueTuple definitions by jcouv · Pull Request #44231 · dotnet/roslyn

jcouv · 2020-05-13T23:28:56Z

The first commit removes the wrapping of fields on ValueTuple definitions. It fixes #43597 and fixes #43549

The second commit removes some field.TupleUnderlyingField ?? field patterns that were added as mitigations.

The third commit fixes #43595 (don't emit tuple error fields)

cston · 2020-05-14T18:14:18Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

-                            var field = (FieldSymbol)member;
-                            var fieldSymbol = (field.TupleUnderlyingField ?? field) as SourceMemberFieldSymbol;
+                            FieldSymbol fieldSymbol = member as FieldSymbol;
                            if ((object)fieldSymbol != null)


Can use explicit cast and avoid the null check. #Resolved

Could also do

if (member is FieldSymbol fieldSymbol) ``` #Resolved

cston · 2020-05-14T18:20:57Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

                VisitFieldType(element);
-                if (!element.IsImplicitlyDeclared)
+                if (!element.IsImplicitlyDeclared &&
+                    !(symbol.IsGenericType && symbol.IsDefinition)) // C# tuples are generic


Are we checking if this is a default field rather than a named field? If so, is there a more explicit way to do that check that is language-independent? Perhaps element.CorrespondingTupleField != null. #Resolved

I've tried to think of different approaches, and tried a few, but this is really tricky.
We don't want to display the Item1 in VT'2 (which is !IsImplicitlyDeclared, but has a CorrespondingTupleField).
Also checks are whether fields are default are not don't work, as (int Item1, int).Item1 is a default field even though explicitly named.

So ended up sticking with this check. #ByDesign

cston · 2020-05-14T18:24:16Z

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

            get
            {
-                return false;
+                return ContainingType.IsDefinition && TupleElementIndex >= 0;


ContainingType.IsDefinition [](start = 23, length = 27)

TupleElementIndex includes this check. #Resolved

cston · 2020-05-14T18:24:21Z

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

            get
            {
-                return null;
+                return (ContainingType.IsDefinition && TupleElementIndex >= 0) ? this : null;


ContainingType.IsDefinition [](start = 24, length = 27)

TupleElementIndex includes this check. #Resolved

That property is virtual though. Do we feel comfortable depending on all the overrides also enforcing this behavior? #Resolved

cston · 2020-05-14T18:24:57Z

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

        {
            get
            {
+                if (ContainingType.IsTupleType && ContainingType.IsDefinition)


ContainingType.IsDefinition [](start = 50, length = 27)

Why is it important that the containing type is the definition? #Resolved

In thinking about this question, I realized it's not. This implementation should handle the underlying fields in a constructed tuple. Will add test and fix

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

cston · 2020-05-14T18:25:57Z

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

+                if (ContainingType.IsTupleType && ContainingType.IsDefinition)
+                {
+                    var i = NamedTypeSymbol.MatchesCanonicalElementName(Name);
+                    int itemsPerType = Math.Min(ContainingType.Arity, NamedTypeSymbol.ValueTupleRestPosition - 1);


itemsPerType [](start = 24, length = 12)

Consider inlining this to only calculate when i > 0. #Resolved

cston · 2020-05-14T18:28:58Z

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

+
+                        RuntimeMembers.MemberDescriptor relativeDescriptor = WellKnownMembers.GetDescriptor(wellKnownMember);
+                        var found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), relativeDescriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,
+                                                                  accessWithinOpt: null); // force lookup of public members only


This looks expensive. Can we calculate TupleElementIndex at most once? (Perhaps this property should be implemented in derived types where it might make more sense to cache the value.) #ByDesign

I didn't want to add state to every source or PE symbol just because it might be a tuple element.
By limiting this to definitions of ValueTuple and doing a cheap name check first, I think this becomes reasonable.

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

cston · 2020-05-14T18:32:21Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

                    case SymbolKind.Field:
-                        var field = (FieldSymbol)m;
-                        yield return field.TupleUnderlyingField ?? field;
+                        if (m is TupleErrorFieldSymbol)


if (m is TupleErrorFieldSymbol) [](start = 24, length = 31)

When do we try to emit with error fields? #Resolved

struct ValueTuple<T1, T2> { }

This is recognized as a 2-tuple, and thus has Item1 and Item2

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

cston · 2020-05-14T18:33:14Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs

+                            if (m is TupleErrorFieldSymbol)
+                            {
+                                break;
+                            }


Is this needed? #Resolved

For empty structs, we emit some layout information ([StructLayout(LayoutKind.Sequential, Size = 1)]).
Without this change, we don't emit this layout information correctly, and so PEVerification fails (struct must have a type or a size).

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

Why do we have an error field in a valid struct?

In reply to: 425410620 [](ancestors = 425410620,425349126)

Because a type System.ValueTuple'2 is recognized as a tuple, and therefore has two fields Item1 and Item2 that can be accessed regardless of what the source says. We make them error fields because they lack real/backing fields.

Similarly, on an 8-tuple, we have an Item8 (an error field) if ValueTuple'1 lacks an Item1.

In reply to: 425411648 [](ancestors = 425411648,425410620,425349126)

cston · 2020-05-14T18:44:09Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

+                            {
+                                // No need to wrap fields on System.ValueTuple definitions
+                                members.Add(field);
+                                int tupleFieldIndex2 = currentFieldsForElements.IndexOf(field, ReferenceEqualityComparer.Instance);


currentFieldsForElements [](start = 55, length = 24)

Consider using a Dictionary<Symbol, int> for currentFieldsForElements. #Closed

Never mind. This is the same lookup we were performing previously, and there are at most 8 items in the collection.

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

cston · 2020-05-14T18:49:17Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

@@ -26463,6 +26464,7 @@ void verifyTupleTypeWithErrorUnderlyingType(CSharpCompilation compilation, bool
        [WorkItem(41207, "https://github.com/dotnet/roslyn/issues/41207")]
        [WorkItem(1056281, "https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1056281")]
        [WorkItem(43549, "https://github.com/dotnet/roslyn/issues/43549")]


[WorkItem(43549, "https://github.com//issues/43549")] [](start = 8, length = 66)

Duplicate. Same comment for other tests below. #Resolved

cston · 2020-05-14T18:51:13Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs

 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Test.Utilities;
 using Roslyn.Utilities;
+using TestResources.NetFX;


TestResources.NetFX [](start = 6, length = 19)

Is this used? #Resolved

AlekseyTs · 2021-02-26T16:02:40Z

Done with review pass (commit 31) #Closed

AlekseyTs

LGTM (commit 32)

jcouv · 2021-03-01T17:45:31Z

@dotnet/roslyn-compiler for a second review. I'm happy to jump on a call and walk you through if needed. Thanks

jcouv · 2021-03-02T22:07:22Z

@dotnet/roslyn-compiler for a second review. I'm happy to jump on a call and walk you through if needed. Thanks

jaredpar · 2021-03-02T22:08:59Z

@333fred can u take a look at this?

jcouv · 2021-03-09T16:55:48Z

Gentle ping for second review. Thanks

jaredpar · 2021-03-09T18:47:44Z

@333fred can u take a look

jcouv · 2021-03-15T20:05:27Z

@333fred Can you review this week? Thanks

333fred · 2021-03-15T21:22:41Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

-            Debug.Assert(!fieldSymbol.IsVirtualTupleField && (object)(fieldSymbol.TupleUnderlyingField ?? fieldSymbol) == fieldSymbol, "tuple fields should be rewritten to underlying by now");
+            Debug.Assert(!fieldSymbol.IsVirtualTupleField &&
+                (object)(fieldSymbol.TupleUnderlyingField ?? fieldSymbol) == fieldSymbol &&
+                !(fieldSymbol is TupleErrorFieldSymbol), "tuple fields should be rewritten to underlying by now");


!(fieldSymbol is TupleErrorFieldSymbol) [](start = 16, length = 39)

Nit: consider is not. #Resolved

333fred · 2021-03-15T21:23:34Z

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

        {
            get
            {
+                Debug.Assert(!(this is TupleElementFieldSymbol) && !(this is TupleErrorFieldSymbol));


!(this is TupleElementFieldSymbol) && !(this is TupleErrorFieldSymbol) [](start = 29, length = 70)

Consider this is not (TupleElementFieldSymbol or TupleErrorFieldSymbol) #Resolved

And below

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

cston · 2021-03-19T01:39:45Z

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

+                // wrapped tuple fields already have this information and override this property
+                Debug.Assert(!(this is TupleElementFieldSymbol) && !(this is TupleErrorFieldSymbol));
+
+                if (ContainingType.IsTupleType)


if (ContainingType.IsTupleType) [](start = 16, length = 31)

This check seems unnecessary. #Closed

cston · 2021-03-19T02:17:38Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs

+                    return null;
+                }
+
                return _containingTuple.GetTupleMemberSymbolForUnderlyingMember(_underlyingField.AssociatedSymbol);


AssociatedSymbol [](start = 97, length = 16)

When is there an associated symbol for a tuple field?

In short, are fields of tuple types that aren't tuple elements also represented as TupleFieldElementSymbol rather than PEFieldSymbol? (I realize this hasn't changed with this PR but it seems surprising that a tuple element could have an associated field.) #Closed

I think you're right. This can be simplified (always null). Will add test explicitly for this scenario.

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

cston · 2021-03-19T03:33:47Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

+            _ = this.GetMembers();
+        }
+
+        public TMember? GetTupleMemberSymbolForUnderlyingMember<TMember>(TMember underlyingMemberOpt) where TMember : Symbol?


? [](start = 124, length = 1)

Why where TMember : Symbol? rather than where TMember : Symbol and a nullable parameter TMember? underlyingMemberOpt ? #Closed

cston · 2021-03-19T03:36:08Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

                return fields.ToImmutableAndFree();
            }
+
+            static ImmutableArray<Location> getElementLocations(ref ImmutableArray<Location?> elementLocations, int tupleFieldIndex)


ref [](start = 64, length = 3)

Should this be in or passed by value? #Closed

cston · 2021-03-19T03:41:41Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

+                                    if (IsDefinition)
+                                    {
+                                        defaultTupleField = field;
+                                        fieldDefinitionsToIndexMap!.Add(field.OriginalDefinition, tupleFieldIndex);


.OriginalDefinition [](start = 77, length = 19)

Isn't field.OriginalDefinition == field? #Closed

cston · 2021-03-19T03:46:36Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

            }

            elementsMatchedByFields.Free();
+            members.AddRange(nonFieldMembers);


members.AddRange(nonFieldMembers); [](start = 12, length = 34)

Why is it necessary for fields to precede non-fields?

I had a vague recollection that we may have some code depending on that, but that doesn't seem to be the case.
I tested the change again just now and only a few tests that verify the members in order are affected.

@AlekseyTs Maybe you remember about this? Is it important to have the fields come first? If not, I'll update the test baseline.

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

Maybe you remember about this? Is it important to have the fields come first? If not, I'll update the test baseline.

I think PENamedTypeSymbol assumes some grouping and ordering of members.

In reply to: 601706851 [](ancestors = 601706851,597388310)

cston · 2021-03-19T03:52:44Z

        Assert.Equal("RetargetingMethodSymbol", methodM.GetType().Name);

Consider using Assert.IsType<...>(...) for each of these cases. #Closed

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

cston · 2021-03-25T21:13:59Z

    public override Symbol? AssociatedSymbol

sealed?

Refers to: src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs:95 in 2484dc0. [](commit_id = 2484dc0, deletion_comment = False)

jcouv added Area-Compilers Feature - Tuples Tuples labels May 13, 2020

jcouv added this to the 16.7 milestone May 13, 2020

jcouv self-assigned this May 13, 2020

Julien Couvreur added 2 commits May 13, 2020 17:19

Don't wrap original tuple fields

72dc304

No need to unwrap real tuple fields

881e92d

jcouv force-pushed the tuple-fields branch 2 times, most recently from b06ad5e to 881e92d Compare May 14, 2020 15:11

Julien Couvreur added 2 commits May 14, 2020 08:30

Don't emit tuple error fields

21f72af

More tests

348daea

jcouv marked this pull request as ready for review May 14, 2020 18:07

jcouv requested a review from a team as a code owner May 14, 2020 18:07

jcouv requested a review from AlekseyTs May 14, 2020 18:07

cston reviewed May 14, 2020

View reviewed changes

Address feedback

4154f07

jcouv marked this pull request as draft May 15, 2020 15:48

Revert symbol display logic

758a8be

jcouv force-pushed the tuple-fields branch from 09eabb0 to 758a8be Compare May 15, 2020 17:16

Remove TupleFieldSymbol

f4b0b9e

Remove if. Fix retargeting option

1b2eb98

AlekseyTs approved these changes Mar 1, 2021

View reviewed changes

Base automatically changed from master to main March 3, 2021 23:52

333fred reviewed Mar 15, 2021

View reviewed changes

cston reviewed Mar 19, 2021

View reviewed changes

Address feedback from Chuck

2484dc0

cston approved these changes Mar 25, 2021

View reviewed changes

sealed

f02ee32

cston approved these changes Mar 25, 2021

View reviewed changes

jcouv merged commit f23a180 into dotnet:main Mar 26, 2021

ghost modified the milestones: 16.10, Next Mar 26, 2021

jcouv deleted the tuple-fields branch March 26, 2021 00:03

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Conversation

jcouv commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 14, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented May 13, 2020 •

edited

Loading

cston May 14, 2020 •

edited by jcouv

Loading

jaredpar May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

jcouv May 15, 2020 •

edited

Loading

cston May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

jaredpar May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

jcouv May 15, 2020 •

edited

Loading

cston May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited

Loading

cston May 14, 2020 •

edited by jcouv

Loading

cston May 14, 2020 •

edited by jcouv

Loading

AlekseyTs commented Feb 26, 2021 •

edited

Loading

333fred Mar 15, 2021 •

edited by jcouv

Loading

333fred Mar 15, 2021 •

edited by jcouv

Loading

cston Mar 19, 2021 •

edited

Loading

cston Mar 19, 2021 •

edited

Loading

cston Mar 19, 2021 •

edited

Loading

cston Mar 19, 2021 •

edited

Loading

cston Mar 19, 2021 •

edited

Loading