Unexpected System.ArrayTypeMismatchException at execution time

[AlekseyTs]

Compile and run:

class Program
{
    static void Main()
    {
        Test1((Base[])new[] {new Derived(), new Derived()});
        System.Console.WriteLine(""D2"");
    }
    
    static void Test1<T>(T[] x) where T : I1
    {
        x[0].P+=2;
    }
    
    static int GetX()=> 0;
    static int GetY()=> 0;
}

interface I1 
{
    public int P {get; set;}
}

class Base : I1
{
    public int P {get; set;}
}

class Derived : Base {}

Observed:

System.ArrayTypeMismatchException : Attempted to access an element as a type incompatible with the array.

  Stack Trace: 
    Program.Test1[T](T[] x) in :line 12
    Program.Main() in :line 6
    RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
    MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Other scenarios where compiler captures references to array elements, which could be reference types, could be affected as well.

[jjonescz]

@AlekseyTs I see you have tests for this - are you working on the fix as well or should I be looking into it since I was assigned?

[AlekseyTs]

I am not working on the fix at the moment.

[jjonescz]

The compiler generates a reference access to x[0] of type T in Test1 (IL: ldelema !!T). That can't work when T is Base and x is Derived[]. I think we can instead

• store the value of x[0] in a temp if we know T is a class;
• take a reference to x[0] as we do today but only if we know T is a struct;
• otherwise check at runtime whether T is a struct or not (if (default(T) is null)) and do either of the above.

[AlekseyTs]

I think there is another option, which is implemented by UseTwiceArrayAccess in src\Compilers\VisualBasic\Portable\Lowering\UseTwiceRewriter.vb.

[jjonescz]

There's another option I'm investigating - emitting readonly. before ldelema, otherwise no IL changes are necessary. The only downside is that it's not verifiable code anymore. Not sure if that's a good tradeoff.

[jjonescz]

Investigation notes:

• Currently codegen takes a reference to the array element and stores it in a local via ldelema T which throws ArrayTypeMismatchException at runtime for variant arrays.
• Easiest fix is to emit readonly. before ldelema T which skips the array type check at runtime. But if the reference is stored in a local (which it is with current codegen), the code becomes unverifiable which is undesirable for existing scenarios like this. (Note: if readonly.ldelema T is only ever on stack, it seems verifiable.)
  • This could be implemented by setting a flag on bound assignment nodes during lowering and using that flag in emit.
• Problematic scenarios are where StoreToTemp is called with RefKind.Ref on a receiver. Scenarios in CodeGenCallTests.cs should cover that, but should be extended - each scenario should have these alternatives:
  • Where receiver is array element access.
  • Async / not async. Note that async might already work since it spills into fields.
  • Generic (T[]) / non-generic.
  • Complex scenarios (that's where ReferToTempIfReferenceTypeReceiver helper is called) / simplistic scenarios (where the helper isn't called).
• VB seems to handle this correctly, can be used for inspiration. There are equivalent tests in CodeGenCallTests.vb.
• Fix should likely use IsInvariantArray check.
• Async/spill rewriters might need updating if bound tree shape that we produce from local rewriter is different. See also Ensure proper receiver value is used for a constrained call invocation #65642 - changes there in the SpillSequenceSpiller might need updating.