[qa-feature-bug] evaluation order for member access and assignment

[GeeLaw]

I was studying how C# defines / Microsoft C# compiler implements the evaluation order for

public class ReferenceType
{
  public int Field;
  public int SettableProperty { set { } }
  public ref int ByRefProperty { get { return ref Field; } }
  public int this[int index] { set { } }
  public ref int this[int a, int b] { get { return ref Field; } }
}

ReferenceType o = null;
// "Method" has side effects.
o.Field = Method();
o.SettableProperty = Method();
o.ByRefProperty = Method();
o[Method()] = Method();
o[0, Method()] = Method();

Please see the test here on GitHub Gist.

Also reference to Raymond Chen's blog on C# evaluation order:

With the right-to-left rule, you get a different surprise:

x.value += Calculate();
x[index] += Calculate();

If x is null, or if index is out of bounds, the corresponding exception is not raised until after the Calculate has occurred. Some people may find this surprising.

Note that he was talking about a hypothetical situation to justify the left-to-right rule. Imagine my surprise when I found otherwise. (See array indexer at last!)

From C# spec 12.23.2 simple assignment and 12.8.7 member access and 15.7 properties...

o.Field = Method();

From 12.8.7:

• "The member_access is evaluated and classified as follows:",
• item "If E is a property access, indexer access, variable, or value" (E is o, a variable),
• subitem "If T is a class_type and I identifies an instance field of that class_type:" (T is ReferenceType and I is Field),

the left-hand side is evaluated and classified as follows.

• If the value of E is null, then a System.NullReferenceException is thrown.
• Otherwise, if the field is readonly and the reference occurs outside an instance constructor of the class in which the field is declared, then the result is a value, namely the value of the field I in the object referenced by E.
• Otherwise, the result is a variable, namely the field I in the object referenced by E.

The left-hand side is classified as a variable, and it is the variable o.Field if o is not null, and it throws NullReferenceException if the o is null upon evaluation.

From 12.23.2:

• "which consists of the following recursive steps"
• first item "x is evaluated if it wasn’t already." (x is o.Field, so o.Field should be evaluated)
• second item "If x is classified as a variable" ...

This means the evaluation of o.Field = value should be as follows:

// specified behavior
var E = o;
ref var x = ref E.Field; // NullReferenceException
var y = Method();        // not executed
x = y;

However, the test (GitHub Gist) shows otherwise:

// observed behavior
var E = o;
var y = Method();        // say this throws
ref var x = ref E.Field; // not executed, no NullReferenceException
x = y;

According to my reading of the spec, I expect it to produce

testing null.Field = throw;
caught NullReferenceException

testing null.Field = value;
caught NullReferenceException

But on .NET 10 (Windows 11 version 24H2), it produces

testing null.Field = throw;
RHS throw
caught ApplicationException

testing null.Field = value;
RHS
caught NullReferenceException

This seems to be the case also in CLR v2.0.50727 (.NET 2.0 on Windows 11 version 24H2).

o.SettableProperty = Method();

The wording of the spec appears to do something redundant and the implementation doesn't do that (as it shouldn't). I fully expect this statement to be the same as o.set_SettableProperty(Method());, which does the following:

1. Evaluate o and store it in temp1.
2. Evaluate Method() and store it in temp2.
3. If temp1 is null, throw NullReferenceException.
4. Otherwise, invoke temp1.set_SettableProperty(temp2).

This is also what happens in the actual implementation, but the wording of spec is annoying.

From 12.23.2:

• "which consists of the following recursive steps"
• first item "x is evaluated if it wasn’t already." (x is o.SettableProperty, so o.SettableProperty should be evaluated)

Of course, the settable property is not gettable, hence cannot be evaluated, because the meaning of "evaluation" of o.SettableProperty is defined in 12.8.7:

• "If E is a property access, indexer access, variable, or value"
• First, if E is a property or indexer access, then the value of the property or indexer access is obtained (§12.2.2) and E is reclassified as a value.

From 12.2.2:

• The value of a property access expression is obtained by invoking the get accessor of the property. If the property has no get accessor, a compile-time error occurs. Otherwise, a function member invocation (§12.6.6) is performed, and the result of the invocation becomes the value of the property access expression.

Of course, there's no need to know the value of the left-hand side to proceed with evaluation.

I would suggest updating the wording of 12.23.2, so that for assignment to settable property, the property itself is not got.

o.ByRefProperty = Method();

The spec currently does not define how this is processed. Potential candidates:

// candidate 1, consistent with left-to-right principle
var E = o;
ref var x = ref E.ByRefProperty;
var y = Method();
x = y;

// candidate 2, consistent with observed behavior for fields
//          and feels like observed behavior of settable property
var E = o;
var y = Method();
ref var x = ref E.ByRefProperty;
x = y;
// But this is hectic when the getter of ByRefProperty has side effects.

The observed behavior is candidate 1 (consistent with left-to-right principle).

o[Method1()] = Method2(); (settable indexer)

The spec says the behavior is

var target = o;
var index = Method1();

_ = E[index];
// ^^^ This is from "`x` is evaluated if it wasn’t already", but not really expected nor desired.

var value = Method2();
target.set_Items(index, value);
// ^^^ Or whatever indexer setter name it is.

The desired and actual behavior is

var target = o;
var index = Method1();
var value = Method2();
target.set_Items(index, value);

o[0, Method1()] = Method2(); (byref indexer)

The spec currently does not define how this is processed. Potential candidates:

