Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect inferred tuple names with null coalescing operator #16825

Closed
TessenR opened this issue Jan 30, 2017 · 22 comments
Closed

Incorrect inferred tuple names with null coalescing operator #16825

TessenR opened this issue Jan 30, 2017 · 22 comments
Labels
Area-Compilers Feature - Tuples Tuples Resolution-By Design The behavior reported in the issue matches the current design

Comments

@TessenR
Copy link

TessenR commented Jan 30, 2017

In the following code sample variables 'person' and 'person2' should have same set of elements' names. I.e. they should both have '(string name, string)?' type. However, 'person2' has '(string name, string address)?' type.

class Sample
{
  void M(bool b)
  {
      var personWithAddress =  GetPersonWithPhysicalAddress();
      var personWithEmail = GetPersonWithEmail();
      var person = personWithAddress != null ? personWithAddress : personWithEmail;
      var person2 = personWithAddress ?? personWithEmail;
  }

  public (string name, string address)? GetPersonWithPhysicalAddress() => null;
  public (string name, string email)? GetPersonWithEmail() => ("Jane", "Jane@doe.com");
}

This issue makes it impossible to replace the null coalescing operator with a conditional operator as following code will become invalid:

person2.Value.address;
@gafter
Copy link
Member

gafter commented Jan 30, 2017

@jcouv @jaredpar This is a serious bug that we probably want to try to get fixed in RTM. It would be breaking to fix the bug later. However, it is possible it is already fixed.

@gafter gafter added this to the 2.0 (RTM) milestone Jan 30, 2017
@HaloFour
Copy link

It's a little concerning that C# would allow you to so easily convert between named tuples in either case. The equivalent Swift code would fail to compile.

@jcouv jcouv self-assigned this Jan 30, 2017
@jcouv
Copy link
Member

jcouv commented Jan 31, 2017

@gafter @jaredpar Confirmed this behavior. The logic for inferring type in BindNullCoalescingOperator doesn't rely on BestTypeInferrer (which is used for inferring type in array creations, conditional operators and lambdas with multiple returns).

FYI @VSadov @AlekseyTs

@jcouv
Copy link
Member

jcouv commented Jan 31, 2017

From discussion with @VSadov this is by design. I'll confirm, document and add tests.

@VSadov
Copy link
Member

VSadov commented Jan 31, 2017

This is ByDesign.

Operands of ?? coalesce operators do not have to be related in any way, as long as there is an implicit conversion one way or another. It does not even need to be a standard conversion. A user-defined conversion will do.

@VSadov
Copy link
Member

VSadov commented Jan 31, 2017

Example:

        static void Main(string[] args)
        {
            (long One, object Two)? left = null;

            // There is an implicit conversion (user defined + standard) like:
            //        C1 ===> (int Alice, int Bob) ===> (long One, object Two)
            //
            // the following is valid
            // resutl has type (long One, object Two)
            var result = left ?? new C1();
        }

        class C1
        {
            public static implicit operator (int Alice, int Bob) (C1 x)
            {
                return (1, 1);
            }
        }

@VSadov
Copy link
Member

VSadov commented Jan 31, 2017

@HaloFour Not sure about Swift. Interesting fact.

C# tuple types convert as long as corresponding element types convert.
The element names are optional compile-time annotations subject to Erasure. They have no effect on convertability.

Here: string has identity conversion to string, therefore
(string Alice, string Bob) has identity conversion to either (string, string) or (string X, string Y)

@HaloFour
Copy link

HaloFour commented Jan 31, 2017

@VSadov

I understand that the names are optional and erased, but it worries me that they are as treated so inconsequentially by the C# compiler. I foresee lots of missed opportunities in preventing logic errors.

In Swift, (name : String, address : String) is implicitly convertible to (String, String) which is then implicitly convertible to (name : String, email : String), but it is illegal to convert directly from (name : String, address : String) to (name : String, email : String). The Swift compiler assumes (probably correctly) that the developer is trying to do something wrong.

Furthermore, Swift allows implicit convertion from (name : String, address : String) to (address : String, name : String), but Swift treats that as a transposition of the tuple elements whereas C# does not. I guarantee that will be a source of confusion and error.

using System;
public class C {
    public (String name, String email) GetPerson() {
        return ("Bill", "bill@foo.com");
    }
   
    public void M() {
        (string address, string name) t2 = GetPerson();
        string s = t2.address; // so much fail
        Console.WriteLine(s); // prints "Bill"
    }
}

@jcouv
Copy link
Member

jcouv commented Jan 31, 2017

@HaloFour When we do a warning wave, one of the warnings we'd like to add would cover your scenario above (using name in the wrong position). More details in #14217

@HaloFour
Copy link

HaloFour commented Jan 31, 2017

@jcouv

A poor consolation prize. Compiler warning waves seem better suited to helping identify legal code that can be used incorrectly in unexpected ways after a feature ships, not to cover poor initial design.

Note that tuples don't even align behaviorally with named arguments, from which their syntax is derived. At least there compiler is smart enough there to know when to transpose values. That will only add to the confusion.

void M() {
    void L(string name, string email) {
        Console.WriteLine(email);
    }

    L(email: "foo", name: "bar"); // prints bar

    (string name, string email) tuple = (email: "foo", name: "bar");
    Console.WriteLine(tuple.email); // prints foo, so much fail
}

Sorry to be a bit harsh about this, but I've been harping on it since CodePlex.

@VSadov
Copy link
Member

VSadov commented Jan 31, 2017

There is nothing semantically wrong with assigning (string name, string address) to (string key, string value) . It is suspicious, but the program is not broken. - You can assign strings to strings regardless how they named.

Compiler can give warnings on "Suspicious" use of names, but it is not required by the language specification.

@jnm2
Copy link
Contributor

jnm2 commented Feb 1, 2017

@HaloFour I'm sympathetic to the desire for safety but I also really do think a warning wave is actually ideal.
The reason is that IMO the guiding principle for tuples is to make them distributive. To treat them as much as possible like a list of individual variables. Any rule that holds for two side-by-side variables (or arguments in the analogous argument list) should probably hold for tuples.

@HaloFour
Copy link

HaloFour commented Feb 1, 2017

@VSadov

You're technically correct, but it's still very likely a logic error and a perfect opportunity for the C# compiler to step in and nudge the developer to be more explicit regarding their intent. In that sense I don't see how it's any different than how the C# compiler forbids implicit switch case fall-through.

Compiler can give warnings on "Suspicious" use of names, but it is not required by the language specification.

Considering that the language specification is being amended in tandem with the language I don't see how that's relevant. The specification could just as easily be written to make the names more strictly enforced.

@jnm2

Any rule that holds for two side-by-side variables (or arguments in the analogous argument list) should probably hold for tuples.

I've just demonstrated that this is not the case. Named arguments in an argument list and named tuples behave very differently despite having identical syntax. The former will silently transpose the values, the latter will not. What a mess that will be if splatting of tuples into arguments is eventually considered.

Beyond that, I largely agree. I think tuples should be treated as a loose association of values, and the compiler should treat conversion rules and whatnot for each of the elements individually. But if the developer is going to go to the trouble of naming the elements then the compiler should go to the trouble of trying to ensure that the developer isn't making a simple mistake. And by adding it after the fact via a warning wave means that an entire generation of projects will be created that will never have that protection as doing so means upgrading the language version and opting into stricter warnings for that project.

@jnm2
Copy link
Contributor

jnm2 commented Feb 1, 2017

Named arguments in an argument list and named tuples behave very differently despite having identical syntax. The former will silently transpose the values, the latter will not. What a mess that will be if splatting of tuples into arguments is eventually considered.

Hmm, that hadn't quite sunk in but you've won me over. To the extent that they differ, tuples should be more like an argument list than a list of variables.

@VSadov
Copy link
Member

VSadov commented Feb 1, 2017

@HaloFour tuple literals have syntactical similarities with named arguments and there are some warnings when names are lost to conversions. These cases are more obvious and fixes are easy.

When expressions have types that involve tuples, the similarity ends since values can be passed around, can be cast to base types and back and so on..., without using element names at all.

            List<(int Alice, int Bob)>[] x = ...;
            List<(int Bob, int Alice)>[] y = ...;

            List<(int, int)>[] t;

            // the following is allowed
            y = t = x;

            // the following works too
            y = (List<(int Bob, int Alice)>[])(object)x;

            // but the following does not? or does it transpose something?
            y = x;

We may have better analysis and more warnings in #14217 for obvious mistakes related to the tuple names.

The structural equivalence is, however, a major design choice and thus y = x in the sample above is permitted by the language.
I.E. Compilers/analyzers/etc.. can add whatever warnings they may find relevant and user can configure them, but the spec assigns a very clear semantics to what y = x means.

@AdamSpeight2008
Copy link
Contributor

Tuple Literal Syntax is analogous to Parameter Arguments.

@gafter
Copy link
Member

gafter commented Feb 1, 2017

@HaloFour

A poor consolation prize. Compiler warning waves seem better suited to helping identify legal code that can be used incorrectly in unexpected ways after a feature ships, not to cover poor initial design.

The initial design included warnings for permuted names in tuple conversions. See #14217 (comment) and the LDM meeting notes it links to. We simply did not get started on the implementation of those warnings in time to include them in C# 7. Nevertheless, we believe our customers are better served getting the tuple feature in C# 7 with this aspect deferred rather than deferring the entire tuple feature until C# 8.

@HaloFour
Copy link

HaloFour commented Feb 1, 2017

@jnm2

I've opened #16862 regarding that specific difference.

@VSadov

           // the following is allowed
           y = t = x;

           // the following works too
           y = (List<(int Bob, int Alice)>[])(object)x;

Yes, the developer is very explicitly working to circumvent the protections.

           // but the following does not? or does it transpose something?
           y = x;

Personally I'd think that should be an error since there is no simple transposition. It's interesting in this case that Swift silently allows the conversion:

var d1 = [String : (name : String, email : String)]()
d1["foo"] = (name: "x", email: "y")

let d2 : [String : (email : String, name : String)] = s1;
d2["foo"]?.email   // prints "x"

@AdamSpeight2008

Tuple Literal Syntax is analogous to Parameter Arguments.

Except when they're not. Named parameter arguments transpose.

@gafter

Understood. But it scares me how many people might be adversely affected by accidental transmutations and will never see those warnings. And if C# 7.0 ships without transposition then that will be permanently off of the table.

@jcouv jcouv removed their assignment Feb 1, 2017
@jcouv jcouv removed this from the 2.0 (RTM) milestone Feb 1, 2017
@jcouv jcouv added Resolution-By Design The behavior reported in the issue matches the current design and removed Bug labels Feb 1, 2017
@gafter
Copy link
Member

gafter commented Mar 10, 2017

@MadsTorgersen Can you please confirm if the current behavior is as you would expect (i.e. by design)?

@MadsTorgersen
Copy link
Contributor

The design philosophy behind this aspect of tuples was/is as follows:

  1. tuple assignments and conversions are always positional
  2. element names are just "sugar on top"
  3. we will warn about common mistakes likely to be bugs, such as
    1. using names in tuple literals that don't match names in the receiving type
    2. assigning from one tuple type to another where the same name occurs in a different position

Unfortunately we missed 3.ii in the 7.0 timeframe. We would like to add it in as part of a warning wave.

You may disagree with this design, but we found the alternative, where element names are semantically part of the tuple type, to be unnecessarily restrictive, and the cause of lots of needless "translation" code just to rename things.

Agreed that the difference in semantics with named arguments is confusing. 3.ii was intended to cleanly address any mistaken code arising from that, and it is therefore a bit unfortunate that we didn't get it in.

@gafter
Copy link
Member

gafter commented Mar 13, 2017

@MadsTorgersen Can you please confirm if the current behavior described in this issue - that is, the difference in behavior between a ?? b and (a != null) ? a : b in the way tuple element names are inferred - is intentional?

@MadsTorgersen
Copy link
Contributor

This is intentional.

The spec for the null-coalescing operator is here. There is no connection between the null-coalescing and the conditional operators: one is not described in terms of the other, and there is no expectation that they are interchangeable.

Specifically, the type of a conditional expression (spec here) is determined "symmetrically" between the two branches, whereas the null-coalescing operator has a strong bias for the type of the first operand.

While the spec for C# 7.0 isn't finished yet, the natural extension of these rules leads to what you are seeing in the original example above. C# tries to find the "best common type" in the conditional expression, so where they don't agree on tuple names, it just elides those names. A null-coalescing expression, on the other hand, will try to keep the type of the left-hand operand (or the underlying type, if, as here, it is a nullable type), seeing that the right hand operand implicitly converts to it. That's what you are seeing in the behavior above.

Note that the example conceals a likely bug, since you probably didn't intend to be able to access the email element as an address element after the null coalescing operator. Had you written it using a literal as the right operand:

        var person2 = personWithAddress ?? (name: "Jane", email: "Jane@doe.com");

you would get a squiggle saying that you are renaming some elements. This illustrates the fine balance we have to make of the warnings. When it's a literal, we can be pretty certain that you meant for the element names to match, but when you get the right operand from elsewhere, we deem it too likely that you meant to rename the element, and it is too risky to give the warning. Also, it would not be as easy for the user to address the warning in that case.

@gafter gafter closed this as completed Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Tuples Tuples Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

8 participants