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

Parse parameter list on an interface in error recovery mode #66940

Merged
merged 2 commits into from
Feb 21, 2023
Merged

Parse parameter list on an interface in error recovery mode #66940

merged 2 commits into from
Feb 21, 2023

Conversation

AlekseyTs
Copy link
Contributor

@AlekseyTs AlekseyTs commented Feb 17, 2023 •
edited
Loading

This also includes the following public API changes:

  • TypeDeclarationSyntax.ParameterList becomes abstract
  • TypeDeclarationSyntax.WithParameterList is added
  • TypeDeclarationSyntax.AddParameterListParameters is added
  • InterfaceDeclarationSyntax.ParameterList is added as an override
  • InterfaceDeclarationSyntax.WithParameterList is added
  • InterfaceDeclarationSyntax.AddParameterListParameters is added
  • new overload of InterfaceDeclarationSyntax.Update is added
  • new overload of SyntaxFactory.InterfaceDeclaration is added
  • ClassDeclarationSyntax.ParameterList is changed to an override
  • StructDeclarationSyntax.ParameterList is changed to an override
  • RecordDeclarationSyntax.ParameterList is changed to an override. This API was previously shipped as a regular property.

@AlekseyTs
Parse parameter list on an interface in error recovery mode
7330315 
This also includes the following public API changes:
- TypeDeclarationSyntax.ParameterList becomes abstract
- TypeDeclarationSyntax.WithParameterList is added
- TypeDeclarationSyntax.AddParameterListParameters is added
- InterfaceDeclarationSyntax.ParameterList is added as an override
- InterfaceDeclarationSyntax.WithParameterList is added
- InterfaceDeclarationSyntax.AddParameterListParameters is added
- new overload of InterfaceDeclarationSyntax.Update is added
- new overload of SyntaxFactory.InterfaceDeclaration is added
- ClassDeclarationSyntax.ParameterList is changed to an override
- StructDeclarationSyntax.ParameterList is changed to an override
- RecordDeclarationSyntax.ParameterList is changed to an override. This API was previousli shipped as a regular property.
@AlekseyTs AlekseyTs requested review from a team as code owners February 17, 2023 15:13
CyrusNajmabadi
CyrusNajmabadi reviewed Feb 17, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Declarations/DeclarationTreeBuilder.cs
MessageID.IDS_FeaturePrimaryConstructors.CheckFeatureAvailability(diagnostics, node.ParameterList);
if (node.Kind() is SyntaxKind.InterfaceDeclaration)
{
diagnostics.Add(ErrorCode.ERR_UnexpectedParameterList, node.ParameterList.GetLocation());
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 17, 2023 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

nice. glad this is not in the parser. #Resolved

CyrusNajmabadi
CyrusNajmabadi reviewed Feb 17, 2023
View reviewed changes
src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs
@@ -2,6 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable disable
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 17, 2023 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

curious why add nullable disable? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

curious why add nullable disable?

Nullable analysis has zero value in tests. And adds an extra const of suppressing warnings.

@AlekseyTs
Copy link
Contributor Author

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

@cston
Copy link
Member

cston commented Feb 18, 2023

Are these API changes based on #66914? In that issue, TypeDeclarationSyntax.ParameterList is proposed as a non-abstract property and ClassDeclarationSyntax.ParameterList and StructDeclarationSyntax.ParameterList are new properties rather than override.

cston
cston reviewed Feb 18, 2023
View reviewed changes
src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs
// (1,12): error CS8803: Top-level statements must precede namespace and type declarations.
// interface P(int x, int y);
Diagnostic(ErrorCode.ERR_TopLevelStatementAfterNamespaceOrType, "(int x, int y);").WithLocation(1, 12));
tree.GetDiagnostics().Verify();
Copy link
Member

@cston cston Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

tree.GetDiagnostics().Verify();

Consider verifying the shape of the syntax tree.

UsingNode(tree.GetRoot());

N(SyntaxKind.CompilationUnit);
...

And consider testing with options: TestOptions.RegularPreview as well.

@AlekseyTs
Copy link
Contributor Author

Are these API changes based on #66914? In that issue, TypeDeclarationSyntax.ParameterList is proposed as a non-abstract property and ClassDeclarationSyntax.ParameterList and StructDeclarationSyntax.ParameterList are new properties rather than override.

The issue reflects the state of the API before this PR. This PR is an attempt to follow-up on feedback that was provided during its review.

@AlekseyTs AlekseyTs enabled auto-merge (squash) February 21, 2023 16:39
@AlekseyTs AlekseyTs merged commit cfe9f31 into dotnet:features/PrimaryConstructors Feb 21, 2023
@AlekseyTs AlekseyTs mentioned this pull request Feb 21, 2023
Closed
@allisonchou allisonchou mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@cston cston cston approved these changes

@jjonescz jjonescz jjonescz approved these changes

Assignees
No one assigned
Labels
Area-Compilers Feature - Primary Constructors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlekseyTs @cston @jjonescz @CyrusNajmabadi