Skip to content

Comments

Extension indexers: indexing with implicit indexers#82453

Open
jcouv wants to merge 1 commit intodotnet:features/extensionsfrom
jcouv:indexers_08
Open

Extension indexers: indexing with implicit indexers#82453
jcouv wants to merge 1 commit intodotnet:features/extensionsfrom
jcouv:indexers_08

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Feb 19, 2026

The implicit indexers in list-patterns will be addressed in a follow-up.

I'm still working on updating the spec. In short, for each scope (ie. instance, then iterative extension scopes), we look for real indexer and fall back to implicit indexer.
This means that both parts of an implicit indexer must be present in the same scope to count.

Relates to test plan #81505

@jcouv jcouv self-assigned this Feb 19, 2026
@jcouv jcouv force-pushed the indexers_08 branch 2 times, most recently from 9388e37 to 19e7436 Compare February 19, 2026 18:29
@jcouv jcouv changed the base branch from main to features/extensions February 19, 2026 18:46
@dotnet-policy-service dotnet-policy-service bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Feb 19, 2026
@dotnet-policy-service
Copy link
Contributor

This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging.

@jcouv jcouv marked this pull request as ready for review February 20, 2026 00:57
@jcouv jcouv requested a review from a team as a code owner February 20, 2026 00:57
@jcouv jcouv removed Needs API Review Needs to be reviewed by the API review council VSCode labels Feb 20, 2026
Comment on lines +9114 to +9120
if (arguments.Arguments.Count != 1)
{
return IndexOrRangeArgKind.None;
}

TypeSymbol? argType = arguments.Arguments[0].Type;
if (argType is null)
Copy link
Member

@jjonescz jjonescz Feb 20, 2026

Choose a reason for hiding this comment

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

nit: could be simplified to

Suggested change
if (arguments.Arguments.Count != 1)
{
return IndexOrRangeArgKind.None;
}
TypeSymbol? argType = arguments.Arguments[0].Type;
if (argType is null)
if (arguments.Arguments is not [{ Type: TypeSymbol argType }])
{
return IndexOrRangeArgKind.None;
}
``` #Pending

@@ -702,6 +701,41 @@ public int this[int i]

[Fact]
public void Indexing_09()
Copy link
Member

Choose a reason for hiding this comment

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

Renumbering lots of tests seems unfortunate; next time consider some suffix like Indexing_09_02 to "insert" a new test.

return IndexOrRangeArgKind.None;
}

if (TypeSymbol.Equals(argType, Compilation.GetWellKnownType(WellKnownType.System_Index), TypeCompareKind.ConsiderEverything))
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

TypeCompareKind.ConsiderEverything

It looks like we are all over the place when it comes to TypeCompareKind used when we are matching Index type. Consider extracting a helper and unifying all usages on TypeCompareKind.AllIgnoreOptions. #Pending

return IndexOrRangeArgKind.Index;
}

if (TypeSymbol.Equals(argType, Compilation.GetWellKnownType(WellKnownType.System_Range), TypeCompareKind.ConsiderEverything))
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

TypeCompareKind.ConsiderEverything

Similar suggestion #Pending


lookupResult.Free();
actualArguments?.Free();
if (argKind != IndexOrRangeArgKind.None
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

argKind != IndexOrRangeArgKind.None

Consider checking receiverPlaceholder for not null instead. Then we wouldn't need a suppression below. #Pending

LookupResultKind.Viable, hasErrors: false).MakeCompilerGenerated();

lengthOrCountAccess = binder.CheckValue(lengthOrCountAccess, BindValueKind.RValue, diagnostics);

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to extract a common helper to share with TryBindLengthOrCount?

Copy link
Contributor

Choose a reason for hiding this comment

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

Reusing GetExtensionMemberAccess or its relevant portion would be fine too.


candidates.Free();
diagnostics.Add(syntax, useSiteInfo);
return lengthOrCountProperty is not null;
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

lengthOrCountProperty

It looks like we are grabbing the first found property and ignoring any ambiguity across same name properties. Also, it looks like we are not checking applicability to the receiver type. It feels like we need to collect a group. run an overload resolution process to get a best match, then create a BoundPropertyAccess (hopefully reusing GetExtensionMemberAccess or its relevant portion). #Pending

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, a true ambiguity should probably prevent us from going to the next scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope we will be able to share/reuse code that deals with binding extension properties explicitly used in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. I missed the generic cases. Noticed the problem while working on next PR. Unlike instance scenario, we need to do overload resolution, that will give us type inference and deal with ambiguities

candidates, WellKnownMemberNames.Indexer, alternativeName: null,
arity, lookupOptions, originalBinder: binder);

var intPlaceholder = new BoundImplicitIndexerValuePlaceholder(syntax, binder.Compilation.GetSpecialType(SpecialType.System_Int32)) { WasCompilerGenerated = true };
Copy link
Contributor

Choose a reason for hiding this comment

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

binder.Compilation.GetSpecialType(SpecialType.System_Int32)

Why is it safe to ignore any use-site errors here?

analyzedArguments.Free();
candidates.Free();
diagnostics.Add(syntax, useSiteInfo);
return indexerOrSliceAccess is not null;
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

indexerOrSliceAccess

It looks like we have the same issue with ambiguity handling here too.

var properties = ArrayBuilder<PropertySymbol>.GetInstance();
properties.Add(property);

OverloadResolutionResult<PropertySymbol> overloadResolutionResult = resolveIndexers(receiver, properties, binder, analyzedArguments, ref actualArguments, ref useSiteInfo);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

OverloadResolutionResult overloadResolutionResult = resolveIndexers(receiver, properties, binder, analyzedArguments, ref actualArguments, ref useSiteInfo);

It feels like we should be able to share/reuse tryBindExtensionIndexersInScope or at least significant portion of it, rather than duplicating the logic here and below. #Pending


candidates.Free();
diagnostics.Add(syntax, useSiteInfo);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

return true;

It looks like we have the same issue with ambiguity handling here too.

{
if (TryBindExtensionIndexer(syntax, receiver, analyzedArguments, diagnostics, out BoundIndexerAccess extensionIndexerAccess))
if (TryBindNonExtensionImplicitIndexer(syntax, receiver, analyzedArguments, diagnostics, out var fallbackIndexerAccess)
|| TryBindExtensionIndexer(syntax, receiver, analyzedArguments, diagnostics, out fallbackIndexerAccess))
Copy link
Contributor

Choose a reason for hiding this comment

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

|| TryBindExtensionIndexer(syntax, receiver, analyzedArguments, diagnostics, out fallbackIndexerAccess)

This doesn't feel right. It looks like the enclosing method is called from BindNonExtensionIndexerOrIndexedPropertyAccess. Therefore, I would not expect it to touch extensions at all.

}
}

internal static bool MethodHasValidSliceSignature(MethodSymbol method)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

MethodHasValidSliceSignature

Please move this method to its original place so that the diff is clear. #Pending

/// Checks whether a property has a valid getter for the Length/Count implicit indexer pattern
/// (non-static, returns int, non-ref).
/// </summary>
private static bool IsValidLengthOrCountGetter(PropertySymbol property)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

IsValidLengthOrCountGetter

HasValidLengthOrCountGetter? #Pending

/// Checks whether a property is a valid implicit Index indexer
/// (single int parameter, non-ref, non-static).
/// </summary>
private static bool IsValidImplicitIndexIndexer(PropertySymbol property)
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 20, 2026

Choose a reason for hiding this comment

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

IsValidImplicitIndexIndexer

Unlike the previous helper, this one doesn't check if there is a getter. Should it? #Pending

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

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.

3 participants