-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Mass promotion of queries to quality
status
#19776
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
base: main
Are you sure you want to change the base?
Conversation
7686f1f
to
69734a8
Compare
69734a8
to
0c31838
Compare
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 various JavaScript CodeQL queries to quality status by adding the @tags quality
annotation and updating related tags, and updates the integration-tests suite to include all newly promoted queries.
- Bulk update: Added
@tags quality
and refined tag lists in Declarations, DOM, AngularJS, and other query modules. - Test suite update: Extended
javascript-code-quality.qls.expected
to cover all newly promoted queries.
Reviewed Changes
Copilot reviewed 96 out of 96 changed files in this pull request and generated no comments.
File | Description |
---|---|
javascript/ql/src/Declarations/* | Added @tags quality and adjusted tags on declaration queries |
javascript/ql/src/DOM/* | Added @tags quality and refined tags on DOM queries |
javascript/ql/src/AngularJS/* | Added @tags quality and refined tags on AngularJS queries |
javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected | Updated expected suite list to include all promoted queries |
Comments suppressed due to low confidence (1)
javascript/ql/src/DOM/PseudoEval.ql:11
- The CWE identifier was changed from
cwe-676
tocwe-67
, which appears incorrect. Please restore the correct identifier (e.g.,external/cwe/cwe-676
).
* external/cwe/cwe-67
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.
Part 1 - more review to follow. I got as far as javascript/ql/src/Expressions/UnclearOperatorPrecedence.qhelp
* correctness | ||
* @tags quality | ||
* maintainability | ||
* readability |
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.
I think this should be correctness
, because in the majority of cases it indicates a copy-paste error/typo, and the user will adjust the condition rather than deleting that branch. That's how we've tagged this query in go, which has the same query. And it matches how this query was tagged before this PR.
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.
It feels like a bit of a gray area. When you look at the top-level categories, you could make a case for either. Sometimes, duplicate conditions hint at a copy-paste mistake (which could affect correctness), but not always. To me, it felt more like this falls under for queries that detect patterns that make it harder for developers to make changes to the code
rather than for queries that detect issues that affect whether the code will perform as expected during execution
.
That’s why I ended up leaning more towards @tags maintainability
and specifically @tags readability
.
I actually debated between @tags useless-code
and @tags readability
, and I thought I’d get more comments here. 😄
But I really don’t mind changing it to correctness
and reliability
, we’ll probably change these tags quite a bit over time anyway, depending on feedback. :)
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.
We agreed that when something matched maintainability
and reliability
we would choose reliability
, since that is a higher priority for users.
I think we have far more correctness
bugs than anything else because that's what users care about - an alert saying "you should change the name of this class" is annoying, but an alert saying "this code doesn't make a lot of sense, look again because there's probably a copy-paste error" is actually going to catch quite a few bugs.
If you don't mind changing it then I think you should, to be consistent with the other languages.
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.
The phrasing of the qhelp is "This most likely indicates a copy-paste error where the first condition was copied and then not properly adjusted." which I think makes it clear that it was written expecting to find correctness problems.
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.
Finished.
javascript/ql/src/Expressions/WhitespaceContradictsPrecedence.ql
Outdated
Show resolved
Hide resolved
javascript/ql/src/Statements/SuspiciousUnusedLoopIterationVariable.ql
Outdated
Show resolved
Hide resolved
Thank you for the helpful feedback! 🙇 |
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.
Indeed, let's not spend too long debating tags.
This PR promotes multiple JavaScript queries by adding quality-related tags, clearly indicating their promotion to quality status.
Notes for reviewers:
js/loop-iteration-skipped-due-to-shifting
because it is currently being promoted in another pr JS: Promotejs/loop-iteration-skipped-due-to-shifting
to the Code Quality suite #19743