dotnet / csharplang

The official repo for the design of the C# programming language
11.1k stars 1.01k forks source link

Proposal: Permit trailing commas in method signatures and invocations #391

Open MgSam opened 7 years ago

MgSam commented 7 years ago

Migrated from https://github.com/dotnet/roslyn/issues/12496

Proposal

Allow trailing commas in method/constructor declarations and invocations.

void Foo(
    int bar, 
    int baz,
) { }

Foo(
    bar: 5,
    baz: 3,
);

Rational

One very small but appreciated feature in C# is the tolerance of the compiler for trailing commas in array initialization or object initialization:

var a = new int[] 
{ 
    3, 
    4, 
    5, 
};
var b = new 
{ 
    Bark = "woof", 
    Bite = "chomp", 
}

This makes it easier to reorder or edit the item order without taking time to make sure that last comma gets deleted.

It would be nice if this feature also extended to method signatures and invocation. It would be especially useful in the post C# 4.0 where named parameters can be reordered or used to make a highly descriptive method call.

Example

void Foo (int bar, String baz, Func<int> cat, Action<String> dot, int elf) { }

Foo(
    bar: 5, 
    baz: "",
    cat: () => 6,
);

Wait, having cat last is confusing, let me place it after bar...

//Rather than just copy and paste the relevant line...
Foo(
    bar: 5, 
    cat: () => 6, //Now I need to add this comma here!
    baz: "" //And I need to remove this comma here!
);

Instead, if method invocations were also tolerant of an extra comma, it would make editing easier:

Foo(
    bar: 5, 
    cat: () => 6, 
    baz: "", //Extra comma here, it's not hurting anyone!
);

Final thoughts

TypeScript has and soon JavaScript will have this feature. I think it will work great in C# as well.

DavidArno commented 7 years ago
    baz: "", //Extra comma here, it's not hurting anyone!

It's hurting readability, simply to make the lives of lazy developers easier. It's bad enough that C# already supports superfluous commas in some cases already. Extending that to other cases would be very sad development.

Pzixel commented 7 years ago

@MgSam it's easy to write and hard to read. As you know, 80% of time you are reading the code. Thus this should be never get implemented.

jveselka commented 7 years ago

@DavidArno

It's hurting readability

How? Whats so confusing about it?

...simply to make the lives of lazy developers easier

Of course - thats what all language features are about

It's bad enough that C# already supports superfluous commas in some cases already.

Its already allowed at least on enums and object initializers and how is it bad there? Did it ever caused you any issues?

Frankly, I do not understand why would anyone oppose this improvement.

wanton7 commented 7 years ago

I think IDE should be responsible of adding and removing commas when you paste something. Example if you paste something inside list or array initialization. But this shouldn't be added to language. TypeScript probably added this because it's in some future JavaScript spec. This just adds noise to language (hurts readability). This probably wouldn't be so big of an issue if C# had standard formatter like golang has. If you add this that comma will be all over the place.

I can see this happening

var a = new int[] 
{ 
    3, 
    4, 
    5
, 
};
alrz commented 7 years ago

Trailing commas are usually allowed when it doesn't affect type or binding. e.g. object initializers, probably match, etc. In an argument list it does affect binding (because of overload resolution), however, with named arguments it is not always significant.

DavidArno commented 7 years ago

@zippec,

It's hurting readability How? Whats so confusing about it?

I refer you to my answer to a near identical question from a year ago:


"I'm aware it's optional; that's it's problem. When looking at a piece of code with a trailing comma, one must ask: is it there because the author likes to use them, because they forgot to refactor, or because something has been missed and thus it could be a bug. They create a serious hindrance to readability just to pander to developers who want their lives made fractionally easier when writing the code."

Frankly, I do not understand why would anyone oppose this improvement.

Because some of us see their use as indicative of sloppy programming practices that favour making code easy to write over easy to read.

svick commented 7 years ago

@alrz

In an argument list it does affect binding (because of overload resolution)

Do you have an example? I tried a trivial example of adding the trailing comma in current compiler and the error I got was:

error CS1525: Invalid expression term ')'

It seems the compiler never reached the overload resolution phase.

orthoxerox commented 7 years ago

I have to write a lot of SQL at work, and the lack of trailing commas is the bane of every SQL programmer (any probably of every programmer that has to write JSON, too). While I do not think the arguments are edited that often to really require support for trailing commas, I also do not oppose this idea.

alrz commented 7 years ago

@svick

I meant the number of arguments, for example you could add property initializers without changing the return type, so it's safe to allow it there. But if you add an argument it might change the selected method.

HaloFour commented 7 years ago

punctuation means something to scatter it about just to placate formatting or decades-old diff technology is silly we have refactoring tools which manage the proper placement of commas on reordering members or arguments

jnm2 commented 7 years ago

I'm torn. On one hand trailing commas irritate me whenever I see them; on the other hand, it would be more consistent with the rest of the language.

Much as I instinctively dislike this, punctuation exists to serve the wishes of those who use it and no one would be forcing me to use it that way.

alrz commented 7 years ago

As it is a matter of code style, an analyzer could preserve, insert, remove or prohibit usages in the code, so I wouldn't mind even if it would be allowed everywhere in the language that we have a list-like construct.

MgSam commented 7 years ago

@HaloFour It has nothing to do with diff tools. Anyone who does a lot of editing of code with long method invocations with many named parameters runs into this. As @orthoxerox said, the problem is even more acute in SQL, and in my personal experience, some of those invocations of which I speak are calls to SQL stored procs or tables.

The fact that JS is adding this feature in 2017 means that a committee comprised heavily from the biggest tech companies ratified it. That's a pretty big hurdle to cross. It's not some poorly thought out artifact left over from the language's inception like some other JS features. I think that's a pretty good indication that the use case here is real and pervasive.

HaloFour commented 7 years ago

@MgSam

Anyone who does a lot of editing of code with long method invocations with many named parameters runs into this.

Indeed I have. Many, many, many times. Particularly with SQL and particularly with stored procs with dozens of parameters. Or tables with dozens to hundreds of fields. It's of the most minor of inconveniences and not one that I feel deserves special punctuation rules. Developers should probably be forced to manually fix their code more often after a bad copy&paste.

MgSam commented 7 years ago

It's definitely minor, and it only takes a second or two to correct. But correcting interrupts your train of thought. And when this happens a few times a day, everyday, the time adds up.

jnm2 commented 7 years ago

@MgSam If the IDE magically handled it for you, that would solve your problem.

alrz commented 7 years ago

magically

Can you elaborate on that?

wanton7 commented 7 years ago

@MgSam You talked about JSON before having same problem. Visual Studio 2017 already adds and removes commas when you paste to JSON array. Visual Studio also adds quotes if paste plain text to an JSON array and probably more.

@alrz This same kind of mechanism could be added for C# arrays. It's magic ;)

aluanhaddad commented 7 years ago

While I think this will do little harm, it is a bad idea.

The argument I always hear in favor of this, time and time again, is that it affects multiple lines of code in a diff. I wont belabor this as @HaloFour already put it quite eloquently

punctuation means something to scatter it about just to placate formatting or decades-old diff technology is silly we have refactoring tools which manage the proper placement of commas on reordering members or arguments

but considering the number of programming languages, let alone natural languages, that use commas to enumerate lists, it really is amazing that standard diff tools and formatters have not become more sophisticated. Lets not provide yet another reason for the evolution of our tools to stagnate.

@eyalsk

It should word like this: JavaScript gonna have this feature, therefor, TypeScript will. πŸ˜‰

Truer words were never spoken!

While on the subject of JavaScript (TypeScript added this because JavaScript added it), if this is added, it will create yet another point of contention around code style and there are frankly enough of them already. As soon as this was added to JavaScript, a lot of people wasted a lot of time debating it.

AlenPelin commented 6 years ago

This feature must be added to the standard due to a number of reasons:

P.S. I'd also add a compiler switch that makes trailing commas essential everywhere.

AlenPelin commented 6 years ago

As for IDE handling that - in general case, we cannot rely on having IDE in all cases at all stages for everybody.

Pzixel commented 6 years ago

@AlenPelin it's looks like curing the headash by heading yourself... Traling comma looks like empty statement which is pretty valid and which is the source of great amount of bugs. Tool like R# underline them and suggest to remove...

I'm totally against it. If you add a new line to some dictionary in every commit then you probably doing something wrong. It happens, but it shouldn't enforce an error prone feature in the language.

yaakov-h commented 6 years ago
void Foo (int bar, String baz, Func<int> cat) { }

var a = new int[] 
{ 
      3 
    , 4
    , 5
};

var b = new 
{ 
      Bark = "woof"
    , Bite = "chomp"
};

Foo(
      bar: 5
    , baz: ""
    , cat: () => 6
);
AlenPelin commented 6 years ago
void Foo (
    int bar, 
    string baz, 
    Func<int> cat, 
) { }

var a = new int[] 
{ 
    3, 
    4,
    5,
};

var b = new 
{ 
    Bark = "woof",
    Bite = "chomp",
};

Foo(
    bar: 5,
    baz: "",
    cat: () => 6,
);
AlenPelin commented 6 years ago

@Pzixel could you elaborate on what you meant?

Traling comma looks like empty statement which is pretty valid and which is the source of great amount of bugs

Could you provide couple examples of bugs it can cause?

Tool like R# underline them and suggest to remove...

Only because it is not supported by language/compiler. For arrays and properties it does not underline them by default.

I'm totally against it. If you add a new line to some dictionary in every commit then you probably doing something wrong.

Your position is understandable, and you are lucky that you can configure R# to treat trailing commas as errors to prevent compiling. Unlike you, me and others who want trailing commas in methods don't have such an option. In my opinion, it is not fair and needs to be fixed.

HaloFour commented 6 years ago

What I really don't like about this is that it's an example of modifying the language syntax to satisfy bad tooling, whether that be refactoring tools (or rather the lack of them) or diff tools. The fact that the language does allow it today in limited circumstances is a source of unwanted inconsistency. I'm not sure which I consider the worse sin.

AlenPelin commented 6 years ago

@HaloFour agree, tools must better support language and do not require that. In real world though tools are always imperfect and it is sad when you cannot use great tool just because it does not support certain C# limitations.

Pzixel commented 6 years ago

@AlenPelin you already have several options:

  1. Start new line with commas just like with dots. No one cares about LINQ because most of queries are written in dots first style.
  2. Append new lines at beginning instead of the end like i did yesterday: image
  3. Don't care.

I personally prefer the combination of 2nd and 3rd.

alrz commented 6 years ago

@HaloFour

Tooling is a part of the ecosystem and actually an important scenario when it comes to language design, remember one of the challenges that delayed generators is tooling. I'd argue that the syntax should be developer-friendly at the end (not merely tool-friendly), the team is willing to change the language towards that goal, as they did for nullable reference types which is totally practical as a sole tooling feature just like R# demonstrates. But that wouldn't be much convinient.

I don't know how you imagine this could be addressed with "better" tooling. specialized diff algorithm? some editor features? those have much greater cost and wouldn't be universal. you should use a particular editor, doesn't seem to be an attractive solution (or rather, workaround).

CyrusNajmabadi commented 6 years ago

I would personally like this; primarily because of consistency:

  1. The consistency between all lists. i.e. we already support this for arrays, why not for things like parameters?
  2. The consistency in code when writing things out. If i have a list of things, one per line, it seems unfortunately inconsistent that one item is different than all the rest.

This also makes things more annoying when working with effectively any tool. If i just want to swap lines around then I'm inconvenienced anytime i'm working with last and second-to-last elements.

yaakov-h commented 6 years ago

And ultimately anyone who doesn’t like trailing commas can write an analyzer. πŸ˜‰

CyrusNajmabadi commented 6 years ago

Experimenting with the code change here: https://github.com/dotnet/roslyn/pull/22851/files

Looks like it would be fairly simple to implement.

MgSam commented 6 years ago

Nice. Now you just have to slip it into 7.3, Cyrus. :smiley:

AlenPelin commented 6 years ago

@Pzixel

Append new lines at beginning instead of the end like i did yesterday

This won't work at all with method declaration as the order matters:

void Welcome(
  string name, 
  string title
) { ... }

void Welcome(
  string title,
  string name
) { ... }

This won't work with method call unnamed arguments as the order matters:

void Welcome(string name, string title) { ... }

Welcome(
  "John Smith",
  "MR"
);

Welcome(
  "MR",
  "John Smith"
);
orthoxerox commented 6 years ago

Shouldn't they be allowed in tuples, too?

AlenPelin commented 6 years ago

@orthoxerox why not? But that's slightly out of this subject "Permit trailing commas in method signatures and invocations"

DavidArno commented 6 years ago

@yaakov-h,

We could make it a compiler switch... πŸ˜‰

yaakov-h commented 6 years ago

@DavidArno <Features>pedantic-developer-compat</Features>

Pzixel commented 6 years ago

@AlenPelin good point. However, 99% of methods in my practice are singleliners because they don't have more than 4 arguments which is concidered a bad practice. I agree that sometimes these arguments are likeExpression<Func<IEnumerable<KeyValuePair<TKey, TValue>>>>>, but it doesn't happen very often :)

However, I see that trailing commas allowing to make a better diff, but makes it harder to read. You make this diff once but read this line many many times after, and every time you see comma you want to see some continuation but there isn't. I condider it's harmful :)

alrz commented 6 years ago

If we want this for all syntax lists, I think we should include declarators as well,

const int 
     A = 1,
     B = 2,
     ;
DavidArno commented 6 years ago

Has anyone mentioned extends/implements lists yet?

class Foo : IFoo, Bar,
{

And let's not forget property patterns too:

if (x is Foo(X: var x, Y: var y,)) ...

In fact, whilst we are at it, why not just allow a trailing comma on every line of code, just in case? 😱

jnm2 commented 6 years ago

@DavidArno I was just going to say, why not do it for generic arities as well? =P

ValueTuple<IEnumerable<Foo>, Bar, IDictionary<string, Baz>, >

DavidArno commented 6 years ago

@jnm2,

That's a good point. My "trailing comma on every line" idea would sadly be a breaking change here:

Type d1 = typeof(Dictionary<,,
>);

Oh well, back to just hating this idea. πŸ˜†

aluanhaddad commented 6 years ago

Maybe one day, diff tools will understand what lists are. Then they would have a mode where only one line is shown as changed.

To put it another way, maybe they can evolve to have a notion of logical lines instead of physical lines, kind of like how hardware abstraction layers expose information to an operating system.

CyrusNajmabadi commented 6 years ago

If we want this for all syntax lists, I think we should include declarators as well,

My impl would allow for this.

Has anyone mentioned extends/implements lists yet?

My impl would allow for this.

I was just going to say, why not do it for generic arities as well?

This and tuples would worry me. I can envision us doing things in the future where generic arities might be inferred, or tuple values might be omitted. I can see having a trailing comma having meaning. So i don't want to do this yet.

CyrusNajmabadi commented 6 years ago

Maybe one day, diff tools

We have to accept that there is a broad set of tooling out there that has existed for decades and which has not done these "really nice to have things that would make the world come together and make a happier place". Sometimes the path of least resistance and most effectiveness is just to accept that and make the change in the single tool itself.

This is similar to when i added support to C# to understand git-merge-markers. Yes, we could hope all tools out there would understand and process them properly. But that might take decades. Or, we could just do the couple of hours of work and make the experience substantially better today.

I far prefer pragmatic solutions that bring about things now, versus waiting years.

--

BTW: i've worked in this area for 15 years now. I remember these same discussions 15 years ago. And even back then people had been talking about them for ages. It's just not going to happen for a long long time :)

CyrusNajmabadi commented 6 years ago

Another case i didn't want to change was commas in a lambda. i.e allowing (a, b, c, ) => ... I could imagine us allowing omitted parameters here in the future, and i don't want to make this ambiguous.

jnm2 commented 6 years ago

I was just going to say, why not do it for generic arities as well?

This and tuples would worry me.

Good, that was the motivation for me saying it. =)

jveselka commented 6 years ago

@DavidArno

"I'm aware it's optional; that's it's problem. When looking at a piece of code with a trailing comma, one must ask: is it there because the author likes to use them, because they forgot to refactor, or because something has been missed and thus it could be a bug. They create a serious hindrance to readability just to pander to developers who want their lives made fractionally easier when writing the code."

I can make the same argument about dozens of existing and completely fine constructs:

if (condition)
{
   action();
}

Are the unnecessary brackets "there because the author likes to use them, because they forgot to refactor, or because something has been missed and thus it could be a bug."

If you approach code with assumption that it is wrong, of course you end up with a lot of questions.

ufcpp commented 6 years ago

I often generate source codes like the following:

void Method(
    T1 arg1,
    T2 arg2,
    T3 arg3,
    T4 arg4,
    bool dummy = false)

I really want this proposal.