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

Jump to bottom

Use init-only for records and adjust binding #44582

Merged
merged 9 commits into from
May 30, 2020
Merged

Use init-only for records and adjust binding #44582

merged 9 commits into from
May 30, 2020

Conversation

agocke
Copy link
Member

@agocke agocke commented May 27, 2020 •
edited by jcouv
Loading

Changes records to generate init-only properties instead of get-only,
and modifies the with expression to treat arguments as assignments
to the left-hand side, with the safety rules of object initializers
(meaning init-only assignments are allowed).

Also changes records to generate a Clone method and a copy constructor.
The clone calls the copy constructor and the copy constructor does a
memberwise copy of the instance fields of the other type.

This change also tries to simplify and reuse more pieces of binding and
lowering from other initializer-based forms. The with-expression is now
bound as though it were an assignment to a field or member access on a
call to the Clone method.

Spec update: dotnet/csharplang#3505

Relates to #40726 (records)

@agocke agocke force-pushed the with-expr-fix-binding branch 2 times, most recently from 31f1942 to 68463b2 Compare May 27, 2020 02:13
@agocke
Use init-only for records and adjust binding
13e15e1 
Changes records to generate init-only properties instead of get-only,
and modifies the `with` expression to treat arguments as assignments
to the left-hand side, with the safety rules of object initializers
(meaning init-only assignments are allowed).

Also changes records to generate a Clone method and a copy constructor.
The clone calls the copy constructor and the copy constructor does a
memberwise copy of the instance fields of the other type.

This change also tries to simplify and reuse more pieces of binding and
lowering from other initializer-based forms. The with-expression is now
bound as though it were an assignment to a field or member access on a
call to the Clone method.
@agocke agocke marked this pull request as ready for review May 27, 2020 05:55
@agocke agocke requested a review from a team as a code owner May 27, 2020 05:55
@gafter
Copy link
Member

gafter commented May 27, 2020 •
edited
Loading

These changes do not appear to implement things documented in the specification. #Resolved

@gafter gafter self-assigned this May 27, 2020
@agocke
Copy link
Member Author

agocke commented May 27, 2020 •
edited by jcouv
Loading

Writing up the spec changes now. #Resolved

@agocke
Copy link
Member Author

agocke commented May 27, 2020 •
edited
Loading

Corresponding spec changes: dotnet/csharplang#3505

@jcouv
Copy link
Member

jcouv commented May 27, 2020 •
edited
Loading 

    public override BoundNode VisitWithExpression(BoundWithExpression withExpr)

Can we re-use MakeObjectCreationWithInitializer here? I think we mostly just need to give it receiverLocal and a node that holds all the initializers (a BoundObjectInitializerExpression). #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_WithExpression.cs:17 in 13e15e1. [](commit_id = 13e15e1, deletion_comment = False)

@agocke
Copy link
Member Author

agocke commented May 27, 2020 •
edited by jcouv
Loading

I think it's a bad idea to reuse too much of object initializer code here. There are many more things supported in object initializers than are supported in with expressions. For every bug we avoid by re-using the code I worry we would add another because something that wasn't supposed to work now does (or fails weirdly). #Closed

@jcouv
Copy link
Member

jcouv commented May 27, 2020 •
edited
Loading 

            F.InstanceCall(VisitExpression(withExpr.Receiver), "Clone"),

not related to this PR: I think we should be using the CloneMethod from the BoundWithExpression node.
It looks like we have a similar situation with a hardcoded "Equals" in a call to InstanceCall that we might want to re-evaluate (in SynthesizedRecordObjEquals).

We may want to use LocalRewriter.SynthesizeCall and decide whether to pass allowExtensionAndOptionalParameters as true or false (affects whether literally zero arguments are passed, or whether we tolerate params/default parameters. #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_WithExpression.cs:38 in 13e15e1. [](commit_id = 13e15e1, deletion_comment = False)

jcouv
jcouv reviewed May 28, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 3). The factoring looks really nice, thanks!

@agocke agocke requested review from a team as code owners May 28, 2020 23:51
jaredpar
jaredpar previously requested changes May 29, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_WithExpression.cs Show resolved Hide resolved
src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
@@ -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 •
edited by jcouv
Loading

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

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectCreationExpression.cs

