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.

Sign up for GitHub

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

Don't wrap fields in ValueTuple definitions #44231

Merged
merged 34 commits into from
Mar 26, 2021
Merged

Don't wrap fields in ValueTuple definitions #44231

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
72dc304
Don't wrap original tuple fields
jcouv May 13, 2020
881e92d
No need to unwrap real tuple fields
jcouv May 13, 2020
21f72af
Don't emit tuple error fields
jcouv May 13, 2020
348daea
More tests
jcouv May 14, 2020
4154f07
Address feedback
jcouv May 14, 2020
758a8be
Revert symbol display logic
jcouv May 15, 2020
f4b0b9e
Remove TupleFieldSymbol
jcouv May 15, 2020
df14519
Address feedback from Aleksey
jcouv May 18, 2020
f491231
Cache some information for TupleElementIndex
jcouv May 18, 2020
6400146
Address more feedback
jcouv May 18, 2020
2054720
Keep fields first in GetMembers()
jcouv May 18, 2020
fc85a89
Address more feedback
jcouv May 19, 2020
74cfed2
Add HasExplicitTupleElementName API
jcouv Jun 25, 2020
c422d49
Don't retarget tuple error fields
jcouv Jun 25, 2020
c57a02b
Put event backing field back in map
jcouv Jun 26, 2020
c18e709
Use original definitions for map to index
jcouv Jun 26, 2020
49199b8
Optimize map from field definition to index
jcouv Jun 26, 2020
5575125
Verify metadata emitted for fixed field
jcouv Jun 26, 2020
52dba4d
Verify IsDefinition on long tuples elements
jcouv Jul 5, 2020
92cf81d
Empty line
jcouv Jul 6, 2020
e40f8c3
Tweaks
jcouv Jul 6, 2020
ae512eb
Tweaks
jcouv Jul 6, 2020
612364d
Merge remote-tracking branch 'dotnet/master' into tuple-fields
jcouv Jul 6, 2020
a3706e7
Implement interface on IDE layer symbol
jcouv Jul 6, 2020
6b52689
Restore OriginalDefinition logic
jcouv Jul 7, 2020
9eeeecb
Merge remote-tracking branch 'dotnet/master' into tuple-fields
jcouv Dec 28, 2020
d260464
Add public API
jcouv Dec 28, 2020
59933df
order modifiers
jcouv Dec 29, 2020
465438c
Address feedback
jcouv Feb 26, 2021
77e86db
Use other retargeting option
jcouv Feb 26, 2021
ac45f85
Merge remote-tracking branch 'dotnet/master' into tuple-fields
jcouv Feb 26, 2021
1b2eb98
Remove if. Fix retargeting option
jcouv Feb 26, 2021
2484dc0
Address feedback from Chuck
jcouv Mar 25, 2021
f02ee32
sealed
jcouv Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,8 @@ private void AddTupleTypeName(INamedTypeSymbol symbol)
}

VisitFieldType(element);
if (!element.IsImplicitlyDeclared)
if (!element.IsImplicitlyDeclared &&
!(symbol.IsGenericType && symbol.IsDefinition)) // C# tuples are generic
Copy link
Member

@cston cston May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jcouv jcouv May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@AlekseyTs AlekseyTs May 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!(symbol.IsGenericType && symbol.IsDefinition) [](start = 20, length = 46)

This logic looks suspicious. Is this going to do the right thing for VB symbols? Is this PR affecting behavior of IsImplicitlyDeclared? Is implementation of VB SymbolDisplay going to do the right thing for C# symbols now, I do not see any changes made there?
Also, I am assuming that for (T1 Item1, T2) we want to print "Item1", but for (T1, T2 Item2) we don't want to print it. I don't see how (symbol.IsGenericType && symbol.IsDefinition) could yield different value for both these types. Perhaps I am misunderstanding the scenario. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR, we're not changing the behavior of IsImplicitlyDeclared on TupleElementFieldSymbol (wrapped fields) or TupleErrorFieldSymbol. But for fields that are now unwrapped, namely fields that appear in the ValueTuple<...> types, IsImplicitlyDeclared now returns false.
For instance, in struct ValueTuple<T1, T2) { T1 Item1; T2 Item2; string goo; } the three fields have IsImplicitlyDeclared=false. But when we display such a type (which is a definition), we don't want to display (T1 Item1, T2 Item2) but rather (T1, T2). We can distinguish that using IsDefinition.
But in VB, all tuple types are definitions, so I'm using the fact that they are not generic to avoid affecting them.

I'll look for VB tests to make sure the scenario you raised is covered.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels to me that relying on IsImplicitlyDeclared to decide whether to include names is fragile and probably a wrong thing to do. It feels like that should be decided on whether explicit names were specified in the tuple type syntax used to "create" the tuple type. That is not the same as IsImplicitlyDeclared, or at least it is no longer the same. We probably should base the logic on a more specific API/information.

In reply to: 426687365 [](ancestors = 426687365,426205983)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth discussing this issue in more depth offline before we decide the course of action

In reply to: 426774418 [](ancestors = 426774418,426687365,426205983)

{
AddSpace();
builder.Add(CreatePart(SymbolDisplayPartKind.FieldName, symbol, element.Name));
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ internal override ITypeSymbolInternal CommonGetWellKnownType(WellKnownType wellk
return GetWellKnownType(wellknownType);
}

internal static Symbol? GetRuntimeMember(NamedTypeSymbol declaringType, in MemberDescriptor descriptor, SignatureComparer<MethodSymbol, FieldSymbol, PropertySymbol, TypeSymbol, ParameterSymbol> comparer, AssemblySymbol accessWithinOpt)
internal static Symbol? GetRuntimeMember(NamedTypeSymbol declaringType, in MemberDescriptor descriptor, SignatureComparer<MethodSymbol, FieldSymbol, PropertySymbol, TypeSymbol, ParameterSymbol> comparer, AssemblySymbol? accessWithinOpt)
{
var members = declaringType.GetMembers(descriptor.Name);
return GetRuntimeMember(members, descriptor, comparer, accessWithinOpt);
Expand Down
24 changes: 22 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
Expand Down Expand Up @@ -399,7 +400,7 @@ public virtual bool IsDefaultTupleElement
{
get
{
return false;
return ContainingType.IsDefinition && TupleElementIndex >= 0;
Copy link
Member

@cston cston May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TupleElementIndex includes this check. #Resolved

}
}

Expand All @@ -424,7 +425,7 @@ public virtual FieldSymbol CorrespondingTupleField
{
get
{
return null;
return (ContainingType.IsDefinition && TupleElementIndex >= 0) ? this : null;
Copy link
Member

@cston cston May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TupleElementIndex includes this check. #Resolved

Copy link
Member

@jaredpar jaredpar May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand All @@ -445,6 +446,25 @@ public virtual int TupleElementIndex
{
get
{
if (ContainingType.IsTupleType && ContainingType.IsDefinition)
Copy link
Member

@cston cston May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
var i = NamedTypeSymbol.MatchesCanonicalElementName(Name);
int itemsPerType = Math.Min(ContainingType.Arity, NamedTypeSymbol.ValueTupleRestPosition - 1);
Copy link
Member

@cston cston May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (i > 0 && i <= itemsPerType)
{
WellKnownMember wellKnownMember = NamedTypeSymbol.GetTupleTypeMember(ContainingType.Arity, i);

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
Copy link
Member

@cston cston May 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


if (found is object)
{
return i - 1;
}
}
}

return -1;
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public TupleFieldSymbol(NamedTypeSymbol container, FieldSymbol underlyingField,
Debug.Assert(container.Equals(underlyingField.ContainingType, TypeCompareKind.IgnoreDynamicAndTupleNames) || this is TupleVirtualElementFieldSymbol,
"virtual fields should be represented by " + nameof(TupleVirtualElementFieldSymbol));

// The fields on definition of ValueTuple<...> types don't need to be wrapped
Debug.Assert(!container.IsDefinition);

_containingTuple = container;
_tupleElementIndex = tupleElementIndex;
}
Expand Down Expand Up @@ -151,6 +154,7 @@ public override sealed bool Equals(Symbol obj, TypeCompareKind compareKind)
/// <summary>
/// Represents an element field of a tuple type (such as (int, byte).Item1)
/// that is backed by a real field with the same name within the tuple underlying type.
/// Note that original tuple fields (like `System.ValueTuple'2.Item1` do not get wrapped.
/// </summary>
internal class TupleElementFieldSymbol : TupleFieldSymbol
{
Expand Down
124 changes: 62 additions & 62 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ internal static int IsTupleElementNameReserved(string name)
return 0;
}

return matchesCanonicalElementName(name);
return MatchesCanonicalElementName(name);

static bool isElementNameForbidden(string name)
{
Expand All @@ -463,26 +463,27 @@ static bool isElementNameForbidden(string name)
return false;
}
}
}

// Returns 3 for "Item3".
// Returns -1 otherwise.
static int matchesCanonicalElementName(string name)
/// <summary>
/// Returns 3 for "Item3".
/// Returns -1 otherwise.
/// </summary>
internal static int MatchesCanonicalElementName(string name)
{
if (name.StartsWith("Item", StringComparison.Ordinal))
{
if (name.StartsWith("Item", StringComparison.Ordinal))
string tail = name.Substring(4);
Copy link
Member

@jaredpar jaredpar May 18, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Substring usage here means that this method will cause a small allocation and this method seems to be in fairly high use with this change. If we were on .NET Core only we could use the following:

ReadOnlySpan<char> tail = name.AsSpan().Slice(4);

Unfortunately there are no overloads of TryParse on netstandard20 which accept ReadOnlySpan<T>. Do we have existing non-allocating int conversion code in Roslyn we could re-use here?

#WontFix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked around but didn't find something better. Not sure I want to add one for this PR, especially since it's a pre-existing usage of Substring :-/

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

if (int.TryParse(tail, out int number))
{
string tail = name.Substring(4);
int number;
if (int.TryParse(tail, out number))
if (number > 0 && string.Equals(name, TupleMemberName(number), StringComparison.Ordinal))
{
if (number > 0 && String.Equals(name, TupleMemberName(number), StringComparison.Ordinal))
{
return number;
}
return number;
}
}

return -1;
}

return -1;
}

/// <summary>
Expand Down Expand Up @@ -601,6 +602,19 @@ public sealed override ImmutableArray<FieldSymbol> TupleElements
continue;
}

if (this.IsDefinition)
{
// No need to wrap fields on System.ValueTuple definitions
members.Add(field);
int tupleFieldIndex2 = currentFieldsForElements.IndexOf(field, ReferenceEqualityComparer.Instance);
Copy link
Member

@cston cston May 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (tupleFieldIndex2 >= 0)
{
elementsMatchedByFields[tupleFieldIndex2] = true; // mark as handled
}

continue;
}

var underlyingField = field is TupleElementFieldSymbol tupleElement ? tupleElement.UnderlyingField.OriginalDefinition : field.OriginalDefinition;
int tupleFieldIndex = currentFieldsForElements.IndexOf(underlyingField, ReferenceEqualityComparer.Instance);
if (underlyingField is TupleErrorFieldSymbol)
Expand All @@ -621,11 +635,7 @@ public sealed override ImmutableArray<FieldSymbol> TupleElements
var providedName = elementNames.IsDefault ? null : elementNames[tupleFieldIndex];

ImmutableArray<Location> locations;
if (this.IsDefinition)
{
locations = member.Locations;
}
else if (elementLocations.IsDefault)
if (elementLocations.IsDefault)
{
locations = ImmutableArray<Location>.Empty;
}
Expand Down Expand Up @@ -1051,57 +1061,47 @@ internal SmallDictionary<Symbol, Symbol> UnderlyingDefinitionToMemberMap
{
get
{
return _lazyUnderlyingDefinitionToMemberMap ??
(_lazyUnderlyingDefinitionToMemberMap = ComputeDefinitionToMemberMap());
}
}
return _lazyUnderlyingDefinitionToMemberMap ??= computeDefinitionToMemberMap();

private SmallDictionary<Symbol, Symbol> ComputeDefinitionToMemberMap()
{
var map = new SmallDictionary<Symbol, Symbol>(ReferenceEqualityComparer.Instance);
var members = TupleUnderlyingType.GetMembers();

// Go in reverse because we want members with default name, which precede the ones with
// friendly names, to be in the map.
for (int i = members.Length - 1; i >= 0; i--)
{
var member = members[i];
switch (member.Kind)
SmallDictionary<Symbol, Symbol> computeDefinitionToMemberMap()
{
case SymbolKind.Method:
case SymbolKind.Property:
case SymbolKind.NamedType:
map.Add(member.OriginalDefinition, member);
break;

case SymbolKind.Field:
var tupleUnderlyingField = ((FieldSymbol)member).TupleUnderlyingField;
if (tupleUnderlyingField is object)
{
map[tupleUnderlyingField.OriginalDefinition] = member;
}
break;
var map = new SmallDictionary<Symbol, Symbol>(ReferenceEqualityComparer.Instance);
var members = TupleUnderlyingType.GetMembers();

case SymbolKind.Event:
var underlyingEvent = (EventSymbol)member;
var underlyingAssociatedField = underlyingEvent.AssociatedField;
// The field is not part of the members list
if (underlyingAssociatedField is object)
// Go in reverse because we want members with default name, which precede the ones with
// friendly names, to be in the map.
for (int i = members.Length - 1; i >= 0; i--)
{
var member = members[i];
switch (member.Kind)
{
Debug.Assert((object)underlyingAssociatedField.ContainingSymbol == TupleUnderlyingType);
Debug.Assert(TupleUnderlyingType.GetMembers(underlyingAssociatedField.Name).IndexOf(underlyingAssociatedField) < 0);
map.Add(underlyingAssociatedField.OriginalDefinition, new TupleFieldSymbol(TupleUnderlyingType, underlyingAssociatedField, -i - 1));
Copy link
Contributor

@AlekseyTs AlekseyTs May 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map.Add(underlyingAssociatedField.OriginalDefinition, new TupleFieldSymbol(TupleUnderlyingType, underlyingAssociatedField, -i - 1)); [](start = 32, length = 132)

It is not clear why this is no longer needed. We don't need/want to create an instance of a TupleFieldSymbol, but it feel like we still want an entry in the map if we wanted to have it in the map before this PR. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put the logic back for completeness, but the only place we use this is in TupleFieldSymbol.AssociatedSymbol (I don't think event backing fields would use it).

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

case SymbolKind.Method:
case SymbolKind.Property:
case SymbolKind.NamedType:
map.Add(member.OriginalDefinition, member);
break;

case SymbolKind.Field:
var tupleUnderlyingField = ((FieldSymbol)member).TupleUnderlyingField;
if (tupleUnderlyingField is object)
{
map[tupleUnderlyingField.OriginalDefinition] = member;
}
break;

case SymbolKind.Event:
var underlyingEvent = (EventSymbol)member;
map.Add(underlyingEvent.OriginalDefinition, member);
break;

default:
throw ExceptionUtilities.UnexpectedValue(member.Kind);
}
}

map.Add(underlyingEvent.OriginalDefinition, member);
break;

default:
throw ExceptionUtilities.UnexpectedValue(member.Kind);
return map;
}
}

return map;
}

public TMember? GetTupleMemberSymbolForUnderlyingMember<TMember>(TMember underlyingMemberOpt) where TMember : Symbol
Expand Down
Loading