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

Invert the binding order of InitializerExpressions and CreationExpressions #30805

Merged
merged 4 commits into from
Oct 30, 2018
Merged

Invert the binding order of InitializerExpressions and CreationExpressions #30805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7c8473
Invert the order of binding for InitializerExpressions and CreationEx…
chsienki Oct 26, 2018
9f083e1
Pass through the original type for class/pointer intialization binding
chsienki Oct 29, 2018
d7a285a
PR Feedback:
chsienki Oct 29, 2018
bf2e893
Simply test code.
chsienki Oct 29, 2018
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
85 changes: 55 additions & 30 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 @@ -3703,36 +3703,29 @@ protected BoundExpression BindObjectCreationExpression(ObjectCreationExpressionS
{
var typeWithAnnotations = BindType(node.Type, diagnostics);
var type = typeWithAnnotations.TypeSymbol;
var originalType = type;

if (typeWithAnnotations.IsAnnotated && !type.IsNullableType())
{
diagnostics.Add(ErrorCode.ERR_AnnotationDisallowedInObjectCreation, node.Location, type);
}

BoundObjectInitializerExpressionBase boundInitializerOpt = node.Initializer == null ?
null :
BindInitializerExpression(
syntax: node.Initializer,
type: type,
typeSyntax: node.Type,
diagnostics: diagnostics);

switch (type.TypeKind)
{
case TypeKind.Struct:
case TypeKind.Class:
case TypeKind.Enum:
case TypeKind.Error:
return BindClassCreationExpression(node, (NamedTypeSymbol)type, GetName(node.Type), boundInitializerOpt, diagnostics);
return BindClassCreationExpression(node, (NamedTypeSymbol)type, GetName(node.Type), diagnostics, originalType);

case TypeKind.Delegate:
return BindDelegateCreationExpression(node, (NamedTypeSymbol)type, diagnostics);

case TypeKind.Interface:
return BindInterfaceCreationExpression(node, (NamedTypeSymbol)type, boundInitializerOpt, diagnostics);
return BindInterfaceCreationExpression(node, (NamedTypeSymbol)type, diagnostics);

case TypeKind.TypeParameter:
return BindTypeParameterCreationExpression(node, (TypeParameterSymbol)type, boundInitializerOpt, diagnostics);
return BindTypeParameterCreationExpression(node, (TypeParameterSymbol)type, diagnostics);

case TypeKind.Submission:
// script class is synthesized and should not be used as a type of a new expression:
Expand Down Expand Up @@ -3930,7 +3923,7 @@ private BoundExpression BindDelegateCreationExpression(ObjectCreationExpressionS
}
}

private BoundExpression BindClassCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol type, string typeName, BoundObjectInitializerExpressionBase boundInitializerOpt, DiagnosticBag diagnostics)
private BoundExpression BindClassCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol type, string typeName, DiagnosticBag diagnostics, TypeSymbol initializerType = null)
{
// Get the bound arguments and the argument names.
AnalyzedArguments analyzedArguments = AnalyzedArguments.GetInstance();
Expand All @@ -3944,29 +3937,33 @@ private BoundExpression BindClassCreationExpression(ObjectCreationExpressionSynt
if (type.IsStatic)
{
diagnostics.Add(ErrorCode.ERR_InstantiatingStaticClass, node.Location, type);
return MakeBadExpressionForObjectCreation(node, type, boundInitializerOpt, analyzedArguments);
return MakeBadExpressionForObjectCreation(node, type, analyzedArguments, diagnostics);
}
else if (node.Type.Kind() == SyntaxKind.TupleType)
{
diagnostics.Add(ErrorCode.ERR_NewWithTupleTypeSyntax, node.Type.GetLocation());
return MakeBadExpressionForObjectCreation(node, type, boundInitializerOpt, analyzedArguments);
return MakeBadExpressionForObjectCreation(node, type, analyzedArguments, diagnostics);
}

return BindClassCreationExpression(node, typeName, node.Type, type, analyzedArguments, diagnostics, boundInitializerOpt);
return BindClassCreationExpression(node, typeName, node.Type, type, analyzedArguments, diagnostics, node.Initializer, initializerType);
}
finally
{
analyzedArguments.Free();
}
}

private BoundExpression MakeBadExpressionForObjectCreation(ObjectCreationExpressionSyntax node, TypeSymbol type, BoundExpression boundInitializerOpt, AnalyzedArguments analyzedArguments)
private BoundExpression MakeBadExpressionForObjectCreation(ObjectCreationExpressionSyntax node, TypeSymbol type, AnalyzedArguments analyzedArguments, DiagnosticBag diagnostics)
Copy link
Member

Choose a reason for hiding this comment

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

In the case node.Initializer is null the diagnostics parameter won't be used. Should we assert that it's empty in that case?

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 don't know that it will definitely be empty... It gets passed in from much higher up the stack. All we know is that we wouldn't add anything to it, but the code is simple enough that it seems trivially obvious we haven't changed it in the non-initializer case?

{
var children = ArrayBuilder<BoundExpression>.GetInstance();
children.AddRange(BuildArgumentsForErrorRecovery(analyzedArguments));
if (boundInitializerOpt != null)
if (node.Initializer != null)
{
children.Add(boundInitializerOpt);
var boundInitializer = BindInitializerExpression(syntax: node.Initializer,
type: type,
typeSyntax: node.Type,
diagnostics: diagnostics);
children.Add(boundInitializer);
}

return new BoundBadExpression(node, LookupResultKind.NotCreatable, ImmutableArray.Create<Symbol>(type), children.ToImmutableAndFree(), type);
Expand Down Expand Up @@ -4711,7 +4708,8 @@ protected BoundExpression BindClassCreationExpression(
NamedTypeSymbol type,
AnalyzedArguments analyzedArguments,
DiagnosticBag diagnostics,
BoundObjectInitializerExpressionBase boundInitializerOpt = null)
InitializerExpressionSyntax initializerSyntaxOpt = null,
TypeSymbol initializerTypeOpt = null)
{

BoundExpression result = null;
Expand All @@ -4724,6 +4722,7 @@ protected BoundExpression BindClassCreationExpression(
}

HashSet<DiagnosticInfo> useSiteDiagnostics = null;
BoundObjectInitializerExpressionBase boundInitializerOpt = null;

// If we have a dynamic argument then do overload resolution to see if there are one or more
// applicable candidates. If there are, then this is a dynamic object creation; we'll work out
Expand All @@ -4744,6 +4743,7 @@ protected BoundExpression BindClassCreationExpression(

hasErrors &= ReportBadDynamicArguments(node, argArray, refKindsArray, diagnostics, queryClause: null);

boundInitializerOpt = makeBoundInitializerOpt();
result = new BoundDynamicObjectCreationExpression(
node,
typeName,
Expand Down Expand Up @@ -4795,7 +4795,7 @@ protected BoundExpression BindClassCreationExpression(
ReportDiagnosticsIfObsolete(diagnostics, method, node, hasBaseReceiver: false);
// NOTE: Use-site diagnostics were reported during overload resolution.

ConstantValue constantValueOpt = (boundInitializerOpt == null && method.IsDefaultValueTypeConstructor()) ?
ConstantValue constantValueOpt = (initializerSyntaxOpt == null && method.IsDefaultValueTypeConstructor()) ?
FoldParameterlessValueTypeConstructor(type) :
null;

Expand All @@ -4816,6 +4816,7 @@ protected BoundExpression BindClassCreationExpression(
diagnostics);
}

boundInitializerOpt = makeBoundInitializerOpt();
result = new BoundObjectCreationExpression(
node,
method,
Expand Down Expand Up @@ -4864,15 +4865,27 @@ protected BoundExpression BindClassCreationExpression(

var childNodes = ArrayBuilder<BoundExpression>.GetInstance();
childNodes.AddRange(BuildArgumentsForErrorRecovery(analyzedArguments, candidateConstructors));
if (boundInitializerOpt != null)
if (initializerSyntaxOpt != null)
{
childNodes.Add(boundInitializerOpt);
childNodes.Add(boundInitializerOpt ?? makeBoundInitializerOpt());
}

return new BoundBadExpression(node, resultKind, symbols.ToImmutableAndFree(), childNodes.ToImmutableAndFree(), type);

BoundObjectInitializerExpressionBase makeBoundInitializerOpt()
{
if (initializerSyntaxOpt != null)
{
return BindInitializerExpression(syntax: initializerSyntaxOpt,
type: initializerTypeOpt ?? type,
typeSyntax: typeNode,
diagnostics: diagnostics);
}
return null;
}
}

private BoundExpression BindInterfaceCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol type, BoundObjectInitializerExpressionBase boundInitializerOpt, DiagnosticBag diagnostics)
private BoundExpression BindInterfaceCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol type, DiagnosticBag diagnostics)
{
Debug.Assert((object)type != null);

Expand All @@ -4889,7 +4902,7 @@ private BoundExpression BindInterfaceCreationExpression(ObjectCreationExpression
NamedTypeSymbol coClassType = type.ComImportCoClass;
if ((object)coClassType != null)
{
return BindComImportCoClassCreationExpression(node, type, coClassType, boundInitializerOpt, diagnostics);
return BindComImportCoClassCreationExpression(node, type, coClassType, diagnostics);
}
}

Expand All @@ -4909,7 +4922,7 @@ private BoundExpression BindBadInterfaceCreationExpression(ObjectCreationExpress
return result;
}

private BoundExpression BindComImportCoClassCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol interfaceType, NamedTypeSymbol coClassType, BoundObjectInitializerExpressionBase boundInitializerOpt, DiagnosticBag diagnostics)
private BoundExpression BindComImportCoClassCreationExpression(ObjectCreationExpressionSyntax node, NamedTypeSymbol interfaceType, NamedTypeSymbol coClassType, DiagnosticBag diagnostics)
{
Debug.Assert((object)interfaceType != null);
Debug.Assert(interfaceType.IsInterfaceType());
Expand Down Expand Up @@ -4944,10 +4957,10 @@ private BoundExpression BindComImportCoClassCreationExpression(ObjectCreationExp
// NoPIA support
if (interfaceType.ContainingAssembly.IsLinked)
{
return BindNoPiaObjectCreationExpression(node, interfaceType, coClassType, boundInitializerOpt, diagnostics);
return BindNoPiaObjectCreationExpression(node, interfaceType, coClassType, diagnostics);
}

var classCreation = BindClassCreationExpression(node, coClassType, coClassType.Name, boundInitializerOpt, diagnostics);
var classCreation = BindClassCreationExpression(node, coClassType, coClassType.Name, diagnostics, interfaceType);
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
Conversion conversion = this.Conversions.ClassifyConversionFromExpression(classCreation, interfaceType, ref useSiteDiagnostics, forCast: true);
diagnostics.Add(node, useSiteDiagnostics);
Expand Down Expand Up @@ -4985,7 +4998,6 @@ private BoundExpression BindNoPiaObjectCreationExpression(
ObjectCreationExpressionSyntax node,
NamedTypeSymbol interfaceType,
NamedTypeSymbol coClassType,
BoundObjectInitializerExpressionBase boundInitializerOpt,
DiagnosticBag diagnostics)
{
string guidString;
Expand All @@ -4996,6 +5008,12 @@ private BoundExpression BindNoPiaObjectCreationExpression(
guidString = System.Guid.Empty.ToString("D");
}

var boundInitializerOpt = node.Initializer == null ? null :
BindInitializerExpression(syntax: node.Initializer,
type: interfaceType,
typeSyntax: node.Type,
diagnostics: diagnostics);

var creation = new BoundNoPiaObjectCreationExpression(node, guidString, boundInitializerOpt, interfaceType);

// Get the bound arguments and the argument names, it is an error if any are present.
Expand All @@ -5020,7 +5038,7 @@ private BoundExpression BindNoPiaObjectCreationExpression(
return creation;
}

private BoundExpression BindTypeParameterCreationExpression(ObjectCreationExpressionSyntax node, TypeParameterSymbol typeParameter, BoundObjectInitializerExpressionBase boundInitializerOpt, DiagnosticBag diagnostics)
private BoundExpression BindTypeParameterCreationExpression(ObjectCreationExpressionSyntax node, TypeParameterSymbol typeParameter, DiagnosticBag diagnostics)
{
AnalyzedArguments analyzedArguments = AnalyzedArguments.GetInstance();
BindArgumentsAndNames(node.ArgumentList, diagnostics, analyzedArguments);
Expand All @@ -5039,10 +5057,17 @@ private BoundExpression BindTypeParameterCreationExpression(ObjectCreationExpres
}
else
{
var boundInitializerOpt = node.Initializer == null ?
null :
BindInitializerExpression(
syntax: node.Initializer,
type: typeParameter,
typeSyntax: node.Type,
diagnostics: diagnostics);
return new BoundNewT(node, boundInitializerOpt, typeParameter);
}

return MakeBadExpressionForObjectCreation(node, typeParameter, boundInitializerOpt, analyzedArguments);
return MakeBadExpressionForObjectCreation(node, typeParameter, analyzedArguments, diagnostics);
}
finally
{
Expand Down
47 changes: 47 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/OutVarTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34493,6 +34493,53 @@ public class C
");
Assert.Null(model.GetOperation(node3).Parent);
}

[Fact]
public void OutVarInConstructorUsedInObjectInitializer()
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 have a similar test for collection initializers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added OutVarInConstructorUsedInCollectionInitializer

{
var source =
@"
public class C
{
public int Number { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: space before brace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


public C(out int n)
{
n = 1;
}

public static void Main()
{
C c = new C(out var i) { Number = i };
System.Console.WriteLine(c.Number);
}
}
";
CompileAndVerify(source, expectedOutput: @"1");
Copy link
Member

Choose a reason for hiding this comment

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

Use the CreateCompilation helper. It's the standard helper we should be using whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could just use CompileAndVerify. Not blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this was just copy/paste from the other tests. Cleaned up.


[Fact]
public void OutVarInConstructorUsedInCollectionInitializer()
{
var source =
@"
public class C : System.Collections.Generic.List<int>
{
public C(out int n)
{
n = 1;
}

public static void Main()
{
C c = new C(out var i) { i, i, i };
System.Console.WriteLine(c[0]);
}
}
";
CompileAndVerify(source, expectedOutput: @"1");
}

}

internal static class OutVarTestsExtensions
Expand Down