New: function-paren-newline rule (fixes #6074)

[not-an-aardvark]

What is the purpose of this pull request? (put an "X" next to item)

[x] New rule (see #6074)

What changes did you make? (Give an overview)

This implements the function-paren-newline rule as described in #6074, and enables it on the ESLint codebase for dogfooding. The rule applies to all function parameters, as well as CallExpression/NewExpression arguments.

I decided to use multiline as the default, instead of always as described in #6074, because always seems like a very unusual style.

Is there anything you'd like reviewers to focus on?

• I'm unsure of the correct behavior for the multiline option. There are two possible behaviors that I can think of, which are subtly different:

  1. Require newlines if there are linebreaks anywhere within the parameters/arguments.
  2. Require newlines if there are any linebreaks between two parameters/arguments.

  If option 1 is used, the following code is reported, because the arguments span multiple lines. I think this is very undesirable.

  array.map(function(x) {
    return x + 2;
  });

  If option 2 is used, the rule reports code like this, which is slightly undesirable:

  return Boolean(
    foo && bar &&
    baz && qux
  );

  Currently, option 2 is implemented, but I'm concerned that it makes code less readable in some cases.

• The rule can produce some very long lines. For example, the following code gets reported:

  foo(
    bar("baz", qux(
      new Foo()
    ))
  );
  
  // gets fixed to
  
  foo(bar("baz", qux(new Foo())))

  I'm concerned that this can make the code less readable.

• Due to Modify rules for handling indentation of closing parentheses #7522, the autofixer for this rule can be difficult to use. For example:

  function foo() {
      foo(
          bar,
          baz);
  }
  
  // gets fixed to:
  
  function foo() {
      foo(
          bar,
          baz
  );
  }

  Since the indent rule doesn't report or fix the indentation of the closing ), it usually ends up in the wrong place. The indent rewrite fixes this issue, so maybe we should wait until 4.0.0 before releasing this rule.

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @kaicataldo and @gyandeeps to be potential reviewers.

[eslintbot]

LGTM

[platinumazure]

Do we need an autofix in the first release of this rule? We could put a fixer in for 4.0 but release a usable rule in a minor release, no?

[kaicataldo]

Looks like we have some merge conflicts here. No need to rush this though since it doesn't have to be released in a major version.

[ilyavolodin]

We don't need this anymore.

[not-an-aardvark]

Does anyone have any thoughts on how to address the usability issues described in the comment at the top?

[ilyavolodin]

I think the only way to fix this usability issue is to introduce yet another option. While option 2 is reasonable, I personally prefer option 1. I also see an issue of requiring closing brace to be on it's own line (I personally prefer it to be on the same line as last argument).

[ebramanti]

What is the status on this?

[not-an-aardvark]

I've been thinking about this more. What if we added a selector option to the rule to allow greater configurability? Would that solve all the use cases we need? For example, maybe people could use that to enforce parens for nested functions.

[jainanshul]

Is there an update on this issue?

[platinumazure]

Could we maybe add a consistent option?

• Wouldn't enforce having or not having newlines inside function parens, in general, but
• A newline is required after an opening function paren iff a newline is present before a closing function paren

I think that would be a sensible default, as well.

Also, does this apply to function calls, function declarations, or both? If both, is it worth creating rules for (or supporting options in this rule for) different behaviors for function calls vs function declarations?

[not-an-aardvark]

Could we maybe add a consistent option?

I agree, adding a consistent option sounds like a good idea.

I think we should make consistent a string option like always and never, rather than adding it to the object with minItems. We added a similar thing to the object for object-curly-newline, and ended up with a confusing schema where one option affects the behavior of another.

Also, does this apply to function calls, function declarations, or both?

It applies to all "function parens" (i.e. the parentheses around the arguments in CallExpressions and NewExpressions, and the parentheses around the parameters in FunctionDeclarations, FunctionExpressions, and ArrowFunctionExpressions).

[platinumazure]

I think we should make consistent a string option like always and never, rather than adding it to the object with minItems.

👍, I had that in mind as well.

It applies to all "function parens" (i.e. the parentheses around the arguments in CallExpressions and NewExpressions, and the parentheses around the parameters in FunctionDeclarations, FunctionExpressions, and ArrowFunctionExpressions).

I'm okay with this for now, but does it not seem likely that we'll get a request to support different behavior? (But that can certainly be added later as a non-breaking change.)

[eslintbot]

LGTM

[eslintbot]

LGTM

Dismissing this review because a lot of time has passed and the proposal has changed a bit.

[eslintbot]

LGTM

[not-an-aardvark]

@eslint/eslint-team This should be ready to merge after review. Sorry about the long bikeshedding delay.

[eslintbot]

LGTM

[eslintbot]

LGTM

[jainanshul]

When do you guys plan to release a new eslint version with this feature?

[not-an-aardvark]

An hour ago

[jainanshul]

multiline rule doesn't seem to like if we break an only function parameter in a new line.

function test(a: string) {
  console.log(a);
}

test(
  'hello',
);

In some cases the only argument is a really long argument and it seems nicer, being able to break in on the next line. Is there a way to allow 1 function parameter to be on a new line as part of `multiline` rule?

[jstephenson]

Is there a proposed solution for @jainanshul's scenario above?

[not-an-aardvark]

No. Currently, the rule is working as designed, but if you'd like to propose an enhancement to this rule, please create a new issue.