github · mbaluda · Mar 6, 2023 · Jan 9, 2023 · Jan 9, 2023 · Jan 10, 2023
diff --git a/change_notes/2023-01-09-cstylecasts-template-parameters.md b/change_notes/2023-01-09-cstylecasts-template-parameters.md
@@ -0,0 +1,2 @@
+ - `A5-2-2`
+   - `CStyleCasts.ql` - exclude template parameters to avoid false positives when using the "functional notation" syntax. In addition, provide a greater explanation on limitations of this query.
diff --git a/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql b/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql
@@ -44,11 +44,43 @@ class LibraryMacro extends Macro {
  LibraryMacro() { not this instanceof UserProvidedMacro }
 }

+/*
+ * In theory this query should exclude casts using the "functional notation" syntax, e.g.
+ * ```
+ * int(x);
+ * ```
+ * This is because this is not a C-style cast, as it is not legitimate C code. However, our database
+ * schema does not distinguish between C-style casts and functional casts, so we cannot exclude just
+ * those.
+ *
+ * In addition, we do not get `CStyleCasts` in cases where the cast is converted to a `ConstructorCall`.
+ * This holds for both the "functional notation" syntax and the "c-style" syntax, e.g. both of these
+ * are represented in our model as `ConstructorCall`s only:
+ * ```
+ * class A { public: A(int); };
+ * void create() {
+ *   (A)1;
+ *   A(1);
+ * }
+ * ```
+ *
+ * As a consequence this query:
+ *  - Produces false positives when primitive types are cast using the "functional notation" syntax.
+ *  - Produces false negatives when a C-style cast is converted to a `ConstructorCall` e.g. when the
+ *    argument type is compatible with a single-argument constructor.
+ */
+
 from CStyleCast c, string extraMessage, Locatable l, string supplementary
 where
  not isExcluded(c, BannedSyntaxPackage::traditionalCStyleCastsUsedQuery()) and
  not c.isImplicit() and
  not c.getType() instanceof UnknownType and
+  // For casts in templates that occur on types related to a template parameter, the copy of th
+  // cast in the uninstantiated template is represented as a `CStyleCast` even if in practice all
+  // the instantiations represent it as a `ConstructorCall`. To avoid the common false positive case
+  // of using the functional cast notation to call a constructor we exclude all `CStyleCast`s on
+  // uninstantiated templates, and instead rely on reporting results within instantiations.
+  not c.isFromUninstantiatedTemplate(_) and
  // Exclude casts created from macro invocations of macros defined by third parties
  not getGeneratedFrom(c) instanceof LibraryMacro and
  // If the cast was generated from a user-provided macro, then report the macro that generated the

diff --git a/cpp/autosar/test/rules/A5-2-2/TraditionalCStyleCastsUsed.expected b/cpp/autosar/test/rules/A5-2-2/TraditionalCStyleCastsUsed.expected
@@ -5,3 +5,5 @@
 | test.cpp:79:3:79:18 | (int)... | Use of explicit c-style cast to int generated from macro $@. | test.cpp:71:1:71:36 | #define NESTED_ADD_ONE(x) ADD_ONE(x) | NESTED_ADD_ONE |
 | test.cpp:85:19:85:26 | (int)... | Use of explicit c-style cast to int. | test.cpp:85:19:85:26 | (int)... |  |
 | test.cpp:86:27:86:34 | (int)... | Use of explicit c-style cast to int. | test.cpp:86:27:86:34 | (int)... |  |
+| test.cpp:114:10:114:13 | (int)... | Use of explicit c-style cast to int. | test.cpp:114:10:114:13 | (int)... |  |
+| test.cpp:149:12:149:26 | (unsigned int)... | Use of explicit c-style cast to unsigned int. | test.cpp:149:12:149:26 | (unsigned int)... |  |
diff --git a/cpp/autosar/test/rules/A5-2-2/options.clang b/cpp/autosar/test/rules/A5-2-2/options.clang
@@ -0,0 +1 @@
+-I../../../../common/test/includes/custom-library
diff --git a/cpp/autosar/test/rules/A5-2-2/options.gcc b/cpp/autosar/test/rules/A5-2-2/options.gcc
@@ -0,0 +1 @@
+-I../../../../common/test/includes/custom-library
diff --git a/cpp/autosar/test/rules/A5-2-2/test.cpp b/cpp/autosar/test/rules/A5-2-2/test.cpp
@@ -6,7 +6,7 @@ int foo() { return 1; }
 void test_c_style_cast() {
  double f = 3.14;
  std::uint32_t n1 = (std::uint32_t)f; // NON_COMPLIANT - C-style cast
-  std::uint32_t n2 = unsigned(f);      // NON_COMPLIANT - functional cast
+  std::uint32_t n2 = unsigned(f);      // COMPLIANT[FALSE_POSITIVE]

  std::uint8_t n3 = 1;
  std::uint8_t n4 = 1;
@@ -86,4 +86,75 @@ void test_macro_cast() {
  LIBRARY_NO_CAST_ADD_TWO((int)1.0); // NON_COMPLIANT - library macro with
                                     // c-style cast in argument, written by
                                     // user so should be reported
-}
+}
+
+class D {
+public:
+  D(int x) : fx(x), fy(0) {}
+  D(int x, int y) : fx(x), fy(y) {}
+
+private:
+  int fx;
+  int fy;
+};
+
+D testNonFunctionalCast() {
+  return (D)1; // NON_COMPLIANT[FALSE_NEGATIVE]
+}
+
+D testFunctionalCast() {
+  return D(1); // COMPLIANT
+}
+
+D testFunctionalCastMulti() {
+  return D(1, 2); // COMPLIANT
+}
+
+template <typename T> T testFunctionalCastTemplate() {
+  return T(1); // COMPLIANT[FALSE_POSITIVE]
+}
+
+template <typename T> T testFunctionalCastTemplateMulti() {
+  return T(1, 2); // COMPLIANT
+}
+
+void testFunctionalCastTemplateUse() {
+  testFunctionalCastTemplate<D>();
+  testFunctionalCastTemplate<int>();
+  testFunctionalCastTemplateMulti<D>();
+}
+
+template <typename T> class E {
+public:
+  class F {
+  public:
+    F(int x) : fx(x), fy(0) {}
+    F(int x, int y) : fx(x), fy(y) {}
+
+  private:
+    int fx;
+    int fy;
+  };
+
+  F f() {
+    return F(1); // COMPLIANT
+  }
+
+  D d() {
+    return D(1); // COMPLIANT
+  }
+
+  int i() {
+    double f = 3.14;
+    return (unsigned int)f; // NON_COMPLIANT
+  }
+};
+
+class G {};
+
+void testE() {
+  E<G> e;
+  e.f();
+  e.d();
+  e.i();
+}
diff --git a/rule_packages/cpp/BannedSyntax.json b/rule_packages/cpp/BannedSyntax.json
@@ -141,7 +141,14 @@
          "tags": [
            "correctness",
            "scope/single-translation-unit"
-          ]
+          ],
+          "implementation_scope": {
+            "description": "This query has the following limitations:",
+            "items": [
+              "It erroneously reports functional notation casts on primitive types (e.g. int(x)) as traditional C-style casts.",
+              "It will not report C-Style casts that result in a direct initialization via a constructor call with the given argument."
+            ]
+          }
        }
      ],
      "title": "Traditional C-style casts shall not be used."
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		- `A5-2-2`
		- `CStyleCasts.ql` - exclude template parameters to avoid false positives when using the "functional notation" syntax. In addition, provide a greater explanation on limitations of this query.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		-I../../../../common/test/includes/custom-library