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

Add parsing for the with-expression form #42304

Merged
merged 4 commits into from
Mar 13, 2020

Conversation

agocke
Copy link
Member

@agocke agocke commented Mar 9, 2020

Relates to #40726 (test plan for records)

@@ -9091,6 +9093,10 @@ private ExpressionSyntax ParseExpressionContinued(ExpressionSyntax leftOperand,
{
opKind = SyntaxKind.SwitchExpression;
}
else if (tk == SyntaxKind.WithKeyword && this.PeekToken(1).Kind == SyntaxKind.OpenBraceToken)
Copy link
Member

Choose a reason for hiding this comment

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

do you even need the open brace? it couldn't be legal otherwise, right? so maybe just try to parse here and say that the { is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I don't know. I can't think of how it would be ambiguous, but maybe I'm just not thinking of something :)

@agocke agocke force-pushed the with-expr branch 4 times, most recently from 3f97fca to d18582b Compare March 10, 2020 08:08
<Kind Name="OpenBraceToken"/>
</Field>
<!-- PROTOTYPE: This is the same syntax as for an anonymous object creation, but do we want to share the nodes? -->
<Field Name="Initializers" Type="SeparatedSyntaxList&lt;AnonymousObjectMemberDeclaratorSyntax&gt;" AllowTrailingSeparator="true" />
Copy link
Member

@alrz alrz Mar 10, 2020

Choose a reason for hiding this comment

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

Have you considered nested withers? Something like:

record with { Owner = { Name = "name" } } ->
record.With(owner: record.Owner.With(name: "name"))

Also, it seems like collections could use some help, we already have this pattern:

node with { ArgList = { Args = { 1 } } } ->
node.With(argList: node.ArgList.With(args: node.ArgList.Args.Add(1))

though with that syntax it's not clear if we want Add or a complete replacement.
edit: in fact, if we wanted a replacement, we could say { Args = new .. } that's exactly how collection initializers work today.

So I think it would be actually more similar to object/collection initializers, at least syntactically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nested withers is a neat idea, but out of scope for this PR, I think. We would have to discuss it and approve it in the design meeting.

Copy link
Member

Choose a reason for hiding this comment

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

@agocke agocke marked this pull request as ready for review March 10, 2020 18:48
@agocke agocke requested review from a team as code owners March 10, 2020 18:48
@agocke agocke closed this Mar 10, 2020
@agocke agocke reopened this Mar 10, 2020
@@ -23,219 +23,6 @@ protected override SyntaxTree ParseTree(string text, CSharpParseOptions options)
return SyntaxFactory.ParseSyntaxTree(text, options ?? TestOptions.Regular);
}

[Fact]
public void RecordParsing01()
Copy link
Member

@gafter gafter Mar 10, 2020

Choose a reason for hiding this comment

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

RecordParsing01 [](start = 20, length = 15)

Why delete these tests? #Resolved

Copy link
Member Author

@agocke agocke Mar 10, 2020

Choose a reason for hiding this comment

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

Woops, did not mean to, just meant to consolidate into a single file. Re-added. #Resolved

@gafter
Copy link
Member

gafter commented Mar 10, 2020

Finished review (Iteration 1) #Resolved

@333fred 333fred self-assigned this Mar 11, 2020
Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 4)

@agocke agocke merged commit af17b5d into dotnet:features/records Mar 13, 2020
@agocke agocke deleted the with-expr branch March 13, 2020 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants