-
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
Allow with
on anonymous types
#53248
Changes from 5 commits
1365a81
302fbcc
ff57fe8
dc0c581
85b60cf
ff73199
13ceeb7
ef7097e
f930d4b
0e97cf9
2bad303
9f3cdac
3a6e4e0
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 |
---|---|---|
|
@@ -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) | ||
&& !isAllowedReadonlyOnlySet(receiver)) | ||
{ | ||
Error(diagnostics, ErrorCode.ERR_AssgReadonlyProp, node, propertySymbol); | ||
return false; | ||
|
@@ -1203,6 +1204,17 @@ bool reportUseSite(MethodSymbol accessor) | |
return false; | ||
} | ||
|
||
static bool isAllowedReadonlyOnlySet(BoundExpression receiver) | ||
{ | ||
// ok: anonymousType with { Property = ... } | ||
if (receiver is BoundObjectOrCollectionValuePlaceholder && receiver.Type.IsAnonymousType) | ||
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. nit: it feels like the |
||
{ | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
bool isAllowedInitOnlySet(BoundExpression receiver) | ||
{ | ||
// ok: new C() { InitOnlyProperty = ... } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -103,7 +104,9 @@ 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 | ||
// | ||
|
@@ -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, receiver.P2); | ||
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. |
||
// | ||
// 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. | ||
// | ||
|
@@ -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(); | ||
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. nit: it feels like in this scenario we could use an ArrayBuilder which maps 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. Made the change. Thanks |
||
foreach (BoundExpression initializer in withExpr.InitializerExpression.Initializers) | ||
{ | ||
var assignment = (BoundAssignmentOperator)initializer; | ||
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. |
||
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); | ||
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. 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 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. 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")] | ||
|
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.
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.
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.
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.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Is it intentional to use "only" twice? Not sure what "OnlySet" is supposed to convey. #Closed
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.
We can set despite the property only being readonly. How about
isAllowedDespiteReadonly
?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.
I suggest inlining the condition and avoiding all the naming "problem"