Extensions: allow cref references to extension members #78735

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

Open

jcouv wants to merge 1 commit into dotnet:main from jcouv:extensions-cref

+2,637 −70

Member

jcouv commented May 28, 2025 •

edited

Loading

Relates to test plan #76130
Review of public API: #78738

jcouv self-assigned this

jcouv added Area-Compilers Feature - Extension Everything labels

jcouv force-pushed the extensions-cref branch from 34b59c7 to 34ef1fb Compare

May 28, 2025 21:34

dotnet-policy-service bot added the Needs API Review label

jcouv mentioned this pull request

Extensions: public API for extension syntax in CREF #78738

Open


          Extensions: allow cref references to extension members

15fdb0b

jcouv force-pushed the extensions-cref branch from 34ef1fb to 15fdb0b Compare

May 28, 2025 23:01

jcouv commented

View reviewed changes

...Compilers/CSharp/Portable/DocumentationComments/DocumentationCommentIDVisitor.PartVisitor.cs

                                   builder.Append('.');
                               }
-                              builder.Append(symbol.IsExtension ? symbol.ExtensionName : symbol.Name);
+                              builder.Append(symbol.IsExtension ? escape(symbol.ExtensionName) : symbol.Name);

Member Author

jcouv May 29, 2025 •

edited

Loading

📝 I debated whether to escape here, or push that responsibility to the caller of DocumentationCommentIDVisitor. What do you think? #Resolved

Contributor

AlekseyTs May 30, 2025

Since we replace </> with {/} for type argument list below, it probably makes sense to avoid using them here as well.

jcouv marked this pull request as ready for review

May 29, 2025 17:30

jcouv requested review from a team as code owners

May 29, 2025 17:30

AlekseyTs reviewed

View reviewed changes

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

+                  <Field Name="DotToken" Type="SyntaxToken">
+                    <Kind Name="DotToken"/>
+                  </Field>
+                  <Field Name="Member" Type="MemberCrefSyntax"/>

Contributor

AlekseyTs May 30, 2025

"MemberCrefSyntax"

This feels too flexible. For example, this cannot be ExtensionMemberCrefSyntax. Could we restrict allowed nodes by kind? If not, perhaps for the syntax model it is fine as is. Is there a corresponding proposal for the grammar change?

AlekseyTs reviewed

View reviewed changes

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

+                          }
+                          SyntaxToken dotToken = EatToken(SyntaxKind.DotToken);
+                          MemberCrefSyntax member = ParseMemberCref();

Contributor

AlekseyTs May 30, 2025

ParseMemberCref()

It looks like this is going to allow an indirect recursion, which is not expected.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

    
                              TypeArgumentListSyntax? extensionTypeArguments = syntax.TypeArgumentList;

                              int extensionArity = extensionTypeArguments?.Arguments.Count ?? 0;

                              sortedSymbols = computeSortedAndFilteredCrefExtensionMembers(namedContainer, simpleName.Identifier.ValueText, extensionArity, arity, extensionTypeArguments, diagnostics, syntax);

Contributor

AlekseyTs May 30, 2025

ValueText

Do we have a test with escaped identifier at this position?

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

    
                          if (sortedSymbols.IsDefaultOrEmpty)

                          {

                              ambiguityWinner = null;

                              return [];

Contributor

AlekseyTs May 30, 2025

return [];

Consider leaving a follow up comment to handle operators.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

    
                                       refCustomModifiers: [],

                                       explicitInterfaceImplementations: []);

                                  ImmutableArray<ParameterSymbol> extensionParameterSymbols = syntax.Parameters is { } extensionParameterListSyntax ? BindCrefParameters(extensionParameterListSyntax, diagnostics) : default;

Contributor

AlekseyTs May 30, 2025

syntax.Parameters is { } extensionParameterListSyntax

