[clang-tidy] Add misc-bool-bitwise-operation check

clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp

@@ -0,0 +1,291 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "BoolBitwiseOperationCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include <array>
+#include <optional>
+#include <utility>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+static const DynTypedNode *ignoreParensTowardsTheRoot(const DynTypedNode *N,
+                                                      ASTContext *AC) {
+  if (const auto *S = N->get<Stmt>(); isa_and_nonnull<ParenExpr>(S)) {
+    auto Parents = AC->getParents(*S);
+    // FIXME: do we need to consider all `Parents` ?
+    if (!Parents.empty())
+      return ignoreParensTowardsTheRoot(&Parents[0], AC);
+  }
+  return N;
+}
+
+static bool assignsToBoolean(const BinaryOperator *BinOp, ASTContext *AC) {
+  TraversalKindScope RAII(*AC, TK_AsIs);
+  auto Parents = AC->getParents(*BinOp);
+
+  return llvm::any_of(Parents, [&](const DynTypedNode &Parent) {
+    const auto *S = ignoreParensTowardsTheRoot(&Parent, AC)->get<Stmt>();
+    const auto *ICE = dyn_cast_if_present<ImplicitCastExpr>(S);
+    return ICE ? ICE->getType().getDesugaredType(*AC)->isBooleanType() : false;
+  });
+}
+
+static constexpr std::array<std::pair<StringRef, StringRef>, 8U>
+    OperatorsTransformation{{{"|", "||"},
+                             {"|=", "||"},
+                             {"&", "&&"},
+                             {"&=", "&&"},
+                             {"bitand", "and"},
+                             {"and_eq", "and"},
+                             {"bitor", "or"},
+                             {"or_eq", "or"}}};
+
+static StringRef translate(StringRef Value) {
+  for (const auto &[Bitwise, Logical] : OperatorsTransformation) {
+    if (Value == Bitwise)
+      return Logical;
+  }
+
+  return {};
+}
+
+static bool isBooleanBitwise(const BinaryOperator *BinOp, ASTContext *AC,
+                             std::optional<bool> &RootAssignsToBoolean) {
+  if (!BinOp)
+    return false;
+
+  if (!llvm::is_contained(llvm::make_first_range(OperatorsTransformation),
+                          BinOp->getOpcodeStr()))
+    return false;
+
+  bool IsBooleanLHS = BinOp->getLHS()
+                          ->IgnoreImpCasts()
+                          ->getType()
+                          .getDesugaredType(*AC)
+                          ->isBooleanType();
+  bool IsBooleanRHS = BinOp->getRHS()
+                          ->IgnoreImpCasts()
+                          ->getType()
+                          .getDesugaredType(*AC)
+                          ->isBooleanType();
+  if (IsBooleanLHS && IsBooleanRHS) {
+    RootAssignsToBoolean = RootAssignsToBoolean.value_or(false);
+    return true;
+  }
+  if (((IsBooleanLHS || IsBooleanRHS) && assignsToBoolean(BinOp, AC)) ||
+      RootAssignsToBoolean.value_or(false)) {
+    RootAssignsToBoolean = RootAssignsToBoolean.value_or(true);
+    return true;
+  }
+  if (BinOp->isCompoundAssignmentOp() && IsBooleanLHS) {
+    RootAssignsToBoolean = RootAssignsToBoolean.value_or(true);
+    return true;
+  }
+
+  std::optional<bool> DummyFlag = false;
+  IsBooleanLHS = IsBooleanLHS ||
+                 isBooleanBitwise(dyn_cast<BinaryOperator>(
+                                      BinOp->getLHS()->IgnoreParenImpCasts()),
+                                  AC, DummyFlag);
+  IsBooleanRHS = IsBooleanRHS ||
+                 isBooleanBitwise(dyn_cast<BinaryOperator>(
+                                      BinOp->getRHS()->IgnoreParenImpCasts()),
+                                  AC, DummyFlag);
+
+  if (IsBooleanLHS && IsBooleanRHS) {
+    RootAssignsToBoolean = RootAssignsToBoolean.value_or(false);
+    return true;
+  }
+  return false;
+}
+
+static const Expr *getAcceptableCompoundsLHS(const BinaryOperator *BinOp) {
+  assert(BinOp->isCompoundAssignmentOp());
+  const Expr *LHS = BinOp->getLHS()->IgnoreImpCasts();
+  return isa<DeclRefExpr, MemberExpr>(LHS) ? LHS : nullptr;
+}
+
+BoolBitwiseOperationCheck::BoolBitwiseOperationCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      UnsafeMode(Options.get("UnsafeMode", false)),
+      IgnoreMacros(Options.get("IgnoreMacros", false)) {}
+
+void BoolBitwiseOperationCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "UnsafeMode", UnsafeMode);
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+}
+
+void BoolBitwiseOperationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      binaryOperator(unless(isExpansionInSystemHeader()),
+                     unless(hasParent(binaryOperator())) // ignoring parenExpr
+                     )
+          .bind("binOpRoot"),
+      this);
+}
+
+void BoolBitwiseOperationCheck::emitWarningAndChangeOperatorsIfPossible(
+    const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
+    const clang::SourceManager &SM, clang::ASTContext &Ctx) {
+  auto DiagEmitter = [BinOp, this] {
+    return diag(BinOp->getOperatorLoc(),
+                "use logical operator '%0' for boolean semantics instead of "
+                "bitwise operator '%1'")
+           << translate(BinOp->getOpcodeStr()) << BinOp->getOpcodeStr();
+  };
+
+  const bool HasVolatileOperand = llvm::any_of(
+      std::array{BinOp->getLHS(), BinOp->getRHS()}, [&](const Expr *E) {
+        return E->IgnoreImpCasts()
+            ->getType()
+            .getDesugaredType(Ctx)
+            .isVolatileQualified();
+      });
+  if (HasVolatileOperand) {
+    DiagEmitter();
+    return;
+  }
+
+  const bool HasSideEffects = BinOp->getRHS()->HasSideEffects(
+      Ctx, /*IncludePossibleEffects=*/!UnsafeMode);
+  if (HasSideEffects) {
+    DiagEmitter();
+    return;
+  }
+
+  SourceLocation Loc = BinOp->getOperatorLoc();
+
+  if (Loc.isInvalid() || Loc.isMacroID()) {
+    IgnoreMacros || DiagEmitter();
+    return;
+  }
+
+  Loc = SM.getSpellingLoc(Loc);
+  if (Loc.isInvalid() || Loc.isMacroID()) {
+    IgnoreMacros || DiagEmitter();
+    return;
+  }
+
+  const CharSourceRange TokenRange = CharSourceRange::getTokenRange(Loc);
+  if (TokenRange.isInvalid()) {
+    IgnoreMacros || DiagEmitter();
+    return;
+  }
+
+  const StringRef FixSpelling =
+      translate(Lexer::getSourceText(TokenRange, SM, Ctx.getLangOpts()));
+
+  if (FixSpelling.empty()) {
+    DiagEmitter();
+    return;
+  }
+
+  FixItHint InsertEqual;
+  if (BinOp->isCompoundAssignmentOp()) {
+    const auto *LHS = getAcceptableCompoundsLHS(BinOp);
+    if (!LHS) {
+      DiagEmitter();
+      return;
+    }
+    const SourceLocation LocLHS = LHS->getEndLoc();
+    if (LocLHS.isInvalid() || LocLHS.isMacroID()) {
+      IgnoreMacros || DiagEmitter();
+      return;
+    }
+    const SourceLocation InsertLoc =
+        clang::Lexer::getLocForEndOfToken(LocLHS, 0, SM, Ctx.getLangOpts());
+    if (InsertLoc.isInvalid() || InsertLoc.isMacroID()) {
+      IgnoreMacros || DiagEmitter();
+      return;
+    }
+    auto SourceText = static_cast<std::string>(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(LHS->getSourceRange()), SM,
+        Ctx.getLangOpts()));
+    llvm::erase_if(SourceText,
+                   [](unsigned char Ch) { return std::isspace(Ch); });
+    InsertEqual = FixItHint::CreateInsertion(InsertLoc, " = " + SourceText);
+  }
+
+  auto ReplaceOperator = FixItHint::CreateReplacement(TokenRange, FixSpelling);
+
+  std::optional<BinaryOperatorKind> ParentOpcode;
+  if (ParentBinOp)
+    ParentOpcode = ParentBinOp->getOpcode();
+
+  const auto *RHS = dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreImpCasts());
+  std::optional<BinaryOperatorKind> RHSOpcode;
+  if (RHS)
+    RHSOpcode = RHS->getOpcode();
+
+  const Expr *SurroundedExpr = nullptr;
+  if ((BinOp->getOpcode() == BO_Or && ParentOpcode == BO_LAnd) ||
+      (BinOp->getOpcode() == BO_And &&
+       llvm::is_contained({BO_Xor, BO_Or}, ParentOpcode))) {
+    const Expr *Side = ParentBinOp->getLHS()->IgnoreParenImpCasts() == BinOp
+                           ? ParentBinOp->getLHS()
+                           : ParentBinOp->getRHS();
+    SurroundedExpr = Side->IgnoreImpCasts();
+    assert(SurroundedExpr->IgnoreParens() == BinOp);
+  } else if (BinOp->getOpcode() == BO_AndAssign && RHSOpcode == BO_LOr)
+    SurroundedExpr = RHS;
+
+  if (isa_and_nonnull<ParenExpr>(SurroundedExpr))
+    SurroundedExpr = nullptr;
+
+  FixItHint InsertBrace1;
+  FixItHint InsertBrace2;
+  if (SurroundedExpr) {
+    const SourceLocation InsertFirstLoc = SurroundedExpr->getBeginLoc();
+    const SourceLocation InsertSecondLoc = clang::Lexer::getLocForEndOfToken(
+        SurroundedExpr->getEndLoc(), 0, SM, Ctx.getLangOpts());
+    if (InsertFirstLoc.isInvalid() || InsertFirstLoc.isMacroID() ||
+        InsertSecondLoc.isInvalid() || InsertSecondLoc.isMacroID()) {
+      IgnoreMacros || DiagEmitter();
+      return;
+    }
+    InsertBrace1 = FixItHint::CreateInsertion(InsertFirstLoc, "(");
+    InsertBrace2 = FixItHint::CreateInsertion(InsertSecondLoc, ")");
+  }
+
+  DiagEmitter() << InsertEqual << ReplaceOperator << InsertBrace1
+                << InsertBrace2;
+}
+
+void BoolBitwiseOperationCheck::visitBinaryTreesNode(
+    const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
+    const clang::SourceManager &SM, clang::ASTContext &Ctx,
+    std::optional<bool> &RootAssignsToBoolean) {
+  if (!BinOp)
+    return;
+
+  if (isBooleanBitwise(BinOp, &Ctx, RootAssignsToBoolean))
+    emitWarningAndChangeOperatorsIfPossible(BinOp, ParentBinOp, SM, Ctx);
+
+  visitBinaryTreesNode(
+      dyn_cast<BinaryOperator>(BinOp->getLHS()->IgnoreParenImpCasts()), BinOp,
+      SM, Ctx, RootAssignsToBoolean);
+  visitBinaryTreesNode(
+      dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreParenImpCasts()), BinOp,
+      SM, Ctx, RootAssignsToBoolean);
+}
+
+void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *BinOpRoot = Result.Nodes.getNodeAs<BinaryOperator>("binOpRoot");
+  const SourceManager &SM = *Result.SourceManager;
+  ASTContext &Ctx = *Result.Context;
+  std::optional<bool> RootAssignsToBoolean = std::nullopt;
+  visitBinaryTreesNode(BinOpRoot, nullptr, SM, Ctx, RootAssignsToBoolean);
+}
+
+} // namespace clang::tidy::misc

clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.h

@@ -0,0 +1,51 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOLBITWISEOPERATIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOLBITWISEOPERATIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::misc {
+
+/// Finds potentially inefficient use of bitwise operators such as ``&``,  ``|``
+/// and their compound analogues on Boolean values where logical operators like
+/// ``&&`` and ``||`` would be more appropriate.
+///
+/// For the user-facing documentation see:
+/// https://clang.llvm.org/extra/clang-tidy/checks/misc/bool-bitwise-operation.html
+class BoolBitwiseOperationCheck : public ClangTidyCheck {
+public:
+  BoolBitwiseOperationCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus || LangOpts.C99;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+
+private:
+  void emitWarningAndChangeOperatorsIfPossible(
+      const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
+      const clang::SourceManager &SM, clang::ASTContext &Ctx);
+  void visitBinaryTreesNode(const BinaryOperator *BinOp,
+                            const BinaryOperator *ParentBinOp,
+                            const clang::SourceManager &SM,
+                            clang::ASTContext &Ctx,
+                            std::optional<bool> &RootAssignsToBoolean);
+
+  bool UnsafeMode;
+  bool IgnoreMacros;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOLBITWISEOPERATIONCHECK_H

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

@@ -18,6 +18,7 @@ add_custom_target(genconfusable DEPENDS Confusables.inc)
 set_target_properties(genconfusable PROPERTIES FOLDER "Clang Tools Extra/Sourcegenning")
 
 add_clang_library(clangTidyMiscModule STATIC
+  BoolBitwiseOperationCheck.cpp
   ConstCorrectnessCheck.cpp
   CoroutineHostileRAIICheck.cpp
   DefinitionsInHeadersCheck.cpp

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

@@ -9,6 +9,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "BoolBitwiseOperationCheck.h"
 #include "ConfusableIdentifierCheck.h"
 #include "ConstCorrectnessCheck.h"
 #include "CoroutineHostileRAIICheck.h"
@@ -40,6 +41,8 @@ namespace misc {
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<BoolBitwiseOperationCheck>(
+        "misc-bool-bitwise-operation");
     CheckFactories.registerCheck<ConfusableIdentifierCheck>(
         "misc-confusable-identifiers");
     CheckFactories.registerCheck<ConstCorrectnessCheck>(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -220,6 +220,13 @@ New checks
   Finds calls to STL library iterator algorithms that could be replaced with
   LLVM range-based algorithms from ``llvm/ADT/STLExtras.h``.
 
+- New :doc:`misc-bool-bitwise-operation
+  <clang-tidy/checks/misc/bool-bitwise-operation>` check.
+
+  Finds potentially inefficient use of bitwise operators such as ``&``,  ``|``
+  and their compound analogues on Boolean values where logical operators like
+  ``&&`` and ``||`` would be more appropriate.
+
 - New :doc:`misc-override-with-different-visibility
   <clang-tidy/checks/misc/override-with-different-visibility>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -262,6 +262,7 @@ Clang-Tidy Checks
    :doc:`llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace>`,
    :doc:`llvmlibc-inline-function-decl <llvmlibc/inline-function-decl>`, "Yes"
    :doc:`llvmlibc-restrict-system-libc-headers <llvmlibc/restrict-system-libc-headers>`, "Yes"
+   :doc:`misc-bool-bitwise-operation <misc/bool-bitwise-operation>`, "Yes"
    :doc:`misc-confusable-identifiers <misc/confusable-identifiers>`,
    :doc:`misc-const-correctness <misc/const-correctness>`, "Yes"
    :doc:`misc-coroutine-hostile-raii <misc/coroutine-hostile-raii>`,