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

Jump to bottom

Tuple and underlying type unification. Part 1. #11006

Merged
merged 3 commits into from
May 3, 2016
Merged

Tuple and underlying type unification. Part 1. #11006

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eda603e
Extract common code into Wrapper...Symbol classes.
AlekseyTs Apr 25, 2016
dad5ac2
Tuple and underlying type unification. Part 1.
AlekseyTs Apr 30, 2016
1a6fce1
Move TupleTypeSymbol.cs and TupleFieldSymbol.cs into Tuples folder.
AlekseyTs May 1, 2016
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
13 changes: 4 additions & 9 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,26 +344,21 @@ private BoundExpression CreateTupleConversion(CSharpSyntaxNode syntax, BoundTupl

NamedTypeSymbol targetType = (NamedTypeSymbol)destinationWithoutNullable;

NamedTypeSymbol targetUnderlyingType;
if (targetType.IsTupleType)
{
var destTupleType = (TupleTypeSymbol)targetType;
targetUnderlyingType = destTupleType.UnderlyingTupleType;
// do not lose the original element names in the literal if different from names in the target

// PROTOTYPE(tuples): Come back to this, what about locations?
Copy link
Member

@gafter gafter May 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If this will be targeted to the future branch, this needs to be resolved. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not targeted to the future.

targetType = destTupleType.WithElementNames(sourceTuple.ArgumentNamesOpt);
Copy link
Member

@gafter gafter May 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This code appears to be changing the destination type. It results in a bound expression whose type isn't always the provided "destination" type. Because it doesn't follow the usual pattern of a conversion resulting in an expression of the converted-to type, I don't see how that fits into the usual type and conversion machinery. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

Resolved by recording open issue #11013

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it doesn't. This is on my list to look at once symbol model is in the state we would like it to be. This activity is out of scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

That is for cases like

( (long a, long b) ) ( c: 1, d: 2 );

The literal is target-typed to (long c, long d), because target-typing of elements affects only types, not names. I think that was the idea. We can change that.

Then there is a conversion node that identity-converts to (long a, long b).
The result is (long a, long b)

}
else
{
targetUnderlyingType = targetType;
}

var arguments = sourceTuple.Arguments;
Debug.Assert(Compilation.IsWellKnownTupleType(targetUnderlyingType, arguments.Length), "converting a tuple literal to incompatible type?");

var convertedArguments = ArrayBuilder<BoundExpression>.GetInstance(arguments.Length);
var targetElementTypes = ArrayBuilder<TypeSymbol>.GetInstance(arguments.Length);

TupleTypeSymbol.AddElementTypes(targetUnderlyingType, targetElementTypes);
TupleTypeSymbol.AddElementTypes(targetType, targetElementTypes);
Debug.Assert(targetElementTypes.Count == arguments.Length, "converting a tuple literal to incompatible type?");

for (int i = 0; i < arguments.Length; i++)
{
Expand Down
32 changes: 18 additions & 14 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,14 +666,26 @@ private BoundExpression BindTupleExpression(TupleExpressionSyntax node, Diagnost

var boundArguments = ArrayBuilder<BoundExpression>.GetInstance(arguments.Count);
var elementTypes = ArrayBuilder<TypeSymbol>.GetInstance(arguments.Count);
var elementLocations = ArrayBuilder<Location>.GetInstance(arguments.Count);
ArrayBuilder<string> elementNames = null;
int countOfExplicitNames = 0;

// prepare and check element names and types
for (int i = 0, l = numElements; i < l; i++)
{
ArgumentSyntax argumentSyntax = arguments[i];
string name = argumentSyntax.NameColon?.Name?.Identifier.ValueText;
string name = null;
IdentifierNameSyntax nameSyntax = argumentSyntax.NameColon?.Name;

if (nameSyntax != null)
{
name = nameSyntax.Identifier.ValueText;
elementLocations.Add(nameSyntax.Location);
}
Copy link
Member

Choose a reason for hiding this comment

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

CheckTupleMemberName could be called in this branch, rather than in separate check below. This would be similar to the change below, in BindTupleType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make the change in the next PR.

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

else
{
elementLocations.Add(argumentSyntax.Location);
}

// validate name if we have one
if (name != null)
Expand Down Expand Up @@ -711,12 +723,13 @@ private BoundExpression BindTupleExpression(TupleExpressionSyntax node, Diagnost
default(ImmutableArray<string>) :
elementNames.ToImmutableAndFree();

TupleTypeSymbol tupleTypeOpt = null;
NamedTypeSymbol tupleTypeOpt = null;
var elements = elementTypes.ToImmutableAndFree();
var locations = elementLocations.ToImmutableAndFree();

if (hasNaturalType)
{
tupleTypeOpt = TupleTypeSymbol.Create(elements, elementNamesArray, this.Compilation, node, diagnostics);
tupleTypeOpt = TupleTypeSymbol.Create(node.Location, elements, locations, elementNamesArray, this.Compilation, node, diagnostics);
}

return new BoundTupleLiteral(node, elementNamesArray, boundArguments.ToImmutableAndFree(), tupleTypeOpt, hasErrors);
Expand Down Expand Up @@ -3929,15 +3942,6 @@ private bool IsConstructorAccessible(MethodSymbol constructor, ref HashSet<Diagn
BoundExpression boundInitializerOpt = null)
{

var typeContainingConstructors = type;
if (type.IsTupleType)
{
// in the currentl model (which is subject to change)
// tuple "inherits" all the members of the underlying type for lookup purposes
// including constructors.
typeContainingConstructors = ((TupleTypeSymbol)type).UnderlyingTupleType;
}

BoundExpression result = null;
bool hasErrors = type.IsErrorType();
if (type.IsAbstract)
Expand All @@ -3957,7 +3961,7 @@ private bool IsConstructorAccessible(MethodSymbol constructor, ref HashSet<Diagn
if (analyzedArguments.HasDynamicArgument)
{
OverloadResolutionResult<MethodSymbol> overloadResolutionResult = OverloadResolutionResult<MethodSymbol>.GetInstance();
this.OverloadResolution.ObjectCreationOverloadResolution(GetAccessibleConstructorsForOverloadResolution(typeContainingConstructors, ref useSiteDiagnostics), analyzedArguments, overloadResolutionResult, ref useSiteDiagnostics);
this.OverloadResolution.ObjectCreationOverloadResolution(GetAccessibleConstructorsForOverloadResolution(type, ref useSiteDiagnostics), analyzedArguments, overloadResolutionResult, ref useSiteDiagnostics);
diagnostics.Add(node, useSiteDiagnostics);
useSiteDiagnostics = null;

Expand Down Expand Up @@ -3989,7 +3993,7 @@ private bool IsConstructorAccessible(MethodSymbol constructor, ref HashSet<Diagn
ImmutableArray<MethodSymbol> candidateConstructors;

if (TryPerformConstructorOverloadResolution(
typeContainingConstructors,
type,
analyzedArguments,
typeName,
typeNode.Location,
Expand Down
7 changes: 0 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,13 +715,6 @@ internal virtual bool SupportsExtensionMethods
break;
}

var tuple = currentType as TupleTypeSymbol;
if ((object)tuple != null)
{
currentType = tuple.UnderlyingTupleType;
continue;
}

if (basesBeingResolved != null && basesBeingResolved.ContainsReference(type.OriginalDefinition))
{
var other = GetNearestOtherSymbol(basesBeingResolved, type);
Expand Down
33 changes: 24 additions & 9 deletions src/Compilers/CSharp/Portable/Binder/Binder_Symbols.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ private TypeSymbol BindTupleType(TupleTypeSyntax syntax, DiagnosticBag diagnosti
{
int numElements = syntax.Elements.Count;
var types = ArrayBuilder<TypeSymbol>.GetInstance(numElements);
var locations = ArrayBuilder<Location>.GetInstance(numElements);
ArrayBuilder<string> elementNames = null;

// set of names already used
Expand All @@ -417,16 +418,26 @@ private TypeSymbol BindTupleType(TupleTypeSyntax syntax, DiagnosticBag diagnosti
Error(diagnostics, ErrorCode.ERR_FieldCantBeRefAny, argumentSyntax, argumentType);
}

string name = argumentSyntax.Name?.Identifier.ValueText;
string name = null;
IdentifierNameSyntax nameSyntax = argumentSyntax.Name;

// validate name if we have one
if (name != null)
if (nameSyntax != null)
{
name = nameSyntax.Identifier.ValueText;

// validate name if we have one
countOfExplicitNames++;
CheckTupleMemberName(name, i, argumentSyntax.Name, diagnostics, uniqueFieldNames);
CheckTupleMemberName(name, i, nameSyntax, diagnostics, uniqueFieldNames);
locations.Add(nameSyntax.Location);
}
else
{
locations.Add(argumentSyntax.Location);
}

CollectTupleFieldMemberNames(name, i + 1, numElements, ref elementNames);
}

uniqueFieldNames.Free();

if (countOfExplicitNames != 0 && countOfExplicitNames != numElements)
Expand All @@ -435,18 +446,22 @@ private TypeSymbol BindTupleType(TupleTypeSyntax syntax, DiagnosticBag diagnosti
}

ImmutableArray<TypeSymbol> typesArray = types.ToImmutableAndFree();
ImmutableArray<Location> locationsArray = locations.ToImmutableAndFree();

if (typesArray.Length < 2)
{
return new ExtendedErrorTypeSymbol(this.Compilation.Assembly.GlobalNamespace, LookupResultKind.NotCreatable, diagnostics.Add(ErrorCode.ERR_TupleTooFewElements, syntax.Location));
}

return TupleTypeSymbol.Create(typesArray,
elementNames == null ?
return TupleTypeSymbol.Create(syntax.Location,
typesArray,
locationsArray,
elementNames == null ?
default(ImmutableArray<string>) :
elementNames.ToImmutableAndFree(),
this.Compilation,
syntax,
diagnostics);
this.Compilation,
syntax,
diagnostics);
}

private static void CollectTupleFieldMemberNames(string name, int position, int tupleSize, ref ArrayBuilder<string> elementNames)
Expand Down
11 changes: 3 additions & 8 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,14 +709,9 @@ protected override bool HasImplicitTupleConversion(BoundExpression source, TypeS
var tupleExpression = (BoundTupleLiteral)source;
var arguments = tupleExpression.Arguments;

// unwrap tuple to its underlying
if (destination.IsTupleType)
{
destination = ((TupleTypeSymbol)destination).UnderlyingTupleType;
}

// check if underlying type is actually a possible underlying type for a tuple of given arity
if(!Compilation.IsWellKnownTupleType(destination, arguments.Length))
// check if the type is actually compatible type for a tuple of given cardinality
int cardinality;
if (!destination.IsTupleCompatible(out cardinality) || cardinality != arguments.Length)
Copy link
Member

@VSadov VSadov May 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This seems to be a common check. Should it be a helper?
And you would not need to declare a temp for cardinality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll consider this for the next change.

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

{
return false;
}
Expand Down
27 changes: 9 additions & 18 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,16 +650,11 @@ private void MakeExplicitParameterTypeInferences(Binder binder, BoundTupleLitera
}

var destination = (NamedTypeSymbol)target;

if (destination.IsTupleType)
{
destination = ((TupleTypeSymbol)destination).UnderlyingTupleType;
}

var sourceArguments = argument.Arguments;

// check if underlying type is actually a possible underlying type for a tuple of given arity
if (!binder.Compilation.IsWellKnownTupleType(destination, sourceArguments.Length))
// check if the type is actually compatible type for a tuple of given cardinality
int cardinality;
if (!destination.IsTupleCompatible(out cardinality) || cardinality != sourceArguments.Length)
{
// target is not a tuple of appropriate shape
return;
Expand Down Expand Up @@ -839,16 +834,12 @@ private void MakeOutputTypeInferences(Binder binder, BoundTupleLiteral argument,

var destination = (NamedTypeSymbol)formalType;

if (destination.IsTupleType)
{
destination = ((TupleTypeSymbol)destination).UnderlyingTupleType;
}

Debug.Assert(argument.Type == null, "should not need to dig into elements if tuple has natural type");
var sourceArguments = argument.Arguments;

// check if underlying type is actually a possible underlying type for a tuple of given arity
if (!binder.Compilation.IsWellKnownTupleType(destination, sourceArguments.Length))
// check if the type is actually compatible type for a tuple of given cardinality
int cardinality;
if (!destination.IsTupleCompatible(out cardinality) || cardinality != sourceArguments.Length)
{
return;
}
Expand Down Expand Up @@ -1618,13 +1609,13 @@ private bool ExactTupleInference(TypeSymbol source, TypeSymbol target, ref HashS
bool hasTuples = false;
if (source.IsTupleType)
{
source = ((TupleTypeSymbol)source).UnderlyingTupleType;
source = source.TupleUnderlyingType;
hasTuples = true;
}

if (target.IsTupleType)
{
target = ((TupleTypeSymbol)target).UnderlyingTupleType;
target = target.TupleUnderlyingType;
hasTuples = true;
}

Expand Down Expand Up @@ -2489,7 +2480,7 @@ private bool Fix(int iParam, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
{
if (candidate.IsTupleType)
{
best = ((TupleTypeSymbol)candidate).UnderlyingTupleType;
best = candidate.TupleUnderlyingType;
break;
}

Expand Down
10 changes: 3 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1950,16 +1950,12 @@ private bool ExpressionMatchExactly(BoundTupleLiteral tupleSource, TypeSymbol ta

var destination = (NamedTypeSymbol)targetType;

if (destination.IsTupleType)
{
destination = ((TupleTypeSymbol)destination).UnderlyingTupleType;
}

Debug.Assert((object)tupleSource.Type == null, "should not need to dig into elements if tuple has natural type");
var sourceArguments = tupleSource.Arguments;

// check if underlying type is actually a possible underlying type for a tuple of given arity
if (!Compilation.IsWellKnownTupleType(destination, sourceArguments.Length))
// check if the type is actually compatible type for a tuple of given cardinality
int cardinality;
if (!destination.IsTupleCompatible(out cardinality) || cardinality != sourceArguments.Length)
{
return false;
}
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpCodeAnalysis.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,9 @@
<Compile Include="Symbols\AnonymousTypes\SynthesizedSymbols\AnonymousType.ToStringMethodSymbol.cs" />
<Compile Include="Symbols\AnonymousTypes\SynthesizedSymbols\AnonymousType.TypeParameterSymbol.cs" />
<Compile Include="Symbols\TupleFieldSymbol.cs" />
<Compile Include="Symbols\Tuples\TupleMethodSymbol.cs" />
<Compile Include="Symbols\Tuples\TupleParameterSymbol.cs" />
<Compile Include="Symbols\Tuples\TupleErrorFieldSymbol.cs" />
<Compile Include="Symbols\TupleTypeSymbol.cs" />
<Compile Include="Symbols\ArrayTypeSymbol.cs" />
<Compile Include="Symbols\AssemblySymbol.cs" />
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2831,7 +2831,7 @@ protected override INamedTypeSymbol CommonCreateTupleTypeSymbol(ImmutableArray<I
typesBuilder.Add(elementTypes[i].EnsureCSharpSymbolOrNull<ITypeSymbol, TypeSymbol>($"{nameof(elementTypes)}[{i}]"));
}

return TupleTypeSymbol.Create(typesBuilder.ToImmutableAndFree(), elementNames, this);
return TupleTypeSymbol.Create(null, typesBuilder.ToImmutableAndFree(), default(ImmutableArray<Location>), elementNames, this);
Copy link
Member

Choose a reason for hiding this comment

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

Consider using the parameter name to clarify what is the null for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather add a comment, too many names to add. Next PR.

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

}

protected override ITypeSymbol CommonDynamicType
Expand Down
16 changes: 12 additions & 4 deletions src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ protected virtual Cci.IModuleReference TranslateModule(ModuleSymbol module, Diag
}
else if (namedTypeSymbol.IsTupleType)
{
namedTypeSymbol = ((TupleTypeSymbol)namedTypeSymbol).UnderlyingTupleType;
namedTypeSymbol = namedTypeSymbol.TupleUnderlyingType;
}

// Substitute error types with a special singleton object.
Expand Down Expand Up @@ -929,8 +929,12 @@ internal static Cci.IGenericParameterReference Translate(TypeParameterSymbol par
DiagnosticBag diagnostics,
bool needDeclaration = false)
{
if (fieldSymbol.IsTupleField)
Copy link
Member

Choose a reason for hiding this comment

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

Do we expect unlowered tuple field accesses in emit, or this is "just in case"

Copy link
Member

Choose a reason for hiding this comment

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

Just curious. It is not necessarily a bad idea.
I'd be worried if tuple members require special handling in lambdas and such. If no, then it seems ok to lower them in Emit.

Copy link
Member

Choose a reason for hiding this comment

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

Also, since access to elements 7+ need to be lowered anyways, perhaps it is better to just require them all be rewritten by the time we are in Emit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not required at the moment, I believe, but this is a correct way to deal with them in emit.

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

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be an assert. If we forget to lower field accesses in some context (we had such bug in object initializers), it is better if assert below is triggered right away, than a situation with a scenario working for small tuples and not working with big tuples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the next PR

In reply to: 61801140 [](ancestors = 61801140,61800802)

{
fieldSymbol = fieldSymbol.TupleUnderlyingField;
}

Debug.Assert(fieldSymbol.IsDefinitionOrDistinct());
Debug.Assert(!fieldSymbol.ContainingType.IsTupleType, "tuple fields should be rewritten to underlying by now");

if (!fieldSymbol.IsDefinition)
{
Expand Down Expand Up @@ -1085,13 +1089,17 @@ internal override Cci.IMethodReference Translate(MethodSymbol symbol, Diagnostic
Cci.IMethodReference methodRef;
NamedTypeSymbol container = methodSymbol.ContainingType;

Debug.Assert(methodSymbol.IsDefinitionOrDistinct());

// Method of anonymous type being translated
if (container.IsAnonymousType)
{
methodSymbol = AnonymousTypeManager.TranslateAnonymousTypeMethodSymbol(methodSymbol);
}
else if (methodSymbol.IsTupleMethod)
{
methodSymbol = methodSymbol.TupleUnderlyingMethod;
}

Debug.Assert(methodSymbol.IsDefinitionOrDistinct());

if (!methodSymbol.IsDefinition)
{
Expand Down
Loading