Skip to content

feat: support TS syntax in no-loop-func #19559

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 7, 2025
Merged

feat: support TS syntax in no-loop-func #19559

merged 7 commits into from
Apr 7, 2025

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Mar 22, 2025 •
edited
Loading

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Refs #19173

It adds support for TypeScript types in no-loop-func.

/*eslint no-loop-func: "error"*/

type MyType = 1;
let someArray: MyType[] = [];
for (let i = 0; i < 10; i += 1) {
    someArray = someArray.filter((item: MyType) => !!item);
}

Is there anything you'd like reviewers to focus on?

  • The rule didn't required any code changes 😄

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Mar 22, 2025
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Mar 22, 2025
@netlify Netlify
Copy link

netlify bot commented Mar 22, 2025 •
edited
Loading

✅ Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 1e74207
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/67ea3735e0e238000879191c
😎 Deploy Preview https://deploy-preview-19559--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@snitin315 snitin315 mentioned this pull request Mar 22, 2025
Open
19 tasks
@snitin315 snitin315 marked this pull request as ready for review March 22, 2025 18:05
@snitin315 snitin315 requested a review from a team as a code owner March 22, 2025 18:05
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Mar 22, 2025
JoshuaKGoldberg
JoshuaKGoldberg reviewed Mar 27, 2025
View reviewed changes
tests/lib/rules/no-loop-func.js Outdated
`,
],
invalid: [
// Invalid TS code
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snitin315 did you mean to push a commit with tests here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah added in 6337f87

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I just noticed that in error message, we repeat the same variable name in few cases - Function declared in a loop contains unsafe references to variable(s) 'i', 'i'.

Is this expected, or should we print i just once?

@mdjermanovic @JoshuaKGoldberg ^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, nice catch. I don't know if it's expected but my personal intuition is that it's a bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tracking: #19586 -> #19595 ✔️

mdjermanovic
mdjermanovic reviewed Mar 27, 2025
View reviewed changes
lib/rules/no-loop-func.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there really no changes needed in the code? Asking because @typescript-eslint/no-loop-func isn't marked as deprecated, and the implementation additionally checks isTypeReference attribute.

Copy link
Contributor Author

@snitin315 snitin315 Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, there might be some missing test cases in typescrip-eslint. Let me check on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this check and still all tests passed in typescript-eslint codebase, @JoshuaKGoldberg This might be dead code. Also I have tests all test cases from the typescript-eslint repo all are passing with this rule's current implementation in js.

Added few more test cases in d1c3bd5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it does look like nothing is necessary. I filed typescript-eslint/typescript-eslint#11015 to double-check on the typescript-eslint side.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the problem for which the extension rule was added (typescript-eslint/typescript-eslint#2448 - false positives on unresolved type references) has been fixed by 42faa17 in the core rule.

@nzakas nzakas moved this from Needs Triage to Implementing in Triage Mar 27, 2025
@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 27, 2025
@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Mar 31, 2025
Closed
2 tasks
mdjermanovic
mdjermanovic approved these changes Apr 2, 2025
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Leaving open for @JoshuaKGoldberg to verify.

JoshuaKGoldberg
JoshuaKGoldberg approved these changes Apr 7, 2025
View reviewed changes
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, glad this really didn't need any source logic changes! 👏

@mdjermanovic mdjermanovic merged commit 9535cff into main Apr 7, 2025
31 checks passed
@mdjermanovic mdjermanovic deleted the ts/no-loop-func branch April 7, 2025 20:00
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Apr 7, 2025
@mdjermanovic mdjermanovic mentioned this pull request Apr 7, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Triage
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@snitin315 @JoshuaKGoldberg @mdjermanovic