Address review feedback

Author: MichaelRFairhurst

c/misra/src/codeql-suites/misra-c-default.qls

@@ -7,4 +7,5 @@
 - exclude:
     tags contain:
     - external/misra/audit
+    - external/misra/strict
     - external/misra/default-disabled

c/misra/src/codeql-suites/misra-c-strict.qls

@@ -0,0 +1,8 @@
+- description: MISRA C 2012 (Strict)
+- qlpack: codeql/misra-c-coding-standards
+- include:
+    kind:
+    - problem
+    - path-problem
+    tags contain:
+    - external/misra/strict

c/misra/test/rules/RULE-2-8/UnusedObjectDefinition.expected

@@ -6,3 +6,5 @@
 | test.c:45:9:45:10 | definition of g6 | Unused object definition 'g6'. | test.c:45:9:45:10 | test.c:45:9:45:10 |  |
 | test.c:51:5:51:6 | definition of g7 | Unused object definition 'g7'. | test.c:51:5:51:6 | test.c:51:5:51:6 |  |
 | test.c:64:3:64:18 | ONLY_DEF_VAR(x) | Unused object definition 'l2' from macro '$@'. | test.c:60:1:60:34 | test.c:60:1:60:34 | ONLY_DEF_VAR |
+| test.c:117:11:117:13 | definition of g10 | Unused object definition 'g10'. | test.c:117:11:117:13 | test.c:117:11:117:13 |  |
+| test.c:122:13:122:14 | definition of l2 | Unused object definition 'l2'. | test.c:122:13:122:14 | test.c:122:13:122:14 |  |

c/misra/test/rules/RULE-2-8/test.c

