Add clang-tidy check readability-math-missing-parentheses

[11happy]

Overview:
This pull request fixes #80850 where author suggests adding a readability check to detect missing parentheses around mathematical expressions when operators of different priorities are used.

Testing:

• Tested the updated code.
• Verified that other functionalities remain unaffected.
  

Dependencies:

• No dependencies on other pull requests.

CC:

• @PiotrZSL

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Bhuminjay Soni (11happy)

Changes

Overview:
This pull request fixes #80850 where author suggests adding a readability check to detect missing parentheses around mathematical expressions when operators of different priorities are used.

Testing:

• Tested the updated code.
• Verified that other functionalities remain unaffected.
  

Dependencies:

• No dependencies on other pull requests.

CC:

• @PiotrZSL

---

Full diff: https://github.com/llvm/llvm-project/pull/84481.diff

8 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/CMakeLists.txt (+1)
• (added) clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp (+167)
• (added) clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.h (+31)
• (modified) clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp (+3)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
• (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
• (added) clang-tools-extra/docs/clang-tidy/checks/readability/math-missing-parentheses.rst (+19)
• (added) clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp (+42)

diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
index a6c8cbd8eb448a..0d4fa095501dfb 100644
--- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -27,6 +27,7 @@ add_clang_library(clangTidyReadabilityModule
   IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
   MakeMemberFunctionConstCheck.cpp
+  MathMissingParenthesesCheck.cpp
   MisleadingIndentationCheck.cpp
   MisplacedArrayIndexCheck.cpp
   NamedParameterCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp b/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp
new file mode 100644
index 00000000000000..d9574a9fb7a476
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.cpp
@@ -0,0 +1,167 @@
+//===--- MathMissingParenthesesCheck.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 "MathMissingParenthesesCheck.h"
+#include "../utils/ASTUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Preprocessor.h"
+#include <set>
+#include <stack>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::readability {
+
+void MathMissingParenthesesCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(binaryOperator(unless(hasParent(binaryOperator())),
+                                    hasDescendant(binaryOperator()))
+                         .bind("binOp"),
+                     this);
+}
+static int precedenceCheck(const char op) {
+  if (op == '/' || op == '*' || op == '%')
+    return 5;
+
+  else if (op == '+' || op == '-')
+    return 4;
+
+  else if (op == '&')
+    return 3;
+  else if (op == '^')
+    return 2;
+
+  else if (op == '|')
+    return 1;
+
+  else
+    return 0;
+}
+static bool isOperand(const char c) {
+  if (c >= 'a' && c <= 'z')
+    return true;
+  else if (c >= 'A' && c <= 'Z')
+    return true;
+  else if (c >= '0' && c <= '9')
+    return true;
+  else if (c == '$')
+    return true;
+  else
+    return false;
+}
+static bool conditionForNegative(const std::string s, int i,
+                                 const std::string CurStr) {
+  if (CurStr[0] == '-') {
+    if (i == 0) {
+      return true;
+    } else {
+      while (s[i - 1] == ' ') {
+        i--;
+      }
+      if (!isOperand(s[i - 1])) {
+        return true;
+      } else {
+        return false;
+      }
+    }
+  } else {
+    return false;
+  }
+}
+static std::string getOperationOrder(std::string s, std::set<char> &Operators) {
+  std::stack<std::string> StackOne;
+  std::string TempStr = "";
+  for (int i = 0; i < s.length(); i++) {
+    std::string CurStr = "";
+    CurStr += s[i];
+    if (CurStr == " ")
+      continue;
+    else {
+      if (isOperand(CurStr[0]) || conditionForNegative(s, i, CurStr)) {
+        while (i < s.length() && (isOperand(s[i]) || s[i] == '-')) {
+          if (s[i] == '-') {
+            TempStr += "$";
+          } else {
+            TempStr += CurStr;
+          }
+          i++;
+          CurStr = s[i];
+        }
+        TempStr += " ";
+      } else if (CurStr == "(") {
+        StackOne.push("(");
+      } else if (CurStr == ")") {
+        while (StackOne.top() != "(") {
+          TempStr += StackOne.top();
+          StackOne.pop();
+        }
+        StackOne.pop();
+      } else {
+        while (!StackOne.empty() && precedenceCheck(CurStr[0]) <=
+                                        precedenceCheck((StackOne.top())[0])) {
+          TempStr += StackOne.top();
+          StackOne.pop();
+        }
+        StackOne.push(CurStr);
+      }
+    }
+  }
+  while (!StackOne.empty()) {
+    TempStr += StackOne.top();
+    StackOne.pop();
+  }
+  std::stack<std::string> StackTwo;
+  for (int i = 0; i < TempStr.length(); i++) {
+    if (TempStr[i] == ' ')
+      continue;
+    else if (isOperand(TempStr[i])) {
+      std::string CurStr = "";
+      while (i < TempStr.length() && isOperand(TempStr[i])) {
+        if (TempStr[i] == '$') {
+          CurStr += "-";
+        } else {
+          CurStr += TempStr[i];
+        }
+        i++;
+      }
+      StackTwo.push(CurStr);
+    } else {
+      std::string OperandOne = StackTwo.top();
+      StackTwo.pop();
+      std::string OperandTwo = StackTwo.top();
+      StackTwo.pop();
+      Operators.insert(TempStr[i]);
+      StackTwo.push("(" + OperandTwo + " " + TempStr[i] + " " + OperandOne +
+                    ")");
+    }
+  }
+  return StackTwo.top();
+}
+void MathMissingParenthesesCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binOp");
+  if (!BinOp)
+    return;
+  clang::SourceManager &SM = *Result.SourceManager;
+  clang::LangOptions LO = Result.Context->getLangOpts();
+  clang::CharSourceRange Range =
+      clang::CharSourceRange::getTokenRange(BinOp->getSourceRange());
+  std::string Expression = clang::Lexer::getSourceText(Range, SM, LO).str();
+  std::set<char> Operators;
+  std::string FinalExpression = getOperationOrder(Expression, Operators);
+  if (Operators.size() > 1) {
+    if (FinalExpression.length() > 2) {
+      FinalExpression = FinalExpression.substr(1, FinalExpression.length() - 2);
+    }
+    diag(BinOp->getBeginLoc(),
+         "add parantheses to clarify the precedence of operations")
+        << FixItHint::CreateReplacement(Range, FinalExpression);
+  }
+}
+
+} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.h b/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.h
new file mode 100644
index 00000000000000..60b402831b45e0
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/readability/MathMissingParenthesesCheck.h
@@ -0,0 +1,31 @@
+//===--- MathMissingParenthesesCheck.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_READABILITY_MATHMISSINGPARENTHESESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MATHMISSINGPARENTHESESCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::readability {
+
+/// Checks for mathematical expressions that involve operators of different
+/// priorities.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability/math-missing-parentheses.html
+class MathMissingParenthesesCheck : public ClangTidyCheck {
+public:
+  MathMissingParenthesesCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_MATHMISSINGPARENTHESESCHECK_H
diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
index 87b299bf1ef1c5..b198153d27ee49 100644
--- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -31,6 +31,7 @@
 #include "IsolateDeclarationCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MakeMemberFunctionConstCheck.h"
+#include "MathMissingParenthesesCheck.h"
 #include "MisleadingIndentationCheck.h"
 #include "MisplacedArrayIndexCheck.h"
 #include "NamedParameterCheck.h"
@@ -101,6 +102,8 @@ class ReadabilityModule : public ClangTidyModule {
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");
+    CheckFactories.registerCheck<MathMissingParenthesesCheck>(
+        "readability-math-missing-parentheses");
     CheckFactories.registerCheck<RedundantInlineSpecifierCheck>(
         "readability-redundant-inline-specifier");
     CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fd2cba4e4f463b..61b9665ffe73ed 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,6 +100,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`readability-math-missing-parentheses
+  <clang-tidy/checks/readability/math-missing-parentheses>` check.
+
+  Checks for mathematical expressions that involve operators
+  of different priorities.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index f773e80b562e4f..0cc5c217899d4d 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -359,6 +359,7 @@ Clang-Tidy Checks
    :doc:`readability-isolate-declaration <readability/isolate-declaration>`, "Yes"
    :doc:`readability-magic-numbers <readability/magic-numbers>`,
    :doc:`readability-make-member-function-const <readability/make-member-function-const>`, "Yes"
+   :doc:`readability-math-missing-parentheses <readability/math-missing-parentheses>`, "Yes"
    :doc:`readability-misleading-indentation <readability/misleading-indentation>`,
    :doc:`readability-misplaced-array-index <readability/misplaced-array-index>`, "Yes"
    :doc:`readability-named-parameter <readability/named-parameter>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/math-missing-parentheses.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/math-missing-parentheses.rst
new file mode 100644
index 00000000000000..fb3167e8e0f8f1
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/math-missing-parentheses.rst
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - readability-math-missing-parentheses
+
+readability-math-missing-parentheses
+====================================
+
+Checks for mathematical expressions that involve operators of different priorities.
+
+Before:
+
+.. code-block:: c++
+
+  int x = 1 + 2 * 3 - 4 / 5;
+
+
+After:
+
+.. code-block:: c++
+
+  int x = (1 + (2 * 3)) - (4 / 5);
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp
new file mode 100644
index 00000000000000..54cc0d4dabbdee
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/math-missing-parentheses.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s readability-math-missing-parentheses %t
+
+// FIXME: Add something that triggers the check here.
+void f(){
+    //CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    //CHECK-FIXES: int a = 1 + (2 * 3);
+    int a = 1 + 2 * 3; 
+
+    int b = 1 + 2 + 3; // No warning
+
+    int c = 1 * 2 * 3; // No warning
+
+    //CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    //CHECK-FIXES: int d = (1 + (2 * 3)) - (4 / 5);
+    int d = 1 + 2 * 3 - 4 / 5;
+
+    //CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    //CHECK-FIXES: int e = (1 & (2 + 3)) | (4 * 5);
+    int e = 1 & 2 + 3 | 4 * 5;
+
+    //CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    //CHECK-FIXES: int f = (1 * -2) + 4;
+    int f = 1 * -2 + 4;
+
+    //CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    //CHECK-FIXES: int g = (((1 * 2) * 3) + 4) + 5;
+    int g = 1 * 2 * 3 + 4 + 5;
+
+    // CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    // CHECK-FIXES: int h = (120 & (2 + 3)) | (22 * 5);
+    int h = 120 & 2 + 3 | 22 * 5;
+
+    int i = 1 & 2 & 3; // No warning
+
+    int j = 1 | 2 | 3; // No warning
+
+    int k = 1 ^ 2 ^ 3; // No warning
+
+    // CHECK-MESSAGES: :[[@LINE+2]]:13: warning: add parantheses to clarify the precedence of operations [readability-math-missing-parentheses]
+    // CHECK-FIXES: int l = (1 + 2) ^ 3;
+    int l = 1 + 2 ^ 3;
+}

[PiotrZSL]

In short, manual code parsing is not allowed, for that e got AST already.
Sad thing is that you already wrote all those parsing functions, because this check can be implemented in 60 lines of code.
It maybe wouldn't be too bad if it would just be restricted to literals, but it's not and it shouldn't be.

[5chmidti]

This is going in the right direction.

You can omit clang:: (e.g., SourceLocation), because you already are inside the clang namespace.

[PiotrZSL]

Not bad, still few issues left, mainly with getOpcode.

[11happy]

Not bad, still few issues left, mainly with getOpcode.

Thanks for your review!
Update: I will update the PR with the requested changes after 24 as I have mid semester exams from 21-24.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[PiotrZSL]

For sure it shouldn't check Or/And, as for those there is compiler warning.

[5chmidti]

I had an idea to simplify addParantheses and check a bit, otherwise this looks good.

[PiotrZSL]

Overall, I like what I see.
Just few nits.

[5chmidti]

LGTM from my side, others are still reviewing.

Your test file contains a few trailing whitespaces that you could remove.

[11happy]

Humble reminder! @PiotrZSL

[PiotrZSL]

LGTM,
Would be good to run it on llvm or any other code base, and check what it found.

[11happy]

Hello @PiotrZSL , I ran it on simple codebases & it looks good. I havent tried it on llvm codebase. it was taking a lot of time on my pc.

[5chmidti]

I let it run for a few minutes and didn't observe any crashes (on a release build though). However, I did find an issue with macros:

int sink(int);
#define FUN(ARG) (sink(ARG))
#define FUN2(ARG) sink((ARG))
#define FUN3(ARG) sink(ARG)

void f() {
    ...

    //CHECK-MESSAGES: :[[@LINE+4]]:17: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
    //CHECK-FIXES: int r = FUN(0 + (1 * 2));
    int r = FUN(0 + 1 * 2);

    //CHECK-MESSAGES: :[[@LINE+4]]:17: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
    //CHECK-FIXES: int s = FUN2(0 + (1 * 2));
    int s = FUN2(0 + 1 * 2);

    //CHECK-MESSAGES: :[[@LINE+4]]:17: warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
    //CHECK-FIXES: int s = FUN3(0 + (1 * 2));
    int t = FUN3(0 + 1 * 2);
}

All three of these fail, because the closing parentheses is not added.

Files with issues:

• polly/lib/External/isl/isl_map.c
• /home/julian/dev/llvm/llvm-tmp/llvm/unittests/DebugInfo/MSF/MSFBuilderTest.cpp
• unittest files will generally have issues with this, because of the test macros
  • found by invoking this inside the build dir: run-clang-tidy -clang-tidy-binary /path/to/clang-tidy -checks="-*,readability-math-missing*" -quiet unittests

Checking EndLoc.isValid() reveals that the location is invalid for all of these cases (llvm::errs() << "EndLoc: " << EndLoc.isValid() << "\n";).

The documentation for getLocForEndOfToken also explicitly mentions this case for macros:

/// token where it expected something different that it received. If
/// the returned source location would not be meaningful (e.g., if
/// it points into a macro), this routine returns an invalid
/// source location.

[11happy]

hello @5chmidti I am sorry for my delayed response, was quite busy with my academic work last week, I have added this change

if (EndLoc.isInvalid())
      return;

this works fine for those macros.
Thank you

[HerrCai0907]

LGTM