[clang] 54f57d3 - [clang] Add a fixit for warn-self-assign if LHS is a field with the same name as parameter on RHS
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Sat Jul 9 00:28:17 PDT 2022
Author: Nathan James
Date: 2022-07-09T08:28:07+01:00
New Revision: 54f57d3847c00d0233e287ebb5283d04e6083062
URL: https://github.com/llvm/llvm-project/commit/54f57d3847c00d0233e287ebb5283d04e6083062
DIFF: https://github.com/llvm/llvm-project/commit/54f57d3847c00d0233e287ebb5283d04e6083062.diff
LOG: [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
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaChecking.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/SemaCXX/warn-self-assign-builtin.cpp
clang/test/SemaCXX/warn-self-assign-field-builtin.cpp
clang/test/SemaCXX/warn-self-move.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c6d36568099b4..5dae6205efa05 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/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
-------------------------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index fd10bd9c70e6a..0d25c718d09e9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/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<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 4fc53a0697a5c..75a2e7eb31d19 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5170,6 +5170,11 @@ class Sema final {
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
index 3ed745c5634c8..d3929361213ad 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/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()
- << 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 @@ void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
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<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()
+ << RHSExpr->getSourceRange();
}
//===--- Layout compatibility ----------------------------------------------//
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index b9ecde6f20a0a..742c4828b8dcc 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/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
- : 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
index 212fc241e3e7e..5211accce79d3 100644
--- a/clang/test/SemaCXX/warn-self-assign-builtin.cpp
+++ b/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);
+ }
+};
diff --git a/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp b/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp
index 7ae07e7de672d..57526242833ed 100644
--- a/clang/test/SemaCXX/warn-self-assign-field-builtin.cpp
+++ b/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}}
diff --git a/clang/test/SemaCXX/warn-self-move.cpp b/clang/test/SemaCXX/warn-self-move.cpp
index f1bcf6e051976..0987e9b6bf601 100644
--- a/clang/test/SemaCXX/warn-self-move.cpp
+++ b/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 {};
More information about the cfe-commits
mailing list