C++: recognize assignment helpers returning reference parameters

cpp/ql/src/change-notes/2026-11-12-assignment-helper-return.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/assignment-does-not-return-this` query now recognizes assignment operators that return `*this` through non-virtual helper functions whose reachable returns directly return the same reference parameter, reducing false positives.

cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql

@@ -17,54 +17,114 @@ import cpp
  * Applies to all assignment operators, not just the copy assignment operator.
  */
 
-predicate callOnThis(FunctionCall fc) {
-  // `this->f(...)`
-  fc.getQualifier() instanceof ThisExpr
-  or
-  // `(*this).f(...)`
-  fc.getQualifier().(PointerDereferenceExpr).getChild(0) instanceof ThisExpr
+/** Holds if `e` has an explicit cast anywhere in its conversion chain. */
+private predicate hasExplicitCast(Expr e) {
+  exists(Cast c | c = e.getUnconverted().getConversion*() and not c.isImplicit())
 }
 
-predicate pointerThis(Expr e) {
-  e instanceof ThisExpr
-  or
-  // `f(...)`
-  // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`)
-  exists(FunctionCall fc | fc = e and callOnThis(fc) | returnsPointerThis(fc.getTarget()))
-  or
-  // `this = ...` (where `=` is not overloaded, so an `AssignExpr`)
-  pointerThis(e.(AssignExpr).getLValue())
+predicate callOnThis(FunctionCall fc, boolean allowCasts) {
+  allowCasts = [true, false] and
+  (allowCasts = true or not hasExplicitCast(fc.getQualifier())) and
+  (
+    // `this->f(...)`
+    fc.getQualifier() instanceof ThisExpr
+    or
+    // `(*this).f(...)`
+    fc.getQualifier().(PointerDereferenceExpr).getChild(0) instanceof ThisExpr and
+    (
+      allowCasts = true or
+      not hasExplicitCast(fc.getQualifier().(PointerDereferenceExpr).getChild(0))
+    )
+  )
+}
+
+predicate pointerThis(Expr e, boolean allowCasts) {
+  allowCasts = [true, false] and
+  (allowCasts = true or not hasExplicitCast(e)) and
+  (
+    e instanceof ThisExpr
+    or
+    // `f(...)`
+    // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`)
+    exists(FunctionCall fc | fc = e and callOnThis(fc, allowCasts) |
+      returnsPointerThis(fc.getTarget(), allowCasts)
+    )
+    or
+    // `this = ...` (where `=` is not overloaded, so an `AssignExpr`)
+    pointerThis(e.(AssignExpr).getLValue(), allowCasts)
+  )
 }
 
-predicate dereferenceThis(Expr e) {
-  pointerThis(e.(PointerDereferenceExpr).getChild(0))
-  or
-  // `f(...)`
-  // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`)
-  exists(FunctionCall fc | fc = e and callOnThis(fc) | returnsDereferenceThis(fc.getTarget()))
-  or
-  // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`)
-  dereferenceThis(e.(AssignExpr).getLValue())
-  or
-  // `e ? ... : ... `
-  exists(ConditionalExpr cond |
-    cond = e and
-    dereferenceThis(cond.getThen()) and
-    dereferenceThis(cond.getElse())
+/**
+ * Holds if the function containing `p` returns a reference to `p` on every
+ * reachable return. Require a visible, non-virtual definition and matching
+ * reference base types to exclude copies and user-defined conversions.
+ */
+private predicate returnsReferenceParameter(Parameter p) {
+  exists(Function f |
+    f = p.getFunction() and
+    exists(f.getBlock()) and
+    not f.isVirtual() and
+    f.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() =
+      p.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() and
+    exists(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s)) and
+    forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
+      s.getExpr().getUnconverted().(VariableAccess).getTarget() = p and
+      not hasExplicitCast(s.getExpr())
+    )
+  )
+}
+
+// Preserve the existing treatment of casts outside helper arguments. Within a
+// helper argument, check conversions at every step that supplies the result.
+predicate dereferenceThis(Expr e, boolean allowCasts) {
+  allowCasts = [true, false] and
+  (allowCasts = true or not hasExplicitCast(e)) and
+  (
+    pointerThis(e.(PointerDereferenceExpr).getChild(0), allowCasts)
+    or
+    // `f(...)`
+    // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`)
+    exists(FunctionCall fc | fc = e and callOnThis(fc, allowCasts) |
+      returnsDereferenceThis(fc.getTarget(), allowCasts)
+    )
+    or
+    // `helper(*this, ...)`, where every return directly returns the corresponding parameter.
+    exists(FunctionCall fc, Parameter p, Expr arg |
+      fc = e and
+      not hasExplicitCast(fc) and
+      fc.getTarget() = p.getFunction() and
+      returnsReferenceParameter(p) and
+      arg = fc.getArgument(p.getIndex()) and
+      arg.getUnconverted().getType().getUnspecifiedType() =
+        p.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() and
+      dereferenceThis(arg, false)
+    )
+    or
+    // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`)
+    dereferenceThis(e.(AssignExpr).getLValue(), allowCasts)
+    or
+    // `e ? ... : ... `
+    exists(ConditionalExpr cond |
+      cond = e and
+      dereferenceThis(cond.getThen(), allowCasts) and
+      dereferenceThis(cond.getElse(), allowCasts)
+    )
+    or
+    // `..., ... `
+    dereferenceThis(e.(CommaExpr).getRightOperand(), allowCasts)
   )
-  or
-  // `..., ... `
-  dereferenceThis(e.(CommaExpr).getRightOperand())
 }
 
 /**
  * Holds if all `return` statements in `f` return `this`, possibly indirectly.
  * This includes functions whose body is not in the database.
  */
