'max-len' conflict with 'function-paren-newline'

[blord-fullscreen]

There currently appears to be no way to use function-paren-newline in a way that takes account of maximum line length. With the default multiline option and a function with a single long parameter name, there is no way to format the code without violating either function-paren-newline or max-len. Examples:

1. This snippet:

export default connect(mapStateToProps, mapDispatchToProps)(
  generateDndContextWithBackend(SiteEditorContainer));

triggers the error Unexpected newline after "(".

2. Reformatting to:

export default connect(mapStateToProps, mapDispatchToProps)(
  generateDndContextWithBackend(SiteEditorContainer),
);

triggers the error twice:

Unexpected newline after "("
Unexpected newline before ")"

3. Removing all the newlines:

export default connect(mapStateToProps, mapDispatchToProps)(generateDndContextWithBackend(SiteEditorContainer));

Triggers the max-len error. If there is an error-free formatting solution given the current defaults, please educate me!

If this is a bug, one possibility could be to make the existing multiline option sensitive to max line length. Another could be to create a new option.

ESLint Version: 4.8.0
Node Version: 6.11.1
npm Version: 5.2.0

eslintrc

{

  "extends": "airbnb",
  "plugins": [
    "react"
  ],
  "parser": "babel-eslint",
  "parserOptions": {
    "ecmaFeatures": {
        "jsx": true
    }
  },
  "rules": {
    "jsx-a11y/click-events-have-key-events": "off",
    "jsx-a11y/anchor-is-valid": "off",
    "react/display-name": 0,
    "react/require-default-props": 0,
    "indent": ["error", 2, {"SwitchCase": 1}],
    "no-mixed-operators": ["error", {
      "groups": [
        ["+", "-", "*", "/", "%", "**"],
        ["&", "|", "^", "~", "<<", ">>", ">>>"],
        ["==", "!=", "===", "!==", ">", ">=", "<", "<="],
        ["&&", "||"],
        ["in", "instanceof"]
      ],
      allowSamePrecedence: true
    }],
    "object-curly-newline": "off"
  },
  "env": {
    "browser": true,
    "node": true,
    "es6": true,
    "amd" : true,
    "mocha": true,
    "jest": true
  }
}

This issue originally raised at airbnb/javascript#1584

[not-an-aardvark]

Thanks for the report.

I'm not sure about the best solution for this. On the one hand, I'm not fully convinced that it needs to be fixed at all. With sufficiently long identifier names, max-len is unsatisfiable even without function-paren-newline being involved, so arguably function-paren-newline isn't the issue here.

In the past, we've generally avoided adding options related to the text length of a piece of code -- see #7526 for the reasoning.

That said, I acknowledge there's a usability concern here -- I noticed a similar thing in #8102 when creating the function-paren-newline rule. One possible solution would be to add an exception for function calls which are arguments of other function calls, although it seems like that might just be making the problem more uncommon rather than solving it (since it seems like a similar issue could occur when using the array-bracket-newline rule with an array literal as an argument). Maybe another solution would be to check some "complexity" limit for the argument, other than its text length (e.g. the number of nested AST nodes).

[blord-fullscreen]

Thanks for the thoughtful consideration.

On the one hand, I'm not fully convinced that it needs to be fixed at all. With sufficiently long identifier names, max-len is unsatisfiable even without function-paren-newline being involved, so arguably function-paren-newline isn't the issue here.

Sure, but the case of ultra-long identifiers seems very rare. A simple chain of higher-order functions with descriptive names like what I posted is much more common. It's unacceptable to me that a modern formatter has no solution here. We have higher-order components like this all around the projects we work on at my job, and the identifiers aren't crazy-verbose.

Checking the AST for a measure of complexity also kind of seems like a road to ruin, liable to produce allow some weird unexpected results when code is tersely written. I'd like to think there is a better way.

I don't understand why rules in principle can't look at (certain) other rules. You say the reason is that "since it has to be possible to enable/disable rules independently". But this would be possible even with interdependent rules. If a user configured function-paren-newline to somehow look at max-len, and max-len was turned off, then function-paren-newline's internal representation of max-len could simply revert to some default, possibly Infinity. The rules are still atomic, even if they interact. Having to set a max-len option in multiple different places, for separate rules, is error prone, and clearly not an ideal developer experience.

If you're worried about circular dependencies between formatting rules (a legitimate concern), one solution could be to elevate max-len to a "super-rule", of which there could only be a few (or perhaps only one!). Rules would only be able to access their own config, and that of the super- rule(s). Please bear with me here--this isn't as crazy as it sounds.

Here's the crux: I think it's fair to expect max-len to almost always be configured. A tool designed for formatting legibility has to draw the line somewhere. If I disable max-len entirely, or set it to Infinity, and I run Prettier (with the eslint integration) on my script, I fully expect it to remove all newlines completely, and wrap everything onto a single line. IMHO If you turn off max-len you should expect to be running a very crude minifier.

Line length is one of the most crucial code formatting criteria. It's more like a super-criterion. It's not just another rule. This is one of the things that Prettier got spectacularly right from the start. Prettier has subtly changed the way people think about line-length, correctly putting max line-length conceptually "at the top" of the formatting configuration. Everything has to be considered within the constraints of line-length. I would like to see ESLint assimilate this insight, in order to solve problems like this one.

[not-an-aardvark]

Thanks for continuing the discussion. I have a few thoughts on what you said:

I don't understand why rules in principle can't look at (certain) other rules. You say the reason is that "since it has to be possible to enable/disable rules independently". But this would be possible even with interdependent rules.

Since ESLint was created, there has been a consistent design goal that rules should not be able to know about other rules. There are a few reasons for this:

• Users can create custom rules at runtime, or load rules that were imported from plugins. In many cases, people make custom rules to enforce a stylistic preference that isn't supported by the bundled rules, and then they sometimes turn a corresponding bundled rule off. It would be very difficult to create custom rules if they had to care about all the combinations of other rules that might be enabled.
• Every rule is independently configurable, so it's not safe to assume that a given rule is configured just because a different rule is configured.

There are a few more details I could go into on this point; to summarize, I think it's extremely unlikely that we will make a change that allows rules to see the configuration of other rules. If we're going to resolve the usability issue you've brought up with function-paren-newline, I think we should consider alternative solutions that wouldn't require this.

I should mention that we do have a settings configuration object, which gets used by plugins for project-wide configuration settings. For example, users of eslint-plugin-react can use this value to declare the version of React that they're using, which affects multiple rules in the plugin. In principle, we could tell users to put their line length preference in that object somewhere, and then have core rules read from that object. (However, I don't think this would solve the problem here, given that line length is not the only thing we would need to measure -- see #7526 (comment).)

Here's the crux: I think it's fair to expect max-len to almost always be configured.

This isn't really true.

• Many projects only use ESLint to detect errors, and they don't use it to enforce consistent formatting. (For example: anyone using create-react-app without ejecting)
• Many projects only use ESLint to detect errors along with a few "very common" stylistic preferences, such as indentation and quote style, without requiring that the rest of the code formatting is consistent. (For example: anyone using a config generated with eslint --init without enabling additional rules)
• Many projects use ESLint to enforce a lot of stylistic rules, but they don't use max-len because they don't want to enforce line length as a number of characters. Instead they use other rules such as max-statements-per-line and object-curly-newline, in combination with code review, to make sure lines aren't too complex. (ESLint itself currently does this, although we might enable max-len soon anyway.
• Some projects use customized versions of core rules like max-len, and so they use a modified rule that enforces line length rather than the built-in max-len rule. (eslint-plugin-babel has a few variants of core rules for custom syntax, although it doesn't have one for max-len.)

In general, I think it's definitely not safe to assume that a user has expressed a preferred line length in their config.

This is one of the things that Prettier got spectacularly right from the start. Prettier has subtly changed the way people think about line-length, correctly putting max line-length conceptually "at the top" of the formatting configuration.

Keep in mind that ESLint is a linter, not a code formatter -- its main use case is pointing out potential problems in code, and allowing users to supplement it by adding their own rules. This means that ESLint rules have to make formatting decisions based on an unknown set of preferences that could be modified by the user at runtime. Prettier operates under a different set of design constraints, in that all possible options that could be provided by the user are known at development time. This means Prettier can use the preferred line length (along with some other preferences) to make very sophisticated formatting choices, but it also means that Prettier less configurable than ESLint in that it's not possible to create plugins that make arbitrary modifications to the formatting.

I'm also not really convinced that there is anything special about the max line length preference as opposed to other stylistic preferences, e.g. the choice of whether to put spaces after commas. The latter would cause the length of a line to be modified, which could similarly change the desired formatting for any given maximum line length preference.

[the-noob]

I have this bit from a create-react-app install and after upgrading to 4.10 this fails with multiline

const isLocalhost = Boolean(
  window.location.hostname === 'localhost' ||
  // [::1] is the IPv6 localhost address.
  window.location.hostname === '[::1]' ||
  // 127.0.0.1/8 is considered localhost for IPv4.
  window.location.hostname.match(
    /^127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/,
  ),
);

[noahgrant]

Hi, I understand everyone's arguments in this issue, but I would love an addendum to the multiline rule that would look for newlines inside arguments—similar to array-element-newline (though i wish that rule looked for newlines between elements the way this rule does).

So, I guess I wish both rules met the same criteria for multiline, which would look for newlines inside elements as well as between them. I think that should address all those conflicts with max-len, since it's up to the developer to add the newlines, regardless of line-length. Examples listed in this issue so far would pass, as would these arrays:

// array-element-newline: ['error', 'multiline']
const shortArray = [1, 2, 3];
const longArray = [
  'someReallyLongString',
  'letsImagineThatAllOnOneLineTheseWouldCrossOurMaxLenThreshold',
  'puppies'
];

Does that seem reasonable? Thank you for your time.

[not-an-aardvark]

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get consensus from the team, so I'm closing it. We define consensus as having three 👍s from team members, as well as a team member willing to champion the proposal. This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is consensus among the team that they're worth adding.

Since ESLint is pluggable and can load custom rules at runtime, the lack of consensus among the ESLint team doesn't need to be a blocker for you using this in your project, if you'd find it useful. It just means that you would need to implement the rule yourself, rather than using a bundled rule that is packaged with ESLint.

[noahgrant]

thanks, @not-an-aardvark. for posterity, i did implement my versions of the rules i commented about (array-element-newline and function-paren-newline) in a custom plugin eslint-plugin-sift.