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.

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

Implement definite assignment analysis for tuples #13107

Merged
merged 4 commits into from
Aug 17, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 53 additions & 0 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,8 @@ private void NoteWrite(BoundExpression n, BoundExpression value, bool read)
/// </summary>
protected int VariableSlot(Symbol symbol, int containingSlot = 0)
{
containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, forceContainingSlotsToExist: false);

int slot;
return (_variableSlot.TryGetValue(new VariableIdentifier(symbol, containingSlot), out slot)) ? slot : -1;
}
Expand All @@ -674,6 +676,9 @@ protected int VariableSlot(Symbol symbol, int containingSlot = 0)
protected int GetOrCreateSlot(Symbol symbol, int containingSlot = 0)
{
if (symbol is RangeVariableSymbol) return -1;

containingSlot = DescendThroughTupleRestFields(ref symbol, containingSlot, forceContainingSlotsToExist: true);

VariableIdentifier identifier = new VariableIdentifier(symbol, containingSlot);
int slot;

Expand All @@ -700,6 +705,54 @@ protected int GetOrCreateSlot(Symbol symbol, int containingSlot = 0)
return slot;
}

/// <summary>
/// Descends through Rest fields of a tuple if "symbol" is an extended field
/// As a result the "symbol" will be adjusted to be the field of the innermost tuple
/// and a corresponding containingSlot is returned.
/// Return value -1 indicates a failure which could happen for the following reasons
/// a) Rest field does not exist, which could happen in rare error scenarios involving broken ValueTuple types
/// b) Rest is not tracked already and forceSlotsToExist is false (otherwise we create slots on demand)
/// </summary>
private int DescendThroughTupleRestFields(ref Symbol symbol, int containingSlot, bool forceContainingSlotsToExist)
{
var fieldSymbol = symbol as TupleFieldSymbol;
if ((object)fieldSymbol != null)
{
TypeSymbol containingType = ((TupleTypeSymbol)symbol.ContainingType).UnderlyingNamedType;

// for tuple fields the variable identifier represents the underlying field
symbol = fieldSymbol.TupleUnderlyingField;

// descend through Rest fields
// force corresponding slots if do not exist
while (containingType != symbol.ContainingType)
{
var restField = containingType.GetMembers(TupleTypeSymbol.RestFieldName).FirstOrDefault() as FieldSymbol;
if ((object)restField == null)
{
return -1;
Copy link
Member

@jcouv jcouv Aug 16, 2016

Choose a reason for hiding this comment

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

Is it possible to hit this case?
Nevermind, I was thinking about a field that is not a FieldSymbol, but really this is for missing Rest field. I see the test case for that.

}

if (forceContainingSlotsToExist)
{
containingSlot = GetOrCreateSlot(restField, containingSlot);
}
else
{
if (!_variableSlot.TryGetValue(new VariableIdentifier(restField, containingSlot), out containingSlot))
{
return -1;
}
}

containingType = restField.Type.TupleUnderlyingTypeOrSelf();
}
}

return containingSlot;
}


