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

Bug: [prefer-for-of] triggered for loops that can not be converted #8984

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

Bug: [prefer-for-of] triggered for loops that can not be converted #8984

yohny opened this issue Apr 24, 2024 · 4 comments
Labels
awaiting response Issues waiting for a reply from the OP or another party bug Something isn't working 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 maintainers 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 maintainers 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Issues waiting for a reply from the OP or another party bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

4 participants