Merge branch 'main' into jsinglet/compiler-path-fix

jsinglet · web-flow · commit e3b9a555047a · 2023-03-07T13:59:51.000-05:00
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/change_notes/2023-03-06-A13-2-2-message.md b/change_notes/2023-03-06-A13-2-2-message.md
@@ -0,0 +1,2 @@
+ - `A13-2-2` - `BinaryOperatorAndBitwiseOperatorReturnAPrvalue.ql`
+   - The formatting of the query output message has been changed and operators are now displayed starting with the return type instead of ending with it.
diff --git a/cpp/autosar/src/rules/A12-1-1/ExplicitConstructorBaseClassInitialization.ql b/cpp/autosar/src/rules/A12-1-1/ExplicitConstructorBaseClassInitialization.ql
@@ -35,7 +35,7 @@ where
     not init.isCompilerGenerated()
   ) and
   // Must be a defined constructor
-  c.isDefined() and
+  c.hasDefinition() and
   // Not a compiler-generated constructor
   not c.isCompilerGenerated() and
   // Not a defaulted constructor
diff --git a/cpp/autosar/src/rules/A13-2-2/BinaryOperatorAndBitwiseOperatorReturnAPrvalue.ql b/cpp/autosar/src/rules/A13-2-2/BinaryOperatorAndBitwiseOperatorReturnAPrvalue.ql
@@ -17,6 +17,7 @@
 import cpp
 import codingstandards.cpp.autosar
 import codingstandards.cpp.Operator
+import semmle.code.cpp.Print
 
 from Operator o
 where
@@ -30,5 +31,5 @@ where
     o.getType() instanceof ReferenceType
   )
 select o,
-  "User-defined bitwise or arithmetic operator " + o.getFullSignature() +
+  "User-defined bitwise or arithmetic operator " + getIdentityString(o) +
     " does not return a prvalue."
diff --git a/cpp/autosar/src/rules/A2-10-4/IdentifierNameOfStaticFunctionReusedInNamespace.ql b/cpp/autosar/src/rules/A2-10-4/IdentifierNameOfStaticFunctionReusedInNamespace.ql
@@ -17,7 +17,7 @@ import codingstandards.cpp.autosar
 
 class CandidateFunction extends Function {
   CandidateFunction() {
-    isDefined() and
+    hasDefinition() and
     isStatic() and
     not isMember() and
     not (
diff --git a/cpp/autosar/src/rules/A3-1-1/ViolationsOfOneDefinitionRule.ql b/cpp/autosar/src/rules/A3-1-1/ViolationsOfOneDefinitionRule.ql
@@ -65,7 +65,7 @@ where
     or
     //an non-const object defined in a header
     exists(GlobalOrNamespaceVariable object |
-      object.isDefined() and
+      object.hasDefinition() and
       not (
         object.isConstexpr()
         or
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/A13-2-2/BinaryOperatorAndBitwiseOperatorReturnAPrvalue.expected b/cpp/autosar/test/rules/A13-2-2/BinaryOperatorAndBitwiseOperatorReturnAPrvalue.expected
@@ -1,4 +1,4 @@
-| test.cpp:16:9:16:17 | operator- | User-defined bitwise or arithmetic operator operator-(const A &, int) -> const A does not return a prvalue. |
-| test.cpp:20:4:20:12 | operator\| | User-defined bitwise or arithmetic operator operator\|(const A &, const A &) -> A * does not return a prvalue. |
-| test.cpp:24:9:24:18 | operator<< | User-defined bitwise or arithmetic operator operator<<(const A &, const A &) -> const A does not return a prvalue. |
-| test.cpp:34:6:34:14 | operator+ | User-defined bitwise or arithmetic operator NS_C::operator+(const C &, const C &) -> int & does not return a prvalue. |
+| test.cpp:16:9:16:17 | operator- | User-defined bitwise or arithmetic operator A const operator-(A const&, int) does not return a prvalue. |
+| test.cpp:20:4:20:12 | operator\| | User-defined bitwise or arithmetic operator A* operator\|(A const&, A const&) does not return a prvalue. |
+| test.cpp:24:9:24:18 | operator<< | User-defined bitwise or arithmetic operator A const operator<<(A const&, A const&) does not return a prvalue. |
+| test.cpp:34:6:34:14 | operator+ | User-defined bitwise or arithmetic operator int& NS_C::operator+(C const&, C const&) does not return a prvalue. |
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/cpp/common/src/codingstandards/cpp/TrivialType.qll b/cpp/common/src/codingstandards/cpp/TrivialType.qll
@@ -48,7 +48,7 @@ predicate hasTrivialMoveConstructor(Class c) {
   forall(Class baseClass | baseClass = c.getABaseClass() | hasTrivialMoveConstructor(baseClass)) and
   // The class has to be defined, otherwise we may not see the information required to deduce
   // whether it does or does not have a trivial move constructor
-  c.isDefined()
+  c.hasDefinition()
 }
 
 /** A trivial copy or move constructor (see [class.copy]/12). */
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 @@`
	`1`	+ - `A5-2-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,2 @@`
	`1`	+ - `A13-2-2` - `BinaryOperatorAndBitwiseOperatorReturnAPrvalue.ql`
	`2`	`+ - The formatting of the query output message has been changed and operators are now displayed starting with the return type instead of ending with it.`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+-I../../../../common/test/includes/custom-library`