@@ -111,3 +111,21 @@ void f8() {
 void f9() {
   DEF_ATTR_UNUSED_INNER_VAR(); // COMPLIANT
 }
+
+// Const variable tests:
+const int g9 = 1; // COMPLIANT
+const int g10 = 1; // NON-COMPLIANT
+
+void f10() {
+  g9;
+  const int l1 = 1; // COMPLIANT
+  const int l2 = 1; // NON-COMPLIANT
+  l1;
+}
+
+// Side effects should not disable this rule:
+void f11() {
+  int l1 = 1; // COMPLIANT
+  int l2 = l1++; // COMPLIANT
+  l2;
+}

cpp/common/src/codingstandards/cpp/deadcode/UnusedObjects.qll

@@ -1,4 +1,5 @@
 import cpp
+import codingstandards.cpp.deadcode.UnusedVariables
 import codingstandards.cpp.alertreporting.HoldsForAllCopies
 import codingstandards.cpp.alertreporting.DeduplicateMacroResults
 
@@ -15,10 +16,13 @@ import codingstandards.cpp.alertreporting.DeduplicateMacroResults
  */
 class UnusedObjectDefinition extends VariableDeclarationEntry {
   UnusedObjectDefinition() {
+    (
+      getVariable() instanceof BasePotentiallyUnusedLocalVariable
+      or
+      getVariable() instanceof BasePotentiallyUnusedGlobalOrNamespaceVariable
+    ) and
     not exists(VariableAccess access | access.getTarget() = getVariable()) and
-    getVariable().getDefinition() = this and
-    not this instanceof ParameterDeclarationEntry and
-    not getVariable() instanceof MemberVariable
+    getVariable().getDefinition() = this
   }
 
   /* Dead objects with these attributes are reported in the "strict" queries. */

cpp/common/src/codingstandards/cpp/deadcode/UnusedVariables.qll

@@ -36,11 +36,11 @@ predicate declarationHasSideEffects(Variable v) {
   v.getType() instanceof TemplateDependentType
 }
 
-/** A `LocalVariable` which is a candidate for being unused. */
-class PotentiallyUnusedLocalVariable extends LocalVariable {
-  PotentiallyUnusedLocalVariable() {
-    // Ignore variables declared in macro expansions
-    not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion()) and
+/**
+ * A `LocalVariable` which is a candidate for being unused, and may or may not be defined in a macro.
+ */
+class BasePotentiallyUnusedLocalVariable extends LocalVariable {
+  BasePotentiallyUnusedLocalVariable() {
     // Ignore variables where initializing the variable has side effects
     not declarationHasSideEffects(this) and // TODO non POD types with initializers? Also, do something different with templates?
     exists(Function f | f = getFunction() |
@@ -56,6 +56,16 @@ class PotentiallyUnusedLocalVariable extends LocalVariable {
   }
 }
 
+/**
+ * A `LocalVariable` which is a candidate for being unused, and not defined in a macro.
+ */
+class PotentiallyUnusedLocalVariable extends BasePotentiallyUnusedLocalVariable {
+  PotentiallyUnusedLocalVariable() {
+    // Ignore variables declared in macro expansions
+    not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion())
+  }
+}
+
 /** Holds if `mf` is "defined" in this database. */
 predicate isDefined(MemberFunction mf) {
   exists(MemberFunction definedMemberFunction |
@@ -105,13 +115,11 @@ class PotentiallyUnusedMemberVariable extends MemberVariable {
   }
 }
 
-/** A `GlobalOrNamespaceVariable` which is potentially unused. */
-class PotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVariable {
-  PotentiallyUnusedGlobalOrNamespaceVariable() {
+/** A `GlobalOrNamespaceVariable` which is potentially unused and may or may not be defined in a macro */
+class BasePotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVariable {
+  BasePotentiallyUnusedGlobalOrNamespaceVariable() {
     // A non-defined variable may never be used
     hasDefinition() and
-    // Not declared in a macro expansion
-    not isInMacroExpansion() and
     // No side-effects from declaration
     not declarationHasSideEffects(this) and
     // exclude uninstantiated template members
@@ -121,6 +129,17 @@ class PotentiallyUnusedGlobalOrNamespaceVariable extends GlobalOrNamespaceVariab
   }
 }
 
+/**
+ * A `GlobalOrNamespaceVariable` which is potentially unused, and is not defined in a macro.
+*/
+class PotentiallyUnusedGlobalOrNamespaceVariable extends BasePotentiallyUnusedGlobalOrNamespaceVariable
+{
+  PotentiallyUnusedGlobalOrNamespaceVariable() {
+    // Not declared in a macro expansion
+    not isInMacroExpansion()
+  }
+}
+
 predicate isUnused(Variable variable) {
   not exists(variable.getAnAccess()) and
   variable.getInitializer().fromSource()

docs/development_handbook.md

@@ -51,6 +51,8 @@ Each coding standard consists of a list of "guidelines", however not all the gui
 
 For some of the rules which are not amenable to static analysis, we may opt to provide a query which aids with "auditing" the rules. For example, AUTOSAR includes a rule (A10-0-1) "Public inheritance shall be used to implement 'is-a' relationship.". This is not directly amenable to static analysis, because it requires external context around the concept being modeled. However, we can provide an "audit" rule which reports all the public and private inheritance relationships in the program, so they can be manually verified.
 
+For other rules, there may be means of indicating that a contravention is intentional, and where requiring a _devation report_ may be extra burdensome on developers and require double-entry. These results should be reported under a "strict" query. For instance, `RULE-2-8` "A project should not contain unused object definitions," where adding `__attribute__((unused))` may be preferable in order to suppress compiler warnings (which _deviation reports_ do not do) and are highly indicative of an intentional contravention by a developer.
+
 For each rule which will be implemented with a query we have assigned a "rule package". Rule packages represent sets of rules, possibly across standards, that will be implemented together. Examples of rule packages include "Exceptions", "Naming", "Pointers" and so forth. By implementing queries for related rules together, we intend to maximize the amount of code shared between queries, and to ensure query developers can gain a deep understanding of that specific topic.
 
 The canonical list of rules, with implementation categorization and assigned rule packages, are stored in this repository in the `rules.csv` file.

docs/user_manual.md

@@ -71,6 +71,7 @@ For each rule we therefore identify whether it is supportable or not. Furthermor
 
 - **Automated** - the queries for the rule find contraventions directly.
 - **Audit only** - the queries for the rule does not find contraventions directly, but instead report a list of _candidates_ that can be used as input into a manual audit. For example, `A10-0-1` (_Public inheritance shall be used to implement 'is-a' relationship_) is not directly amenable to static analysis, but CodeQL can be used to produce a list of all the locations that use public inheritance so they can be manually reviewed.
+- **Strict only** - the queries for the rule find contraventions directly, but find results which are strongly indicated to be intentional, and where adding a _deviation report_ may be extra burden on developers. For example, in `RULE-2-8` (_A project should not contain unused object definitions_), declaring objects with `__attribute__((unused))` may be preferable to a _deviation report_, which will not suppress relevant compiler warnings, and therefore would otherwise require developer double-entry.
 
 Each supported rule is implemented as one or more CodeQL queries, with each query covering an aspect of the rule. In many coding standards, the rules cover non-trivial semantic properties of the codebase under analysis.
 
@@ -214,14 +215,22 @@ The following flags may be passed to the `database analyze` command to adjust th
 
 The output of this command will be a [SARIF file](https://sarifweb.azurewebsites.net/) called `<name-of-results-file>.sarif`.
 
-#### Running the analysis for audit level queries
+#### Running the analysis for strict and/or audit level queries
 
-Optionally, you may want to run the "audit" level queries. These queries produce lists of results that do not directly highlight contraventions of the rule. Instead, they identify locations in the code that can be manually audited to verify the absence of problems for that particular rule.
+Optionally, you may want to run the "strict" or "audit" level queries.
+
+Audit queries produce lists of results that do not directly highlight contraventions of the rule. Instead, they identify locations in the code that can be manually audited to verify the absence of problems for that particular rule.
 
 ```bash
 codeql database analyze --format=sarifv2.1.0 --output=<name-of-results-file>.sarif path/to/<output_database_name> path/to/codeql-coding-standards/cpp/<coding-standard>/src/codeql-suites/<coding-standard>-audit.qls...
 ```
 
+Strict queries identify contraventions in the code that strongly suggest they are deliberate, and where adding an explicit _deviation report_ may be extra burden on developers.
+
+```bash
+codeql database analyze --format=sarifv2.1.0 --output=<name-of-results-file>.sarif path/to/<output_database_name> path/to/codeql-coding-standards/cpp/<coding-standard>/src/codeql-suites/<coding-standard>-strict.qls...
+```
+
 For each Coding Standard you want to run, add a trailing entry in the following format: `path/to/codeql-coding-standards/cpp/<coding-standard>/src/codeql-suites/<coding-standard>-default.qls`.
 
 #### Producing an analysis report

rule_packages/c/DeadCode2.json

@@ -60,7 +60,10 @@
           ]
         }
       ],
-      "title": "A project should not contain unused object definitions"
+      "title": "A project should not contain unused object definitions",
+      "implementation_scope": {
+        "description": "Unused object definitions marked with `__attribute__((unused))` (and `used`, `maybe_used`, `cleanup`) are separately reported under the 'strict' query suite. This is because these attributes strongly indicate the contravention is intentional, and a deviation report alone will not suppress compiler warnings."
+      }
     }
   }
 }