private void Normalize(ref LocalState state)
{
int oldNext = state.Assigned.Capacity;
Expand Down
13 changes: 11 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,8 @@ private bool CheckStructInstanceFields(ConsList<NamedTypeSymbol> typesWithMember

if ((object)field != null)
{
var actualFiledType = field.Type;
if (!IsEmptyStructType(actualFiledType, typesWithMembersOfThisType))
var actualFieldType = field.Type;
if (!IsEmptyStructType(actualFieldType, typesWithMembersOfThisType))
{
return false;
}
Expand Down Expand Up @@ -167,6 +167,15 @@ private FieldSymbol GetActualInstanceField(Symbol member, NamedTypeSymbol type)
{
case SymbolKind.Field:
var field = (FieldSymbol)member;

// Do not report virtual tuple fields.
// They are additional aliases to the fields of the underlying struct or nested extensions.
// and as such are already accounted for via the nonvirtual fields.
if (field.IsVirtualTupleField)
{
return null;
}

return (field.IsFixed || ShouldIgnoreStructField(field, field.Type)) ? null : field.AsMember(type);

case SymbolKind.Event:
Expand Down
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,17 @@ public virtual bool IsTupleField
}
}

/// <summary>
/// Returns True when field symbol is not mapped directly to a field in the underlying tuple struct.
/// </summary>
internal virtual bool IsVirtualTupleField
{
get
{
return false;
}
}

/// <summary>
/// If this is a field of a tuple type, return corresponding underlying field from the
/// tuple underlying type. Otherwise, null. In case of a malformed underlying type
Expand Down
55 changes: 42 additions & 13 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ internal class TupleFieldSymbol : WrappedFieldSymbol
protected readonly TupleTypeSymbol _containingTuple;

/// <summary>
/// If this field represents a tuple element (including the name match),
/// If this field represents a tuple element,
/// id is an index of the element (zero-based).
/// Otherwise, (-1 - [index in members array]);
/// </summary>
Expand All @@ -24,6 +24,9 @@ internal class TupleFieldSymbol : WrappedFieldSymbol
public TupleFieldSymbol(TupleTypeSymbol container, FieldSymbol underlyingField, int tupleFieldId)
: base(underlyingField)
{
Debug.Assert(container.UnderlyingNamedType.Equals(underlyingField.ContainingType, TypeCompareKind.IgnoreDynamicAndTupleNames) || this is TupleVirtualElementFieldSymbol,
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer we avoid depending on the identity of particular internal implementation classes. Can this is TupleVirtualElementFieldSymbol be replaced by this.IsVirtualTupleField?

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of this assert is to actually check the internal implementation. I think it is ok to be strict about types here.

"virtual fields should be represented by " + nameof(TupleVirtualElementFieldSymbol));

_containingTuple = container;
_tupleFieldId = tupleFieldId;
}
Expand All @@ -37,7 +40,7 @@ public override bool IsTupleField
}

/// <summary>
/// If this field represents a tuple element (including the name match),
/// If this field represents a tuple element,
/// id is an index of the element (zero-based).
/// Otherwise, (-1 - [index in members array]);
/// </summary>i
Expand Down Expand Up @@ -100,22 +103,27 @@ internal override DiagnosticInfo GetUseSiteDiagnostic()

public override sealed int GetHashCode()
{
return Hash.Combine(_containingTuple.GetHashCode(), _tupleFieldId.GetHashCode());
return Hash.Combine(
Hash.Combine(_containingTuple.GetHashCode(), _tupleFieldId.GetHashCode()),
this.Name.GetHashCode());
}

public override sealed bool Equals(object obj)
{
return Equals(obj as TupleFieldSymbol);
}
var other = obj as TupleFieldSymbol;

public bool Equals(TupleFieldSymbol other)
{
if ((object)other == this)
{
return true;
}

return (object)other != null && _tupleFieldId == other._tupleFieldId && _containingTuple == other._containingTuple;
// note we have to compare both index and name because
// in nameless tuple there could be fields that differ only by index
// and in named tupoles there could be fields that differ only by name
return (object)other != null &&
_tupleFieldId == other.TupleElementIndex &&
_containingTuple == other._containingTuple &&
this.Name == other.Name;
}
}

Expand Down Expand Up @@ -185,18 +193,31 @@ public override Symbol AssociatedSymbol
}

/// <summary>
/// Represents an element field of a tuple type (such as (int a, byte b).a, or (int a, byte b).b)
/// that is backed by a real field with a different name within the tuple underlying type.
/// Represents an element field of a tuple type that is not backed by a real field
/// with the same name within the tuple underlying type.
///
/// Examples
/// // alias to Item1 with a different name
/// (int a, byte b).a
///
/// // not backed directly by the underlying type
/// (int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8).i8
///
/// NOTE: For any virtual element, there is a nonvirtual way to access the same underlying field.
/// In scenarios where we need to enumerate actual fields of a struct,
/// virtual fields should be ignored.
/// </summary>
internal sealed class TupleRenamedElementFieldSymbol : TupleElementFieldSymbol
internal sealed class TupleVirtualElementFieldSymbol : TupleElementFieldSymbol
{
private readonly string _name;

public TupleRenamedElementFieldSymbol(TupleTypeSymbol container, FieldSymbol underlyingField, string name, int tupleElementOrdinal, Location location)
public TupleVirtualElementFieldSymbol(TupleTypeSymbol container, FieldSymbol underlyingField, string name, int tupleElementOrdinal, Location location)
: base(container, underlyingField, tupleElementOrdinal, location)
{
Debug.Assert(name != null);
Debug.Assert(name != underlyingField.Name);
Debug.Assert(name != underlyingField.Name || !container.UnderlyingNamedType.Equals(underlyingField.ContainingType, TypeCompareKind.IgnoreDynamicAndTupleNames),
"fields that map directly to underlying should not be represented by " + nameof(TupleVirtualElementFieldSymbol));

_name = name;
}

Expand All @@ -223,5 +244,13 @@ public override Symbol AssociatedSymbol
return null;
}
}

