No write-back of mutable struct instances to live constant boxes

[bartdesmet]

Consider the following test:

[Theory]
[ClassData(typeof(CompilationTypes))]
public static void ValueTypeIndexAssign(bool useInterpreter)
{
    Expression index = Expression.Property(Expression.Constant(new StructWithPropertiesAndFields()), typeof(StructWithPropertiesAndFields).GetProperty("Item"), new Expression[] { Expression.Constant(1) });

    Expression<Func<bool>> func = Expression.Lambda<Func<bool>>(
        Expression.Block(
            Expression.Assign(index, Expression.Constant(123)),
            Expression.Equal(index, Expression.Constant(123))
            )
        );

    Assert.True(func.Compile(useInterpreter)());
}

which uses a lambda equivalent to:

() =>
{
    o[1] = 123;
    return o[1] == 123;
};

where o is a mutable struct. With the expression interpreter, the assert succeeds, but with the expression compiler, the assert fails.

The issue with the compiler is in the use of unbox.any upon accessing the Closure.Constants array slot containing the live constant containing the boxed instance of new StructWithPropertiesAndFields(). No write-back is ever attempted. Contrast this to:

var o = new StructWithPropertiesAndFields();
Func<bool> f = () =>
{
    o[1] = 123;
    return o[1] == 123;
};
Console.WriteLine(f());

This will print true as well. The use of an object[] combined with unbox.any instructions for accessing live constants prevents the ability to perform a mutation on the original storage location. In fact, the emitted code is:

ldarg.0    
ldfld      class [System.Linq.Expressions]System.Runtime.CompilerServices.Closure::Constants
ldc.i4.0   
ldelem.ref 
unbox.any  valuetype [Foo]StructWithPropertiesAndFields
stloc.s    V_4
ldloca.s   V_4
ldloc.0    
ldc.i4.s   123
call       instance void valuetype [Foo]StructWithPropertiesAndFields::set_Item(int32,int32)

The use of stloc and ldloca originates from the EmitAddress case for the Constant node, so we create yet another copy prior to taking the address.

With my pending changes on statically typed closures, this behavior will be corrected because the constants will be stored in a statically typed field in a closure class. Obtaining the address of the instance for use by the indexer assignment will emit ldflda.

If we were to fix this in the current code, we'd have to check for (mutable non-primitive) structs and store them in a StrongBox<T> in the Constants array slot instead, so we can avoid the use of unbox.any on the struct value (and the copying of the value to a temporary to obtain the address). Instead, we'd "unbox" the strong box from the Constants storage slot and EmitAddress for the Value field in the strong box.

[karelz]

cc: @VSadov

[VSadov]

1. Is it really an intended behavior that constants would be mutable?
   If that is the case we should emit unbox, not unbox.any

2. Would it be a breaking change now if we allow mutations in this scenario?

[ViktorHofer]

@JonHanna @bartdesmet @VSadov As 2.0 comes closer, I kindly ask if there is an update on that topic?

[JonHanna]

Is it really an intended behavior that constants would be mutable?

I'm not sure that they should be. Certainly reference types held in such nodes are mutable, but that seems to me to be akin to how a readonly field of a reference type is readable.

Of course CS1648 means we can't generally attempt to mutate a readonly field where the type is a value type, but consider:

public interface IValue
{
	int Value { get; set; }
}

public struct Mutable : IValue
{
	public int Value { get; set; }
}

public class Reference : IValue
{
	public int Value { get; set; }
}

public class HasValues<T> where T : IValue, new()
{
	private T X = new T();
	private readonly T Y = new T();
	
	public void AddAndPrint()
	{
		X.Value++;
		X.Value++;
		X.Value++;
		Y.Value++;
		Y.Value++;
		Y.Value++;
		Console.WriteLine(X.Value);
		Console.WriteLine(Y.Value);
	}
}

static void Main()
{
	new HasValues<Mutable>().AddAndPrint();
	new HasValues<Reference>().AddAndPrint();
}

Output:

3
0
3
3

X is mutated and Y is copied first, then copy mutated and and discarded, which does or does not affect the actual field depending on whether the field is a reference type (and hence the copy is aliasing) or not.

The "constant" of ConstantExpression seems to me to suggest something similar, and that we would hence expect the behaviour we get. All the more so the description of the expression type as "an expression that has a constant value".

[dotnet-policy-service]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

[dotnet-policy-service]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.