diff --git a/change_notes/2023-01-09-cstylecasts-template-parameters.md b/change_notes/2023-01-09-cstylecasts-template-parameters.md new file mode 100644 index 0000000000..610c4b01fe --- /dev/null +++ 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 index e7f6e96eb5..c769339d65 100644 --- 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 index 291eb53348..a7b7eef66c 100644 --- 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 new file mode 100644 index 0000000000..a275a21895 --- /dev/null +++ b/cpp/autosar/test/rules/A5-2-2/options.clang @@ -0,0 +1 @@ +-I../../../../common/test/includes/custom-library \ No newline at end of file diff --git a/cpp/autosar/test/rules/A5-2-2/options.gcc b/cpp/autosar/test/rules/A5-2-2/options.gcc new file mode 100644 index 0000000000..a275a21895 --- /dev/null +++ b/cpp/autosar/test/rules/A5-2-2/options.gcc @@ -0,0 +1 @@ +-I../../../../common/test/includes/custom-library \ No newline at end of file diff --git a/cpp/autosar/test/rules/A5-2-2/test.cpp b/cpp/autosar/test/rules/A5-2-2/test.cpp index 6bcd09c777..fb39868560 100644 --- 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 -} \ No newline at end of file +} + +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 T testFunctionalCastTemplate() { + return T(1); // COMPLIANT[FALSE_POSITIVE] +} + +template T testFunctionalCastTemplateMulti() { + return T(1, 2); // COMPLIANT +} + +void testFunctionalCastTemplateUse() { + testFunctionalCastTemplate(); + testFunctionalCastTemplate(); + testFunctionalCastTemplateMulti(); +} + +template 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 e; + e.f(); + e.d(); + e.i(); +} diff --git a/rule_packages/cpp/BannedSyntax.json b/rule_packages/cpp/BannedSyntax.json index e2f3ce6ef7..0f559e60b7 100644 --- 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."