New warning for mismatch between not and and/or operators.

[rtrieu]

Summary

Emit a warning when a bitwise not operator is used with a logical and/or operator or a logical not with bitwise and/or is detected. Also emit fix-it hints to change the not operator and to change the and/or operator.

Examples:
int x = ~y || ~z;
Fixed with || => |

bool a = ~b && !c
Fixed with ~b => !b

No false positives found, although the impact of using a bitwise operator in place of a logical operator is lower than the other way round.

[• arthur.j.odwyer]

Test cases?

Examples:
int x = ~y | ~z;
Fixed with | => ||

The code ~y | ~z looks reasonable to me. It's equivalent to ~(y & z), a.k.a. "all the flags that aren't part of either y or z".
Did you mean to denigrate !y | !z instead? In that case, the fixit | => || would definitely be appropriate IMHO.

bool a = ~b && !c
Fixed with ~b => !b

I wouldn't expect the compiler to suggest a fixit in this case, but if it did, this code is sufficiently confusing that I personally would rather see the compiler suggest ~b => (b != -1), rather than suggesting a fixit that quietly changes the meaning of the code.

[rtrieu]

In D5765#3, @arthur.j.odwyer wrote:

Test cases?

Forgot to add the test case so the diff didn't pick it up. Test is included now.

Examples:
int x = ~y | ~z;
Fixed with | => ||

The code ~y | ~z looks reasonable to me. It's equivalent to ~(y & z), a.k.a. "all the flags that aren't part of either y or z".
Did you mean to denigrate !y | !z instead? In that case, the fixit | => || would definitely be appropriate IMHO.

Fixed the test case. I intended to go || to | in the example.

bool a = ~b && !c
Fixed with ~b => !b

I wouldn't expect the compiler to suggest a fixit in this case, but if it did, this code is sufficiently confusing that I personally would rather see the compiler suggest ~b => (b != -1), rather than suggesting a fixit that quietly changes the meaning of the code.

Both operator~ and operator! are called the not operator and the keys are right next to each other on US keyboards, so it would be easy to confuse the two. From my testing, this was a common mistake of using ! when ~ was intended in bitwise contexts.

[rtrieu]

Ping.

[rtrieu]

Update to newer base revision.

[danielmarjamaki]

I am personally a bit skeptic about this.

what does it solve to use:

a = !b || !c;

instead of:

a = !b | !c;

If it's only stylistic .. then why not let the programmer decide which way is better.

I am not saying that I am against this.. just that I am a bit skeptic.

[rtrieu]

In D5765#134133, @danielmarjamaki wrote:

I am personally a bit skeptic about this.

what does it solve to use:

a = !b || !c;

instead of:

a = !b | !c;

If it's only stylistic .. then why not let the programmer decide which way is better.

The same reason Clang suggests parentheses for possible order of operations bugs. It is easy to make such bugs and the correction helps make the code more readable. Perhaps the opposite suggestion should also be made?

a = ~b | ~c;

which does change how the code works.

I am not saying that I am against this.. just that I am a bit skeptic.

Switching from bitwise to logical operator also produced different code when compiled in at least one case. See https://llvm.org/bugs/show_bug.cgi?id=22723

include/clang/Basic/DiagnosticGroups.td

 def : DiagGroup<"missing-format-attribute">;
 def : DiagGroup<"missing-include-dirs">;
 def MissingNoreturn : DiagGroup<"missing-noreturn">;
 def MultiChar : DiagGroup<"multichar">;
 def : DiagGroup<"nested-externs">;
 def CXX11LongLong : DiagGroup<"c++11-long-long">;
 def LongLong : DiagGroup<"long-long", [CXX11LongLong]>;
 def MethodSignatures : DiagGroup<"method-signatures">;
+def MismatchedOperators : DiagGroup<"mismatched-operators">;
 def MismatchedParameterTypes : DiagGroup<"mismatched-parameter-types">;
 def MismatchedReturnTypes : DiagGroup<"mismatched-return-types">;
 def MismatchedTags : DiagGroup<"mismatched-tags">;
 def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleConflict : DiagGroup<"module-conflict">;
 def NewlineEOF : DiagGroup<"newline-eof">;
 def NullArithmetic : DiagGroup<"null-arithmetic">;

include/clang/Basic/DiagnosticSemaKinds.td

