Merge pull request #754 from jbj/copy-assignment-no-effect

C++: Exclude assignment operator in ExprHasNoEffect

Author: dave-bartolomeo

cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql

@@ -91,12 +91,27 @@ predicate definedInIfDef(Function f) {
   )
 }
 
+/**
+ * Holds if `call` has the form `B::f()` or `q.B::f()`, where `B` is a base
+ * class of the class containing `call`.
+ *
+ * This is most often used for calling base-class functions from within
+ * overrides. Those functions may have no side effect in the current
+ * implementation, but we should not advise callers to rely on this. That would
+ * break encapsulation.
+ */
+predicate baseCall(FunctionCall call) {
+  call.getNameQualifier().getQualifyingElement() =
+    call.getEnclosingFunction().getDeclaringType().(Class).getABaseClass+()
+}
+
 from PureExprInVoidContext peivc, Locatable parent,
   Locatable info, string info_text, string tail
 where // EQExprs are covered by CompareWhereAssignMeant.ql
       not peivc instanceof EQExpr and
       // as is operator==
       not peivc.(FunctionCall).getTarget().hasName("operator==") and
+      not baseCall(peivc) and
       not accessInInitOfForStmt(peivc) and
       not peivc.isCompilerGenerated() and
       not exists(Macro m | peivc = m.getAnInvocation().getAnExpandedElement()) and

cpp/ql/src/semmle/code/cpp/NameQualifiers.qll

@@ -67,7 +67,10 @@ class NameQualifier extends NameQualifiableElement, @namequalifier {
  * the latter is qualified by `N1`.
  */
 class NameQualifiableElement extends Element, @namequalifiableelement {
-  /** Gets the name qualifier associated with this element. */
+  /**
+   * Gets the name qualifier associated with this element. For example, the
+   * name qualifier of `N::f()` is `N`.
+   */
   NameQualifier getNameQualifier() {
     namequalifiers(unresolveElement(result),underlyingElement(this),_,_)
   }

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/test.cpp

@@ -64,3 +64,45 @@ void testFunc2()
 	MyAssignable v1, v2;
 	v2 = v1;
 }
+
+namespace std {
+  typedef unsigned long size_t;
+}
+
+void* operator new  (std::size_t size, void* ptr) noexcept;
+
+struct Base {
+  Base() { }
+
+  Base &operator=(const Base &rhs) {
+    return *this;
+  }
+
+  virtual ~Base() { }
+};
+
+struct Derived : Base {
+  Derived &operator=(const Derived &rhs) {
+    if (&rhs == this) {
+      return *this;
+    }
+
+    // In case base class has data, now or in the future, copy that first.
+    Base::operator=(rhs); // GOOD
+
+    this->m_x = rhs.m_x;
+    return *this;
+  }
+
+  int m_x;
+};
+
+void use_Base() {
+  Base base_buffer[1];
+  Base *base = new(&base_buffer[0]) Base();
+
+  // In case the destructor does something, now or in the future, call it. It
+  // won't get called automatically because the object was allocated with
+  // placement new.
+  base->~Base(); // GOOD (because the call has void type)
+}