Implement parsing for record_base_arguments. by AlekseyTs · Pull Request #44733 · dotnet/roslyn

AlekseyTs · 2020-05-31T02:59:16Z

record_class_base
    : class_type record_base_arguments?
    | interface_type_list
    | class_type record_base_arguments? ',' interface_type_list
    ;

record_base_arguments
    : '(' argument_list? ')'
    ;

``` record_class_base : class_type record_base_arguments? | interface_type_list | class_type record_base_arguments? ',' interface_type_list ; record_base_arguments : '(' argument_list? ')' ; ```

gafter · 2020-05-31T21:41:56Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

        }

-        private BaseListSyntax ParseBaseList()
+        private BaseListSyntax ParseBaseList(SyntaxToken classOrStructOrInterfaceKeyword, bool haveParameters)


ParseBaseList [](start = 31, length = 13)

This syntax contradicts the records specification at https://github.com/dotnet/csharplang/blob/master/proposals/records.md. According to the records specification, a class declaration with parameters has no base clause. The grammar appearing in the OP is not used in any production so it does not affect the specification.

If records are permitted to have base clauses and/or base ctor arguments, please document those (syntax, static semantics, and dynamic semantics) in https://github.com/dotnet/csharplang/blob/master/proposals/records.md so this PR can be reviewed against the specification. If a record class is permitted to have a base type, please adjust the other sections of the specification to account for that. #Resolved

The syntax is specified in docs\features\records.md and the desired effect of the PR is clearly stated in the description. I think this is good enough to be able to review the PR based on that. Clearly the definition of static semantics, and dynamic semantics is not necessary because this PR isn't trying to implement these aspects of the feature. CC @agocke.

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

The document https://github.com/dotnet/roslyn/blob/master/docs/features/records.md is the same four-year-old draft that was moved to the rejected folder in csharplang recently. Language specifications are kept in csharplang. The current specification for records is at https://github.com/dotnet/csharplang/blob/master/proposals/records.md and there is clearly no base clause in the syntax. #Resolved

The document https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md is the same one that was moved to the rejected folder in csharplang recently. Language specifications are kept in csharplang.

I am not sure what point you are trying to make and how covariant-returns.md is related to this PR. The document I referenced above is https://github.com/dotnet/roslyn/blob/features/records/docs/features/records.md

The current specification for records is at https://github.com/dotnet/csharplang/blob/master/proposals/records.md and there is clearly no base clause in the syntax.

I understand that. I believe the PR description is sufficient to draw conclusion about the intent. Could you review this PR assuming that this is the design? Is it not clear? If you have questions on the grammar, I might be able to answer them. If you have arguments against the proposed grammar, this should be taken offline and should not block this PR. We will get necessary approvals and reconcile the specification separately. Otherwise, you always have an option to not review this PR. Thanks.

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

Our process is not to infer the specification from draft implementation, but to check the draft implementation against the specification. #WontFix

Our process is not to infer the specification from draft implementation, but to check the draft implementation against the specification.

When multiple people sharing work on the feature under tight schedule, some things happen in parallel, or out of perfect alignment.

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

AlekseyTs · 2020-06-01T04:47:20Z

@agocke, @cston, @jcouv, @dotnet/roslyn-compiler Please review.

agocke · 2020-06-01T04:52:00Z

This seems to match what I wrote up at dotnet/csharplang#3527 so it seems like we're on the same page. I'll try to review this tomorrow. #Closed

AlekseyTs · 2020-06-01T16:16:15Z

@agocke, @cston, @jcouv, @dotnet/roslyn-compiler Please review.

gafter · 2020-06-01T16:52:57Z

This seems to match what I wrote up at dotnet/csharplang#3527 so it seems like we're on the same page.

That specification does not have an argument list in the base clause for a class, struct, or interface, which is what is implemented by this PR. #Resolved

AlekseyTs · 2020-06-01T16:59:51Z

That specification does not have an argument list in the base clause for a class, struct, or interface, which is what is implemented by this PR.

We don't have separate parsing for record types. Structures are valid record types, at least syntactically, at the moment. For structures and interfaces we parse the argument list in error recovery mode and report syntax error for it. #Resolved

agocke · 2020-06-01T17:01:37Z

I will have a PR with the change to use the record syntax shortly and we can merge those together whenever it's appropriate. #Resolved

gafter · 2020-06-01T19:40:10Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                {
+                    argumentList = this.ParseParenthesizedArgumentList();
+
+                    if (classOrStructOrInterfaceKeyword.Kind != SyntaxKind.ClassKeyword || !haveParameters)


!haveParameters [](start = 91, length = 15)

The specification at dotnet/csharplang#3527 does not support an error in the case where there are no parameters. #Resolved

The specification at dotnet/csharplang#3527 does not support an error in the case where there are no parameters.

As it stands right now, parameters are required for a record declaration and only a class record declaration is allowed to have arguments. Given the current implementation of type parsing, presence of parameters is an indicator that we are parsing a record. This is a error recovery situation, we could not even attempt to parse the arguments, but I chose to parse them and report an error instead. Reconciling parsing of other parts of record declaration with the updated spec is out of scope for this PR. When that work will happen, necessary adjustments are going to be made to this code, if necessary.

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

gafter · 2020-06-01T19:40:46Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                {
+                    argumentList = this.ParseParenthesizedArgumentList();
+
+                    if (classOrStructOrInterfaceKeyword.Kind != SyntaxKind.ClassKeyword || !haveParameters)


ClassKeyword [](start = 75, length = 12)

The specification does not support arguments when the class keyword is used. #Resolved

The specification does not support arguments when the class keyword is used.

As clearly stated in the PR title, this PR is focused on parsing base clause for records. Given current implementation, class keyword is required to declare a record. Reconciling that aspect of parsing with the updated spec is out of scope for this PR.

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

gafter · 2020-06-01T19:43:32Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

    <Kind Name="SimpleBaseType"/>
    <Field Name="Type" Type="TypeSyntax" Override="true">
    </Field>
+    <Field Name="ArgumentList" Type="ArgumentListSyntax" Optional="true"/>


ArgumentList [](start = 17, length = 12)

It feels like this is a case where a different node derived from BaseTypeSyntax would make more sense than adding arguments to SimpleBaseTypeSyntax #Pending

It feels like this is a case where a different node derived from BaseTypeSyntax would make more sense than adding arguments to SimpleBaseTypeSyntax

I considered that, however I believe existing tools will be served better by not forcing them to change what types of nodes they need to handle. I'll record this as an open issue.

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

This anticipated future scenario (adding arguments in the base clause for records) is precisely the reason that BaseTypeSyntax was separated from SimpleBaseTypeSyntax in the first place.

In reply to: 433461884 [](ancestors = 433461884,433449588)

This anticipated future scenario (adding arguments in the base clause for records) is precisely the reason that BaseTypeSyntax was separated from SimpleBaseTypeSyntax in the first place.

Quite possibly true. We didn't have that many existing tools at the time. In order to avoid a possibility that many of the tools would need an adjustment to deal with the new node, I believe it is safer to not introduce a new node for preview 3. We will follow up on this design question at a later time.

In reply to: 433525828 [](ancestors = 433525828,433461884,433449588)

AlekseyTs · 2020-06-01T20:31:18Z

@gafter I think I responded to all your feedback.

agocke

LGTM. I updated the specification to note that an error is produced if the record type has a base argument list without a parameter list.

agocke · 2020-06-02T00:22:03Z

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs

        }
+
+        [Theory]
+        [CombinatorialData]


I've never seen this trait before, this is pretty cool :) #Resolved

AlekseyTs · 2020-06-02T00:25:19Z

@gafter, @cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off.

AlekseyTs · 2020-06-02T15:27:56Z

@gafter, @cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off.

cston · 2020-06-02T18:43:53Z

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs

+            [CombinatorialValues("class", "struct")] string typeKeyword,
+            [CombinatorialValues(true, false)] bool withParameters,
+            [CombinatorialValues(true, false)] bool withBaseArguments,
+            [CombinatorialValues(true, false)] bool withBody)


Are the [CombinatorialValues] attributes needed for bool parameters? #WontFix

Are the [CombinatorialValues] attributes needed for bool parameters?

I don't know, its presence works great.

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

Implement parsing for record_base_arguments.

e1c80a6

``` record_class_base : class_type record_base_arguments? | interface_type_list | class_type record_base_arguments? ',' interface_type_list ; record_base_arguments : '(' argument_list? ')' ; ```

AlekseyTs added Area-Compilers Feature - Records Records labels May 31, 2020

AlekseyTs requested review from agocke, cston and jcouv May 31, 2020 02:59

AlekseyTs requested a review from a team as a code owner May 31, 2020 02:59

AlekseyTs added 3 commits May 30, 2020 20:08

Fixup

2fd0193

Update PublicAPI file

7f0feee

Fixup

15f1154

gafter reviewed May 31, 2020

View reviewed changes

gafter reviewed Jun 1, 2020

View reviewed changes

agocke approved these changes Jun 2, 2020

View reviewed changes

cston reviewed Jun 2, 2020

View reviewed changes

cston approved these changes Jun 2, 2020

View reviewed changes

AlekseyTs merged commit 0f68e24 into dotnet:features/records Jun 2, 2020

AlekseyTs mentioned this pull request Jun 2, 2020

Finalize the design for the shape of the BaseTypeSyntax in relation to the argument list for records #44795

Closed

Conversation

AlekseyTs commented May 31, 2020

Uh oh!

gafter May 31, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

gafter Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 1, 2020

Uh oh!

agocke commented Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 1, 2020

Uh oh!

gafter commented Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke commented Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

gafter Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

gafter Jun 1, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

gafter Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 1, 2020

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

agocke Jun 2, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 2, 2020

Uh oh!

gafter May 31, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Jun 1, 2020 •

edited

Loading

gafter Jun 1, 2020 •

edited by AlekseyTs

Loading

gafter Jun 1, 2020 •

edited by AlekseyTs

Loading

AlekseyTs Jun 1, 2020 •

edited

Loading

agocke commented Jun 1, 2020 •

edited by AlekseyTs

Loading

gafter commented Jun 1, 2020 •

edited by AlekseyTs

Loading

AlekseyTs commented Jun 1, 2020 •

edited

Loading

agocke commented Jun 1, 2020 •

edited by AlekseyTs

Loading

gafter Jun 1, 2020 •

edited by AlekseyTs

Loading

gafter Jun 1, 2020 •

edited by AlekseyTs

Loading

gafter Jun 1, 2020 •

edited by AlekseyTs

Loading

agocke Jun 2, 2020 •

edited by AlekseyTs

Loading

cston Jun 2, 2020 •

edited by AlekseyTs

Loading