Merge pull request #471 from github/lcartey/m16-1-1

M16-1-1: Optimize query and improve detection of nested uses of `defined`

c/misra/src/rules/RULE-20-8/ControllingExpressionIfDirective.ql

@@ -14,40 +14,35 @@
 
 import cpp
 import codingstandards.c.misra
+import codingstandards.cpp.PreprocessorDirective
 
 /*  A controlling expression is evaluated if it is not excluded (guarded by another controlling expression that is not taken). This translates to it either being taken or not taken.  */
 predicate isEvaluated(PreprocessorBranch b) { b.wasTaken() or b.wasNotTaken() }
 
-class IfOrElifPreprocessorBranch extends PreprocessorBranch {
-  IfOrElifPreprocessorBranch() {
-    this instanceof PreprocessorIf or this instanceof PreprocessorElif
-  }
-}
-
 /**
  * Looks like it contains a single macro, which may be undefined
  */
-class SimpleMacroPreprocessorBranch extends IfOrElifPreprocessorBranch {
+class SimpleMacroPreprocessorBranch extends PreprocessorIfOrElif {
   SimpleMacroPreprocessorBranch() { this.getHead().regexpMatch("[a-zA-Z_][a-zA-Z0-9_]+") }
 }
 
-class SimpleNumericPreprocessorBranch extends IfOrElifPreprocessorBranch {
+class SimpleNumericPreprocessorBranch extends PreprocessorIfOrElif {
   SimpleNumericPreprocessorBranch() { this.getHead().regexpMatch("[0-9]+") }
 }
 
 class ZeroOrOnePreprocessorBranch extends SimpleNumericPreprocessorBranch {
   ZeroOrOnePreprocessorBranch() { this.getHead().regexpMatch("[0|1]") }
 }
 
-predicate containsOnlySafeOperators(IfOrElifPreprocessorBranch b) {
+predicate containsOnlySafeOperators(PreprocessorIfOrElif b) {
   containsOnlyDefinedOperator(b)
   or
   //logic: comparison operators eval last, so they make it safe?
   b.getHead().regexpMatch(".*[\\&\\&|\\|\\||>|<|==].*")
 }
 
 //all defined operators is definitely safe
-predicate containsOnlyDefinedOperator(IfOrElifPreprocessorBranch b) {
+predicate containsOnlyDefinedOperator(PreprocessorIfOrElif b) {
   forall(string portion |
     portion =
       b.getHead()
@@ -65,7 +60,7 @@ class BinaryValuedMacro extends Macro {
   BinaryValuedMacro() { this.getBody().regexpMatch("\\(?(0|1)\\)?") }
 }
 
-from IfOrElifPreprocessorBranch b, string msg
+from PreprocessorIfOrElif b, string msg
 where
   not isExcluded(b, Preprocessor3Package::controllingExpressionIfDirectiveQuery()) and
   isEvaluated(b) and

change_notes/2023-12-06-m16-1-1-perf.md

@@ -0,0 +1,4 @@
+ - `M16-1-1` - `DefinedPreProcessorOperatorGeneratedFromExpansionFound.ql`:
+   - Optimize query to improve performance
+   - Improve detection of macros whose body contains the `defined` operator after the start of the macro (e.g. `#define X Y || defined(Z)`).
+   - Enable exclusions to be applied for this rule.

cpp/autosar/src/rules/M16-1-1/DefinedMacro.qll

@@ -2,27 +2,27 @@ import cpp
 import codingstandards.cpp.autosar
 
 /**
- * A helper class describing macros wrapping defined operator
+ * A helper class describing macros wrapping the defined operator
  */
-class DefinedMacro extends Macro {
-  DefinedMacro() {
-    this.getBody().regexpMatch("defined\\s*\\(.*")
+class MacroUsesDefined extends Macro {
+  MacroUsesDefined() {
+    // Uses `defined` directly
+    exists(this.getBody().regexpFind("\\bdefined\\b", _, _))
     or
-    this.getBody().regexpMatch("defined[\\s]+|defined$")
+    // Uses a macro that uses `defined` (directly or indirectly)
+    exists(MacroUsesDefined dm | exists(this.getBody().regexpFind(dm.getRegexForMatch(), _, _)))
   }
 
-  Macro getAUse() {
-    result = this or
-    anyAliasing(result, this)
+  /**
+   * Gets a regex for matching uses of this macro.
+   */
+  string getRegexForMatch() {
+    exists(string arguments |
+      // If there are arguments
+      if getHead() = getName() then arguments = "" else arguments = "\\s*\\("
+    |
+      // Use word boundary matching to find identifiers that match
+      result = "\\b" + getName() + "\\b" + arguments
+    )
   }
 }
-
-predicate directAlias(Macro alias, Macro aliased) {
-  not alias.getBody() = alias.getBody().replaceAll(aliased.getHead(), "")
-}
-
-predicate anyAliasing(Macro alias, Macro inQ) {
-  directAlias(alias, inQ)
-  or
-  exists(Macro intermediate | anyAliasing(intermediate, inQ) and anyAliasing(alias, intermediate))
-}

cpp/autosar/src/rules/M16-1-1/DefinedPreProcessorOperatorGeneratedFromExpansionFound.ql

@@ -14,18 +14,12 @@
 
 import cpp
 import codingstandards.cpp.autosar
+import codingstandards.cpp.PreprocessorDirective
 import DefinedMacro
 
-from DefinedMacro m, PreprocessorBranch e
+from PreprocessorIfOrElif e, MacroUsesDefined m
 where
-  (
-    e instanceof PreprocessorIf or
-    e instanceof PreprocessorElif
-  ) and
-  (
-    e.getHead().regexpMatch(m.getAUse().getHead() + "\\s*\\(.*")
-    or
-    e.getHead().regexpMatch(m.getAUse().getHead().replaceAll("(", "\\(").replaceAll(")", "\\)"))
-  ) and
-  not isExcluded(e)
+  not isExcluded(e, MacrosPackage::definedPreProcessorOperatorInOneOfTheTwoStandardFormsQuery()) and
+  // A`#if` or `#elif` which uses a macro which uses `defined`
+  exists(e.getHead().regexpFind(m.getRegexForMatch(), _, _))
 select e, "The macro $@ expands to 'defined'", m, m.getName()

cpp/autosar/src/rules/M16-1-1/DefinedPreProcessorOperatorInOneOfTheTwoStandardForms.ql

@@ -15,6 +15,7 @@
 
 import cpp
 import codingstandards.cpp.autosar
+import codingstandards.cpp.PreprocessorDirective
 
 //get what comes after each 'defined' used with or without parenth
 string matchesDefinedOperator(PreprocessorBranch e) {
@@ -34,12 +35,8 @@ string matchesDefinedOperator(PreprocessorBranch e) {
   )
 }
 
-from PreprocessorBranch e, string arg
+from PreprocessorIfOrElif e, string arg
 where
-  (
-    e instanceof PreprocessorIf or
-    e instanceof PreprocessorElif
-  ) and
   arg = matchesDefinedOperator(e) and
   not arg.regexpMatch("^\\w*$") and
   not isExcluded(e, MacrosPackage::definedPreProcessorOperatorInOneOfTheTwoStandardFormsQuery())

cpp/autosar/test/rules/M16-1-1/DefinedPreProcessorOperatorGeneratedFromExpansionFound.expected

@@ -1,2 +1,4 @@
-| test.cpp:22:1:22:18 | #if DBLWRAPUSES(X) | The macro $@ expands to 'defined' | test.cpp:18:1:18:22 | #define BADDEF defined | BADDEF |
+| test.cpp:22:1:22:18 | #if DBLWRAPUSES(X) | The macro $@ expands to 'defined' | test.cpp:21:1:21:24 | #define DBLWRAPUSES USES | DBLWRAPUSES |
 | test.cpp:26:1:26:16 | #if BADDEFTWO(X) | The macro $@ expands to 'defined' | test.cpp:25:1:25:31 | #define BADDEFTWO(X) defined(X) | BADDEFTWO |
+| test.cpp:29:1:29:16 | #if BADDEFTWO(Y) | The macro $@ expands to 'defined' | test.cpp:25:1:25:31 | #define BADDEFTWO(X) defined(X) | BADDEFTWO |
+| test.cpp:42:1:42:11 | #if WRAPPER | The macro $@ expands to 'defined' | test.cpp:40:1:40:35 | #define WRAPPER X < Y \|\| defined(z) | WRAPPER |

cpp/autosar/test/rules/M16-1-1/DefinedPreProcessorOperatorInOneOfTheTwoStandardForms.expected

@@ -2,4 +2,4 @@
 | test.cpp:9:1:9:19 | #elif defined X < Y | Use of defined with non-standard form: X < Y. |
 | test.cpp:13:1:13:18 | #if defined(X > Y) | Use of defined with non-standard form: X > Y. |
 | test.cpp:14:1:14:20 | #elif defined(X < Y) | Use of defined with non-standard form: X < Y. |
-| test.cpp:34:1:34:47 | #if defined(X) \|\| defined _Y_ + X && defined(Y) | Use of defined with non-standard form: _Y_ + X. |
+| test.cpp:37:1:37:47 | #if defined(X) \|\| defined _Y_ + X && defined(Y) | Use of defined with non-standard form: _Y_ + X. |

cpp/autosar/test/rules/M16-1-1/test.cpp

@@ -26,10 +26,18 @@
 #if BADDEFTWO(X) // NON_COMPLIANT
 #endif
 
+#if BADDEFTWO(Y) // NON_COMPLIANT
+#endif
+
 // clang-format off
 #if defined (X) || (defined(_Y_)) // COMPLIANT
 // clang-format on
 #endif
 
 #if defined(X) || defined _Y_ + X && defined(Y) // NON_COMPLIANT
+#endif
+
+#define WRAPPER X < Y || defined(z)
+
+#if WRAPPER // NON_COMPLIANT
 #endif

cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll

@@ -30,3 +30,13 @@ PreprocessorDirective isLocatedInAMacroInvocation(MacroInvocation m) {
     result = p
   )
 }
+
+/**
+ * An `if` or `elif` preprocessor branch.
+ */
+class PreprocessorIfOrElif extends PreprocessorBranch {
+  PreprocessorIfOrElif() {
+    this instanceof PreprocessorIf or
+    this instanceof PreprocessorElif
+  }
+}

cpp/common/src/codingstandards/cpp/rules/undefinedmacroidentifiers/UndefinedMacroIdentifiers.qll

@@ -1,5 +1,6 @@
 import cpp
 import codingstandards.cpp.Exclusions
+import codingstandards.cpp.PreprocessorDirective
 
 abstract class UndefinedMacroIdentifiersSharedQuery extends Query { }
 
@@ -76,17 +77,10 @@ string getAnIfDefdMacroIdentifier(PreprocessorBranch pb) {
   )
 }
 
-class IfAndElifs extends PreprocessorBranch {
-  IfAndElifs() {
-    this instanceof PreprocessorIf or
-    this instanceof PreprocessorElif
-  }
-}
-
-class BadIfAndElifs extends IfAndElifs {
+class UndefinedIdIfOrElif extends PreprocessorIfOrElif {
   string undefinedMacroIdentifier;
 
-  BadIfAndElifs() {
+  UndefinedIdIfOrElif() {
     exists(string defRM |
       defRM =
         this.getHead()
@@ -113,7 +107,7 @@ class BadIfAndElifs extends IfAndElifs {
   string getAnUndefinedMacroIdentifier() { result = undefinedMacroIdentifier }
 }
 
-query predicate problems(BadIfAndElifs b, string message) {
+query predicate problems(UndefinedIdIfOrElif b, string message) {
   not isExcluded(b, getQuery()) and
   message =
     "#if/#elif that uses a macro identifier " + b.getAnUndefinedMacroIdentifier() +