tools: remove no-let-in-for-declaration lint rule

[TimothyGu]

After the upgrade of V8 to a version powered by TurboFan, the
recommendation not to use let for for loops is obsolete.

Checklist

• commit message follows commit guidelines

Affected core subsystem(s)

tools

[BridgeAR]

LGTM but do you have a reference to something that describes that this became unnecessary?

[TimothyGu]

@BridgeAR Well there's https://v8project.blogspot.com/2017/05/launching-ignition-and-turbofan.html... Not sure if there's anything more I could use.

[apapirovski]

Btw slight typo in the description (and commit):

the recommendation not to use let for for loops is obsolete.

(the above has var) Just so no one is confused about what this is addressing.

[BridgeAR]

I fixed the typo in the description.

The reference to the launch of ignition and turbofan is not a proper sign that let is not causing any trouble anymore though. See 55f9c85 as a example that even const had issues until now.

@bmeurer can you give us any further insight if there are any potential deopts left or is let definitely on par with var?

[lpinca]

The reference to the launch of ignition and turbofan is not a proper sign that let is not causing any trouble anymore though. See 55f9c85 as a example that even const had issues until now.

If I remember correctly this was before v8 6.0 but to be honest I don't know if everything has been resolved in v8 6.0.

[TimothyGu]

See 55f9c85 as a example that even const had issues until now.

👍 to @lpinca's comment. It was intended to be a hot patch until Crankshaft is no longer used as the main optimizing compiler. The effects of that patch has also been cancelled by 1b54371.

[BridgeAR]

The effects of that patch has also been cancelled by 1b54371.

That was only one of two of those.

It was intended to be a hot patch until Crankshaft is no longer used as the main optimizing compiler.

When reading the description of the mentioned commit it does not sound like that to me.

With the new Ignition+TurboFan pipeline, the instanceof fast-path can be
missed if the right-hand side needs a TDZ check, i.e. is const declared
on a surrounding scope. This doesn't apply to Node 8 at this point,
where it's at V8 5.8, but it applies as soon as V8 5.9 rolls. There's
work going on in Ignition (and TurboFan) to optimize those TDZ checks
properly, but those changes will land in V8 6.1, so might not end up in
Node 8.

[TimothyGu]

It was intended to be a hot patch until Crankshaft is no longer used as the main optimizing compiler.

When reading the description of the mentioned commit it does not sound like that to me.

Oops, misread the description. We are at 6.1 now, but let's wait until @bmeurer can explain the performance implication a bit more clearly.

[bmeurer]

for (let i...) and for (var i...) have different semantics, especially if i is used in a closure or otherwise escapes, like:

for (let i = 0; i < 1000; ++i) {
  array.forEach(o => o.i += i);
}

This will be slow even with TurboFan. And it also has a different meaning than if you'd use var.

So it might be a good idea to keep the rule in place, even if only to prevent the bugs.

[TimothyGu]

I think @bmeurer has made a good enough case against this, especially if escape analysis is to be disabled momentarily. Thus, closing for now.