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

[njames93]

Summary

Add a fix-it for the common case of setters/constructors using parameters with the same name as fields

struct A{
  int X;
  A(int X) { /*this->*/X = X; }
  void setX(int X) { /*this->*/X = X;
};

[njames93]

Remove unnecessary extra include.

[aaron.ballman]

Thank you for working on this, I think it's a nice new diagnostic. You should add a release note to describe it. One question I have is about how chatty this is over real world code, especially older code bases. There used to be two prevailing ways to silence the "unused variable" warnings you would get when a variable is used in an assert: (void)whatever; and whatever = whatever;; I'm worried the latter case is still prevalent enough that having this warning on by default would be a problem, but I'm also optimistic my worry is unfounded.

clang/include/clang/Basic/DiagnosticSemaKinds.td

6625

We don't normally have trailing punctuation... except for question marks which we will happily use when asking the user a question.

clang/lib/Sema/SemaExpr.cpp

14676–14681

Removing the llvm:: from those mostly because we already call cast<> without the prefix. Oddly, I find myself liking the llvm:: in llvm::find_if though, so take or leave the suggestions about it.

14687

Why wouldn't we want to look through base classes for a member there? Considering a case like:

struct Base {
  int X;
};

struct Derived : Base {
  Derived(int X) { X = X; } // Oooooops
};

it seems like we would want to use a real lookup operation instead of the fields of the class. Or did you try that and found it was a performance concern?

14692–14695

Use of a note like this isn't awful, but I'm not super happy that the note always shows up on the same line as the warning. It seems like it would be cleaner (in terms of the user experience) to use a %select on the warning diagnostic to decide whether to add the "did you mean" information, and associate the fix directly with the warning. However, there's a slight functional difference with that approach which may matter to you: fixits attached to a note are not automatically applied when in fixit mode, but they are when attached to a warning or error. I don't know a reason why applying the fix-it automatically would be a bad thing in this case, so use of a note loses a bit of useful functionality.

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

69–73

An additional test case I'd like to see is:

struct S {
  int X;

  S(int X) : X(X) {} // Don't warn about this
};

[njames93]

In D129202#3633618, @aaron.ballman wrote:

Thank you for working on this, I think it's a nice new diagnostic. You should add a release note to describe it. One question I have is about how chatty this is over real world code, especially older code bases. There used to be two prevailing ways to silence the "unused variable" warnings you would get when a variable is used in an assert: (void)whatever; and whatever = whatever;; I'm worried the latter case is still prevalent enough that having this warning on by default would be a problem, but I'm also optimistic my worry is unfounded.

It's not adding a new warning, so chattiness of this patch shouldn't really be much of an issue.

clang/lib/Sema/SemaExpr.cpp

14687

Once you start getting into deep class hierarchies we start to lose confidence that the fix is what the user intended. Then there is also the issue of access where the field may not even be accessible in Derived and it just adds complexity that I'm not sure is really warranted.

14692–14695

That is exactly the reason for the note. Because we aren't fixing invalid code(self assignment is suspicious but not an error) and the fix has a noticeable change of behaviour, it doesn't seem wise to automatically apply the fix-it.
I don't have a plan down the line to implement this, but what if the intent of the programmer was actually a near miss, like in the, heavily contrived, example below.

struct Foo {
  int Bar;
  void Stuff(int Bar) {
    int bar;
    // ...
    Bar = Bar; // Did they intend bar = Bar or this->Bar = Bar?
  }
};

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

69–73

This patch isn't adding a warning, it's just adding a fix to an existing warning. However there is currently no test cases for that particular instance so I can see value for that use case

[njames93]

Address comments.

[aaron.ballman]

In D129202#3634646, @njames93 wrote:

In D129202#3633618, @aaron.ballman wrote:

Thank you for working on this, I think it's a nice new diagnostic. You should add a release note to describe it. One question I have is about how chatty this is over real world code, especially older code bases. There used to be two prevailing ways to silence the "unused variable" warnings you would get when a variable is used in an assert: (void)whatever; and whatever = whatever;; I'm worried the latter case is still prevalent enough that having this warning on by default would be a problem, but I'm also optimistic my worry is unfounded.

It's not adding a new warning, so chattiness of this patch shouldn't really be much of an issue.

It's not triggering a diagnostic in a situation we didn't used to diagnose, but it is doubling the number of diagnostics emitted in some circumstances. So it's not a new warning, but chattiness is still a valid concern. We don't want to swamp the user with diagnostic messages.

clang/lib/Sema/SemaExpr.cpp

14687

Once you start getting into deep class hierarchies we start to lose confidence that the fix is what the user intended.

Why? If what we find is a variable (or data member),

Then there is also the issue of access where the field may not even be accessible in Derived and it just adds complexity that I'm not sure is really warranted.

The complexity question is certainly something we should keep in mind, but I'm not really sure that using a Lookup method on Sema adds a significant amount of complexity, either.

14692–14695

That is exactly the reason for the note. Because we aren't fixing invalid code(self assignment is suspicious but not an error) and the fix has a noticeable change of behaviour, it doesn't seem wise to automatically apply the fix-it.

Hmmm, I agree with you that self assignment is suspicious but not an error. However, using a K&R C function declaration is also suspicious but not an error; we still attach a fix-it to the warning for it: https://godbolt.org/z/afPr45Pd7. Self-assignment warnings are off-by-default (same as strict prototype warnings), so it seems reasonable to me that if a user opts into the diagnostic, they get the fixes on the warnings.

[njames93]

Attach fix directly to the warning.
Extend checking to also work with move assignments.

[aaron.ballman]

LGTM with some minor stuff you can fix when landing. Thank you for this!

clang/include/clang/Sema/Sema.h

5173–5174

Should this return a const pointer, and have some comments?

[njames93]

Address comment.

clang/docs/ReleaseNotes.rst

- When using class templates without arguments, clang now tells developers
   that template arguments are missing in certain contexts.
   This fixes `Issue 55962 <https://github.com/llvm/llvm-project/issues/55962>`_.
 - Printable Unicode characters within `static_assert` messages are no longer
   escaped.
 - The ``-Winfinite-recursion`` diagnostic no longer warns about
   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
 -------------------------------------------------
 
 - Improve __builtin_dump_struct:
 
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_concatenated_string_literal_silence :Note<
   "place parentheses around the string literal to silence warning">;
 
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%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<
   "unsupported signature for %q0">;
 def err_use_of_unaddressable_function : Error<
   "taking address of non-addressable standard library function">;
 // FIXME: This should also be in -Wc++23-compat once we have it.
 def warn_cxx20_compat_use_of_unaddressable_function : Warning<
   "taking address of non-addressable standard library function "
   "is incompatible with C++20">, InGroup<CXX20Compat>;
 

clang/include/clang/Sema/Sema.h

public:
   /// \p PossibleBody, has a suspicious null statement as a body.
   void DiagnoseEmptyLoopBody(const Stmt *S,
                              const Stmt *PossibleBody);
 
   /// Warn if a value is moved to itself.
   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,
                                            SourceLocation Loc);
 
   /// Warn when implicitly casting 0 to nullptr.
   void diagnoseZeroToNullptrConversion(CastKind Kind, const Expr *E);
 

clang/lib/Sema/SemaChecking.cpp

void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
   // Two DeclRefExpr's, check that the decls are the same.
   if (LHSDeclRef && RHSDeclRef) {
     if (!LHSDeclRef->getDecl() || !RHSDeclRef->getDecl())
       return;
     if (LHSDeclRef->getDecl()->getCanonicalDecl() !=
         RHSDeclRef->getDecl()->getCanonicalDecl())
       return;
 
-    Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
-                                        << LHSExpr->getSourceRange()
-                                        << RHSExpr->getSourceRange();
+    auto D = Diag(OpLoc, diag::warn_self_move)
+             << LHSExpr->getType() << LHSExpr->getSourceRange()
+    if (const FieldDecl *F =
+            getSelfAssignmentClassMemberCandidate(RHSDeclRef->getDecl()))
+      D << 1 << F
+        << FixItHint::CreateInsertion(LHSDeclRef->getBeginLoc(), "this->");
+    else
+      D << 0;
     return;
   }
 
   // Member variables require a different approach to check for self moves.
   // MemberExpr's are the same if every nested MemberExpr refers to the same
   // Decl and that the base Expr's are DeclRefExpr's with the same Decl or
   // the base Expr's are CXXThisExpr's.
   const Expr *LHSBase = LHSExpr;
void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
   RHSDeclRef = dyn_cast<DeclRefExpr>(RHSBase);
   if (LHSDeclRef && RHSDeclRef) {
     if (!LHSDeclRef->getDecl() || !RHSDeclRef->getDecl())
       return;
     if (LHSDeclRef->getDecl()->getCanonicalDecl() !=
         RHSDeclRef->getDecl()->getCanonicalDecl())
       return;
 
-    Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
-                                        << LHSExpr->getSourceRange()
-                                        << RHSExpr->getSourceRange();
+    Diag(OpLoc, diag::warn_self_move)
+        << LHSExpr->getType() << 0 << LHSExpr->getSourceRange()
     return;
   }
 
   if (isa<CXXThisExpr>(LHSBase) && isa<CXXThisExpr>(RHSBase))
-    Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType()
-                                        << LHSExpr->getSourceRange()
-                                        << RHSExpr->getSourceRange();
+    Diag(OpLoc, diag::warn_self_move)
+        << LHSExpr->getType() << 0 << LHSExpr->getSourceRange()
 }
 
 //===--- Layout compatibility ----------------------------------------------//
 
 static bool isLayoutCompatible(ASTContext &C, QualType T1, QualType T2);
 
 /// Check if two enumeration types are layout-compatible.
 static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {

clang/lib/Sema/SemaExpr.cpp

static inline UnaryOperatorKind ConvertTokenKindToUnaryOpcode(
   case tok::exclaim:      Opc = UO_LNot; break;
   case tok::kw___real:    Opc = UO_Real; break;
   case tok::kw___imag:    Opc = UO_Imag; break;
   case tok::kw___extension__: Opc = UO_Extension; break;
   }
   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,
                                    SourceLocation OpLoc, bool IsBuiltin) {
   if (S.inTemplateInstantiation())
     return;
   if (S.isUnevaluatedContext())
     return;
static void DiagnoseSelfAssignment(Sema &S, Expr *LHSExpr, Expr *RHSExpr,
   if (LHSDecl != RHSDecl)
     return;
   if (LHSDecl->getType().isVolatileQualified())
     return;
   if (const ReferenceType *RefTy = LHSDecl->getType()->getAs<ReferenceType>())
     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)
               << LHSDeclRef->getType() << LHSExpr->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
 /// is usually indicative of introspection within the Objective-C pointer.
 static void checkObjCPointerIntrospection(Sema &S, ExprResult &L, ExprResult &R,
                                           SourceLocation OpLoc) {
   if (!S.getLangOpts().ObjC)
     return;
 
   const Expr *ObjCPointerExpr = nullptr, *OtherExpr = nullptr;
   const Expr *LHS = L.get();
   const Expr *RHS = R.get();
 
   if (LHS->IgnoreParenCasts()->getType()->isObjCObjectPointerType()) {
     ObjCPointerExpr = LHS;
     OtherExpr = RHS;
   }
   else if (RHS->IgnoreParenCasts()->getType()->isObjCObjectPointerType()) {
     ObjCPointerExpr = RHS;
     OtherExpr = LHS;
   }
 
   // This warning is deliberately made very specific to reduce false
   // positives with logic that uses '&' for hashing.  This logic mainly
   // looks for code trying to introspect into tagged pointers, which
   // code should generally never do.
   if (ObjCPointerExpr && isa<IntegerLiteral>(OtherExpr->IgnoreParenCasts())) {

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

 void g() {
   T a;
   a = a; // expected-warning{{explicitly assigning}}
 }
 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

 // RUN: %clang_cc1 -fsyntax-only -Wself-assign-field -verify %s
 
 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}}
     a = b;
 
     this->a = a;       // expected-warning {{assigning field to itself}}
     this->b = b;       // expected-warning {{assigning field to itself}}
     a = this->a;       // expected-warning {{assigning field to itself}}

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

 class field_test {
   int x;
   field_test(field_test&& other) {
     x = std::move(x);  // expected-warning{{explicitly moving}}
     x = std::move(other.x);
     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 {};
 struct B { A a; };
 struct C { C() {}; ~C() {} };
 void struct_test() {
   A a;
   a = std::move(a);  // expected-warning{{explicitly moving}}
 
   B b;
   b = std::move(b);  // expected-warning{{explicitly moving}}
   b.a = std::move(b.a);  // expected-warning{{explicitly moving}}
 
   C c;
   c = std::move(c);  // expected-warning{{explicitly moving}}
 }