Implement -Wparentheses: warn about using assignments in contexts that require

conditions.  Add a fixit to insert the parentheses.  Also fix a very minor
possible memory leak in 'for' conditions.

Fixes PR 4876 and rdar://problem/7289172

llvm-svn: 83907

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -1727,7 +1727,11 @@ def err_not_tag_in_scope : Error<
 
 def err_cannot_form_pointer_to_member_of_reference_type : Error<
   "cannot form a pointer-to-member to member %0 of reference type %1">;
-  
+
+def warn_condition_is_assignment : Warning<"using the result of an "
+  "assignment as a condition without parentheses">,
+  InGroup<Parentheses>, DefaultIgnore;
+
 def warn_value_always_zero : Warning<"%0 is always zero in this context">;
 def warn_value_always_false : Warning<"%0 is always false in this context">;
 

clang/lib/Sema/Sema.h

@@ -3675,6 +3675,20 @@ public:
                                  SourceLocation lbrac, SourceLocation rbrac,
                                  QualType &ReturnType);
 
+  /// CheckBooleanCondition - Diagnose problems involving the use of
+  /// the given expression as a boolean condition (e.g. in an if
+  /// statement).  Also performs the standard function and array
+  /// decays, possibly changing the input variable.
+  ///
+  /// \param Loc - A location associated with the condition, e.g. the
+  /// 'if' keyword.
+  /// \return true iff there were any errors
+  bool CheckBooleanCondition(Expr *&CondExpr, SourceLocation Loc);
+
+  /// DiagnoseAssignmentAsCondition - Given that an expression is
+  /// being used as a boolean condition, warn if it's an assignment.
+  void DiagnoseAssignmentAsCondition(Expr *E);
+
   /// CheckCXXBooleanCondition - Returns true if conversion to bool is invalid.
   bool CheckCXXBooleanCondition(Expr *&CondExpr);
 

clang/lib/Sema/SemaExpr.cpp

@@ -6268,3 +6268,56 @@ bool Sema::CheckCallReturnType(QualType ReturnType, SourceLocation Loc,
   return false;
 }
 
+// Diagnose the common s/=/==/ typo.  Note that adding parentheses
+// will prevent this condition from triggering, which is what we want.
+void Sema::DiagnoseAssignmentAsCondition(Expr *E) {
+  SourceLocation Loc;
+
+  if (isa<BinaryOperator>(E)) {
+    BinaryOperator *Op = cast<BinaryOperator>(E);
+    if (Op->getOpcode() != BinaryOperator::Assign)
+      return;
+
+    Loc = Op->getOperatorLoc();
+  } else if (isa<CXXOperatorCallExpr>(E)) {
+    CXXOperatorCallExpr *Op = cast<CXXOperatorCallExpr>(E);
+    if (Op->getOperator() != OO_Equal)
+      return;
+
+    Loc = Op->getOperatorLoc();
+  } else {
+    // Not an assignment.
+    return;
+  }
+
+  // We want to insert before the start of the expression...
+  SourceLocation Open = E->getSourceRange().getBegin();
+  // ...and one character after the end.
+  SourceLocation Close = E->getSourceRange().getEnd().getFileLocWithOffset(1);
+  
+  Diag(Loc, diag::warn_condition_is_assignment)
+    << E->getSourceRange()
+    << CodeModificationHint::CreateInsertion(Open, "(")
+    << CodeModificationHint::CreateInsertion(Close, ")");
+}
+
+bool Sema::CheckBooleanCondition(Expr *&E, SourceLocation Loc) {
+  DiagnoseAssignmentAsCondition(E);
+
+  if (!E->isTypeDependent()) {
+    DefaultFunctionArrayConversion(E);
+
+    QualType T = E->getType();
+
+    if (getLangOptions().CPlusPlus) {
+      if (CheckCXXBooleanCondition(E)) // C++ 6.4p4
+        return true;
+    } else if (!T->isScalarType()) { // C99 6.8.4.1p1
+      Diag(Loc, diag::err_typecheck_statement_requires_scalar)
+        << T << E->getSourceRange();
+      return true;
+    }
+  }
+
+  return false;
+}

clang/lib/Sema/SemaStmt.cpp

@@ -214,20 +214,9 @@ Sema::ActOnIfStmt(SourceLocation IfLoc, FullExprArg CondVal,
   Expr *condExpr = CondResult.takeAs<Expr>();
 
   assert(condExpr && "ActOnIfStmt(): missing expression");
-
-  if (!condExpr->isTypeDependent()) {
-    DefaultFunctionArrayConversion(condExpr);
-    // Take ownership again until we're past the error checking.
+  if (CheckBooleanCondition(condExpr, IfLoc)) {
     CondResult = condExpr;
-    QualType condType = condExpr->getType();
-
-    if (getLangOptions().CPlusPlus) {
-      if (CheckCXXBooleanCondition(condExpr)) // C++ 6.4p4
-        return StmtError();
-    } else if (!condType->isScalarType()) // C99 6.8.4.1p1
-      return StmtError(Diag(IfLoc,
-                            diag::err_typecheck_statement_requires_scalar)
-                       << condType << condExpr->getSourceRange());
+    return StmtError();
   }
 
   Stmt *thenStmt = ThenVal.takeAs<Stmt>();
@@ -577,18 +566,9 @@ Sema::ActOnWhileStmt(SourceLocation WhileLoc, FullExprArg Cond, StmtArg Body) {
   Expr *condExpr = CondArg.takeAs<Expr>();
   assert(condExpr && "ActOnWhileStmt(): missing expression");
 
-  if (!condExpr->isTypeDependent()) {
-    DefaultFunctionArrayConversion(condExpr);
+  if (CheckBooleanCondition(condExpr, WhileLoc)) {
     CondArg = condExpr;
-    QualType condType = condExpr->getType();
-
-    if (getLangOptions().CPlusPlus) {
-      if (CheckCXXBooleanCondition(condExpr)) // C++ 6.4p4
-        return StmtError();
-    } else if (!condType->isScalarType()) // C99 6.8.5p2
-      return StmtError(Diag(WhileLoc,
-                            diag::err_typecheck_statement_requires_scalar)
-                       << condType << condExpr->getSourceRange());
+    return StmtError();
   }
 
   Stmt *bodyStmt = Body.takeAs<Stmt>();
@@ -605,18 +585,9 @@ Sema::ActOnDoStmt(SourceLocation DoLoc, StmtArg Body,
   Expr *condExpr = Cond.takeAs<Expr>();
   assert(condExpr && "ActOnDoStmt(): missing expression");
 
-  if (!condExpr->isTypeDependent()) {
-    DefaultFunctionArrayConversion(condExpr);
+  if (CheckBooleanCondition(condExpr, DoLoc)) {
     Cond = condExpr;
-    QualType condType = condExpr->getType();
-
-    if (getLangOptions().CPlusPlus) {
-      if (CheckCXXBooleanCondition(condExpr)) // C++ 6.4p4
-        return StmtError();
-    } else if (!condType->isScalarType()) // C99 6.8.5p2
-      return StmtError(Diag(DoLoc,
-                            diag::err_typecheck_statement_requires_scalar)
-                       << condType << condExpr->getSourceRange());
+    return StmtError();
   }
 
   Stmt *bodyStmt = Body.takeAs<Stmt>();
@@ -632,7 +603,7 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
                    StmtArg first, ExprArg second, ExprArg third,
                    SourceLocation RParenLoc, StmtArg body) {
   Stmt *First  = static_cast<Stmt*>(first.get());
-  Expr *Second = static_cast<Expr*>(second.get());
+  Expr *Second = second.takeAs<Expr>();
   Expr *Third  = static_cast<Expr*>(third.get());
   Stmt *Body  = static_cast<Stmt*>(body.get());
 
@@ -652,17 +623,9 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
       }
     }
   }
-  if (Second && !Second->isTypeDependent()) {
-    DefaultFunctionArrayConversion(Second);
-    QualType SecondType = Second->getType();
-
-    if (getLangOptions().CPlusPlus) {
-      if (CheckCXXBooleanCondition(Second)) // C++ 6.4p4
-        return StmtError();
-    } else if (!SecondType->isScalarType()) // C99 6.8.5p2
-      return StmtError(Diag(ForLoc,
-                            diag::err_typecheck_statement_requires_scalar)
-        << SecondType << Second->getSourceRange());
+  if (Second && CheckBooleanCondition(Second, ForLoc)) {
+    second = Second;
+    return StmtError();
   }
 
   DiagnoseUnusedExprResult(First);
@@ -670,7 +633,6 @@ Sema::ActOnForStmt(SourceLocation ForLoc, SourceLocation LParenLoc,
   DiagnoseUnusedExprResult(Body);
 
   first.release();
-  second.release();
   third.release();
   body.release();
   return Owned(new (Context) ForStmt(First, Second, Third, Body, ForLoc,

clang/test/SemaCXX/warn-assignment-condition.cpp

@@ -0,0 +1,65 @@
+// RUN: clang-cc -fsyntax-only -Wparentheses -verify %s
+
+struct A {
+  int foo();
+  friend A operator+(const A&, const A&);
+  operator bool();
+};
+
+void test() {
+  int x, *p;
+  A a, b;
+
+  // With scalars.
+  if (x = 7) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  if ((x = 7)) {}
+  do {
+  } while (x = 7); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  do {
+  } while ((x = 7));
+  while (x = 7) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  while ((x = 7)) {}
+  for (; x = 7; ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  for (; (x = 7); ) {}
+
+  if (p = p) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  if ((p = p)) {}
+  do {
+  } while (p = p); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  do {
+  } while ((p = p));
+  while (p = p) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  while ((p = p)) {}
+  for (; p = p; ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  for (; (p = p); ) {}
+
+  // Initializing variables (shouldn't warn).
+  if (int y = x) {}
+  while (int y = x) {}
+  if (A y = a) {}
+  while (A y = a) {}
+
+  // With temporaries.
+  if (x = (b+b).foo()) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  if ((x = (b+b).foo())) {}
+  do {
+  } while (x = (b+b).foo()); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  do {
+  } while ((x = (b+b).foo()));
+  while (x = (b+b).foo()) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  while ((x = (b+b).foo())) {}
+  for (; x = (b+b).foo(); ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  for (; (x = (b+b).foo()); ) {}
+
+  // With a user-defined operator.
+  if (a = b + b) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  if ((a = b + b)) {}
+  do {
+  } while (a = b + b); // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  do {
+  } while ((a = b + b));
+  while (a = b + b) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  while ((a = b + b)) {}
+  for (; a = b + b; ) {} // expected-warning {{using the result of an assignment as a condition without parentheses}}
+  for (; (a = b + b); ) {}
+}