Merge pull request #19931 from michaelnebel/ql4ql/qualitytagcheck

Ql4ql: Quality query tagging.

Author: michaelnebel

docs/query-metadata-style-guide.md

@@ -157,7 +157,7 @@ Each code quality related query should have **one** of these two "top-level" cat
 * `@tags maintainability`–for queries that detect patterns that make it harder for developers to make changes to the code.
 * `@tags reliability`–for queries that detect issues that affect whether the code will perform as expected during execution.
 
-In addition to the "top-level" categories, we will also add sub-categories to further group code quality related queries:
+In addition to the "top-level" categories, we may also add sub-categories to further group code quality related queries:
 
 * `@tags maintainability`–for queries that detect patterns that make it harder for developers to make changes to the code.
   * `@tags readability`–for queries that detect confusing patterns that make it harder for developers to read the code.
@@ -171,6 +171,7 @@ In addition to the "top-level" categories, we will also add sub-categories to fu
   * `@tags concurrency`-for queries that detect concurrency related issues such as race conditions, deadlocks, thread safety, etc
   * `@tags error-handling`-for queries that detect issues related to unsafe error handling such as uncaught exceptions, etc
 
+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.
 
 There are also more specific `@tags` that can be added. See, the following pages for examples of the low-level tags:
 

java/ql/src/Language Abuse/TypeVariableHidesType.ql

@@ -6,10 +6,10 @@
  * @problem.severity warning
  * @precision medium
  * @id java/type-variable-hides-type
- * @tags reliability
+ * @tags quality
+ *       maintainability
  *       readability
  *       types
- *       quality
  */
 
 import java

javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql

@@ -6,7 +6,7 @@
  * @problem.severity warning
  * @precision high
  * @tags quality
- *       maintainability
+ *       reliability
  *       error-handling
  *       frameworks/nodejs
  */

ql/ql/src/codeql/files/FileSystem.qll

@@ -63,3 +63,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"] + "/%") }
+}

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

@@ -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, if any. */
+  string getAQueryTag() {
+    exists(string tags | tags = this.getContents().regexpCapture("(?s).*@tags ([^@]+)", 1) |
+      result = tags.splitAt("*").trim() and
+      result.regexpMatch("[\\w\\s\\-]+")
+    )
+  }
 }
 
 class BlockComment extends TBlockComment, Comment {

ql/ql/src/queries/style/MissingQualityMetadata.ql

@@ -0,0 +1,52 @@
+/**
+ * @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.getAQueryTag() = "quality" }
+
+private predicate correctTopLevelCategorisation(QueryDoc doc) {
+  strictcount(string s | s = doc.getAQueryTag() and s = ["maintainability", "reliability"]) = 1
+}
+
+private predicate reliabilitySubCategory(QueryDoc doc) {
+  doc.getAQueryTag() = ["correctness", "performance", "concurrency", "error-handling"]
+}
+
+private predicate maintainabilitySubCategory(QueryDoc doc) {
+  doc.getAQueryTag() = ["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
+  (
+    not correctTopLevelCategorisation(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
+    correctTopLevelCategorisation(doc) and
+    (
+      doc.getAQueryTag() = "reliability" and
+      not reliabilitySubCategory(doc) and
+      maintainabilitySubCategory(doc) and
+      msg =
+        "This query file has a sub-category of maintainability but has the `@tags reliability` tag."
+      or
+      doc.getAQueryTag() = "maintainability" and
+      not maintainabilitySubCategory(doc) and
+      reliabilitySubCategory(doc) and
+      msg =
+        "This query file has a sub-category of reliability but has the `@tags maintainability` tag."
+    )
+  )
+select doc, msg

ql/ql/src/queries/style/MissingSecurityMetadata.ql

@@ -1,6 +1,6 @@
 /**
  * @name Missing security metadata
- * @description Security queries should have both a `@tag security` and a `@security-severity` tag.
+ * @description Security queries should have both a `@tags security` and a `@security-severity` tag.
  * @kind problem
  * @problem.severity warning
  * @precision very-high
@@ -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.getAQueryTag() = "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.getAQueryTag() = "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
+select doc, msg

ql/ql/test/queries/style/MissingQualityMetadata/MissingQualityMetadata.expected

@@ -0,0 +1,4 @@
+| testcases/BadQualityMaintainabilityWrongToplevel.ql:1:1:11:3 | QueryDoc | This query file has a sub-category of reliability but has the `@tags maintainability` tag. |
+| testcases/BadQualityMultipleTopLevel.ql:1:1:11:3 | QueryDoc | 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:10:3 | QueryDoc | 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:11:3 | QueryDoc | This query file has a sub-category of maintainability but has the `@tags reliability` tag. |

ql/ql/test/queries/style/MissingQualityMetadata/MissingQualityMetadata.qlref

@@ -0,0 +1 @@
+queries/style/MissingQualityMetadata.ql