Skip to content
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

Unify representation of an implicit Index indexer over an array and non-array types. #57918

Merged
merged 1 commit into from
Nov 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 31 additions & 13 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3220,7 +3220,7 @@ private BoundExpression BindArrayDimension(ExpressionSyntax dimension, BindingDi
var size = BindValue(dimension, diagnostics, BindValueKind.RValue);
if (!size.HasAnyErrors)
{
size = ConvertToArrayIndex(size, diagnostics, allowIndexAndRange: false);
size = ConvertToArrayIndex(size, diagnostics, allowIndexAndRange: false, indexOrRangeWellknownType: out _);
if (IsNegativeConstantForArraySize(size))
{
Error(diagnostics, ErrorCode.ERR_NegativeArraySize, dimension);
Expand Down Expand Up @@ -7517,7 +7517,7 @@ private BoundExpression BindArrayAccess(SyntaxNode node, BoundExpression expr, A
Error(diagnostics, ErrorCode.ERR_NamedArgumentForArray, node);
}

bool hasErrors = ReportRefOrOutArgument(arguments, diagnostics);
ReportRefOrOutArgument(arguments, diagnostics);
var arrayType = (ArrayTypeSymbol)expr.Type;

// Note that the spec says to determine which of {int, uint, long, ulong} *each* index
Expand All @@ -7535,11 +7535,12 @@ private BoundExpression BindArrayAccess(SyntaxNode node, BoundExpression expr, A

// Convert all the arguments to the array index type.
BoundExpression[] convertedArguments = new BoundExpression[arguments.Arguments.Count];
WellKnownType indexOrRangeWellknownType = WellKnownType.Unknown;
for (int i = 0; i < arguments.Arguments.Count; ++i)
{
BoundExpression argument = arguments.Arguments[i];

BoundExpression index = ConvertToArrayIndex(argument, diagnostics, allowIndexAndRange: rank == 1);
BoundExpression index = ConvertToArrayIndex(argument, diagnostics, allowIndexAndRange: rank == 1, out indexOrRangeWellknownType);
convertedArguments[i] = index;

// NOTE: Dev10 only warns if rank == 1
Expand All @@ -7555,23 +7556,37 @@ private BoundExpression BindArrayAccess(SyntaxNode node, BoundExpression expr, A
}
}

TypeSymbol resultType = rank == 1 &&
TypeSymbol.Equals(
convertedArguments[0].Type,
Compilation.GetWellKnownType(WellKnownType.System_Range),
TypeCompareKind.ConsiderEverything)
TypeSymbol resultType = indexOrRangeWellknownType == WellKnownType.System_Range
? arrayType
: arrayType.ElementType;

return hasErrors
? new BoundArrayAccess(node, BindToTypeForErrorRecovery(expr), convertedArguments.Select(e => BindToTypeForErrorRecovery(e)).AsImmutableOrNull(), resultType, hasErrors: true)
: new BoundArrayAccess(node, expr, convertedArguments.AsImmutableOrNull(), resultType, hasErrors: false);
if (indexOrRangeWellknownType == WellKnownType.System_Index)
{
Debug.Assert(convertedArguments.Length == 1);

var int32 = GetSpecialType(SpecialType.System_Int32, diagnostics, node);
var receiverPlaceholder = new BoundImplicitIndexerReceiverPlaceholder(expr.Syntax, GetValEscape(expr, LocalScopeDepth), expr.Type) { WasCompilerGenerated = true };
var argumentPlaceholders = ImmutableArray.Create(new BoundImplicitIndexerValuePlaceholder(convertedArguments[0].Syntax, int32) { WasCompilerGenerated = true });

return new BoundImplicitIndexerAccess(
node,
argument: convertedArguments[0],
lengthOrCountAccess: new BoundArrayLength(node, receiverPlaceholder, int32) { WasCompilerGenerated = true },
receiverPlaceholder,
indexerOrSliceAccess: new BoundArrayAccess(node, expr, ImmutableArray<BoundExpression>.CastUp(argumentPlaceholders), resultType),
argumentPlaceholders,
resultType);
}

return new BoundArrayAccess(node, expr, convertedArguments.AsImmutableOrNull(), resultType);
}

private BoundExpression ConvertToArrayIndex(BoundExpression index, BindingDiagnosticBag diagnostics, bool allowIndexAndRange)
private BoundExpression ConvertToArrayIndex(BoundExpression index, BindingDiagnosticBag diagnostics, bool allowIndexAndRange, out WellKnownType indexOrRangeWellknownType)
{
Debug.Assert(index != null);

indexOrRangeWellknownType = WellKnownType.Unknown;

if (index.Kind == BoundKind.OutVariablePendingInference)
{
return ((OutVariablePendingInference)index).FailInference(this, diagnostics);
Expand All @@ -7597,6 +7612,7 @@ private BoundExpression ConvertToArrayIndex(BoundExpression index, BindingDiagno
result = TryImplicitConversionToArrayIndex(index, WellKnownType.System_Range, node, diagnostics);
if (result is object)
{
indexOrRangeWellknownType = WellKnownType.System_Range;
// This member is needed for lowering and should produce an error if not present
_ = GetWellKnownTypeMember(
WellKnownMember.System_Runtime_CompilerServices_RuntimeHelpers__GetSubArray_T,
Expand All @@ -7606,6 +7622,8 @@ private BoundExpression ConvertToArrayIndex(BoundExpression index, BindingDiagno
}
else
{
indexOrRangeWellknownType = WellKnownType.System_Index;

// This member is needed for lowering and should produce an error if not present
_ = GetWellKnownTypeMember(
WellKnownMember.System_Index__GetOffset,
Expand Down Expand Up @@ -7733,7 +7751,7 @@ private BoundExpression BindPointerElementAccess(SyntaxNode node, BoundExpressio

BoundExpression index = arguments[0];

index = ConvertToArrayIndex(index, diagnostics, allowIndexAndRange: false);
index = ConvertToArrayIndex(index, diagnostics, allowIndexAndRange: false, indexOrRangeWellknownType: out _);
return new BoundPointerElementAccess(node, expr, index, CheckOverflowAtRuntime, refersToLocation: false, pointedAtType, hasErrors);
}

Expand Down
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1610,7 +1610,9 @@ internal static PropertySymbol GetPropertySymbol(BoundExpression expr, out Bound
BoundImplicitIndexerAccess { IndexerOrSliceAccess: BoundCall call } => call.Method,
// this[int]
BoundImplicitIndexerAccess { IndexerOrSliceAccess: BoundIndexerAccess indexerAccess } => indexerAccess.Indexer,
// array[int]
// array[Index]
BoundImplicitIndexerAccess { IndexerOrSliceAccess: BoundArrayAccess } => null,
// array[int or Range]
BoundArrayAccess => null,
BoundDynamicIndexerAccess => null,
BoundBadExpression => null,
Expand Down
14 changes: 14 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundArrayAccess.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace Microsoft.CodeAnalysis.CSharp
{
internal partial class BoundArrayAccess
{
internal BoundArrayAccess WithReceiver(BoundExpression receiver)
{
return this.Update(receiver, this.Indices, this.Type);
}
}
}
19 changes: 17 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDagEvaluation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,29 @@ private Symbol? Symbol
BoundDagTypeEvaluation e => e.Type,
BoundDagDeconstructEvaluation e => e.DeconstructMethod,
BoundDagIndexEvaluation e => e.Property,
BoundDagSliceEvaluation e => e.IndexerAccess is BoundArrayAccess arrayAccess ? arrayAccess.Expression.Type : Binder.GetIndexerOrImplicitIndexerSymbol(e.IndexerAccess),
BoundDagIndexerEvaluation e => e.IndexerAccess is BoundArrayAccess arrayAccess ? arrayAccess.Expression.Type : Binder.GetIndexerOrImplicitIndexerSymbol(e.IndexerAccess),
BoundDagSliceEvaluation e => getSymbolFromIndexerAccess(e.IndexerAccess),
BoundDagIndexerEvaluation e => getSymbolFromIndexerAccess(e.IndexerAccess),
BoundDagAssignmentEvaluation => null,
_ => throw ExceptionUtilities.UnexpectedValue(this.Kind)
};

Debug.Assert(result is not null || this is BoundDagAssignmentEvaluation);
return result;

static Symbol? getSymbolFromIndexerAccess(BoundExpression indexerAccess)
{
switch (indexerAccess)
{
case BoundArrayAccess arrayAccess:
return arrayAccess.Expression.Type;

case BoundImplicitIndexerAccess { IndexerOrSliceAccess: BoundArrayAccess arrayAccess }:
Copy link
Member

@jcouv jcouv Nov 22, 2021

Choose a reason for hiding this comment

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

nit: Consider commenting: this case is array[int] and the one above is array[Index/Range] #Pending

return arrayAccess.Expression.Type;

default:
return Binder.GetIndexerOrImplicitIndexerSymbol(indexerAccess);
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ internal BoundExpression GetReceiver()
{
BoundIndexerAccess { ReceiverOpt: var r } => r,
BoundCall { ReceiverOpt: var r } => r,
BoundArrayAccess { Expression: var r } => r,
_ => throw ExceptionUtilities.UnexpectedValue(this.IndexerOrSliceAccess.Kind)
};

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2078,7 +2078,7 @@
<Field Name="ReceiverPlaceholder" Type="BoundImplicitIndexerReceiverPlaceholder" SkipInVisitor="true" />

<!--
May be BoundIndexerAccess or BoundCall,
May be BoundIndexerAccess or BoundCall or BoundArrayAccess,
which are built using the real receiver (not a placeholder) and placeholder(s) for argument(s).
We don't mark this property as SkipInVisitor="true" because it contains real receiver, but we
make sure SemanticModel doesn't dig too deep into the node.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,33 +685,6 @@ public override BoundNode VisitArrayAccess(BoundArrayAccess node)

BoundNode resultExpr;
if (TypeSymbol.Equals(
indexType,
_compilation.GetWellKnownType(WellKnownType.System_Index),
TypeCompareKind.ConsiderEverything))
{
// array[Index] is treated like a pattern-based System.Index indexing
// expression, except that array indexers don't actually exist (they
// don't have symbols)

var arrayLocal = F.StoreToTemp(
VisitExpression(node.Expression),
out BoundAssignmentOperator arrayAssign);

BoundExpression makeOffsetInput = DetermineMakePatternIndexOffsetExpressionStrategy(node.Indices[0], out PatternIndexOffsetLoweringStrategy strategy);

var indexOffsetExpr = MakePatternIndexOffsetExpression(
makeOffsetInput,
F.ArrayLength(arrayLocal),
strategy);

resultExpr = F.Sequence(
ImmutableArray.Create(arrayLocal.LocalSymbol),
ImmutableArray.Create<BoundExpression>(arrayAssign),
F.ArrayAccess(
arrayLocal,
ImmutableArray.Create(indexOffsetExpr)));
}
else if (TypeSymbol.Equals(
indexType,
_compilation.GetWellKnownType(WellKnownType.System_Range),
TypeCompareKind.ConsiderEverything))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ private BoundExpression VisitImplicitIndexerAccess(BoundImplicitIndexerAccess no
private BoundExpression VisitIndexPatternIndexerAccess(BoundImplicitIndexerAccess node, bool isLeftOfAssignment)
{
Debug.Assert(node.ArgumentPlaceholders.Length == 1);
Debug.Assert(node.IndexerOrSliceAccess is BoundIndexerAccess);
Debug.Assert(node.IndexerOrSliceAccess is BoundIndexerAccess or BoundArrayAccess);

Debug.Assert(TypeSymbol.Equals(
node.Argument.Type,
Expand Down Expand Up @@ -259,15 +259,17 @@ private BoundExpression VisitIndexPatternIndexerAccess(BoundImplicitIndexerAcces
switch (strategy)
{
case PatternIndexOffsetLoweringStrategy.SubtractFromLength:
// ensure we evaluate the input before accessing length
if (makeOffsetInput.ConstantValue is null)
BoundExpression lengthAccess = RewriteLengthAccess(node, receiver);

// ensure we evaluate the input before accessing length, unless it is an array length
Copy link
Member

@jcouv jcouv Nov 22, 2021

Choose a reason for hiding this comment

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

why is array length different in terms of ordering? #Resolved

Copy link
Contributor Author

@AlekseyTs AlekseyTs Nov 22, 2021

Choose a reason for hiding this comment

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

why is array length different in terms of ordering?

Because it is an IL instruction, not a property access. I.e. not observable. There is no change in behavior here, we are doing what we used to do.

if (makeOffsetInput.ConstantValue is null && lengthAccess.Kind is not BoundKind.ArrayLength)
{
makeOffsetInput = F.StoreToTemp(makeOffsetInput, out BoundAssignmentOperator inputStore);
locals.Add(((BoundLocal)makeOffsetInput).LocalSymbol);
sideeffects.Add(inputStore);
}

integerArgument = MakePatternIndexOffsetExpression(makeOffsetInput, RewriteLengthAccess(node, receiver), strategy);
integerArgument = MakePatternIndexOffsetExpression(makeOffsetInput, lengthAccess, strategy);
break;

case PatternIndexOffsetLoweringStrategy.UseAsIs:
Expand All @@ -282,40 +284,46 @@ private BoundExpression VisitIndexPatternIndexerAccess(BoundImplicitIndexerAcces
throw ExceptionUtilities.UnexpectedValue(strategy);
}

var indexerAccess = (BoundIndexerAccess)node.IndexerOrSliceAccess;
Debug.Assert(node.ArgumentPlaceholders.Length == 1);
var argumentPlaceholder = node.ArgumentPlaceholders[0];
AddPlaceholderReplacement(argumentPlaceholder, integerArgument);

BoundExpression rewrittenIndexerAccess;

if (isLeftOfAssignment && indexerAccess.Indexer.RefKind == RefKind.None)
if (node.IndexerOrSliceAccess is BoundIndexerAccess indexerAccess)
{
ImmutableArray<BoundExpression> rewrittenArguments = VisitArguments(
indexerAccess.Arguments,
indexerAccess.Indexer,
indexerAccess.ArgsToParamsOpt,
indexerAccess.ArgumentRefKindsOpt,
ref receiver,
out ArrayBuilder<LocalSymbol>? temps);
if (isLeftOfAssignment && indexerAccess.Indexer.RefKind == RefKind.None)
{
ImmutableArray<BoundExpression> rewrittenArguments = VisitArguments(
indexerAccess.Arguments,
indexerAccess.Indexer,
indexerAccess.ArgsToParamsOpt,
indexerAccess.ArgumentRefKindsOpt,
ref receiver,
out ArrayBuilder<LocalSymbol>? temps);

if (temps is not null)
{
locals.AddRange(temps);
temps.Free();
}

if (temps is not null)
rewrittenIndexerAccess = indexerAccess.Update(
receiver, indexerAccess.Indexer, rewrittenArguments,
indexerAccess.ArgumentNamesOpt, indexerAccess.ArgumentRefKindsOpt,
indexerAccess.Expanded,
indexerAccess.ArgsToParamsOpt,
indexerAccess.DefaultArguments,
indexerAccess.Type);
}
else
{
locals.AddRange(temps);
temps.Free();
rewrittenIndexerAccess = VisitIndexerAccess(indexerAccess.WithReceiver(receiver), isLeftOfAssignment);
}

rewrittenIndexerAccess = indexerAccess.Update(
receiver, indexerAccess.Indexer, rewrittenArguments,
indexerAccess.ArgumentNamesOpt, indexerAccess.ArgumentRefKindsOpt,
indexerAccess.Expanded,
indexerAccess.ArgsToParamsOpt,
indexerAccess.DefaultArguments,
indexerAccess.Type);
}
else
{
rewrittenIndexerAccess = VisitIndexerAccess(indexerAccess.WithReceiver(receiver), isLeftOfAssignment);
rewrittenIndexerAccess = (BoundExpression)VisitArrayAccess(((BoundArrayAccess)node.IndexerOrSliceAccess).WithReceiver(receiver));
}

RemovePlaceholderReplacement(argumentPlaceholder);
Expand Down
15 changes: 15 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/IndexAndRangeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class C
{
static int M1(int[] arr) => arr[^1];
static char M2(string s) => s[^1];
static int M3(int[] arr, int i) => arr[^i];
}
";
var verifier = CompileAndVerifyWithIndexAndRange(src);
Expand Down Expand Up @@ -152,6 +153,20 @@ .maxstack 3
IL_0009: callvirt ""char string.this[int].get""
IL_000e: ret
}
");
verifier.VerifyIL("C.M3", @"
{
// Code size 8 (0x8)
.maxstack 3
IL_0000: ldarg.0
IL_0001: dup
IL_0002: ldlen
IL_0003: conv.i4
IL_0004: ldarg.1
IL_0005: sub
IL_0006: ldelem.i4
IL_0007: ret
}
");
}

Expand Down
Loading