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

Fix FP S4138 ('prefer-for-of'): Check if node does have an iterator to use for-of #3385

Closed
ilia-kebets-sonarsource opened this issue Sep 15, 2022 · 5 comments · Fixed by SonarSource/rspec#1303
Assignees
Labels
type: false positive Issue is reported when it should NOT be
Milestone

Comments

@ilia-kebets-sonarsource
Copy link
Contributor

The rule S4138 can raise issues when for...of is not available for the iterable.

The typescript-eslint rule implementation does not check the type, so we could add a extend the current decorator to filter the flagged nodes for type.

Reported in https://community.sonarsource.com/t/fp-seeking-for-of-where-typescript-prohibits-it/67142

@ilia-kebets-sonarsource ilia-kebets-sonarsource added the type: false positive Issue is reported when it should NOT be label Sep 15, 2022
@yassin-kammoun-sonarsource yassin-kammoun-sonarsource added this to the 9.9 milestone Sep 22, 2022
@yassin-kammoun-sonarsource yassin-kammoun-sonarsource changed the title FP S4138: check if node does have an iterator to use for-of Fix FP S4138 ('prefer-for-of'): Check if node does have an iterator to use for-of Sep 28, 2022
@ilia-kebets-sonarsource
Copy link
Contributor Author

Goal: don't report if arrayLike does not support for-of (flagged by TS)

We are sure that HTMLFormElement has the [Symbol.iterator] generator function (see typedef in Refs below), but when loading the required libs in the tsconfig compilerOptions, we get the following error:

index.ts:3:22 - error TS2495: Type 'HTMLFormElement' is not an array type or a string type.

3   for(const input of form) {
                       ~~~~

Therefore there must be another property that typescript looks for in order to allow the usage of for-of.

Using the typechecker, we can find that arrayLikes that implement the iterable protocol have a property __@iterator@XX where "XX" is a number. It is:

  • 10 for arrays
  • 26 for HTLMFormElement

See code in https://github.com/SonarSource/SonarJS/pull/3423/files#diff-adf2d54f95549748d4e372051a31e8a553506d508fdac5c884002cd01cac3ad7R59

typescript flags for-of when the arrayLike is missing:

  • Symbol.iterator
  • some other field!

We need to find this (or these) other field(s)!

Refs:

{
  "compilerOptions": {
    "lib": [
      "DOM.Iterable",
      "DOM"
    ]
  }
}
  • The test code: index.ts
function foo(form: HTMLFormElement) {
  const disabledFields: Element[] = [];
  for(const input of form) {
    if(input.hasAttribute('disabled')) {
      disabledFields.push(input);
    }
  }
  return disabledFields;
}

@ilia-kebets-sonarsource ilia-kebets-sonarsource removed their assignment Sep 29, 2022
@victor-diez-sonarsource
Copy link
Contributor

I tested that when adding this to the tsconfig the error disappears:

{
  "compilerOptions": {
    "target": "es6",
  }
}

Source: https://bobbyhadz.com/blog/typescript-type-iterableiterator-is-not-an-array-type

Still, should we rely on user's tsconfig in order to raise or not raise?

@ilia-kebets-sonarsource
Copy link
Contributor Author

ilia-kebets-sonarsource commented Sep 29, 2022

The proposal from the community user who reported the issue is for Sonar product not to lead towards changes that would conflict with TypeScript, even if it is because of an error in the tsconfig.

  1. Do we wish to act like this?
  2. If yes, is it possible to know that our proposed changes do not conflict with TypeScript?
Sonar should not report a TypeScript issue seeking a for-of field in cases where TypeScript prohibits it, 
even if the prohibition is a TypeScript issue associated with not properly including the “dom.iterable” option in the “lib” compiler option.

@ilia-kebets-sonarsource
Copy link
Contributor Author

Closing this as we will modify the message in rspec: SonarSource/rspec#1303

@wbt
Copy link

wbt commented Oct 3, 2022

Can Sonar ask TypeScript if there is an iterator, and not require one if TypeScript says no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment