Node: Implement automatic fixes for our custom eslint rules

[joyeecheung]

• Version: master
• Subsystem: tools

The rules are in https://github.com/nodejs/node/tree/master/tools/eslint-rules
Some of them should be possible to get fixed automatically e.g. to fix require-buffer simply put a line below 'use strict' and stuff.

ESLint documentation on how to implement custom fixes: https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

[shobhitchittora]

Hi @joyeecheung. I'd like to pick this up as a good first issue. I've already forked, built and tested the master. Please provide further steps to test my fixer methods. Any guidance is more than appreciated.

[gibfahn]

@shobhitchittora a good way to get started would be to:

• pick a rule in https://github.com/nodejs/node/tree/master/tools/eslint-rules.
• Run the linter command*, hopefully it passes
• Modify the node source code so that the linter command fails with the rule you picked
• Try the lint command with --fix on the end and see if it fixes the problem (it probably won't).
• Read the eslint docs about writing error fixes
• Implement the automatic fix (if possible)
• Get lint command + --fix to pass
• Open a PR with your changes (see the contributing guide) for more info.

* Command is:

./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md benchmark doc lib test tools

Feel free to comment here if you run into any issues.

BTW the linter command is what gets run when you run make lint (which calls lint-js):

https://github.com/nodejs/node/blob/46ca1775a31d5e76a9f00f641c624c720a6468ab/Makefile#L994-L1000

[shobhitchittora]

Thanks for the help. I'm working on writing fixture functions and will create a PR.

[MhdTlb]

Can I work on this issue?

[joyeecheung]

@MhdTlb Of course! You can try to fix one that has not been taken yet (searching the name in this thread or the PR filter should probably be enough)

[Bamieh]

Here is a complete list of the custom linters and which fixers have ready PRs for:

• [ ] alphabetize-errors
• [x] buffer-constructor
• [x] crypto-check
• [ ] documented-errors
• [x] inspector-check
• [x] no-let-in-for-declaration
• [x] no-unescaped-regexp-dot
• [x] prefer-assert-iferror
• [x] prefer-assert-methods
• [ ] prefer-common-mustnotcall
• [ ] prefer-util-format-errors
• [x] require-buffer
• [ ] required-modules
• [ ] rules-utils

Please update as needed

[apapirovski]

@joyeecheung and anyone else that is passionate about getting these in, it would be nice to figure out parameters around what a successful fixer can and can't do. Right now we have a bunch of PRs open and in limbo because some of these fixes are actually somewhat complicated and it's hard to tell what the criteria for success look like.

As an example, the prefer-assert-methods will do what it's supposed to but create an additional error if assert is not required. This might be acceptable but I think collaborators have avoided landing the PR because it's unclear and no one is willing to commit one way or another.

[Bamieh]

@apapirovski then maybe these issues should be addressed individually on their respective pull request since not all of them are complicated.

for example, for the require-buffer i have added the "algorithm" in which the fixer works:

• If the file has a 'use strict'; line, add the require after it on a separate line.
• Otherwise add it after the first comment block and before the start of any real code.

[Trott]

As an example, the prefer-assert-methods will do what it's supposed to but create an additional error if assert is not required. This might be acceptable but I think collaborators have avoided landing the PR because it's unclear and no one is willing to commit one way or another.

@apapirovski FWIW, the fixers that come with ESLint can create lint errors and that seems to be considered not-a-defect. For example, many times a fixer will result in a line longer than max-len specifies.

So if the behavior of built-in ESLint fixers is deemed as a reasonable measure for our own fixers, then I think the prefer-assert-methods fix is probably OK.

(I don't think the prefer-assert-methods can cause that problem though because it will only flag a call to an assert function.)

[shobhitchittora]

Will pick up prefer-util-format-errors.

[shobhitchittora]

@gibfahn the script you mentioned above -
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md benchmark doc lib test tools
isn't working with the latest master. Any alternative?

[gibfahn]

@shobhitchittora seems to work okay for me (although the first run took 36s on my machine).

./node tools/node_modules/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md benchmark doc lib test tools

FWIW I got that command by running make lint, however it looks like it's no longer printed by default, so you have to unhide the output and pull out the command from the mass of text:

diff --git a/Makefile b/Makefile
index 547890111b..03158e6200 100644
--- a/Makefile
+++ b/Makefile
@@ -1125,7 +1125,7 @@ run-lint-js-ci = tools/lint-js.js $(PARALLEL_ARGS) -f tap -o test-eslint.tap \
 # On the CI the output is emitted in the TAP format.
 lint-js-ci:
    @echo "Running JS linter..."
-   @$(call available-node,$(run-lint-js-ci))
+   $(call available-node,$(run-lint-js-ci))

 jslint-ci: lint-js-ci
    @echo "Please use lint-js-ci instead of jslint-ci"

So if make lint works, then the command in it should work.

[shobhitchittora]

@gibfahn Seems like that the path to eslint got changed from - tools/eslint/bin/eslint.js to tools/node_modules/eslint/bin/eslint.js. Thanks ! 👍🏻