[clang-tools-extra] [clang-tidy] Add bugprone-chained-comparison check (PR #76365)

[Julian Schmidt via cfe-commits]

================
@@ -0,0 +1,147 @@
+//===--- ChainedComparisonCheck.cpp - clang-tidy --------------------------===//
+//
+// 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 "ChainedComparisonCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include <algorithm>
+#include <array>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+static bool isExprAComparisonOperator(const Expr *E) {
+  if (const auto *Op = dyn_cast_or_null<BinaryOperator>(E->IgnoreImplicit()))
+    return Op->isComparisonOp();
+  if (const auto *Op =
+          dyn_cast_or_null<CXXOperatorCallExpr>(E->IgnoreImplicit()))
+    return Op->isComparisonOp();
+  return false;
+}
+
+namespace {
+AST_MATCHER(BinaryOperator,
+            hasBinaryOperatorAChildComparisonOperatorWithoutParen) {
+  return isExprAComparisonOperator(Node.getLHS()) ||
+         isExprAComparisonOperator(Node.getRHS());
+}
+
+AST_MATCHER(CXXOperatorCallExpr,
+            hasCppOperatorAChildComparisonOperatorWithoutParen) {
+  return std::any_of(Node.arg_begin(), Node.arg_end(),
+                     isExprAComparisonOperator);
+}
+
+struct ChainedComparisonData {
+  llvm::SmallString<256U> Name;
+  llvm::SmallVector<const Expr *, 32U> Operands;
+
+  explicit ChainedComparisonData(const Expr *Op) { extract(Op); }
+
+private:
+  void add(const Expr *Operand);
+  void add(llvm::StringRef Opcode);
+  void extract(const Expr *Op);
+  bool extract(const BinaryOperator *Op);
+  bool extract(const CXXOperatorCallExpr *Op);
+};
+
+void ChainedComparisonData::add(const Expr *Operand) {
+  if (!Name.empty())
+    Name += ' ';
+  Name += 'v';
+  Name += std::to_string(Operands.size());
+  Operands.push_back(Operand);
+}
+
+void ChainedComparisonData::add(llvm::StringRef Opcode) {
+  Name += ' ';
+  Name += Opcode.str();
+}
+
+bool ChainedComparisonData::extract(const BinaryOperator *Op) {
+  if (!Op)
+    return false;
+
+  if (isExprAComparisonOperator(Op->getLHS()))
+    extract(Op->getLHS()->IgnoreImplicit());
+  else
+    add(Op->getLHS()->IgnoreUnlessSpelledInSource());
+
+  add(Op->getOpcodeStr());
+
+  if (isExprAComparisonOperator(Op->getRHS()))
+    extract(Op->getRHS()->IgnoreImplicit());
+  else
+    add(Op->getRHS()->IgnoreUnlessSpelledInSource());
+  return true;
+}
+
+bool ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) {
+  if (!Op || Op->getNumArgs() != 2U)
+    return false;
+
+  const Expr *FirstArg = Op->getArg(0U)->IgnoreImplicit();
+  if (isExprAComparisonOperator(FirstArg))
+    extract(FirstArg);
+  else
+    add(FirstArg->IgnoreUnlessSpelledInSource());
+
+  add(getOperatorSpelling(Op->getOperator()));
+
+  const Expr *SecondArg = Op->getArg(1U)->IgnoreImplicit();
+  if (isExprAComparisonOperator(SecondArg))
+    extract(SecondArg);
+  else
+    add(SecondArg->IgnoreUnlessSpelledInSource());
+  return true;
+}
+
+void ChainedComparisonData::extract(const Expr *Op) {
+  extract(dyn_cast_or_null<BinaryOperator>(Op)) ||
+      extract(dyn_cast_or_null<CXXOperatorCallExpr>(Op));
+}
+
+} // namespace
+
+void ChainedComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  const auto OperatorMatcher = expr(anyOf(
+      binaryOperator(isComparisonOperator(),
+                     hasBinaryOperatorAChildComparisonOperatorWithoutParen()),
+      cxxOperatorCallExpr(
+          isComparisonOperator(),
+          hasCppOperatorAChildComparisonOperatorWithoutParen())));
+
+  Finder->addMatcher(
+      expr(OperatorMatcher, unless(hasParent(OperatorMatcher))).bind("op"),
+      this);
+}
+
+void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedOperator = Result.Nodes.getNodeAs<Expr>("op");
+
+  ChainedComparisonData Data(MatchedOperator);
+  if (Data.Operands.empty())
+    return;
+
+  diag(MatchedOperator->getBeginLoc(),
+       "chained comparison '%0' may generate unintended results, use "
+       "parentheses to specify order of evaluation or a logical operator to "
+       "separate comparison expressions")
+      << llvm::StringRef(Data.Name).trim();
+
+  for (std::size_t Index = 0U; Index < Data.Operands.size(); ++Index) {
+    diag(Data.Operands[Index]->getBeginLoc(), "operand 'v%0' is here",
+         DiagnosticIDs::Note)
+        << Index;
+  }
----------------
5chmidti wrote:

What do you think about streaming the source range of the operand as well, to get the whole underline instead of just the `^` (for warning and notes)? Doing this would also allow multi-line matches to be printed as a whole, not just the first line.

https://github.com/llvm/llvm-project/pull/76365