def warn_redundant_loop_iteration : Warning<
   InGroup<LoopAnalysis>, DefaultIgnore;
 def note_loop_iteration_here : Note<"%select{decremented|incremented}0 here">;
 
 def warn_duplicate_enum_values : Warning<
   "element %0 has been implicitly assigned %1 which another element has "
   "been assigned">, InGroup<DiagGroup<"duplicate-enum">>, DefaultIgnore;
 def note_duplicate_element : Note<"element %0 also has value %1">;
 
+def warn_wrong_binary_operator : Warning<
+  "using %select{bitwise|logical}0 operator when both operands are "
+  "%select{logical|bitwise}0">, InGroup<MismatchedOperators>, DefaultIgnore;
+def warn_mismatch_operators : Warning<
+  "using %select{bitwise|logical}0 %select{and|or}1 operator with "
+  "%select{logical|bitwise}0 not">, InGroup<MismatchedOperators>, DefaultIgnore;
+def note_suggest_operator : Note<
+  "did you mean to use %select{bitwise|logical}0 %select{and|or|not}1?">;
+
 // Absolute value functions
 def warn_unsigned_abs : Warning<
   "taking the absolute value of unsigned type %0 has no effect">,
   InGroup<AbsoluteValue>;
 def note_remove_abs : Note<
   "remove the call to '%0' since unsigned values cannot be negative">;
 def warn_abs_too_small : Warning<
   "absolute value function %0 given an argument of type %1 but has parameter "

lib/Sema/SemaExpr.cpp

static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
   }
 
   // Warn on overloaded shift operators and comparisons, such as:
   // cout << 5 == 4;
   if (BinaryOperator::isComparisonOp(Opc))
     DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);
 }
 
+namespace {
+
+enum OperatorKind { Other, Bitwise, Logical };
+
+// Determine if the operator is a logical or bitwise type.
+OperatorKind getOperatorKind(BinaryOperatorKind Opc) {
+  switch (Opc) {
+  default:
+    return Other;
+  case BO_Or:
+  case BO_And:
+    return Bitwise;
+  case BO_LOr:
+  case BO_LAnd:
+    return Logical;
+  }
+}
+
+// Determine if the operator is a logical or bitwise type.
+OperatorKind getOperatorKind(UnaryOperator *UO) {
+  if (!UO)
+    return Other;
+  switch (UO->getOpcode()) {
+  default:
+    return Other;
+  case UO_LNot:
+    return Logical;
+  case UO_Not:
+    return Bitwise;
+  }
+}
+
+// Switch between bitwise and logical operators.
+BinaryOperatorKind getOppositeOpcode(BinaryOperatorKind Opc) {
+  switch (Opc) {
+  default:
+    llvm_unreachable("Unexpected opcode");
+  case BO_LAnd:
+    return BO_And;
+  case BO_LOr:
+    return BO_Or;
+  case BO_And:
+    return BO_LAnd;
+  case BO_Or:
+    return BO_LOr;
+  }
+}
+
+// Switch between bitwise and logical operators.
+UnaryOperatorKind getOppositeOpcode(UnaryOperatorKind Opc) {
+  switch (Opc) {
+  default:
+    llvm_unreachable("Unexpected opcode");
+  case UO_LNot:
+    return UO_Not;
+  case UO_Not:
+    return UO_LNot;
+  }
+}
+
+static void DiagnoseIncorrectNot(Sema &Self, BinaryOperatorKind Opc,
+                                 SourceLocation OpLoc, Expr *LHSExpr,
+                                 Expr *RHSExpr) {
+  const OperatorKind BinOpKind = getOperatorKind(Opc);
+  if (BinOpKind == Other)
+    return;
+
+  UnaryOperator *LHS_UO = dyn_cast<UnaryOperator>(LHSExpr);
+  OperatorKind LHSKind = getOperatorKind(LHS_UO);
+
+  UnaryOperator *RHS_UO = dyn_cast<UnaryOperator>(RHSExpr);
+  OperatorKind RHSKind = getOperatorKind(RHS_UO);
+
+  if (RHSKind == LHSKind) {
+    // LHS and RHS have same type, possible suggest to change binary operator.
+    if (RHSKind == Other) {
+      // No not operators.
+      return;
+    }
+    if (LHSKind == BinOpKind) {
+      // Operands and binary operator are the same type.
+      return;
+    }
+
+    const BinaryOperatorKind NewOpcode = getOppositeOpcode(Opc);
+    const StringRef NewOperator = BinaryOperator::getOpcodeStr(NewOpcode);
+    const bool BinaryOrOperator = Opc == BO_Or || Opc == BO_LOr;
+
+    Self.Diag(OpLoc, diag::warn_wrong_binary_operator)
+        << (BinOpKind == Logical);
+    Self.Diag(OpLoc, diag::note_suggest_operator)
+        << (BinOpKind != Logical) << BinaryOrOperator
+        << FixItHint::CreateReplacement(SourceRange(OpLoc), NewOperator);
+    return;
+  }
+
+  if (RHSKind == Other || LHSKind == Other) {
+    // Either LHS or RHS do not use a not operator.  Possible suggest either
+    // switching the not operator or switching the binary operator.
+    OperatorKind Kind = (RHSKind != Other) ? RHSKind : LHSKind;
+    if (Kind == BinOpKind)
+      return;
+
+    UnaryOperator *NotExpr = (RHSKind != Other) ? RHS_UO : LHS_UO;
+
+    const BinaryOperatorKind NewBinOpcode = getOppositeOpcode(Opc);
+    const StringRef NewBinaryOperator =
+        BinaryOperator::getOpcodeStr(NewBinOpcode);
+    const UnaryOperatorKind NewUnaryOpcode =
+        getOppositeOpcode(NotExpr->getOpcode());
+    const StringRef NewUnaryOperator =
+        UnaryOperator::getOpcodeStr(NewUnaryOpcode);
+    const bool BinaryOrOperator = Opc == BO_Or || Opc == BO_LOr;
+
+    Self.Diag(OpLoc, diag::warn_mismatch_operators) << (BinOpKind == Logical)
+                                                    << BinaryOrOperator
+                                                    << NotExpr->getExprLoc();
+    Self.Diag(OpLoc, diag::note_suggest_operator)
+        << (BinOpKind == Logical) << BinaryOrOperator
+        << FixItHint::CreateReplacement(SourceRange(OpLoc), NewBinaryOperator);
+    Self.Diag(OpLoc, diag::note_suggest_operator)
+        << (Kind == Logical) << 2 /*not*/
+        << FixItHint::CreateReplacement(SourceRange(NotExpr->getOperatorLoc()),
+                                        NewUnaryOperator);
+    return;
+  }
+
+  if (RHSKind == BinOpKind || LHSKind == BinOpKind) {
+    // RHS and LHS are different kinds of not operators.  Suggest switching one
+    // of them to match the binary operator.
+    UnaryOperator *UO = (RHSKind != BinOpKind) ? RHS_UO : LHS_UO;
+    const bool BinaryOrOperator = Opc == BO_Or || Opc == BO_LOr;
+    const UnaryOperatorKind NewUnaryOpcode = getOppositeOpcode(UO->getOpcode());
+    const StringRef NewUnaryOperator =
+        UnaryOperator::getOpcodeStr(NewUnaryOpcode);
+    Self.Diag(OpLoc, diag::warn_mismatch_operators)
+        << (BinOpKind == Logical) << BinaryOrOperator << UO->getExprLoc();
+    Self.Diag(OpLoc, diag::note_suggest_operator)
+        << (BinOpKind == Logical) << 2 /*not*/
+        << FixItHint::CreateReplacement(SourceRange(UO->getOperatorLoc()),
+                                        NewUnaryOperator);
+    return;
+  }
+
+  llvm_unreachable("unable to process BinaryOperator");
+}
+}
+
 // Binary Operators.  'Tok' is the token for the operator.
 ExprResult Sema::ActOnBinOp(Scope *S, SourceLocation TokLoc,
                             tok::TokenKind Kind,
                             Expr *LHSExpr, Expr *RHSExpr) {
   BinaryOperatorKind Opc = ConvertTokenKindToBinaryOpcode(Kind);
   assert(LHSExpr && "ActOnBinOp(): missing left expression");
   assert(RHSExpr && "ActOnBinOp(): missing right expression");
 
   // Emit warnings for tricky precedence issues, e.g. "bitfield & 0x4 == 0"
   DiagnoseBinOpPrecedence(*this, Opc, TokLoc, LHSExpr, RHSExpr);
 
+  DiagnoseIncorrectNot(*this, Opc, TokLoc, LHSExpr, RHSExpr);
+
   return BuildBinOp(S, TokLoc, Opc, LHSExpr, RHSExpr);
 }
 
 /// Build an overloaded binary operator expression in the given scope.
 static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc,
                                        BinaryOperatorKind Opc,
                                        Expr *LHS, Expr *RHS) {
   // Find all of the overloaded operators visible from this

test/SemaCXX/warn-mismatch.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wmismatched-operators %s
+
+void test_negative(int x, int y, int z) {
+  z = x | y;
+  z = x & y;
+  z = ~x | y;
+  z = ~x & y;
+  z = x | ~y;
+  z = x & ~y;
+  z = ~x | ~y;
+  z = ~x & ~y;
+
+  z = x || y;
+  z = x && y;
+  z = !x || y;
+  z = !x && y;
+  z = x || !y;
+  z = x && !y;
+  z = !x || !y;
+  z = !x && !y;
+}
+
+void test_same_operand(int x, int y, int z) {
+  z = !x | !y;
+  // expected-warning@-1 {{using bitwise operator when both operands are logical}}
+  // expected-note@-2 {{did you mean to use logical or?}}
+  z = !x & !y;
+  // expected-warning@-1 {{using bitwise operator when both operands are logical}}
+  // expected-note@-2 {{did you mean to use logical and?}}
+  z = ~x || ~y;
+  // expected-warning@-1 {{using logical operator when both operands are bitwise}}
+  // expected-note@-2 {{did you mean to use bitwise or?}}
+  z = ~x && ~y;
+  // expected-warning@-1 {{using logical operator when both operands are bitwise}}
+  // expected-note@-2 {{did you mean to use bitwise and?}}
+}
+
+void test_different_operand(int x, int y, int z) {
+  z = !x | ~y;
+  // expected-warning@-1 {{using bitwise or operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise not?}}
+  z = !x & ~y;
+  // expected-warning@-1 {{using bitwise and operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise not?}}
+  z = !x || ~y;
+  // expected-warning@-1 {{using logical or operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical not?}}
+  z = !x && ~y;
+  // expected-warning@-1 {{using logical and operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical not?}}
+
+  z = ~x | !y;
+  // expected-warning@-1 {{using bitwise or operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise not?}}
+  z = ~x & !y;
+  // expected-warning@-1 {{using bitwise and operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise not?}}
+  z = ~x || !y;
+  // expected-warning@-1 {{using logical or operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical not?}}
+  z = ~x && !y;
+  // expected-warning@-1 {{using logical and operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical not?}}
+}
+
+void test_one_operand(int x, int y, int z) {
+  z = !x | y;
+  // expected-warning@-1 {{using bitwise or operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise or?}}
+  // expected-note@-3 {{did you mean to use logical not?}}
+  z = !x & y;
+  // expected-warning@-1 {{using bitwise and operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise and?}}
+  // expected-note@-3 {{did you mean to use logical not?}}
+  z = ~x || y;
+  // expected-warning@-1 {{using logical or operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical or?}}
+  // expected-note@-3 {{did you mean to use bitwise not?}}
+  z = ~x && y;
+  // expected-warning@-1 {{using logical and operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical and?}}
+  // expected-note@-3 {{did you mean to use bitwise not?}}
+
+  z = x | !y;
+  // expected-warning@-1 {{using bitwise or operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise or?}}
+  // expected-note@-3 {{did you mean to use logical not?}}
+  z = x & !y;
+  // expected-warning@-1 {{using bitwise and operator with logical not}}
+  // expected-note@-2 {{did you mean to use bitwise and?}}
+  // expected-note@-3 {{did you mean to use logical not?}}
+  z = x || ~y;
+  // expected-warning@-1 {{using logical or operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical or?}}
+  // expected-note@-3 {{did you mean to use bitwise not?}}
+  z = x && ~y;
+  // expected-warning@-1 {{using logical and operator with bitwise not}}
+  // expected-note@-2 {{did you mean to use logical and?}}
+  // expected-note@-3 {{did you mean to use bitwise not?}}
+}