Merge pull request #725 from rak3-sh/rp/m0-1-10-711

Fix false positives of M0-1-10 (#711)

Author: lcartey

change_notes/2024-10-02-fix-fp-711-M0-1-10.md

@@ -0,0 +1,2 @@
+- `M0-1-10` - `UnusedFunction.ql`:
+  - Fixes #711. Excludes constexpr functions, considers functions from GoogleTest as an EntryPoint and does not consider special member functions. Another query called UnusedSplMemberFunction.ql is created that reports unused special member functions. This is done so as to enable deviations to be applied to this case.

cpp/autosar/src/rules/M0-1-10/UnusedFunction.ql

@@ -26,5 +26,6 @@ where
     then name = unusedFunction.getQualifiedName()
     else name = unusedFunction.getName()
   ) and
-  not unusedFunction.isDeleted()
+  not unusedFunction.isDeleted() and
+  not unusedFunction instanceof SpecialMemberFunction
 select unusedFunction, "Function " + name + " is " + unusedFunction.getDeadCodeType()

cpp/autosar/src/rules/M0-1-10/UnusedSplMemberFunction.ql

@@ -0,0 +1,31 @@
+/**
+ * @id cpp/autosar/unused-spl-member-function
+ * @name M0-1-10: Every defined function should be called at least once
+ * @description Uncalled functions complicate the program and can indicate a possible mistake on the
+ *              part of the programmer.
+ * @kind problem
+ * @precision medium
+ * @problem.severity warning
+ * @tags external/autosar/id/m0-1-10
+ *       readability
+ *       maintainability
+ *       external/autosar/allocated-target/implementation
+ *       external/autosar/enforcement/automated
+ *       external/autosar/obligation/advisory
+ */
+
+import cpp
+import codingstandards.cpp.autosar
+import codingstandards.cpp.deadcode.UnusedFunctions
+
+from UnusedFunctions::UnusedSplMemberFunction unusedSplMemFunction, string name
+where
+  not isExcluded(unusedSplMemFunction, DeadCodePackage::unusedFunctionQuery()) and
+  (
+    if exists(unusedSplMemFunction.getQualifiedName())
+    then name = unusedSplMemFunction.getQualifiedName()
+    else name = unusedSplMemFunction.getName()
+  ) and
+  not unusedSplMemFunction.isDeleted()
+select unusedSplMemFunction,
+  "Special member function " + name + " is " + unusedSplMemFunction.getDeadCodeType()

cpp/autosar/test/rules/M0-1-10/UnusedFunction.expected

@@ -10,4 +10,5 @@
 | test.cpp:50:5:50:6 | i3 | Function C<T>::i3 is never called. |
 | test.cpp:51:8:51:9 | i4 | Function C<T>::i4 is never called. |
 | test.cpp:52:15:52:16 | i5 | Function C<T>::i5 is never called. |
-| test.cpp:69:17:69:18 | g4 | Function g4 is never called. |
+| test.cpp:79:6:79:21 | anUnusedFunction | Function anUnusedFunction is never called. |
+| test.cpp:113:17:113:18 | g4 | Function g4 is never called. |

cpp/autosar/test/rules/M0-1-10/UnusedSplMemberFunction.expected

@@ -0,0 +1,2 @@
+| test.cpp:71:5:71:16 | ANestedClass | Special member function ANestedClass is never called. |
+| test.cpp:82:5:82:22 | AnotherNestedClass | Special member function AnotherNestedClass is never called from a main function or entry point. |

cpp/autosar/test/rules/M0-1-10/UnusedSplMemberFunction.qlref

@@ -0,0 +1 @@
+rules/M0-1-10/UnusedSplMemberFunction.ql

cpp/autosar/test/rules/M0-1-10/test.cpp

@@ -52,6 +52,50 @@ template <class T> class C {
   inline void i5() {}     // NON_COMPLIANT - never used in any instantiation
 };
 
