JS: Add most medium precision queries to the code-quality-extended suite. #20395
Conversation
michaelnebel
added
the
no-change-note-required
There was a problem hiding this comment.
Pull Request Overview
This PR adds most "medium" precision queries to the code-quality-extended suite for JavaScript analysis, expanding from just "high" and "very-high" precision queries. The changes update query metadata tags to align with quality standards and verify these queries are included in the extended suite.
- Updates tags in 12 JavaScript query files to use standardized quality tags
- Adds medium precision queries to the
code-quality-extendedsuite - Updates test expectations to reflect the inclusion of these queries
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
InconsistentReturn.ql |
Updated tags from reliability, maintainability to include quality, correctness, readability |
ImplicitReturn.ql |
Updated tags from maintainability to include quality, reliability, correctness, readability |
BackspaceEscape.ql |
Updated tags from maintainability, regular-expressions to quality, maintainability, readability, correctness |
ReassignParameterAndUseArguments.ql |
Updated tags from efficiency, maintainability to quality, reliability, performance |
Eval.ql |
Updated tags from maintainability, language-features to quality, reliability, correctness |
DebuggerStatement.ql |
Updated tags from efficiency, maintainability, language-features to quality, reliability, correctness, performance |
ArgumentsCallerCallee.ql |
Updated tags from maintainability, language-features to quality, reliability, correctness |
UnusedParameter.ql |
Updated tags from maintainability to quality, reliability, correctness, readability |
RedeclaredVariable.ql |
Updated tags from reliability to include quality, correctness |
Alert.ql |
Updated tags from maintainability to quality, reliability, correctness |
TodoComments.ql |
Updated tags from maintainability to include quality, readability |
CommentedOutCode.ql |
Updated tags from maintainability, statistical, non-attributable to quality, maintainability, readability |
not_included_in_qls.expected |
Removed entries for queries now included in the suite |
javascript-code-quality-extended.qls.expected |
Added entries for the newly included queries |
Napalys
left a comment
Napalys
left a comment
There was a problem hiding this comment.
Nice, thank you! I just have couple of questions.
| * @tags quality | ||
| * maintainability | ||
| * readability |
There was a problem hiding this comment.
Is it wrong to also keep old tags?
| * @tags quality | |
| * maintainability | |
| * readability | |
| * @tags quality | |
| * maintainability | |
| * readability | |
| * statistical | |
| * non-attributable |
There was a problem hiding this comment.
Good question. As far as I know, we are not using the tags for anything and we have already started to remove them in some of the other PRs (also I think there is an upper limit on the number of tags - around 10 AFAIR) - so I think keeping them creates more confusion.
IMO, we should probably make sure to cleanup the tags for all the quality queries and only use the main and sub-category tags and CWE tags.
| * @tags quality | ||
| * reliability | ||
| * correctness | ||
| * readability |
There was a problem hiding this comment.
Are we allowed to have sub-category from different top-level category? (readability is from maintainability)
There was a problem hiding this comment.
Yes, we decided to allow this when the code-quality suites were created and the documentation was updated with "You may use sub-categories from both top-level categories on the same query. However, if you only use sub-categories from a single top-level category, then you must also tag the query with that top-level category."
| * correctness | ||
| * performance |
There was a problem hiding this comment.
Can a single query fall under 2 sub-categories from the same top-level category?
(correctness and performance are both from reliability)
There was a problem hiding this comment.
Yes, but that is not clear from the documentation (as the talk about grouping could imply "disjoint" grouping).
| * @tags quality | ||
| * maintainability | ||
| * readability | ||
| * regular-expressions | ||
| * correctness |
There was a problem hiding this comment.
Would it be wrong to keep also old tag for regular-expressions might come useful in the future?
Also correctness falls under top-level tag reliability are we allowed to have it here?
There was a problem hiding this comment.
readability falls under maintainability so it is acceptable to also have correctness. I think the "primary" category is maintability and readability.
I think we should delete regular-expressions for now. If we want to have these tags we should do it consistently across all the languages and introduce it as a sub-category. Maybe we should introduce that sub-category under reliability (and then subsequently update all queries in the quality suites)?
There was a problem hiding this comment.
Maybe we should introduce that sub-category under reliability (and then subsequently update all queries in the quality suites)?
Personally, I'm not sure that this would be a good fit as a sub-category. The existing sub-categories seem to be more abstract, whereas regular-expressions is much more fine-grained. I think it would only make sense to add it if we introduced a third-level category, or if we refactored the current sub-categories to generally be more fine-grained.
Of course, that is just my opinion. 🤷
There was a problem hiding this comment.
That is also a good point!
There was a problem hiding this comment.
error-handling and concurrency are not as abstract as complexity - so we do have something that is not completely abstract.
| * reliability | ||
| * correctness | ||
| * readability |
There was a problem hiding this comment.
readability falls under maintainability and our current top level is reliability.
| * @tags quality | ||
| * reliability | ||
| * correctness | ||
| * readability |
There was a problem hiding this comment.
readability falls under maintainability and our current top level is reliability.
There was a problem hiding this comment.
That is ok as correctness falls under reliability.
Napalys
left a comment
Napalys
left a comment
There was a problem hiding this comment.
Thanks for the clarification! LGTM! 
Thank for the good questions and the review. In any case, I think it makes sense to think about whether we need @jonjanego : We are discussing whether we need another sub-category under |
Hi @michaelnebel - that feels unnecessarily specific to me, personally, but i am curious to hear some more arguments in favour of it? how many queries do we have that would fall into that as a category? |
Good question. I think around 10 for javascript, maybe around 5 for python and then at most 1-2 for the remaining languages. So not that many actually. If you think it is too specific - then lets not do it. |
3 participants
In this PR we add most
mediumprecision queries to thecodeql-quality-extendedsuite, with the exception ofjs/summary/taint-sourcesandjs/summary/taint-sinkswhich highlights potential sources and sinks (look like debugging queries).DCA looks good.
mediumprecision queries results in an overwhelming number of alerts.mediumprecision queries (on top ofhighandvery-high) only increases analysis time around 5%.