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

Allow with on anonymous types #53248

Merged
merged 13 commits into from
May 21, 2021
Merged
Show file tree
Hide file tree
Changes from 10 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
14 changes: 13 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1076,7 +1076,8 @@ private bool CheckPropertyValueKind(SyntaxNode node, BoundExpression expr, BindV
if (setMethod is null)
{
var containing = this.ContainingMemberOrLambda;
if (!AccessingAutoPropertyFromConstructor(receiver, propertySymbol, containing))
if (!AccessingAutoPropertyFromConstructor(receiver, propertySymbol, containing)
&& !isAllowedDespiteReadonly(receiver))
{
Error(diagnostics, ErrorCode.ERR_AssgReadonlyProp, node, propertySymbol);
return false;
Expand Down Expand Up @@ -1203,6 +1204,17 @@ bool reportUseSite(MethodSymbol accessor)
return false;
}

static bool isAllowedDespiteReadonly(BoundExpression receiver)
{
// ok: anonymousType with { Property = ... }
if (receiver is BoundObjectOrCollectionValuePlaceholder && receiver.Type.IsAnonymousType)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it feels like the && can be collapsed into a single recursive pattern and this can be inlined at the call site :)

{
return true;
}

return false;
}

bool isAllowedInitOnlySet(BoundExpression receiver)
{
// ok: new C() { InitOnlyProperty = ... }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,14 @@ private BoundExpression BindWithExpression(WithExpressionSyntax syntax, BindingD
}

MethodSymbol? cloneMethod = null;
if (receiverType.IsValueType)
if (receiverType.IsValueType && !receiverType.IsPointerOrFunctionPointer())
{
CheckFeatureAvailability(syntax, MessageID.IDS_FeatureWithOnStructs, diagnostics);
}
else if (receiverType.IsAnonymousType)
{
CheckFeatureAvailability(syntax, MessageID.IDS_FeatureWithOnAnonymousTypes, diagnostics);
}
else if (!receiverType.IsErrorType())
{
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
Expand All @@ -61,7 +65,7 @@ private BoundExpression BindWithExpression(WithExpressionSyntax syntax, BindingD
isForNewInstance: true,
diagnostics);

// N.B. Since we only don't parse nested initializers in syntax there should be no extra
// N.B. Since we don't parse nested initializers in syntax there should be no extra
// errors we need to check for here.

return new BoundWithExpression(
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6576,6 +6576,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureWithOnStructs" xml:space="preserve">
<value>with on structs</value>
</data>
<data name="IDS_FeatureWithOnAnonymousTypes" xml:space="preserve">
<value>with on anonymous types</value>
</data>
<data name="IDS_FeaturePositionalFieldsInRecords" xml:space="preserve">
<value>positional fields in records</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ internal enum MessageID
IDS_FeatureGlobalUsing = MessageBase + 12798,
IDS_FeatureInferredDelegateType = MessageBase + 12799,
IDS_FeatureLambdaAttributes = MessageBase + 12800,
IDS_FeatureWithOnAnonymousTypes = MessageBase + 12801,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -334,6 +335,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureSealedToStringInRecord: // semantic check
case MessageID.IDS_FeatureRecordStructs:
case MessageID.IDS_FeatureWithOnStructs: // semantic check
case MessageID.IDS_FeatureWithOnAnonymousTypes: // semantic check
case MessageID.IDS_FeaturePositionalFieldsInRecords: // semantic check
case MessageID.IDS_FeatureGlobalUsing:
case MessageID.IDS_FeatureInferredDelegateType: // semantic check
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -103,11 +104,13 @@ public override BoundNode VisitObjectCreationExpression(BoundObjectCreationExpre

public override BoundNode VisitWithExpression(BoundWithExpression withExpr)
{
Debug.Assert(withExpr.Receiver.Type!.Equals(withExpr.Type, TypeCompareKind.ConsiderEverything));
TypeSymbol type = withExpr.Type;
BoundExpression receiver = withExpr.Receiver;
Debug.Assert(receiver.Type!.Equals(type, TypeCompareKind.ConsiderEverything));

// for a with expression of the form
//
// receiver with { P1 = e1, P2 = e2 }
// receiver with { P1 = e1, P2 = e2 } // P3 is copied implicitly
//
// if the receiver is a struct, duplicate the value, then set the given struct properties:
//
Expand All @@ -116,6 +119,10 @@ public override BoundNode VisitWithExpression(BoundWithExpression withExpr)
// tmp.P2 = e2;
// tmp
//
// if the receiver is an anonymous type, then invoke its constructor:
//
// new Type(e1, e2, receiver.P3);
//
// otherwise the receiver is a record class and we want to lower it to a call to its `Clone` method, then
// set the given record properties. i.e.
//
Expand All @@ -124,29 +131,80 @@ public override BoundNode VisitWithExpression(BoundWithExpression withExpr)
// tmp.P2 = e2;
// tmp

BoundExpression expression;
BoundExpression rewrittenReceiver = VisitExpression(receiver);

if (withExpr.Type.IsValueType)
if (type.IsAnonymousType)
{
expression = VisitExpression(withExpr.Receiver);
var anonymousType = (AnonymousTypeManager.AnonymousTypePublicSymbol)type;
var sideEffects = ArrayBuilder<BoundExpression>.GetInstance();
var temps = ArrayBuilder<LocalSymbol>.GetInstance();
BoundLocal oldValue = _factory.StoreToTemp(rewrittenReceiver, out BoundAssignmentOperator boundAssignmentToTemp);
temps.Add(oldValue.LocalSymbol);
sideEffects.Add(boundAssignmentToTemp);

BoundExpression value = _factory.New(anonymousType, getAnonymousTypeValues(withExpr, oldValue, anonymousType, sideEffects, temps));

return new BoundSequence(withExpr.Syntax, temps.ToImmutableAndFree(), sideEffects.ToImmutableAndFree(), value, type);
}

BoundExpression expression;
if (type.IsValueType)
{
expression = rewrittenReceiver;
}
else
{
Debug.Assert(withExpr.CloneMethod is not null);
Debug.Assert(withExpr.CloneMethod.ParameterCount == 0);

expression = _factory.Convert(
withExpr.Type,
type,
_factory.Call(
VisitExpression(withExpr.Receiver),
rewrittenReceiver,
withExpr.CloneMethod));
}

return MakeExpressionWithInitializer(
withExpr.Syntax,
expression,
withExpr.InitializerExpression,
withExpr.Type);
type);

ImmutableArray<BoundExpression> getAnonymousTypeValues(BoundWithExpression withExpr, BoundExpression oldValue, AnonymousTypeManager.AnonymousTypePublicSymbol anonymousType,
ArrayBuilder<BoundExpression> sideEffects, ArrayBuilder<LocalSymbol> temps)
{
var dictionary = PooledDictionary<PropertySymbol, BoundExpression>.GetInstance();
Copy link
Contributor

@RikkiGibson RikkiGibson May 18, 2021

Choose a reason for hiding this comment

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

nit: it feels like in this scenario we could use an ArrayBuilder which maps the AnonymousTypePropertySymbol.MemberIndexOpt.Value to a BoundLocal?. Also could consider exposing a member on AnonymousTypePropertySymbol like int MemberIndex => _index. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Made the change. Thanks

foreach (BoundExpression initializer in withExpr.InitializerExpression.Initializers)
{
var assignment = (BoundAssignmentOperator)initializer;
Copy link
Contributor

@AlekseyTs AlekseyTs May 11, 2021

Choose a reason for hiding this comment

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

(BoundAssignmentOperator)initializer;

Is it that simple? It feels like we should be prepared at least to deal with some of the additional things that AddObjectInitializer helper is dealing with. #Closed

var left = (BoundObjectInitializerMember)assignment.Left;
Debug.Assert(left.MemberSymbol is not null);

// We evaluate the values provided in source first
BoundLocal valueTemp = _factory.StoreToTemp(assignment.Right, out BoundAssignmentOperator boundAssignmentToTemp);
Copy link
Contributor

@RikkiGibson RikkiGibson May 18, 2021

Choose a reason for hiding this comment

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

nit: it feels like it's possible to avoid storing to temps when the assignments occur in the same order as the constructor parameters, similar to the way method call arguments are rewritten during lowering. However, I don't think it's necessary to implement this optimization in this PR. #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Also, the cases where this optimization could actually play out aren't the common case so it seems fine to punt.

temps.Add(valueTemp.LocalSymbol);
sideEffects.Add(boundAssignmentToTemp);

dictionary.Add((PropertySymbol)left.MemberSymbol, valueTemp);
}

var builder = ArrayBuilder<BoundExpression>.GetInstance(anonymousType.Properties.Length);
foreach (var property in anonymousType.Properties)
{
if (dictionary.TryGetValue(property, out var initializerValue))
{
builder.Add(initializerValue);
}
else
{
// The values that are implicitly copied over will get evaluated afterwards, in the order they are needed
builder.Add(_factory.Property(oldValue, property));
}
}

dictionary.Free();
return builder.ToImmutableAndFree();
}
}

[return: NotNullIfNotNull("initializerExpressionOpt")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,12 @@ public BoundObjectCreationExpression New(NamedTypeSymbol type, params BoundExpre
public BoundObjectCreationExpression New(MethodSymbol ctor, params BoundExpression[] args)
=> New(ctor, args.ToImmutableArray());

public BoundObjectCreationExpression New(NamedTypeSymbol type, ImmutableArray<BoundExpression> args)
{
var ctor = type.InstanceConstructors.Single(c => c.ParameterCount == args.Length);
return New(ctor, args);
}

public BoundObjectCreationExpression New(MethodSymbol ctor, ImmutableArray<BoundExpression> args)
=> new BoundObjectCreationExpression(Syntax, ctor, args) { WasCompilerGenerated = true };

Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf

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

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf

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

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

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

Loading