[clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

[python3kgae]

Summary

Fixes 42469 https://github.com/llvm/llvm-project/issues/42469

Only check self assignment on BO_Assign when BuildOverloadedBinOp.

[rsmith]

clang/lib/Sema/SemaExpr.cpp

15669

This is the same thing, but for this->x += this->x. I think we should also suppress those warnings for compound assignment, except when one of the operands is an implicit member access and one is an explicit member access (this->x += x; should still warn if the latter x is also interpreted as this->x).

[python3kgae]

clang/lib/Sema/SemaExpr.cpp

15669

For case like this:

class Vector {
public:
    Vector& operator+=(const Vector &v) { return *this; }
    Vector& operator-=(const Vector &v) { return *this; }
};

class A {
public:
  A(){}
  Vector x;
  void foo() {
     this->x -= this->x;
     this->x -= x;
     this->x += this->x;
     this->x += x;
  }
};

clang will report 2 warning:

<source>:14:14: warning: assigning field to itself [-Wself-assign-field]
     this->x -= this->x;
             ^
<source>:15:14: warning: assigning field to itself [-Wself-assign-field]
     this->x -= x;

Do you mean we should make it report warning on this->x -= x; and this->x += x; ?

[rsmith]

clang/lib/Sema/SemaExpr.cpp

15669

Let's not add any new warnings. Pretend my comment was about -=, not +=.

[python3kgae]

skip warning on field for compound assignment when isImplicitCXXThis matches.

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

14143

Spurious whitespace change.

clang/test/SemaCXX/warn-self-assign-field-overloaded.cpp

71–91 ↗

(On Diff #510976)

The behavior of this diagnostic is almost inscrutable without really sitting down to think about it... and I'm not even certain I get the logic for it despite thinking about it heavily for a while now. I think my confusion boils down to this diagnostic trying to diagnose two different kinds of problems.

Situation 1 is where there's a possible typo and the user meant a different object:

a /= a;
this->a /= this->a;

// Less-contrived example
a00 /= a01;
a01 /= a02;
a02 /= a02; // Oops, typo!

(Note, I think this->a and a should be handled the same in this case because there are coding styles out there that mandate adding this-> to any member lookup, so there's no reason to think that this->a /= this->a; is any less of a typo than a /= a;.)

Situation 2 is where the user is trying to work around the user-unfriendly lookup rules of C++, or there is a typo, but got confused because lookup found the same object on both sides:

this->a /= a;
a /= this->a;

// Less-contrived example
int a;
struct S {
  int a;

  void foo() {
    // Divide the member variable by the global variable
    this->a /= a; // Oops, that's not how this works in practice
  }

  void bar(int ab) {
    // Divide the member variable by the parameter
    this->a /= a; // Oops, typo, meant to use 'ab' instead
  }
};

So I'm surprised that we'd want to silence the warnings on a /= a; as we're doing here. Frankly, I think we should be silent on a *= a; because that's an extremely common way to square a variable, but diagnose almost all the other cases with a "did you mean?" diagnostic. a -= a; and a %= a; are bad ways to write a = 0, a += a; is a bad way to write a *= 2; (but might be common enough we want to silence it as well), and a /= a; is a bad way to write a = 1; (The bitwise operators may still be reasonable to silence the warning on, but even a &= a; and a |= a; seems like weird no-ops...)

All that said, my understanding of the crux of #42469 is that we wanted to silence the warning whenever the operator being used is overloaded because we have no idea whether self-assign is intended in that case. So I would expect these cases to all consistently not diagnose because they're all using the overloaded operator.

[rjmccall]

Yeah, I feel there are three ideas here:

• Clang should warn about self simple assignment in all cases, because it's probably a mistake. We can assume it's a mistake because it's reasonable to assume that the simple assignment operator behaves like a value assignment, even if it's user-defined, and overwriting a value with itself is a pointless operation.
• Clang should warn about self compound assignment when the type is arithmetic for these specific operators where algebraically x OP x would yield a constant value, because it's probably a mistake. This is because we know the exact behavior of the operator, and producing a constant value by cancellation is a pointless operation.
• Clang should warn about inconsistent use of this on self compound assignment for all operators, because it's probably a mistake. This is because the implication of writing the member reference two different ways is that there are two different variables in play, but in fact there are not.

The third idea seems like a valuable warning, but it's basically a *different* warning and shouldn't be lumped into this one. I understand the instinct to carve it out here so that we don't regress on warning about it, but I think we should just do it separately. And we should do it much more generally, because there's no reason that logic is limited to just these specific compound operators, or in fact to any individual operator; we should probably warn about all inconsistent references to the same variable within a single statement. (I'd draw the line at statement boundaries, though, because requiring function-wide consistency could be really noisy.)

[dblaikie]

we should probably warn about all inconsistent references to the same variable within a single statement.

+1 here, that that seems like the suitable generalization/unrelated to the use in/restriction to assignments

[rsmith]

In D146897#4249300, @rjmccall wrote:

The third idea seems like a valuable warning, but it's basically a *different* warning and shouldn't be lumped into this one. I understand the instinct to carve it out here so that we don't regress on warning about it, but I think we should just do it separately. And we should do it much more generally, because there's no reason that logic is limited to just these specific compound operators, or in fact to any individual operator; we should probably warn about all inconsistent references to the same variable within a single statement.

Yeah, good point; I'm convinced.

(I'd draw the line at statement boundaries, though, because requiring function-wide consistency could be really noisy.)

At least, I don't think we should warn if both a and this->a are used in different statements in the same function, and a is shadowed by a local declaration wherever this->a is used, and I think it might be reasonable to not warn on a member a if the name a is ever declared in the function regardless of where it's in scope. For the remaining cases, where a is never declared locally, I suspect the false positive rate for warning if the function uses both a and this->a would be lower, but we could probably get some data on that pretty easily. Speaking of data, it'd be nice to get some evidence that this is a mistake that actually happens before landing a dedicated warning for it :)

[rjmccall]

Totally fair :)

[python3kgae]

Revert change for self-assign-field which deserve its own pull request.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

15660

[python3kgae]

Update the comment done by rjmccall.

clang/lib/Sema/SemaExpr.cpp

const ValueDecl *RHSDecl =
         cast<ValueDecl>(MR->getMemberDecl()->getCanonicalDecl());
     if (LHSDecl != RHSDecl)
       return;
     if (LHSDecl->getType().isVolatileQualified())
       return;
     if (const ReferenceType *RefTy = LHSDecl->getType()->getAs<ReferenceType>())
       if (RefTy->getPointeeType().isVolatileQualified())
         return;
 
     Sema.Diag(Loc, diag::warn_identity_field_assign) << 0;
   }
 
   // Objective-C instance variables
   ObjCIvarRefExpr *OL = dyn_cast<ObjCIvarRefExpr>(LHSExpr);
   ObjCIvarRefExpr *OR = dyn_cast<ObjCIvarRefExpr>(RHSExpr);
   if (OL && OR && OL->getDecl() == OR->getDecl()) {
     DeclRefExpr *RL = dyn_cast<DeclRefExpr>(OL->getBase()->IgnoreImpCasts());

 }
 
 /// Build an overloaded binary operator expression in the given scope.
 static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc,
                                        BinaryOperatorKind Opc,
                                        Expr *LHS, Expr *RHS) {
   switch (Opc) {
   case BO_Assign:
+    // Skip diagnose on compound assignment.
+    DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
+    [[fallthrough]];
   case BO_DivAssign:
   case BO_RemAssign:
   case BO_SubAssign:
   case BO_AndAssign:
   case BO_OrAssign:
   case BO_XorAssign:
-    DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false);
     CheckIdentityFieldAssignment(LHS, RHS, OpLoc, S);
     break;
   default:
     break;
   }
 
   // Find all of the overloaded operators visible from this point.
   UnresolvedSet<16> Functions;
   S.LookupBinOp(Sc, OpLoc, Opc, Functions);

clang/test/SemaCXX/warn-self-assign-overloaded.cpp

void f() {
   b = b; // expected-warning{{explicitly assigning}}
   a = b;
   b = a = b;
   a = a = a; // expected-warning{{explicitly assigning}}
   a = b = b = a;
 
 #ifndef DUMMY
   a *= a;
-  a /= a; // expected-warning {{explicitly assigning}}
-  a %= a; // expected-warning {{explicitly assigning}}
+  a /= a;
+  a %= a;
   a += a;
-  a -= a; // expected-warning {{explicitly assigning}}
+  a -= a;
   a <<= a;
   a >>= a;
-  a &= a; // expected-warning {{explicitly assigning}}
-  a |= a; // expected-warning {{explicitly assigning}}
-  a ^= a; // expected-warning {{explicitly assigning}}
+  a &= a;
+  a |= a;
+  a ^= a;
 #endif
 }
 
 void false_positives() {
 #define OP =
 #define LHS a
 #define RHS a
   S a;