Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Dec 9, 2025

The main changes in this PR:

  • addition of TryBindExtensionIndexer
  • add new feature IDS_FeatureExtensionIndexers and check LangVer (declarations and usages, see CheckExtensionMembers and ReportDiagnosticsIfDisallowedExtensionIndexer)
  • round-tripping of indexers in metadata: emit DefaultMemberAttribute on 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 named this[] in metadata.

The usages of extension indexers: element access, null-conditional indexing and assignment, list and spread pattern, and object initializer.

Corresponding spec is incoming here. Some notable choices (will confirm with LDM):

  • extension indexers come before implicit indexers
  • no extension indexers on base

Relates to test plan #81505

@jcouv jcouv self-assigned this Dec 9, 2025
@jcouv jcouv changed the title Extension indexers: resolve extension indexers Extension indexers: allow and resolve extension indexers Dec 9, 2025
@jcouv jcouv force-pushed the extension-indexers branch 3 times, most recently from 37bad6f to 344fa46 Compare December 9, 2025 22:59
@jcouv jcouv force-pushed the extension-indexers branch from 344fa46 to f504f6e Compare December 9, 2025 23:28
@jcouv jcouv marked this pull request as ready for review December 10, 2025 02:19
@jcouv jcouv requested a review from a team as a code owner December 10, 2025 02:19
@AlekseyTs
Copy link
Contributor

It looks like the speclet says nothing about consumption scenarios

@jcouv
Copy link
Member Author

jcouv commented Dec 10, 2025

It looks like the speclet says nothing about consumption scenarios

Yes, the speclet was not yet updated.

Corresponding spec is incoming. Some notable choices (will confirm with LDM):

  • extension indexers come before implicit indexers
  • no extension indexers on base

if (member is PropertySymbol)
{
diagnostics.Add(new CSDiagnostic(
new CSDiagnosticInfo(ErrorCode.ERR_ExtensionDisallowsIndexerMember, new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureExtensionIndexers.RequiredVersion())),
Copy link
Contributor

Choose a reason for hiding this comment

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

new CSDiagnosticInfo(ErrorCode.ERR_ExtensionDisallowsIndexerMember, new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureExtensionIndexers.RequiredVersion())),

I think we can use regular language version error here

if (!IsAllowedExtensionMember(member, languageVersion))
{
diagnostics.Add(ErrorCode.ERR_ExtensionDisallowsMember, member.GetFirstLocation());
if (member is PropertySymbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

member is PropertySymbol

It looks like we are assuming that this is an indexer. Consider asserting the fact, or including this as a condition.

<data name="IDS_FeatureExtensionIndexers" xml:space="preserve">
<value>extension indexers</value>
</data>
<data name="ERR_ExtensionDisallowsIndexerMember" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

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

ERR_ExtensionDisallowsIndexerMember

It doesn't look like it is necessary to have this error, a regular language version error for the feature would work as good.


internal static void ReportDiagnosticsIfDisallowedExtensionIndexer(BindingDiagnosticBag diagnostics, PropertySymbol property, SyntaxNode syntax)
{
if (property.IsExtensionBlockMember())
Copy link
Contributor

Choose a reason for hiding this comment

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

property

How do we know that this is an indexer?

var setMethod = replace(SetMethod);
Symbol symbol = ReferenceEquals(Symbol, Method) && method is not null ? method : Symbol;

Debug.Assert(SetMethod?.IsExtensionBlockMember() != true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug.Assert(SetMethod?.IsExtensionBlockMember() != true);

It is not obvious why this condition was correct for properties and isn't for indexers. Is it still worth asserting for properties?

lookupResult, WellKnownMemberNames.Indexer, arity: 0, LookupOptions.AllMethodsOnArityZero,
originalBinder: binder, useSiteInfo: ref useSiteInfo);

if (!lookupResult.IsMultiViable || lookupResult.Symbols.All(s => s.Kind != SymbolKind.Property))
Copy link
Contributor

Choose a reason for hiding this comment

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

s.Kind != SymbolKind.Property

Should we also check that it is an indexer?


MemberResolutionResult<PropertySymbol> resolutionResult = result.ValidResult;
Debug.Assert(resolutionResult.Result.ConversionForArg(0).Exists);
resolutionResult = resolutionResult.WithResult(resolutionResult.Result.WithoutReceiverArgument());
Copy link
Contributor

Choose a reason for hiding this comment

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

resolutionResult = resolutionResult.WithResult(resolutionResult.Result.WithoutReceiverArgument());

Since we are dropping receiver argument, should argumentNames and argumentRefKinds be adjusted as well?

ArrayBuilder<PropertySymbol>? properties = null;
foreach (var member in lookupResult.Symbols)
{
if (member is PropertySymbol property)
Copy link
Contributor

Choose a reason for hiding this comment

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

member is PropertySymbol property

Should we check that it is an indexer?


overloadResolutionResult.Free();
return propertyAccess;
bool isNewExtensionMember = property.IsExtensionBlockMember();
Copy link
Contributor

Choose a reason for hiding this comment

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

isNewExtensionMember

Consider avoiding using "neExtension" term. All features are new only until they are shipped. Consider using terms that "survive" long term


// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

BoundExpression

BoundIndexerAccess?

Binder binder,
ExtensionScope scope,
BindingDiagnosticBag diagnostics,
out BoundExpression? extensionIndexerAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

BoundExpression

BoundIndexerAccess?

}
}

private bool TryBindExtensionIndexer(SyntaxNode syntax, BoundExpression left, AnalyzedArguments analyzedArguments, BindingDiagnosticBag diagnostics, [NotNullWhen(true)] out BoundExpression? extensionIndexerAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

BoundExpression

BoundIndexerAccess?

singleLookupResults.Free();
}

private void LookupAllNewExtensionMembersInSingleBinder(LookupResult result, string? name,
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2025

Choose a reason for hiding this comment

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

NewExtensionMembers

Similar comment for "NewExtensionMembers" term. It looks like this is applicable to other new APIs.

int arity, LookupOptions options, Binder originalBinder, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
{
var singleLookupResults = ArrayBuilder<SingleLookupResult>.GetInstance();
EnumerateAllNewExtensionMembersInSingleBinder(singleLookupResults, name, arity, options, originalBinder, ref useSiteInfo);
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2025

Choose a reason for hiding this comment

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

EnumerateAllNewExtensionMembersInSingleBinder

Consider asserting that result is null. Or conditionally Free it.

CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = binder.GetNewCompoundUseSiteInfo(diagnostics);

scope.Binder.LookupAllNewExtensionMembersInSingleBinder(
lookupResult, WellKnownMemberNames.Indexer, arity: 0, LookupOptions.AllMethodsOnArityZero,
Copy link
Contributor

Choose a reason for hiding this comment

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

LookupOptions.AllMethodsOnArityZero

Do we actually expect to get back any methods?

return;
case BoundIndexerAccess { Indexer.SetMethod: { } indexerSet } indexer when indexerSet.IsExtensionBlockMember():
methodInvocationInfo = MethodInvocationInfo.FromIndexerAccess(indexer);
Debug.Assert(ReferenceEquals(methodInvocationInfo.MethodInfo.Method, indexerSet));
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug.Assert(ReferenceEquals(methodInvocationInfo.MethodInfo.Method, indexerSet));

What method do we get now?

// 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());
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug.Assert(!indexerAccess.Indexer.IsExtensionBlockMember());

This looks suspicious. Are we saying that extension indexers will not participate in implicit indexing pattern?

Copy link
Contributor

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

member is PropertySymbol property

Consider either asserting, or, even better, explicitly checking that this is an indexer.


internal static bool IsAllowedExtensionMember(Symbol member, LanguageVersion languageVersion)
{
if (IsAllowedExtensionMemberInCSharp14(member))
Copy link
Contributor

Choose a reason for hiding this comment

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

IsAllowedExtensionMemberInCSharp14

It looks like this is the only use site for this method. Consider merging two methods, or, in other words, modifying the original method instead of splitting it.

yield return defaultMemberAttribute;
}

static SynthesizedAttributeData? synthesizeDefaultMemberAttributeIfNeeded(ImmutableArray<ExtensionMarkerType> extensionMarkerTypes)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2025

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

Copy link
Contributor

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'
Copy link
Contributor

Choose a reason for hiding this comment

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

// (2,5): error CS0021: Cannot apply indexing with [] to an expression of type 'C'

I assume a design decision about this behavior is still pending. Consider adding a PROTOTYPE comment

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();
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2025

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();

This looks strange. Based on the previous test, int-based extension indexer is not used by pattern matching, but Index-based is used. Consider leaving a PROTOTYPE comment.

@AlekseyTs
Copy link
Contributor

    comp = CreateCompilation(src, targetFramework: TargetFramework.Net70);

Is there still a value in creating this compilation?


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:27521 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

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();
Copy link
Contributor

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();
Copy link
Contributor

Choose a reason for hiding this comment

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

CompileAndVerify(comp, expectedOutput: @"12: (o, s) => get_Item(o, s)").VerifyDiagnostics();

This doesn't look right. We decided that we don't support extension properties (see the following test), the same reason is applicable to extension indexers, I think.

@AlekseyTs
Copy link
Contributor

    s = e.get_Item(x, 0);

Is this a typo? I assume we expect to get similar ref safety errors on this and the previous line


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:45874 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    s[1.0] = x;

I assume we should get the same ref safety error on this line as for the next line, but it looks like we don't get it.


Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs:45876 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

        context.RegisterSyntaxNodeAction(handle, SyntaxKind.PropertyDeclaration);

Consider registering for IndexerDeclarationSyntax as well. Otherwise, we are not asserting events for indexers.


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
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 11, 2025

Choose a reason for hiding this comment

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

// (15,9): warning CS9193: Argument 0 should be a variable because it is passed to a 'ref readonly' parameter

Do we report this warning for an extension property? #Closed

Copy link
Contributor

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();
Copy link
Contributor

Choose a reason for hiding this comment

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

comp2.VerifyDiagnostics();

I think we should assert IL for the two cases that do not report an error.

@AlekseyTs
Copy link
Contributor

public void Declaration_06()

Dupe of the previous test?


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:184 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

public void Declaration_11()

Dupe of Declaration_01?


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:295 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    // Color Color receiver

I do not think there is such a thing with an index expression. Not to mention the fact that indexers cannot be static.


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:738 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

class C { }

Consider also testing with a struct type (receiver must be boxed).


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:890 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    // inaccessible

Consider actually adding a test with accessibility


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:1063 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

        // (1,24): error CS0131: The left-hand side of an assignment must be a variable, property or indexer

Does this look like a bug?


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:2441 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

public void ConditionalAssignment_01()

Consider covering nullable value type scenario


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:2533 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

public void ConditionalAccess_01()

Consider covering nullable value type scenario


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:2576 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

{
if (property.IsExtensionBlockMember())
{
MessageID.IDS_FeatureExtensionIndexers.CheckFeatureAvailability(diagnostics, syntax);
Copy link
Contributor

Choose a reason for hiding this comment

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

MessageID.IDS_FeatureExtensionIndexers.CheckFeatureAvailability(diagnostics, syntax);

Consider skipping the check if the indexer is declared in the same module. We already complained on the declaration

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 11, 2025

public void Metadata_01()

Consider also testing with IndexerName attribute #Closed


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:2736 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 11, 2025

public void Metadata_02(bool useCompilationReference)

Consider also testing with IndexerName attribute #Closed


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:2834 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

_ = d[0];

We should test scenario with 'dynamic' argument and non-dynamic receiver


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4903 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

    // Irregular/legacy behavior for indexer arguments

I do not see any irregularity for indexer arguments. Could you elaborate?


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:4926 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

public void RefAnalysis_Indexing_01()

Were RefAnalysis tests cloned from some existing regular indexers tests?


Refers to: src/Compilers/CSharp/Test/CSharp15/ExtensionIndexersTests.cs:5019 in f504f6e. [](commit_id = f504f6e, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@jcouv
Copy link
Member Author

jcouv commented Dec 11, 2025

It looks like the speclet says nothing about consumption scenarios

Here's the corresponding PR to update the spec: dotnet/csharplang#9869

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants