-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Extension indexers: allow and resolve extension indexers #81607
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
base: features/extensions
Are you sure you want to change the base?
Conversation
37bad6f to
344fa46
Compare
344fa46 to
f504f6e
Compare
|
It looks like the speclet says nothing about consumption scenarios |
Yes, the speclet was not yet updated.
|
| if (member is PropertySymbol) | ||
| { | ||
| diagnostics.Add(new CSDiagnostic( | ||
| new CSDiagnosticInfo(ErrorCode.ERR_ExtensionDisallowsIndexerMember, new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureExtensionIndexers.RequiredVersion())), |
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.
| if (!IsAllowedExtensionMember(member, languageVersion)) | ||
| { | ||
| diagnostics.Add(ErrorCode.ERR_ExtensionDisallowsMember, member.GetFirstLocation()); | ||
| if (member is PropertySymbol) |
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.
| <data name="IDS_FeatureExtensionIndexers" xml:space="preserve"> | ||
| <value>extension indexers</value> | ||
| </data> | ||
| <data name="ERR_ExtensionDisallowsIndexerMember" xml:space="preserve"> |
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.
|
|
||
| internal static void ReportDiagnosticsIfDisallowedExtensionIndexer(BindingDiagnosticBag diagnostics, PropertySymbol property, SyntaxNode syntax) | ||
| { | ||
| if (property.IsExtensionBlockMember()) |
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.
| var setMethod = replace(SetMethod); | ||
| Symbol symbol = ReferenceEquals(Symbol, Method) && method is not null ? method : Symbol; | ||
|
|
||
| Debug.Assert(SetMethod?.IsExtensionBlockMember() != true); |
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.
| lookupResult, WellKnownMemberNames.Indexer, arity: 0, LookupOptions.AllMethodsOnArityZero, | ||
| originalBinder: binder, useSiteInfo: ref useSiteInfo); | ||
|
|
||
| if (!lookupResult.IsMultiViable || lookupResult.Symbols.All(s => s.Kind != SymbolKind.Property)) |
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.
|
|
||
| MemberResolutionResult<PropertySymbol> resolutionResult = result.ValidResult; | ||
| Debug.Assert(resolutionResult.Result.ConversionForArg(0).Exists); | ||
| resolutionResult = resolutionResult.WithResult(resolutionResult.Result.WithoutReceiverArgument()); |
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.
| ArrayBuilder<PropertySymbol>? properties = null; | ||
| foreach (var member in lookupResult.Symbols) | ||
| { | ||
| if (member is PropertySymbol property) |
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.
|
|
||
| overloadResolutionResult.Free(); | ||
| return propertyAccess; | ||
| bool isNewExtensionMember = property.IsExtensionBlockMember(); |
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.
|
|
||
| // Make sure that the result of overload resolution is valid. | ||
| var gotError = MemberGroupFinalValidationAccessibilityChecks(receiver, property, syntax, diagnostics, invokedAsExtensionMethod: false); | ||
| private BoundExpression BindIndexerOrIndexedPropertyAccessContinued(SyntaxNode syntax, BoundExpression receiver, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics, ImmutableArray<string> argumentNames, ImmutableArray<RefKind> argumentRefKinds, MemberResolutionResult<PropertySymbol> resolutionResult) |
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.
| Binder binder, | ||
| ExtensionScope scope, | ||
| BindingDiagnosticBag diagnostics, | ||
| out BoundExpression? extensionIndexerAccess) |
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.
| } | ||
| } | ||
|
|
||
| private bool TryBindExtensionIndexer(SyntaxNode syntax, BoundExpression left, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics, [NotNullWhen(true)] out BoundExpression? extensionIndexerAccess) |
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.
| singleLookupResults.Free(); | ||
| } | ||
|
|
||
| private void LookupAllNewExtensionMembersInSingleBinder(LookupResult result, string? name, |
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.
| int arity, LookupOptions options, Binder originalBinder, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo) | ||
| { | ||
| var singleLookupResults = ArrayBuilder<SingleLookupResult>.GetInstance(); | ||
| EnumerateAllNewExtensionMembersInSingleBinder(singleLookupResults, name, arity, options, originalBinder, ref useSiteInfo); |
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.
| CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = binder.GetNewCompoundUseSiteInfo(diagnostics); | ||
|
|
||
| scope.Binder.LookupAllNewExtensionMembersInSingleBinder( | ||
| lookupResult, WellKnownMemberNames.Indexer, arity: 0, LookupOptions.AllMethodsOnArityZero, |
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.
| return; | ||
| case BoundIndexerAccess { Indexer.SetMethod: { } indexerSet } indexer when indexerSet.IsExtensionBlockMember(): | ||
| methodInvocationInfo = MethodInvocationInfo.FromIndexerAccess(indexer); | ||
| Debug.Assert(ReferenceEquals(methodInvocationInfo.MethodInfo.Method, indexerSet)); |
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.
| // Tracked by https://github.com/dotnet/roslyn/issues/71056 | ||
| AddPlaceholderReplacement(argumentPlaceholder, integerArgument); | ||
| // https://github.com/dotnet/roslyn/issues/78829 - Do we need to do something special for recievers of extension indexers here? | ||
| Debug.Assert(!indexerAccess.Indexer.IsExtensionBlockMember()); |
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.
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.
I think the comment is useful until the design decision is pending. Consider transforming it into a PROTOTYPE comment, if you simply want to remove a link to the issue.
| return true; | ||
| } | ||
|
|
||
| if (member is PropertySymbol property) |
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.
|
|
||
| internal static bool IsAllowedExtensionMember(Symbol member, LanguageVersion languageVersion) | ||
| { | ||
| if (IsAllowedExtensionMemberInCSharp14(member)) |
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.
| yield return defaultMemberAttribute; | ||
| } | ||
|
|
||
| static SynthesizedAttributeData? synthesizeDefaultMemberAttributeIfNeeded(ImmutableArray<ExtensionMarkerType> extensionMarkerTypes) |
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.
This implementation is valid only under condition that all indexers use the same metadata name. I think that right now this is enforced only by the fact that we disallow usage of "IndexerName" attribute on extension indexers. But we haven't made the final design decision on that. If we start supporting the attribute, then we either must enforce (not assume) the same metadata name for all indexers in the group, or the indexer name must become another part of the grouping name. #Closed
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.
I think that right now this is enforced only by the fact that we disallow usage of "IndexerName" attribute on extension indexers.
It looks like my recollection was wrong. We allow "simple" forms of the attribute. And it looks like the enforcement of the same metadata name for all indexers in a group is already in place. But it would be good to ensure that we have a targeted test for that enforcement.
|
|
||
| comp = CreateCompilation(src, targetFramework: TargetFramework.Net70); | ||
| comp.VerifyEmitDiagnostics( | ||
| // (2,5): error CS0021: Cannot apply indexing with [] to an expression of type 'C' |
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.
| Diagnostic(ErrorCode.ERR_ExtensionDisallowsIndexerMember, "this").WithArguments("preview").WithLocation(12, 20)); | ||
|
|
||
| comp = CreateCompilation(src, targetFramework: TargetFramework.Net70); | ||
| CompileAndVerify(comp, expectedOutput: ExpectedOutput("^1"), verify: Verification.Skipped).VerifyDiagnostics(); |
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.
| Diagnostic(ErrorCode.ERR_ExtensionDisallowsIndexerMember, "this").WithArguments("preview").WithLocation(12, 20)); | ||
|
|
||
| comp = CreateCompilation(src, targetFramework: TargetFramework.Net70); | ||
| CompileAndVerify(comp, expectedOutput: ExpectedOutput("^1"), verify: Verification.Skipped).VerifyDiagnostics(); |
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.
CompileAndVerify(comp, expectedOutput: ExpectedOutput("^1"), verify: Verification.Skipped).VerifyDiagnostics();
Consider also testing scenarios when receiver must be boxed, see #81173
| comp.VerifyDiagnostics(); | ||
|
|
||
| comp = CreateCompilation(src, options: TestOptions.DebugExe); | ||
| CompileAndVerify(comp, expectedOutput: @"12: (o, s) => get_Item(o, s)").VerifyDiagnostics(); |
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.
Consider registering for Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:51905 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
| if (refKind == "ref readonly") | ||
| { | ||
| comp2.VerifyDiagnostics( | ||
| // (15,9): warning CS9193: Argument 0 should be a variable because it is passed to a 'ref readonly' parameter |
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.
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.
It looks like we do
| } | ||
| else | ||
| { | ||
| comp2.VerifyDiagnostics(); |
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.
| { | ||
| if (property.IsExtensionBlockMember()) | ||
| { | ||
| MessageID.IDS_FeatureExtensionIndexers.CheckFeatureAvailability(diagnostics, syntax); |
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.
|
Done with review pass (commit 1) |
Here's the corresponding PR to update the spec: dotnet/csharplang#9869 |
The main changes in this PR:
TryBindExtensionIndexerIDS_FeatureExtensionIndexersand check LangVer (declarations and usages, seeCheckExtensionMembersandReportDiagnosticsIfDisallowedExtensionIndexer)DefaultMemberAttributeon grouping type, which contains the extension indexer(s)Identified some follow-ups around nullability, CREF binding,
GetMemberGroup, "countable" properties, interpolation handlers, improving diagnostic quality, an existing crash with a method namedthis[]in metadata.The usages of extension indexers: element access, null-conditional indexing and assignment, list and spread pattern, and object initializer.
Corresponding spec is
incominghere. Some notable choices (will confirm with LDM):baseRelates to test plan #81505