dotnet / dotnet/csharpstandard

Evaluation order of (simple) assignments (incl. field assignment, property setter, indexer setter, array element assignment, byref property, byref indexers)

Open
#1,478 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
815
Forks
99
Avg merge
1d 14h
Merged PRs (30d)
16

Description

**Describe the bug**

The current spec concerning simple assignment is inconsistent with the current Microsoft implementation of C#. In particular, various `null` checks and index range checks happen in inconsistent order w.r.t. other side effects.

**Example** See [this GitHub Gist](https://gist.github.com/GeeLaw/e442311dcc1503979988db927b0735f4).

**Expected behavior** See below for analysis. All the tests by me were done on Windows 11 version 24H2 with C# compiler that comes with VS 2026 (18.0.1), targeting a few runtimes.

**Cross references** #169, #401, dotnet/csharplang#9836.

## Field assignment

Same issue with #401.

- Spec requires `E.M = F` (for reference-type `E` and field `M`) to evaluate `E` to `e`, then check `e` for `null`, then evaluate `F` to `f`, then store `f` in `e.M`.
- Implementation is to evaluate `E` to `e`, then evaluate `F` to `f`, then check `e` for `null`, then store `f` in `e.M`.

**Note the timing of `null` check is different!**

## Property setter ~~(not bug, but we should know for a comprehensive view)~~ (wording bug in spec, see [comment 2](https://github.com/dotnet/csharpstandard/issues/1478#issuecomment-3575530175))

## Indexer setter ~~(not bug, for comprehensive view)~~ (wording bug in spec similar to property setter, see comment 2)

## Array element assignment, single-dimensional zero-based (SZArray)

This is the same as #169. Last time it was discussed, the resolution was to put the deviation into documentation, instead of changing the spec. I think this should be reconsidered for two reasons:

1. It's still [not documented](https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/breaking-changes/deviations%20from%20standard) after **3 years of resolution**.
2. Is there really an implementation that doesn't agree with Roslyn? (What's the behavior of the old native compiler?) If all implementations agree, the spec should just be updated.

- Spec requires `A[I] = F` (for `A` of type `T[]` and `I` of appropriate type) to be done by evaluating `A` to `a`, then `I` to `i`, then check `a` for `null`, then check `a,i` for range, then evaluate `F` to `f`, then check for variance, then store `f` in `a[i]`.
- Implementation evaluates `A` to `a`, then `I` to `i`, then `F` to `f`, then checks for `null` and range and variance, then stores `f` in `a[i]`.

## Array element assignment, multi-dimensional

- Spec requires `A[I1, ..., In] = F` (for `A` of type `T[,...]` and `I`s of appropriate type) to be done by evaluating `A` to `a`, then `I1` to `i1`, ..., `In` to `in`, then check `a` for `null`, then check `a,i1,...,in` for range, then evaluate `F` to `f`, then check for variance, then store `f` in `a[i1, ..., in]`.
- Implementation (when combined with JIT in .NET Framework 2.0-4.5) agrees with the spec.
- Implementation (when combined with JIT in .NET 8.0) doesn't agree with the spec. It evaluates `F` before checking for `null` and bounds.

The difference with SZArray is because the C# compiler uses `stelem` for SZArray, but `Set` for MDArray. (There's also single-dimensional non-zero-based arrays, but those are unspeakable in C#, so not of concern.)

**This is very worth of being documented,** because behavior changes between .NET Framework and .NET (Core), which could trip people. Moreover, C# compiler emits `Set` method call for both .NET Framework and .NET (Core), so it's the JIT doing something differently. Consider cc'ing the JIT people?

In fact, this is very surprising, because C# compiler emits `call` (not `callvirt`) to `Set`, which means as far as JIT is concerned, there's no `null` check until control **has entered** the `Set` method:

```msil
// Suppose we're storing into int[,].
// load a onto stack
// load i1 onto stack
// ...
// load in onto stack
// load f onto stack
call instance void int32[,]::Set(int32, int32, int32)
```

Therefore, the only place `NullReferenceException` for `a` can be thrown is inside `Set`, at which point `F` has been evaluated into `f`.

- The current C# compiler always emits incorrect IL as far as IL and (specked) C# semantics are concerned.
- The JIT of .NET Framework compiles IL into incorrect native code as far as IL semantics are concerned. This (sometimes?) make the C# semantics correctly reflected in native code. (`(-1)*(-1)=+1`.)
- The JIT of .NET (Core) compiles IL into correct native code as far as IL semantics are concerned, so the native code does not reflect C# semantics.

## Byref property and byref indexer

It's unclear what the spec says here, because the spec text **does not deal with property/indexer access without a setter.** (See "12.23.2 Simple assignment", where if the left-hand side is a property/indexer access, it requires a setter.)

This should be specified. The current behavior is as follows:

1. Evaluate the receiver.
2. Evaluate all the indices (possible 0) from left to right.
3. Invoke the byref getter method, which checks for `null` receiver and surfaces all the side effects of the getter. (We have now obtained the location.)
4. Evaluate the right-hand side. (We have not obtained the value.)
5. Store the value into the location.

## What now?

I would advocate rewording of spec to reflect the behavior on .NET 8.0. Moreover, it's based on a very simple principle:

> For all constructs prior to C# 7.0, `E.M = F` and `E[I1, ..., In] = F` should have the same effect as if it's some `E.setter(I1, ..., In, F)`, regardless of the nature of `M` or the indexer. In other words, things that visually appear similar should have the same order of side effects.
>
> Since C# 7.0, byref property/indexer assignments translate to `E.getter(I1, ..., In) = F`.

From this we deduce

```csharp
// E.Field = F; // field
[ref] var e = E;
[ref] var f = F; // [ref] in case it's ref reassign into ref field in byref value type
e.Field = [ref] f; // null check for `e` here
// == current impl
// != current spec

// E.SP = F; // property setter
E.set_SP(F); // = current impl = current spec

// E[I1, ..., In] = F; // index setter
E.set_Items(I1, ..., In, F); // = current impl = current spec

// E[I] = F; // SZArray
var e = E;
var i = I;
var f = F;
e[i] = f; // null check, range check, variance check
// == current impl
// != current spec

// E[I1, ..., In] = F; // MDArray
var e = E;
var i1 = I1;
// ...
var in = In;
var f = F;
e[i1, ..., in] = f; // null check, range check, variance check
// == IL emitted by current MS C# compiler
// == JIT'd code in .NET Core 8.0 (JIT is correct)
// != JIT'd code in .NET Framework 2.0-4.5 (JIT "misoptimizes")
// != current spec

// E.BRP = F; // byref property
[ref] var e = E;
ref var target = ref e.get_BRP();
// ^^^ null check, getter side effects
var f = F;
target = f;
// == current impl
// no current spec

// E[I1, ..., In] = F; // byref indexer
[ref] var e = E;
var i1 = I1;
// ...
var in = In;
ref var target = ref e.get_Items(i1, ..., in);
// ^^^ null check, getter side effects
var f = F;
target = f; // null check, getter effects
// == current impl
// no current spec
```

**Some Considerations.** An argument against my proposal is that array element access are closer to byref indexers, because it's possible to say `ref E[I1, ..., In]` for array type `E` of rank `n`. However, that is not true and that is an unfortunate evil due to the worst copycat feature from Java --- array variance (second next paragraph).

For the upsides, my proposal keeps the existing behavior in .NET 8.0 (I suspect it's the behavior in all .NET Core and .NET 5.0+), and the discrepancy between SZ and MD arrays in .NET Framework is not something one would want.

Arrays are really special constructs.

```
object[] array;
int index;
object item;

array[index] = item;
// not the same as
ref object target = ref array[index];
target = item;
```

because taking the address (`ldelema`) requires the static type of array to be exact (no variance allowed since once the `ref` is obtained, there's no way to control how it's used).

**Why the spec should be changed.** Due to byref properties and indexers, the meaning of assignment anyway has to be updated to take care of them. This is a good chance to clean up some historical issues.

**If the working group is unwilling to change the spec...** At least, truly document the implementation deviations. Actually, do also document the behavior of .NET Framework for multi-dimensional arrays (it's very, very surprising to me).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.