C# 7.3 ArrayTypeMismatchException in ref initialization/assignment

[KalleOlaviNiemitalo]

Describe the bug

In C# 7.3, ref initialization (§13.6.2) or assignment (§12.21.3) should do a run-time check for array covariance and possibly throw System.ArrayTypeMismatchException, but the standard omits this check. The behaviour should depend on ref vs. ref readonly, and this distinction should propagate through the conditional operator (§12.18).

Example

SharpLab

class C1 {}
class C2 : C1 {}

static class Program {
    // Fields rather than constants, to hinder compile-time optimization.
    static bool yes = true;
    static bool no = false;

    // The hint parameter is to make the IL disassembly
    // easier to correlate with the source code.
    static void UseRef(string hint, ref C1 r) {}
    static void UseIn(string hint, in C1 r) {}

    static void Main() {
        C1[] exact = new C1[1];
        C1[] covariant = new C2[1];
        
        // §13.6.2 Local variable declarations
        ref C1 mutableRef1 = ref covariant[0]; // ArrayTypeMismatchException, but the C# 7.x draft omits this check.
        ref readonly C1 readonlyRef1 = ref covariant[0]; // No run-time check
        
        // §12.21.3 Ref assignment
        mutableRef1 = ref covariant[0]; // ArrayTypeMismatchException, but the C# 7.x draft omits this check.
        readonlyRef1 = ref covariant[0]; // No run-time check
        
        // §12.6.2.3 Run-time evaluation of argument lists
        UseRef("ref [0]", ref covariant[0]); // ArrayTypeMismatchException
        UseIn("in [0]", in covariant[0]); // No run-time check
        UseRef("ref mut", ref mutableRef1); // No run-time check, but the C# 7.x draft requires this check.  The check would pass.
        UseIn("in mut", in mutableRef1); // No run-time check
        UseIn("in ro", in readonlyRef1); // No run-time check

        // §12.18 Conditional operator
        ref C1 mutableRef2 = ref yes ? ref covariant[0] : ref exact[0]; // Run-time check on covariant[0] only, ArrayTypeMismatchException, but the C# 7.x draft omits this check.
        ref C1 mutableRef3 = ref no ? ref covariant[0] : ref exact[0]; // Run-time check on exact[0] only, no exception, but the C# 7.x draft omits this check.
        ref readonly C1 readonlyRef2 = ref yes ? ref covariant[0] : ref exact[0]; // No run-time check
        ref readonly C1 readonlyRef3 = ref no ? ref covariant[0] : ref exact[0]; // No run-time check
        
        // §12.6.2.3 and §12.18 interaction
        UseRef("ref yes?:", ref yes ? ref covariant[0] : ref exact[0]); // Run-time check on covariant[0] only, ArrayTypeMismatchException
        UseRef("ref no?:", ref no ? ref covariant[0] : ref exact[0]); // Run-time check on exact[0] only, no exception
        UseIn("in yes?:", in yes ? ref covariant[0] : ref exact[0]); // No run-time check
        UseIn("in no?:", in no ? ref covariant[0] : ref exact[0]); // No run-time check
    }
}

Expected behavior

Not sure how you want to model this. Currently, the C# 7.x draft requires a run-time check for array elements even when the variable reference comes from a reference variable (or ref-returning method, property, indexer, or local function) and the run-time type has thus been checked already (unless it came from non-C# code or a C# 11 unsafe pointer cast dotnet/csharplang#6476). The run-time check would be inefficient to implement and would always pass, and Roslyn omits the check. However, standardizing how it actually works could make at least §12.18 (Conditional operator) more complex.

§13.6.2 (Local variable declarations) should specify that, when a ref (but not ref readonly) variable is initialized with an array element of reference type, then a run-time check is done and ArrayTypeMismatchException can be thrown.

§12.21.3 (Ref assignment) should specify that, when a ref (but not ref readonly) variable is assigned, and the right operand is an array element of reference type, then a run-time check is done and ArrayTypeMismatchException can be thrown.

§12.18 (Conditional operator) should perhaps specify that, if the expression is used in a context that requires a mutable reference (ref, rather than ref readonly or in), and the x or y operand selected by the condition value is an array element of reference type, then a run-time check is done and ArrayTypeMismatchException can be thrown. Alternatively, this could specify that the element type is not checked here but the value of the expression is "a variable reference with covariance risk", for which a run-time check would then be required in §13.6.2, §12.21.2, §12.21.3, and §12.6.2.3.

§12.6.2.3 (Run-time evaluation of argument lists) and §12.21.2 (Simple assignment) should perhaps specify that the run-time check is done only when the expression is syntactically an array element, rather than a reference that can be assumed to have been checked already.

No change to §17.6 (Array covariance).

§21.5 (Common exception classes) should mention references and preferably have a note referencing §12.6.2.3, §12.18, §12.21.3, and §13.6.2 for the details.

Additional context

Noted in #837 (comment).

[KalleOlaviNiemitalo]

Other target-typed stuff: C# 7.1 default #67, C# 9.0 new dotnet/csharplang#100, C# 9.0 conditional expression dotnet/csharplang#2460, future name lookup dotnet/csharplang#2926. This makes me think propagating the readonlyness of the target ref type in the conditional ref expression would be better than introducing "variable reference with covariance risk" or keeping the unnecessary element-type checks.

[KalleOlaviNiemitalo]

When the readonlyness does not match:

class C {
    int i1 = 1;
    readonly int i2 = 2;
    ref int R(bool flag) => ref flag ? ref i1 : ref i2;
}

.NET SDK 6.0.410 reports error CS8160 and error CS8156 at the expression flag ? ref i1 : ref i2, rather than only at the subexpression i2. Which seems to mean it did not propagate the non-readonlyness of the target ref int R(bool flag) to each branch of the conditional expression and validate them one by one. I suppose the standard need not go to such detail, though.

[jskeet]

Thanks for catching this. I suspect we'll need to discuss this in the meeting - I'm sufficiently confused by it as to not be able to propose anything else.

[BillWagner]

Tagging @jaredpar

I can see a few ways to resolve this:

1. Make the proposed changes to §13.6.2 and §12.21.3 only. Then, point out that any other reference variable that is an element of a reference array, assignment may throw an ArrayTypeMismatchException.
2. Add a paragraph to §17.6 (Array covariance) stating that any operation involving a reference variable array may throw an ArrayTypeMismatchException as part of a ref assignment.
3. Make a statement that once you assign a reference variable to an element of an array of a reference type and an implicit conversion is required, the behavior is undefined.

The simplest fix would be (3). Although, I would hope we can define the behavior better.

(Edited based on the following comment)

[KalleOlaviNiemitalo]

What do you mean with "arrays of reference variables to reference types"? As a reference variable to a reference type is like

string s = "huh";
ref string r = ref s; // r is a reference variable

How would you even declare an array of those?

[Nigel-Ecma]

Clause §13.6.2 (at least) is suffering badly from features, e.g. var & ref, being introduced over time and needs a rewrite. For example it currently states that it is a compile time error for ref var declarations to have no initializer, but it only implies the same for inferred type (var) declarations. It is also unclear on how the type in the declaration and the type of an initializer are “reconciled”, except in the case of inferred type declarations, and this issue regarding array type mismatch is related to this particular issue.

Trying to fix this array type mismatch without first addressing the existing structural issues is not the way to go here.

(I’ll look into suitable changes but won’t assign this to myself at this time.)

[jskeet]

Meeting 2023-09-06: we recognize that this needs fixing, but don't want to actually make the change before the C# 7 spec is cut - it's the sort of change that we want to be done early in a release cycle.

Assigning to Nigel to look at this, but also changing the milestone C# 8.