// candidate 1, consistent with left-to-right principle
var target = o;
var index1 = 0;
var index2 = Method1();
ref var x = ref target[index1, index2];
var y = Method2();
x = y;

// candidate 2, feels like observed behavior for settable indexers
var target = o;
var index1 = 0;
var index2 = Method1();
var y = Method2();
ref var x = ref target[index1, index2];
x = y;
// But this is hectic when the byref indexer getter has side effects other than bounds check.

Array indexers? a[Method1()] = Method2();

It turns out that, although array indexers are byref indexers, the observed behavior is NOT the same as user-defined byref indexers! Furthermore, it is also different from what the spec says!

// spec says (see 12.8.12.2 array access, then follow the logic of field assignment above)
var array = a;
var index = Method1();
ref var item = ref array[index];
var value = Method2();
item = value;

// observed
var array = a;
var index = Method1();
var value = Method2();
ref var item = ref array[index];
item = value;

What now?

My questions/suggestions:

1. Am I reading the spec correctly? Is there a bug in C# spec or Microsoft's C# compiler or JIT?
2. If there is a bug, where is this bug? In C# spec or in implementation of C# compiler and JIT?
3. Which parts should be updated?

Personally, I think it's good to do the following:

1. Review prehistoric behaviors. Check whether the behavior has changed in different versions of .NET Framework, .NET Core, .NET, and different versions of C# compilers shipped by Microsoft.
2. Pick the correct behavior for field access, settable properties, settable indexers, array indexers. Update the implementation. Update the spec to clarify what actually is implemented.
3. Assignment to byref properties and indexers should be properly defined and implemented.

In general, I wish lookalike things behave similarly. The observed behaviors for fields, settable properties, settable indexers, array indexers are consistent: It's always the same as target.MutatingMethod(indices..., value);, where null check for target (as well as whatever range for indices... and validity of value etc.) comes after fully evaluating target, indices..., value in that order.

For this reason, I propose that byref properties and indexers should be implemented as follows:

// consistent with target.Mutating(indices..., value)

// Method1().ByRefProperty = Method2();
var target = Method1();
var value = Method2();

// Method1().ByRefIndexer[Method2(), ...] = Method3();
var target = Method1();
var index1 = Method2();
// ...
var value = Method3();
ref var item = target[index1, ...];
item = value;

They are different from what's implemented today.

Other opinions welcomed.

[HaloFour]

That's a wall of text and a half. My understanding is that the spec says that all assignment expressions evaluate RHS side first.

[GeeLaw]

That's blatantly not the case from day 1 of C# (all sub-expressions are supposed to evaluate left-to-right, and assignment is not an exception to this rule from the face value of text). Unfortunately, I don't have a shorter version of the text, because the issue in question is in the nature of language lawyers.

[HaloFour]

According to 12.4.2:

The assignment operators, the null coalescing operator and the conditional operator (?:) are right-associative, meaning that operations are performed from right to left.

[GeeLaw]

Associativity is not the same as evaluation order of subexpressions. A.X = B.Y = C.Z being right-associative and left-to-right-evaluated means (suppose X, Y, Z are properties and A, B, C are reference-typed):

// "A.X = B.Y = C.Z;" means "A.X = (B.Y = C.Z);"
var iife = () =>
{
  // Evaluate subexpression "A"
  var a = A;
  // rhs_iife represents subexpression "B.Y = C.Z"
  var rhs_iife = () =>
  {
    // Evaluate subexpression "B"
    var b = B;
    // rrhs_iife represents subexpression "C.Z"
    var rrhs_iife = () =>
    {
      // Evaluate subexpression "C"
      var c = C;
      return c.get_Z();
    };
    // Evaluate subexpression "C.Z"
    var rrhs = rrhs_iife();
    // Complete assignment
    b.set_Y(rrhs);
    // Return the value of subexpression "B.Y = C.Z",
    // defined to be the value of RHS
    return rrhs;
  };
  // Evaluate subexpression "B.Y = C.Z"
  var rhs = rhs_iife();
  // Complete assignment
  a.set_X(rhs);
  // Return the value of entire expression
  return rhs;
};
// Evaluate entire expression
iife();

If you don't believe me, just try this:

int[] A() { Console.Write("A"); return new int[1]; }
int   B() { Console.Write("B"); return 0; }
int   C() { Console.Write("C"); return 0; }
A()[B()] = C();
// Output is ABC, not CAB nor CBA.

The array indexer part in this discussion concerns when null and index range checks are performed.

[HaloFour]

So you're suggesting that the language and spec have always been wrong? I suggest it's much more likely that you're interpreting it incorrectly. Either way, 25 years in the behavior of the language isn't going to change.

[GeeLaw]

Unfortunately, I can't test this on the old compiler, so I can't really check if the current behavior is the same as the past. It's entirely possible that the spec and the implementation haven't agreed for a long time.

I suggest it's much more likely that you're interpreting it incorrectly.

That's definitely a possibility, but I would suggest that you put more effort into understanding the spec before making a prediction.

25 years in the behavior of the language isn't going to change.

If you do care about the issue, you can read the "What now?" section. I wasn't suggesting that the only solution is to change the implementation, and I suggested changing the spec. In addition, the behavior for byref properties and byref indexers are pretty new and has some inconsistencies with the other kinds of assignments.

[GeeLaw]

After some more search, it appears this issue has been raised: dotnet/csharpstandard#169 and dotnet/csharpstandard#401.

[GeeLaw]

Manually converted to issue dotnet/csharpstandard#1478.