Rule Change: Make max-lines-per-function only reports lines that exceed the limit, not all

[alvis]

What rule do you want to change?

max-lines-per-function

What change to do you want to make?

Generate fewer warnings

How do you think the change should be implemented?

A new default behavior

Example code

/* eslint max-lines-per-function: ["error", {"max": 3}] */
export function bulkyFunction() {
  // line 1
  // line 2
  // line 3
}

What does the rule currently do for this code?

Currently it will report the range of line numbers from the beginning of the function to the very last line of it.

What will the rule do after it's changed?

Following the same spirit of max-lines implemented in #13318, we should only report those lines which exceed the limit.
With this example, it should only report line 4 and 5.

Participation

• I am willing to submit a pull request to implement this change.

Additional comments

The current default is annoying because once the function exceeds even a single line, the whole function is highlighted by the editor and other important errors may be suppressed as a result.

[nzakas]

I’d say this is a bug given the different behavior in max-lines, so I’m in favor of bringing this rule into alignment.

[btmills]

Also in favor. #13318 updated max-lines as part of the broader highlight location updates in in #12334, and those were done as semver-minor enhancements, so I'd say this should be done that way as well instead of as a semver-patch bug fix.

[nzakas]

Fair enough. @alvis would you be interested in submitting a pull request for this?

[LilCthulu]

I would love to take this on as first contribution if possible!

[alvis]

@LilCthulu I'd appreciate it very much if you can help ❤️

[nzakas]

@LilCthulu happy to have your help. Please post back here if you have any questions or need some pointers.

[LilCthulu]

@nzakas @alvis Awesome! I'll get started right away. Any specific file to start at? Also anything general to know before I dive in?

[nzakas]

This is the main file to look at:
https://github.com/eslint/eslint/blob/master/lib/rules/max-lines-per-function.js

You’ll also need to update the tests:
https://github.com/eslint/eslint/blob/master/tests/lib/rules/max-lines-per-function.js

The pull request making a similar change to max-lines might be useful as an example:
https://github.com/eslint/eslint/pull/13318/files

[LilCthulu]

@nzakas Thanks! I'll get started right away and keep you posted!

[The-x-Theorist]

I'd like to make my first open source contribution by updating this rule. 😄

[nzakas]

@The-x-Theorist thanks. We already have @LilCthulu volunteering to work on this, so let’s give them the chance to complete the work first.

[The-x-Theorist]

@nzakas it's okay, I will wait. 😃

[nzakas]

@LilCthulu are you still planning on working on this?

[Gautam-Arora24]

@nzakas Can I work on this issue?

[LilCthulu]

@nzakas I am indeed sorry for being slow! Had a bunch of stuff going on.

[nzakas]

@LilCthulu thats okay, we just need regular updates because there are others interested in implementing this. If you can’t get to this soon, please let us know so we can let someone else work on it.

@Gautam-Arora24 right now @LilCthulu is working on this and @The-x-Theorist is next line.

[LilCthulu]

@nzakas Sorry for the delay it is fine if someone else grabs this

[Gautam-Arora24]

@LilCthulu Can I grab this one?

[nzakas]

@Gautam-Arora24 @The-x-Theorist has already submitted a pull request.

[nzakas]

Here it is: #15140

[Gautam-Arora24]

No worries @nzakas. I'll pick some another issue :)

[mdjermanovic]

I have concerns about this change. The starting location of the reported error will be floating inside the function, so it will be basically impossible to use eslint-disable-line or eslint-disable-next-line comments when someone wants to allow a particular function to exceed the limit.

Would it make more sense to report just the header?

[alvis]

Hmm. Not sure if putting eslint-disable-line before the first line of the function works, but users can still wrap the function by the eslint-disable max-lines-per-function and eslint-enable max-lines-per-function pair, which makes it much clearer about the intention.

[nzakas]

I also don’t think the ability to use disable comments should be the deciding factor here. I’d suggest we try out the pull request and see what the UX feels like.

[mdjermanovic]

This change may bring UX improvements in some cases, but I still have the following concerns:

• All existing // eslint-disable-line max-lines-per-functions and // eslint-disable-next-line max-lines-per-functions directives in users' codebases will become unused, and the rule will report errors on those functions after we release this change.
• Wrapping a function in eslint-disable/eslint-enable disables this check in its inner functions as well. There will be no good way to disable this check for a single function only.
• It will still highlight a certain number of lines and thus visually suppress other important errors on those lines, which is the problem this change aims to solve as mentioned in the original issue.
• Highlighting lines that exceed the limit gives a nice visual clue on the number of lines that should be removed, but it also suggests that those are the lines that should be removed, which doesn't have to be the case. All lines equally contribute to the total number of lines, and therefore in my opinion it does make sense to report the whole function.

