Repository navigation
enabling eslint's prefer-const rule #3118
Description
Activity
FWIW I just ran a benchmark on jsperf.com and it looks like
letperforms the same asvarwith v8 4.5.please done use those. they're useless on a benchmark of this scale.
Right now in the general case let performance is comparable to var. But there are specific scenarios where it completely fails and is much slower. One such case is when a class is defined within the same scope where let is used. Here's the generated output from v8's optimizing compiler, and you can see the let case generates 100+ more lines of instructions for the simple for loop: https://gist.github.com/trevnorris/73b0d8c0e831d6e48caa
The argument against this is to slowly move to using const where possible, rather than making a massive diff that screws up git blame. (I am favor of moving slowly to it)
@Fishrock123 makes an appropriate point. This is how we've migrated code in the past. Unless the code was breaking for some reason, we've generally left these types of changes to organically migrate through the code base.
Though how do we then address new PRs? I think it's a deterrent for new contributors when they are barrage with nit style comments. In the past I've made changes to the commits before landing it. Though that has always been for minor things (e.g. trailing whitespace). Not sure how we'd feel about doing the same for the many variable declarations.
If
no-varis off the table (and it sounds like it is, at least for the foreseeable future), turning onprefer-constby itself only flags five lines in four files of the current code base. I'm actually all for turning it on.@Trott could you get us a diff? :)
➜ io.js git:(master) ✗ make lint ./node tools/eslint/bin/eslint.js src lib test --rulesdir tools/eslint-rules --reset --quiet lib/cluster.js 294:12 error `debugPort` is never modified, use `const` instead prefer-const test/parallel/test-buffer-zero-fill-reset.js 17:6 error `ui` is never modified, use `const` instead prefer-const test/parallel/test-http-flush-headers.js 13:6 error `req` is never modified, use `const` instead prefer-const test/sequential/test-child-process-fork-getconnections.js 9:6 error `sockets` is never modified, use `const` instead prefer-const 45:6 error `sockets` is never modified, use `const` instead prefer-const ✖ 5 problems (5 errors, 0 warnings) make: *** [jslint] Error 1 ➜ io.js git:(master) ✗ git --no-pager diff diff --git a/.eslintrc b/.eslintrc index cf1f768..eeaaff4 100644 --- a/.eslintrc +++ b/.eslintrc @@ -70,6 +70,8 @@ rules: # require space after keywords, eg 'for (..)' space-after-keywords: 2 + prefer-const: 2 + # Strict Mode # list: https://github.com/eslint/eslint/tree/master/docs/rules#strict-mode ## 'use strict' on topSince this is almost entirely in tests, I'm +1 for the change.
I have a hard time believing those are the only spots, could it be the rule isn't that good at picking things up?
@Fishrock123 The rule only applies to 'let' declarations and not 'var'. 'const' would change the scope of a 'var' but not a 'let'.) So that's why the number of changes would be small.
The argument against this is to slowly move to using const where possible, rather than making a massive diff that screws up git blame. (I am favor of moving slowly to it)
This is frustrating because it leaves us in the land of the implicit which is terrible unless everyone's on the same page, which we are not. I don't recall a discussion where it was agreed that we were moving to const slowly and now we are in a situation where a subgroup of the collaborators push on this when they are reviewing PRs and others don't leading to uncertainty, like in #2411, which is an unpleasant experience for contributors (and collaborators!).
Either it's explicit and stated somewhere (do we need a styleguide we can bikeshed over?) or it should be left up to contributors and not haggled over by reviewers.
If no-var is off the table (and it sounds like it is, at least for the foreseeable future), turning on prefer-const by itself only flags five lines in four files of the current code base. I'm actually all for turning it on.
I'm +1 for turning
prefer-conston in such situation.Above all, it gives a clear hint to any person who is looking on the code that the variable isn't going to be re-assigned. It has at least that advantage over
var.I'm with @rvagg on this. The current approach is haphazard at best. We need
consistency. I've got no problem with moving to const systematically but if
we're doing so it needs to be something we're all doing.
On Sep 29, 2015 8:58 PM, "Rod Vagg" notifications@github.com wrote:The argument against this is to slowly move to using const where possible,
rather than making a massive diff that screws up git blame. (I am favor of
moving slowly to it)This is frustrating because it leaves us in the land of the implicit which
is terrible unless everyone's on the same page, which we are not. I don't
recall a discussion where it was agreed that we were moving to const slowly
and now we are in a situation where a subgroup of the collaborators push on
this when they are reviewing PRs and others don't leading to uncertainty,
like in #2411 #2411, which is an
unpleasant experience for contributors (and collaborators!).Either it's explicit and stated somewhere (do we need a styleguide we can
bikeshed over?) or it should be left up to contributors and not haggled
over by reviewers.—
Reply to this email directly or view it on GitHub
#3118 (comment).28 remaining items
- added a commit that references this issue
on Oct 29, 2015 - added 2 commits that reference this issue
on Dec 29, 2015 - added 2 commits that reference this issue
on Jan 19, 2016 Given that the codebase will survive multiple versions of V8, how long do we expect to see the let perf hit survive?
We don't code for what will be optimized, but what is optimized. And I have no idea when it will be fixed.
How big is the perf hit, if we take it — do we hit those corner cases? If so, can we rework to avoid those corner cases and add lint rules around them in the meantime?
In for loops it is substantial, and I don't have a conclusive list of every case where this is applicable. As you can see from my test case, the optimizing compiler completely fails. Generating almost twice the number of instructions. This is enough to turn from an annoyance to a blocking issue.
Even if we don't hit a case today, and we don't know all cases where it may appear, what will happen if a change comes in that hits it after this lands?
I'm +1 on making everything const that can be, but not at the cost of forcing let down everyone's throat.
I know this is closed, and there may be a more relevant discussion (someone please link?) but my solution is:
// let is much faster than const in for..of loops // eslint-disable-next-line prefer-constwe should ping folks on the V8 team to get an idea of why this is still an issue. it’s been several years now and this syntax isn’t exactly new. it might just be a matter of adding this case into some of the perf suites JS VM’s are running.
we should ping folks on the V8 team to get an idea of why this is still an issue. it’s been several years now and this syntax isn’t exactly new. it might just be a matter of adding this case into some of the perf suites JS VM’s are running.
It has been resolved for some time. Loops using
lethave comparable performance to loops usingvar.It has been resolved for some time. Loops using let have comparable performance to loops using var.
what about
const?It has been resolved for some time. Loops using let have comparable performance to loops using var.
what about
const?Doh! I did the "see what I expected to see and not what is actually there on the screen in front of me" thing. Sorry. I don't know the answer to that.
It has been resolved for some time. Loops using let have comparable performance to loops using var.
what about
const?Doh! I did the "see what I expected to see and not what is actually there on the screen in front of me" thing. Sorry. I don't know the answer to that.
I don't know what sort of credibility jsperf has with the team here, but I created a test earlier today that addresses this question:
https://jsperf.com/foreach-vs-for-of-performance-test/9
After my initial run,
letwas definitively faster. After testing multiple times, I see variance between test runs. Sometimesletis faster and sometimesconstis faster. It may vary through the amount of processing within the loop?bleh, I'm just a slave to whatever
standardtells me these days
This conversation started here: #3036 (comment).
It seems there's an implicit rule of preferring
constovervarfor variables that hold values that never change, which makes sense. It's been mentioned in #1243, and seems to be enforced during code reviews.However, it's not currently caught by our eslint setup. To paraphrase #3036 (comment), I'm not a big fan of having implicit coding style rules. It becomes frustrating for maintainers to enforce them every time, and for contributors because they don't have any tool to check that their code complies to the guidelines.
However, using the prefer-const rule does not apply to function-scoped variable declarations, so we would need to use the no-var eslint rule too and use
letinstead ofvar, which I assume would require a significant amount of work.#1243 mentions that
let's V8's implementation had performance at some point.My question thus is: is moving from
vartoletand enforcingconstwith eslint worth investigating now?/cc @nodejs/tsc