Skip to content

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.

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

Merged
merged 12 commits into from
Jun 12, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 135 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Crefs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ private ImmutableArray<Symbol> BindCrefInternal(CrefSyntax syntax, out Symbol? a
case SyntaxKind.IndexerMemberCref:
case SyntaxKind.OperatorMemberCref:
case SyntaxKind.ConversionOperatorMemberCref:
case SyntaxKind.ExtensionMemberCref:
return BindMemberCref((MemberCrefSyntax)syntax, containerOpt: null, ambiguityWinner: out ambiguityWinner, diagnostics: diagnostics);
default:
throw ExceptionUtilities.UnexpectedValue(syntax.Kind());
Expand Down Expand Up @@ -125,6 +126,9 @@ private ImmutableArray<Symbol> BindMemberCref(MemberCrefSyntax syntax, Namespace
case SyntaxKind.ConversionOperatorMemberCref:
result = BindConversionOperatorMemberCref((ConversionOperatorMemberCrefSyntax)syntax, containerOpt, out ambiguityWinner, diagnostics);
break;
case SyntaxKind.ExtensionMemberCref:
result = BindExtensionMemberCref((ExtensionMemberCrefSyntax)syntax, containerOpt, out ambiguityWinner, diagnostics);
break;
default:
throw ExceptionUtilities.UnexpectedValue(syntax.Kind());
}
Expand Down Expand Up @@ -216,6 +220,137 @@ private ImmutableArray<Symbol> BindIndexerMemberCref(IndexerMemberCrefSyntax syn
diagnostics: diagnostics);
}