Can this be false?

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

    
                                  var constructedNested = (NamedTypeSymbol)ConstructWithCrefTypeParameters(extensionArity, extensionTypeArguments, nested);

                                  var candidateExtensionSignature = new SignatureOnlyMethodSymbol(

Contributor

AlekseyTs May 30, 2025

var candidateExtensionSignature = new SignatureOnlyMethodSymbol(

Consider adding a comment that we are using signature method symbols to match extension blocks

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

    
                                  ImmutableArray<ParameterSymbol> extensionParameterSymbols = syntax.Parameters is { } extensionParameterListSyntax ? BindCrefParameters(extensionParameterListSyntax, diagnostics) : default;

                                  var providedExtensionSignature = new SignatureOnlyMethodSymbol(

Contributor

AlekseyTs May 30, 2025

providedExtensionSignature

It looks like we can initialize outside of the loop

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs

    
                                       refCustomModifiers: [],

                                       explicitInterfaceImplementations: []);

                                  ImmutableArray<ParameterSymbol> extensionParameterSymbols = syntax.Parameters is { } extensionParameterListSyntax ? BindCrefParameters(extensionParameterListSyntax, diagnostics) : default;

Contributor

AlekseyTs May 30, 2025

BindCrefParameters(extensionParameterListSyntax, diagnostics)

It looks like we should attempt to bind parameters even when the enclosing loop never reaches this line.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

                                   return ((OperatorMemberCrefSyntax)crefSyntax).Parameters != null;
                               case SyntaxKind.ConversionOperatorMemberCref:
                                   return ((ConversionOperatorMemberCrefSyntax)crefSyntax).Parameters != null;
+                              case SyntaxKind.ExtensionMemberCref:
+                                  return HasParameterList(((ExtensionMemberCrefSyntax)crefSyntax).Member);

Contributor

AlekseyTs May 30, 2025 •

edited by jcouv

Loading

return HasParameterList(((ExtensionMemberCrefSyntax)crefSyntax).Member);

What about ExtensionMemberCrefSyntax.Parameters? #Resolved

Member Author

jcouv May 30, 2025

Parameters on extension are intentionally not considered here.
The value from HasParameterList affects the result kind from GetCrefSymbolInfo below, to report "ambiguous" vs. "overload resolution failure". In E.extension(int).M... we only allow the last parameter list to be omitted, so the presence/absence of the parameter list on extension is not relevant to was result kind we report.

AlekseyTs reviewed

View reviewed changes

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

+                                      N(SyntaxKind.CloseParenToken);
+                                  }
+                                  N(SyntaxKind.DotToken);
+                                  N(SyntaxKind.ExtensionMemberCref);

Contributor

AlekseyTs May 30, 2025

N(SyntaxKind.ExtensionMemberCref);

The nested ExtensionMemberCref feels wrong

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs

@@ @@ -1915,7 +1915,10 @@ void I.M() { } @@
                       comp.VerifyEmitDiagnostics(
                           // (10,16): error CS0541: 'Extensions.extension(object).M()': explicit interface declaration can only be declared in a class, record, struct or interface
                           //         void I.M() { }
-                          Diagnostic(ErrorCode.ERR_ExplicitInterfaceImplementationInNonClassOrStruct, "M").WithArguments("Extensions.extension(object).M()").WithLocation(10, 16));
+                          Diagnostic(ErrorCode.ERR_ExplicitInterfaceImplementationInNonClassOrStruct, "M").WithArguments("Extensions.extension(object).M()").WithLocation(10, 16),
+                          // (10,16): error CS9282: Extension declarations can include only methods or properties

Contributor

AlekseyTs May 30, 2025

// (10,16): error CS9282: Extension declarations can include only methods or properties

Technically this error reported for a method.

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs

+                  public void Cref_44()
+                  {
+                      var src = """
+              /// <see cref="extension(int).M"/>

Contributor

AlekseyTs May 30, 2025

extension(int).M

Do we have a test for a success scenario with syntax like that?

AlekseyTs reviewed

View reviewed changes

src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs

+              /// <see cref="E.extension(int).M2"/>
+              static class E
+              {
+                  extension(int i)

Contributor

AlekseyTs May 30, 2025

extension(int i)

Consider adding a test where ambiguous candidates are in different blocks.

Contributor

AlekseyTs commented May 30, 2025

Done with review pass (commit 1)

This was referenced May 30, 2025

Compiler crash generating XML docs for extension #78768

Closed

Update XML output for extensions dotnet/csharplang#9416

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything Needs API Review