Warn on self-assignment to member variables. PR13104.

llvm-svn: 159394

clang/include/clang/Basic/DiagnosticGroups.td

@@ -169,7 +169,8 @@ def ReturnTypeCLinkage : DiagGroup<"return-type-c-linkage">;
 def ReturnType : DiagGroup<"return-type", [ReturnTypeCLinkage]>;
 def BindToTemporaryCopy : DiagGroup<"bind-to-temporary-copy",
                                     [CXX98CompatBindToTemporaryCopy]>;
-def SelfAssignment : DiagGroup<"self-assign">;
+def SelfAssignmentMemvar : DiagGroup<"self-assign-memvar">;
+def SelfAssignment : DiagGroup<"self-assign", [SelfAssignmentMemvar]>;
 def SemiBeforeMethodBody : DiagGroup<"semicolon-before-method-body">;
 def Sentinel : DiagGroup<"sentinel">;
 def MissingMethodReturnType : DiagGroup<"missing-method-return-type">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -5274,6 +5274,10 @@ def warn_stringcompare : Warning<
   "unspecified (use strncmp instead)">,
   InGroup<DiagGroup<"string-compare">>;
 
+def warn_identity_memvar_assign : Warning<
+  "assigning %select{member variable|instance variable}0 to itself">,
+  InGroup<SelfAssignmentMemvar>;
+
 // Generic selections.
 def err_assoc_type_incomplete : Error<
   "type %0 in generic association incomplete">;

clang/lib/Sema/SemaExpr.cpp

@@ -7558,7 +7558,27 @@ static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) {
   return true;
 }
 
+static void CheckIdentityMemvarAssignment(Expr *LHSExpr, Expr *RHSExpr,
+                                          SourceLocation Loc,
+                                          Sema &Sema) {
+  // C / C++ memvars
+  MemberExpr *ML = dyn_cast<MemberExpr>(LHSExpr);
+  MemberExpr *MR = dyn_cast<MemberExpr>(RHSExpr);
+  if (ML && MR && ML->getMemberDecl() == MR->getMemberDecl()) {
+    if (isa<CXXThisExpr>(ML->getBase()) && isa<CXXThisExpr>(MR->getBase()))
+      Sema.Diag(Loc, diag::warn_identity_memvar_assign) << 0;
+  }
 
+  // Objective-C memvars
+  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());
+    DeclRefExpr *RR = dyn_cast<DeclRefExpr>(OR->getBase()->IgnoreImpCasts());
+    if (RL && RR && RL->getDecl() == RR->getDecl())
+      Sema.Diag(Loc, diag::warn_identity_memvar_assign) << 1;
+  }
+}
 
 // C99 6.5.16.1
 QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
@@ -7575,6 +7595,10 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
                                              CompoundType;
   AssignConvertType ConvTy;
   if (CompoundType.isNull()) {
+    Expr *RHSCheck = RHS.get();
+
+    CheckIdentityMemvarAssignment(LHSExpr, RHSCheck, Loc, *this);
+
     QualType LHSTy(LHSType);
     ConvTy = CheckSingleAssignmentConstraints(LHSTy, RHS);
     if (RHS.isInvalid())
@@ -7595,7 +7619,6 @@ QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
     // If the RHS is a unary plus or minus, check to see if they = and + are
     // right next to each other.  If so, the user may have typo'd "x =+ 4"
     // instead of "x += 4".
-    Expr *RHSCheck = RHS.get();
     if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(RHSCheck))
       RHSCheck = ICE->getSubExpr();
     if (UnaryOperator *UO = dyn_cast<UnaryOperator>(RHSCheck)) {

clang/test/Sema/warn-self-assign-memvar.mm

@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class S {
+ public:
+  int a_;
+  void s(int a) {
+    a_ = a_;  // expected-warning {{assigning member variable to itself}}
+
+    // Don't really care about this one either way.
+    this->a_ = a_;  // expected-warning {{assigning member variable to itself}}
+
+    a_ += a_;  // Shouldn't warn.
+  }
+};
+
+void f0(S* s) {
+  // Would be nice to have, but not important.
+  s->a_ = s->a_;
+}
+
+void f1(S* s, S* t) {
+  // Shouldn't warn.
+  t->a_ = s->a_;
+}
+
+struct T {
+  S* s_;
+};
+
+void f2(T* t) {
+  // Would be nice to have, but even less important.
+  t->s_->a_ = t->s_->a_;
+}
+
+void f3(T* t, T* t2) {
+  // Shouldn't warn.
+  t2->s_->a_ = t->s_->a_;
+}
+
+void f4(int i) {
+  // This is a common pattern to silence "parameter unused". Shouldn't warn.
+  i = i;
+
+  int j = 0;
+  j = j;  // Likewise.
+}
+
+@interface I {
+  int a_;
+}
+@end
+
+@implementation I
+- (void)setA:(int)a {
+  a_ = a_;  // expected-warning {{assigning instance variable to itself}}
+}
+
+- (void)foo:(I*)i {
+  // Don't care much about this warning.
+  i->a_ = i->a_;  // expected-warning {{assigning instance variable to itself}}
+
+  // Shouldn't warn.
+  a_ = i->a_;
+  i->a_ = a_;
+}
+@end

clang/test/SemaObjC/provisional-ivar-lookup.m

@@ -36,7 +36,7 @@
 
 @synthesize PROP=PROP;
 - (void)setPROP:(int)value {
-    PROP = PROP;        // OK
+    PROP = value;        // OK
 }
 
 @end