Enable the ESLint no-shadow rule
#11566
Conversation
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/ee79194e2dc1060/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/53caf0b0ec56b40/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/ee79194e2dc1060/output.txt Total script time: 2.26 mins
Image differences available at: http://54.215.176.217:8877/ee79194e2dc1060/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/53caf0b0ec56b40/output.txt Total script time: 19.52 mins
Image differences available at: http://54.67.70.0:8877/53caf0b0ec56b40/reftest-analyzer.html#web=eq.log |
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/e80f9f69455c7c8/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/e80f9f69455c7c8/output.txt Total script time: 1.72 mins Published |
Snuffleupagus
force-pushed
the
no-shadow
branch
from
addeab9
to
d43202d
Compare
Snuffleupagus
force-pushed
the
no-shadow
branch
5 times, most recently
from
9207c17
to
0ab57bd
Compare
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/4379cee6dd6b722/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/9f230e38c9a71d3/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/9f230e38c9a71d3/output.txt Total script time: 26.39 mins
Image differences available at: http://54.215.176.217:8877/9f230e38c9a71d3/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/4379cee6dd6b722/output.txt Total script time: 60.00 mins |
|
/botio-linux test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/d5f5ffb2159afbe/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/d5f5ffb2159afbe/output.txt Total script time: 19.55 mins
Image differences available at: http://54.67.70.0:8877/d5f5ffb2159afbe/reftest-analyzer.html#web=eq.log |
Snuffleupagus
force-pushed
the
no-shadow
branch
5 times, most recently
from
8d298b6
to
871528f
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/191faa6430a5002/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/b1b3f04631758be/output.txt Total script time: 60.00 mins |
|
/botio-windows test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/840338d8f7a669a/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/840338d8f7a669a/output.txt Total script time: 60.00 mins |
|
/botio-windows test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 1 Live output at: http://54.215.176.217:8877/0da03101e85d78e/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.215.176.217:8877/0da03101e85d78e/output.txt Total script time: 60.00 mins |
|
@brendandahl Could you kick the Windows bot again? It seems to stop relatively often lately due to not being able to remove a file. |
Snuffleupagus
force-pushed
the
no-shadow
branch
from
871528f
to
23de690
Compare
Snuffleupagus
force-pushed
the
no-shadow
branch
from
23de690
to
79bc34f
Compare
…d/util.js` Rather than duplicating the lookup and caching in multiple files, it seems easier to simply move all of this functionality into `src/shared/util.js` instead. This will also help avoid a bunch of ESLint errors when the `no-shadow` rule is enabled.
Trying to enable the ESLint rule `no-shadow`, against the `master` branch, would result in a fair number of errors in the `Glyph` class in `src/core/fonts.js`. Since the glyphs are exposed through the API, we can't very well change the `isSpace` property on `Glyph` instances. Thus the best approach seems, at least to me, to simply rename the `isSpace` helper function to `isWhiteSpace` which shouldn't cause any issues since it's only used in the `src/core/` folder.
…ted cases Given the way that classes was/is implemented, using closures, there'll be a fair number of false positives when the `no-shadow` ESLint rule is enabled. Note that while *some* of these `eslint-disable` statements can be removed if/when the relevant code is converted to proper `class`es, we'll probably never be able to get rid of all of them given our naming/coding conventions (however I don't really see this as a problem).
This rule is *not* currently enabled in mozilla-central, but it appears commented out[1] in the ESLint definition file; see https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#238-239 Unfortunately this rule is, for fairly obvious reasons, impossible to `--fix` automatically (even partially) and each case thus require careful manual tweaks. Hence this ESLint rule is, by some margin, probably the most difficult one that we've enabled thus far. However, using this rule does seem like a good idea in general since allowing variable shadowing could lead to subtle (and difficult to find) bugs or at the very least confusing code. *Please note:* Possibly these changes ought to have been split into *multiple* commits, e.g. one per directory, so if this is considered "un-reviewable" as-is I can try to do that. Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-shadow
Snuffleupagus
force-pushed
the
no-shadow
branch
from
79bc34f
to
9c8a5fd
Compare
|
/botio lint |
From: Bot.io (Windows)ReceivedCommand cmd_lint from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/541897c493eb3ea/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_lint from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3aedc58a70bd746/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/3aedc58a70bd746/output.txt Total script time: 1.11 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/541897c493eb3ea/output.txt Total script time: 2.74 mins
|
|
In order to get this merged, I'd like to say that the smaller chunks really helped for me to review it. I'm hoping this can be split up even more so we can enable this rule, because shadowing can be an annoying source of bugs. |
My intention is to split this into one PR per directory, but I've not yet had time to do that; hopefully soon though :-) |
This rule is not currently enabled in mozilla-central, but it appears commented out[1] in the ESLint definition file; see https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#238-239
Unfortunately this rule is, for fairly obvious reasons, impossible to
--fixautomatically (even partially) and each case thus require careful manual tweaks.Hence this ESLint rule is, by some margin, probably the most difficult one that we've enabled thus far. However, using this rule does seem like a good idea in general since allowing variable shadowing could lead to subtle (and difficult to find) bugs or at the very least confusing code.
Please note: Possibly these changes ought to have been split into multiple commits, e.g. one per directory, so if this is considered "un-reviewable" as-is I can try to do that.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-shadow
[1] This suggests, at least to me, a desire to possibly enable it at some future point. Most likely, a very large number of failures currently prevent doing so.