-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Promote js/loop-iteration-skipped-due-to-shifting
to the Code Quality suite
#19743
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR promotes the js/loop-iteration-skipped-due-to-shifting
query to the Code Quality suite, refines its false-positive logic, and adds corresponding tests.
- Updated query metadata and suite inclusion for Code Quality promotion
- Excluded splice calls used in conditional guards from false positives
- Extended tests and expected results for both true positives and a retained false positive
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js | Added test functions covering try/catch, logical-and, and if‐guards |
javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected | Added expected alerts for new true-positive cases |
javascript/ql/src/change-notes/2025-06-12-loop-iteration.md | Documented addition of reliability tag |
javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md | Noted fix for false positives when splice is used in conditions |
javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql | Changed tags to quality, added guard to skip splice-in-condition |
javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected | Included the query in the JavaScript Code Quality suite |
Comments suppressed due to low confidence (2)
javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql:9
- The
reliability
andcorrectness
lines aren’t prefixed with@tags
, so they may not be recognized. Consider combining all tags on one annotation, e.g.* @tags quality reliability correctness
.
* reliability
javascript/ql/src/change-notes/2025-06-12-loop-iteration.md:4
- [nitpick] Since the PR also promotes this query to the Code Quality suite, consider adding a bullet note about that promotion to keep change-notes in sync with the integration-test update.
* The `js/loop-iteration-skipped-due-to-shifting` query has been updated with `reliability` tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments and suggestions, otherwise this looks good to me! 👍
javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js
Show resolved
Hide resolved
javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Taus <[email protected]>
Co-authored-by: Taus <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In it goes.
The following pull request makes changes to
js/loop-iteration-skipped-due-to-shifting
:splice
is used as a condition that adjusts the loop counter.MRVA shows good results (61 out of which most are true positives).
Autofix
seems to be able to do the job, it even marked the false positive which I have mentioned before as false positive 🤯