From dc0d14a0d7ca1d444c66075817eea535176c97da Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 9 Jan 2023 22:59:02 +0000 Subject: [PATCH 1/5] 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/TraditionalCStyleCastsUsed.ql | 30 +++++++++++++ cpp/autosar/test/rules/A5-2-2/test.cpp | 42 +++++++++++++++++-- rule_packages/cpp/BannedSyntax.json | 9 +++- 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql b/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql index e7f6e96eb5..e8b6b25d5b 100644 --- a/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql +++ b/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql @@ -44,11 +44,41 @@ 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 + // All c-style and functional notation casts on template parameters result in a `CStyleCast`. This + // is because the cast is only converted to a `ConstructorCall` (if appropriate) in the + // instantiated template. As a result, we exclude all casts to template parameters. + not c.getType() instanceof TemplateParameter 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/test.cpp b/cpp/autosar/test/rules/A5-2-2/test.cpp index 6bcd09c777..e9f680269d 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; @@ -45,12 +45,12 @@ void test_cpp_style_cast() { class A5_2_2a { public: template - static void Foo(const std::string &name, As &&...rest) { + static void Foo(const std::string &name, As &&... rest) { Fun(Log( std::forward(rest)...)); // COMPLIANT - reported as a false positive } - template static std::string Log(As &&...tail) { + template static std::string Log(As &&... tail) { return std::string(); } @@ -86,4 +86,40 @@ 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 T testFunctionalCastTemplate() { + return T(1); // COMPLIANT[FALSE_POSITIVE] +} + +template T testFunctionalCastTemplateMulti() { + return T(1, 2); // COMPLIANT +} + +void testFunctionalCastTemplateUse() { + testFunctionalCastTemplate(); + testFunctionalCastTemplate(); + testFunctionalCastTemplateMulti(); } \ No newline at end of file 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." From 7dbc9adc5510bc69058683fd17da96099ea8bbed Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 9 Jan 2023 23:03:11 +0000 Subject: [PATCH 2/5] Add change note --- change_notes/2023-01-09-cstylecasts-template-parameters.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 change_notes/2023-01-09-cstylecasts-template-parameters.md 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. From 8df881e098c826ca39b58b247ad778a832e442de Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 10 Jan 2023 10:19:01 +0000 Subject: [PATCH 3/5] 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. --- .../A5-2-2/TraditionalCStyleCastsUsed.ql | 10 +++--- .../TraditionalCStyleCastsUsed.expected | 2 ++ cpp/autosar/test/rules/A5-2-2/test.cpp | 35 +++++++++++++++++++ 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql b/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql index e8b6b25d5b..c769339d65 100644 --- a/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql +++ b/cpp/autosar/src/rules/A5-2-2/TraditionalCStyleCastsUsed.ql @@ -75,10 +75,12 @@ where not isExcluded(c, BannedSyntaxPackage::traditionalCStyleCastsUsedQuery()) and not c.isImplicit() and not c.getType() instanceof UnknownType and - // All c-style and functional notation casts on template parameters result in a `CStyleCast`. This - // is because the cast is only converted to a `ConstructorCall` (if appropriate) in the - // instantiated template. As a result, we exclude all casts to template parameters. - not c.getType() instanceof TemplateParameter 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/test.cpp b/cpp/autosar/test/rules/A5-2-2/test.cpp index e9f680269d..a9bf492a00 100644 --- a/cpp/autosar/test/rules/A5-2-2/test.cpp +++ b/cpp/autosar/test/rules/A5-2-2/test.cpp @@ -122,4 +122,39 @@ 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(); } \ No newline at end of file From 602ab4b8b8b62b82598bb34ade7f92bce9414f07 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 6 Mar 2023 21:34:00 +0100 Subject: [PATCH 4/5] Fix test formatting --- cpp/autosar/test/rules/A5-2-2/test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/autosar/test/rules/A5-2-2/test.cpp b/cpp/autosar/test/rules/A5-2-2/test.cpp index a9bf492a00..fb39868560 100644 --- a/cpp/autosar/test/rules/A5-2-2/test.cpp +++ b/cpp/autosar/test/rules/A5-2-2/test.cpp @@ -45,12 +45,12 @@ void test_cpp_style_cast() { class A5_2_2a { public: template - static void Foo(const std::string &name, As &&... rest) { + static void Foo(const std::string &name, As &&...rest) { Fun(Log( std::forward(rest)...)); // COMPLIANT - reported as a false positive } - template static std::string Log(As &&... tail) { + template static std::string Log(As &&...tail) { return std::string(); } @@ -157,4 +157,4 @@ void testE() { e.f(); e.d(); e.i(); -} \ No newline at end of file +} From 396bdf0e10900e5e754e1bee217b31b2283595f3 Mon Sep 17 00:00:00 2001 From: Mauro Baluda Date: Mon, 6 Mar 2023 22:04:58 +0100 Subject: [PATCH 5/5] Include custom library --- cpp/autosar/test/rules/A5-2-2/options.clang | 1 + cpp/autosar/test/rules/A5-2-2/options.gcc | 1 + 2 files changed, 2 insertions(+) create mode 100644 cpp/autosar/test/rules/A5-2-2/options.clang create mode 100644 cpp/autosar/test/rules/A5-2-2/options.gcc 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