internal override bool IsVirtualTupleField
{
get
{
return true;
}
}
}
}
36 changes: 20 additions & 16 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ internal sealed class TupleTypeSymbol : WrappedNamedTypeSymbol

internal const int RestPosition = 8; // The Rest field is in 8th position
internal const string TupleTypeName = "ValueTuple";
internal const string RestFieldName = "Rest";

private TupleTypeSymbol(Location locationOpt, NamedTypeSymbol underlyingType, ImmutableArray<Location> elementLocations, ImmutableArray<string> elementNames, ImmutableArray<TypeSymbol> elementTypes)
: this(locationOpt == null ? ImmutableArray<Location>.Empty : ImmutableArray.Create(locationOpt),
Expand Down Expand Up @@ -555,6 +556,15 @@ internal static int IsElementNameReserved(string name)
return 0;
}

return MatchesCanonicalElementName(name);
}

/// <summary>
/// Returns 3 for "Item3".
/// Returns -1 otherwise.
/// </summary>
private static int MatchesCanonicalElementName(string name)
{
if (name.StartsWith("Item", StringComparison.Ordinal))
{
string tail = name.Substring(4);
Expand Down Expand Up @@ -723,13 +733,13 @@ private ImmutableArray<FieldSymbol> CollectTupleElementFields()
continue;
}

int index = (member as TupleFieldSymbol)?.TupleElementIndex ??
((TupleErrorFieldSymbol)member).TupleElementIndex;
var field = (FieldSymbol)member;
var index = field.TupleElementIndex;

if (index >= 0)
if (index >= 0 && index == MatchesCanonicalElementName(field.Name) - 1)
{
Debug.Assert((object)builder[index] == null);
builder[index] = (FieldSymbol)member;
builder[index] = field;
}
}

Expand Down Expand Up @@ -813,21 +823,15 @@ private ImmutableArray<Symbol> CreateMembers()
if (namesOfVirtualFields[tupleFieldIndex] != defaultName)
{
// The name given doesn't match default name Item8, etc.
members.Add(new TupleRenamedElementFieldSymbol(this, fieldSymbol, defaultName, -members.Count - 1, null));
// Add a field with default name and make it virtual since we are not at the top level
members.Add(new TupleVirtualElementFieldSymbol(this, fieldSymbol, defaultName, tupleFieldIndex, null));
}

// Add a field with the given name
var location = _elementLocations.IsDefault ? null : _elementLocations[tupleFieldIndex];
members.Add(new TupleVirtualElementFieldSymbol(this, fieldSymbol, namesOfVirtualFields[tupleFieldIndex],
tupleFieldIndex, location));

if (field.Name == namesOfVirtualFields[tupleFieldIndex])
{
members.Add(new TupleElementFieldSymbol(this, fieldSymbol, tupleFieldIndex, location));
}
else
{
members.Add(new TupleRenamedElementFieldSymbol(this, fieldSymbol, namesOfVirtualFields[tupleFieldIndex],
tupleFieldIndex, location));
}
}
else if (field.Name == namesOfVirtualFields[tupleFieldIndex])
{
Expand All @@ -837,12 +841,12 @@ private ImmutableArray<Symbol> CreateMembers()
else
{
// Add a field with default name
members.Add(new TupleFieldSymbol(this, fieldSymbol, -members.Count - 1));
members.Add(new TupleFieldSymbol(this, fieldSymbol, tupleFieldIndex));

// Add a field with the given name
if (namesOfVirtualFields[tupleFieldIndex] != null)
{
members.Add(new TupleRenamedElementFieldSymbol(this, fieldSymbol, namesOfVirtualFields[tupleFieldIndex], tupleFieldIndex,
members.Add(new TupleVirtualElementFieldSymbol(this, fieldSymbol, namesOfVirtualFields[tupleFieldIndex], tupleFieldIndex,
_elementLocations.IsDefault ? null : _elementLocations[tupleFieldIndex]));
}
}
Expand Down
Loading