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

[Suggestion] Remove/relax CS1738 "Named argument specifications must appear after all fixed arguments have been specified" #518

Closed
gafter opened this issue Apr 28, 2017 · 22 comments

Comments

@gafter
Copy link
Member

gafter commented Apr 28, 2017

@dsaf commented on Thu Jun 02 2016

I like using named arguments for code readability when passing literals. However the subj limitation forces me to either not use them or specify them for every subsequent argument:

var name = "John";
var age = 30;

DoSomething(isEmployed:true, name, age); //CS1738

@bondsbw commented on Mon Jun 06 2016

I like this, but only if argument order aligns with the parameter list until at least the last unnamed argument.

Example:

public void DoSomething(bool isEmployed, string personName, int personAge) { ... }

DoSomething(true, personName:name, age); // would be legal
DoSomething(name, isEmployed:true, age); // remains illegal
DoSomething(name, age, isEmployed:true); // remains illegal
DoSomething(true, personAge:age, personName:name); // already legal

@dsaf commented on Tue Jun 07 2016

@bondsbw of course.

@jnm2
Copy link
Contributor

jnm2 commented Apr 29, 2017

Yes! Another frequent wish.

@sharwell
Copy link
Member

sharwell commented Apr 30, 2017

@gafter I would be interested in your feedback regarding the best way to move this proposal forward.

The current language specification contains the following limitation:

It is an error for a positional argument to appear after a named argument in an argument_list.

I would suggest the following alternative:

It is an error for a positional argument to appear after a named argument in an argument_list for which the named argument's position does not equal the corresponding parameter position in the target of the invocation.

This change in wording should not negatively impact the ability of named arguments to be used for overload resolution, even in cases like the following:

private static void Foo(int x, int a) { }
private static void Foo<T>(T y, int a) { }

private static void Bar()
{
    Foo(3, 4);
    Foo(y: 3, 4);
}

I am worried about the complexity this feature would introduce in dealing with the following:

private static void Foo(int x, int y) { }
private static void Foo<T>(T y, int x) { }

private static void Bar()
{
    Foo(3, 4);
    Foo(y: 3, x: 4); // Invokes Foo(int, int)
    Foo(y: 3, 4); // Invokes Foo<T>(T, int)
}

@gafter
Copy link
Member Author

gafter commented May 1, 2017

@gafter I would be interested in your feedback regarding the best way to move this proposal forward.

@sharwell The next step would be to find someone on the LDM @dotnet/csharplangdesign who believes this is both desirable and of higher priority than many other things we could do, and who is willing to invest their time to push the issue forward. That LDM member would then volunteer to "champion" this proposal, and bring it to LDM to be triaged. I believe there are two or three members of the LDM on your team.

@gafter
Copy link
Member Author

gafter commented May 1, 2017

@sharwell Also, one should compare doing this language feature with the alternative of doing an IDE feature that fills in the parameter names for you at the call site.

@DavidArno
Copy link

DavidArno commented May 2, 2017

I think it reasonable to assume that the overwhelmingly most common use-case for this will be for handling bool parameters:

 DoSomething(isEmployed:true, name, age);

The use of bool parameters like this is a code smell in my view, as it's very likely that DoSomething will like:

void DoSomething(bool isEmployed, string Name, int Age)
{
    if (isEmployed)
    {
        // do something with employed person
    }
    else
    {
        // do something with unemployed person
    }
}

In such situations, it makes more sense to provide two methods:

DoSomethingWhenEmployed(name, age);
DoSomethingWhenUnemployed(name, age);

(Updated to retract the claim that most use cases will be for bool parameters due to feedback from others that other use-cases are common. This comment is now therefore orthogonal to the thread, but is left here to give subsequent comments context).

@bondsbw
Copy link

bondsbw commented May 2, 2017

But then you double each overload that includes the bool. And more bools causes them to increase exponentially.

The main reason bools are used is laziness. A new enum requires typing more characters and LOC than the bool keyword, and the EnumeratedType.Option callsite syntax is verbose. using static makes this much nicer.

ADTs would be even better, preventing the use of invalid values.

@sharwell
Copy link
Member

sharwell commented May 2, 2017

@DavidArno passing null is another common case. And while you could argue for your own code to avoid cases where this would be relevant, it's a moot point for published third-party libraries with stable APIs. Maybe it should have been different, but it won't/can't be now.

Literals of various kinds are also frequent in unit tests and named parameters can help readers be confident that the correct methods are being tested.

@bondsbw
Copy link

bondsbw commented May 2, 2017

Like most things which could break BC in the language, that should probably be left to an analyzer. But this proposal is a good start and can even be coupled with an IDE feature that targets literal arguments.

@jnm2
Copy link
Contributor

jnm2 commented May 2, 2017

I have this problem with ints too, not just bools and null.

@mattwar
Copy link
Contributor

mattwar commented May 11, 2017

Yes, any time you are passing a literal value where it is not clear what the value represents it is useful to use a named parameter to help document what is happening in the code, but a nuisance that all following parameter must now be named as well.

@sharwell
Copy link
Member

@gafter I believe the main value in this comes from the fact that the place I see this requested the most is in code reviews, where IDE features aren't playing a role. Perhaps we can get GitHub someday to incorporate Roslyn into their PR UI (can you imagine?!), but until then I have to say this is a pretty compelling request!

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented May 11, 2017

I woudl love this feature. And I would champion this. My hope is that it could slot in close to being a 'bugfix' sized change.

@jcouv
Copy link
Member

jcouv commented May 11, 2017

Talked to Cyrus. I'll go ahead and start a championed proposal. Link coming shortly.
He also pointed out that there is some impact on an IDE feature that suggests adding named-parameters. It could just suggest a minimal fix (ie. not add names to all the rest of the parameters).

@jcouv
Copy link
Member

jcouv commented May 11, 2017

Opened #570 to start the process. I'll write a proposal that can be discussed in LDM to get a rough sense of prioritization.

@svick
Copy link
Contributor

svick commented May 11, 2017

@sharwell There are tools like Reviewable that build on top of GitHub's PRs. If you want to use Roslyn with PRs, I think an external tool like that is more likely than GitHub adding explicit support for C#.

@svick
Copy link
Contributor

svick commented May 11, 2017

Would this allow using named arguments with params?

In https://github.com/dotnet/corefx/issues/19599#issuecomment-300527721, this method was proposed:

public class Task
{
    public static Task When(TaskStatus all, TaskStatus any, params Task[] tasks);
}

It would be great if such method could be called as:

Task.When(all: TaskStatus.RanToCompletion, any: TaskStatus.Faulted, task1, task2)

I think this would be useful for other params methods too.

@jcouv
Copy link
Member

jcouv commented Jun 5, 2017

@sharwell The problem you point out is not new. Calling Foo(3, 4) picks one, while calling Foo(3, x: 4) picks the other.
Granted this problem only occurs in trailing positions today.

private static void Foo(int x, int y) { }
private static void Foo<T>(T y, int x) { }

@jcouv
Copy link
Member

jcouv commented Jun 5, 2017

@svick Thanks for bringing up the params example. Yes, I think that should work (I took a note in proposal and I'll add test cases to the prototype).

For each candidate method considered for applicability:

  • If you use a named argument in the right position, the argument is used positionally.
  • If you use a named argument out-of-position, then following arguments must be named. Otherwise the candidate is not applicable.

In your example with params, the argument is used in the right position, so the rest of the parameters continue to work positionally.

@contextfree
Copy link

I also like to use named arguments whenever I call a method with multiple parameters of the same type; it makes it harder to accidentally get the order of parameters wrong, by "making wrong code look wrong". Currently this makes me also use named arguments for every other parameter as well, even when some of them have different types and so should be covered by the type checker.

@grzfes
Copy link

grzfes commented Jul 24, 2018

(a bit late, but)
Would not be good to give a warning for this known case (or suggestion):

private static void Foo(int x, int y) { }
private static void Foo<T>(T y, int x) { }

private static void Bar()
{
    Foo(3, 4);
    Foo(y: 3, 4); // warning
}

Determining the problem

Collect candidates with an argument called y¹. Then compare overloads by type:

Foo(int, int)
Foo(T, int)

so as it usually happens: passing 2x intFoo(int, int) is better.

Message

Method Foo<T>(T, int) will be used instead of the preferred Foo(int, int) based on the positions of named arguments.

Possible solutions

  1. Foo(int, int) performs exactly the same as Foo(T, int) without a significant difference in performance (simple examples: .ToString(), .GetHashCode() etc.).

Correction: removal of unnecessary overload Foo(int, int).

  1. Foo(int, int) performs tasks much more efficiently for int (frequent use, less resource consumption, no casting etc.).

Correction:

private static void Foo(int x, int y) { }
private static void Foo<T>(int x, T y) { }

or

private static void Foo(int y, int x) { }
private static void Foo<T>(T y, int x) { }

After change, a specialized overload Foo(int, int) will now be selected in both cases.

  1. T y¹ means something different than int y¹.

Correction: change of argument name, e.g.:

private static void Foo(int z, int x) { }
private static void Foo<T>(T y, int x) { }
  1. Inability to make changes 1-3.

Correction:

  • use of named parameters in the order defined in the preferred overload.
  • not to use named parameters for arguments that may cause this problem.
  • abandoning the use of named parameters for this method and its overloads.

(not solution, but rather conscious use)
Ignore the warning (suppress), if this is intentional call to the generic overload throughout positions of named arguments.

¹ - name of the first named argument that caused this problem.


I think that the unconscious changes in the way of choosing overload is a serious problem.
It is like suddenly there was no overloads like Foo(int, int).

@sharwell
Copy link
Member

@gafter Can we close this as now implemented?

@gafter
Copy link
Member Author

gafter commented Nov 11, 2019

Yes; this was implemented in C# 7.2 per #570

@gafter gafter closed this as completed Nov 11, 2019
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