A5-2-2: Exclude results in uninstantiated templates, explain limitations (#160)

* A5-2-2: Clarify c-style casts scope, exclude templates

Clarify what `CStyleCast` does and does not cover by adding a comment,
expanding the test case and providing an implementation scope. In
addition, exclude casts on template parameters to avoid unnecessary
false positives.

* A5-2-2: Exclude uninstantiated templates

Any cast in an uninstantiated template that is related to the template
parameter may be converted to a `ConstructorCall` when the template is
instantiated. To avoid the common false positive case where the
functional cast notation is used to call a constructor, we exclude all
results in uninstantiated templates and instead rely on reporting
results in template instantiations instead.

---------

Co-authored-by: Mauro Baluda <mbaluda@github.com>

Author: lcartey

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.

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

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)... |  |

cpp/autosar/test/rules/A5-2-2/options.clang

@@ -0,0 +1 @@
+-I../../../../common/test/includes/custom-library

cpp/autosar/test/rules/A5-2-2/options.gcc

@@ -0,0 +1 @@
+-I../../../../common/test/includes/custom-library

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();
+}

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."