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

Extensions parsing #76867

Open
wants to merge 9 commits into
base: features/extensions
Choose a base branch
from
Open

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Jan 22, 2025

Relates to test plan #76130

@jcouv jcouv added the Feature - Extension Everything The extension everything feature label Jan 22, 2025
@jcouv jcouv self-assigned this Jan 22, 2025
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 22, 2025
@dotnet-policy-service dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Jan 22, 2025
@@ -81,6 +81,7 @@ member_declaration
| base_type_declaration
| delegate_declaration
| enum_member_declaration
| extension_container
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

don't love that it's not called _declaration. #Resolved

@@ -348,6 +349,14 @@ delegate_declaration
: attribute_list* modifier* 'delegate' type identifier_token type_parameter_list? parameter_list type_parameter_constraint_clause* ';'
;

extension_container
: attribute_list* modifier* syntax_token type_parameter_list? '(' receiver_parameter ')' type_parameter_constraint_clause* '{' member_declaration* '}'
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

syntax_token? not 'extension'? #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

nit: allow an identifier. people will try to write it, and it will allow for better error recovery.

Copy link
Member

Choose a reason for hiding this comment

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

nit: allow a parameter_list, as people will write more, and it will enable better error recovery.

;

receiver_parameter
: attribute_list* modifier* type identifier_token?
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

we should allow a parameter_syntax. it will make it much easier ot recover and represent throughout the system. (similar to how even a => a uses a parameter_syntax).

overfitting syntax is often problematic. both for normal typing cases, and for expanding on this in the future. #Resolved

@@ -1732,7 +1732,7 @@ private TypeDeclarationSyntax ParseClassOrStructOrInterfaceDeclaration(SyntaxLis
_termState |= TerminatorState.IsEndOfRecordOrClassOrStructOrInterfaceSignature;

var saveTerm = _termState;
_termState |= TerminatorState.IsPossibleAggregateClauseStartOrStop;
//_termState |= TerminatorState.IsPossibleAggregateClauseStartOrStop; // TODO2 looking for affected test
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

mark as prototype? so this doesn't get merged in? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is draft. Not ready for review. TODO2 comments are flagged by our build and are useful to track things that need to be handled in the PR ;-)

Copy link
Member

Choose a reason for hiding this comment

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

oh. gmail didn't mark this as draft. sorry!

gtk about TODO2. i didn't realize that.

@@ -1962,6 +1963,151 @@ static TypeDeclarationSyntax constructTypeDeclaration(ContextAwareSyntax syntaxF
}
}

private MemberDeclarationSyntax ParseExtensionContainer(SyntaxList<AttributeListSyntax> attributes, SyntaxListBuilder modifiers)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

make return type strongly typed. #Resolved

var outerSaveTerm = _termState;
_termState |= TerminatorState.IsPossibleAggregateClauseStartOrStop; // TODO2 add test affected

TypeParameterListSyntax typeParameters = this.ParseTypeParameterList();
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

check for identifier and recover gracefully. #Resolved

_termState |= TerminatorState.IsPossibleAggregateClauseStartOrStop; // TODO2 add test affected

TypeParameterListSyntax typeParameters = this.ParseTypeParameterList();
ReceiverParameterSyntax receiverParameter = parseReceiverParameter(out var openParenToken, out var closeParenToken);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

parse a parameter list, with a requirement that at least a single parameter is required. This can be done by updating ParseParameterList to take a parameter stating if a single parameter is required (and passing that to hte hlper it calls).

you can optionally error at parse time or binding time if there is >1 parameter. I recommend binding time. #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

By requiring a single parameter, you can be sure in later phases that it is safe to access [0]

_termState |= TerminatorState.IsEndOfRecordOrClassOrStructOrInterfaceSignature;
constraints = _pool.Allocate<TypeParameterConstraintClauseSyntax>();
this.ParseTypeParameterConstraintClauses(constraints);
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

extract this into helper. otehrwise it can get out of sync with type parameter parsing elsewhere. #Resolved

parseMembers = false;
}

if (parseMembers)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

having a variable to just expand on if (!openBrace.IsMissing) seems excessive. #Resolved


var modifiersList = (SyntaxList<SyntaxToken>)modifiers.ToList();
var membersList = (SyntaxList<MemberDeclarationSyntax>)members;
var constraintsList = (SyntaxList<TypeParameterConstraintClauseSyntax>)constraints;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

this is confusing. we have the .ToListAndFree helpers. Why not use those? then you don't need a try/finally either. #Resolved

}
}

if (openBrace.IsMissing)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

here you check the openBrace, instead of parseMembers #Resolved

{
members = _pool.Allocate<MemberDeclarationSyntax>();

while (true)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

afaict, this is a copy of ParseClassOrStructOrInterfaceDeclaration. Seems like we can just expand that to parse extensions, just with some extension-specific code. Note: as all teh rest support the primary cosntructor parameter list, this all falls out. The only difference that i can tell so far is simple that in the case of extensions we want at least one parameter (though this could be a binding-time check), and that we do not have an identifier. All of that seems similar to add to ParseClassOrStructOrInterfaceDeclaration vs this copy :) #Resolved

SyntaxToken? identifier;
if (this.CurrentToken.Kind == SyntaxKind.CloseParenToken)
{
identifier = null;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

ah, you want identifiers to be optional. mixed feelings. but that feels very doable still :) #Resolved

@@ -43,5 +43,6 @@ public enum CompilerFeature
RecordStructs,
RequiredMembers,
RefLifetime,
Extensions,
}
}
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2025

Choose a reason for hiding this comment

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

incremental tests that changing class C(int i) to extension(int i) and vvice versa works would be good.
#Resolved

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jan 22, 2025

Overall, this looks quite good to me. My pref is to share more int eh parser. either higher level parsing functinos, or lower level shared routines :) #Resolved

@jcouv jcouv force-pushed the extensions-parsing branch from 15cf64b to 8e29f61 Compare January 23, 2025 14:28
@jcouv jcouv marked this pull request as ready for review January 23, 2025 16:38
@jcouv jcouv requested a review from a team as a code owner January 23, 2025 16:38
@jcouv jcouv changed the base branch from main to features/extensions January 23, 2025 19:02
}

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

Choose a reason for hiding this comment

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

BaseListSyntax? baseList

This parameter is unexpected

@jcouv
Copy link
Member Author

jcouv commented Jan 23, 2025

@jcouv We probably should create a new feature branch for the new approach.

Yes. I've created features/extensions, but forgot to target this PR to that branch. Fixed

/// <item><description><see cref="SyntaxKind.ExtensionDeclaration"/></description></item>
/// </list>
/// </remarks>
public sealed partial class ExtensionDeclarationSyntax : TypeDeclarationSyntax
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2025

Choose a reason for hiding this comment

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

public sealed partial class ExtensionDeclarationSyntax

Is this supposed to be added to the PublicAPI.Unshipped.txt? #Resolved

skipBadParameterListTokens,
allowTrailingSeparator: false,
requireOneElement: false,
requireOneElement: forExtension, // For extension declarations, we require one receiver parameter
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2025

Choose a reason for hiding this comment

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

requireOneElement: forExtension, // For extension declarations, we require one receiver parameter

It looks like the meaning of this parameter is to ensure that the syntax list is not empty, i.e. at least one item is expected. That doesn't mean that the helper won't accept more that one item. The comment, however, might be interpreted as though we instruct the helper to accept exactly one item (no more, no less). #Pending


SyntaxToken? identifier;
if (this.CurrentToken.Kind == SyntaxKind.IdentifierToken && IsCurrentTokenWhereOfConstraintClause())
{
identifier = this.AddError(CreateMissingIdentifierToken(), ErrorCode.ERR_IdentifierExpected);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2025

Choose a reason for hiding this comment

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

identifier = this.AddError(CreateMissingIdentifierToken(), ErrorCode.ERR_IdentifierExpected);

It feels like this error recovery might not be appropriate when identifier is optional. Specifically, we probably do not want to create a missing identifier. #Pending

skipBadParameterListTokens,
allowTrailingSeparator: false,
requireOneElement: false,
requireOneElement: forExtension, // For extension declarations, we require one receiver parameter
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2025

Choose a reason for hiding this comment

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

requireOneElement: forExtension

It feels like it might be good to suppress any parsing diagnostics from syntax errors in the second and any following parameter in the list. Not sure how easy to do this, given that diagnostics is attached to nodes. Consider making a note to follow up later. #Pending

@@ -244,6 +244,7 @@ public static bool IsAnyToken(SyntaxKind kind)
case SyntaxKind.UnderscoreToken:
case SyntaxKind.MultiLineRawStringLiteralToken:
case SyntaxKind.SingleLineRawStringLiteralToken:
case SyntaxKind.ExtensionKeyword:
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 23, 2025

Choose a reason for hiding this comment

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

case SyntaxKind.ExtensionKeyword

Is this entry necessary? It looks like there is no entry for AllowsKeyword. #Pending

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

        TestOptions.RegularNext);

We also test language version behavior with RegularPreview. #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:134 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

        TestOptions.RegularNext);

I think tests that are not focused on language version should target RegularPreview. #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:200 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    TestOptions.RegularNext);

Test with RegularPreview as well? #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:704 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    TestOptions.RegularNext);

Test with RegularPreview as well? #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:744 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

extension(Type, Type) { }

Consider covering more error scenarios:

  • missing comma
  • some other syntax error in the second parameter
  • missing ) (with and without the second parameter)
  • etc.

Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:931 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    const int i = 0;

Do we expect a semantic error?


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1713 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    fixed int field[10];

Do we expect a semantic error? #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1783 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    event EventHandler eventField;

Do we expect a semantic error? #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1857 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    class D { }

Do we expect a semantic error?


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1921 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

    var comp = CreateCompilation(src, parseOptions: TestOptions.RegularNext);

Consider testing with RegularPreview as well. #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3261 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 23, 2025

        TestOptions.RegularNext,

Consider testing with RegularPreview as well. #Pending


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3268 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

        TestOptions.RegularNext);

Consider testing with RegularPreview as well.


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3311 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    var comp = CreateCompilation(src, parseOptions: TestOptions.RegularNext);

Consider testing with RegularPreview as well.


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3397 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    UsingTree(src, TestOptions.RegularNext);

Consider testing with RegularPreview as well.


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3400 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

        TestOptions.RegularNext);

Consider testing with RegularPreview as well.


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3441 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

                        M(SyntaxKind.IdentifierToken);

The missing identifier feels unexpected


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:3676 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 9)

@jcouv
Copy link
Member Author

jcouv commented Jan 24, 2025

    const int i = 0;

I think this may end up allowed, since emitted as static field.


In reply to: 2611226090


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1713 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    const int i = 0;

I think this may end up allowed, since emitted as static field.

The spec doesn't allow fields and we didn't even think what would it mean for implementation. Also, is this going to be an extension field?


In reply to: 2611341439


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1713 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    const int i = 0;

I suggest sticking to the spec for now


In reply to: 2611344424


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1713 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Jan 24, 2025

    class D { }

This may be allowed (TBD)


In reply to: 2611227756


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1921 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    class D { }

This may be allowed (TBD)

I suggest sticking to the spec for now


In reply to: 2611345773


Refers to: src/Compilers/CSharp/Test/Syntax/Parsing/ExtensionsParsingTests.cs:1921 in 51686c8. [](commit_id = 51686c8, deletion_comment = False)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Extension Everything The extension everything feature Needs API Review Needs to be reviewed by the API review council untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants