Dynamic binders fails to properly assign values to by-ref properties or indexers

[jcouv]

@VladimirReshetnikov commented on Tue Mar 14 2017

Version Used:

Microsoft Visual Studio Community 2017
Version 15.0.26228.4 D15RTWSVC
Microsoft .NET Framework
Version 4.6.01586

Steps to Reproduce:

Compile and run:

class C
{
    static void Main()
    {
        var c = new C();
        dynamic index = 1;
        c[index] = 1;
    }

    private static int x;
    public ref int this[int index] => ref x;
}

Expected Behavior: Indexer assignment is performed successfully. If it is not feasible for some reason, then the compiler should give a compile-time error in cases like this, rather than allowing program to fail at runtime.

Actual Behavior:

Unhandled Exception: Microsoft.CSharp.RuntimeBinder.RuntimeBinderException: Property or indexer 'C.this[int]' cannot be assigned to -- it is read only
   at CallSite.Target(Closure , CallSite , C , Object , Int32 )
   at System.Dynamic.UpdateDelegates.UpdateAndExecute3[T0,T1,T2,TRet](CallSite site, T0 arg0, T1 arg1, T2 arg2)
   at C.Main()

---

@gafter commented on Tue Mar 14 2017

@VSadov What is the appropriate repo for filing bugs against the dynamic runtime?

---

@VSadov commented on Wed Mar 15 2017

C# Binder lives in CoreFX.
However, this repro should probably be a compile error - we statically know it in not going to work.

---

@jcouv commented on Wed Dec 27 2017

@VSadov Can you triage relative to your other assigned issues?

---

@VSadov commented on Thu Jan 04 2018

After thinking about this, it might be possible to fix the binder/expressions to understand such assignments.

Ref assignments are tricky and will require special APIs, but regular assignments are just assignments and should be completely representable in existing trees. Most likely it only needs that factories allow ref members as assignment targets, and make ET compiler do indirect assignments.

---

@VSadov commented on Mon Jan 08 2018

We should fix this in the dynamic binder.

This should be moved to CoreFx

[jcouv]

@VSadov Please label and triage as appropriate. Thanks

[JonHanna]

Problem 1. The dynamic binder can't create a valid tree because the expression produced would have to work on a property of with the same type as (using constant expressions for simplicity):

Expression.MakeIndex(Expression.Constant(c),
		typeof(C).GetProperties()[0],
		new[] { Expression.Constant(1)})

That throws an ArgumentException with the message "Property cannot have a managed pointer type." This would have to be changed to be considered valid.

Problem 2. If it was allowed it wouldn't be considered an lvalue, so the lvalue rules would have to be changed to consider get-only properties (and for that matter, methods) which return references as lvalues.

Problem 3A. The IL generation would need changes to handle those cases.

Problem 3B. The interpreter tries to do property-gets through PropertyInfo.GetValue() which would throw NotSupportedException with a message of "ByRef return value not supported in reflection invocation." and to assign to properties with PropertyInfo.SetValue() which of course is not appropriate. For support where IL emitting isn't supported we need better support for ref types in reflection, and for the interpreter to use it (and quite likely do writebacks depending on how much said reflection does for us).

Only when S.L.Expressions can handle such expressions will it be worth changing MS.CSharp to produce them.

[VSadov]

Right. We may have to split this into two bugs -

• supporting ref-returning members as lvalues in expression trees (probably the bigger part)
• fixing dynamic binder to use that.

As for support in interpreter - that is not possible now. That would need new reflection APIs.

In general reflection APIs are not awesome for byrefs - even for parameters. Anyone who tried to call Interlocked.CompareExchange via reflection/interpreter knows that. :-/

[JonHanna]

Another variant is assigning to ref-returning methods. With:

public class C
{
	public static int val;
	public ref int AccessVal(int _) => ref val;
}

void Main()
{
	var d = new C();
	dynamic p = 2;
	d.AccessVal(p) = 2;
	Console.WriteLine(C.val);
}

The static compiler sees this as if AccessVal() must not be a ref-returning method and accordingly raises CS0131. There isn't anything else it can do though, as there isn't anything offered by the dynamic compiler for it to call into. SetMember() could feasibly be allowed to do so, without changing the signature, but that might not be the best approach — for one thing a new method would lead to more obvious failures if using a supporting C# version with the current API.

[JonHanna]

Looking into the expressions part of this raises some further issues. In particular if we don't include explicit in/ref readonly support in the API from the beginning we'd be digging ourselves into a compatibility hole if we wanted to later, so it's best there from the get-go. PTAL at dotnet/corefx#26772

[ghost]

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.

[ghost]

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.