-
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 11 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 |
---|---|---|
|
@@ -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,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: | ||
// | ||
|
@@ -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. | ||
// | ||
|
@@ -124,29 +131,84 @@ public override BoundNode VisitWithExpression(BoundWithExpression withExpr) | |
// tmp.P2 = e2; | ||
// tmp | ||
|
||
BoundExpression expression; | ||
BoundExpression rewrittenReceiver = VisitExpression(receiver); | ||
|
||
if (type.IsAnonymousType) | ||
{ | ||
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); | ||
|
||
if (withExpr.Type.IsValueType) | ||
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 = VisitExpression(withExpr.Receiver); | ||
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) | ||
{ | ||
// map: [propertyIndex] -> valueTemp | ||
var valueTemps = ArrayBuilder<BoundExpression?>.GetInstance(anonymousType.Properties.Length, fillWithValue: null); | ||
|
||
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); | ||
|
||
var property = left.MemberSymbol; | ||
Debug.Assert(property.MemberIndexOpt!.Value >= 0 && property.MemberIndexOpt.Value < anonymousType.Properties.Length); | ||
valueTemps[property.MemberIndexOpt.Value] = valueTemp; | ||
} | ||
|
||
var builder = ArrayBuilder<BoundExpression>.GetInstance(anonymousType.Properties.Length); | ||
foreach (AnonymousTypeManager.AnonymousTypePropertySymbol property in anonymousType.Properties) | ||
{ | ||
if (valueTemps[property.MemberIndexOpt!.Value] is BoundExpression 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)); | ||
} | ||
} | ||
|
||
valueTemps.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.
nit: it feels like the
&&
can be collapsed into a single recursive pattern and this can be inlined at the call site :)