private ImmutableArray<Symbol> BindExtensionMemberCref(ExtensionMemberCrefSyntax syntax, NamespaceOrTypeSymbol? containerOpt, out Symbol? ambiguityWinner, BindingDiagnosticBag diagnostics)
{
if (containerOpt is not NamedTypeSymbol namedContainer)
{
ambiguityWinner = null;
return ImmutableArray<Symbol>.Empty;
}

ImmutableArray<Symbol> sortedSymbols = default;
int arity = 0;
TypeArgumentListSyntax? typeArgumentListSyntax = null;
CrefParameterListSyntax? parameters = null;

if (syntax.Member is NameMemberCrefSyntax { Name: SimpleNameSyntax simpleName } nameMember)
{
arity = simpleName.Arity;
typeArgumentListSyntax = simpleName is GenericNameSyntax genericName ? genericName.TypeArgumentList : null;
parameters = nameMember.Parameters;

TypeArgumentListSyntax? extensionTypeArguments = syntax.TypeArgumentList;
int extensionArity = extensionTypeArguments?.Arguments.Count ?? 0;
sortedSymbols = computeSortedAndFilteredCrefExtensionMembers(namedContainer, simpleName.Identifier.ValueText, extensionArity, arity, extensionTypeArguments, diagnostics, syntax);
Copy link
Contributor

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

ValueText

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

}

if (sortedSymbols.IsDefaultOrEmpty)
{
ambiguityWinner = null;
return [];
Copy link
Contributor

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

return [];

Consider leaving a follow up comment to handle operators. #Closed

}

Debug.Assert(sortedSymbols.All(s => s.GetIsNewExtensionMember()));

return ProcessCrefMemberLookupResults(sortedSymbols, arity, syntax, typeArgumentListSyntax, parameters, out ambiguityWinner, diagnostics);

ImmutableArray<Symbol> computeSortedAndFilteredCrefExtensionMembers(NamedTypeSymbol container, string name, int extensionArity, int arity, TypeArgumentListSyntax? extensionTypeArguments, BindingDiagnosticBag diagnostics, ExtensionMemberCrefSyntax syntax)
{
Debug.Assert(name is not null);

LookupOptions options = LookupOptions.AllMethodsOnArityZero | LookupOptions.MustNotBeParameter;
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = this.GetNewCompoundUseSiteInfo(diagnostics);
ArrayBuilder<Symbol>? sortedSymbolsBuilder = null;

foreach (var nested in container.GetTypeMembers())
{
if (!nested.IsExtension || nested.Arity != extensionArity || nested.ExtensionParameter is null)
{
continue;
}

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

var candidateExtensionSignature = new SignatureOnlyMethodSymbol(
Copy link
Contributor

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

var candidateExtensionSignature = new SignatureOnlyMethodSymbol(

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

methodKind: MethodKind.Ordinary,
typeParameters: IndexedTypeParameterSymbol.TakeSymbols(constructedNested.Arity),
parameters: [constructedNested.ExtensionParameter],
callingConvention: Cci.CallingConvention.Default,
// These are ignored by this specific MemberSignatureComparer.
containingType: null,
name: null,
refKind: RefKind.None,
isInitOnly: false,
isStatic: false,
returnType: default,
refCustomModifiers: [],
explicitInterfaceImplementations: []);

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

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

syntax.Parameters is { } extensionParameterListSyntax

Can this be false? #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

BindCrefParameters(extensionParameterListSyntax, diagnostics)

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


var providedExtensionSignature = new SignatureOnlyMethodSymbol(
Copy link
Contributor

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

providedExtensionSignature

It looks like we can initialize outside of the loop #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

We need an arity which is specific to an iteration of the loop

Copy link
Contributor

Choose a reason for hiding this comment

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

We need an arity which is specific to an iteration of the loop

Don't we make sure nested.Arity == extensionArity when we reach this point, and extensionArity doesn't change in the loop?

methodKind: MethodKind.Ordinary,
typeParameters: IndexedTypeParameterSymbol.TakeSymbols(constructedNested.Arity),
parameters: extensionParameterSymbols,
callingConvention: Cci.CallingConvention.Default,
// These are ignored by this specific MemberSignatureComparer.
containingType: null,
name: null,
refKind: RefKind.None,
isInitOnly: false,
isStatic: false,
returnType: default,
refCustomModifiers: [],
explicitInterfaceImplementations: []);

if (!MemberSignatureComparer.CrefComparer.Equals(candidateExtensionSignature, providedExtensionSignature))
{
continue;
}

var candidates = constructedNested.GetMembers(name);

foreach (var candidate in candidates)
{
if (!SourceMemberContainerTypeSymbol.IsAllowedExtensionMember(candidate))
{
continue;
}

if (arity != 0 && candidate.GetArity() != arity)
{
continue;
}

// Note: we bypass the arity check here, as it would check for total arity (extension + member arity)
SingleLookupResult result = this.CheckViability(candidate, arity: 0, options, accessThroughType: null, diagnose: true, useSiteInfo: ref useSiteInfo);

if (result.Kind == LookupResultKind.Viable)
{
sortedSymbolsBuilder ??= ArrayBuilder<Symbol>.GetInstance();
sortedSymbolsBuilder.Add(result.Symbol);
}
}
}

diagnostics.Add(syntax, useSiteInfo);

if (sortedSymbolsBuilder is null)
{
return ImmutableArray<Symbol>.Empty;
}

// Since we resolve ambiguities by just picking the first symbol we encounter,
// the order of the symbols matters for repeatability.
if (sortedSymbolsBuilder.Count > 1)
{
sortedSymbolsBuilder.Sort(ConsistentSymbolOrder.Instance);
}

return sortedSymbolsBuilder.ToImmutableAndFree();
}
}

// NOTE: not guaranteed to be a method (e.g. class op_Addition)
// NOTE: constructor fallback logic applies
private ImmutableArray<Symbol> BindOperatorMemberCref(OperatorMemberCrefSyntax syntax, NamespaceOrTypeSymbol? containerOpt, out Symbol? ambiguityWinner, BindingDiagnosticBag diagnostics)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ private void AddTypeParameters(TypeSyntax typeSyntax, MultiDictionary<string, Ty
AddTypeParameters(qualifiedNameSyntax.Left, map);
break;
case SyntaxKind.GenericName:
AddTypeParameters((GenericNameSyntax)typeSyntax, map);
AddTypeParameters(((GenericNameSyntax)typeSyntax).TypeArgumentList.Arguments, map);
break;
case SyntaxKind.IdentifierName:
case SyntaxKind.PredefinedType:
Expand All @@ -103,20 +103,28 @@ private void AddTypeParameters(TypeSyntax typeSyntax, MultiDictionary<string, Ty
private void AddTypeParameters(MemberCrefSyntax memberSyntax, MultiDictionary<string, TypeParameterSymbol> map)
{
// Other members have arity 0.
if (memberSyntax.Kind() == SyntaxKind.NameMemberCref)
if (memberSyntax is NameMemberCrefSyntax nameMemberCref)
{
AddTypeParameters(((NameMemberCrefSyntax)memberSyntax).Name, map);
AddTypeParameters(nameMemberCref.Name, map);
}
else if (memberSyntax is ExtensionMemberCrefSyntax extensionCref)
{
if (extensionCref.TypeArgumentList is { } extensionTypeArguments)
{
AddTypeParameters(extensionTypeArguments.Arguments, map);
}

AddTypeParameters(extensionCref.Member, map);
}
}

private static void AddTypeParameters(GenericNameSyntax genericNameSyntax, MultiDictionary<string, TypeParameterSymbol> map)
private static void AddTypeParameters(SeparatedSyntaxList<TypeSyntax> typeArguments, MultiDictionary<string, TypeParameterSymbol> map)
{
// NOTE: Dev11 does not warn about duplication, it just matches parameter types to the
// *last* type parameter with the same name. That's why we're iterating backwards and
// skipping subsequent symbols with the same name. This can result in some surprising
// behavior. For example, both 'T's in "A<T>.B<T>" bind to the second implicitly
// declared type parameter.
SeparatedSyntaxList<TypeSyntax> typeArguments = genericNameSyntax.TypeArgumentList.Arguments;
for (int i = typeArguments.Count - 1; i >= 0; i--)
{
// Other types (non-identifiers) are allowed in error scenarios, but they do not introduce new
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,8 @@ internal static bool HasParameterList(CrefSyntax crefSyntax)
return ((OperatorMemberCrefSyntax)crefSyntax).Parameters != null;
case SyntaxKind.ConversionOperatorMemberCref:
return ((ConversionOperatorMemberCrefSyntax)crefSyntax).Parameters != null;
case SyntaxKind.ExtensionMemberCref:
return HasParameterList(((ExtensionMemberCrefSyntax)crefSyntax).Member);
Copy link
Contributor

@AlekseyTs AlekseyTs May 30, 2025

Choose a reason for hiding this comment

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

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

What about ExtensionMemberCrefSyntax.Parameters? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

return false;
Expand All @@ -379,7 +381,7 @@ private static SymbolInfo GetCrefSymbolInfo(OneOrMany<Symbol> symbols, SymbolInf

LookupResultKind resultKind = LookupResultKind.Ambiguous;

// The boundary between Ambiguous and OverloadResolutionFailure is let clear-cut for crefs.
// The boundary between Ambiguous and OverloadResolutionFailure is less clear-cut for crefs.
// We'll say that overload resolution failed if the syntax has a parameter list and if
// all of the candidates have the same kind.
SymbolKind firstCandidateKind = symbols[0].Kind;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public override object VisitNamedType(NamedTypeSymbol symbol, StringBuilder buil
builder.Append('.');
}

builder.Append(symbol.IsExtension ? symbol.ExtensionName : symbol.Name);
builder.Append(symbol.IsExtension ? escape(symbol.ExtensionName) : symbol.Name);
Copy link
Member Author

@jcouv jcouv May 29, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some additional thinking about this issue. Now, I think we need to consider possible usage scenarios for the ids that we produce here. What users actually might want to do with them. On one hand, the ids are included into an XML document and reserved XML characters must be replaced with XML entities inside the document. On the other hand, if I am not mistaken, when an XML document is read by an XML reader, the reader replaces the XML entities back to the corresponding characters. Is there a scenario when one tries to match an id gotten from an XML reader with an id that we produce here and exposing through GetDocumentationCommentId API? Or vise versa? If so, doing this transformation here is not the right thing to do. The replacement for </> with {/} for type argument lists is different, it doesn't involve XML entities, an XML reader wouldn't reverse it.

I am leaning towards doing one of the following:

  • Moving this "escaping" to the step that serializes an XML document. Basically, during serialization all ids should be checked for presence of </> and they should be replaced with corresponding XML entities.
  • Or we can by pass the problem by simply avoiding using reserved XML characters in unspeakable extension names that we generate.

Copy link
Member Author

Choose a reason for hiding this comment

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

One additional data point: in error scenarios, we already escape CREF strings in ToBadCrefString (that's done outside of GetDocumentationCommentId.
I'll move the escaping outside for now. We can discuss the other option (using a different unspeakable name) in WG.

Copy link
Contributor

@AlekseyTs AlekseyTs Jun 2, 2025

Choose a reason for hiding this comment

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

escape(symbol.ExtensionName)

It looks like now one can refer to an extension type by name across assembly boundaries. I.e. doc comments for one assembly can refer to an extension type defined in a different assembly by using its unspeakable name. BTW, are we testing this scenario? Should a reference like this be considered a subject for a binary breaking change when a source change in the referenced assembly causes a change in the unspeakable name? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Should a reference like this be considered a subject for a binary breaking change when a source change in the referenced assembly causes a change in the unspeakable name?

I have a follow-up to discuss with WG: "Are extension metadata names problematic for versioning docs?".
Yes, I think the current scheme means that shuffling extensions around breaks CREF/XML doc consumers.


if (symbol.Arity != 0)
{
Expand Down Expand Up @@ -215,6 +215,12 @@ public override object VisitNamedType(NamedTypeSymbol symbol, StringBuilder buil
}

return null;

static string escape(string s)
{
Debug.Assert(!s.Contains("&"));
return s.Replace("<", "&lt;").Replace(">", "&gt;");
}
}

public override object VisitPointerType(PointerTypeSymbol symbol, StringBuilder builder)
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading