[Sema] Warn on ternary comparison

[mgrabovsky]

Summary

This change adds a Sema diagnostic for comparisons like a < b < c,
which usually do not behave as intended by the author.

Fixes bug 20082.

[aaron.ballman]

Should there be an exception to this diagnostic for code involving Boolean values? e.g.,

void f(bool a, bool b, bool c) {

if (a == b == c)
  ;

}

At the very least, it seems like this should also follow GCC's behavior and suggest parenthesis directly.

include/clang/Basic/DiagnosticSemaKinds.td

5875

This diagnostic somewhat implies that ?: does not work as expected. I prefer GCC's wording for this:

"comparisons like 'X<=Y<=Z' do not have their mathematical meaning"

However, I would love it if we could do one step better and use the same operators the user wrote, if reasonable. ;-)

[mgrabovsky]

In D13643#265976, @aaron.ballman wrote:

Should there be an exception to this diagnostic for code involving Boolean values? e.g.,

void f(bool a, bool b, bool c) {

if (a == b == c)
  ;

}

At the very least, it seems like this should also follow GCC's behavior and suggest parenthesis directly.

Interesting. This might be more complex than I had predicted. I'll try to dig into GCC's code and see what they do exactly. I can't guarantee returning sane, though.

Some thoughts: Even if a, b, c are Booleans, what did the user mean by a == b == c? Do they realise that the compiler interprets it as (a == b) == c? What about 'type mismatches', i.e. if a, b are ints, while b is bool – clearly the user must be aware of the interpretation above here. What if a, b, c are all ints – C90 doesn't have Booleans, so it might be intended as either.

include/clang/Basic/DiagnosticSemaKinds.td

5875

This diagnostic somewhat implies that ?: does not work as expected.

?: is a ternary operator but it's not a comparison.

I prefer GCC's wording for this

I wasn't sure if we wanted to be copycats, but OK.

would love it if we could do one step better and use the same operators the user wrote

Yes, that would be nice.

[aaron.ballman]

In D13643#266649, @mgrabovsky wrote:

In D13643#265976, @aaron.ballman wrote:

Should there be an exception to this diagnostic for code involving Boolean values? e.g.,

void f(bool a, bool b, bool c) {

if (a == b == c)
  ;

}

At the very least, it seems like this should also follow GCC's behavior and suggest parenthesis directly.

Interesting. This might be more complex than I had predicted. I'll try to dig into GCC's code and see what they do exactly. I can't guarantee returning sane, though.

Some thoughts: Even if a, b, c are Booleans, what did the user mean by a == b == c? Do they realise that the compiler interprets it as (a == b) == c? What about 'type mismatches', i.e. if a, b are ints, while b is bool – clearly the user must be aware of the interpretation above here. What if a, b, c are all ints – C90 doesn't have Booleans, so it might be intended as either.

I would spend some time digging into how GCC handles those cases, and use that as a baseline that we can then improve upon. I like the fact that GCC basically says "use parens to clarify your intent", as that solves all of the cases regarding equality and inequality.

I would imagine type == type == bool should behave the same as bool == bool == bool; it can be valid code that just needs parens to clarify intent. As for C90 behavior, I'm not too worried about what we do there.

include/clang/Basic/DiagnosticSemaKinds.td

5875

I wasn't sure if we wanted to be copycats, but OK.

If we can improve on their wording, I am all for it. I just find their wording more clear than the proposed wording.

[mgrabovsky]

In D13643#266926, @aaron.ballman wrote:

I would spend some time digging into how GCC handles those cases, and use that as a baseline that we can then improve upon. I like the fact that GCC basically says "use parens to clarify your intent", as that solves all of the cases regarding equality and inequality.

I would imagine type == type == bool should behave the same as bool == bool == bool; it can be valid code that just needs parens to clarify intent. As for C90 behavior, I'm not too worried about what we do there.

I'm back. The warning is emitted in gcc/c-family/c-common.c L11488–11503; GCC only does this for what LLVM calls 'relational' operators (i.e., comparisons except == and !=) and for integral types.

[mgrabovsky]

Change message wording

[danielmarjamaki]

Interesting checker.

I'll test it on some debian projects.

If you're interested.. it does not currently warn about "1.0 < 2.0 < 3.0" as far as I see.

[danielmarjamaki]

I tested this on 2199 debian projects.. and got 16 warnings.

ftp://ftp.se.debian.org/debian/pool/main/t/tf5/tf5_5.0beta8.orig.tar.gz
expr.c:738:22: warning: comparisons such as 'a < b != c' do not have their mathematical meaning [-Wparentheses]
        if (neg0 == neg1 && sum<0 != neg0) {
                            ^~~~~~~~~~~~~

I guess it could be intentional. I haven't looked in the real code.

ftp://ftp.se.debian.org/debian/pool/main/m/medusa/medusa_2.1.1.orig.tar.gz
cvs.c:111:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
ftp.c:140:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
imap.c:143:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 2) )
         ^~~~~~~~~~~~~~
mysql.c:135:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
nntp.c:110:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
pcanywhere.c:133:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
pop3.c:133:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
rexec.c:95:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
rlogin.c:94:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
rsh.c:96:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
smtp.c:135:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
smtp-vrfy.c:114:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
snmp.c:154:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
telnet.c:143:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~
vmauthd.c:109:10: warning: comparisons such as 'a <= b <= c' do not have their mathematical meaning [-Wparentheses]
  if ( !(0 <= argc <= 3) )
         ^~~~~~~~~~~~~~

these are surely true positives.

[mgrabovsky]

Fix comparisons with implicit casts and mismatched types

[mgrabovsky]

In D13643#270813, @danielmarjamaki wrote:

If you're interested.. it does not currently warn about "1.0 < 2.0 < 3.0" as far as I see.

I have now fixed this for C, but the issue remains for C++ and I have absolutely no idea what's going on. Can someone knowledgeable chime in?

[mgrabovsky]

Correction for C++ tests

[danielmarjamaki]

In D13643#271885, @danielmarjamaki wrote:

I tested this on 2199 debian projects.. and got 16 warnings.

