diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -279,6 +279,9 @@ unevaluated operands of a ``typeid`` expression, as they are now modeled correctly in the CFG. This fixes `Issue 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 ------------------------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6610,13 +6610,16 @@ "'%1' will be evaluated first">, InGroup; 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, 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, 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, DefaultIgnore; def err_builtin_move_forward_unsupported : Error< diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -5170,6 +5170,11 @@ 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, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -16830,9 +16830,15 @@ 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() + << 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 @@ 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() + << RHSExpr->getSourceRange(); return; } if (isa(LHSBase) && isa(RHSBase)) - Diag(OpLoc, diag::warn_self_move) << LHSExpr->getType() - << LHSExpr->getSourceRange() - << RHSExpr->getSourceRange(); + Diag(OpLoc, diag::warn_self_move) + << LHSExpr->getType() << 0 << LHSExpr->getSourceRange() + << RHSExpr->getSourceRange(); } //===--- Layout compatibility ----------------------------------------------// diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -14600,6 +14600,40 @@ 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(SelfAssigned)) + return nullptr; + const auto *Method = + dyn_cast_or_null(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 @@ if (RefTy->getPointeeType().isVolatileQualified()) return; - S.Diag(OpLoc, IsBuiltin ? diag::warn_self_assignment_builtin - : diag::warn_self_assignment_overloaded) - << LHSDeclRef->getType() << LHSExpr->getSourceRange() - << RHSExpr->getSourceRange(); + 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 diff --git a/clang/test/SemaCXX/warn-self-assign-builtin.cpp b/clang/test/SemaCXX/warn-self-assign-builtin.cpp --- a/clang/test/SemaCXX/warn-self-assign-builtin.cpp +++ b/clang/test/SemaCXX/warn-self-assign-builtin.cpp @@ -65,3 +65,26 @@ g(); g(); } + +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); + } +}; diff --git a/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp b/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp --- a/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp +++ b/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp @@ -4,6 +4,8 @@ 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}} diff --git a/clang/test/SemaCXX/warn-self-move.cpp b/clang/test/SemaCXX/warn-self-move.cpp --- a/clang/test/SemaCXX/warn-self-move.cpp +++ b/clang/test/SemaCXX/warn-self-move.cpp @@ -39,6 +39,9 @@ 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 {};