Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A case not handled by no-for-loop #295

Open
golopot opened this issue May 8, 2019 · 11 comments
Open

A case not handled by no-for-loop #295

golopot opened this issue May 8, 2019 · 11 comments
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted

Comments

@golopot
Copy link
Contributor

golopot commented May 8, 2019

Issuehunt badges

/* eslint unicorn/no-for-loop: 2 */

const arr = [];

for (let i = 0, j = arr.length; i < j; i += 1) {
  const element = arr[i];
  console.log(element);
}

no-for-loop should report on this but it does not.

This slightly unusual pattern is used all over the place in eslint-plugin-react.

This issue is a subtask of #250.

IssueHunt Summary

Sponsors (Total: $40.00)

Become a sponsor now!

Or submit a pull request to get the deposits!

Tips

@sindresorhus
Copy link
Owner

@golopot This was not handled by #297, right?

@golopot
Copy link
Contributor Author

golopot commented May 31, 2019

It is not.

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 31, 2019
@IssueHuntBot
Copy link

@IssueHunt has funded $40.00 to this issue.


@MrHen
Copy link
Contributor

MrHen commented Jun 2, 2019

I took a quick look at this and think this should be a separate rule that auto-fixes this shape into a normal for loop. no-for-loop can then convert the normal for loop since it already handles that.

Original:

for (let i = 0, j = arr.length; i < j; i += 1) {
  const element = arr[i];
  console.log(element);
}

Fixed:

for (let i = 0; i < arr.length; i += 1) {
  const element = arr[i];
  console.log(element);
}

Original:

for (let i = 0, j = arr.length; i < j; i += 1) {
  const element = arr[i];
  console.log(element);
  console.log(j);
}

Fixed:

for (let i = 0; i < arr.length; i += 1) {
  const element = arr[i];
  console.log(element);
  console.log(arr.length);
}

@sindresorhus
Copy link
Owner

sindresorhus commented Jun 10, 2019

@MrHen Are we sure we can depend on the fixer output of other rules (the order of fixes)? I don't fully remember how fixers work. How about a rule named simplify-for-loop?

Personally, I don't want for-loops at all, so I would prefer just keeping the logic in this rule, as I don't really see the point of simplifying a for-loop if it can just be removed altogether.

@MrHen
Copy link
Contributor

MrHen commented Jun 10, 2019

@sindresorhus auto-fix will repeatedly attempt to validate / fix after it applies a fixing. I'm not sure what happens if two things want to fix the same code but I don't think it would matter here because the fixers are not going to immediately overlap.

The main reason I'd want this check in a different rule is because it would make the rule logic and test cases easier to understand and maintain. The end result will be:

for (let i = 0, j = arr.length; i < j; i += 1) {
  const element = arr[i];
  console.log(element);
}

Will be auto-fixed to:

for (let i = 0; i < arr.length; i += 1) {
  const element = arr[i];
  console.log(element);
}

Which is then auto fixed to:

for (const [i, element] of arr) {
  console.log(element);
}

The end result will be exactly what you want.

@sindresorhus
Copy link
Owner

Alright, let's do it as a separate rule then.

@jmoore914
Copy link
Contributor

I can take this one!

@fisker
Copy link
Collaborator

fisker commented Dec 27, 2019

@jmoore914 I thought this is fixed by #476 , but apparently ArrayPattern is missed, I just send a follow up PR #489 fix this.

@sindresorhus @MrHen I missed this issue, when I review #476, we already do destructuring in the for...of.

Update: #489 didn't fix this

@snowteamer
Copy link

Alright, let's do it as a separate rule then.

What about restricting the scope of this proposed new separate rule to only address array length caching ?

  • it would still address the specific OP issue
  • simpler than checking for every possible way of declaring an extra variable
  • array length caching was once a possible performance optimization, but nowadays modern browsers automatically cache it in such for loops, making manual caching an obsolete practice unless targeting old browsers
  • all proposed pass/fail exemples in Add no-for-loop-extra-variable rule #492 already concern a "length" variable anyway
  • maybe also the possibility to name the new rule no-array-length-caching instead of no-for-loop-extra-variable

@sindresorhus
Copy link
Owner

If anyone wants to work on this, please continue where #492 left off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💵 Funded on Issuehunt This issue has been funded on Issuehunt enhancement help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants