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

Implement parsing changes for Primary Constructors feature #65720

Conversation

AlekseyTs
Copy link
Contributor

For classes and structures:

  • Support parameters
  • Support base arguments
  • Support semicolon as body

https://github.com/dotnet/csharplang/blob/main/proposals/primary-constructors.md#syntax

For classes and structures:
- Support parameters
- Support base arguments
- Support semicolon as body

https://github.com/dotnet/csharplang/blob/main/proposals/primary-constructors.md#syntax
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler Please review.

@@ -3355,7 +3355,7 @@
</PropertyComment>
</Field>
</AbstractNode>
<Node Name="ClassDeclarationSyntax" Base="TypeDeclarationSyntax">
<Node Name="ClassDeclarationSyntax" Base="TypeDeclarationSyntax" SkipConvenienceFactories="true">
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 2, 2022

Choose a reason for hiding this comment

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

what's the impact of SkipConvenienceFactories? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's the impact of SkipConvenienceFactories?

Responded at location that illustrates the impact. See #65720 (comment)

}

/// <summary>Creates a new ClassDeclarationSyntax instance.</summary>
public static ClassDeclarationSyntax ClassDeclaration(SyntaxList<AttributeListSyntax> attributeLists, SyntaxTokenList modifiers, SyntaxToken identifier, TypeParameterListSyntax? typeParameterList, BaseListSyntax? baseList, SyntaxList<TypeParameterConstraintClauseSyntax> constraintClauses, SyntaxList<MemberDeclarationSyntax> members)
Copy link
Contributor Author

@AlekseyTs AlekseyTs Dec 3, 2022

Choose a reason for hiding this comment

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

ClassDeclaration

what's the impact of SkipConvenienceFactories?

Generator stops auto generating these three factories. Since braces become optional, generated implementation starts using default for for them. And these factories stop producing syntactically valid nodes. I moved them with original implementation to SyntaxFactory.cs. We no longer rely on generator to generate them.

The same goes for three similar factories for structures below. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for the explanation!

@AlekseyTs AlekseyTs requested a review from a team as a code owner December 4, 2022 19:33
@AlekseyTs AlekseyTs requested review from jjonescz and cston December 5, 2022 15:06
@AlekseyTs
Copy link
Contributor Author

@cston, @jjonescz, @dotnet/roslyn-compiler Please review.

var identifier = this.ParseIdentifierToken();
SyntaxToken identifier;

if (this.CurrentToken.Kind == SyntaxKind.IdentifierToken && IsCurrentTokenWhereOfConstraintClause())
Copy link
Member

@cston cston Dec 5, 2022

Choose a reason for hiding this comment

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

&& IsCurrentTokenWhereOfConstraintClause()

Is this an error recovery scenario that is specific to class and struct or could we get here with record too? Are we testing the record case? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Dec 5, 2022

Choose a reason for hiding this comment

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

Is this an error recovery scenario that is specific to class and struct or could we get here with record too?

This recovery is not specific to any particular type declaration.

Are we testing the record case?

No. Do you think we have to?

public void Class_ParameterListAfterGenericTypeParameters_01(bool @struct)
{
var text = @"
" + (@struct ? "struct" : "class") + @" C<T>(T x);";
Copy link
Member

@cston cston Dec 5, 2022

Choose a reason for hiding this comment

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

C(T x);"

Are there tests for class C<T>; and class C<T> where T : U;? #Resolved

var text = @"
" + (@struct ? "struct" : "class") + @" C
<T,
(T x);";
Copy link
Member

@cston cston Dec 5, 2022

Choose a reason for hiding this comment

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

Consider testing class C<T>();, class C<>;, class C<>(); #Resolved

@AlekseyTs
Copy link
Contributor Author

@jjonescz, @dotnet/roslyn-compiler For the second review

@AlekseyTs AlekseyTs merged commit 8c2cfad into dotnet:features/PrimaryConstructors Dec 6, 2022
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.

4 participants