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

Forbid constructing tuples via "new" #10891

Closed
VSadov opened this issue Apr 26, 2016 · 17 comments · Fixed by #14161
Closed

Forbid constructing tuples via "new" #10891

VSadov opened this issue Apr 26, 2016 · 17 comments · Fixed by #14161

Comments

@VSadov
Copy link
Member

VSadov commented Apr 26, 2016

The following should work (or perhaps not):

new (int x1, int x2, ...., int x12) ( 1,2,...., 12);

It could be more complicated when constructor initializers are involved, like:
new (int x1, int x2, ...., int x12) ( 1,2,...., 12) { x1 = 1, x12 = 2};

If not, then new (int, int)(1, 2) should not work either.

@gafter
Copy link
Member

gafter commented Apr 27, 2016

I suggest that it should not work. That is, I do not suggest the object creation syntax should support tuple types. Instead we would give an error message that recommends using a tuple literal instead.

    var x = new (int x1, int x2)(3, 4);
    // error: cannot use 'new' with a tuple type. Use '(x1: 3, x2: 4)' instead

There are other contexts we might disallow tuple types (e.g. an is expression, since we cannot know if an object is of type (int x, int y) at runtime, when those names are absent, and since that syntax would conflict with pattern-matching on tuples).

@MadsTorgersen @dotnet/ldm @jcouv @AlekseyTs What do you think?

@jcouv
Copy link
Member

jcouv commented Apr 27, 2016

I see no benefit to this syntax either.

@AlekseyTs
Copy link
Contributor

I see no reason to disallow this.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Apr 27, 2016

We let people write: new int?(4)

So i somewhat agree with @AlekseyTs, i don't see why this shouldn't be disallowed.

That said, i also ascribe to the: everything starts at -100 points. This seems to have pretty darn marginal value. Indeed, given how confusing it looks, i could argue it has anti-value. So i have no problem with it being disallowed.

@gafter
Copy link
Member

gafter commented Apr 27, 2016

One reason to disallow it is that ValueTuple<int, int, int, int, int, int, int, ValueTuple<int, int>> simply does not contain a constructor that accepts an argument list (int, int, int, int, int, int, int, int, int). This is the same reason we do not allow new int(3).

@VSadov
Copy link
Member Author

VSadov commented Apr 27, 2016

Note that there is also a parameterless case -

new (int a, int b, int c)();

// same  as
default((int a, int b, int c));

It might need to be considered together with parameterized case.

@gafter
Copy link
Member

gafter commented May 12, 2016

See #347 (comment)

I think we've convinced ourselves in the C# design meeting that

new (byte, double) (1, 2);

is not going to be legal syntax. It won't be legal state a tuple type in a new expression. Instead, just use the tuple construction syntax.

(byte, double) x = (1, 2);

or

var x = ((byte)1, (double)2);

or

var x = ((byte, double)) (1, 2);

@mattwar
Copy link
Contributor

mattwar commented May 12, 2016

Now Neal is quoting me!

@gafter gafter added Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. Feature Request and removed Bug labels May 16, 2016
@VSadov VSadov added Bug and removed Feature Request Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. labels Jun 11, 2016
@VSadov
Copy link
Member Author

VSadov commented Jun 11, 2016

I have removed "Wont FIx" and made this a bug. I think "new" works currently with tuples <= 7 and not with bigger ones.
Regardless of the decision on this, the bug is actionable since we should either extend the support or disallow entirely.

@VSadov VSadov self-assigned this Jun 11, 2016
@VSadov VSadov added this to the 2.0 (Preview 1) milestone Jun 11, 2016
@mattwar
Copy link
Contributor

mattwar commented Jun 12, 2016

What is the bug exactly? That using tuple type syntax in object creation expression is not allowed?

@gafter
Copy link
Member

gafter commented Jun 12, 2016

I think the bug is that it is allowed (for sufficiently small tuples).

@VSadov
Copy link
Member Author

VSadov commented Jun 13, 2016

Semantically new (int, int)(1, 2) is valid since (int, int) type does have a constructor taking two ints.
Perhaps we should prohibit this in the parser, which could be difficult, or introduce a rule that (int, int) syntax is not allowed to mean a type in object creation expressions.

@CyrusNajmabadi
Copy link
Member

Perhaps we should prohibit this in the parser, which could be difficult

I don’t think this would be difficult TBH. We’d just need to update “ParseTypeCore” to take a Boolean saying if tuples were allowed. That would just go into ParseUnderlyingType, which would then ignore open parens.

       -- Cyrus

From: VSadov [mailto:notifications@github.com]
Sent: Monday, June 13, 2016 12:51 PM
To: dotnet/roslyn roslyn@noreply.github.com
Cc: Cyrus Najmabadi cyrusn@microsoft.com; Team mention team_mention@noreply.github.com
Subject: Re: [dotnet/roslyn] Add support for constructing tuples via "new" for 7+ elements (#10891)

Semantically new (int, int)(1, 2) is valid since (int, int) type does have a constructor taking two ints.
Perhaps we should prohibit this in the parser, which could be difficult, or introduce a rule that (int, int) syntax is not allowed to mean a type in object creation expressions.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//issues/10891#issuecomment-225688544, or mute the threadhttps://github.com/notifications/unsubscribe/AEWmY8MbzLDx8H_fm4eSBWySXKjFeubOks5qLbS1gaJpZM4IQW-Y.

@gafter gafter modified the milestones: 2.0 (Preview 1), 2.0 (RC) Jun 20, 2016
@jaredpar jaredpar modified the milestones: 2.0 (RC), 2.0 (RTM) Jul 18, 2016
@gafter gafter changed the title Add support for constructing tuples via "new" for 7+ elements Forbid constructing tuples via "new" Aug 8, 2016
@jcouv jcouv modified the milestones: 2.0 (Preview 5), 2.0 (RTM) Aug 9, 2016
@jaredpar
Copy link
Member

jaredpar commented Sep 9, 2016

Didn't we decide in a recent LDM meeting that this should be allowed?

Bug seems out of date with more recent LDM discussions, wanted to get clarity on whether it was still relevant or not.

@jaredpar jaredpar modified the milestones: 2.0 (RC), 2.0 (Preview 5) Sep 9, 2016
@VSadov
Copy link
Member Author

VSadov commented Sep 9, 2016

As far as i know it is still supposed to be disallowed.

@gafter
Copy link
Member

gafter commented Sep 9, 2016

Part of the reason we decided to disallow this is that we are considering adding target-typed construction in the future

KeyValuePair<int, int> kvp;
kvp = new (1, 2);

We need to disallow new (T1, T2) to avoid a syntactic ambiguity.

@VSadov
Copy link
Member Author

VSadov commented Sep 9, 2016

Also, allowing "new" with long tuples is a bit of work, while literals provide all the functionality needed with more concise syntax.
Basically why do extra work to just allow same thing be doable in two similar ways?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment