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.

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 0 additions & 2 deletions csharp/ql/src/API Abuse/CallToGCCollect.ql
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
* @precision very-high
* @id cs/call-to-gc
* @tags quality
* reliability
* performance
*/

import csharp
Expand Down
5 changes: 5 additions & 0 deletions ql/ql/src/codeql/files/FileSystem.qll
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,8 @@ class File extends Container, Impl::File {
/** Holds if this file was extracted from ordinary source code. */
predicate fromSource() { any() }
}

/** A test file. */
class TestFile extends File {
TestFile() { this.getRelativePath().matches("%/" + ["experimental", "examples", "test"] + "/%") }
}
26 changes: 22 additions & 4 deletions ql/ql/src/codeql_ql/ast/Ast.qll
Original file line number Diff line number Diff line change
Expand Up @@ -202,25 +202,43 @@ class QueryDoc extends QLDoc {

override string getAPrimaryQlClass() { result = "QueryDoc" }

/** Gets the @kind for the query */
/** Gets the @kind for the query. */
string getQueryKind() {
result = this.getContents().regexpCapture("(?s).*@kind ([\\w-]+)\\s.*", 1)
}

/** Gets the @name for the query */
/** Gets the @name for the query. */
string getQueryName() {
result = this.getContents().regexpCapture("(?s).*@name (.+?)(?=\\n).*", 1)
}

/** Gets the id part (without language) of the @id */
/** Gets the id part (without language) of the @id. */
string getQueryId() {
result = this.getContents().regexpCapture("(?s).*@id (\\w+)/([\\w\\-/]+)\\s.*", 2)
}

/** Gets the language of the @id */
/** Gets the language of the @id. */
string getQueryLanguage() {
result = this.getContents().regexpCapture("(?s).*@id (\\w+)/([\\w\\-/]+)\\s.*", 1)
}

/** Gets the @precision for the query. */
string getQueryPrecision() {
result = this.getContents().regexpCapture("(?s).*@precision ([\\w\\-]+)\\s.*", 1)
}

/** Gets the @security-severity for the query. */
string getQuerySecuritySeverity() {
result = this.getContents().regexpCapture("(?s).*@security\\-severity ([\\d\\.]+)\\s.*", 1)
}

/** 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.

exists(string tags | tags = this.getContents().regexpCapture("(?s).*@tags ([^@]+)", 1) |
result = tags.splitAt("*").trim() and
result.regexpMatch("[\\w\\s\\-]+")
)
}
}

class BlockComment extends TBlockComment, Comment {
Expand Down
47 changes: 47 additions & 0 deletions ql/ql/src/queries/style/MissingQualityMetadata.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* @name Missing quality metadata
* @description Quality queries should have exactly one top-level category and if sub-categories are used, the appropriate top-level category should be used.
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/missing-quality-metadata
* @tags correctness
*/

import ql

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 😄

}

private predicate reliabilitySubCategory(QueryDoc doc) {
doc.getQueryTags() = ["correctness", "performance", "concurrency", "error-handling"]
}

private predicate maintainabilitySubCategory(QueryDoc doc) {
doc.getQueryTags() = ["readability", "useless-code", "complexity"]
}

from TopLevel t, QueryDoc doc, string msg
where
doc = t.getQLDoc() and
not t.getLocation().getFile() instanceof TestFile and
hasQualityTag(doc) and
(
incorrectTopLevelCategorisation(doc) and
msg =
"This query file has incorrect top-level categorisation. It should have exactly one top-level category, either `@tags maintainability` or `@tags reliability`."
or
maintainabilitySubCategory(doc) and
not doc.getQueryTags() = "maintainability" and
msg =
"This query file has a sub-category of maintainability but is missing the `@tags maintainability` tag."
or
reliabilitySubCategory(doc) and
not doc.getQueryTags() = "reliability" and
msg =
"This query file has a sub-category of reliability but is missing the `@tags reliability` tag."
)
select t, msg
45 changes: 13 additions & 32 deletions ql/ql/src/queries/style/MissingSecurityMetadata.ql
Original file line number Diff line number Diff line change
Expand Up @@ -10,45 +10,26 @@

import ql

predicate missingSecuritySeverity(QLDoc doc) {
exists(string s | s = doc.getContents() |
exists(string securityTag | securityTag = s.splitAt("@") |
securityTag.matches("tags%security%")
) and
exists(string precisionTag | precisionTag = s.splitAt("@") |
precisionTag.matches("precision %")
) and
not exists(string securitySeverity | securitySeverity = s.splitAt("@") |
securitySeverity.matches("security-severity %")
)
)
predicate missingSecuritySeverity(QueryDoc doc) {
doc.getQueryTags() = "security" and
exists(doc.getQueryPrecision()) and
not exists(doc.getQuerySecuritySeverity())
}

predicate missingSecurityTag(QLDoc doc) {
exists(string s | s = doc.getContents() |
exists(string securitySeverity | securitySeverity = s.splitAt("@") |
securitySeverity.matches("security-severity %")
) and
exists(string precisionTag | precisionTag = s.splitAt("@") |
precisionTag.matches("precision %")
) and
not exists(string securityTag | securityTag = s.splitAt("@") |
securityTag.matches("tags%security%")
)
)
predicate missingSecurityTag(QueryDoc doc) {
exists(doc.getQuerySecuritySeverity()) and
exists(doc.getQueryPrecision()) and
not doc.getQueryTags() = "security"
}

from TopLevel t, string msg
from TopLevel t, QueryDoc doc, string msg
where
t.getLocation().getFile().getBaseName().matches("%.ql") and
not t.getLocation()
.getFile()
.getRelativePath()
.matches("%/" + ["experimental", "examples", "test"] + "/%") and
doc = t.getQLDoc() and
not t.getLocation().getFile() instanceof TestFile and
(
missingSecuritySeverity(t.getQLDoc()) and
missingSecuritySeverity(doc) and
msg = "This query file is missing a `@security-severity` tag."
or
missingSecurityTag(t.getQLDoc()) and msg = "This query file is missing a `@tag security`."
missingSecurityTag(doc) and msg = "This query file is missing a `@tags security`."
)
select t, msg
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
| testcases/BadQualityMaintainabilityWrongToplevel.ql:1:1:17:13 | TopLevel | This query file has a sub-category of reliability but is missing the `@tags reliability` tag. |
| testcases/BadQualityMultipleTopLevel.ql:1:1:17:13 | TopLevel | This query file has incorrect top-level categorisation. It should have exactly one top-level category, either `@tags maintainability` or `@tags reliability`. |
| testcases/BadQualityNoToplevel.ql:1:1:16:13 | TopLevel | This query file has incorrect top-level categorisation. It should have exactly one top-level category, either `@tags maintainability` or `@tags reliability`. |
| testcases/BadQualityReliabilityWrongToplevel.ql:1:1:17:13 | TopLevel | This query file has a sub-category of maintainability but is missing the `@tags maintainability` tag. |
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* maintainability
* error-handling
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* maintainability
* reliability
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* someothertag
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* reliability
* readability
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @security-severity 10.0
* @precision very-high
* @id ql/quality-query-test
* @tags security
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @security-severity 10.0
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* maintainability
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* maintainability
* readability
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* reliability
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Some query
* @description Some description
* @kind problem
* @problem.severity warning
* @precision very-high
* @id ql/quality-query-test
* @tags quality
* reliability
* correctness
*/

import ql

from Class c
where none()
select c, ""
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
| testcases/BadNoSecurity.ql:1:1:16:9 | TopLevel | This query file is missing a `@tag security`. |
| testcases/BadNoSecurity.ql:1:1:16:9 | TopLevel | This query file is missing a `@tags security`. |
| testcases/BadNoSeverity.ql:1:1:16:9 | TopLevel | This query file is missing a `@security-severity` tag. |
Loading