Skip to content

Support implicit indexer access in object initializers#70649

Merged
jcouv merged 19 commits intodotnet:mainfrom
jcouv:index-initializer
Dec 7, 2023
Merged

Support implicit indexer access in object initializers#70649
jcouv merged 19 commits intodotnet:mainfrom
jcouv:index-initializer

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Nov 1, 2023 •
edited
Loading

Fixes #67533

Since it's been a while since the Ranges feature was released, I put the fix behind a LangVer/feature check.

@jcouv jcouv self-assigned this Nov 1, 2023
@ghost ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 1, 2023
@jcouv jcouv force-pushed the index-initializer branch 6 times, most recently from a3c76d7 to a716dee Compare November 1, 2023 22:39
@jcouv jcouv force-pushed the index-initializer branch from a716dee to acfbe40 Compare November 2, 2023 00:33
@jcouv jcouv marked this pull request as ready for review November 2, 2023 06:59
@jcouv jcouv requested a review from a team as a code owner November 2, 2023 06:59
AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Portable/CSharpResources.resx Outdated
<value>The diagnosticId argument to the 'Experimental' attribute must be a valid identifier</value>
</data>
<data name="IDS_ImplicitIndexInitializer" xml:space="preserve">
<value>implicit Index initializer</value>
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Index

Indexer? #Closed

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated
MessageID.IDS_ImplicitIndexInitializer.CheckFeatureAvailability(diagnostics, implicitIndexer.Syntax);

hasErrors |= isRhsNestedInitializer
&& !CheckNestedObjectInitializerPropertySymbol(GetPropertySymbol(implicitIndexer, out _, out _), leftSyntax, diagnostics, hasErrors, ref resultKind);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

GetPropertySymbol(implicitIndexer, out _, out _)

Are we going to get here for an array or a slice case? It looks like this helper is going to return null then. #Closed

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
hasErrors |= isRhsNestedInitializer
&& !CheckNestedObjectInitializerPropertySymbol(GetPropertySymbol(implicitIndexer, out _, out _), leftSyntax, diagnostics, hasErrors, ref resultKind);

return hasErrors ? boundMember : CheckValue(boundMember, valueKind, diagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

CheckValue(boundMember, valueKind, diagnostics)

It is not obvious why this is necessary and what effect is this going to have. For example, it looks like we are not doing this for a regular indexer. #Closed

Copy link
Member Author

@jcouv jcouv Nov 3, 2023

Choose a reason for hiding this comment

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

The regular indexer case (BoundKind.IndexerAccess) does this after the switch (line 5429 below).
The effect is shown in tests like InObjectInitializer_Readonly and InObjectInitializer_Readonly.

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
...arp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs Outdated
if (assignment.Left.Kind != BoundKind.PointerElementAccess)
// Do not lower pointer or implicit indexer access yet, we'll do them later.
BoundKind leftKind = assignment.Left.Kind;
if (leftKind != BoundKind.PointerElementAccess && leftKind != BoundKind.ImplicitIndexerAccess)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

&& leftKind != BoundKind.ImplicitIndexerAccess

It is not obvious why this is necessary. Why regular way of doing things works for regular indexer and doesn't work for an implicit indexer? It is quite possible that I would be more comfortable with adjusting existing code paths instead of introducing completely new case below. #Closed

Copy link
Member Author

@jcouv jcouv Nov 3, 2023

Choose a reason for hiding this comment

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

The regular indexer case is handled inside VisitObjectInitializerMember which takes a BoundObjectInitializerMember. So it doesn't seem very suitable for sharing.

I could convert the ternary expression below into a switch and essentially do rewrittenLeft = VisitImplicitIndexerAccess((BoundImplicitIndexerAccess)assignment.Left, ...); here (instead of rewrittenAccess = VisitImplicitIndexerAccess(...) below) if that feels more comfortable, but I don't think we'll be able to share.

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs Outdated
}
}
""";
var comp = CreateCompilationWithIndex(source, parseOptions: TestOptions.RegularNext);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

RegularNext

Are we testing against the latest version? #Closed

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs Outdated
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/67533")]
public void InObjectInitializer_Nested()
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

InObjectInitializer_Nested

Are we testing similar scenarios with an array? #Closed

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs Outdated
""";
var comp = CreateCompilationWithIndexAndRange(source);
comp.VerifyDiagnostics(
// (2,1): error CS0131: The left-hand side of an assignment must be a variable, property or indexer
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

I understand that this is a pre-existing condition, but the error feels confusing. The language says it is an indexer, but the error says it is not. #Closed

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs Outdated
Buffer10 b = default;
b[..][0] = 1;

_ = new Buffer10() { [..][0] = 2 }; // 1
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

[..][0]

Are we testing similar scenario with index indexer (including arrays)? #Closed

AlekseyTs
AlekseyTs reviewed Nov 3, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs Outdated
Buffer10 b = default;
b[..] = null; // 1

_ = new Buffer10() { [..] = null }; // 2
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

null

Nested initializer should probably work with the slice scenario. Are we testing it? #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 3, 2023

Done with review pass (commit 1).

@jcouv jcouv marked this pull request as draft November 5, 2023 16:14
@jcouv jcouv force-pushed the index-initializer branch from d1a0cbb to a9a2e26 Compare November 21, 2023 20:57
@jcouv jcouv force-pushed the index-initializer branch from a9a2e26 to e4c79c2 Compare November 21, 2023 22:56
cston
cston reviewed Dec 4, 2023
View reviewed changes
src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs
void addIndexes(IMemberInitializerOperation memberInitializer)
{
var lhs = memberInitializer.InitializedMember;
// If we have an element access of the form `[arguments] = { ... }`, we'll evaluate `arguments` only
Copy link
Contributor

@cston cston Dec 4, 2023 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

[arguments] = { ... }

Should this be [arguments] = { } instead, with no initializers? #Resolved

Copy link
Member Author

@jcouv jcouv Dec 5, 2023

Choose a reason for hiding this comment

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

No, there's possibly some nested initializers in there. As long as the leaves are empty initializers, the optimization kicks in and we only need to evaluate the indexer arguments.

@runfoapp runfoapp bot mentioned this pull request Dec 4, 2023
Closed
cston
cston reviewed Dec 4, 2023
View reviewed changes
...arp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectOrCollectionInitializerExpression.cs Outdated
cacheAllArgumentsOnly: true,
result, temps);

if (rewrittenAccess is BoundIndexerAccess indexerAccess1)
Copy link
Contributor

@cston cston Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

indexerAccess1

Minor: Consider dropping the 1 suffix. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 4, 2023 •
edited
Loading 

    }

