Skip to content

JS: Add most medium precision queries to the code-quality-extended suite. #20395

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

Merged
merged 2 commits into from
Sep 17, 2025
Merged

JS: Add most medium precision queries to the code-quality-extended suite. #20395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58862f4
JavaScript: Add some medium precision queries to the code-quality-ext…
michaelnebel Sep 9, 2025
1c801bd
JavaScript: Update integration test expected output.
michaelnebel Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions javascript/ql/src/Comments/CommentedOutCode.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/commented-out-code
* @tags maintainability
* statistical
* non-attributable
* @tags quality
* maintainability
* readability
Comment on lines +7 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it wrong to also keep old tags?

Suggested change
* @tags quality
* maintainability
* readability
* @tags quality
* maintainability
* readability
* statistical
* non-attributable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

* @precision medium
*/

Expand Down
4 changes: 3 additions & 1 deletion javascript/ql/src/Comments/TodoComments.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/todo-comment
* @tags maintainability
* @tags quality
* maintainability
* readability
* external/cwe/cwe-546
* @precision medium
*/
Expand Down
4 changes: 3 additions & 1 deletion javascript/ql/src/DOM/Alert.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/alert-call
* @tags maintainability
* @tags quality
* reliability
* correctness
* external/cwe/cwe-489
* @precision medium
*/
Expand Down
4 changes: 3 additions & 1 deletion javascript/ql/src/Declarations/RedeclaredVariable.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/variable-redeclaration
* @tags reliability
* @tags quality
* reliability
* correctness
* readability
* @precision medium
*/
Expand Down
5 changes: 4 additions & 1 deletion javascript/ql/src/Declarations/UnusedParameter.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
* @kind problem
* @problem.severity recommendation
* @id js/unused-parameter
* @tags maintainability
* @tags quality
* reliability
* correctness
* readability
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we allowed to have sub-category from different top-level category? (readability is from maintainability)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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."

* @precision medium
*/

Expand Down
5 changes: 3 additions & 2 deletions javascript/ql/src/LanguageFeatures/ArgumentsCallerCallee.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/call-stack-introspection
* @tags maintainability
* language-features
* @tags quality
* reliability
* correctness
* @precision medium
*/

Expand Down
7 changes: 4 additions & 3 deletions javascript/ql/src/LanguageFeatures/DebuggerStatement.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
* @kind problem
* @problem.severity recommendation
* @id js/debugger-statement
* @tags efficiency
* maintainability
* language-features
* @tags quality
* reliability
* correctness
* performance
Comment on lines +9 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a single query fall under 2 sub-categories from the same top-level category?
(correctness and performance are both from reliability)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that is not clear from the documentation (as the talk about grouping could imply "disjoint" grouping).

* external/cwe/cwe-489
* @precision medium
*/
Expand Down
5 changes: 3 additions & 2 deletions javascript/ql/src/LanguageFeatures/Eval.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/eval-call
* @tags maintainability
* language-features
* @tags quality
* reliability
* correctness
* external/cwe/cwe-676
* @precision medium
*/
Expand Down
5 changes: 3 additions & 2 deletions javascript/ql/src/Performance/ReassignParameterAndUseArguments.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
* @kind problem
* @problem.severity recommendation
* @id js/parameter-reassignment-with-arguments
* @tags efficiency
* maintainability
* @tags quality
* reliability
* performance
* @precision medium
*/

Expand Down
5 changes: 3 additions & 2 deletions javascript/ql/src/RegExp/BackspaceEscape.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
* @kind problem
* @problem.severity recommendation
* @id js/regex/backspace-escape
* @tags maintainability
* @tags quality
* maintainability
* readability
* regular-expressions
* correctness
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor

Choose a reason for hiding this comment

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

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. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is also a good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error-handling and concurrency are not as abstract as complexity - so we do have something that is not completely abstract.

* @precision medium
*/

Expand Down
5 changes: 4 additions & 1 deletion javascript/ql/src/Statements/ImplicitReturn.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
* @kind problem
* @problem.severity recommendation
* @id js/implicit-return
* @tags maintainability
* @tags quality
* reliability
* correctness
* readability
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

readability falls under maintainability and our current top level is reliability.

* @precision medium
*/

Expand Down
6 changes: 4 additions & 2 deletions javascript/ql/src/Statements/InconsistentReturn.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
* @kind problem
* @problem.severity recommendation
* @id js/mixed-returns
* @tags reliability
* maintainability
* @tags quality
* reliability
* correctness
* readability
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

readability falls under maintainability and our current top level is reliability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is ok as correctness falls under reliability.

* @precision medium
*/

Expand Down