Java: Tag quality queries with quality and sub-category
#19799
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds the new quality top-level tag and appropriate sub-category tags (e.g., reliability, correctness, performance, etc.) to Java CodeQL queries, replacing legacy maintainability, reliability, and efficiency tags for consistency. It also updates the change notes to document these metadata changes. Removal of all legacy tags is deferred to a future PR.
Reviewed Changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| java/ql/src/change-notes/2025-06-17-add-tags-to-quality-queries.md | Updated bullets to describe added/removed tags |
| java/ql/src/Violations of Best Practice/Undesirable Calls/PrintLnArray.ql | Replaced maintainability with quality + reliability + correctness tags |
| java/ql/src/Violations of Best Practice/Undesirable Calls/DefaultToString.ql | Replaced reliability + maintainability with quality + reliability + correctness tags |
Comments suppressed due to low confidence (3)
java/ql/src/change-notes/2025-06-17-add-tags-to-quality-queries.md:6
- Remove the trailing "and ." and ensure the list of queries is complete.
* The tag `readability` has been added to `java/missing-override-annotation`, `java/deprecated-call`, `java/inconsistent-javadoc-throws`, `java/unknown-javadoc-parameter`, `java/jdk-internal-api-access`, `java/underscore-identifier`, `java/misleading-indentation`, `java/constants-only-interface` and .
java/ql/src/Violations of Best Practice/Undesirable Calls/PrintLnArray.ql:10
java/print-arrayshould not include thereliabilitytag per the change notes, so remove this line.
* reliability
java/ql/src/Violations of Best Practice/Undesirable Calls/DefaultToString.ql:10
java/call-to-object-tostringshould not include thereliabilitytag per the change notes, so remove this line.
* reliability
| @@ -5,8 +5,9 @@ | |||
| * @problem.severity recommendation | |||
| * @precision high | |||
| * @id java/inefficient-empty-string-test | |||
| * @tags efficiency | |||
| * maintainability | |||
| * @tags quality | |||
There was a problem hiding this comment.
I think we should drop this query - it's just a recommendation, it's very opinionated, and the claim it states might be wrong (it's the sort of claim that potentially doesn't age well). We should perhaps just delete it.
There was a problem hiding this comment.
I've changed it to readability, because when I searched online that was the main justification people gave for preferring isEmpty or a length check.
| * maintainability | ||
| * @tags quality | ||
| * reliability | ||
| * performance |
There was a problem hiding this comment.
Putting this in performance is perhaps a bit of a stretch. It's probably more like a bad practice.
| * @tags reliability | ||
| * maintainability | ||
| * @tags quality | ||
| * reliability |
There was a problem hiding this comment.
I'd stick this in maintainability+readability instead.
There was a problem hiding this comment.
I disagree with this one. It is a kind of "best practice" to not expose internal representation, but the reason is that it can lead to correctness problems when the internal state is mutated in an unexpected way. So I think it should stay in correctness. If we make a best-practice category in future then we could put it in that.
| --- | ||
| * The tag `quality` has been added to multiple Java quality queries for consistency. They have all been given a tag for one of the two top-level categories `reliability` or `maintainability`, and a tag for a sub-category. See [Query file metadata and alert message style guide](https://github.com/github/codeql/blob/main/docs/query-metadata-style-guide.md#quality-query-sub-category-tags) for more information about these categories. | ||
| * The tag `external/cwe/cwe-571` has been added to `java/equals-on-unrelated-types`. | ||
| * The tag `readability` has been added to `java/missing-override-annotation`, `java/deprecated-call`, `java/inconsistent-javadoc-throws`, `java/unknown-javadoc-parameter`, `java/jdk-internal-api-access`, `java/underscore-identifier`, `java/misleading-indentation`, `java/constants-only-interface` and . |
There was a problem hiding this comment.
Something's missing at the end of this sentence?
| ql/java/ql/src/Compatibility/JDK9/JdkInternalAccess.ql | ||
| ql/java/ql/src/Compatibility/JDK9/UnderscoreIdentifier.ql | ||
| ql/java/ql/src/DeadCode/UselessParameter.ql | ||
| ql/java/ql/src/Language Abuse/ChainedInstanceof.ql |
There was a problem hiding this comment.
This query uses a hardcoded threshold. So possibly it should be excluded for now?
|
I think you've acquired a conflict with the newly merged #19808 |
owen-mc
force-pushed
the
java/quality-tags
branch
from
ae3b46c to
7a50298
Compare
|
Thanks for pointing that out @aschackmull . I've rebased and fixed the failing test. |
Updated tags for high and very high precision quality queries, using the query metadata guide. It is intended that all of these queries will now be in the quality suite.
I used copilot to do most of these, though I did review them all and change many of them.
Something that came up several times is a query that finds what is probably a coding mistake which leads to dead code. I erred on the side of tagging these as correctness (or sometimes error-handling) rather than useless-code, as the more serious issue is that the code probably contains a correctness error, rather than that there is some useless code that should be deleted.
The existing
efficiencyandlogictags seem to be superseded by the newperformanceandcorrectnesssub-categories, but I will leave removing them for another PR so as not to delay this one.