Skip to content

Support implicit indexer access in object initializers #70649

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.

Sign up for GitHub

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

Merged
jcouv merged 19 commits into from
Dec 7, 2023
Merged

Support implicit indexer access in object initializers #70649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
acfbe40
Support implicit indexer access in object initializers
Oct 31, 2023
73ec15e
Address feedback
Nov 3, 2023
e4c79c2
Merge remote-tracking branch 'dotnet/main' into index-initializer
Nov 21, 2023
d7b7a7a
Add test
Nov 22, 2023
b3d83e9
Optimize empty nested initializer case
Nov 22, 2023
0be63ec
Fix IOperation and optimize more cases
Nov 27, 2023
a05a62a
tweaks
Nov 29, 2023
403a895
Address feedback
Nov 30, 2023
1434496
Fix Range in array case
Nov 30, 2023
ac4f099
Tweaks
Nov 30, 2023
ae28ef3
cleanup
Nov 30, 2023
b4e5f55
Remove unused usings
Nov 30, 2023
f133848
Address feedback
Dec 1, 2023
dc67a10
Add assertion
Dec 1, 2023
6ca0f90
Use guard instead of assertion
Dec 1, 2023
5ab3948
Fix IOperation for event case
Dec 1, 2023
cb65f3f
Unused using
Dec 4, 2023
87eb9a1
Verify IOperation on array/Range
Dec 5, 2023
80895f9
Update tests
Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/Language Feature Status.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ efforts behind them.
| [Roles/Extensions](https://github.com/dotnet/csharplang/issues/5497) | [roles](https://github.com/dotnet/roslyn/tree/features/roles) | [In Progress](https://github.com/dotnet/roslyn/issues/66722) | [jcouv](https://github.com/jcouv) | [AlekseyTs](https://github.com/AlekseyTs), [jjonescz](https://github.com/jjonescz) | | [MadsTorgersen](https://github.com/MadsTorgersen) |
| [Escape character](https://github.com/dotnet/csharplang/issues/7400) | N/A | [In Progress](https://github.com/dotnet/roslyn/pull/70497) | [CyrusNajmabadi](https://github.com/CyrusNajmabadi) | [jcouv](https://github.com/jcouv), [RikkiGibson](https://github.com/RikkiGibson) | | [CyrusNajmabadi](https://github.com/CyrusNajmabadi) |
| [Method group natural type improvements](https://github.com/dotnet/csharplang/blob/main/proposals/method-group-natural-type-improvements.md) | main | In Progress | [jcouv](https://github.com/jcouv) | [AlekseyTs](https://github.com/AlekseyTs), [cston](https://github.com/cston) | | [jcouv](https://github.com/jcouv) |
| Implicit indexer access in object initializers | main | Merged into 17.9p3 | [jcouv](https://github.com/jcouv) | | | |

# C# 12.0

Expand Down
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5400,6 +5400,17 @@ private BoundExpression BindObjectInitializerMember(
break;
}

case BoundKind.ImplicitIndexerAccess:
var implicitIndexer = (BoundImplicitIndexerAccess)boundMember;
MessageID.IDS_ImplicitIndexerInitializer.CheckFeatureAvailability(diagnostics, implicitIndexer.Syntax);

if (isRhsNestedInitializer && GetPropertySymbol(implicitIndexer, out _, out _) is { } property)
{
hasErrors |= !CheckNestedObjectInitializerPropertySymbol(property, leftSyntax, diagnostics, hasErrors, ref resultKind);
}

return hasErrors ? boundMember : CheckValue(boundMember, valueKind, diagnostics);
Copy link
Copy Markdown
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
Copy Markdown
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.


case BoundKind.DynamicObjectInitializerMember:
break;

Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7830,4 +7830,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_InvalidExperimentalDiagID" xml:space="preserve">
<value>The diagnosticId argument to the 'Experimental' attribute must be a valid identifier</value>
</data>
<data name="IDS_ImplicitIndexerInitializer" xml:space="preserve">
<value>implicit indexer initializer</value>
</data>
</root>
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@ internal enum MessageID
IDS_FeatureCollectionExpressions = MessageBase + 12837,
IDS_FeatureRefReadonlyParameters = MessageBase + 12838,
IDS_StringEscapeCharacter = MessageBase + 12839,

IDS_ImplicitIndexerInitializer = MessageBase + 12840,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -458,6 +460,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)

// C# preview features.
case MessageID.IDS_StringEscapeCharacter:
case MessageID.IDS_ImplicitIndexerInitializer:
return LanguageVersion.Preview;

// C# 12.0 features.
Expand Down
1 change: 1 addition & 0 deletions ...pilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ private BoundExpression TransformIndexPatternIndexerAccess(BoundImplicitIndexerA
implicitIndexerAccess,
isLeftOfAssignment: true,
isRegularAssignmentOrRegularCompoundAssignment: isRegularCompoundAssignment,
cacheAllArgumentsOnly: false,
stores, temps);

if (access is BoundIndexerAccess indexerAccess)
Expand Down
121 changes: 83 additions & 38 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,11 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.CodeGen;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
Expand Down Expand Up @@ -429,6 +425,7 @@ private BoundExpression VisitIndexPatternIndexerAccess(BoundImplicitIndexerAcces
BoundExpression rewrittenIndexerAccess = GetUnderlyingIndexerOrSliceAccess(
node, isLeftOfAssignment,
isRegularAssignmentOrRegularCompoundAssignment: isLeftOfAssignment,
cacheAllArgumentsOnly: false,
sideeffects, locals);

return _factory.Sequence(
Expand All @@ -441,6 +438,7 @@ private BoundExpression GetUnderlyingIndexerOrSliceAccess(
BoundImplicitIndexerAccess node,
bool isLeftOfAssignment,
bool isRegularAssignmentOrRegularCompoundAssignment,
bool cacheAllArgumentsOnly,
ArrayBuilder<BoundExpression> sideeffects,
ArrayBuilder<LocalSymbol> locals)
{
Expand All @@ -457,37 +455,41 @@ private BoundExpression GetUnderlyingIndexerOrSliceAccess(

var receiver = VisitExpression(node.Receiver);

// Do not capture receiver if it is a local or parameter and we are evaluating a pattern
// If length access is a local, then we are evaluating a pattern
if (node.LengthOrCountAccess.Kind is not BoundKind.Local || receiver.Kind is not (BoundKind.Local or BoundKind.Parameter))
// Do not capture receiver if we're in an initializer
if (!cacheAllArgumentsOnly)
{
Debug.Assert(receiver.Type is { });

var receiverLocal = F.StoreToTemp(
receiver,
out var receiverStore,
// Store the receiver as a ref local if it's a value type to ensure side effects are propagated
receiver.Type.IsReferenceType ? RefKind.None : RefKind.Ref);
locals.Add(receiverLocal.LocalSymbol);

if (receiverLocal.LocalSymbol.IsRef &&
CodeGenerator.IsPossibleReferenceTypeReceiverOfConstrainedCall(receiverLocal) &&
!CodeGenerator.ReceiverIsKnownToReferToTempIfReferenceType(receiverLocal) &&
((isLeftOfAssignment && !isRegularAssignmentOrRegularCompoundAssignment) ||
!CodeGenerator.IsSafeToDereferenceReceiverRefAfterEvaluatingArguments(ImmutableArray.Create(makeOffsetInput))))
// Do not capture receiver if it is a local or parameter and we are evaluating a pattern
// If length access is a local, then we are evaluating a pattern
if (node.LengthOrCountAccess.Kind is not BoundKind.Local || receiver.Kind is not (BoundKind.Local or BoundKind.Parameter))
{
BoundAssignmentOperator? extraRefInitialization;
ReferToTempIfReferenceTypeReceiver(receiverLocal, ref receiverStore, out extraRefInitialization, locals);

if (extraRefInitialization is object)
Debug.Assert(receiver.Type is { });

var receiverLocal = F.StoreToTemp(
receiver,
out var receiverStore,
// Store the receiver as a ref local if it's a value type to ensure side effects are propagated
receiver.Type.IsReferenceType ? RefKind.None : RefKind.Ref);
locals.Add(receiverLocal.LocalSymbol);

if (receiverLocal.LocalSymbol.IsRef &&
CodeGenerator.IsPossibleReferenceTypeReceiverOfConstrainedCall(receiverLocal) &&
!CodeGenerator.ReceiverIsKnownToReferToTempIfReferenceType(receiverLocal) &&
((isLeftOfAssignment && !isRegularAssignmentOrRegularCompoundAssignment) ||
!CodeGenerator.IsSafeToDereferenceReceiverRefAfterEvaluatingArguments(ImmutableArray.Create(makeOffsetInput))))
{
sideeffects.Add(extraRefInitialization);
BoundAssignmentOperator? extraRefInitialization;
ReferToTempIfReferenceTypeReceiver(receiverLocal, ref receiverStore, out extraRefInitialization, locals);

if (extraRefInitialization is object)
{
sideeffects.Add(extraRefInitialization);
}
}
}

sideeffects.Add(receiverStore);
sideeffects.Add(receiverStore);

receiver = receiverLocal;
receiver = receiverLocal;
}
}

AddPlaceholderReplacement(node.ReceiverPlaceholder, receiver);
Expand Down Expand Up @@ -524,15 +526,19 @@ private BoundExpression GetUnderlyingIndexerOrSliceAccess(

Debug.Assert(node.ArgumentPlaceholders.Length == 1);
var argumentPlaceholder = node.ArgumentPlaceholders[0];
AddPlaceholderReplacement(argumentPlaceholder, integerArgument);
Debug.Assert(integerArgument.Type!.SpecialType == SpecialType.System_Int32);

BoundExpression rewrittenIndexerAccess;

if (node.IndexerOrSliceAccess is BoundIndexerAccess indexerAccess)
{
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

{

Consider adding an assert that indexerAccess.Arguments.Length == 1. #Closed

Debug.Assert(indexerAccess.Arguments.Length == 1);
if (isLeftOfAssignment && indexerAccess.GetRefKind() == RefKind.None)
{
// Note: we currently don't honor cacheAllArgumentsOnly in this branch, and let
// callers do the caching instead
// Tracked by https://github.com/dotnet/roslyn/issues/71056
AddPlaceholderReplacement(argumentPlaceholder, integerArgument);
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

AddPlaceholderReplacement(argumentPlaceholder, integerArgument);

What feels strange or confusing about this method now, is the fact that we do not respect cacheAllArgumentsOnly parameter on this code path. Two consumers make an extra effort to ensure that arguments are actually cached if we hit this code path. Before, this method didn't have to deal with caching arguments at all. I am not saying, that we absolutely have to clean this up in context of this PR, but this feels like something we should follow up on. #Closed

ImmutableArray<BoundExpression> rewrittenArguments = VisitArgumentsAndCaptureReceiverIfNeeded(
ref receiver,
captureReceiverMode: ReceiverCaptureMode.Default,
Expand All @@ -555,12 +561,30 @@ private BoundExpression GetUnderlyingIndexerOrSliceAccess(
}
else
{
if (cacheAllArgumentsOnly)
{
var integerTemp = F.StoreToTemp(integerArgument, out BoundAssignmentOperator integerStore);
locals.Add(integerTemp.LocalSymbol);
sideeffects.Add(integerStore);
integerArgument = integerTemp;
}

AddPlaceholderReplacement(argumentPlaceholder, integerArgument);
rewrittenIndexerAccess = VisitIndexerAccess(indexerAccess, isLeftOfAssignment);
}
}
else
{
rewrittenIndexerAccess = (BoundExpression)VisitArrayAccess(((BoundArrayAccess)node.IndexerOrSliceAccess));
if (cacheAllArgumentsOnly)
{
var integerTemp = F.StoreToTemp(integerArgument, out BoundAssignmentOperator integerStore);
locals.Add(integerTemp.LocalSymbol);
sideeffects.Add(integerStore);
integerArgument = integerTemp;
}

AddPlaceholderReplacement(argumentPlaceholder, integerArgument);
rewrittenIndexerAccess = (BoundExpression)VisitArrayAccess((BoundArrayAccess)node.IndexerOrSliceAccess);
}

RemovePlaceholderReplacement(argumentPlaceholder);
Expand Down Expand Up @@ -694,6 +718,20 @@ private BoundExpression DetermineMakePatternIndexOffsetExpressionStrategy(
}

private BoundExpression VisitRangePatternIndexerAccess(BoundImplicitIndexerAccess node)
{
var F = _factory;
var localsBuilder = ArrayBuilder<LocalSymbol>.GetInstance();
var sideEffectsBuilder = ArrayBuilder<BoundExpression>.GetInstance();

var rewrittenIndexerAccess = VisitRangePatternIndexerAccess(node, localsBuilder, sideEffectsBuilder, cacheAllArgumentsOnly: false);

return F.Sequence(
localsBuilder.ToImmutableAndFree(),
sideEffectsBuilder.ToImmutableAndFree(),
rewrittenIndexerAccess);
}

private BoundExpression VisitRangePatternIndexerAccess(BoundImplicitIndexerAccess node, ArrayBuilder<LocalSymbol> localsBuilder, ArrayBuilder<BoundExpression> sideEffectsBuilder, bool cacheAllArgumentsOnly)
{
Debug.Assert(node.ArgumentPlaceholders.Length == 2);
Debug.Assert(node.IndexerOrSliceAccess is BoundCall);
Expand Down Expand Up @@ -721,9 +759,6 @@ private BoundExpression VisitRangePatternIndexerAccess(BoundImplicitIndexerAcces
PatternIndexOffsetLoweringStrategy startStrategy, endStrategy;
RewriteRangeParts(rangeArg, out rangeExpr, out startMakeOffsetInput, out startStrategy, out endMakeOffsetInput, out endStrategy, out rewrittenRangeArg);

var localsBuilder = ArrayBuilder<LocalSymbol>.GetInstance();
var sideEffectsBuilder = ArrayBuilder<BoundExpression>.GetInstance();

// Do not capture receiver if it is a local or parameter and we are evaluating a pattern
// If length access is a local, then we are evaluating a pattern
if (node.LengthOrCountAccess.Kind is not BoundKind.Local || receiver.Kind is not (BoundKind.Local or BoundKind.Parameter))
Expand Down Expand Up @@ -889,6 +924,19 @@ private BoundExpression VisitRangePatternIndexerAccess(BoundImplicitIndexerAcces
startExpr = MakePatternIndexOffsetExpression(startMakeOffsetInput, lengthAccess, startStrategy);
BoundExpression endExpr = MakePatternIndexOffsetExpression(endMakeOffsetInput, lengthAccess, endStrategy);
rangeSizeExpr = MakeRangeSize(ref startExpr, endExpr, localsBuilder, sideEffectsBuilder);

if (cacheAllArgumentsOnly)
{
var startLocal = F.StoreToTemp(startExpr, out var startStore);
localsBuilder.Add(startLocal.LocalSymbol);
sideEffectsBuilder.Add(startStore);
startExpr = startLocal;

var rangeSizeLocal = F.StoreToTemp(rangeSizeExpr, out var rangeSizeStore);
localsBuilder.Add(rangeSizeLocal.LocalSymbol);
sideEffectsBuilder.Add(rangeSizeStore);
rangeSizeExpr = startLocal;
}
}
else
{
Expand All @@ -907,10 +955,7 @@ private BoundExpression VisitRangePatternIndexerAccess(BoundImplicitIndexerAcces
RemovePlaceholderReplacement(node.ArgumentPlaceholders[1]);
RemovePlaceholderReplacement(node.ReceiverPlaceholder);

return F.Sequence(
localsBuilder.ToImmutableAndFree(),
sideEffectsBuilder.ToImmutableAndFree(),
rewrittenIndexerAccess);
return rewrittenIndexerAccess;
}

private BoundExpression MakeRangeSize(ref BoundExpression startExpr, BoundExpression endExpr, ArrayBuilder<LocalSymbol> localsBuilder, ArrayBuilder<BoundExpression> sideEffectsBuilder)
Expand Down
Loading