Warnings missing on tuple name rearrangement

[gafter]

The following program is supposed to give a warning about shuffled tuple names, but we report nothing.

class Program
{
    static (int x, int y) f = (1, 2);

    static void Main()
    {
        (int y, int x) z = f; // warning: tuple conversion misplaces member 'x' from Item1 to Item2.
    }
}

[gafter]

@jcouv @VSadov @jaredpar I want to call this to your attention, as it appears to be a significant work item not yet started. It appears we have special-cased certain contexts (e.g. overriding), but not implemented the general case (identity conversion).

This is specified at the top of #10429 (The April 6 design notes).

Element names are immaterial to tuple conversions. Tuples with the same types in the same order are identity convertible to each other, regardless of the names.

That said, if you have an element name at one position on one side of a conversion, and the same name at another position on the other side, you almost certainly have bug in your code:

(string first, string last) GetNames() { ... }
(string last, string first) names = GetNames(); // Oops!

To catch this glaring case, we'll have a warning. In the unlikely case that you meant to do this, you can easily silence it e.g. by assigning through a tuple without names at all.

There is a similar work item for VB: see #11370, which says

Same warnings for name mix-ups, case-insensitive name comparisons (of course).

[gafter]

@VSadov pointed out it would be silly to have this warning for identity conversions but not for implicit tuple conversions, such as from (int a, int b) to (long b, long a).

[gafter]

Tuple name rearrangement warnings

Tuple name rearrangement warnings were called out by the LDM; see the top of #10429 (The April 6 design notes):

Element names are immaterial to tuple conversions. Tuples with the same types in the same order are identity convertible to each other, regardless of the names.

That said, if you have an element name at one position on one side of a conversion, and the same name at another position on the other side, you almost certainly have bug in your code:

(string first, string last) GetNames() { ... }
(string last, string first) names = GetNames(); // Oops!

To catch this glaring case, we'll have a warning. In the unlikely case that you meant to do this, you can easily silence it e.g. by assigning through a tuple without names at all.

We may have a warning on tuple name rearrangement when the following kinds of implicit conversions are used

Implicit Tuple Literal Conversion

This one may not need a separate warning, as there is already a warning for this situation due to the dropped name. However, warning about rearragement instead in this case is probably more valuable to the user than the existing warning about dropped names.

Identity Conversion

This is the one called out by the original LDM requirement in the context of a variable initialization.

(string first, string last) GetNames() { ... }
(string last, string first) names = GetNames(); // Oops!

Presumably it should apply to an assignment expression as well.

(string first, string last) GetNames() { ... }
(string last, string first) names;
names = GetNames(); // Oops!

Implicit Tuple Conversion

The implicit tuple (elementwise) conversion was added subsequently, and we agree that if we have a warning for the identity conversion case, the warning should also be applied to the implicit tuple conversion case.

(string first, string last) GetNames() { ... }
(object last, object first) names = GetNames(); // Oops!

Contexts in which a tuple name rearrangement warning might be due

It seems unlikely that the LDM intended the requirement for the rearrangement warning to be produced only on the variable initialization and not, for example, on the assignment expression. There are many other contexts in which conversions are used to define the semantics of the language, and for which diagnostics would be helpful.

Local variable initialization

This is where, by example, the LDM explicitly acknowledged the utility of the warning. We imagine that field and property initialization would follow.

Assignment (including ref)

Analogous to variable initialization.

Object initializer

In a context such as

new C() { X = Y }

where the position of names in X and Y differ. It should similarly apply to other forms of object initializers.

Overriding, Implementing, and Hiding (OHI already requires a strict match)

In the places where types must match for OHI purposes, we already have custom rules. Generally, names must match exactly.

We need to verify the required behavior for explicit hiding (new). Are there diagnostics in that case?

Argument conversion (including ref, out)

For example

(string first, string last) GetNames() { ... }
void UseNames((string last, string first) names) { ... }
UseNames(GetNames());

Proposed: yes, the warning applies to this context. This applies to operator invocation, delegate invocation, indexer invocation, etc. It applied even if the argument is passed by ref or out. Is applies to normal or expanded form. We should report it in an attribute argument as well.

Extension method receiver

For example

(string first, string last) GetNames() { ... }
static void UseNames(this (string last, string first) names) { ... }
GetNames().UseNames();

Proposed: yes.

Foreach

For example

List<(string first, string last)> list = ...;
foreach ((string last, string first) item in list) ... // context 1
foreach ((string last, string first) in list) ... // context 2
foreach (var (last, first) in list) ... // context 3

This question can be combined with Deconstruct.

class C
{
    public void Deconstruct(out string first, out string last) { ... }
}
List<C> list = ...;
foreach ((string last, string first) in list) ... // context 4
foreach (var (last, first) in list) ... // context 5

Since the foreach loop is currently specified to use an explicit conversion, one would imagine that no warning would be required. However,

1. Such a warning would be extremely useful to the programmer, as it highlights a likely bug
2. Although an explicit conversion is required by the language, in contexts where the conversion is determined to be an implicit conversion, it would make sense from the language point of view for the warning to be produced.

Based on this approach, contexts 1 could be given a diagnostic. Contexts 2, 3, 4, and 5 are deconstruction contexts, and if we want a warning for these situations (LDM should chime in here) it would have to be specified separately. I propose that yes, we want such a warning for deconstruction declarations as well.

Deconstruction

As described in the discussion of foreach, the following would be contexts in which the target type does not have conflicting names, but it declares conflicting names:

(string first, string last) GetNames() { ... }
(string last, string first) = GetNames(); // context 1
var (last, first) = GetNames(); // context 2

A similar situation arises with Deconstruct.

class C
{
    public void Deconstruct(out string first, out string last) { ... }
}
C GetNames() { ... }
(string last, string first) = GetNames(); // context 3
var (last, first) = GetNames(); // context 4

A similar question would arise for deconstruction into existing (rather than newly declared) variables.

We need to decide for each of these contexts whether or not a warning is intended.

Return, Yield Return

(string first, string last) GetNames()
{
    (string last, string first) v = ...;
    return v; // warning?
}

Under the principle that the use of an identity or implicit tuple conversion should be capable of producing a name rearrangement warning, this context should. This applies to block-bodied lambdas, methods, local functions, operator declarations, etc. It should apply appropriately to async methods.

Conditional, Coalesce (best common type)

(string first, string last) x1 = ...;
(string last, string first) x2 = ...;
bool b = ...;
var x3 = b ? x1 : x2;

In this case, due to newly introduced type inference rules, the type of x3 is (string, string) and no problematic conversion is applied. So propose no warning for this case. Similarly, in the case of

(string first, string last)? x1 = ...;
(string last, string first) x2 = ...;

var x3 = x1 ?? x2;

We infer the type (string string) for x3. Propose no warning for this case.

Lambda Conversion e.g. from ((string first, string last) x)=>{} to Action<(string last, string first)>

    Action<(string last, string first)> a = ((string first, string last) x) => {};

Propose yes, warning for this case. On the other hand, I do not believe that arguments being declared to be of tuple type are likely to occur much in practice.

Type Arguments in Identity Conversion

class C<T> {}
C<(string first, string last)> x = ...;
C<(string last, string first)> y = x; // warning?

Proposed: yes

Type Arguments in Reference Conversion

interface I<T> {}
class C<T> : I<T> {}
C<(string first, string last)> x = ...;
I<(string last, string first)> y = x; // warning?

Proposed: yes

Variance Conversion e.g. from Action(I<(f, l)>) to Action(C<l, f)>) where C:I

interface I<T> {}
class C<T> : I<T> {}
interface A<in T> {}
A<I<(string first, string last)>> x = ...;
A<C<(string last, string first)>> y = x; // warning?

Propose yes, warning for this case.

from (string first, string last) in e select e.first

The code

var r = from (string first, string last) xin e select x.first;

is, by specification, translated into

var r = e.Cast<(string first, string last)>().Select(x => x.first);

where Cast is an invocation of

public static IEnumerable<TResult> Cast<TResult>(this IEnumerable source);

In other words, the language does not provide a reasonable place for this user error to be caught, just as this code is accepted today:

string[] e = null;
var r = from int x in e select x + 1;

So propose no diagnostic for this situation.

Cast

What about an explicit cast?

(string first, string last) x = ...;
(string last, string first) y = ((string last, string first))x; // warning?

On the one hand, this cast does make use of the problematic conversion, so one could reasonably argue that the warning should be required. On the other hand, the user may have intended to circumvent the warning by the use of the cast.

Propose that the warning be required here. To suppress the warning, cast to (string string) instead.

Array intitializer

The elements of an array initializer must be implicitly convertible to the type of the array. The warning on name rearrangement should apply in this case.

Array type inference (best common type)

(string first, string last) x = ...;
(string last, string first) y = ...;
var a = new[] { x, y }; // what is the type of a?

Presumably we'd use the same solution as for conditional and coalesce, and infer nameless types; the type of a is (string, string)[].

Identity of types

Consider

class C<T>
{
    public static T Value;
}

C<(string first, string last)>.Value.first = "first";
Console.WriteLine(C<(string last, string first)>.Value.last); // prints "first"

I don't think there is anything we can do about this.

Type parameter constraints

Consider

T M<T, U>(U u) where T : U
{
    return u;
}
(string first, string last) x = ...;
(string last, string first) y = M<(string last, string first), (string first, string last)>(x);

We can detect this problem when verifying that the (inferred or explicit) type arguments satisfy the type parameter's constraints. I propose that we do so.

Dynamic type test

The dynamic type test is operator has no mechanism to test for a tuple type with names. Similarly, the dynamic type never has names. Therefore there is no need to consider this warning in that context.

Partial Methods

Consider

partial void M((string first, string last) x) {}
partial void M((string last, string first) y);

Proposed: warning required.

Constraints on types of operators

The spec says

10.10.1 Unary operators
The following rules apply to unary operator declarations, where T denotes the instance type of the class or struct that contains the operator declaration:
• A unary +, -, !, or ~ operator must take a single parameter of type T or T? and can return any type.
• A unary ++ or -- operator must take a single parameter of type T or T? and must return that same type or a type derived from it.

(Emphasis mine)

As this requirement for the type to be the same could only make a difference inside the declaration of ValueTuple, I suggest we implement it or not, whichever is most convenient.

Arrays

A conversion from an array of type (string first, string last)[] to an array of type (string last, string first)[].

Similarly, a conversion from an array of type List<(string first, string last)>[] to an array of type List<(string last, string first)>[].

Generic type inference

(string first, string last) x = ...;
(string last, string first) y = ...;
T M<T>(T x, T y) => ...;
var z = M(x, y);

In this case the type of z should be (string, string), which is the inferred type of the type parameter. No warning is required.

Expression lambda return value

(string first, string last) x = ...;
Func<(string last, string first)> y = () => x 

Proposed: yes, warning required.

Compound Assignment

7.17.2 Compound assignment
An operation of the form x op= y is processed by applying binary operator overload resolution (§7.3.4) as if the operation was written (x) op y. Let R be the return type of the selected operator, and T the type of x. Then,
• If an implicit conversion from an expression of type R to the type T exists, the operation is evaluated as x = (T)((x) op y), except that x is evaluated only once.
• Otherwise, if the selected operator is a predefined operator, if R is explicitly convertible to T, and if y is implicitly convertible to T or the operator is a shift operator, then the operation is evaluated as x = (T)((x) op y), except that x is evaluated only once.

My emphasis.

If we use this rule in analyzing an operator, we should produce a warning if appropriate.

Default argument

void M((string first, string last) name = default((string last, string first))) ...

We should produce a warning here.

Method Group Conversion

void M((string first, string last) x) {}
Action<(string last, string first)> a = M;

We should produce a warning here.

[gafter]

@jcouv @VSadov @AlekseyTs We stood up today and enumerated a few of the contexts in which the tuple name rearrangement warning might arise in the language. I made a quick pass through the language spec to fill in a few more that I think we missed, and recorded them above. This list also includes places where I do not recommend we add a warning, but I put the places on the list for completeness.

[jcouv]

Per today's LDM, we won't be able to fit this change into C# 7. We'll consider for a warning wave.

[gafter]

Warning waves are described in #1580.

[gafter]

@jcouv I don't understand why this was moved to the 3.0 milestone. The sense I got from the LDM (e.g. #16482 and #16674) is that they would have preferred it in C# 7.0 (milestone 2.0), but were forced to wait due to schedule overflow until milestone 2.1 along with warning waves. What changed?

[jcouv]

I thought of an analogous situation with out vars:

void M(out bool first, out bool second);
M(out var second, out var first);