+#include "test.hpp"
+#include <type_traits>
+
+template <typename T1, typename T2>
+constexpr bool aConstExprFunc() noexcept { // COMPLIANT
+  static_assert(std::is_trivially_copy_constructible<T1>() &&
+                    std::is_trivially_copy_constructible<T2>(),
+                "assert");
+  return true;
+}
+
+template <typename T, int val> class AClass { T anArr[val]; };
+
+void aCalledFunc1() // COMPLIANT
+{
+  struct ANestedClass {
+    ANestedClass() noexcept(false) { // COMPLIANT: False Positive!
+      static_cast<void>(0);
+    }
+  };
+  static_assert(std::is_trivially_copy_constructible<AClass<ANestedClass, 5>>(),
+                "Must be trivially copy constructible");
+}
+
+void anUnusedFunction() // NON_COMPLIANT
+{
+  struct AnotherNestedClass {
+    AnotherNestedClass() noexcept(false) { // NON_COMPLAINT
+      static_cast<void>(0);
+    }
+  };
+  AnotherNestedClass d;
+}
+
+void aCalledFunc2() // COMPLIANT
+{
+  struct YetAnotherNestedClass {
+    YetAnotherNestedClass() noexcept(false) {
+      static_cast<void>(0);
+    } // COMPLIANT
+  };
+  YetAnotherNestedClass d;
+};
+
 int main() { // COMPLIANT - this is a main like function which acts as an entry
              // point
   f3();
@@ -88,8 +132,37 @@ int main() { // COMPLIANT - this is a main like function which acts as an entry
   c1.getAT();
   S s;
   c2.i1(s);
+
+  int aVar;
+  aConstExprFunc<decltype(aCalledFuncInHeader(aVar)), int>();
+  aCalledFunc1();
+  aCalledFunc2();
 }
 class M {
 public:
   M(const M &) = delete; // COMPLIANT - ignore if deleted
-};
+};
+
+#include <gtest/gtest.h>
+int called_from_google_test_function(
+    int a_param) // COMPLIANT - called from TEST
+{
+  int something = a_param;
+  something++;
+  return something;
+}
+
+TEST(sample_test,
+     called_from_google_test_function) // COMPLIANT - Google Test function
+{
+  bool pass = false;
+  if (called_from_google_test_function(0) >= 10)
+    pass = true;
+  struct a_nested_class_in_gtest {
+    a_nested_class_in_gtest() noexcept(false) {
+      static_cast<void>(0);
+    } // COMPLIANT
+  };
+  static_assert(std::is_trivially_copy_constructible<a_nested_class_in_gtest>(),
+                "assert");
+}

cpp/autosar/test/rules/M0-1-10/test.hpp

@@ -0,0 +1,4 @@
+template <class T>
+constexpr T aCalledFuncInHeader(T value) noexcept { // COMPLIANT
+  return static_cast<T>(value);
+}

cpp/common/src/codingstandards/cpp/EncapsulatingFunctions.qll

@@ -3,6 +3,7 @@
  */
 
 import cpp
+import codingstandards.cpp.Class
 
 /** A function which represents the entry point into a specific thread of execution in the program. */
 abstract class MainLikeFunction extends Function { }
@@ -18,6 +19,37 @@ class MainFunction extends MainLikeFunction {
   }
 }
 
+/**
+ * A test function from the GoogleTest infrastructure.
+ *
+ * Such functions can be treated as valid EntryPoint functions during analysis
+ * of "called" or "unused" functions. It is not straightforward to identify
+ * such functions, however, they have certain features that can be used for
+ * identification. This can be refined based on experiments/real-world use.
+ */
+class GoogleTestFunction extends MainLikeFunction {
+  GoogleTestFunction() {
+    // A GoogleTest function is named "TestBody" and
+    (
+      this.hasName("TestBody") or
+      this instanceof SpecialMemberFunction
+    ) and
+    // it's parent class inherits a base class
+    exists(Class base |
+      base = this.getEnclosingAccessHolder+().(Class).getABaseClass+() and
+      (
+        // with a name "Test" inside a namespace called "testing"
+        base.hasName("Test") and
+        base.getNamespace().hasName("testing")
+        or
+        // or at a location in a file called gtest.h (or gtest-internal.h,
+        // gtest-typed-test.h etc).
+        base.getDefinitionLocation().getFile().getBaseName().regexpMatch("gtest*.h")
+      )
+    )
+  }
+}
+
 /**
  * A "task main" function.
  */

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

@@ -13,6 +13,7 @@ import cpp
 import codingstandards.cpp.DynamicCallGraph
 import codingstandards.cpp.EncapsulatingFunctions
 import codingstandards.cpp.FunctionEquivalence
+import codingstandards.cpp.Class
 
 module UnusedFunctions {
   /**
@@ -119,7 +120,12 @@ module UnusedFunctions {
   class UnusedFunction extends UsableFunction {
     UnusedFunction() {
       // This function, or an equivalent function, is not reachable from any entry point
-      not exists(EntryPoint ep | getAnEquivalentFunction(this) = ep.getAReachableFunction())
+      not exists(EntryPoint ep | getAnEquivalentFunction(this) = ep.getAReachableFunction()) and
+      // and it is not a constexpr. Refer issue #646.
+      // The usages of constexpr is not well tracked and hence
+      // to avoid false positives, this is added. In case there is an improvement in
+      // handling constexpr in CodeQL, we can consider removing it.
+      not this.isConstexpr()
     }
 
     string getDeadCodeType() {
@@ -128,4 +134,7 @@ module UnusedFunctions {
       else result = "never called."
     }
   }
+
+  /** A `SpecialMemberFunction` which is an `UnusedFunction`. */
+  class UnusedSplMemberFunction extends UnusedFunction, SpecialMemberFunction { }
 }