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

Use init-only for records and adjust binding #44582

Merged
merged 9 commits into from
May 30, 2020
Merged
Show file tree
Hide file tree
Changes from 8 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
43 changes: 27 additions & 16 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4298,7 +4298,13 @@ private BoundObjectInitializerExpressionBase BindInitializerExpression(
switch (syntax.Kind())
{
case SyntaxKind.ObjectInitializerExpression:
return BindObjectInitializerExpression(syntax, type, diagnostics, implicitReceiver);
// Uses a special binder to produce customized diagnostics for the object initializer
return BindObjectInitializerExpression(
syntax, type, diagnostics, implicitReceiver, useObjectInitDiagnostics: true);

case SyntaxKind.WithInitializerExpression:
return BindObjectInitializerExpression(
syntax, type, diagnostics, implicitReceiver, useObjectInitDiagnostics: false);

case SyntaxKind.CollectionInitializerExpression:
return BindCollectionInitializerExpression(syntax, type, diagnostics, implicitReceiver);
Expand All @@ -4321,6 +4327,7 @@ private BoundExpression BindInitializerExpressionOrValue(
{
case SyntaxKind.ObjectInitializerExpression:
case SyntaxKind.CollectionInitializerExpression:
Debug.Assert(syntax.Parent.Parent.Kind() != SyntaxKind.WithInitializerExpression);
return BindInitializerExpression((InitializerExpressionSyntax)syntax, type, typeSyntax, diagnostics);
default:
return BindValue(syntax, diagnostics, BindValueKind.RValue);
Expand All @@ -4331,44 +4338,51 @@ private BoundObjectInitializerExpression BindObjectInitializerExpression(
InitializerExpressionSyntax initializerSyntax,
TypeSymbol initializerType,
DiagnosticBag diagnostics,
BoundObjectOrCollectionValuePlaceholder implicitReceiver)
BoundObjectOrCollectionValuePlaceholder implicitReceiver,
bool useObjectInitDiagnostics)
{
// SPEC: 7.6.10.2 Object initializers
//
// SPEC: An object initializer consists of a sequence of member initializers, enclosed by { and } tokens and separated by commas.
// SPEC: Each member initializer must name an accessible field or property of the object being initialized, followed by an equals sign and
// SPEC: an expression or an object initializer or collection initializer.

Debug.Assert(initializerSyntax.Kind() == SyntaxKind.ObjectInitializerExpression);
Debug.Assert(initializerSyntax.Kind() == SyntaxKind.ObjectInitializerExpression ||
initializerSyntax.Kind() == SyntaxKind.WithInitializerExpression);
Debug.Assert((object)initializerType != null);

var initializerBuilder = ArrayBuilder<BoundExpression>.GetInstance();

// Member name map to report duplicate assignments to a field/property.
var memberNameMap = new HashSet<string>();

// We use a location specific binder for binding object initializer field/property access to generate object initializer specific diagnostics:
// 1) CS1914 (ERR_StaticMemberInObjectInitializer)
// 2) CS1917 (ERR_ReadonlyValueTypeInObjectInitializer)
// 3) CS1918 (ERR_ValueTypePropertyInObjectInitializer)
// Note that this is only used for the LHS of the assignment - these diagnostics do not apply on the RHS.
// For this reason, we will actually need two binders: this and this.WithAdditionalFlags.
var objectInitializerMemberBinder = this.WithAdditionalFlags(BinderFlags.ObjectInitializerMember);
var objectInitializerMemberBinder = useObjectInitDiagnostics
? this.WithAdditionalFlags(BinderFlags.ObjectInitializerMember)
: this;

var initializers = ArrayBuilder<BoundExpression>.GetInstance(initializerSyntax.Expressions.Count);

// Member name map to report duplicate assignments to a field/property.
var memberNameMap = PooledHashSet<string>.GetInstance();
foreach (var memberInitializer in initializerSyntax.Expressions)
{
BoundExpression boundMemberInitializer = BindObjectInitializerMemberAssignment(
BoundExpression boundMemberInitializer = BindInitializerMemberAssignment(
memberInitializer, initializerType, objectInitializerMemberBinder, diagnostics, implicitReceiver);

initializerBuilder.Add(boundMemberInitializer);
initializers.Add(boundMemberInitializer);

ReportDuplicateObjectMemberInitializers(boundMemberInitializer, memberNameMap, diagnostics);
}

return new BoundObjectInitializerExpression(initializerSyntax, implicitReceiver, initializerBuilder.ToImmutableAndFree(), initializerType);
return new BoundObjectInitializerExpression(
initializerSyntax,
implicitReceiver,
initializers.ToImmutableAndFree(),
initializerType);
}

private BoundExpression BindObjectInitializerMemberAssignment(
private BoundExpression BindInitializerMemberAssignment(
ExpressionSyntax memberInitializer,
TypeSymbol initializerType,
Binder objectInitializerMemberBinder,
Expand Down Expand Up @@ -4402,7 +4416,6 @@ private BoundExpression BindObjectInitializerMemberAssignment(
// See comments in BindObjectInitializerExpression for more details.

Debug.Assert(objectInitializerMemberBinder != null);
Debug.Assert(objectInitializerMemberBinder.Flags.Includes(BinderFlags.ObjectInitializerMember));

boundLeft = objectInitializerMemberBinder.BindObjectInitializerMember(initializer, implicitReceiver, diagnostics);
}
Expand Down Expand Up @@ -4572,8 +4585,6 @@ private BoundExpression BindObjectInitializerMember(
// CheckValueKind to generate possible diagnostics for invalid initializers non-viable member lookup result:
// 1) CS0154 (ERR_PropertyLacksGet)
// 2) CS0200 (ERR_AssgReadonlyProp)

Debug.Assert(Flags.Includes(CSharp.BinderFlags.ObjectInitializerMember));
if (!CheckValueKind(boundMember.Syntax, boundMember, valueKind, checkingReceiver: false, diagnostics: diagnostics))
{
hasErrors = true;
Expand Down
91 changes: 8 additions & 83 deletions src/Compilers/CSharp/Portable/Binder/Binder_WithExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ internal partial class Binder
{
private BoundExpression BindWithExpression(WithExpressionSyntax syntax, DiagnosticBag diagnostics)
{
// PROTOTYPE: this entire method is likely to change
var receiver = BindRValueWithoutTargetType(syntax.Receiver, diagnostics);
var receiverType = receiver.Type;

Expand Down Expand Up @@ -95,94 +94,20 @@ private BoundExpression BindWithExpression(WithExpressionSyntax syntax, Diagnost
}

var cloneReturnType = cloneMethod?.ReturnType;
var initializer = BindInitializerExpression(
syntax.Initializer,
cloneReturnType ?? receiverType,
syntax.Receiver,
diagnostics);

var args = ArrayBuilder<(Symbol?, BoundExpression)>.GetInstance();
// Bind with expression arguments
foreach (var expr in syntax.Initializer.Expressions)
{
Symbol? member = null;
BoundExpression boundRight;
// We're expecting a simple assignment only, with an ID on the left
if (!(expr is AssignmentExpressionSyntax assignment) ||
!(assignment.Left is IdentifierNameSyntax left))
{
boundRight = BindExpression(expr, diagnostics);
hasErrors = true;
diagnostics.Add(ErrorCode.ERR_BadWithExpressionArgument, expr.Location);
}
else
{
var propName = left.Identifier.Text;
if (!(cloneReturnType is null))
{
var location = left.Location;
this.LookupMembersInType(
lookupResult,
cloneReturnType,
propName,
arity: 0,
basesBeingResolved: null,
options: LookupOptions.Default,
originalBinder: this,
diagnose: false,
useSiteDiagnostics: ref useSiteDiagnostics);
// PROTOTYPE: Should handle hiding/overriding and bind like regular accesses
if (lookupResult.IsSingleViable &&
lookupResult.SingleSymbolOrDefault is var sym)
{
switch (sym.Kind)
{
case SymbolKind.Property:
member = sym;
// PROTOTYPE: this should check for init-only, but that isn't a separate feature yet
// It also will not work in metadata.
if (!(sym is SynthesizedRecordPropertySymbol))
{
goto default;
}
break;

default:
hasErrors = true;
diagnostics.Add(
ErrorCode.ERR_WithMemberIsNotRecordProperty,
location);
break;
}
}

if (!hasErrors && member is null)
{
hasErrors = true;
Error(
diagnostics,
ErrorCode.ERR_NoSuchMemberOrExtension,
location,
cloneReturnType,
propName);
}
}

boundRight = BindValue(assignment.Right, diagnostics, BindValueKind.RValue);
if (!(member is null))
{
boundRight = GenerateConversionForAssignment(
member.GetTypeOrReturnType().Type,
boundRight,
diagnostics);
}
lookupResult.Clear();
}
args.Add((member, boundRight));
}

lookupResult.Free();
// N.B. Since we only don't parse nested initializers in syntax there should be no extra
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
// errors we need to check for here.

return new BoundWithExpression(
syntax,
receiver,
cloneMethod,
args.ToImmutableAndFree(),
initializer,
cloneReturnType ?? receiverType,
hasErrors: hasErrors);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2131,7 +2131,7 @@
<!-- CloneMethod may be null in error scenarios-->
<Field Name="CloneMethod" Type="MethodSymbol?" />
<!-- Members and expressions passed as arguments to the With expression. -->
<Field Name="Arguments" Type="ImmutableArray&lt;(Symbol? Member, BoundExpression Expression)&gt;" />
<Field Name="InitializerExpression" Type="BoundObjectInitializerExpressionBase" />
Copy link
Member

@jaredpar jaredpar May 29, 2020

Choose a reason for hiding this comment

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

This seems to really simplify the code. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the type made BoundObjectInitializerExpressionBase instead of BoundObjectInitializerExpression? Is it just to make it easier to call BindInitializerExpression?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason, I was just leaving space for potentially creating a BoundWithInitializerExpression in the future, if we want it.

</Node>

</Tree>
3 changes: 0 additions & 3 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6130,9 +6130,6 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_AssignmentInitOnly" xml:space="preserve">
<value>Init-only property or indexer '{0}' can only be assigned in an object initializer, or on 'this' or 'base' in an instance constructor or an 'init' accessor.</value>
</data>
<data name="ERR_BadWithExpressionArgument" xml:space="preserve">
<value>Arguments to a `with` expression must be simple assignments</value>
</data>
<data name="ERR_DesignatorBeneathPatternCombinator" xml:space="preserve">
<value>A variable may not be declared within a 'not' or 'or' pattern.</value>
</data>
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1803,7 +1803,6 @@ internal enum ErrorCode
ERR_NoSingleCloneMethod = 8858,
ERR_ContainingTypeMustDeriveFromWithReturnType = 8859,
ERR_WithMemberIsNotRecordProperty = 8860,
ERR_BadWithExpressionArgument = 8861,

#endregion

Expand Down
27 changes: 15 additions & 12 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading