[clang] Add a fixit for warn-self-assign if LHS is a field with the same name as parameter on RHS

Add a fix-it for the common case of setters/constructors using parameters with the same name as fields
```lang=c++
struct A{
  int X;
  A(int X) { /*this->*/X = X; }
  void setX(int X) { /*this->*/X = X;
};
```

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D129202

clang/docs/ReleaseNotes.rst

@@ -279,6 +279,9 @@ Improvements to Clang's diagnostics
   unevaluated operands of a ``typeid`` expression, as they are now
   modeled correctly in the CFG. This fixes
   `Issue 21668 <https://github.com/llvm/llvm-project/issues/21668>`_.
+- ``-Wself-assign``, ``-Wself-assign-overloaded`` and ``-Wself-move`` will 
+  suggest a fix if the decl being assigned is a parameter that shadows a data
+  member of the contained class.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -6610,13 +6610,16 @@ def warn_addition_in_bitshift : Warning<
   "'%1' will be evaluated first">, InGroup<ShiftOpParentheses>;
 
 def warn_self_assignment_builtin : Warning<
-  "explicitly assigning value of variable of type %0 to itself">,
+  "explicitly assigning value of variable of type %0 to itself%select{|; did "
+  "you mean to assign to member %2?}1">,
   InGroup<SelfAssignment>, DefaultIgnore;
 def warn_self_assignment_overloaded : Warning<
-  "explicitly assigning value of variable of type %0 to itself">,
+  "explicitly assigning value of variable of type %0 to itself%select{|; did "
+  "you mean to assign to member %2?}1">,
   InGroup<SelfAssignmentOverloaded>, DefaultIgnore;
 def warn_self_move : Warning<
-  "explicitly moving variable of type %0 to itself">,
+  "explicitly moving variable of type %0 to itself%select{|; did you mean to "
+  "move to member %2?}1">,
   InGroup<SelfMove>, DefaultIgnore;
 
 def err_builtin_move_forward_unsupported : Error<

clang/include/clang/Sema/Sema.h

@@ -5170,6 +5170,11 @@ public:
   void DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
                         SourceLocation OpLoc);
 
+  /// Returns a field in a CXXRecordDecl that has the same name as the decl \p
+  /// SelfAssigned when inside a CXXMethodDecl.
+  const FieldDecl *
+  getSelfAssignmentClassMemberCandidate(const ValueDecl *SelfAssigned);
+
   /// Warn if we're implicitly casting from a _Nullable pointer type to a
   /// _Nonnull one.
   void diagnoseNullableToNonnullConversion(QualType DstType, QualType SrcType,

clang/lib/Sema/SemaChecking.cpp

@@ -16830,9 +16830,15 @@ void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
         RHSDeclRef->getDecl()->getCanonicalDecl())
       return;
 
-    Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
+    auto D = Diag(OpLoc, diag::warn_self_move)
-                                        << LHSExpr->getSourceRange()
+             << LHSExpr->getType() << LHSExpr->getSourceRange()
-                                        << RHSExpr->getSourceRange();
+             << RHSExpr->getSourceRange();
+    if (const FieldDecl *F =
+            getSelfAssignmentClassMemberCandidate(RHSDeclRef->getDecl()))
+      D << 1 << F
+        << FixItHint::CreateInsertion(LHSDeclRef->getBeginLoc(), "this->");
+    else
+      D << 0;
     return;
   }
 
@@ -16867,16 +16873,16 @@ void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
         RHSDeclRef->getDecl()->getCanonicalDecl())
       return;
 
-    Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
+    Diag(OpLoc, diag::warn_self_move)
-                                        << LHSExpr->getSourceRange()
+        << LHSExpr->getType() << 0 << LHSExpr->getSourceRange()
-                                        << RHSExpr->getSourceRange();
+        << RHSExpr->getSourceRange();
     return;
   }
 
   if (isa<CXXThisExpr>(LHSBase) && isa<CXXThisExpr>(RHSBase))
-    Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
+    Diag(OpLoc, diag::warn_self_move)
-                                        << LHSExpr->getSourceRange()
+        << LHSExpr->getType() << 0 << LHSExpr->getSourceRange()
-                                        << RHSExpr->getSourceRange();
+        << RHSExpr->getSourceRange();
 }
 
 //===--- Layout compatibility ----------------------------------------------//

clang/lib/Sema/SemaExpr.cpp

@@ -14600,6 +14600,40 @@ static inline UnaryOperatorKind ConvertTokenKindToUnaryOpcode(
   return Opc;
 }
 
+const FieldDecl *
+Sema::getSelfAssignmentClassMemberCandidate(const ValueDecl *SelfAssigned) {
+  // Explore the case for adding 'this->' to the LHS of a self assignment, very
+  // common for setters.
+  // struct A {
+  // int X;
+  // -void setX(int X) { X = X; }
+  // +void setX(int X) { this->X = X; }
+  // };
+
+  // Only consider parameters for self assignment fixes.
+  if (!isa<ParmVarDecl>(SelfAssigned))
+    return nullptr;
+  const auto *Method =
+      dyn_cast_or_null<CXXMethodDecl>(getCurFunctionDecl(true));
+  if (!Method)
+    return nullptr;
+
+  const CXXRecordDecl *Parent = Method->getParent();
+  // In theory this is fixable if the lambda explicitly captures this, but
+  // that's added complexity that's rarely going to be used.
+  if (Parent->isLambda())
+    return nullptr;
+
+  // FIXME: Use an actual Lookup operation instead of just traversing fields
+  // in order to get base class fields.
+  auto Field =
+      llvm::find_if(Parent->fields(),
+                    [Name(SelfAssigned->getDeclName())](const FieldDecl *F) {
+                      return F->getDeclName() == Name;
+                    });
+  return (Field != Parent->field_end()) ? *Field : nullptr;
+}
+
 /// DiagnoseSelfAssignment - Emits a warning if a value is assigned to itself.
 /// This warning suppressed in the event of macro expansions.
 static void DiagnoseSelfAssignment(Sema &S, Expr *LHSExpr, Expr *RHSExpr,
@@ -14630,10 +14664,16 @@ static void DiagnoseSelfAssignment(Sema &S, Expr *LHSExpr, Expr *RHSExpr,
     if (RefTy->getPointeeType().isVolatileQualified())
       return;
 
-  S.Diag(OpLoc, IsBuiltin ? diag::warn_self_assignment_builtin
+  auto Diag = S.Diag(OpLoc, IsBuiltin ? diag::warn_self_assignment_builtin
-                          : diag::warn_self_assignment_overloaded)
+                                      : diag::warn_self_assignment_overloaded)
-      << LHSDeclRef->getType() << LHSExpr->getSourceRange()
+              << LHSDeclRef->getType() << LHSExpr->getSourceRange()
-      << RHSExpr->getSourceRange();
+              << RHSExpr->getSourceRange();
+  if (const FieldDecl *SelfAssignField =
+          S.getSelfAssignmentClassMemberCandidate(RHSDecl))
+    Diag << 1 << SelfAssignField
+         << FixItHint::CreateInsertion(LHSDeclRef->getBeginLoc(), "this->");
+  else
+    Diag << 0;
 }
 
 /// Check if a bitwise-& is performed on an Objective-C pointer.  This

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

@@ -65,3 +65,26 @@ void instantiate() {
   g<int>();
   g<S>();
 }
+
+struct Foo {
+  int A;
+
+  Foo(int A) : A(A) {}
+
+  void setA(int A) {
+    A = A; // expected-warning{{explicitly assigning value of variable of type 'int' to itself; did you mean to assign to member 'A'?}}
+  }
+
+  void setThroughLambda() {
+    [](int A) {
+      // To fix here we would need to insert an explicit capture 'this'
+      A = A; // expected-warning{{explicitly assigning}}
+    }(5);
+
+    [this](int A) {
+      this->A = 0;
+      // This fix would be possible by just adding this-> as above, but currently unsupported.
+      A = A; // expected-warning{{explicitly assigning}}
+    }(5);
+  }
+};

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

@@ -4,6 +4,8 @@ struct C {
   int a;
   int b;
 
+  C(int a, int b) : a(a), b(b) {}
+
   void f() {
     a = a; // expected-warning {{assigning field to itself}}
     b = b; // expected-warning {{assigning field to itself}}

clang/test/SemaCXX/warn-self-move.cpp

@@ -39,6 +39,9 @@ class field_test {
     other.x = std::move(x);
     other.x = std::move(other.x);  // expected-warning{{explicitly moving}}
   }
+  void withSuggest(int x) {
+    x = std::move(x); // expected-warning{{explicitly moving variable of type 'int' to itself; did you mean to move to member 'x'?}}
+  }
 };
 
 struct A {};