Inconsistent order of evaluation between indexing with System.Index and System.Range

[dgrunwald]

Version Used: Roslyn main (20 Oct 2021)

Steps to Reproduce:

Run the following code:

using System;
using System.Collections;
using System.Collections.Generic;

class Program
{
    static CustomList list;

    public static void Main()
    {
        list = new CustomList { "first", "second", "last" };
        Console.WriteLine(list[AllButLast()]);
        list = new CustomList { "first", "second", "last" };
        Console.WriteLine(list[Last()]); // outputs "last" with VS2019 and "NewLast" with VS2022
    }

    static Range AllButLast()
    {
        list.Add("New");
        list.Add("NewLast");
        return 0..^1;
    }

    static Index Last()
    {
        list.Add("New");
        list.Add("NewLast");
        return ^1;
    }
}

internal class CustomList : IEnumerable<string>
{
    List<string> l = new List<string>();

    public void Add(string arg) => l.Add(arg);

    public int Count => l.Count;
    public string this[int index] => l[index];

    public CustomList Slice(int start, int length)
    {
        CustomList result = new CustomList();
        for (int i = 0; i < length; i++)
        {
            result.Add(this[start + i]);
        }
        return result;
    }

    public IEnumerator<string> GetEnumerator()
    {
        return l.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return l.GetEnumerator();
    }

    public override string ToString()
    {
        return string.Join(", ", l);
    }
}

Expected Behavior:
The methods AllButLast/Last should be called before the indexing operator retrieves list.Count.
Console.WriteLine(list[AllButLast()]) should output first, second, last, New.
Console.WriteLine(list[Last()]); should output NewLast.

Actual Behavior:
VS2019 retrieves list.Count before calling AllButLast/Last, output:

first, second
last

Roslyn main (20 Oct 2021) retrieves list.Count before calling AllButLast, but after calling Last, output:

first, second
NewLast

See on SharpLab

[dgrunwald]

Was the changed order of evaluation for list[Last()] intentional?
I don't see it listed in the "breaking changes" documents.

[hez2010]

Can reproduce with .NET RC2.
I think this is a bug. The current behavior makes below code produce different results:

list = new CustomList { "first", "second", "last" };
var range = AllButLast();
Console.WriteLine(list[range]); // first second last New

list = new CustomList { "first", "second", "last" };
Console.WriteLine(list[AllButLast()]); // first second

Where the second example should produce first second last New.

[jcouv]

Tagging @AlekseyTs to triage since feels like this might be related to #56611

[hez2010]

might be related to #56611

Seems like the same problem but for System.Range.

[AlekseyTs]

Here is a quote from https://github.com/dotnet/csharplang/blob/main/proposals/csharp-8.0/ranges.md#adding-index-and-range-support-to-existing-library-types:

---

The new indexer will be implemented by converting the argument of type Index into an int and emitting a call to the int based indexer. For discussion purposes, let's use the example of receiver[expr]. The conversion of expr to int will occur as follows:

• When the argument is of the form ^expr2 and the type of expr2 is int, it will be translated to receiver.Length - expr2.
• Otherwise, it will be translated as expr.GetOffset(receiver.Length).

---

Therefore, list[Last()] is supposed to be evaluated as list[Last().GetOffset(list.Count)]. I.e. Count should be evaluated after Last() is evaluated. We used to have a bug in this area (see #54085), which got fixed recently.

Here is a quote from https://github.com/dotnet/csharplang/blob/main/proposals/csharp-8.0/ranges.md#implicit-range-support:

---

When the Range based indexer is bound on an expression named receiver, it will be lowered by converting the Range expression into two values that are then passed to the Slice method. For discussion purposes, let's use the example of receiver[expr].

The first argument of Slice will be obtained by converting the range typed expression in the following way:

• When expr is of the form expr1..expr2 (where expr2 can be omitted) and expr1 has type int, then it will be emitted as expr1.
• When expr is of the form ^expr1..expr2 (where expr2 can be omitted), then it will be emitted as receiver.Length - expr1.
• When expr is of the form ..expr2 (where expr2 can be omitted), then it will be emitted as 0.
• Otherwise, it will be emitted as expr.Start.GetOffset(receiver.Length).

This value will be re-used in the calculation of the second Slice argument. When doing so it will be referred to as start. The second argument of Slice will be obtained by converting the range typed expression in the following way:

• When expr is of the form expr1..expr2 (where expr1 can be omitted) and expr2 has type int, then it will be emitted as expr2 - start.
• When expr is of the form expr1..^expr2 (where expr1 can be omitted), then it will be emitted as (receiver.Length - expr2) - start.
• When expr is of the form expr1.. (where expr1 can be omitted), then it will be emitted as receiver.Length - start.
• Otherwise, it will be emitted as expr.End.GetOffset(receiver.Length) - start.

The receiver, Length, and expr expressions will be spilled as appropriate to ensure any side effects are only executed once.

---

Therefore, evaluation of list[AllButLast()] involves list, amount of its items and the value returned by AllButLast() used multiple times. The specification says that the values should be spilled: "The receiver, Length, and expr expressions will be spilled as appropriate to ensure any side effects are only executed once." The relative order for the spilling is not specified explicitly, but the Length comes before expr in that sentence. I am comfortable interpreting that as the intent was to spill the Length first, which matches the current behavior. My recommendation would be to resolve this issue as "By Design".

[dgrunwald]

list[AllButLast()] doesn't fit into any of the listed cases, so I'd expect "Otherwise, ..." to apply.
And if we assume that "the first Slice argument" is evaluated before "the second Slice argument", I get that list[AllButLast()] is equivalent to list.Slice(start = AllButLast().Start.GetOffset(list.Length), AllButLast().End.GetOffset(list.Length) - start); (except that AllButLast is only called once).

So that part of the spec suggests a different evaluation order. So clearly the spec will need to explicitly specify the evaluation order; it's just ambiguous as-is.

[dgrunwald]

I also don't think it's sane to have the order of evaluation depend on the type of the expression.
Current Roslyn main: list[^GetInt()] will call get_Count before GetInt; list[GetIndex()] will call GetIndex before get_Count; and list[GetRange()] will call get_Count before GetRange.

In VS2019 at least it was consistent (get_Count always first). So maybe #56611 should be reverted? But my preferred solution would be to go all the way and put get_Count always last, so that list[expr] is equivalent to var x = expr; list[x] like most programmers would expect.

[333fred]

Discussed in LDM.

Conclusion: Codify the expected behavior as receiver then expr then Length.