-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Tuple and underlying type unification. Part 1. #11006
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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? | ||
targetType = destTupleType.WithElementNames(sourceTuple.ArgumentNamesOpt); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
} | ||
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++) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
else | ||
{ | ||
elementLocations.Add(argumentSyntax.Location); | ||
} | ||
|
||
// validate name if we have one | ||
if (name != null) | ||
|
@@ -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); | ||
|
@@ -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) | ||
|
@@ -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; | ||
|
||
|
@@ -3989,7 +3993,7 @@ private bool IsConstructorAccessible(MethodSymbol constructor, ref HashSet<Diagn | |
ImmutableArray<MethodSymbol> candidateConstructors; | ||
|
||
if (TryPerformConstructorOverloadResolution( | ||
typeContainingConstructors, | ||
type, | ||
analyzedArguments, | ||
typeName, | ||
typeNode.Location, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be a common check. Should it be a helper? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
return false; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using the parameter name to clarify what is the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -929,8 +929,12 @@ internal static Cci.IGenericParameterReference Translate(TypeParameterSymbol par | |
DiagnosticBag diagnostics, | ||
bool needDeclaration = false) | ||
{ | ||
if (fieldSymbol.IsTupleField) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious. It is not necessarily a bad idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
fieldSymbol = fieldSymbol.TupleUnderlyingField; | ||
} | ||
|
||
Debug.Assert(fieldSymbol.IsDefinitionOrDistinct()); | ||
Debug.Assert(!fieldSymbol.ContainingType.IsTupleType, "tuple fields should be rewritten to underlying by now"); | ||
|
||
if (!fieldSymbol.IsDefinition) | ||
{ | ||
|
@@ -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) | ||
{ | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.