-predicate returnsPointerThis(Function f) {
+predicate returnsPointerThis(Function f, boolean allowCasts) {
+  allowCasts = [true, false] and
   forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
     // `return this`
-    pointerThis(s.getExpr())
+    pointerThis(s.getExpr(), allowCasts)
   )
 }
 
@@ -73,10 +133,11 @@ predicate returnsPointerThis(Function f) {
  * possibly indirectly. This includes functions whose body is not in the
  * database.
  */
-predicate returnsDereferenceThis(Function f) {
+predicate returnsDereferenceThis(Function f, boolean allowCasts) {
+  allowCasts = [true, false] and
   forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
     // `return *this`
-    dereferenceThis(s.getExpr())
+    dereferenceThis(s.getExpr(), allowCasts)
   )
 }
 
@@ -94,7 +155,7 @@ predicate assignOperatorWithWrongType(Operator op, string msg) {
 
 predicate assignOperatorWithWrongResult(Operator op, string msg) {
   op.hasName("operator=") and
-  not returnsDereferenceThis(op) and
+  not returnsDereferenceThis(op, true) and
   exists(op.getBlock()) and
   not op.getType() instanceof VoidType and
   not assignOperatorWithWrongType(op, _) and

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected

@@ -2,3 +2,20 @@
 | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. |
 | AV Rule 82.cpp:63:29:63:29 | operator= | Assignment operator in class TemplateReturnAssignment<int> does not return a reference to *this. |
 | AV Rule 82.cpp:199:52:199:52 | operator= | Assignment operator in class TemplatedAssignmentBad should have return type TemplatedAssignmentBad&. Otherwise a copy is created at each call. |
+| casts.cpp:21:24:21:32 | operator= | Assignment operator in class BadHelperReturnCast does not return a reference to *this. |
+| casts.cpp:27:30:27:38 | operator= | Assignment operator in class BadHelperReturnCStyleCast does not return a reference to *this. |
+| casts.cpp:33:26:33:34 | operator= | Assignment operator in class BadHelperArgumentCast does not return a reference to *this. |
+| casts.cpp:40:25:40:33 | operator= | Assignment operator in class BadHelperPointerCast does not return a reference to *this. |
+| casts.cpp:47:29:47:37 | operator= | Assignment operator in class BadHelperConditionalCast does not return a reference to *this. |
+| casts.cpp:54:23:54:31 | operator= | Assignment operator in class BadHelperCommaCast does not return a reference to *this. |
+| casts.cpp:65:30:65:38 | operator= | Assignment operator in class BadHelperMemberReturnCast does not return a reference to *this. |
+| casts.cpp:72:27:72:35 | operator= | Assignment operator in class BadHelperQualifierCast does not return a reference to *this. |
+| casts.cpp:102:24:102:32 | operator= | Assignment operator in class BadHelperResultCast does not return a reference to *this. |
+| helpers.cpp:92:13:92:21 | operator= | Assignment operator in class BadOther does not return a reference to *this. |
+| helpers.cpp:103:16:103:24 | operator= | Assignment operator in class BadBranches does not return a reference to *this. |
+| helpers.cpp:113:19:113:27 | operator= | Assignment operator in class BadConditional does not return a reference to *this. |
+| helpers.cpp:121:22:121:30 | operator= | Assignment operator in class BadValueParameter does not return a reference to *this. |
+| helpers.cpp:129:18:129:26 | operator= | Assignment operator in class BadValueResult should have return type BadValueResult&. Otherwise a copy is created at each call. |
+| helpers.cpp:137:19:137:27 | operator= | Assignment operator in class BadMissingBody does not return a reference to *this. |
+| helpers.cpp:147:15:147:23 | operator= | Assignment operator in class BadVirtual does not return a reference to *this. |
+| helpers.cpp:164:18:164:26 | operator= | Assignment operator in class BadConversion does not return a reference to *this. |

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/casts.cpp

@@ -0,0 +1,106 @@
+// Explicit conversions can select a different subobject of the same base type.
+template<class Base> struct LeftBase : Base {};
+template<class Base> struct RightBase : Base {};
+template<class Base> struct TwoBases : LeftBase<Base>, RightBase<Base> {};
+
+template<class T>
+T& identity(T& destination) { return (destination); }
+
+template<class T>
+T& otherBase(T& destination) {
+  return static_cast<T&>(static_cast<RightBase<T>&>(
+      static_cast<TwoBases<T>&>(static_cast<LeftBase<T>&>(destination))));
+}
+
+template<class T>
+T& otherBaseCStyle(T& destination) {
+  return (T&)(RightBase<T>&)(TwoBases<T>&)(LeftBase<T>&)destination;
+}
+
+struct BadHelperReturnCast {
+  BadHelperReturnCast& operator=(int) { // $ Alert
+    return otherBase(*this);
+  }
+};
+
+struct BadHelperReturnCStyleCast {
+  BadHelperReturnCStyleCast& operator=(int) { // $ Alert
+    return otherBaseCStyle(*this);
+  }
+};
+
+struct BadHelperArgumentCast {
+  BadHelperArgumentCast& operator=(int) { // $ Alert
+    return identity(static_cast<BadHelperArgumentCast&>(static_cast<RightBase<BadHelperArgumentCast>&>(
+        static_cast<TwoBases<BadHelperArgumentCast>&>(static_cast<LeftBase<BadHelperArgumentCast>&>(*this)))));
+  }
+};
+
+struct BadHelperPointerCast {
+  BadHelperPointerCast& operator=(int) { // $ Alert
+    return identity(*static_cast<BadHelperPointerCast*>(static_cast<RightBase<BadHelperPointerCast>*>(
+        static_cast<TwoBases<BadHelperPointerCast>*>(static_cast<LeftBase<BadHelperPointerCast>*>(this)))));
+  }
+};
+
+struct BadHelperConditionalCast {
+  BadHelperConditionalCast& operator=(bool flag) { // $ Alert
+    return identity(flag ? *this : static_cast<BadHelperConditionalCast&>(static_cast<RightBase<BadHelperConditionalCast>&>(
+        static_cast<TwoBases<BadHelperConditionalCast>&>(static_cast<LeftBase<BadHelperConditionalCast>&>(*this)))));
+  }
+};
+
+struct BadHelperCommaCast {
+  BadHelperCommaCast& operator=(int) { // $ Alert
+    return identity((0, static_cast<BadHelperCommaCast&>(static_cast<RightBase<BadHelperCommaCast>&>(
+        static_cast<TwoBases<BadHelperCommaCast>&>(static_cast<LeftBase<BadHelperCommaCast>&>(*this))))));
+  }
+};
+
+struct BadHelperMemberReturnCast {
+  BadHelperMemberReturnCast* otherPointer() {
+    return static_cast<BadHelperMemberReturnCast*>(static_cast<RightBase<BadHelperMemberReturnCast>*>(
+        static_cast<TwoBases<BadHelperMemberReturnCast>*>(static_cast<LeftBase<BadHelperMemberReturnCast>*>(this))));
+  }
+  BadHelperMemberReturnCast& operator=(int) { // $ Alert
+    return identity(*this->otherPointer());
+  }
+};
+
+struct BadHelperQualifierCast {
+  BadHelperQualifierCast* selfPointer() { return this; }
+  BadHelperQualifierCast& operator=(int) { // $ Alert
+    return identity(*static_cast<BadHelperQualifierCast*>(static_cast<RightBase<BadHelperQualifierCast>*>(
+        static_cast<TwoBases<BadHelperQualifierCast>*>(static_cast<LeftBase<BadHelperQualifierCast>*>(this))))->selfPointer());
+  }
+};
+
+struct GoodHelperArgumentPaths {
+  GoodHelperArgumentPaths* selfPointer() { return this; }
+
+  GoodHelperArgumentPaths& operator=(int) { // GOOD: parentheses are not casts.
+    return identity(((*this)));
+  }
+  GoodHelperArgumentPaths& operator=(bool flag) { // GOOD
+    return identity(flag ? *this : *this);
+  }
+  GoodHelperArgumentPaths& operator=(char) { // GOOD
+    return identity((0, *this));
+  }
+  GoodHelperArgumentPaths& operator=(long flag) { // GOOD: the condition does not supply the reference.
+    return identity(static_cast<bool>(flag) ? *this : *this);
+  }
+  GoodHelperArgumentPaths& operator=(unsigned int value) { // GOOD: neither does the left comma operand.
+    return identity((static_cast<void>(value), *this));
+  }
+  GoodHelperArgumentPaths& operator=(double) { // GOOD
+    return identity(*this->selfPointer());
+  }
+};
+
+struct BadHelperResultCast {
+  BadHelperResultCast& operator=(int) { // $ Alert
+    return static_cast<BadHelperResultCast&>(static_cast<RightBase<BadHelperResultCast>&>(
+        static_cast<TwoBases<BadHelperResultCast>&>(static_cast<LeftBase<BadHelperResultCast>&>(identity(*this)))));
+  }
+};