Fix 392

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.

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."

cpp/autosar/src/rules/A12-8-6/CopyAndMoveNotDeclaredProtected.ql

@@ -16,35 +16,49 @@ 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 +"."

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. |

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<T1>' is not declared protected. |
+| test.cpp:86:3:86:12 | declaration of BaseClass7 | Move constructor for base class 'BaseClass7<T1>' is not declared protected. |
+| test.cpp:87:15:87:23 | declaration of operator= | Copy assignment operator for base class 'BaseClass7<T1>' is not declared protected. |
+| test.cpp:88:15:88:23 | declaration of operator= | Move assignment operator for base class 'BaseClass7<T1>' 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<int>' is not declared deleted. |
+| test.cpp:124:26:124:26 | declaration of BaseClass9 | Implicit move constructor for base class 'BaseClass9<int>' is not declared deleted. |
+| test.cpp:124:26:124:26 | declaration of operator= | Implicit copy assignment operator for base class 'BaseClass9<int>' is not declared deleted. |
+| test.cpp:124:26:124:26 | declaration of operator= | Implicit move assignment operator for base class 'BaseClass9<int>' is not declared deleted. |

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
-};
+};
+
+template <class T1> 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 T>
+class DerivedClass7 // COMPLIANT - not a base class itself
+    : public BaseClass7<T> {
+public:
+  DerivedClass7() {}
+};
+
+class DerivedClass8 // COMPLIANT - not a base class itself
+    : public BaseClass7<int> {
+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 T>
+class DerivedClass9 // COMPLIANT - not a base class itself
+    : public BaseClass8 {
+public:
+  DerivedClass9() {}
+
+private:
+  T t;
+};
+
+template <class T> class BaseClass9 { // NON_COMPLIANT
+
+public:
+  BaseClass9() {}
+};
+
+template <class T>
+class DerivedClass10 // COMPLIANT - not a base class itself
+    : public BaseClass9<T> {
+public:
+  DerivedClass10() {}
+};
+
+void test() {
+  BaseClass7<int> b;
+  DerivedClass7<int> d;
+  DerivedClass9<int> e;
+  BaseClass9<int> f;
+  DerivedClass10<int> g;
+}

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) }
 }
 
 /**