Roslyn: Confusing error when generic extension methods with `in this` and T struct not compiling.

[xoofx]

Case: Using generic extension method with in this T and where T is a struct

Version Used:
C# 7.2 (VS 2017 15.5.4)

Steps to Reproduce:

```c#
public static class TestExtension
{
public static void TestDispose(in this T thisArg) where T : struct, IDisposable
{

    }
}

**Expected Behavior**:

The extension method should compile.

**Actual Behavior**:

Error CS8338 The first parameter of an 'in' extension method 'TestDispose' must be a value type.
```

[VSadov]

This is bydesign. At least now. Anything that you do with ‘this’ here will have to be an interface call and thus would result in a copy.

The pattern would be nearly always a bad idea - worse than ordinary byval extension.

This is not a first bug report though. I wonder if we should change the error message or just allow this.

[VSadov]

Also curious if the error prevents some scenario, or just unintuitive.

Why is the expectation that this should compile?

[xoofx]

Sorry, I have changed in by ref... but not sure to understand why this is not allowed with a struct constraint?

[sharwell]

💭 I just realized that semantically this doesn't make sense unless the parameter is ref this T thisArg. Otherwise calling Dispose will not dispose the original instance.

[xoofx]

It should compile because a struct constraint will resolve to a reification of the method by the runtime, so no interface calls will be in place.

[benaadams]

But this works in 7.2?

public static class TestExtension
{
    public static void TestDispose<T>(ref this T thisArg) where T : struct, IDisposable
    {

    }
}

For in, maybe need a T : readonly struct constraint?

Also accessing fields would be ok, so maybe warning/error on calling a method/property rather than error for using in T struct?

[xoofx]

But this works in 7.2?

Hold on, it works indeed...
Ok, I don't know what I messed up, this is an invalid issue 😅

[Edit]Ok, resharper was not giving an error, while Roslyn was just compiling fine, that's why I end up here...[/Edit]

[benaadams]

"Warning calling a method on a readonly non-readonly struct will cause a copy; and the method will be called on the copy" - but with better language.

[xoofx]

Changing back the issue to in for reference. But I don't need/want in actually. I wanted ref instead but tried it on in and it didn't work, so I was assuming the same behavior for ref

[VSadov]

That is the conundrum:

This makes sense and allowed.
```C#
public static class TestExtension
{
public static void TestDispose(ref this T thisArg) where T : struct, IDisposable
{

}

}

The following is a "pit of despair" code. 

It is just subtly different from the code above, but at best it leads to suboptimal performance due to unexpected and not apparent copying, and at worst to bugs - since interface invocations are currently always done on a copy.

```C#
public static class TestExtension
{
    public static void TestDispose<T>(in this T thisArg) where T : struct, IDisposable
    {

    }
}

Compiler disallows the second variant and that still feels right.

However, I have a mental counter on how many time this triggered a bug report and it is at the state "we need to do something".

Long term we can introduce readonly struct constraint or something like that, but short term at least the error message should change to indicate that the error is given intentionally.

Something like:
" The type of in this cannot be a type parameter type. Consider removing in. "

[xoofx]

Something like: " The type of in this cannot be a type parameter type. Consider removing in. "

Maybe one day we will have methods tag-able with "readonly" and we will be able to introduce proper in this otherwise, in the meantime, I'm all for restricting this and getting a more explicit error.

[sharwell]

@VSadov Does this issue only affect generic methods? It seems like a non-generic method should allow in this T, where T is a readonly struct.

[benaadams]

Its only generic methods; non-generic methods allow both readonly structs and non-readonly structs.

It doesn't make sense for generics and non-readonly structs as the only methods you can use are ones constrained by the interface and interfaces don't have field access only properties so every use of a non-reaodnly struct will cause a copy, removing the purpose of in.

in with a non-readonly struct should probably give a warning if a method call or property access on it is performed and that it will perform a copy https://github.com/dotnet/corefxlab/pull/2097#discussion_r165849837

[halter73]

It should compile because a struct constraint will resolve to a reification of the method by the runtime, so no interface calls will be in place.

@VSadov Your comments seem to contradict this. If the struct constraint doesn't resolve to the reification of the method thereby removing the interface call and associated copy, why doesn't it?

[VSadov]

@halter73 - the problem is not in the interface dispatch.
The problem is that the methods on the interface have no way to say "I will not mutate the instance".

Compiler must assume that interface members will mutate receivers. We do pass the receiver by reference to the call, so they certainly can.
If the variable is readonly - and "in" parameter is readonly, C# compiler must make a local copy before making it a receiver of mutating call.

And such copy would have to happen every time any interface member is used on in parameter. You will end up with lots of copying.

[halter73]

That makes sense. Thanks for the explanation.

[Ultrahead]

Why is this not allowed? ...

public static void ExtensionMethod<T>(in this T thisArg) where T : struct, IComparable<T>
{ ... }

... and then it is allowed when you use ref, instead?

(about defensive copies: 1710)

[JimmyLoveSiren]

struct S : I { }
interface I { }

static class Ext
{
    static void A<T>(this in T s) where T : struct, I { } // error
    static void B<T>(this ref T s) where T : struct, I { } // ok
    static void C<T>(in T s) where T : struct, I { } // ok
    static void D(this in S s) { } // ok
}

I list up the cases.
What makes me confused is
If A is not allowed, why C is ok?
Isn't extension method just a syntax sugar?

[glenn-slayden]

@benaadams wrote:
Long term we can introduce readonly struct constraint or something like that...

Indeed allowing readonly struct constraint on generic T seems cleanest:

static void foo<T>(this in T t) where T : readonly struct { }

Is this going to happen?

The constraint approach effectively disentangles the new capability's truly beneficial uses from the murky or dubious interface considerations for which there don't appear to be any sensible scenarios.

For the purpose of better championing this feature proposal, I just wanted to point out that it feels more focused, and thus perhaps even more compelling, when presented and understood as independent of the red-herring interface interactions.

[glenn-slayden]

Here's the part where I confess to subsequently realizing that maybe all of "truly beneficial uses" I envisioned a moment ago are not so mainstream...

As currently implemented throughout my code, it turns out that all of the various C# method bodies I was thinking about for this in fact start with rude coercion of the managed pointer. Because what else can it actually do? Now to most people, the obvious necessity of doing so would be the only way to puzzle any sense out of an otherwise kooky claim. Like, "kooky" that I didn't mention such an important point.

For me, apparently I've lived so long now deep within my land of particularly unorthodox C# where ref and in reinterpretation run free like the water, that it appears that I had forgotten that in, er... "normal" C#, the body of method foo (above) would quickly discover that it can't really do much of anything with t .

So anyway, having explained that embarrassment, and although the feature now appears to be far less earth-shattering than I imagined for pretty much everyone but me, suffice it to say that a readonly struct constraint would be pretty widely useful throughout my codebase, and indeed my own initial enthusiasm for the feature remains unchanged.

[IllidanS4]

I do not think an error nor a warning is justifiable in this case. Generic in this is just as valid as a regular generic in parameter. Even though you might not want to call methods on the value directly, why should that stop you from declaring the method at all?

After all, you can do this without any issue:

private delegate void TestInDelegate<T>(in T val);
private delegate void TestRefDelegate<T>(ref T val);

public static void Test1<T>(in T val) where T : struct
{
    var del = (TestInDelegate<T>)Delegate.CreateDelegate(typeof(TestInDelegate<T>), ((TestRefDelegate<T>)Test2<T>).Method);
    del.Invoke(in val);
}

public static void Test2<T>(ref T val) where T : struct
{
    val = default;
}

Calling Test1 on a readonly value will happily reset it, and the same would be done had the parameter been in this T val. I know this abuses the runtime's ignorance (anyway out and in should have been modreq from the beginning in my humble opinion), but the point is that even if you cannot do much with the in reference yourself, you can still pass it to another method that might, be it a smart dynamically generated method, a "const-casted" method like in my example, or an external method from an assembly compiled from a language that knows better how to deal with readonly types and values than C#.

So, there are 4 things that need to be done:

1) in this T should be allowed. It works just as well (or just as bad) as in T and so the same rules should apply to it. The limitations of ref this T (value type or generic parameter constrained to a value type) are just as applicable.

2) Calling interface methods on any in T parameter should produce a warning that a defensive copy is about to be produced. If the point of in is to prevent producing copies, then the programmer should be warned in the case when a copy is about to be produced by every instance call rather than just once when the whole method is called. At least until there are readonly interface methods available.

3) readonly struct generic constraint, as suggested. Even though (in my opinion), readonly type attribute should have been accompanied by readonly for methods (and properties) from the beginning, it is the least that can be done to make efficient value-type generics viable.

4) Disambiguate between this T and ref this T when calling an extension method. At the moment, when both overloads are present, it is an ambiguous call, but there is no way to select the specific overload. The ref one should be chosen if possible, and the normal one in all the other cases.

[glenn-slayden]

What @IllidanS4 said.

Agree 100% and a million upvotes.

[gerhard17]

To bad I'm facing the same problem in my code base.
I strongly vote for this feature!

I have implemented an arbitrary precision floating point library.
The various floats are defined by a struct of desired size which only must implement the empty interface IFpFloatReadonlyStruct. (No struct methods are implemented directly.)
All float structs share the same generic library code.
The core functioniality is implemented in extension methods based on GetStructSizeInSlots() and FractionAt(). These two base methods are using managed pointers similar to the System.Runtime.CompilerServcies.Unsafe class.

The JIT compiler really does an incredible good job with function inlining and generic code expansion.

At the moment every 'in' argument passing is done by using 'ref'.
But using 'ref' instead of 'in' has following drawbacks:

1. the per design readonly input arguments can be assigned and therefore accidentally changed
2. cannot be used with ref readonly 'constants'

Unintended struct copy does not (and would not) occure during this usage.
But any compiler warning - when doing so - would be strongly wellcomed!

BR Gerhard

/* simplified example code */

public interface IFpFloatReadonlyStruct { }


/// <summary>
/// A 256 bytes FpFloat Struct
/// </summary>
public readonly struct FpFloatStruct256 : IFpFloatReadonlyStruct
{
    // The Total Memory Block
#pragma warning disable 0169
    private readonly Mem256Struct _memory;
#pragma warning restore 0169
}


/// <summary>
/// FpFloat Extension Methods
/// </summary>
public static partial class FpFloatStruct
{
    /// <summary>
    /// Gets the float's struct size in count of <see cref="UInt32"/> slots.
    /// </summary>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static int GetStructSizeInSlots<TFloat>()
        where TFloat : struct, IFpFloatReadonlyStruct {
        return UnsafeX.SizeOf<TFloat>() >> 2;
    }

    /// <summary>
    /// Gets the fraction bits (slot) at the given index position.
    /// </summary>
    /// <param name="index">The index of the fraction bits (slot).</param>
    /// <returns>The fraction bits (slot) at the given index position.</returns>
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static ref readonly uint FractionAt<TFloat>(this ref TFloat number, int index)
        where TFloat : struct, IFpFloatReadonlyStruct {
#if DEBUG
        if ((uint)index >= (uint)GetStructSizeInSlots<TFloat>() - 1) {
            throw new IndexOutOfRangeException($"index = {index}");
        }
#endif
        return ref UnsafeX.Add(ref UnsafeX.As<TFloat, uint>(ref number), index + 1);
    }
}


public static partial class FpMath
{
    /// <summary>
    /// Demo: Sum up all fraction slots.
    /// </summary>
    public static ulong BuildDemoSum<TArg>(ref TArg arg)
        where TArg : struct, IFpFloatReadonlyStruct {
        ulong sum = 0;
        var n = FpFloatStruct.GetStructSizeInSlots<TArg>();
        for (int i = 0; i < n; i++) {
            sum += arg.FractionAt(i);
        }
        return sum;
    }
}

[CyrusNajmabadi]

Note: as far as i can tell, the Roslyn compiler is abiding by the current language spec properly. If you'd like a change in behavior here, a proposal will need to be filed over at dotnet/csharplang.

This issue only tracks improving the confusing diagnostic message that is currently being reported.