C++: another shot at implicit/deleted copy

Fixes some cases in instantiations of templates with manually written
copy constructors or copy assignment operators where
hasImplicitCopyConstructor would incorrectly hold

cpp/ql/lib/change-notes/2022-03-03-template-implicit-copy.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `Class::hasImplicitCopyConstructor` and `Class::hasImplicitCopyAssignmentOperator` methods now handle template instantiations more accurately. This should improve results for the `cpp/rule-of-two` query.

cpp/ql/lib/semmle/code/cpp/Class.qll

@@ -285,7 +285,17 @@ class Class extends UserType {
   predicate hasImplicitCopyConstructor() {
     not this.implicitCopyConstructorDeleted() and
     forall(CopyConstructor cc | cc = this.getAMemberFunction() |
-      cc.isCompilerGenerated() and not cc.isDeleted()
+      cc.isCompilerGenerated() and not cc.isDeleted() and not cc.isDefaulted()
+    ) and
+    (
+      not this instanceof ClassTemplateInstantiation
+      or
+      this.(ClassTemplateInstantiation).getTemplate().hasImplicitCopyConstructor()
+    ) and
+    (
+      not this instanceof PartialClassTemplateSpecialization
+      or
+      this.(PartialClassTemplateSpecialization).getPrimaryTemplate().hasImplicitCopyConstructor()
     )
   }
 
@@ -300,7 +310,19 @@ class Class extends UserType {
   predicate hasImplicitCopyAssignmentOperator() {
     not this.implicitCopyAssignmentOperatorDeleted() and
     forall(CopyAssignmentOperator ca | ca = this.getAMemberFunction() |
-      ca.isCompilerGenerated() and not ca.isDeleted()
+      ca.isCompilerGenerated() and not ca.isDeleted() and not ca.isDefaulted()
+    ) and
+    (
+      not this instanceof ClassTemplateInstantiation
+      or
+      this.(ClassTemplateInstantiation).getTemplate().hasImplicitCopyAssignmentOperator()
+    ) and
+    (
+      not this instanceof PartialClassTemplateSpecialization
+      or
+      this.(PartialClassTemplateSpecialization)
+          .getPrimaryTemplate()
+          .hasImplicitCopyAssignmentOperator()
     )
   }
 
@@ -311,6 +333,12 @@ class Class extends UserType {
    * http://en.cppreference.com/w/cpp/language/copy_constructor#Deleted_implicitly-declared_copy_constructor
    */
   predicate implicitCopyConstructorDeleted() {
+    forex(CopyConstructor cc | cc = this.getAConstructor() |
+      cc.isDeleted()
+      or
+      not cc.isCompilerGenerated()
+    )
+    or
     // - T has non-static data members that cannot be copied (have deleted,
     //   inaccessible, or ambiguous copy constructors);
     exists(Type t | t = this.getAFieldSubobjectType().getUnspecifiedType() |
@@ -318,34 +346,6 @@ class Class extends UserType {
       // constructors are considered equal.
       this.cannotAccessCopyConstructorOnAny(t)
     )
-    or
-    // - T has direct or virtual base class that cannot be copied (has deleted,
-    //   inaccessible, or ambiguous copy constructors);
-    exists(Class c | c = this.getADirectOrVirtualBase() |
-      // Note: Overload resolution is not implemented -- all copy
-      // constructors are considered equal.
-      this.cannotAccessCopyConstructorOnThis(c)
-    )
-    or
-    // - T has direct or virtual base class with a deleted or inaccessible
-    //   destructor;
-    exists(Class base | base = this.getADirectOrVirtualBase() |
-      this.cannotAccessDestructor(base, this)
-    )
-    or
-    // - T has a user-defined move constructor or move assignment operator;
-    exists(MoveConstructor mc | mc = this.getAMemberFunction() | not mc.isCompilerGenerated())
-    or
-    exists(MoveAssignmentOperator ma | ma = this.getAMemberFunction() |
-      not ma.isCompilerGenerated()
-    )
-    or
-    // - T is a union and has a variant member with non-trivial copy
-    //   constructor (since C++11)
-    none() // Not implemented
-    or
-    // - T has a data member of rvalue reference type.
-    exists(Type t | t = this.getAFieldSubobjectType() | t instanceof RValueReferenceType)
   }
 
   /**
@@ -355,34 +355,12 @@ class Class extends UserType {
    * http://en.cppreference.com/w/cpp/language/copy_assignment#Deleted_implicitly-declared_copy_assignment_operator
    */
   predicate implicitCopyAssignmentOperatorDeleted() {
-    // - T has a user-declared move constructor;
-    exists(MoveConstructor mc | mc = this.getAMemberFunction() | not mc.isCompilerGenerated())
-    or
-    // - T has a user-declared move assignment operator.
-    exists(MoveAssignmentOperator ma | ma = this.getAMemberFunction() |
-      not ma.isCompilerGenerated()
+    forex(CopyAssignmentOperator ca | ca = this.getAMemberFunction() |
+      ca.isDeleted()
+      or
+      not ca.isCompilerGenerated()
     )
     or
-    // - T has a non-static data member of non-class type (or array thereof)
-    //   that is const;
-    exists(Type t | t = this.getAFieldSubobjectType() |
-      // The rule for this case refers only to non-class types only, but our
-      // implementation extends it to cover class types too. Class types are
-      // supposed to be covered by the rule below on data members that
-      // cannot be copy-assigned. Copy-assigning a const class-typed member
-      // would call an overload of type
-      // `const C& operator=(const C&) const;`. Such an overload is unlikely
-      // to exist because it contradicts the intention of "const": it allows
-      // assigning to a const object. But since we have not implemented the
-      // ability to distinguish between overloads, we cannot distinguish that
-      // overload from the ordinary `C& operator=(const C&);`. Instead, we
-      // require class types to be non-const in this clause.
-      /* not t instanceof Class and */ t.isConst()
-    )
-    or
-    // - T has a non-static data member of a reference type;
-    exists(Type t | t = this.getAFieldSubobjectType() | t instanceof ReferenceType)
-    or
     // - T has a non-static data member or a direct or virtual base class that
     //   cannot be copy-assigned (overload resolution for the copy assignment
     //   fails, or selects a deleted or inaccessible function);
@@ -391,15 +369,6 @@ class Class extends UserType {
       // operators are considered equal.
       this.cannotAccessCopyAssignmentOperatorOnAny(t)
     )
-    or
-    exists(Class c | c = this.getADirectOrVirtualBase() |
-      // Note: Overload resolution is not implemented -- all copy assignment
-      // operators are considered equal.
-      this.cannotAccessCopyAssignmentOperatorOnThis(c)
-    )
-    // - T is a union-like class, and has a variant member whose corresponding
-    //   assignment operator is non-trivial.
-    // Not implemented
   }
 
   /** Gets the destructor of this class, struct or union, if any. */

cpp/ql/test/library-tests/special_members/generated_copy/assign.expected

@@ -11,6 +11,7 @@
 | difference::Base | can | does NOT | have implicit copy assignment |
 | difference::OnlyAssign | can | does | have implicit copy assignment |
 | difference::OnlyCtor | can NOT | does NOT | have implicit copy assignment |
+| instantiated_explicit_ctor::Wrapper<int> | can | does | have implicit copy assignment |
 | moves::MoveAssign | can NOT | does NOT | have implicit copy assignment |
 | moves::MoveCtor | can NOT | does NOT | have implicit copy assignment |
 | private_cc::C | can | does NOT | have implicit copy assignment |

cpp/ql/test/library-tests/special_members/generated_copy/copy.cpp

@@ -131,3 +131,21 @@ namespace difference {
   class OnlyAssign : Base {
   };
 }
+
+namespace instantiated_explicit_ctor {
+  template<class T>
+  class Wrapper {
+  public:
+    Wrapper(Wrapper<T> &other) {
+      m_t = other.m_t;
+    }
+    
+    Wrapper() {
+      m_t = 0;
+    }
+  private:
+    T m_t;
+  };
+
+  Wrapper<int> wrapped_int;
+}

cpp/ql/test/library-tests/special_members/generated_copy/ctor.expected

@@ -11,6 +11,7 @@
 | difference::Base | can | does NOT | have implicit copy constructor |
 | difference::OnlyAssign | can NOT | does NOT | have implicit copy constructor |
 | difference::OnlyCtor | can | does | have implicit copy constructor |
+| instantiated_explicit_ctor::Wrapper<int> | can | does NOT | have implicit copy constructor |
 | moves::MoveAssign | can NOT | does NOT | have implicit copy constructor |
 | moves::MoveCtor | can NOT | does NOT | have implicit copy constructor |
 | private_cc::C | can | does NOT | have implicit copy constructor |

cpp/ql/test/library-tests/special_members/generated_copy/functions.expected

@@ -86,5 +86,9 @@
 | copy.cpp:131:9:131:9 | OnlyAssign | deleted |  |
 | copy.cpp:131:9:131:9 | operator= |  |  |
 | copy.cpp:131:9:131:9 | operator= |  |  |
+| copy.cpp:137:9:137:9 | operator= |  |  |
+| copy.cpp:139:5:139:11 | Wrapper |  |  |
+| copy.cpp:143:5:143:5 | Wrapper |  |  |
+| copy.cpp:143:5:143:11 | Wrapper |  |  |
 | file://:0:0:0:0 | operator= |  |  |
 | file://:0:0:0:0 | operator= |  |  |

cpp/ql/test/query-tests/Best Practices/RuleOfTwo/RuleOfTwo.cpp

@@ -117,12 +117,8 @@ struct HasVPV {
   }
 };
 
-// FALSE NEGATIVE: the relevant copy constructor of ProtectedVolatile is
-// accessible, so our class will get a generated copy constructor. Our query
-// thinks the copy constructor is inaccessible because it picks up the other
-// copy constructor. To fix this, our library should be changed to distinguish
-// between copy constructors and resolve overloading properly instead of
-// assuming that there is at most one.
+// NOT OK: the relevant copy constructor of ProtectedVolatile is
+// accessible, so our class will get a generated copy constructor.
 struct HasPV {
   ProtectedVolatile pv;
   HasPV& operator=(const HasPV& that) {

cpp/ql/test/query-tests/Best Practices/RuleOfTwo/RuleOfTwo.expected

@@ -1,6 +1,6 @@
 | RuleOfTwo.cpp:4:3:4:17 | CopyButNoAssign | No matching copy assignment operator in class CopyButNoAssign. It is good practice to match a copy constructor with a copy assignment operator. |
 | RuleOfTwo.cpp:10:20:10:28 | operator= | No matching copy constructor in class AssignButNoCopy. It is good practice to match a copy assignment operator with a copy constructor. |
 | RuleOfTwo.cpp:81:18:81:26 | operator= | No matching copy constructor in class MyClassFriend. It is good practice to match a copy assignment operator with a copy constructor. |
-| RuleOfTwo.cpp:144:3:144:20 | IsAProtectedAssign | No matching copy assignment operator in class IsAProtectedAssign. It is good practice to match a copy constructor with a copy assignment operator. |
-| RuleOfTwo.cpp:167:19:167:27 | operator= | No matching copy constructor in class IsAProtectedCC. It is good practice to match a copy assignment operator with a copy constructor. |
-| RuleOfTwo.cpp:312:5:312:8 | R1_C | No matching copy assignment operator in class R1_C. It is good practice to match a copy constructor with a copy assignment operator. |
+| RuleOfTwo.cpp:140:3:140:20 | IsAProtectedAssign | No matching copy assignment operator in class IsAProtectedAssign. It is good practice to match a copy constructor with a copy assignment operator. |
+| RuleOfTwo.cpp:163:19:163:27 | operator= | No matching copy constructor in class IsAProtectedCC. It is good practice to match a copy assignment operator with a copy constructor. |
+| RuleOfTwo.cpp:308:5:308:8 | R1_C | No matching copy assignment operator in class R1_C. It is good practice to match a copy constructor with a copy assignment operator. |