-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes from 1 commit
15fdb0b
e6876dd
6441497
6cbca86
c139bae
015a342
e59e48c
9a29064
26f047b
5d277a9
abdae5b
f02ef4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()); | ||
|
@@ -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()); | ||
} | ||
|
@@ -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); | ||
} | ||
|
||
if (sortedSymbols.IsDefaultOrEmpty) | ||
{ | ||
ambiguityWinner = null; | ||
return []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
var providedExtensionSignature = new SignatureOnlyMethodSymbol( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't we make sure |
||
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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parameters on |
||
} | ||
|
||
return false; | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we replace There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I am leaning towards doing one of the following:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I have a follow-up to discuss with WG: "Are extension metadata names problematic for versioning docs?". |
||
|
||
if (symbol.Arity != 0) | ||
{ | ||
|
@@ -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("<", "<").Replace(">", ">"); | ||
} | ||
} | ||
|
||
public override object VisitPointerType(PointerTypeSymbol symbol, StringBuilder builder) | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test with escaped identifier at this position? #Closed