Extensions parsing #76867

jcouv · 2025-01-22T22:18:46Z

Relates to test plan #76130

CyrusNajmabadi · 2025-01-22T23:16:48Z

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4

@@ -81,6 +81,7 @@ member_declaration
  | base_type_declaration
  | delegate_declaration
  | enum_member_declaration
+  | extension_container


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

CyrusNajmabadi · 2025-01-22T23:17:11Z

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4

@@ -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* '}'


syntax_token? not 'extension'? #Resolved

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

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

CyrusNajmabadi · 2025-01-22T23:18:41Z

src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4

+  ;
+
+receiver_parameter
+  : attribute_list* modifier* type identifier_token?


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

CyrusNajmabadi · 2025-01-22T23:20:01Z

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

@@ -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


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

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 ;-)

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

gtk about TODO2. i didn't realize that.

CyrusNajmabadi · 2025-01-22T23:20:22Z

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

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

+        private MemberDeclarationSyntax ParseExtensionContainer(SyntaxList<AttributeListSyntax> attributes, SyntaxListBuilder modifiers)


make return type strongly typed. #Resolved

CyrusNajmabadi · 2025-01-22T23:20:48Z

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

+            var outerSaveTerm = _termState;
+            _termState |= TerminatorState.IsPossibleAggregateClauseStartOrStop; // TODO2 add test affected
+
+            TypeParameterListSyntax typeParameters = this.ParseTypeParameterList();


check for identifier and recover gracefully. #Resolved

CyrusNajmabadi · 2025-01-22T23:22:18Z

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

+            _termState |= TerminatorState.IsPossibleAggregateClauseStartOrStop; // TODO2 add test affected
+
+            TypeParameterListSyntax typeParameters = this.ParseTypeParameterList();
+            ReceiverParameterSyntax receiverParameter = parseReceiverParameter(out var openParenToken, out var closeParenToken);


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

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

CyrusNajmabadi · 2025-01-22T23:22:59Z

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

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


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

CyrusNajmabadi · 2025-01-22T23:23:25Z

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

+                    parseMembers = false;
+                }
+
+                if (parseMembers)


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

CyrusNajmabadi · 2025-01-22T23:24:03Z

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

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


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

CyrusNajmabadi · 2025-01-22T23:24:18Z

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

+                    }
+                }
+
+                if (openBrace.IsMissing)


here you check the openBrace, instead of parseMembers #Resolved

CyrusNajmabadi · 2025-01-22T23:26:29Z

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

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


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

CyrusNajmabadi · 2025-01-22T23:27:48Z

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

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


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

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

CyrusNajmabadi · 2025-01-22T23:29:06Z

src/Compilers/Test/Core/Traits/CompilerFeature.cs

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


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

CyrusNajmabadi · 2025-01-22T23:29:52Z

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

AlekseyTs · 2025-01-23T23:41:20Z

        TestOptions.RegularNext);

Consider testing with RegularPreview as well. #Closed

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

AlekseyTs · 2025-01-23T23:43:07Z

                        M(SyntaxKind.IdentifierToken);

The missing identifier feels unexpected #Closed

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

AlekseyTs · 2025-01-23T23:55:02Z

Done with review pass (commit 9)

jcouv · 2025-01-24T01:17:38Z

    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 · 2025-01-24T01:20:51Z

    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 · 2025-01-24T01:21:38Z

    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 · 2025-01-24T01:22:16Z

    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 · 2025-01-24T01:28:12Z

    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)

jcouv · 2025-01-24T09:06:14Z

@jjonescz for second review. Thanks

CyrusNajmabadi · 2025-01-24T09:16:05Z

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

+    <Field Name="AttributeLists" Type="SyntaxList&lt;AttributeListSyntax&gt;" Override="true"/>
+    <Field Name="Modifiers" Type="SyntaxList&lt;SyntaxToken&gt;" Override="true"/>
+    <Field Name="Keyword" Type="SyntaxToken" Override="true">
+      <ContextualKind Name="ExtensionKeyword"/>


i'm actually slightly surprised by this. i would have thought this would be <kind #Resolved

jjonescz · 2025-01-24T13:21:24Z

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

+    <Field Name="CloseBraceToken" Type="SyntaxToken" Override="true" Optional="true">
+      <Kind Name="CloseBraceToken"/>
+    </Field>
+    <Field Name="SemicolonToken" Type="SyntaxToken" Optional="true" Override="true">


Why is the semicolon token allowed here? It's not in the speclet. #Resolved

I kept it for consistency and simplicity with the other declaration kinds. Will update the speclet.

jjonescz · 2025-01-24T13:29:09Z

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


                default:
                    throw ExceptionUtilities.UnexpectedValue(this.CurrentToken.Kind);
            }
        }

-        private TypeDeclarationSyntax ParseClassOrStructOrInterfaceDeclaration(SyntaxList<AttributeListSyntax> attributes, SyntaxListBuilder modifiers)
+        private TypeDeclarationSyntax ParseClassOrStructOrInterfaceOrExtensionDeclaration(SyntaxList<AttributeListSyntax> attributes, SyntaxListBuilder modifiers)


Looks like this could be renamed to ParseTypeDeclaration as well #Resolved

That name would conflict. Shortened as ParseMainTypeDeclaration

AlekseyTs · 2025-01-24T17:39:16Z

src/Compilers/CSharp/Portable/Syntax/ExtensionDeclarationSyntax.cs

+        public override SyntaxToken Identifier => default;
+
+        internal override BaseTypeDeclarationSyntax WithIdentifierCore(SyntaxToken identifier)
+            => throw new System.NotImplementedException();


NotImplementedException

If we decided to throw, NotSupportedException is probably a better choice here and below

I am not sure why this thread was marked as resolved.

AlekseyTs · 2025-01-24T18:44:26Z

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

+            // as all normal parameters are legal extension parameters.
+            if (parameter.Identifier.Kind() == SyntaxKind.None)
+            {
+                return false;


return false

It looks like in every place where the enclosing method is called, we know whether missing identifier is allowed. We can pass this information and take advantage of that here. #Closed

AlekseyTs · 2025-01-24T19:09:27Z

It looks like there are legitimate test failures in IOperation leg. #Closed

AlekseyTs · 2025-01-24T19:09:59Z

Done with review pass (commit 13).

AlekseyTs · 2025-01-24T20:43:49Z

src/Compilers/CSharp/Portable/Syntax/ExtensionDeclarationSyntax.cs

+    public partial class ExtensionDeclarationSyntax
+    {
+        public override SyntaxToken Identifier
+            => throw new System.NotImplementedException();


throw new System.NotImplementedException();

I think returning default was the right thing to do. A call through base class should be allowed.

It is, however, Ok to throw when one tries to set identifier.

AlekseyTs · 2025-01-24T20:44:07Z

src/Compilers/CSharp/Portable/Syntax/ExtensionDeclarationSyntax.cs

+            => throw new System.NotImplementedException();
+
+        public override BaseListSyntax? BaseList
+            => throw new System.NotImplementedException();


throw new System.NotImplementedException();

Same comment here.

AlekseyTs · 2025-01-24T20:47:05Z

Done with review pass (commit 17)

AlekseyTs · 2025-01-24T22:09:12Z

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

+            // We can only reuse parameters without identifiers (found in extension declarations) in context that allow optional identifiers.
+            // The reverse is fine though.  Normal parameters (from non extensions) can be re-used into an extension declaration
+            // as all normal parameters are legal extension parameters.
+            if (!allowOptionalIdentifier && parameter.Identifier.Kind() == SyntaxKind.None)


!allowOptionalIdentifier &&

It doesn't look like this optimization is covered by tests. Otherwise, I would expect one of the incremental parsing tests to fail (since none were adjusted).

Extensions parsing

d6b6d13

jcouv added the Feature - Extension Everything The extension everything feature label Jan 22, 2025

jcouv self-assigned this Jan 22, 2025

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 bot added the Needs API Review Needs to be reviewed by the API review council label Jan 22, 2025

jcouv added 3 commits January 22, 2025 15:11

More tests

1142e50

Look for affected tests

ade3728

Look for affected tests

19ca312

CyrusNajmabadi reviewed Jan 22, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 22, 2025

View reviewed changes

Re-use TypeDeclarationSyntax and ParameterSyntax

8e29f61

jcouv force-pushed the extensions-parsing branch from 15cf64b to 8e29f61 Compare January 23, 2025 14:28

Tweaks

004ee68

jcouv marked this pull request as ready for review January 23, 2025 16:38

jcouv requested a review from a team as a code owner January 23, 2025 16:38

AlekseyTs and others added 4 commits January 23, 2025 17:32

Remove ExtensionDeclaration.Identifier

e77570f

Address feedback

995a1be

Update incremental tests

0d45252

Tweaks

0f90f6e

jcouv requested a review from AlekseyTs January 24, 2025 09:03

CyrusNajmabadi reviewed Jan 24, 2025

View reviewed changes

jjonescz reviewed Jan 24, 2025

View reviewed changes

Address feedback

8f6b37a

jcouv requested a review from a team as a code owner January 24, 2025 16:35

AlekseyTs reviewed Jan 24, 2025

View reviewed changes

Skip some tests in IOperation

3509847

jcouv requested a review from jjonescz January 24, 2025 19:42

jcouv added 2 commits January 24, 2025 11:44

Throw

cf799f7

CanReuseParameter

5240e8c

AlekseyTs reviewed Jan 24, 2025

View reviewed changes

Extensions parsing #76867

Are you sure you want to change the base?

Extensions parsing #76867

Conversation

jcouv commented Jan 22, 2025 • edited Loading

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Jan 22, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 22, 2025 • edited by jcouv Loading

AlekseyTs commented Jan 23, 2025 • edited Loading

AlekseyTs commented Jan 23, 2025 • edited Loading

AlekseyTs commented Jan 23, 2025

jcouv commented Jan 24, 2025

AlekseyTs commented Jan 24, 2025

AlekseyTs commented Jan 24, 2025

jcouv commented Jan 24, 2025

AlekseyTs commented Jan 24, 2025

jcouv commented Jan 24, 2025

CyrusNajmabadi Jan 24, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

jjonescz Jan 24, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Jan 24, 2025 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 24, 2025 • edited Loading

AlekseyTs commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jan 24, 2025

Choose a reason for hiding this comment

jcouv commented Jan 22, 2025 •

edited

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi Jan 22, 2025 •

edited by jcouv

Loading

CyrusNajmabadi commented Jan 22, 2025 •

edited by jcouv

Loading

AlekseyTs commented Jan 23, 2025 •

edited

Loading

AlekseyTs commented Jan 23, 2025 •

edited

Loading

CyrusNajmabadi Jan 24, 2025 •

edited by jcouv

Loading

jjonescz Jan 24, 2025 •

edited by jcouv

Loading

jjonescz Jan 24, 2025 •

edited by jcouv

Loading

AlekseyTs Jan 24, 2025 •

edited

Loading

AlekseyTs Jan 24, 2025 •

edited

Loading

AlekseyTs commented Jan 24, 2025 •

edited

Loading