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

function-call-argument-newline complains in some cases when it logically shouldn't. #12123

Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@Ferroin
Copy link

Ferroin commented Aug 19, 2019

Tell us about your environment

  • ESLint Version: 6.2.0
  • Node Version: 11.14.0
  • npm Version: 6.7.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

This config is the minimum together with the example code below to reproduce the issue.

Configuration
{
    "extends": "eslint:recommended",
    "env": {
        "browser": true
    },
    "rules": {
        "function-call-argument-newline": [
            "error",
            "consistent"
        ]
    }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

This is a minimal reproducer that I've created based on my own experimentation, not the original code that caused the issue:

document.body.addEventListener('scroll', function() {
    // do something
})

document.body.addEventListener('scroll', function() {
    // do something
}, {
    passive: true,
})

document.body.addEventListener('scroll', function() {
    // do something
}, {passive: true})
npx eslint test.js

What did you expect to happen?

In consistent mode, none of the three calls to document.body.addEventListener() above should trigger an error (the use of newlines between arguments is consistent in all three cases).

What actually happened? Please include the actual, raw output from ESLint.


/mnt/sync/eslint-test/test.js
   7:3  error  There should be no line break here  function-call-argument-newline
  13:3  error  There should be no line break here  function-call-argument-newline

✖ 2 problems (2 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.

Are you willing to submit a pull request to fix this bug?

I don't have enough background with Node.js to be comfortable submitting a PR, but I will be more than happy to help test any that does get submitted.

@Ferroin Ferroin added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Aug 19, 2019
@kaicataldo
Copy link
Member

It looks like the rule is expecting the following:

document.body.addEventListener(
    'scroll', 
    function() {
        // do something
    }, 
    {
        passive: true,
    }
);

document.body.addEventListener(
    'scroll', 
    function() {
        // do something
    }, 
    {passive: true}
);

That being said, I do think it's reasonable to configure it to account for multiline arguments in a way that allows for the expected behavior in the bug report. I wonder if that should be an option (something like ignoreMultilineArgs?)

@kaicataldo kaicataldo added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 19, 2019
@Ferroin
Copy link
Author

Ferroin commented Aug 19, 2019

I think an option would indeed be good. What's got me confused here is that if it's just two arguments, it doesn't seem to care for some reason (regardless of whether the first, the second, or both are multi-line arguments).

@kaicataldo
Copy link
Member

Yeah, that does seem like a bug. Here's some more inconsistency in our demo.

@klimashkin
Copy link

Simple lodash _.reduce case failing with the consistent option:

Screen Shot 2019-08-26 at 3 35 29 PM

@mdjermanovic
Copy link
Member

This isn't related directly to the consistent option, e.g. this is also an error:

/*eslint function-call-argument-newline:["error", "never"] */

foo({
}, bar);

and is reporting a line break between , and bar (Demo)

The issue is with comparing first tokens instead of last vs first (there is also a similar but not same issue with multiline tokens because it compares start with start).

I believe this could be seen as a bug, because:

This rule enforces line breaks between arguments of a function call.
"always" (default) requires line breaks between arguments
"never" disallows line breaks between arguments
"consistent" requires consistent usage of line breaks between arguments

If fixed to compare last to first (and end to start) all examples from this issue should be solved, but the following would be a new error:

/*eslint function-call-argument-newline:["error", "always"] */

foo({
}, bar);

nwoltman added a commit to nwoltman/eslint-config that referenced this issue Sep 21, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Sep 27, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mdjermanovic
Copy link
Member

Reopening because multiple team members think that this could be a bug.

@mdjermanovic mdjermanovic reopened this Sep 27, 2019
@mdjermanovic mdjermanovic removed the auto closed The bot closed this issue label Sep 27, 2019
@kaicataldo
Copy link
Member

kaicataldo commented Sep 28, 2019

This seems like a bug in the implementation to me and I support the change @mdjermanovic suggests in this comment. I think it makes a lot of sense to instead check between the end of the previous argument and the start of the next argument.

@mdjermanovic
Copy link
Member

By #10323 it seems that function-call-argument-newline should have a behavior similar to the behavior of array-element-newline, which indeed compares the last token of an element with the first token of the following element.

These examples are equivalent to the two examples from the initial post, and there are no errors:

/* eslint array-element-newline:["error", "consistent"] */

['scroll', function() {
    // do something
}, {
    passive: true,
}];

['scroll', function() {
    // do something
}, {passive: true}];

Online Demo

Also, function-call-argument-newline with the never option in its current version indirectly disallows newlines inside the arguments (unless it's the last argument). which I believe shouldn't be the responsibility of this rule. The same applies to the consistent option when the first two arguments start on the same line (like in the original example), because it applies never to all following arguments.

I strongly believe this is a bug that should be fixed, though it will generate some new warnings, like in these cases:

/*eslint function-call-argument-newline:["error", "always"] */
foo({
  bar: 1
}, baz);

/*eslint function-call-argument-newline:["error", "consistent"] */
foo(bar,
    function (){
      // do something
    }, {
        a: 1  
    }
);

function-call-argument-newline at the moment doesn't generate warnings in the above cases, but it's probably also a bug since array-element-newline does (Online Demo 1, Online Demo 2).

@kaicataldo can we already accept this as a bug fix that will generate more warnings by default, or maybe wait for one more vote ?

@kaicataldo
Copy link
Member

I’m in favor of this being a semver-minor bug fix but it would be great if we could get some eyes from the TSC before we accept this.

@mdjermanovic
Copy link
Member

@eslint/eslint-team this is probably a bug, but the fix can generate more warnings by default, so it's better to have an additional confirmation before accepting the issue and the related PR.

@fabiospampinato
Copy link

fabiospampinato commented Oct 14, 2019

Another scenario where this fails is when passing a dependency array to a React hook:

useMemo ( () => { // Fine
} );

useMemo ( () => { // No longer fine
}, [foo] );

@g-plane g-plane added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 17, 2019
@ilyavolodin
Copy link
Member

Agree. This seems like a bug.

@mdjermanovic
Copy link
Member

Marking related PR #12280 as accepted.

btmills pushed a commit that referenced this issue Oct 25, 2019
…) (#12280)

* Fix: false positives on newlines in object/array args (fixes #12123)

* Update: Additional tests for multi-line template string
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 24, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.