Bug: max-lines-per-function changes in eslint 8.4.0

[aarongoldenthal]

Environment

Node version: 16.13.1
npm version: 8.1.2
Local ESLint version: 8.4.0
Global ESLint version: none
Operating System: Windows 10 21H1, Alpine Linux 3.14

What parser are you using?

Default (Espree)

What did you do?

With the changes to max-lines-per-function in v8.4.0 per #15140, this rule cannot be disabled at the start of the function with // eslint-disable-next-line max-lines-per-function, which did work in previous versions and has made this a breaking change. The rule now flags this disable as unused since no problems are reported for max-lines-per-function on this line, and flags an error on the first line that exceeds the line length limit.

Configuration

module.exports = {
    env: {
        es6: true,
        node: true
    },
    extends: ['eslint:recommended'],
    reportUnusedDisableDirectives: true,
    rules: {
        'max-lines-per-function': ['error', { 'max': 5}]
    }
};

// eslint-disable-next-line max-lines-per-function -- flagged as unused
const foo = () => {
    const x = 1;
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x); // error is flagged here and subsequent lines
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
};

What did you expect to happen?

// eslint-disable-next-line max-lines-per-function should disable the rule for a function exceeding the max lines, and should not be listed as unused

What actually happened?

// eslint-disable-next-line max-lines-per-function before the function is listed as unused. The only way to disable for the function appears to be wrapping it with disable/enable comments, as shown below. This limits the options to disable the rule, e.g. you might want to disable for a factory function, but enable the rule for any internally defined functions, which isn't an option with this disable.

/* eslint-disable max-lines-per-function */
const foo = () => {
    const x = 1;
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
};
/* eslint-enable max-lines-per-function */

Participation

• I am willing to submit a pull request for this issue.

Additional comments

No response

[mdjermanovic]

Hi @aarongoldenthal, thanks for the issue!

// eslint-disable-next-line max-lines-per-function before the function is listed as unused. The only way to disable for the function appears to be wrapping it with disable/enable comments, as shown below. This limits the options to disable the rule, e.g. you might want to disable for a factory function, but enable the rule for any internally defined functions, which isn't an option with this disable.

You can also add a // eslint-disable-line max-lines-per-function comment on the line where the error is reported (the line that exceeds the limit):

const foo = () => {
    const x = 1;
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x); // eslint-disable-line max-lines-per-function
    console.log(x);
    console.log(x);
    console.log(x);
    console.log(x);
};

Would that work for you?

[aarongoldenthal]

@mdjermanovic The place I really see it a lot is in tests, like the example below. I'd like to disable the rule for the describe function, but leave for the test (it) functions. Previously the // eslint-disable-next-line could be put before the describe and it was obvious where it was applicable. If I use the // eslint-disable-line option, it does disable for the describe, and will still flag for tests that exceed the limit, but it ends up in the middle of a test and because it's now nested in functions it's not obvious where it applies.

describe('test something', () => {
    it('should...', async () => {
        ...
    });

    it('should...', async () => {
        ...
    });

    it('should...', async () => {
        ...
    });

    it('should...', async () => {
        expect.hasAssertions(); // eslint-disable-line max-lines-per-function
        ...
    });
});

It is definitely somewhat of a fringe case, so maybe this is the best option, but in nested function cases like this it's not so intuitive. I do like that it now flags only the lines past the threshold, although it did surprise me when a minor release caused eslint to fail on a lot of projects.

[mdjermanovic]

I'd like to disable the rule for the describe function, but leave for the test (it) functions. Previously the // eslint-disable-next-line could be put before the describe and it was obvious where it was applicable. If I use the // eslint-disable-line option, it does disable for the describe, and will still flag for tests that exceed the limit, but it ends up in the middle of a test and because it's now nested in functions it's not obvious where it applies.

Indeed, it seems that in cases with inner function neither /* eslint-disable */ nor // eslint-disable-line are ideal.

a minor release caused eslint to fail on a lot of projects

This is also reported in #15098 (comment).

@eslint/eslint-tsc thoughts about this?

[alan-agius4]

Indeed we are experiencing the same issue here. And I agree with @ljharb that this is a breaking change since it's a change in behaviour in a minor version.

[nzakas]

Given that there are so many edge cases we didn’t see during development, it seems best to roll this change back.

[mdjermanovic]

Agreed. PR to revert the change: #15397