Is there a test asserting IOperation shape for indexing into an array with a System.Range? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:4220 in cb65f3f. [](commit_id = cb65f3f, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 4, 2023

Done with review pass (commit 17)

@jcouv
Copy link
Member Author

jcouv commented Dec 5, 2023 

    }

We only had CFG validation. Expanded the relevant tests

In reply to: 1839590554

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:4220 in cb65f3f. [](commit_id = cb65f3f, deletion_comment = False)

@jcouv jcouv requested review from AlekseyTs and cston December 5, 2023 15:57
@cston
Copy link
Contributor

cston commented Dec 5, 2023 •
edited by jcouv
Loading 

        var comp = CreateCompilationWithIndex(source, parseOptions: useCsharp13 ? TestOptions.RegularNext : TestOptions.RegularPreview);

Will this be changed to TestOptions.Regular13 in the future?

And currently, it looks like TestOptions.RegularNext == TestOptions.RegularPreview currently. Is that expected here? #Resolved

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:57 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@cston
Copy link
Contributor

cston commented Dec 5, 2023 •
edited by jcouv
Loading 

    public void InObjectInitializer_Index_RefReturningIndexer()

Are we testing this case with an empty initializer (with ref object this[int] { get; } perhaps)? #Pending

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:1199 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@cston
Copy link
Contributor

cston commented Dec 5, 2023 

    public void InObjectInitializer_Index_RefReturningIndexer()

Are we testing an indexer that returns ref readonly object with an initializer, and with an empty initializer?

In reply to: 1841587538

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:1199 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Dec 5, 2023 

        var comp = CreateCompilationWithIndex(source, parseOptions: useCsharp13 ? TestOptions.RegularNext : TestOptions.RegularPreview);

Yes, RegularNext will mechanically replaced with Regular13 next August before shipping C# 13. At the moment, Next and Preview are the same, but they will differ after C# 13 ships.

In reply to: 1841558134

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:57 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@cston
Copy link
Contributor

cston commented Dec 5, 2023 •
edited by jcouv
Loading

C.M(1..^1, 2..^2);

System.Console.Write($"Results={c.F._array[1]._array[4]},{c.F._array[1]._array[5]}"); #Pending

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:2319 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@cston
Copy link
Contributor

cston commented Dec 5, 2023 •
edited by jcouv
Loading 

public int[] Slice(int start, int length) => throw null;

Should this be an indexer instead? #Pending

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:3938 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Dec 6, 2023 

    public void InObjectInitializer_Index_RefReturningIndexer()

Added some tests. Was that was you had in mind?

In reply to: 1841590554

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/IndexAndRangeTests.cs:1199 in 87eb9a1. [](commit_id = 87eb9a1, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Dec 6, 2023

@AlekseyTs I believe all feedback was addressed. Thanks

AlekseyTs
AlekseyTs approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM(commit 19), assuming CI is passing

@jcouv
Copy link
Member Author

jcouv commented Dec 7, 2023

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 7, 2023

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv jcouv merged commit 359a6aa into dotnet:main Dec 7, 2023
@jcouv jcouv deleted the index-initializer branch December 7, 2023 08:21
@ghost ghost added this to the Next milestone Dec 7, 2023
@allisonchou allisonchou mentioned this pull request Dec 8, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Range Range untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

17.9 P3

Development

Successfully merging this pull request may close these issues.

Implicit index indexer doesn't work in object initializer

4 participants

@jcouv @AlekseyTs @cston @Cosifne