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 { }
 }

cpp/common/src/codingstandards/cpp/exclusions/cpp/DeadCode.qll

@@ -12,6 +12,7 @@ newtype DeadCodeQuery =
   TUnusedTypeDeclarationsQuery() or
   TUnreachableCodeQuery() or
   TUnusedFunctionQuery() or
+  TUnusedSplMemberFunctionQuery() or
   TInfeasiblePathQuery() or
   TUnusedLocalVariableQuery() or
   TUnusedGlobalOrNamespaceVariableQuery() or
@@ -94,6 +95,15 @@ predicate isDeadCodeQueryMetadata(Query query, string queryId, string ruleId, st
   ruleId = "M0-1-10" and
   category = "advisory"
   or
+  query =
+    // `Query` instance for the `unusedSplMemberFunction` query
+    DeadCodePackage::unusedSplMemberFunctionQuery() and
+  queryId =
+    // `@id` for the `unusedSplMemberFunction` query
+    "cpp/autosar/unused-spl-member-function" and
+  ruleId = "M0-1-10" and
+  category = "advisory"
+  or
   query =
     // `Query` instance for the `infeasiblePath` query
     DeadCodePackage::infeasiblePathQuery() and
@@ -224,6 +234,13 @@ module DeadCodePackage {
       TQueryCPP(TDeadCodePackageQuery(TUnusedFunctionQuery()))
   }
 
+  Query unusedSplMemberFunctionQuery() {
+    //autogenerate `Query` type
+    result =
+      // `Query` type for `unusedSplMemberFunction` query
+      TQueryCPP(TDeadCodePackageQuery(TUnusedSplMemberFunctionQuery()))
+  }
+
   Query infeasiblePathQuery() {
     //autogenerate `Query` type
     result =

cpp/common/test/includes/custom-library/gtest/gtest-internal.h

@@ -0,0 +1,29 @@
+#ifndef GOOGLETEST_INCLUDE_GTEST_INTERNAL_GTEST_INTERNAL_H_
+#define GOOGLETEST_INCLUDE_GTEST_INTERNAL_GTEST_INTERNAL_H_
+
+#define GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) \
+  test_suite_name##_##test_name##_Test
+
+#define GTEST_TEST_(test_suite_name, test_name, parent_class)                   \
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)                       \
+      : public parent_class {                                                   \
+      public:                                                                   \
+    GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() = default;             \
+    ~GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)() override = default;   \
+    GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)                          \
+    (const GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) &) = delete;      \
+    GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) & operator=(             \
+        const GTEST_TEST_CLASS_NAME_(test_suite_name,                           \
+                                     test_name) &) = delete; /* NOLINT */       \
+    GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)                          \
+    (GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) &&) noexcept = delete;  \
+    GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) & operator=(             \
+        GTEST_TEST_CLASS_NAME_(test_suite_name,                                 \
+                               test_name) &&) noexcept = delete; /* NOLINT */   \
+                                                                                \
+   private:                                                                     \
+    void TestBody() override;                                                            \
+    };                                                                          \
+    void GTEST_TEST_CLASS_NAME_(test_suite_name, test_name)::TestBody()         \
+
+#endif // GOOGLETEST_INCLUDE_GTEST_INTERNAL_GTEST_INTERNAL_H_

cpp/common/test/includes/custom-library/gtest/gtest.h

@@ -0,0 +1,28 @@
+#ifndef GOOGLETEST_INCLUDE_GTEST_GTEST_H_
+#define GOOGLETEST_INCLUDE_GTEST_GTEST_H_
+
+#include "gtest/gtest-internal.h"
+
+namespace testing {
+
+class Test
+{
+  public:
+  virtual ~Test();
+  protected:
+  // Creates a Test object.
+  Test();
+  private:
+  virtual void TestBody() = 0;
+  Test(const Test&) = delete;
+  Test& operator=(const Test&) = delete;
+};
+
+#define GTEST_TEST(test_suite_name, test_name)             \
+  GTEST_TEST_(test_suite_name, test_name, ::testing::Test)
+
+#define TEST(test_suite_name, test_name) GTEST_TEST(test_suite_name, test_name)
+
+} // namespace testing
+
+#endif  // GOOGLETEST_INCLUDE_GTEST_GTEST_H_

cpp/options

@@ -1 +1 @@
-semmle-extractor-options:--clang -std=c++14 -nostdinc++ -I../../../../common/test/includes/standard-library -I../../../../common/test/includes/custom-library
+semmle-extractor-options:--clang -std=c++14 -nostdinc++ -I../../../../common/test/includes/standard-library -I../../../../common/test/includes/custom-library

rule_packages/cpp/DeadCode.json

@@ -194,6 +194,21 @@
             "readability",
             "maintainability"
           ]
+        },
+        {
+          "description": "Uncalled functions complicate the program and can indicate a possible mistake on the part of the programmer.",
+          "kind": "problem",
+          "name": "Every defined function should be called at least once",
+          "precision": "medium",
+          "severity": "warning",
+          "short_name": "UnusedSplMemberFunction",
+          "tags": [
+            "readability",
+            "maintainability"
+          ],
+          "implementation_scope": {
+            "description": "In limited cases, this query can raise false-positives for special member function calls invoked from the C++ Metaprogramming library."
+          }
         }
       ],
       "title": "Every defined function should be called at least once."