[clang-tidy] Add new checker for complex conditions with no meaning

[vabridgers]

Summary

This checker finds cases where relational expressions have no meaning.
For example, (x <= y <= z) has no meaning, but just happens to parse to
something deterministic. (x <= y <= z) is parsed to ((x <= y) <= z).
The (x<=y) returns a boolean value which is promoted to an integral
context, 0 (for false) or 1 (for true). So when (x <= y) the expression
becomes (1 <= z) and when (x > y) the expression becomes (0 <= z). When
asked to give all warnings, gcc warns about this. While this may be the
intention in some programming contexts, it may not have been what the
programmer really wanted in all contexts. So it's best to implement this as a
tidy check to start with, get some experience using it, then perhaps promote
this check to a diagnostic.

[vabridgers]

Correct some simple mistakes

[vabridgers]

Another update

[vabridgers]

I believe this is ready for review. Thanks!

[Eugene.Zelenko]

May be this check belongs to bugprone module?

clang-tools-extra/docs/ReleaseNotes.rst

75

Please enclose comparison operators in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/misc-complex-conditions.rst

6

Please synchronize with statement in Release Notes.

8

Please enclose statements examples and 0/1 in double back-ticks.

44

fix-it, check.

[lebedev.ri]

I really think this should be in clang proper.
I have seen such patterns written in the wild,
they should be caught by just the compiler.

[njames93]

For the record X < Y < Z does have a mathematical meaning, Y is constrained between X and Z.
However in the context of C the expression isnt parsed like that.
If someone writes this they likely wanted (X < Y) && (Y < Z)
For this specific check as you pointed out we wouldn't want to make that assumption though there is a case for adding notes to silence the warning by wrapping one of the comparisons in parenthesis.

It appears that this check fire multiple times for the case of W < X < Y < Z
Once for (W < X < Y) < Z and another time for (W < X) < Y
This again likely wont be visible as the warning currently is emitted at the start of the expression

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

28–33

The If looks suspicious, as you match on a BinaryOperator, the If will always be nullptr and should likely be removed.
Likewise Statement will match but should likely be changed to BinOp as that's what it is.

34

Elide braces

[vabridgers]

Thanks all for the prompt and actionable comments. I will address all comments directly and 1-1, but would like to bottom out on one point before driving this forward. @lebedev.ri commented this should be in the Clang front end rather than a tidy check. Can we reach a consensus on that comment before I proceed with either a tidy check renamed to bugprone (comment from @Eugene.Zelenko ) or implement this check in the front end, enabled only when all warnings are enabled? I had considered both approaches when I started this, and thought the tidy checker was the best first step.

Also, @njames93, I take your point the expression does have a mathematical meaning. I was following the gcc warning message for the same expression, but we can cast the warning however we like :) Do you have a suggestion about the warning? I like the general idea of a suggestion to use parenthesis to remove the ambiguity.

Thanks! - Vince

[vabridgers]

Address most review comments.

[vabridgers]

I believe all review comments have been address, except for the discussion on implementing this in the CFE or as a tidy check.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

75

Double back-ticks, not single ones.

clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst

6 ↗

(On Diff #281937)

Double back-ticks, not single ones.

[vabridgers]

Address backticks in rst files

[vabridgers]

Thanks Eugene, I think the backtick issue has been addressed.

[njames93]

In D84898#2184931, @vabridgers wrote:

I believe all review comments have been address, except for the discussion on implementing this in the CFE or as a tidy check.

Could try firing off an RFC to cfe-dev see if other people want to weigh in.

[vabridgers]

@njames93, I'll take a crack at implementing a cfe diagnostic for this, see how far I get. Cheers :)

[vabridgers]

Looks like this can be implemented as a warning in the cfe (as @lebedev.ri suggested). I'll probably abandon this review, but will keep it active until I have the alternative cfe warning patch prepared.

[vabridgers]

I've posted https://reviews.llvm.org/D85097 to replace this review. https://reviews.llvm.org/D85097 implements this check in the CFE instead of as a tidy check per recommendation from @lebedev.ri . If acceptable, I'll abandon this review.

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

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyMiscModule
+  ComplexConditionsCheck.cpp
   DefinitionsInHeadersCheck.cpp
   MiscTidyModule.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoRecursionCheck.cpp
   NonCopyableObjects.cpp
   NonPrivateMemberVariablesInClassesCheck.cpp
   RedundantExpressionCheck.cpp

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

+//===--- ComplexConditionsCheck.h - clang-tidy ------------------*- C++ -*-===//
+//
+// 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_COMPLEXCONDITIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPLEXCONDITIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This checker finds cases where relational expressions have no meaning.
+/// For example, (x <= y <= z) has no meaning, but just happens to parse to
+/// something deterministic. (x <= y <= z) is parsed to ((x <= y) <= z). The
+/// (x<=y) returns a boolean value which is promoted to an integral context,
+/// 0 (for false) or 1 (for true). So when (x <= y) the expression becomes
+/// (1 <= z) and when (x > y) the expression becomes (0 <= z). When asked to
+/// give all warnings, gcc warns about this. While this may be the intention
+/// in some programming contexts, it may not have been what the programmer
+/// really wanted in all contexts. So it's best to implement this as a tidy
+/// check to start with, get some experience using it, then perhaps promote
+/// this check to a diagnostic.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-complex-conditions.html
+class ComplexConditionsCheck : public ClangTidyCheck {
+public:
+  ComplexConditionsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPLEXCONDITIONSCHECK_H

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

+//===--- ComplexConditionsCheck.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 "ComplexConditionsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void ComplexConditionsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(binaryOperator(hasAnyOperatorName("<", ">", "<=", ">="),
+                                    has(binaryOperator(hasAnyOperatorName(
+                                        "<", ">", "<=", ">="))))
+                         .bind("complex_binop"),
+                     this);
+}
+
+void ComplexConditionsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *If = Result.Nodes.getNodeAs<IfStmt>("complex_binop");
+  const auto *Statement = Result.Nodes.getNodeAs<Stmt>("complex_binop");
+  if (If) {
+    diag(If->getBeginLoc(),
+         "comparisons like `X<=Y<=Z` have no mathematical meaning");
+  }
+  if (Statement) {
+    diag(Statement->getBeginLoc(),
+         "comparisons like `X<=Y<=Z` have no mathematical meaning");
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

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

 //===--- MiscTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ComplexConditionsCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoRecursionCheck.h"
 #include "NonCopyableObjects.h"
 #include "NonPrivateMemberVariablesInClassesCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ComplexConditionsCheck>(
+        "misc-complex-conditions");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");

clang-tools-extra/docs/ReleaseNotes.rst

 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 The improvements are...
 
+- New :doc:`misc-complex-conditions
+  <clang-tidy/checks/misc-complex-conditions>` check.
+
+  Finds complex conditions using <, >, <=, and >= that have no mathematical
+  meaning. 
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

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

.. csv-table::
    `llvm-include-order <llvm-include-order.html>`_, "Yes"
    `llvm-namespace-comment <llvm-namespace-comment.html>`_,
    `llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm-prefer-isa-or-dyn-cast-in-conditionals.html>`_, "Yes"
    `llvm-prefer-register-over-unsigned <llvm-prefer-register-over-unsigned.html>`_, "Yes"
    `llvm-twine-local <llvm-twine-local.html>`_, "Yes"
    `llvmlibc-callee-namespace <llvmlibc-callee-namespace.html>`_,
    `llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
    `llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
+   `misc-complex-conditions <misc-complex-conditions.html>`_,
    `misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
    `misc-misplaced-const <misc-misplaced-const.html>`_,
    `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `misc-no-recursion <misc-no-recursion.html>`_,
    `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_,
    `misc-redundant-expression <misc-redundant-expression.html>`_, "Yes"
    `misc-static-assert <misc-static-assert.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/misc-complex-conditions.rst

+.. title:: clang-tidy - misc-complex-conditions
+
+misc-complex-conditions
+=======================
+
+Detects conditions that have no mathematical meaing.
+
+It's possible to write an expression as (x <= y <= z), which has no meaning.
+This expression is parsed as ((x <= y) x<= z). A compare returns a boolean
+value when used in an integral context, is promoted to 0 (for false) or 1
+(for true). So when x <= y the expression becomes (1 <= z) and when (x > y)
+the expression becomes (0 <= z).
+
+While it's possible this is precisely what the programmer wanted, it's also
+possible this was a programming mistake, so this is appropriately flagged as
+a warning when explicitly enabled.
+
+Example cases warned include:
+
+.. code-block:: c
+
+   int a = p1 < p2 < p3;
+
+   int b = (p1 > p2) < p3 < p4;
+
+   if (p1 > p2 < p3) {
+     ...
+   } 
+
+Example cases not warned, since the ambigiuty has been removed, include:
+
+.. code-block:: c
+
+   int a = p1 < (p2 < p3);
+
+   int b = ((p1 > p2) < p3) < p4;
+
+   int c = ((p1 > p2) < (p3 < p4);
+
+   if (p1 > (p2 < p3)) {
+     ...
+   }
+
+This check does not include a fixit since it's not reasonable for the checker
+to guess at the programmer's intention.
+

clang-tools-extra/test/clang-tidy/checkers/misc-complex-conditions.c

+// RUN: %check_clang_tidy %s misc-complex-conditions %t
+
+int case1(int p1, int p2, int p3) {
+  if (p1 < p2 < p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case2(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2 < p3))
+    return 1;
+  return 0;
+}
+
+int case3(int p1, int p2, int p3) {
+  // no warning
+  if ((p1 < p2) && (p2))
+    return 1;
+  return 0;
+}
+
+int case4(int p1, int p2, int p3) {
+  // no warning
+  if ((p1) && (p3 < p2))
+    return 1;
+  return 0;
+}
+
+int case5(int p1, int p2, int p3) {
+  while (p1 < p3 < p2)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case6(int p1, int p2, int p3) {
+  // should not warn
+  while (p1 && p3 < p2)
+    return 1;
+  return 0;
+}
+
+int case7(int p1, int p2, int p3) {
+  // should not warn
+  while ((p1 < p3) < p2)
+    return 1;
+  return 0;
+}
+
+int case8(int p1, int p2, int p3) {
+  int ret = p1 < p2 < p3;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+  return ret;
+}
+
+int case9(int p1, int p2, int p3) {
+  int ret = (p1 < p2) < p3 < p1;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+  return ret;
+}
+
+int case10(int p1, int p2, int p3) {
+  if (p1 <= p2 < p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case11(int p1, int p2, int p3) {
+  if (p1 < p2 <= p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case12(int p1, int p2, int p3) {
+  if (p1 <= p2 <= p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case13(int p1, int p2, int p3) {
+  if (p1 > p2 < p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case14(int p1, int p2, int p3) {
+  if (p1 > p2 > p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case15(int p1, int p2, int p3) {
+  if (p1 >= p2 > p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case16(int p1, int p2, int p3) {
+  if (p1 >= p2 >= p3)
+    return 1;
+  return 0;
+  // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [misc-complex-conditions]
+}
+
+int case17(int p1, int p2, int p3) {
+  // should not warn
+  if (p1 >= p2 || p3)
+    return 1;
+  return 0;
+}