-
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 #Closed |
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.
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 don't think so. We're getting argumentNames and argumentRefKinds from analyzedArguments (not from actualArguments or some other source).
| 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.
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.
We get an identical but not reference-equal method. SubstitutedPropertySymbol.GetMethod and .SetMethod return new symbols every time they are called.
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.
Let's assert the equality fact then?
| // 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.
| 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 #Pending
| 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.
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.
Good catch. Thanks
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 #Resolved |
I think that's In reply to: 3640424908 Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4903 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
For indexers, we skip the safety check for usage of pointer type. Extension indexers carry over the same behavior as non-extension indexers. In reply to: 3640433536 Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4926 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
I used existing and modified ref analysis tests for extension methods In reply to: 3640438499 Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:5019 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
This looks like something that shouldn't work. Dynamic argument assumes dynamic evaluation and runtime binder has no support for extensions #Pending Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4874 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
Indeed. Thanks, Added a comment for it In reply to: 3644345290 Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4903 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
Thanks. Will PROTOTYPE for follow-up In reply to: 3644360564 Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4874 in f504f6e. [](commit_id = f504f6e, deletion_comment = False) |
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