Runtime: Existing reflection APIs should be able to invoke ref-returning methods and return a value

[VSadov]

Related to: https://github.com/dotnet/roslyn/issues/8489

The scenario seems to be explicitly forbidden. Possibly due to ref returning methods not being common in the past.

Note that this issue does not ask for a new API that would allow returning a ref.

This is just about the existing APIs that returns the result as an object.
It seems that they should work with ref returning methods by evaluating the target, reading the value indirectly and returning the value as object.

```C#

object result1 = inst.RefReturningMethod();

// should be the same as
object result2 = inst.GetType().InvokeMember(inst, . . ., "RefReturningMethod",. . .);
```

[jkotas]

Related to https://github.com/dotnet/corefx/issues/14021

[karelz]

@jkotas is it dupe or just related?

[jkotas]

It is not dupe. This one is about fixing existing APIs, the other is about adding new APIs that work even better.

[ghost]

Any preference for what should happen if the method returns a null ref?
Here are the possibilities (ranked in descending order of my preference)

1. Return System.Reflection.Missing.Value
2. Throw NullReferenceException
3. Return null

[jkotas]

Return System.Reflection.Missing.Value

This sounds reasonable to me. Is System.Reflection.Missing.Value used as argument going to turn into a null ref?

[ghost]

Is System.Reflection.Missing.Value used as argument going to turn into a null ref?

I think we could swing that. Today, passing Missing.Value to a ref Object parameter throws ArgumentException: Missing parameter does not have a default value. And I can't see how we would support default values for ref parameters so there isn't a chance for a conflict. Or we could invent a new sentinal value if necessary.

[VSadov]

I'd prefer NullReferenceException.

Missing.Value is used for many other purposes. DLR and VB latebinders, for example use Missing.Value to propagate the fact that user did not pass a particular argument to expected optional parameters.

Returning Missing.Value in a case of a failure will lead to situations where it will be impossible to tell whether the invocation failed or succeeded and returned a Missing.Value.

Returning nullptr or passing a nullptr as a byref argument is extremely rare, and in earlybound code it is nearly always unintentional and nearly always leads to an NRE one way or another.
It feels cheaper and more consistent to just NRE in reflection as well.

[ghost]

What about the scenario where someone wants to write a new value into the referenced location? This tweak alone won't address that situation - is this an important enough scenario that we should try to address this as part of this rather than waiting for the uber TypeReference solution?

[VSadov]

It could be nice if PropertyInfo.SetValue method worked with ref-returning properties/indexers.

[VSadov]

For methods, the scenario would imply MethodInfo.SetValue, but that would be a new API.
I assume this issue is just about generalizing existing ones and new APIs will be handled separately.

[ghost]

Right, and we're principally interested in the data-binding scenario. Getting ref-return support on non-property invokes is more of a "falls out of the implementation" bonus.

[VSadov]

One thing to mention - properties/indexers may have getters that are ref readonly. - they do return by reference, but the references are "readonly" and are not supposed to be used for assignments.

SetValue should not work with those.

[VSadov]

A ref-returning property/method would have System.Runtime.CompilerServices.IsReadOnlyAttribute applied and the return type would have modreq[System.Runtime.CompilerServices.IsReadOnlyAttribute]

You can probably just key off the attribute. The modreq is for signature matching across overrides and to make sure old compilers cannot simply ignore the attribute and party on the ref that is supposed to be readonly.

[MichalStrehovsky]

Return System.Reflection.Missing.Value

I would prefer that option too. Letting it do the boxing and failing with a NullReferenceException as a result sounds more like a bug than a design choice (someone will hit it and file it).

We'll need to also define a behavior for:

• Ref-returning byref-like types (probably no choice but to throw - same as we would if the byref-like type was returned by value)
• Ref-returning pointer types (wrap in System.Reflection.Pointer, I guess)

It could be nice if PropertyInfo.SetValue method worked with ref-returning properties/indexers.

I know the C# language forbids setters on byref properties, but do we limit this in the runtime? If not and someone can actually define a setter for such property, how would they reflection invoke it if we overload the meaning of PropertyInfo.SetValue to mean "call the getter and write into the returned byref location"? How about API consistency (e.g. PropertyInfo.CanWrite would be true, I guess, but PropertyInfo.SetMethod would be null?).

[ghost]

Letting it do the boxing and failing with a NullReferenceException as a result sounds more like a bug

If that's the main concern, we could certainly throw NullReferenceException with a specific error message. Or a different exception. The inband-nature of Missing.Value sounds like it could cause actual problems according to @VSadov.

If not and someone can actually define a setter for such property, how would they reflection invoke it if we overload

I don't think they could usefully use the existing Reflection apis to invoke it anyway. The only ref you can pass as an argument is a temporary one that Reflection itself creates. For true support of ref arguments, you need the more ambitious TypeReference-based Invoke that's tracked in a different issue.

The consistency concern is indeed something to ponder. It's a tradeoff and I'm not firmly on one side or the other at this point.

[MichalStrehovsky]

The inband-nature of Missing.Value sounds like it could cause actual problems

Throwing an exception in a non-exceptional case still doesn't seem like a good API design. Even if we were to invent a new sentinel value, it would suffer from the same problem (someone can misuse a sentinel that is for reflection's private purpose to mean a different thing and then complain they can't tell them apart). I think that's kind of a user's problem, not an API problem.

The only ref you can pass as an argument is a temporary one that Reflection itself creates

I think we pass a ref to the element of the array that the user provided as an argument to Invoke. It's not so temporary.

[ghost]

DLR and VB latebinders passing through Missing.Value to propagate the fact that user did not pass a particular argument to expected optional parameters isn't exactly making it "mean a different thing." The meaning is still "opted out of passing an optional."

If we had a properly named sentinel for this (NullReference.Value), the api would be both clearer in usage and if people did the same thing and propagated that, it's not a big problem. The main impetus of using Missing.Value specifically was to avoid the overhead of an api-review, but we don't need to be constrained by that.

I think we pass a ref to the element of the array that the user provided as an argument to Invoke. It's >not so temporary.

Yes, I expect it would survive the call but it's still pretty limiting. I think if we went down that path, we'd be taking the stance that SetValue isn't meant to be the all purpose solution for ref-heavy applications.

[VSadov]

I thought about this a bit more and I firmly think that NRE is the right choice here.

1. If I do

C# object result = RefReturningMethod();
and if the method returns nullptr, I do get NRE (in both C# and VB and I assume in any other language). Regardless of the actual type of the method. - int, object or Missing -

I _am dereferencing_ when I demand a value form a ref returning method. And dereferencing null throws. I do not see why calling via reflection should behave any differently.

1. It is not uncommon to handle all the stuff in terms of objects when doing something dynamic.
   I might never need to unbox the value or unbox it far away from the call.
   In either case figuring when and what went wrong and why I have a boxed Missing.Value flowing around would be hard.

2. The case is not very different from when I give you a null array and ask for a first element or give a null instance and ask for a field. Would you find it more logical to throw something or return a Missing.Value? Would people think the exception is because of a reflection bug?

The more general TypeReference based API might be able to work with nullptr, since we would be dealing with references. Even then I am not very sure. Probably.

For the _value_ based API the behavior seems clear - the user is asking to obtain a _value_ of a ref-returning method. That implies dereferencing and possible NRE if the method returns nullptr.

[MichalStrehovsky]

MethodInfo.Invoke is not equivalent to object result = RefReturningMethod();.

MethodInfo.Invoke tries it's best to give you a usable result back. That's why you get System.Reflection.Pointer back for methods that return unmanaged pointers (instead of just throwing and telling you to go away because you can't box pointer types). It's a special value that is not what the boxed result of invocation was, but something that lets you reconstruct it.

[VSadov]

System.Reflection.Pointer is nothing similar. Unlike dereferencing nullptr, I can get values of pointer-returning methods in earlybound scenarios just fine.

System.Reflection.Pointer is totally a forced option - indeed to be able to reconstruct earlybound behavior. It is an acceptable compromise given the limitations of boxing.
If you could box pointers you would definitely just box them and return as a result, and S.R.Pointer would not need to exist.

[ghost]

Getting Pointer back is still an unambiguous result given the signature of the method. It doesn't have the same problems that returning Missing.Value has.

We already know that GetValue() isn't going to solve all the ref scenarios that can be thrown at it, so there's wriggle room to define what "best effort" is. We could simply continue to throw NotSupportedException in the case of a null ref return - this is more diagnosable than NullReference and is non-breaking.

[MichalStrehovsky]

Getting Pointer back is still an unambiguous result given the signature of the method. It doesn't have the same problems that returning Missing.Value has.

That concern would be solvable by adding a new sentinel value for this.

We could simply continue to throw NotSupportedException in the case of a null ref return

This would execute the target method (successfully) and then throw - it's a behavior that doesn't have a precedent for this API. I would want to run this through API review.

[ghost]

I've opened a new api-needs-work issue for adding NullReference.Value. (https://github.com/dotnet/corefx/issues/29060). Let's continue this subdiscussion there. We don't need to block work on the main feature on this.

[VSadov]

The earlybound case does throw after successfully invoking.

You have two operations here -

1. invoking the target (that works successfully)
2. dereferencing the pointer and fetching the value (this must throw if invocation yielded nullptr)

I do not see a problem with throwing after invocation - that is _exactly_ what should happen and that is what happens in earlybound case.

[VSadov]

I think you are not realizing the consequences of swallowing a dereference and returning a sentinel.

Consider the following case.
``C# SomeType x = GetAnInstance(); Console.WriteLine(x.RefReturning()); // <-- throws an exception if RefReturning returnsnullptr`

or more general:

```C#
SomeType x = GetAnInstance();
Use(x.RefReturning());   // <-- throws an exception if RefReturning returns `nullptr`

now user decided to use dynamic

C# dynamic x = GetAnInstance(); Console.WriteLine(x.RefReturning()); // <-- prints the sentinel Use(x.RefReturning()); // <-- does something unexpected

Do you realize to what length the dynamic binders will have to go to shield the user from the behavior change?

Would we have to wrap _ALL_ the dynamic invocations in a thunk method that would check for the sentinel and throw NRE if it so happens?

What would happen if someone just returns the sentinel? (you can't guarantee that will not happen).
How to reconcile static and dynamic behavior in that case?

CC: @jaredpar

[MichalStrehovsky]

The dereference is already a problem. MethodInfo.Invoke doesn't dereference things. If we could return a boxed byref, we would have returned that and we wouldn't even have this argument.

How is a dynamic binder going to solve this one:

        var p = new Program(); // swap "var" with "dynamic"
        p.GetX().Val = 123;
        Console.WriteLine(theXThatGetXReturnedReferenceTo.Val);

Dynamic binders (Microsoft.CSharp at least) already reject pointer types because they have properties that are not compatible with dynamic behaviors. Byrefs are the same. Dynamic binders should reject them because they can't guarantee the semantic.

[VSadov]

You are bringing a broken window argument - because there are cases where reflection has insufficient support, it is ok to break other cases that could work.

Why do you insist on making dynamic implementation harder? To whose benefit?
Do you have a bigger customer than dynamic/latebinding?

[MichalStrehovsky]

Do you have a bigger customer than dynamic/latebinding?

People who directly call MethodBase.Invoke and handle the result on their own. You're asking them to wrap their Invoke calls into a catch block because some _successful_ invocations can throw with your design. A sentinel lets them handle it without getting broken into the debugger all the time.

[VSadov]

Dereferencing null is not a successful invocation.

You are confusing a _value_ and a _reference_ to it. User requests _values_ through this API. There is no value to be given when the computed reference is null.

This is a very important distinction - User is not asking for the _reference_. That will be in the other proposed APIs.
In this API he is asking for the _value_ that the computed reference is referring to. If the computed reference is null, dereferencing should throw.

Did you ever try to get an element of a null array?
Did you ever want to get a sentinel value instead of NRE?

[MichalStrehovsky]

User is not asking for the _reference_

User is asking for the boxed result of the invocation or the next best thing if boxing is not possible. User didn't ask for the dereference; it just happens to be the next best thing. An AV is not what I would call the next best thing.

[VSadov]

That does not make sense. You are dereferencing every time when you are reading a value of a byref parameter or a byref return.

How can user be asking for a value returned via indirection and not asking for dereferencing?

[ghost]

Thanks everyone for the spirited debate.

I'm going to have to side with team NullReferenceException on this.

This particular api feature is about convenience and intuition and while the NullReferenceException approach makes the ref case a bit more "special" (*), it mirrors exactly the way that refs make early bound calls special. I don't think it would be confusing. It also has the advantage of not adding new surface area or punning existing ones.

The TypeReferenced-based api will be the one that's functionally complete and has consistent behavior for all types.

(*) It's already special by virtue of the fact that we're bundling an indirection and providing no access to the ref itself.

[ghost]

@vsadov - I think the SetValue portion of this raises enough API consistency issues and layering issues (allowing C#-isms such as "readonly ref" to get enforced by the Reflection layer.) We shouldn't pull on the trigger on that yet.

How much blockage does the inability to write cause? Can we afford to wait for the general TypeReference-based solution?

[VSadov]

Yes. ref readonly is a C# concept.

It would be ok if reflection did not respect that. I do not think it respects readonly on fields either.

[VSadov]

Ref returning methods/properties are now very poorly supported by reflection. Even if we only get read access it would be better.

Let's not gate the reading part on rationalizing the behavior of SetValue if the latter is a much thornier issue.

[ghost]

No need to gate - the reading part is already done and tucked away.