Deconstruct extension methods that use "in" and "ref" should work

[Kentalot]

I'm using C# 7.2 and version 15.5.3 of VS 2017 Enterprise.

Unless I'm doing something wrong, it looks like deconstruction into valuetuples using extension methods along with the in keyword for readonly pass in by reference prevents the unpacking syntax. See below.

Steps to Reproduce:

The following compiles fine:

        public static void Deconstruct(this ushort source, out byte first, out byte second)
        {
            first = (byte)(source >> 8);
            second = (byte)(source & byte.MaxValue);
        }

        public static void TestDeconstruct()
        {
            ushort source = 0;
            (var first, var second) = source;
        }

The following does not compile (using the in keyword for the source ushort):

        public static void Deconstruct(in this ushort source, out byte first, out byte second)
        {
            first = (byte)(source >> 8);
            second = (byte)(source & byte.MaxValue);
        }

        public static void TestDeconstruct()
        {
            ushort source = 0;
            (var first, var second) = source;
        }

Expected Behavior: Both should compile fine and the second example that currently does not compile should also pass in the source ushort by reference.

[jcouv]

@Kentalot What error are you getting?

I tried this and got not error.

I also verified in recent VS (issue may have been fixed in 15.6).

Version 15.6.0 Preview 3.0 [27318.1.d15.6]
VisualStudio.15.IntPreview/15.6.0-pre.3.0+27318.1.d15.6

[Kentalot]

I get No Deconstruct method with 2 out parameters found for type 'ushort'. I'll try it out with 15.6 and close this if it's fixed later on. Thanks!

[jcouv]

Note to self: even if we can confirm this was fixed in 15.6, I need to add some tests to cover this and the variant scenario with ref (currently produces an error, even though field.Deconstruct(...) works). Both of those need to be confirmed with LDM.

[Kentalot]

Actually, ref doesn't work with my version of VS and I actually think that's a good thing? If you use ref with an extension method and can call it w/o using the keyword ref, that's pretty dangerous, isn't it? The in keyword would not be dangerous since nothing is changing. I guess we currently have the same problem with mutable reference types being able to be used in extension methods, but value types has some implicit safety built into our collective psyche or however you call it. Maybe I just need to get used to it 😃

[jcouv]

I agree that in is sensible here, whereas ref seems dangerous, so I'm happy with the current behavior. But I need to confirm that with LDM, as this behavior is an accident of implementation.

Also, it disagrees with the spec:

The current deconstruction speclet says that "The resolution is equivalent to typing rhs.Deconstruct(out var x1, out var x2, ...);". But field.Deconstruct(out var x1, out var x2) works with a void Deconstruct(ref this ...) method, and yet the deconstruction produces an error.

[jcouv]

Got confirmation from LDM. in case should remain allowed and ref case should remain disallowed. PR https://github.com/dotnet/roslyn/pull/24396 (currently in review) adds tests and a specific error message for ref case.

[Kentalot]

Awesome. Sounds great. Thanks!

[jcouv]

After further discussion with LDM, we decided we'll allow the ref case as well. It doesn't seem useful, but blocking it seems irregular. I'll update my PR accordingly.

Note to self: in ref case, it must be called on a mutable L-value.