ftp://ftp.se.debian.org/debian/pool/main/t/tf5/tf5_5.0beta8.orig.tar.gz
expr.c:738:22: warning: comparisons such as 'a < b != c' do not have their mathematical meaning [-Wparentheses]
        if (neg0 == neg1 && sum<0 != neg0) {
                            ^~~~~~~~~~~~~

I guess it could be intentional. I haven't looked in the real code.
....

Do you have any comments on that one.

[mgrabovsky]

In D13643#272046, @danielmarjamaki wrote:

In D13643#271885, @danielmarjamaki wrote:

I tested this on 2199 debian projects.. and got 16 warnings.

ftp://ftp.se.debian.org/debian/pool/main/t/tf5/tf5_5.0beta8.orig.tar.gz
expr.c:738:22: warning: comparisons such as 'a < b != c' do not have their mathematical meaning [-Wparentheses]
        if (neg0 == neg1 && sum<0 != neg0) {
                            ^~~~~~~~~~~~~

I guess it could be intentional. I haven't looked in the real code.
....

Do you have any comments on that one.

In the code you posted, neg0 should be Boolean (technically, it's an int, but it should be either 0 or 1, which clang can detect using Expr::isKnownToHaveBooleanValue). Although, aaron.ballman suggested above that such code is unclear and should use parentheses. Thus this warning is de facto intentional.

[mgrabovsky]

Any other remarks?

[aaron.ballman]

Mostly looks good; the only thing left is a test to ensure the fixits are placing the parens in the expected location.

lib/Sema/SemaChecking.cpp

6725

Missing a period to end the sentence.

[rsmith]

Once this lands, we should also consider the corresponding case from a fold-expression:

template<typename ...T> bool f(T ...t) { return (t < ...); } // warn if sizeof...(T) != 2 and we use built-in operator <.
bool k = f(1.0, 2.0, 3.0);

include/clang/Basic/DiagnosticSemaKinds.td

def warn_condition_is_idiomatic_assignment : Warning<"using the result "
   InGroup<DiagGroup<"idiomatic-parentheses">>, DefaultIgnore;
 def note_condition_assign_to_comparison : Note<
   "use '==' to turn this assignment into an equality comparison">;
 def note_condition_or_assign_to_comparison : Note<
   "use '!=' to turn this compound assignment into an inequality comparison">;
 def note_condition_assign_silence : Note<
   "place parentheses around the assignment to silence this warning">;
 
+def warn_ternary_comparison : Warning<"comparisons such as 'a %0 b %1 c' do not "
+  "have their mathematical meaning">,
+  InGroup<Parentheses>;
+def note_ternary_comparison_to_conjunction : Note<"to achieve the expected behavior, "
+  "turn this expression into a conjunction of two comparisons">;
+def note_ternary_comparison_silence : Note<"place parentheses around either "
+  "of the comparisons to silence this warning">;
+
 def warn_equality_with_extra_parens : Warning<"equality comparison with "
   "extraneous parentheses">, InGroup<ParenthesesOnEquality>;
 def note_equality_comparison_to_assign : Note<
   "use '=' to turn this equality comparison into an assignment">;
 def note_equality_comparison_silence : Note<
   "remove extraneous parentheses around the comparison to silence this warning">;
 
 // assignment related diagnostics (also for argument passing, returning, etc).

lib/Sema/SemaChecking.cpp

static void DiagnoseOutOfRangeComparison(Sema &S, BinaryOperator *E,
   S.DiagRuntimeBehavior(
     E->getOperatorLoc(), E,
     S.PDiag(diag::warn_out_of_range_compare)
         << OS.str() << LiteralOrBoolConstant
         << OtherT << (OtherIsBooleanType && !OtherT->isBooleanType()) << IsTrue
         << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange());
 }
 
+/// Diagnose attempts at ternary comparison, e.g., 1 < x < 2
+static void DiagnoseTernaryComparison(Sema &S, BinaryOperator *E) {
+  BinaryOperator *LHS = dyn_cast<BinaryOperator>(E->getLHS()->IgnoreImpCasts());
+  if (!LHS || !LHS->isComparisonOp())
+    return;
+
+  SourceLocation Loc = E->getSourceRange().getBegin();
+
+  S.Diag(Loc, diag::warn_ternary_comparison)
+    << LHS->getOpcodeStr()
+    << E->getOpcodeStr()
+    << E->getSourceRange();
+  S.Diag(Loc, diag::note_ternary_comparison_to_conjunction);
+  S.Diag(Loc, diag::note_ternary_comparison_silence)
+    << FixItHint::CreateInsertion(LHS->getSourceRange().getBegin(), "(")
+    << FixItHint::CreateInsertion(LHS->getSourceRange().getEnd(), ")");
+}
+
 /// Analyze the operands of the given comparison.  Implements the
 /// fallback case from AnalyzeComparison.
 static void AnalyzeImpConvsInComparison(Sema &S, BinaryOperator *E) {
   AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
   AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
 }
 
 /// \brief Implements -Wsign-compare.
 ///
 /// \param E the binary operator to check for warnings
 static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
+  // We want to do this, no matter the types
+  DiagnoseTernaryComparison(S, E);
+
   // The type the comparison is being performed in.
   QualType T = E->getLHS()->getType();
 
   // Only analyze comparison operators where both sides have been converted to
   // the same type.
   if (!S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType()))
     return AnalyzeImpConvsInComparison(S, E);
 
   // Don't analyze value-dependent comparisons directly.
   if (E->isValueDependent())
     return AnalyzeImpConvsInComparison(S, E);
 
   Expr *LHS = E->getLHS()->IgnoreParenImpCasts();
   Expr *RHS = E->getRHS()->IgnoreParenImpCasts();
-  
   bool IsComparisonConstant = false;
-  
   // Check whether an integer constant comparison results in a value
   // of 'true' or 'false'.
   if (T->isIntegralType(S.Context)) {
     llvm::APSInt RHSValue;
     bool IsRHSIntegralLiteral = 
       RHS->isIntegerConstantExpr(RHSValue, S.Context);
     llvm::APSInt LHSValue;
     bool IsLHSIntegralLiteral = 

test/Sema/bool-compare.c

void f(int x, int y, int z) {
 
   if ((a<y) != 0) {} // no warning
   if ((a<y) != 1) {} // no warning
   if ((a<y) != 2) {} // expected-warning {{comparison of constant 2 with boolean expression is always true}}
   if ((a<y) != z) {} // no warning
   if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with boolean expression is always true}}
 
   if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // no warning
+  if (a>y<z)      {} // expected-warning {{comparisons such as 'a > b < c' do not have their mathematical meaning}} \
+                     // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                     // expected-note {{place parentheses around either of the comparisons to silence this warning}}
   if ((a<y) > z)  {} // no warning
   if((a<y)>(z<y)) {} // no warning
   if((a<y)==(z<y)){} // no warning
   if((a<y)!=(z<y)){} // no warning
   if((z==x)<(y==z)){}// no warning
   if((a<y)!=((z==x)<(y==z))){} //no warning
 
 
void f(int x, int y, int z) {
   if (-1 == (a<y)){} // expected-warning {{comparison of constant -1 with boolean expression is always false}}
 
   if (0 !=(a<y))  {} // no warning
   if (1 !=(a<y))  {} // no warning
   if (2 !=(a<y))  {} // expected-warning {{comparison of constant 2 with boolean expression is always true}}
   if (z !=(a<y))  {} // no warning
   if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with boolean expression is always true}}
 
-  if (z ==(a<y))  {}    // no warning
-  if (z<a>y)      {}        // no warning
-  if (z > (a<y))  {}    // no warning
-  if((z<y)>(a<y)) {}   // no warning
+  if (a==b==z) {} // expected-warning {{comparisons such as 'a == b == c' do not have their mathematical meaning}} \
+                  // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                  // expected-note {{place parentheses around either of the comparisons to silence this warning}}
+  if ((a==y)==z) {} // no warning
+  if (a==(y==z)) {} // no warning
+
+  if (z ==(a<y))  {}  // no warning
+  if (z<a>y)      {}  // expected-warning {{comparisons such as 'a < b > c' do not have their mathematical meaning}} \
+                      // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                      // expected-note {{place parentheses around either of the comparisons to silence this warning}}
+  if (x>y!=z) {} // expected-warning {{comparisons such as 'a > b != c' do not have their mathematical meaning}} \
+                 // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                 // expected-note {{place parentheses around either of the comparisons to silence this warning}}
+  if (1.0<2.0<3.0) {} // expected-warning {{comparisons such as 'a < b < c' do not have their mathematical meaning}} \
+                      // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                      // expected-note {{place parentheses around either of the comparisons to silence this warning}}
+
+  if (z > (a<y))  {}  // no warning
+  if((z<y)>(a<y)) {}  // no warning
   if((z<y)==(a<y)){}  // no warning
   if((z<y)!=(a<y)){}  // no warning
   if((y==z)<(z==x)){}  // no warning
   if(((z==x)<(y==z))!=(a<y)){}  // no warning
 
   if(((z==x)<(-1==z))!=(a<y)){} // no warning
   if(((z==x)<(z==-1))!=(a<y)){} // no warning
   if(((z==x)<-1)!=(a<y)){} // expected-warning {{comparison of constant -1 with boolean expression is always false}}
   if(((z==x)< 2)!=(a<y)){} // expected-warning {{comparison of constant 2 with boolean expression is always true}}
   if(((z==x)<(z>2))!=(a<y)){} // no warning
 
 }

