Roslyn: Compiler permits non-void return of Deconstruct method

[gafter]

Reported on our MVP mailing list

I am just wondering, why Deconstructor methods allows returns.

    public class Employee
    {
        public string FirstName { get; }
        public string LastName { get; }

        public Employee(string firstName, string lastName)
        {
            FirstName = firstName;
            LastName = lastName;
        }

        public string Deconstruct(out string firstName, out string lastName)
        {
            firstName = FirstName;
            lastName = LastName;
            return string.Empty;
        }

        public string Deconstruct(out string firstName)
        {
            firstName = FirstName;
            return string.Empty;
        }
    }

Deconstructor invocations, 

     Employee employee = new Employee("Jaliya", "Udagedara");
     var (firstName, lastName) = employee;
     var (firstName1) = employee;

Above code compiles and runs.

Might be a foolish suggestion, shouldn't Deconstructors be void only? It doesn't sense to have returns (for me).

In fact, we intend to require that a Deconstruct method returns void. If it doesn't return void then it isn't a deconstruct method. We are considering giving meaning to a non-void return type in the future when we support more complex pattern-matching.

[gafter]

/cc @jcouv @VSadov @jaredpar

[jcouv]

My understanding was we do not care about return type (see existing test for that below).
The deconstruction is equivalent to invoking Deconstruct with a bunch of out vars, and we then do a few extra checks (for example, one with params, see binding logic).
It should be easy enough to reject based on return type, but if we plan to use that later, why block it?

```C#
[Fact]
public void DeconstructCanHaveReturnType()
{
string source = @"
class C
{
static void Main()
{
long x;
string y;

    (x, y) = new C();
    System.Console.WriteLine(x + "" "" + y);
}

public int Deconstruct(out int a, out string b)
{
    a = 1;
    b = ""hello"";
    return 42;
}

}
";

        var comp = CompileAndVerify(source, expectedOutput: "1 hello", additionalRefs: s_valueTupleRefs);
        comp.VerifyDiagnostics();
    }

```

[gafter]

It should be easy enough to reject based on return type, but if we plan to use that later, why block it?

Because when Deconstruct returns false, the deconstruction failed and the deconstructed variables should not be definitely assigned. Ignoring a bool return value today means we won't be able to make that change later.

[HaloFour]

If the compiler doesn't care about the return value then it wouldn't be possible to introduce fallible deconstruction in the future, at least in any way that the compiler could enforce reusing the same name/signature as Deconstruct. I think it would be wise to limit the return type of Deconstruct to void today until those questions are answered tomorrow.

[jcouv]

Fixed by https://github.com/dotnet/roslyn/pull/15922 and documented as a breaking change from RC.2.