diff --git a/change_notes/2024-03-01-fix-fp-a12-4-1-and-a12-8-6.md b/change_notes/2024-03-01-fix-fp-a12-4-1-and-a12-8-6.md new file mode 100644 index 0000000000..7ba99b44f1 --- /dev/null +++ b/change_notes/2024-03-01-fix-fp-a12-4-1-and-a12-8-6.md @@ -0,0 +1,6 @@ +- `A12-4-1` - `DestructorOfABaseClassNotPublicVirtual.ql`: + - Fix FP reported in #392. Improve base class detection for template classes. + - Update the alert message to prevent duplicate alerts for base classes that are both derived and abstract. +- `A12-8-6` - `CopyAndMoveNotDeclaredProtected.ql`: + - Fix FP reported in #392. Improve base class detection for template classes. + - Update the alert message to prevent duplicate alerts for base classes that are both derived and abstract. diff --git a/cpp/autosar/src/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.ql b/cpp/autosar/src/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.ql index 4743e0b529..c534f9e591 100644 --- a/cpp/autosar/src/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.ql +++ b/cpp/autosar/src/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.ql @@ -29,9 +29,9 @@ predicate isProtectedNonVirtual(Destructor d) { d.isProtected() and not d.isVirt from Destructor d where not isExcluded(d, VirtualFunctionsPackage::destructorOfABaseClassNotPublicVirtualQuery()) and - isPossibleBaseClass(d.getDeclaringType(), _) and + d.getDeclaringType() instanceof BaseClass and (not isPublicOverride(d) and not isProtectedNonVirtual(d) and not isPublicVirtual(d)) // Report the declaration entry in the class body, as that is where the access specifier should be set select getDeclarationEntryInClassDeclaration(d), - "Destructor of base class " + d.getDeclaringType() + - " is not declared as public virtual, public override, or protected non-virtual." + "Destructor of base class '" + d.getDeclaringType().getQualifiedName() + + "' is not declared as public virtual, public override, or protected non-virtual." diff --git a/cpp/autosar/src/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.ql b/cpp/autosar/src/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.ql index 0098c7e43f..7507eb1d7c 100644 --- a/cpp/autosar/src/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.ql +++ b/cpp/autosar/src/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.ql @@ -16,35 +16,57 @@ import cpp import codingstandards.cpp.autosar import codingstandards.cpp.Class -predicate isInvalidConstructor(Constructor f, string constructorType) { +predicate isInvalidConstructor(Constructor f, string constructorType, string missingAction) { not f.isDeleted() and not f.isProtected() and ( - f instanceof MoveConstructor and constructorType = "Move constructor" + f instanceof MoveConstructor and + if f.isCompilerGenerated() + then constructorType = "Implicit move constructor" and missingAction = "deleted" + else ( + constructorType = "Move constructor" and missingAction = "protected" + ) or - f instanceof CopyConstructor and constructorType = "Copy constructor" + f instanceof CopyConstructor and + if f.isCompilerGenerated() + then constructorType = "Implicit copy constructor" and missingAction = "deleted" + else ( + constructorType = "Copy constructor" and missingAction = "protected" + ) ) } -predicate isInvalidAssignment(Operator f, string operatorType) { +predicate isInvalidAssignment(Operator f, string operatorType, string missingAction) { not f.isDeleted() and ( - f instanceof CopyAssignmentOperator and operatorType = "Copy assignment operator" + f instanceof MoveAssignmentOperator and + if f.isCompilerGenerated() + then operatorType = "Implicit move assignment operator" and missingAction = "deleted" + else ( + operatorType = "Move assignment operator" and missingAction = "protected" + ) or - f instanceof MoveAssignmentOperator and operatorType = "Move assignment operator" + f instanceof CopyAssignmentOperator and + if f.isCompilerGenerated() + then operatorType = "Implicit copy assignment operator" and missingAction = "deleted" + else ( + operatorType = "Copy assignment operator" and missingAction = "protected" + ) ) and not f.hasSpecifier("protected") } -from MemberFunction mf, string type, string baseReason +from BaseClass baseClass, MemberFunction mf, string type, string missingAction where not isExcluded(mf, OperatorsPackage::copyAndMoveNotDeclaredProtectedQuery()) and ( - isInvalidConstructor(mf, type) + isInvalidConstructor(mf, type, missingAction) or - isInvalidAssignment(mf, type) + isInvalidAssignment(mf, type, missingAction) ) and - isPossibleBaseClass(mf.getDeclaringType(), baseReason) + baseClass = mf.getDeclaringType() +// To avoid duplicate alerts due to inaccurate location information in the database we don't use the location of the base class. +// This for example happens if multiple copies of the same header file are present in the database. select getDeclarationEntryInClassDeclaration(mf), - type + " for base class " + mf.getDeclaringType().getQualifiedName() + " (" + baseReason + - ") is not declared protected or deleted." + type + " for base class '" + baseClass.getQualifiedName() + "' is not declared " + missingAction + + "." diff --git a/cpp/autosar/test/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.expected b/cpp/autosar/test/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.expected index f8900da92f..dbe7f89585 100644 --- a/cpp/autosar/test/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.expected +++ b/cpp/autosar/test/rules/A12-4-1/DestructorOfABaseClassNotPublicVirtual.expected @@ -1,2 +1,2 @@ -| test.cpp:4:3:4:4 | definition of ~A | Destructor of base class A is not declared as public virtual, public override, or protected non-virtual. | -| test.cpp:30:3:30:4 | definition of ~E | Destructor of base class E is not declared as public virtual, public override, or protected non-virtual. | +| test.cpp:4:3:4:4 | definition of ~A | Destructor of base class 'A' is not declared as public virtual, public override, or protected non-virtual. | +| test.cpp:30:3:30:4 | definition of ~E | Destructor of base class 'E' is not declared as public virtual, public override, or protected non-virtual. | diff --git a/cpp/autosar/test/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.expected b/cpp/autosar/test/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.expected index 39b98e0500..9f85da12d6 100644 --- a/cpp/autosar/test/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.expected +++ b/cpp/autosar/test/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.expected @@ -1,14 +1,26 @@ -| test.cpp:4:3:4:12 | declaration of BaseClass1 | Copy constructor for base class BaseClass1 (a derived class exists) is not declared protected or deleted. | -| test.cpp:5:3:5:12 | declaration of BaseClass1 | Move constructor for base class BaseClass1 (a derived class exists) is not declared protected or deleted. | -| test.cpp:6:15:6:23 | declaration of operator= | Copy assignment operator for base class BaseClass1 (a derived class exists) is not declared protected or deleted. | -| test.cpp:7:15:7:23 | declaration of operator= | Move assignment operator for base class BaseClass1 (a derived class exists) is not declared protected or deleted. | -| test.cpp:15:7:15:7 | declaration of operator= | Copy assignment operator for base class BaseClass2 (a derived class exists) is not declared protected or deleted. | -| test.cpp:15:7:15:7 | declaration of operator= | Move assignment operator for base class BaseClass2 (a derived class exists) is not declared protected or deleted. | -| test.cpp:55:3:55:12 | declaration of BaseClass5 | Copy constructor for base class BaseClass5 (a derived class exists) is not declared protected or deleted. | -| test.cpp:56:3:56:12 | declaration of BaseClass5 | Move constructor for base class BaseClass5 (a derived class exists) is not declared protected or deleted. | -| test.cpp:57:15:57:23 | declaration of operator= | Copy assignment operator for base class BaseClass5 (a derived class exists) is not declared protected or deleted. | -| test.cpp:58:15:58:23 | declaration of operator= | Move assignment operator for base class BaseClass5 (a derived class exists) is not declared protected or deleted. | -| test.cpp:75:3:75:12 | declaration of BaseClass6 | Copy constructor for base class BaseClass6 (the class is abstract) is not declared protected or deleted. | -| test.cpp:76:3:76:12 | declaration of BaseClass6 | Move constructor for base class BaseClass6 (the class is abstract) is not declared protected or deleted. | -| test.cpp:77:15:77:23 | declaration of operator= | Copy assignment operator for base class BaseClass6 (the class is abstract) is not declared protected or deleted. | -| test.cpp:78:15:78:23 | declaration of operator= | Move assignment operator for base class BaseClass6 (the class is abstract) is not declared protected or deleted. | +| test.cpp:4:3:4:12 | declaration of BaseClass1 | Copy constructor for base class 'BaseClass1' is not declared protected. | +| test.cpp:5:3:5:12 | declaration of BaseClass1 | Move constructor for base class 'BaseClass1' is not declared protected. | +| test.cpp:6:15:6:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass1' is not declared protected. | +| test.cpp:7:15:7:23 | declaration of operator= | Move assignment operator for base class 'BaseClass1' is not declared protected. | +| test.cpp:15:7:15:7 | declaration of operator= | Implicit copy assignment operator for base class 'BaseClass2' is not declared deleted. | +| test.cpp:15:7:15:7 | declaration of operator= | Implicit move assignment operator for base class 'BaseClass2' is not declared deleted. | +| test.cpp:55:3:55:12 | declaration of BaseClass5 | Copy constructor for base class 'BaseClass5' is not declared protected. | +| test.cpp:56:3:56:12 | declaration of BaseClass5 | Move constructor for base class 'BaseClass5' is not declared protected. | +| test.cpp:57:15:57:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass5' is not declared protected. | +| test.cpp:58:15:58:23 | declaration of operator= | Move assignment operator for base class 'BaseClass5' is not declared protected. | +| test.cpp:75:3:75:12 | declaration of BaseClass6 | Copy constructor for base class 'BaseClass6' is not declared protected. | +| test.cpp:76:3:76:12 | declaration of BaseClass6 | Move constructor for base class 'BaseClass6' is not declared protected. | +| test.cpp:77:15:77:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass6' is not declared protected. | +| test.cpp:78:15:78:23 | declaration of operator= | Move assignment operator for base class 'BaseClass6' is not declared protected. | +| test.cpp:85:3:85:12 | declaration of BaseClass7 | Copy constructor for base class 'BaseClass7' is not declared protected. | +| test.cpp:86:3:86:12 | declaration of BaseClass7 | Move constructor for base class 'BaseClass7' is not declared protected. | +| test.cpp:87:15:87:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass7' is not declared protected. | +| test.cpp:88:15:88:23 | declaration of operator= | Move assignment operator for base class 'BaseClass7' is not declared protected. | +| test.cpp:108:3:108:12 | declaration of BaseClass8 | Copy constructor for base class 'BaseClass8' is not declared protected. | +| test.cpp:109:3:109:12 | declaration of BaseClass8 | Move constructor for base class 'BaseClass8' is not declared protected. | +| test.cpp:110:15:110:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass8' is not declared protected. | +| test.cpp:111:15:111:23 | declaration of operator= | Move assignment operator for base class 'BaseClass8' is not declared protected. | +| test.cpp:124:26:124:26 | declaration of BaseClass9 | Implicit copy constructor for base class 'BaseClass9' is not declared deleted. | +| test.cpp:124:26:124:26 | declaration of BaseClass9 | Implicit move constructor for base class 'BaseClass9' is not declared deleted. | +| test.cpp:124:26:124:26 | declaration of operator= | Implicit copy assignment operator for base class 'BaseClass9' is not declared deleted. | +| test.cpp:124:26:124:26 | declaration of operator= | Implicit move assignment operator for base class 'BaseClass9' is not declared deleted. | diff --git a/cpp/autosar/test/rules/A12-8-6/test.cpp b/cpp/autosar/test/rules/A12-8-6/test.cpp index 2dc5425706..6a31ca60ae 100644 --- a/cpp/autosar/test/rules/A12-8-6/test.cpp +++ b/cpp/autosar/test/rules/A12-8-6/test.cpp @@ -77,4 +77,67 @@ class BaseClass6 { BaseClass6 &operator=(BaseClass6 const &) = default; // NON_COMPLIANT BaseClass6 &operator=(BaseClass6 &&) = default; // NON_COMPLIANT virtual void test() = 0; // pure virtual function, making this abstract -}; \ No newline at end of file +}; + +template class BaseClass7 { +public: + BaseClass7() {} + BaseClass7(BaseClass7 const &) = default; // NON_COMPLIANT + BaseClass7(BaseClass7 &&) = default; // NON_COMPLIANT + BaseClass7 &operator=(BaseClass7 const &) = default; // NON_COMPLIANT + BaseClass7 &operator=(BaseClass7 &&) = default; // NON_COMPLIANT + int operator=(int i); // COMPLIANT - not an assignment operator +}; // COMPLIANT + +template +class DerivedClass7 // COMPLIANT - not a base class itself + : public BaseClass7 { +public: + DerivedClass7() {} +}; + +class DerivedClass8 // COMPLIANT - not a base class itself + : public BaseClass7 { +public: + DerivedClass8() {} +}; + +class BaseClass8 { +public: + BaseClass8() {} + BaseClass8(BaseClass8 const &) = default; // NON_COMPLIANT + BaseClass8(BaseClass8 &&) = default; // NON_COMPLIANT + BaseClass8 &operator=(BaseClass8 const &) = default; // NON_COMPLIANT + BaseClass8 &operator=(BaseClass8 &&) = default; // NON_COMPLIANT +}; + +template +class DerivedClass9 // COMPLIANT - not a base class itself + : public BaseClass8 { +public: + DerivedClass9() {} + +private: + T t; +}; + +template class BaseClass9 { // NON_COMPLIANT + +public: + BaseClass9() {} +}; + +template +class DerivedClass10 // COMPLIANT - not a base class itself + : public BaseClass9 { +public: + DerivedClass10() {} +}; + +void test() { + BaseClass7 b; + DerivedClass7 d; + DerivedClass9 e; + BaseClass9 f; + DerivedClass10 g; +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Class.qll b/cpp/common/src/codingstandards/cpp/Class.qll index 2fff01c91f..19bec9fa5f 100644 --- a/cpp/common/src/codingstandards/cpp/Class.qll +++ b/cpp/common/src/codingstandards/cpp/Class.qll @@ -5,15 +5,26 @@ import cpp import codingstandards.cpp.Expr +private Class getADerivedClass(Class c) { + result = c.getADerivedClass() + or + exists(ClassTemplateInstantiation instantiation | + instantiation.getADerivedClass() = result and c = instantiation.getTemplate() + ) +} + /** - * Holds if we believe that `c` is used or intended to be used as a base class. + * A class that is used or intended to be used as a base class. */ -predicate isPossibleBaseClass(Class c, string reason) { - // There exists a derivation in this database - exists(c.getADerivedClass()) and reason = "a derived class exists" - or - // The class must be extended at some point - c.isAbstract() and reason = "the class is abstract" +class BaseClass extends Class { + BaseClass() { + exists(getADerivedClass(this)) + or + this.isAbstract() + } + + // We don't override `getADerivedClass` because that introduces a non-monotonic recursion. + Class getASubClass() { result = getADerivedClass(this) } } /**