test/SemaCXX/bool-compare.cpp

void f(int x, int y, int z) {
 
   if ((a<y) != 0) {} // no warning
   if ((a<y) != 1) {} // no warning
   if ((a<y) != 2) {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
   if ((a<y) != z) {} // no warning
   if ((a<y) != -1) {}// expected-warning {{comparison of constant -1 with expression of type 'bool' is always true}}
 
   if ((a<y) == z) {} // no warning
-  if (a>y<z)      {} // no warning
+  if (a>y<z)      {} // expected-warning {{comparisons such as 'a > b < c' do not have their mathematical meaning}} \
+                     // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                     // expected-note {{place parentheses around either of the comparisons to silence this warning}}
   if ((a<y) > z)  {} // no warning
   if((a<y)>(z<y)) {} // no warning
   if((a<y)==(z<y)){} // no warning
   if((a<y)!=(z<y)){} // no warning
   if((z==x)<(y==z)){}  // no warning
   if((a<y)!=((z==x)<(y==z))){} // no warning
 
 
void f(int x, int y, int z) {
 
   if (0 !=(a<y))  {} // no warning
   if (1 !=(a<y))  {} // no warning
   if (2 !=(a<y))  {} // expected-warning {{comparison of constant 2 with expression of type 'bool' is always true}}
   if (z !=(a<y))  {} // no warning
   if (-1 !=(a<y)) {} // expected-warning {{comparison of constant -1 with expression of type 'bool' is always true}}
 
   if (z ==(a<y))  {} // no warning
-  if (z<a>y)      {} // no warning
+  if (z<a>y)      {} // expected-warning {{comparisons such as 'a < b > c' do not have their mathematical meaning}} \
+                     // expected-note {{to achieve the expected behavior, turn this expression into a conjunction of two comparisons}} \
+                     // expected-note {{place parentheses around either of the comparisons to silence this warning}}
   if (z > (a<y))  {} // no warning
   if((z<y)>(a<y)) {} // no warning
   if((z<y)==(a<y)){} // no warning
   if((z<y)!=(a<y)){} // no warning
   if((y==z)<(z==x)){} // no warning
   if(((z==x)<(y==z))!=(a<y)){}  // no warning
 
   if(((z==x)<(-1==z))!=(a<y)){} // no warning