[nzakas]

These all seem to be the way that max-lines currently works. Am I missing something?

[mdjermanovic]

These all seem to be the way that max-lines currently works.

Indeed, for the last two points. For the same reasons, I'm not sure if reporting extra lines in max-lines was a better choice than, for example, reporting the range of the first source code character.

The difference is in disabling errors. With max-lines, /* eslint-disable max-lines */ or /* eslint max-lines: 0 */ does the work because there is only one error in the file. With max-lines-per-function, there will be no way to disable this rule for just one function.

[alvis]

If I understand the code correctly, instead of reporting node

eslint/lib/rules/max-lines-per-function.js

Lines 195 to 199 in 0d3f95b

	context.report({
	node,
	messageId: "exceed",
	data: { name, linesExceed, maxLines }
	});

we can report loc like in max-line

eslint/lib/rules/max-lines.js

Lines 180 to 187 in 0d3f95b

	context.report({
	loc,
	messageId: "exceed",
	data: {
	max,
	actual: lines.length
	}
	});

Since the input is node in max-lines-per-function, we should be able use // eslint-disable-line max-lines-per-functions just right before the function as in the past. The only thing we need to do is only to compute start and end.

[mdjermanovic]

Since the input is node in max-lines-per-function, we should be able use // eslint-disable-line max-lines-per-functions just right before the function as in the past.

Location of // eslint-disable-line relates to the reported line in the LintMessage. If loc is present in context.report({}), line will be loc.start.line. Only if loc is not present, line will be node.loc.start.line. So, when we add loc, node will not matter for disable comments.

For example, this disable comment:

/* eslint max-lines-per-function: ["error", 5] */

function one() { // eslint-disable-line max-lines-per-function
    two;
    three;
    four;
    five;
    six;
}

would have to be relocated here:

/* eslint max-lines-per-function: ["error", 5] */

function one() {
    two;
    three;
    four;
    five;
    six; // eslint-disable-line max-lines-per-function
}

Then, if anything new is added to the function before that line, the location of the comment must be changed:

/* eslint max-lines-per-function: ["error", 5] */

function one() {
    two;
    three;
    somethingNew;
    four;
    five; // eslint-disable-line max-lines-per-function
    six; 
}

[nzakas]

Can’t people just use a disable comment before the function and then enable after? I don’t think we need to guarantee that every disable directive works in every situation.

[mdjermanovic]

Can’t people just use a disable comment before the function and then enable after?

Yes, that will work. A downside is that it also disables this rule in inner functions. Anyway, since everyone is in favor of this proposal, I agree that we try it out.

Question: do we want to change the error message (as in #15140), or shall we keep the existing one?

[nzakas]

I’m fine either way. There’s something nice about doing the math for users, though.

[btmills]

Thinking about this from the user's perspective, seeing editor squigglies only on the excess lines would be much nicer than squigglying the whole function. The tradeoff that disable-(next-)line comments will have to be moved might be seen as a feature: previously, disabling this rule for a function was binary. Now, someone could disable it on just the n+1th line, but adding an n+2nd line would still error. Block comments are still available to disable the rule for an entire function.

[ljharb]

#15140 (comment)

This seems like a bit of a breaking change.

It's causing my eslint-disable comments - that have always been positioned at the top of the function - to be considered unused, and instead the random line that happens to break the threshold is reported on.

(basically, what's described in #15098 (comment))

Can it be reverted please? I expect any rule named "per-function" to always apply to the entire function.

[mdjermanovic]

It's causing my eslint-disable comments - that have always been positioned at the top of the function - to be considered unused, and instead the random line that happens to break the threshold is reported on.

Can you move the eslint-disable-line (or eslint-disable-next-line) comment to the line where the error is reported?

Or, add /* eslint-disable max-line-per-function */ before and /* eslint-enable max-line-per-function */ after the function.

[ljharb]

I can, but “i have to move code” means it’s a breaking change.

[snitin315]

Should we reopen this issue to track it for v9? The change was reverted in #15397

[mdjermanovic]

Should we reopen this issue to track it for v9?

It seems better not to implement this change at all, for the reasons described in #15390 and in this issue.