Warn about missing parentheses for conditional operator.

Warn in cases such as "x + someCondition ? 42 : 0;",
where the condition expression looks arithmetic, and has
a right-hand side that looks boolean.

This (partly) addresses http://llvm.org/bugs/show_bug.cgi?id=9969

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@132565 91177308-0d34-0410-b5e6-96231b3b80d8

4 files changed, 156 insertions, 40 deletions

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 95f455f6e4..b6183cad35 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2562,6 +2562,14 @@ def note_precedence_bitwise_first : Note<
 def note_precedence_bitwise_silence : Note<
   "place parentheses around the %0 expression to silence this warning">;
 
+def warn_precedence_conditional : Warning<
+  "?: has lower precedence than %0; %0 will be evaluated first">,
+  InGroup<Parentheses>;
+def note_precedence_conditional_first : Note<
+  "place parentheses around the ?: expression to evaluate it first">;
+def note_precedence_conditional_silence : Note<
+  "place parentheses around the %0 expression to silence this warning">;
+
 def warn_logical_instead_of_bitwise : Warning<
   "use of logical %0 with constant operand; switch to bitwise %1 or "
   "remove constant">, InGroup<DiagGroup<"constant-logical-operand">>;
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 9afd5f8ccd..1006b15ff1 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -6167,6 +6167,110 @@ QualType Sema::FindCompositeObjCPointerType(ExprResult &LHS, ExprResult &RHS,
   return QualType();
 }
 
+/// SuggestParentheses - Emit a diagnostic together with a fixit hint that wraps
+/// ParenRange in parentheses.
+static void SuggestParentheses(Sema &Self, SourceLocation Loc,
+                               const PartialDiagnostic &PD,
+                               const PartialDiagnostic &FirstNote,
+                               SourceRange FirstParenRange,
+                               const PartialDiagnostic &SecondNote,
+                               SourceRange SecondParenRange) {
+  Self.Diag(Loc, PD);
+
+  if (!FirstNote.getDiagID())
+    return;
+
+  SourceLocation EndLoc = Self.PP.getLocForEndOfToken(FirstParenRange.getEnd());
+  if (!FirstParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
+    // We can't display the parentheses, so just return.
+    return;
+  }
+
+  Self.Diag(Loc, FirstNote)
+    << FixItHint::CreateInsertion(FirstParenRange.getBegin(), "(")
+    << FixItHint::CreateInsertion(EndLoc, ")");
+
+  if (!SecondNote.getDiagID())
+    return;
+
+  EndLoc = Self.PP.getLocForEndOfToken(SecondParenRange.getEnd());
+  if (!SecondParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
+    // We can't display the parentheses, so just dig the
+    // warning/error and return.
+    Self.Diag(Loc, SecondNote);
+    return;
+  }
+
+  Self.Diag(Loc, SecondNote)
+    << FixItHint::CreateInsertion(SecondParenRange.getBegin(), "(")
+    << FixItHint::CreateInsertion(EndLoc, ")");
+}
+
+static bool IsArithmeticOp(BinaryOperatorKind Opc) {
+  return Opc >= BO_Mul && Opc <= BO_Shr;
+}
+
+static bool IsLogicOp(BinaryOperatorKind Opc) {
+  return (Opc >= BO_LT && Opc <= BO_NE) || (Opc >= BO_LAnd && Opc <= BO_LOr);
+}
+
+/// DiagnoseConditionalPrecedence - Emit a warning when a conditional operator
+/// and binary operator are mixed in a way that suggests the programmer assumed
+/// the conditional operator has higher precedence, for example:
+/// "int x = a + someBinaryCondition ? 1 : 2".
+static void DiagnoseConditionalPrecedence(Sema &Self,
+                                          SourceLocation OpLoc,
+                                          Expr *cond,
+                                          Expr *lhs,
+                                          Expr *rhs) {
+  while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(cond))
+    cond = C->getSubExpr();
+
+  if (BinaryOperator *OP = dyn_cast<BinaryOperator>(cond)) {
+    if (!IsArithmeticOp(OP->getOpcode()))
+      return;
+
+    // Drill down on the RHS of the condition.
+    Expr *CondRHS = OP->getRHS();
+    while (ImplicitCastExpr *C = dyn_cast<ImplicitCastExpr>(CondRHS))
+      CondRHS = C->getSubExpr();
+    while (ParenExpr *P = dyn_cast<ParenExpr>(CondRHS))
+      CondRHS = P->getSubExpr();
+
+    bool CondRHSLooksBoolean = false;
+    if (CondRHS->getType()->isBooleanType())
+      CondRHSLooksBoolean = true;
+    else if (BinaryOperator *CondRHSOP = dyn_cast<BinaryOperator>(CondRHS))
+      CondRHSLooksBoolean = IsLogicOp(CondRHSOP->getOpcode());
+    else if (UnaryOperator *CondRHSOP = dyn_cast<UnaryOperator>(CondRHS))
+      CondRHSLooksBoolean = CondRHSOP->getOpcode() == UO_LNot;
+
+    if (CondRHSLooksBoolean) {
+      // The condition is an arithmetic binary expression, with a right-
+      // hand side that looks boolean, so warn.
+
+      PartialDiagnostic Warn = Self.PDiag(diag::warn_precedence_conditional)
+          << OP->getSourceRange()
+          << BinaryOperator::getOpcodeStr(OP->getOpcode());
+
+      PartialDiagnostic FirstNote =
+          Self.PDiag(diag::note_precedence_conditional_silence)
+          << BinaryOperator::getOpcodeStr(OP->getOpcode());
+
+      SourceRange FirstParenRange(OP->getLHS()->getLocStart(),
+                                  OP->getRHS()->getLocEnd());
+
+      PartialDiagnostic SecondNote =
+          Self.PDiag(diag::note_precedence_conditional_first);
+      SourceRange SecondParenRange(OP->getRHS()->getLocStart(),
+                                   rhs->getLocEnd());
+
+      SuggestParentheses(Self, OpLoc, Warn, FirstNote, FirstParenRange,
+                         SecondNote, SecondParenRange);
+    }
+  }
+}
+
 /// ActOnConditionalOp - Parse a ?: operation.  Note that 'LHS' may be null
 /// in the case of a the GNU conditional expr extension.
 ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
@@ -6211,6 +6315,9 @@ ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
       RHS.isInvalid())
     return ExprError();
 
+  DiagnoseConditionalPrecedence(*this, QuestionLoc, Cond.get(), LHS.get(),
+                                RHS.get());
+
   if (!commonExpr)
     return Owned(new (Context) ConditionalOperator(Cond.take(), QuestionLoc,
                                                    LHS.take(), ColonLoc, 
@@ -8735,45 +8842,6 @@ ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
                                                     CompResultTy, OpLoc));
 }
 
-/// SuggestParentheses - Emit a diagnostic together with a fixit hint that wraps
-/// ParenRange in parentheses.
-static void SuggestParentheses(Sema &Self, SourceLocation Loc,
-                               const PartialDiagnostic &PD,
-                               const PartialDiagnostic &FirstNote,
-                               SourceRange FirstParenRange,
-                               const PartialDiagnostic &SecondNote,
-                               SourceRange SecondParenRange) {
-  Self.Diag(Loc, PD);
-
-  if (!FirstNote.getDiagID())
-    return;
-
-  SourceLocation EndLoc = Self.PP.getLocForEndOfToken(FirstParenRange.getEnd());
-  if (!FirstParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
-    // We can't display the parentheses, so just return.
-    return;
-  }
-
-  Self.Diag(Loc, FirstNote)
-    << FixItHint::CreateInsertion(FirstParenRange.getBegin(), "(")
-    << FixItHint::CreateInsertion(EndLoc, ")");
-
-  if (!SecondNote.getDiagID())
-    return;
-
-  EndLoc = Self.PP.getLocForEndOfToken(SecondParenRange.getEnd());
-  if (!SecondParenRange.getEnd().isFileID() || EndLoc.isInvalid()) {
-    // We can't display the parentheses, so just dig the
-    // warning/error and return.
-    Self.Diag(Loc, SecondNote);
-    return;
-  }
-
-  Self.Diag(Loc, SecondNote)
-    << FixItHint::CreateInsertion(SecondParenRange.getBegin(), "(")
-    << FixItHint::CreateInsertion(EndLoc, ")");
-}
-
 /// DiagnoseBitwisePrecedence - Emit a warning when bitwise and comparison
 /// operators are mixed in a way that suggests that the programmer forgot that
 /// comparison operators have higher precedence. The most typical example of
diff --git a/test/Sema/parentheses.c b/test/Sema/parentheses.c
index a25ded66a6..fa3c3c26b0 100644
--- a/test/Sema/parentheses.c
+++ b/test/Sema/parentheses.c
@@ -39,6 +39,28 @@ void bitwise_rel(unsigned i) {
   (void)(0 || i && i); // no warning.
 }
 
+_Bool someConditionFunc();
+
+void conditional_op(int x, int y, _Bool b) {
+  (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{?: has lower precedence than +}} \
+                                           // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                                           // expected-note {{place parentheses around the + expression to silence this warning}}
+
+  (void)(x - b ? 1 : 2); // expected-warning {{?: has lower precedence than -}} \
+                         // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                         // expected-note {{place parentheses around the - expression to silence this warning}}
+
+  (void)(x * (x == y) ? 1 : 2); // expected-warning {{?: has lower precedence than *}} \
+                                // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                                // expected-note {{place parentheses around the * expression to silence this warning}}
+
+  (void)(x / !x ? 1 : 2); // expected-warning {{?: has lower precedence than /}} \
+                          // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                          // expected-note {{place parentheses around the / expression to silence this warning}}
+
+
+  (void)(x % 2 ? 1 : 2); // no warning
+}
+
 // RUN: %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s
 // CHECK: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]
-
diff --git a/test/Sema/parentheses.cpp b/test/Sema/parentheses.cpp
new file mode 100644
index 0000000000..ad1f399c64
--- /dev/null
+++ b/test/Sema/parentheses.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wparentheses -fixit %s -o - | %clang_cc1 -Wparentheses -Werror -
+
+bool someConditionFunc();
+
+void conditional_op(int x, int y, bool b) {
+  (void)(x + someConditionFunc() ? 1 : 2); // expected-warning {{?: has lower precedence than +}} \
+                                           // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                                           // expected-note {{place parentheses around the + expression to silence this warning}}
+
+  (void)(x - b ? 1 : 2); // expected-warning {{?: has lower precedence than -}} \
+                         // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                         // expected-note {{place parentheses around the - expression to silence this warning}}
+
+  (void)(x * (x == y) ? 1 : 2); // expected-warning {{?: has lower precedence than *}} \
+                                // expected-note {{place parentheses around the ?: expression to evaluate it first}} \
+                                // expected-note {{place parentheses around the * expression to silence this warning}}
+}