Skip to content

Docs: [prefer-for-of] mention non-array loops that cannot be converted #8984

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

Open
4 tasks done
yohny opened this issue Apr 24, 2024 · 12 comments
Open
4 tasks done

Docs: [prefer-for-of] mention non-array loops that cannot be converted #8984

yohny opened this issue Apr 24, 2024 · 12 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@yohny
Copy link

yohny commented Apr 24, 2024

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.4.5&fileType=.ts&code=MYewdgzgLgBADgQwE4QKZJgXhmVB3GAEQHkBZABWTSQAoBKAbgFgAoUSWRFVAEy3iroAdFzQAxJCAC2AZShIAlmADmNAOQRpqGAA8pAGzUAaGGqiodUAPR7DjVu2gwkqCAFd9UAHIger-qK8QsqoUACi%2BqhSqGBQEABCAJ4AKgjKXgjR6prRAPpgvqhq9iysVlYwAAJQiXCuwIpwUAC0rvpK1nAuAGbozd0gSM0g3TCgUnD6CEoQMFAAFgiwAEao%2BiAE6yBwMBDzIB58q86okwjAvDDLiTADQyOsdzA0kbAK-AAMDDDvADwn7k8Pj8ECEkRUC2%2BCgA1NC6DAAN6sGAwcq7KBubrdVgAX1YZQqyzcsAWSxgPBA-gKsFQADcYmNpHAFJErqhgAg3GgYAAJZKkAAyAGEQPpIsAoApwMRRhSqSBYIt6TAANoyRJSZaioQKcxIJaDAC6j0Gz0csCQYBgIwBHm8hQg8KRLBRaOgmOxLDxpRYQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0Wlq4D2fAV3RRe0IZHBgAviFlA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eFYDArugDTg2SrIARqwDavGlHQkATAAZp0yDwm0AJgkjiwAXV4BfEHqA&tokens=false

Repro Code

const parser = new DOMParser();
const parsed = parser.parseFromString('some xml', 'text/xml');
const resultNodes = parsed.getElementsByTagName('some_node');

// @typescript-eslint/prefer-for-of complains that below loop should be replaced by for-of
for (let i = 0; i < resultNodes.length; i++) {
  // stuff
}

// but that does not even compile because HTMLCollectionOf does not have [Symbol.iterator]
for (const rn of resultNodes) {
  // stuff
}

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
     "@typescript-eslint/prefer-for-of": "error"
  },
};

tsconfig

{
  "compilerOptions": {
    "strict": true,
    "lib": [
      "es2022",
      "dom"
    ]
  }
}

Expected Result

The rule suggesting the conversion to for-of loop should not trigger in this case

Actual Result

Rule triggers, suggesting uncompilable conversion

Additional Info

No response

@yohny yohny added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Apr 24, 2024
@kirkwaiblinger
Copy link
Member

I'm curious - are you able to use dom.iterable? I would think any es2022 target should support it, but I've never been totally certain on when you can and can't use dom.iterable/dom.asynciterable.

@kirkwaiblinger kirkwaiblinger added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Apr 25, 2024
@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Apr 25, 2024

Forgot to include playground link with dom.iterable added to the config. It will get rid of the TS complaint for you 🙂 so the question is just whether you plan to run code in an environment that would support it

@Josh-Cena
Copy link
Member

This rule is ported from tslint. It's one of the few rules that (a) doesn't handle TS specific syntax (b) doesn't use type information to check JS patterns. I think this rule ought to use type information because there could be many APIs that are indexable but not iterable—I've always been surprised that this rule false-positives so much but people have never complained.

@bradzacher
Copy link
Member

there could be many APIs that are indexable but not iterable

Theoretically - yes.
In reality - no.

IIRC the rule looks for an indexer using the loop variable and the loop variable being incremented and the loop variable compared against a .length property.

So you need someone to build an API that exposes both contiguous numeric properties AND a .length property.
In reality this doesn't happen because ofc people just use an array.

The DOM pseudo-arrays are the only example I know of and that happens purely because legacy reasons.

@Josh-Cena
Copy link
Member

Josh-Cena commented Jun 1, 2024

So you need someone to build an API that exposes both contiguous numeric properties AND a .length property.

That's the definition of array-like objects. There are many non-array array-like objects, including typed arrays, DOM arrays, arguments, etc. It just happens that almost all array-like objects are also iterable because it's a really low-cost thing to do (you just add [Symbol.iterator]: Array.prototype.values). Nevertheless this rule in its current form just isn't safe, and as I've illustrated above its state is really odd.

@bradzacher
Copy link
Member

It just happens that almost all array-like objects are also iterable because it's a really low-cost thing to do

Which is my point.
In theory it's a large hole - one could define an array-like without an iterator!

In reality
(a) people don't define APIs like that (they just define arrays because who the heck is insane enough to be inventing their own array-like data structure for their library?)
(b) the legacy array-like APIs are all (???) iterable.

So it's a teeny tiny hole that's very very unlikely to occur. Theoretically bad, Realistically fine.

@kirkwaiblinger
Copy link
Member

My 2c

ACK that it's a weird rule to be in typescript-eslint. Seems like in its present implementation it really could/should just be in eslint core.

I'm always very sympathetic to edge-case correctness, but

  1. this has the downside that it would require type info to fix
  2. The existing bug has the benefit that it might make someone more likely to land on this issue or a docs page, where they may learn to add dom.iterable to the config, which is a better fix anyway (maybe we should add it to the doc page for this rule, too!).

So, I'm -0.1 on fixing this, unless we know of some examples in the wild that don't have Symbol.iterator and aren't fixed by dom.iterable.

@bradzacher
Copy link
Member

I think it's okay to just document as a known limitation and move on given how rare the circumstances are. We can include the note about the lib as a solution for the DOM usecase.

@Josh-Cena
Copy link
Member

OK fair. Not sure why this rule should remain in our plugin though🤷‍♂️

@bradzacher
Copy link
Member

It's a stylistic concern - not a correctness one.
Additionally it does have the chance of false positives.
So it's unlikely that eslint core would want it.

So either we pass it off to a catch-all plugin like unicorn or we just keep the status quo. I don't see any real benefit to the user pain of moving it.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jun 2, 2024

So it's unlikely that eslint core would want it.

Interestingly, we've had a bit more willingness than ESLint core to have non-formatting somewhat-mostly-stylistic rules (https://typescript-eslint.io/troubleshooting/formatting#eslint-core-and-formatting). prefer-for-of requires type checking and is generally applicable to all kinds of JavaScript projects.

We mirror the ESLint team's move away from formatting and stylistic rules. With the exception of bug fixes, no new formatting- or stylistic-related pull requests will be accepted into typescript-eslint.

A few references of other rules that could be for JavaScript in general, but need type information:

Similar to #8792, we don't really have an ideal next place for prefer-for-of & co. - even if we were happy with the user pain of moving it.

@JoshuaKGoldberg
Copy link
Member

I think it's okay to just document as a known limitation and move on given how rare the circumstances are. We can include the note about the lib as a solution for the DOM usecase.

👍 Accepting PRs as a documentation issue.

@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: [prefer-for-of] triggered for loops that can not be converted Docs: [prefer-for-of] mention non-array loops that can not be converted Aug 19, 2024
@JoshuaKGoldberg JoshuaKGoldberg added documentation Documentation ("docs") that needs adding/updating accepting prs Go ahead, send a pull request that resolves this issue and removed bug Something isn't working awaiting response Issues waiting for a reply from the OP or another party labels Aug 19, 2024
@kirkwaiblinger kirkwaiblinger changed the title Docs: [prefer-for-of] mention non-array loops that can not be converted Docs: [prefer-for-of] mention non-array loops that cannot be converted Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants