From de24d43f7c5590528e026a6ea21f76d7a971a8a3 Mon Sep 17 00:00:00 2001 From: "rakesh.pothengil" Date: Mon, 11 Nov 2024 13:17:25 +0900 Subject: [PATCH 1/4] Fix #789 --- change_notes/2024-11-11-fix-fp-789.md | 2 ++ .../rules/A7-1-2/VariableMissingConstexpr.ql | 6 +++++ .../A7-1-2/VariableMissingConstexpr.expected | 5 ++-- cpp/autosar/test/rules/A7-1-2/test.cpp | 25 +++++++++++++------ cpp/common/src/codingstandards/cpp/Expr.qll | 4 ++- 5 files changed, 31 insertions(+), 11 deletions(-) create mode 100644 change_notes/2024-11-11-fix-fp-789.md diff --git a/change_notes/2024-11-11-fix-fp-789.md b/change_notes/2024-11-11-fix-fp-789.md new file mode 100644 index 000000000..6ba34dbd7 --- /dev/null +++ b/change_notes/2024-11-11-fix-fp-789.md @@ -0,0 +1,2 @@ +- `A7-1-2` - `VariableMissingConstexpr.ql`: + - Fixes #789. Doesn't alert on non-static member variables and compiler generated variables of range based for-loops. diff --git a/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql b/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql index f0adab07d..b051965a5 100644 --- a/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql +++ b/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql @@ -42,6 +42,12 @@ where not v.isConstexpr() and not v instanceof Parameter and not v.isAffectedByMacro() and + // Don't consider non-static member variables. + ( + not v instanceof MemberVariable + or + v.isStatic() + ) and isLiteralType(v.getType()) and // Unfortunately, `isConstant` is not sufficient here because it doesn't include calls to // constexpr constructors, and does not take into account zero initialization diff --git a/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected b/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected index f86faf1a7..ee33044a2 100644 --- a/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected +++ b/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected @@ -7,9 +7,8 @@ | test.cpp:41:14:41:15 | l2 | Variable 'l2' could be marked 'constexpr'. | | test.cpp:44:16:44:17 | lc | Variable 'lc' could be marked 'constexpr'. | | test.cpp:45:17:45:19 | lc2 | Variable 'lc2' could be marked 'constexpr'. | -| test.cpp:55:7:55:8 | m2 | Variable 'm2' could be marked 'constexpr'. | -| test.cpp:130:7:130:8 | m1 | Variable 'm1' could be marked 'constexpr'. | -| test.cpp:141:7:141:8 | m1 | Variable 'm1' could be marked 'constexpr'. | +| test.cpp:55:20:55:21 | m2 | Variable 'm2' could be marked 'constexpr'. | +| test.cpp:143:5:143:20 | m1 | Variable 'm1' could be marked 'constexpr'. | | test.cpp:221:7:221:8 | l1 | Variable 'l1' could be marked 'constexpr'. | | test.cpp:235:7:235:8 | l6 | Variable 'l6' could be marked 'constexpr'. | | test.cpp:237:7:237:8 | l8 | Variable 'l8' could be marked 'constexpr'. | diff --git a/cpp/autosar/test/rules/A7-1-2/test.cpp b/cpp/autosar/test/rules/A7-1-2/test.cpp index 8395f60ff..3b45516bc 100644 --- a/cpp/autosar/test/rules/A7-1-2/test.cpp +++ b/cpp/autosar/test/rules/A7-1-2/test.cpp @@ -51,9 +51,9 @@ class MemberConstExpr { MemberConstExpr(int p3) : m3(p3) {} private: - int m1; // COMPLIANT - is not always zero initialized - int m2 = 0; // NON_COMPLIANT - int m3 = 0; // COMPLIANT - can be set by constructor + int m1; // COMPLIANT - is not always zero initialized + static const int m2 = 0; // NON_COMPLIANT + int m3 = 0; // COMPLIANT - can be set by constructor }; int h1(int x, int y) { // NON_COMPLIANT @@ -127,7 +127,7 @@ class MissingConstexprClass { MissingConstexprClass(int i) = delete; // NON_COMPLIANT MissingConstexprClass(int i, LiteralClass lc) {} // NON_COMPLIANT private: - int m1 = 0; + int m1 = 0; // COMPLIANT - non-static member variable }; class VirtualBaseClass {}; @@ -138,9 +138,9 @@ class DerivedClass : public virtual VirtualBaseClass { DerivedClass(int i) = delete; // COMPLIANT DerivedClass(int i, LiteralClass lc) {} // COMPLIANT private: - int m1 = 0; + static int m1; // NON_COMPLAINT - static member variable can be constexpr }; - +int DerivedClass::m1 = 0; class NotAllMembersInitializedClass { public: NotAllMembersInitializedClass() = default; // COMPLIANT @@ -274,4 +274,15 @@ template T *init() { return t; } -void test_template_instantiation() { int *t = init(); } \ No newline at end of file +void test_template_instantiation() { int *t = init(); } + +#include +#include +void a_function() { + auto origin = std::vector{1, 2, 3, 4, 5, 6, 7, 8, 9}; + auto values = std::vector>{}; + for (auto &value : + origin) { // Sometimes, CodeQL reports "value" should be constexpr + values.emplace_back(std::make_unique(value)); + } +} diff --git a/cpp/common/src/codingstandards/cpp/Expr.qll b/cpp/common/src/codingstandards/cpp/Expr.qll index fe2877f84..90730b971 100644 --- a/cpp/common/src/codingstandards/cpp/Expr.qll +++ b/cpp/common/src/codingstandards/cpp/Expr.qll @@ -267,7 +267,9 @@ predicate isCompileTimeEvaluatedCall(Call call) { parameterUsingDefaultValue.getAnAssignedValue() = defaultValue | isDirectCompileTimeEvaluatedExpression(defaultValue) - ) + ) and + // 4. the call's qualifier is compile time evaluated. + (not call.hasQualifier() or isCompileTimeEvaluatedExpression(call.getQualifier())) } /* From d584e309b212ad29e05e74a4a0760fb567ed92e7 Mon Sep 17 00:00:00 2001 From: "rakesh.pothengil" Date: Mon, 18 Nov 2024 13:12:44 +0900 Subject: [PATCH 2/4] Consider feedback on #789 --- .../rules/A7-1-2/VariableMissingConstexpr.ql | 17 ++++++++++---- .../A7-1-2/VariableMissingConstexpr.expected | 5 ++-- cpp/autosar/test/rules/A7-1-2/test.cpp | 23 +++++-------------- 3 files changed, 21 insertions(+), 24 deletions(-) diff --git a/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql b/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql index b051965a5..a07dbd43f 100644 --- a/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql +++ b/cpp/autosar/src/rules/A7-1-2/VariableMissingConstexpr.ql @@ -35,18 +35,24 @@ predicate isTypeZeroInitializable(Type t) { t.getUnderlyingType() instanceof ArrayType } -from Variable v +from Variable v, string msg where not isExcluded(v, ConstPackage::variableMissingConstexprQuery()) and v.hasDefinition() and not v.isConstexpr() and not v instanceof Parameter and not v.isAffectedByMacro() and - // Don't consider non-static member variables. ( not v instanceof MemberVariable or - v.isStatic() + // In case member functions are left un-instantiated, it is possible + // the member variable could be modified in them. + // Hence, don't raise an alert in case this member variable's class + // has a member function that doesn't have a definition. + not exists(MemberFunction mf | + mf.getDeclaringType() = v.getDeclaringType() and + mf.isFromUninstantiatedTemplate(_) + ) ) and isLiteralType(v.getType()) and // Unfortunately, `isConstant` is not sufficient here because it doesn't include calls to @@ -72,5 +78,6 @@ where // Exclude variables in uninstantiated templates, as they may be incomplete not v.isFromUninstantiatedTemplate(_) and // Exclude compiler generated variables, which are not user controllable - not v.isCompilerGenerated() -select v, "Variable '" + v.getName() + "' could be marked 'constexpr'." + not v.isCompilerGenerated() and + if v instanceof MemberVariable and not v.isStatic() then msg = " and static." else msg = "." +select v, "Variable '" + v.getName() + "' could be marked 'constexpr'" + msg diff --git a/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected b/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected index ee33044a2..31c26a11f 100644 --- a/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected +++ b/cpp/autosar/test/rules/A7-1-2/VariableMissingConstexpr.expected @@ -7,8 +7,9 @@ | test.cpp:41:14:41:15 | l2 | Variable 'l2' could be marked 'constexpr'. | | test.cpp:44:16:44:17 | lc | Variable 'lc' could be marked 'constexpr'. | | test.cpp:45:17:45:19 | lc2 | Variable 'lc2' could be marked 'constexpr'. | -| test.cpp:55:20:55:21 | m2 | Variable 'm2' could be marked 'constexpr'. | -| test.cpp:143:5:143:20 | m1 | Variable 'm1' could be marked 'constexpr'. | +| test.cpp:55:7:55:8 | m2 | Variable 'm2' could be marked 'constexpr' and static. | +| test.cpp:130:7:130:8 | m1 | Variable 'm1' could be marked 'constexpr' and static. | +| test.cpp:141:7:141:8 | m1 | Variable 'm1' could be marked 'constexpr' and static. | | test.cpp:221:7:221:8 | l1 | Variable 'l1' could be marked 'constexpr'. | | test.cpp:235:7:235:8 | l6 | Variable 'l6' could be marked 'constexpr'. | | test.cpp:237:7:237:8 | l8 | Variable 'l8' could be marked 'constexpr'. | diff --git a/cpp/autosar/test/rules/A7-1-2/test.cpp b/cpp/autosar/test/rules/A7-1-2/test.cpp index 3b45516bc..1bbe32a93 100644 --- a/cpp/autosar/test/rules/A7-1-2/test.cpp +++ b/cpp/autosar/test/rules/A7-1-2/test.cpp @@ -51,9 +51,9 @@ class MemberConstExpr { MemberConstExpr(int p3) : m3(p3) {} private: - int m1; // COMPLIANT - is not always zero initialized - static const int m2 = 0; // NON_COMPLIANT - int m3 = 0; // COMPLIANT - can be set by constructor + int m1; // COMPLIANT - is not always zero initialized + int m2 = 0; // NON_COMPLIANT + int m3 = 0; // COMPLIANT - can be set by constructor }; int h1(int x, int y) { // NON_COMPLIANT @@ -127,7 +127,7 @@ class MissingConstexprClass { MissingConstexprClass(int i) = delete; // NON_COMPLIANT MissingConstexprClass(int i, LiteralClass lc) {} // NON_COMPLIANT private: - int m1 = 0; // COMPLIANT - non-static member variable + int m1 = 0; }; class VirtualBaseClass {}; @@ -138,9 +138,9 @@ class DerivedClass : public virtual VirtualBaseClass { DerivedClass(int i) = delete; // COMPLIANT DerivedClass(int i, LiteralClass lc) {} // COMPLIANT private: - static int m1; // NON_COMPLAINT - static member variable can be constexpr + int m1 = 0; }; -int DerivedClass::m1 = 0; + class NotAllMembersInitializedClass { public: NotAllMembersInitializedClass() = default; // COMPLIANT @@ -275,14 +275,3 @@ template T *init() { } void test_template_instantiation() { int *t = init(); } - -#include -#include -void a_function() { - auto origin = std::vector{1, 2, 3, 4, 5, 6, 7, 8, 9}; - auto values = std::vector>{}; - for (auto &value : - origin) { // Sometimes, CodeQL reports "value" should be constexpr - values.emplace_back(std::make_unique(value)); - } -} From e736cb0dd3c50a7648b89c66f7a2b6f2ad52b468 Mon Sep 17 00:00:00 2001 From: "rakesh.pothengil" Date: Mon, 18 Nov 2024 13:18:14 +0900 Subject: [PATCH 3/4] Update change notes --- change_notes/2024-11-11-fix-fp-789.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change_notes/2024-11-11-fix-fp-789.md b/change_notes/2024-11-11-fix-fp-789.md index 6ba34dbd7..b06ebb9b1 100644 --- a/change_notes/2024-11-11-fix-fp-789.md +++ b/change_notes/2024-11-11-fix-fp-789.md @@ -1,2 +1,3 @@ - `A7-1-2` - `VariableMissingConstexpr.ql`: - - Fixes #789. Doesn't alert on non-static member variables and compiler generated variables of range based for-loops. + - Do not report on member variables if the class has un-instantiated member function(s). + - Check a call's qualifier as well whether it can be compile time evaluated or not. From e9b3ebd1c9e4e92d83ac47a6a65f119709e0a5a5 Mon Sep 17 00:00:00 2001 From: "rakesh.pothengil" Date: Mon, 25 Nov 2024 10:15:29 +0900 Subject: [PATCH 4/4] Add markers for non-compliant cases --- cpp/autosar/test/rules/A7-1-2/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/autosar/test/rules/A7-1-2/test.cpp b/cpp/autosar/test/rules/A7-1-2/test.cpp index 1bbe32a93..664a9cb8e 100644 --- a/cpp/autosar/test/rules/A7-1-2/test.cpp +++ b/cpp/autosar/test/rules/A7-1-2/test.cpp @@ -127,7 +127,7 @@ class MissingConstexprClass { MissingConstexprClass(int i) = delete; // NON_COMPLIANT MissingConstexprClass(int i, LiteralClass lc) {} // NON_COMPLIANT private: - int m1 = 0; + int m1 = 0; // NON_COMPLIANT }; class VirtualBaseClass {}; @@ -138,7 +138,7 @@ class DerivedClass : public virtual VirtualBaseClass { DerivedClass(int i) = delete; // COMPLIANT DerivedClass(int i, LiteralClass lc) {} // COMPLIANT private: - int m1 = 0; + int m1 = 0; // NON_COMPLIANT }; class NotAllMembersInitializedClass {