public override BoundNode VisitWithExpression(BoundWithExpression withExpr)
{
RoslynDebug.AssertNotNull(withExpr.CloneMethod);
Copy link
Member

Choose a reason for hiding this comment

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

Curious: why are we using RoslynDebug.AssertNotNull here vs. Debug.Assert(withExpr.CloneMethod is object) (or insert your favorite non-null expression form).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I just forgot that debug.assert is annotated now

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordClone.cs Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPropertySymbol.cs
@@ -207,5 +208,137 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
F.CloseMethod(F.Block(F.Return(F.Field(F.This(), _property.BackingField))));
}
}

private sealed class InitAccessorSymbol : SynthesizedInstanceMethodSymbol
Copy link
Member

Choose a reason for hiding this comment

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

SythesizedInitAccessorSymbol?

Copy link
Member Author

Choose a reason for hiding this comment

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

since this is a private class I prefer the short name, and it's unlikely to get confused with a different init accessor :)

jaredpar
jaredpar reviewed May 29, 2020
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Sorry, meant to leave my last review as "comment" not "request changes"

agocke and others added 2 commits May 28, 2020 23:49
@agocke
Respond to PR comments
9f605e2
@agocke @jaredpar
Update src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/Synt…
891fd42 
…hesizedRecordClone.cs

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>
@jaredpar jaredpar dismissed their stale review May 29, 2020 15:24

meant to comment only

@RikkiGibson
Copy link
Contributor

CI issues seem to only be related to formatting

src\Compilers\CSharp\Portable\Symbols\Synthesized\Records\SynthesizedRecordClone.cs(26,1): error IDE0055: Fix formatting
src\Test\Utilities\Portable\Assert\AssertEx.cs(446,1): error IDE0055: Fix formatting

This was referenced May 29, 2020
Closed
Merged
Merged
@RikkiGibson
Copy link
Contributor

Should the following tests be unskipped in this PR?

roslyn/src/Compilers/CSharp/Test/Semantic/Semantics/InitOnlyMemberTests.cs

Line 604 in 891fd42

public void InitOnlyPropertyAssignmentAllowedInWithInitializer()

roslyn/src/Compilers/CSharp/Test/Semantic/Semantics/InitOnlyMemberTests.cs

Line 646 in 891fd42

public void InitOnlyPropertyAssignmentAllowedInWithInitializer_Evaluation()

@agocke
Copy link
Member Author

agocke commented May 29, 2020

@RikkiGibson Good question: @jcouv any thoughts?

@jcouv
Copy link
Member

jcouv commented May 29, 2020 

                diagnostics.Add(ErrorCode.ERR_DuplicateRecordConstructor, paramList.Location);

Added a bullet in test plan. Closing this thread.

In reply to: 634988399 [](ancestors = 634988399)

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:2980 in 13e15e1. [](commit_id = 13e15e1, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented May 29, 2020

Yes, InitOnlyPropertyAssignmentAllowedInWithInitializer and other test can be unskipped or removed (we have the coverage). I'm okay to do it in subsequent PR.

jcouv
jcouv approved these changes May 29, 2020
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 8)

RikkiGibson
RikkiGibson reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
@@ -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
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.

@agocke
Copy link
Member Author

agocke commented May 29, 2020

@gafter @jaredpar @RikkiGibson Anything I missed?

RikkiGibson
RikkiGibson reviewed May 29, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs
// class Point(int x, int y);
Diagnostic(ErrorCode.ERR_BadRecordDeclaration, "(int x, int y)").WithLocation(2, 12),
// (2,17): error CS0518: Predefined type 'System.Runtime.CompilerServices.IsExternalInit' is not defined or imported
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an expectation that eventually the compiler will synthesize an internal version of this attribute if needed, similar to IsReadOnlyAttribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked this question as well: no, because there are various ordering problems with synthesizing types used in custom modifiers.

@agocke agocke merged commit f089d87 into dotnet:features/records May 30, 2020
@agocke agocke deleted the with-expr-fix-binding branch May 30, 2020 00:08
@RikkiGibson RikkiGibson mentioned this pull request Jun 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gafter gafter gafter left review comments

@jaredpar jaredpar jaredpar approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

@angelwen1396 angelwen1396 Awaiting requested review from angelwen1396

@333fred 333fred Awaiting requested review from 333fred

Assignees

@gafter gafter

Labels
Area-Compilers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@agocke @gafter @jcouv @RikkiGibson @jaredpar