Skip to content

Ql4ql: Quality query tagging. #19931

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
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Ql4ql: Quality query tagging. #19931

wants to merge 10 commits into from
+292 −44

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Jun 30, 2025 •
edited
Loading

In this PR we introduce a Ql4Ql check for correct use of quality query tags as documented here.
According to the documentation

  • A quality query (a query tagged with quality) should have at least a top level categorisation.
  • A query may have sub-level categories as well. If a quality query is tagged with top-level A and with sub-categories B1,..,Bn then at least one of the sub-categories Bi should be a sub-category of A.

Furthermore, we also fix the existing violations.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 30, 2025
View reviewed changes
ql/ql/test/queries/style/MissingQualityMetadata/MissingQualityMetadata.qlref
@@ -0,0 +1 @@
queries/style/MissingQualityMetadata.ql

Check warning

Code scanning / CodeQL

Query test without inline test expectations Warning test

Query test does not use inline test expectations.
@michaelnebel michaelnebel force-pushed the ql4ql/qualitytagcheck branch from 70d3b6a to eb14420 Compare July 1, 2025 07:42
@github-actions github-actions bot added the C# label Jul 1, 2025
@michaelnebel michaelnebel force-pushed the ql4ql/qualitytagcheck branch from eb14420 to ce5f119 Compare July 1, 2025 07:51
@michaelnebel michaelnebel changed the title Ql4ql/qualitytagcheck Ql4ql: Quality query tagging. Jul 1, 2025
hvitved
hvitved reviewed Jul 2, 2025
View reviewed changes
ql/ql/src/queries/style/MissingQualityMetadata.ql Outdated
private predicate hasQualityTag(QueryDoc doc) { doc.getQueryTags() = "quality" }

private predicate incorrectTopLevelCategorisation(QueryDoc doc) {
count(string s | s = doc.getQueryTags() and s = ["maintainability", "reliability"]) != 1
Copy link
Contributor

Choose a reason for hiding this comment

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

May as well use strictcount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh wait, we can't - the predicate won't hold in the case there is no top level tag (if the count is 0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well ok, I can invert the logic 😄

ql/ql/src/codeql_ql/ast/Ast.qll Outdated
}

/** Gets the individual @tags for the query. */
string getQueryTags() {
Copy link
Contributor

Choose a reason for hiding this comment

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

getAQueryTag would be a better name.

@michaelnebel michaelnebel force-pushed the ql4ql/qualitytagcheck branch from ce5f119 to 179793e Compare July 2, 2025 10:32
@github-actions github-actions bot added JS Java and removed C# labels Jul 2, 2025
@michaelnebel michaelnebel marked this pull request as ready for review July 2, 2025 11:47
@michaelnebel michaelnebel requested review from a team as code owners July 2, 2025 11:47
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Jul 2, 2025
@owen-mc owen-mc mentioned this pull request Jul 2, 2025
Open
@owen-mc
Copy link
Contributor

owen-mc commented Jul 2, 2025

It's hard to know what to do about multiple sub-categories. If we allow multiple subcategories then I don't really see why we shouldn't allow ones in different top-level categories. A lot of queries look for a readability problem which might also be hiding a correctness bug. This in-flight PR is a good example - including special characters in string literals is definitely a readability issue, but it can also cause correctness issues. The qhelp files are very clear about this. I think it makes sense to have the tags maintainability, readability and correctness. We have a restriction that we can only have one top-level category, and we have opted for maintainability as results for this query and almost always readability issues, but we have also added correctness to reflect that correctness issues may well be highlighted by this query.

Whatever we choose, we should update the guide to be clearer.

@michaelnebel
Copy link
Contributor Author

It's hard to know what to do about multiple sub-categories. If we allow multiple subcategories then I don't really see why we shouldn't allow ones in different top-level categories. A lot of queries look for a readability problem which might also be hiding a correctness bug. This in-flight PR is a good example - including special characters in string literals is definitely a readability issue, but it can also cause correctness issues. The qhelp files are very clear about this. I think it makes sense to have the tags maintainability, readability and correctness. We have a restriction that we can only have one top-level category, and we have opted for maintainability as results for this query and almost always readability issues, but we have also added correctness to reflect that correctness issues may well be highlighted by this query.

Whatever we choose, we should update the guide to be clearer.

Fair point; At least we should be able to require the following (to detect incorrect top level categories):
If a quality query is tagged with top-level A and with sub-categories B1,..,Bn then at least one of the sub-categories Bi should be a sub-category of A.

@owen-mc
Copy link
Contributor

owen-mc commented Jul 2, 2025

That sounds good to me.

Napalys
Napalys previously approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@Napalys Napalys left a comment

Choose a reason for hiding this comment

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

Thanks for updating js query tag ❤️

@michaelnebel michaelnebel force-pushed the ql4ql/qualitytagcheck branch from 179793e to aefd941 Compare July 3, 2025 10:05
@michaelnebel michaelnebel requested a review from a team as a code owner July 3, 2025 10:19
@michaelnebel michaelnebel requested a review from hvitved July 3, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Napalys Napalys Napalys left review comments

@hvitved hvitved Awaiting requested review from hvitved

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
documentation Java JS no-change-note-required This PR does not need a change note QL-for-QL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelnebel @owen-mc @hvitved @Napalys