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

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

Open
MgSam opened this issue Mar 31, 2017 · 84 comments
Open

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

MgSam opened this issue Mar 31, 2017 · 84 comments

Comments

@MgSam
Copy link

MgSam commented Mar 31, 2017

Migrated from dotnet/roslyn#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
Copy link

    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
Copy link

Pzixel commented Mar 31, 2017

@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
Copy link

@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
Copy link

wanton7 commented Mar 31, 2017

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
Copy link
Member

alrz commented Mar 31, 2017

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
Copy link

DavidArno commented Mar 31, 2017

@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
Copy link
Contributor

svick commented Mar 31, 2017

@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
Copy link

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
Copy link
Member

alrz commented Mar 31, 2017

@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
Copy link
Contributor

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
Copy link
Contributor

jnm2 commented Mar 31, 2017

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
Copy link
Member

alrz commented Mar 31, 2017

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
Copy link
Author

MgSam commented Mar 31, 2017

@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
Copy link
Contributor

@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
Copy link
Author

MgSam commented Mar 31, 2017

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
Copy link
Contributor

jnm2 commented Mar 31, 2017

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

@alrz
Copy link
Member

alrz commented Mar 31, 2017

magically

Can you elaborate on that?

@wanton7
Copy link

wanton7 commented Mar 31, 2017

@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
Copy link

aluanhaddad commented Apr 1, 2017

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
Copy link

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

  • C# already allows that in arrays and objects
  • it makes diff cleaner (that makes eyes happier)
  • it removes certain merge conflicts (that require human attention and is a great source of bugs)
  • other languages already / will have it

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

@AlenPelin
Copy link

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

@Pzixel
Copy link

Pzixel commented Oct 24, 2017

@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
Copy link
Member

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
Copy link

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
Copy link

AlenPelin commented Oct 25, 2017

@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
Copy link
Contributor

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
Copy link

@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
Copy link

Pzixel commented Oct 25, 2017

@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.

@datvm
Copy link

datvm commented Dec 6, 2020

I want to add a use case for this suggestion, for the new record syntax, this would be awesome (currently impossible):

    public record Foo (
        int bar1,
        string? bar2,
    );

@mikernet
Copy link

mikernet commented Jun 4, 2021

There is another case where I think a trailing comma should be allowed that I don't believe was mentioned: collection initializer Add method parameters, especially when they are params. For example:

public class Simple : IEnumerable<(string, string, string)>
{
    public void Add(string x, string y, string z) { }
}

var a = new Simple() {
    {
        "fkeuahwkeufhawefe",
        "afkuehafkuehkuehf",
        "aswefa4rguikareg", // this comma is not allowed
    },
};

public class WithParams : IEnumerable<IReadOnlyList<string>>
{
    public void Add(params string[] x) { }
}

var b = new WithParams () {
    {
        "fkeuahwkeufhawefe",
        "afkuehafkuehkuehf",
        "aswefa4rguikareg", // this comma is not allowed
    },
    {
        "fkeuahwkeufhawefe",
        "afkuehafkuehkuehf", // this comma is not allowed
    },
    {
        "fkeuahwkeufhawefe",
        "afkuehafkuehkuehf",
        "fkeuahwkeufhawefe",
        "afkuehafkuehkuehf",
        "fkeuahwkeufhawefe",
        "afkuehafkuehkuehf",
        "aswefa4rguikareg", // this comma is not allowed
    },
};

@Pzixel
Copy link

Pzixel commented Jun 4, 2021

I think trailing comma should be available in any context that accepts array of arguments. It could be just an optional trivia for any SeparatedList

@MixusMinimax
Copy link

I strongly agree with this. For the people that find that an extra comma hurts readability, how are you able to read the arguments that come before? Those have a comma after them, and you don't complain there.

Merge conflicts are a huge issue. Sure, there are syntax-aware diff systems, which I hope become more prevalent in the future, but for now, diffs are line based. Adding a new field to a record should not modify two lines.

@theunrepentantgeek
Copy link

For the people that find that an extra comma hurts readability, how are you able to read the arguments that come before? Those have a comma after them, and you don't complain there.

My objection comes from the cognitive science around how people read.

For people whose first reading language uses a comma as a phrase separator (*), the presence of a , is a subconscious visual indicator that there's more to come in this sentence.

(*) From what I've read, it's less clear whether this applies to people whose first language has different forms.

When you read foo, bar, baz each comma is understood by your brain as indicating there's more to come.

When you read foo, bar, baz, your brain has to do a doubletake because the promise made by the , that there's more to come is immediately violated.

Redundant trailing commas are a cognitive speedbump. There's very strong evidence from the natural language field that shows incorrect punctuation greatly slows down comprehension.

My personal experience working in Go backs this up. It's like having an uneven brick in your garden path that you trip on most every time you check the letterbox.

Merge conflicts are a huge issue. Sure, there are syntax-aware diff systems, which I hope become more prevalent in the future, but for now, diffs are line based.

And this is exactly why we should not be changing the language.

Language changes are forever; they can never be undone.

Why should we make a permanent change to the language just to accommodate tooling that hasn't yet caught up?

Language changes are supremely expensive, both to make and to maintain. It would be cheaper to fix the tooling.

@jnm2
Copy link
Contributor

jnm2 commented Nov 24, 2022

My experience is that ite roadbump disappears quickly with use. I switched over for multiline initializers, where the comma appears consistently at the end of the lines which end each item.

@Socolin
Copy link

Socolin commented Nov 24, 2022

For me it would remove the "noise" of all the removed/added lines due to the addtion of a coma, during code review.

Example:

public SomeClass(
	IDeps1 deps1,
-	IDeps2 deps2
+	IDeps2 deps2,
+	IDeps3 deps3,
)

I would need this only for multi line initializer, it does not make sence in single line. And like ; at the end of each line, I think the brain can understand that , at the end of each line of a parameters is a part of the syntax.

public SomeClass(
	IDeps1 deps1,
	IDeps2 deps2,
	IDeps3 deps3,
)

@theunrepentantgeek I'm not sure to understand what the point with one-liner example, since this feature really seems to be useful in a multiline example (I don't see the point in adding extra , if the code is on one line), and I may be wrong but when I'm seeing this, I see this as a list and not as a sentense for which I' m looking for "what next"

foo,
bar,
baz, 

Language changes are supremely expensive, both to make and to maintain. It would be cheaper to fix the tooling.

Changing all the diff tool and merge tool and git blame, to undestand that specific case to improve the diff does not seems cheaper. And I don' t think git will start parsing C# and understant that a coma added between 2 parameters should be ignored anytime soon.

@Logerfo
Copy link
Contributor

Logerfo commented Nov 24, 2022

Reducing noise in diffs is the main reason I always use trailing commas in multi-line scenarios. Allowing them in parameter and argument lists would cover this noticeable feature gap.

@JeroMiya
Copy link

I don't think I would want a diff tool to hide a changed line, even if it were capable of it through syntax aware diffing.

@theunrepentantgeek
Copy link

I'm not sure to understand what the point with one-liner example

I was using a single line because I was giving a natural language example, not a code one.

That said, C# has never been a whitespace sensitive language, so it's irrelevant whether we're talking single- or multi-line. If a trailing comma is permitted in one place, it will be permitted in the other.

Changing all the diff tool and merge tool and git blame, to undestand that specific case to improve the diff does not seems cheaper.

Language changes are extremely expensive, which is why they get discussed and debated endlessly before they're made.

My guestimate is that the cost of changing the language would exceed the cost of updating the tools by at least an order of magnitude (that is, 10x).

My experience is that ite roadbump disappears quickly with use.

We developers can get used to most anything. We're smart like that.

In this case, however, given the hestitation that is built into the way our brains parse language (at an autonomic level), it's not something we can ever avoid, but instead just something we get used to.

I suspect this kind of adaptation is like learning to avoid the squeaky step on a staircase: you stop irritating people in the middle of the night, but the problem is still there.

I've gotten used to trailing commas in Go, simply because I have no choice. But that doesn't mean I like them - because I'm aware of the science that makes them a foolish quirk of the language.

@Socolin
Copy link

Socolin commented Nov 25, 2022

While speaking about tooling, if the only reason not to add the coma is a preference and readability issue, I think then we should allow this and add a setting in the IDEs to show/hide the trailing comma.

This would allow to fix problem with the diff tools and not add new problem for humans.

The comma could be added automatically and hidden depending on the settings of the project / IDE

@miloush
Copy link

miloush commented Nov 25, 2022

I wouldn't support IDE hiding parts of content of the file, but aren't all these cognitive arguments resolved by just not using the feature if it makes the experience worse for you?

@snebjorn
Copy link

JavaScript was able to do trailing commas and it's awesome! JavaScript pretty must runs the world. so if that was able to do it then C# also should 😄

I see comments about "preference and readability". Lets not forget that the trailing commas are optional. If you don't like them or it makes it harder for you to read. Then don't use them.

@MixusMinimax
Copy link

Cognitive science is mainly to be applied to natural languages. A list of arguments is not a sentence, it is a list of arguments. And yes, if you don't like it, then don't write trailing commas, that's what formatters are there for to enforce. But the arguments for merge conflicts, consistency, etc. strongly outweigh "idk it's not as nice to read". That's an argument to not use the feature, sure.

@jnm2
Copy link
Contributor

jnm2 commented Nov 25, 2022

That said, C# has never been a whitespace sensitive language, so it's irrelevant whether we're talking single- or multi-line.

For questions of formatting and readability, whitespace has never been irrelevant.

@theunrepentantgeek
Copy link

Lets not forget that the trailing commas are optional. If you don't like them or it makes it harder for you to read. Then don't use them

This is only relevant for toy projects that will only ever have a single developer.

As soon as you start working on anything with a meaningful size, you're looking at contributions from multiple developers over multiple years.

Later devs don't get a choice, they have to deal with whatever their predecessors have done.

Arguments of "If you don't like it, don't use it" always founder on this fact.

@snebjorn
Copy link

As soon as you start working on anything with a meaningful size, you're looking at contributions from multiple developers over multiple years.

Later devs don't get a choice, they have to deal with whatever their predecessors have done.

Code style is a team decision. They can change in the lifetime of a project. When they do you run the formatting tool with the new settings. I don't see a problem here.

@mrwensveen
Copy link

For me, the inconsistency between initializing a record type (constructor) and a class with properties annoys me the most because the use case is so similar. If it only were to be allowed in collection initializers then I would be okay with it (kind of).

Some people have argued that it's bad enough that C# allows it in other places but that's a matter of opinion. The feature clearly made it through, presumably with people arguing for and against the feature.

So, while I think this feature would be nice, I feel more strongly about the language being inconsistent with regards to trailing comma's.

@bw-flagship
Copy link

The (quite new) language "dart" allows trailing commas. IDEs make use of this and automatically break lines if you add a trailing comma on method calls, constructors, etc. Switching from back to coding c# from coding dart feels like a downgrade because this is not possible.

@KennethHoff
Copy link

With the introduction of Primary constructors I think this "issue" should be looked at. You may wonder what Primary Constructors have to do with anything, so here's my argument (pun intended):

Previously whenever you wanted to add or remove a field from a class you had to do it many places; Add the field, add a parameter to the constructor and set the field from the parameter (And obviously the same thing in reverse for removing), so adding/removing a single comma wasn't that big of a deal.

Now on the other hand you only have to change a single line to do all those three things mentioned previously, so the removal of a comma is now a majority of the time spent. Instead of simply pressing shift-delete (default keybind in Rider for deleting an entire line), you also have to go up a line and remove the comma.

(This is in addition to all the other arguments mentioned above, like consistency with initializers)

@beppemarazzi
Copy link

Or, in other words, in many context (i.e. initialization list, argument list, ...) the comma will act as a "terminator" not as a "separator". Exactly like the semicolon does with statements. (https://en.wikipedia.org/wiki/Comparison_of_programming_languages_(syntax)#:~:text=A%20statement%20separator%20demarcates%20the,end%20of%20an%20individual%20statement.).
It's another (little) point in favor of this proposal to make the whole language more self consistent...

@Bigsby
Copy link

Bigsby commented Jan 29, 2024

This would improve code versioning because, when adding items to lists (e.g., function parameters) only the added line is marked as changed and not the previous last line where the comma was added.

@Foxtrek64
Copy link

Coming over from #8190

This is very useful when targeting multiple TFMs. Take this example that's currently impossible today:

/// <summary>
/// Represents a 9-digit United States social security number.
/// </summary>
[PublicAPI]
[DebuggerDisplay($"{{{nameof(GetDebuggerDisplay)}(),nq}}")]
public readonly struct SocialSecurityNumber
    : IFormattable,
    IEquatable<SocialSecurityNumber>,
#if NET7_0_OR_GREATER
    IEqualityOperators<SocialSecurityNumber, SocialSecurityNumber, bool>,
#endif
#if NET8_0_OR_GREATER
    IUtf8SpanFormattable,
    IUtf8SpanParsable<SocialSecurityNumber>
#endif
{
    // Specification omitted
}

Such is only possible with trailing commas allowed.

There are a couple work-arounds, such as SQL-style commas, but style rules may prohibit this:

/// <summary>
/// Represents a 9-digit United States social security number.
/// </summary>
[PublicAPI]
[DebuggerDisplay($"{{{nameof(GetDebuggerDisplay)}(),nq}}")]
public readonly struct SocialSecurityNumber
#pragma warning disable SA1001 // Commas should not be preceeded by whitespace.
    : IFormattable
    , IEquatable<SocialSecurityNumber>
#if NET7_0_OR_GREATER
    , IEqualityOperators<SocialSecurityNumber, SocialSecurityNumber, bool>
#endif
#if NET8_0_OR_GREATER
    , IUtf8SpanFormattable
    , IUtf8SpanParsable<SocialSecurityNumber>
#endif

Partial classes are another option, but this doesn't make sense in all cases.

@mikernet
Copy link

mikernet commented Jun 7, 2024

@Foxtrek64 Just FYI, you can also do:

public readonly struct SocialSecurityNumber :
#if NET7_0_OR_GREATER
    IEqualityOperators<SocialSecurityNumber, SocialSecurityNumber, bool>,
#endif
#if NET8_0_OR_GREATER
    IUtf8SpanFormattable,
    IUtf8SpanParsable<SocialSecurityNumber>,
#endif
    IFormattable,
    IEquatable<SocialSecurityNumber>
{
}

@aradalvand
Copy link
Contributor

aradalvand commented Jun 29, 2024

The use cases are patently clear and justified. Source generators, TFMs, consistency, diffing, etc. This one little trivial-to-implement feature, which almost every other modern language already has, would improve the experience in all those areas.

So why not just do it and get it over with?

@CyrusNajmabadi
Copy link
Member

So why not just do it and get it over with?

Because our plates are full with lots of other work.

@miyu
Copy link

miyu commented Sep 3, 2024

If this is implemented, it'd be consistent to support trailing commas in array indexers too. This would be useful for a variety of niche DSLs that I've seen... Trivial examples would be DSLs which index by filter condition or slice N-dimensional data grids.

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

No branches or pull requests