Detect invalid deviation markers

Author: lcartey

cpp/common/src/codingstandards/cpp/deviations/CodeIdentifierDeviation.qll

@@ -74,6 +74,16 @@ abstract class CommentDeviationMarker extends Comment {
   DeviationRecord getRecord() { result = record }
 }
 
+/**
+ * A deviation marker in a comment that is not a valid deviation marker.
+ */
+class InvalidCommentDeviationMarker extends Comment {
+  InvalidCommentDeviationMarker() {
+    not this instanceof CommentDeviationMarker and
+    commentMatches(this, "codeql::" + supportedStandard() + "_deviation")
+  }
+}
+
 /**
  * A deviation marker for a deviation that applies to the current line.
  */
@@ -182,9 +192,7 @@ private class BeginStack extends TBeginStack {
   }
 }
 
-private predicate isDeviationRangePaired(
-  DeviationRecord record, DeviationBegin begin, DeviationEnd end
-) {
+predicate isDeviationRangePaired(DeviationRecord record, DeviationBegin begin, DeviationEnd end) {
   exists(File file, int index |
     record = end.getRecord() and
     hasDeviationCommentFileOrdering(record, end, file, index) and
@@ -226,6 +234,21 @@ class DeviationAttribute extends StdAttribute {
   }
 }
 
+/**
+ * A deviation attribute that is not associated with any deviation record.
+ */
+class InvalidDeviationAttribute extends StdAttribute {
+  string unknownCodeIdentifier;
+
+  InvalidDeviationAttribute() {
+    this.hasQualifiedName("codeql", supportedStandard() + "_deviation") and
+    "\"" + unknownCodeIdentifier + "\"" = this.getAnArgument().getValueText() and
+    not exists(DeviationRecord record | record.getCodeIdentifier() = unknownCodeIdentifier)
+  }
+
+  string getAnUnknownCodeIdentifier() { result = unknownCodeIdentifier }
+}
+
 newtype TCodeIndentifierDeviation =
   TSingleLineDeviation(DeviationRecord record, Comment comment, string filepath, int suppressedLine) {
     (

cpp/common/src/codingstandards/cpp/deviations/InvalidDeviationCodeIdentifier.md

@@ -0,0 +1,19 @@
+# Invalid deviation dode identifier
+
+## Overview
+
+Invalid deviation markers in code have no effect on the results but may indicate confusion over which results will be suppressed. 
+
+Deviation code markers are used to suppress CodeQL Coding Standards results, following the process specified in the "MISRA Compliance 2020" document. There are a range of different deviation markers, with specific syntactic requirements. If those syntactic requirements are not met, the marker is invalid and will not be applied, which is likely contrary to developer expectations.
+
+## Recommendation
+
+Ensure the following requirements are met:
+
+ * All `codeql::<standard>_deviation_begin(..)` markers are paired with a matching `codeql::<standard>_deviation_end(..)` marker.
+ * All instances of `codeql::<standard>_deviation` in comments are correctly formatted comment markers, and reference a `code-identifier`s that is specified in a deviation record included in the analysis.
+ * All deviation attributes reference `code-identifier`s that are specified in a deviation record included in the analysis.
+
+## References
+
+* [MISRA Compliance 2020 document - Chapter 4.2 (page 12) - Deviations](https://www.misra.org.uk/app/uploads/2021/06/MISRA-Compliance-2020.pdf)

cpp/common/src/codingstandards/cpp/deviations/InvalidDeviationCodeIdentifier.ql

@@ -0,0 +1,35 @@
+/**
+ * @id cpp/coding-standards/invalid-deviation-code-identifiers
+ * @name Invalid deviation code identifiers
+ * @description Deviation code identifiers must be valid.
+ * @kind problem
+ * @problem.severity error
+ */
+
+import cpp
+import CodeIdentifierDeviation
+
+predicate deviationCodeIdentifierError(Element e, string message) {
+  exists(DeviationEnd end |
+    e = end and
+    not isDeviationRangePaired(_, _, end) and
+    message = "Deviation end block is unmatched."
+  )
+  or
+  exists(InvalidDeviationAttribute b |
+    e = b and
+    message =
+      "Deviation attribute references unknown code identifier " + b.getAnUnknownCodeIdentifier() +
+        "."
+  )
+  or
+  exists(InvalidCommentDeviationMarker m |
+    e = m and
+    message =
+      "Deviation marker does not match an expected format, or references an unknown code identifier."
+  )
+}
+
+from Element e, string message
+where deviationCodeIdentifierError(e, message)
+select e, message

cpp/common/test/deviations/invalid_deviations/InvalidDeviationCodeIdentifier.expected

@@ -0,0 +1,10 @@
+| invalidcodeidentifiers.cpp:1:1:1:45 | // codeql::misra_deviation(x) - invalid, no x | Deviation marker does not match an expected format, or references an unknown code identifier. |
+| invalidcodeidentifiers.cpp:2:1:2:47 | // codeql::autosar_deviation(x) - invalid, no x | Deviation marker does not match an expected format, or references an unknown code identifier. |
+| invalidcodeidentifiers.cpp:3:1:3:44 | // codeql::cert_deviation(x) - invalid, no x | Deviation marker does not match an expected format, or references an unknown code identifier. |
+| invalidcodeidentifiers.cpp:4:1:4:71 | // codeql::misra_deviation_next(a-0-4-2-deviation) - invalid, next_line | Deviation marker does not match an expected format, or references an unknown code identifier. |
+| invalidcodeidentifiers.cpp:5:1:5:73 | // codeql::autosar_deviation_next(a-0-4-2-deviation) - invalid, next_line | Deviation marker does not match an expected format, or references an unknown code identifier. |
+| invalidcodeidentifiers.cpp:6:1:6:70 | // codeql::cert_deviation_next(a-0-4-2-deviation) - invalid, next_line | Deviation marker does not match an expected format, or references an unknown code identifier. |
+| invalidcodeidentifiers.cpp:14:1:14:74 | // codeql::misra_deviation_end(a-0-4-2-deviation) - invalid, unmatched end | Deviation end block is unmatched. |
+| invalidcodeidentifiers.cpp:15:1:15:76 | // codeql::autosar_deviation_end(a-0-4-2-deviation) - invalid, unmatched end | Deviation end block is unmatched. |
+| invalidcodeidentifiers.cpp:16:1:16:73 | // codeql::cert_deviation_end(a-0-4-2-deviation) - invalid, unmatched end | Deviation end block is unmatched. |
+| invalidcodeidentifiers.cpp:18:3:18:25 | misra_deviation | Deviation attribute references unknown code identifier x. |

cpp/common/test/deviations/invalid_deviations/InvalidDeviationCodeIdentifier.qlref

@@ -0,0 +1 @@
+codingstandards/cpp/deviations/InvalidDeviationCodeIdentifier.ql

cpp/common/test/deviations/invalid_deviations/coding-standards.xml

@@ -86,6 +86,11 @@
          <rule-id>RULE-13-6</rule-id>
          <query-id>c/misra/sizeof-operand-with-side-effect</query-id>
       </deviations-entry>
+      <deviations-entry>
+         <rule-id>A0-4-2</rule-id>
+         <justification>long double is required for interaction with third-party libraries.</justification>
+         <code-identifier>a-0-4-2-deviation</code-identifier>
+      </deviations-entry>
    </deviations>
    <deviation-permits>
       <deviation-permits-entry>

cpp/common/test/deviations/invalid_deviations/coding-standards.yml

@@ -46,6 +46,9 @@ deviations:
   - permit-id: DP2
   - rule-id: RULE-13-6
     query-id: c/misra/sizeof-operand-with-side-effect
+  - rule-id: A0-4-2
+    justification: long double is required for interaction with third-party libraries.
+    code-identifier: a-0-4-2-deviation
 deviation-permits:
   - permit-id: DP1
     justification: foo bar baz

cpp/common/test/deviations/invalid_deviations/dummy.cpp

@@ -0,0 +1,22 @@
+// codeql::misra_deviation(x) - invalid, no x
+// codeql::autosar_deviation(x) - invalid, no x
+// codeql::cert_deviation(x) - invalid, no x
+// codeql::misra_deviation_next(a-0-4-2-deviation) - invalid, next_line
+// codeql::autosar_deviation_next(a-0-4-2-deviation) - invalid, next_line
+// codeql::cert_deviation_next(a-0-4-2-deviation) - invalid, next_line
+
+// codeql::misra_deviation_begin(a-0-4-2-deviation)
+// codeql::autosar_deviation_begin(a-0-4-2-deviation)
+// codeql::cert_deviation_begin(a-0-4-2-deviation)
+// codeql::misra_deviation_end(a-0-4-2-deviation)
+// codeql::autosar_deviation_end(a-0-4-2-deviation)
+// codeql::cert_deviation_end(a-0-4-2-deviation)
+// codeql::misra_deviation_end(a-0-4-2-deviation) - invalid, unmatched end
+// codeql::autosar_deviation_end(a-0-4-2-deviation) - invalid, unmatched end
+// codeql::cert_deviation_end(a-0-4-2-deviation) - invalid, unmatched end
+
+[[codeql::misra_deviation("x")]] // invalid
+void test() {}
+
+[[codeql::autosar_deviation("a-0-4-2-deviation")]]
+void test2() {}