[clang-tools-extra] clang-tidy: Add bugprone-eval-order (PR #78028)

[via cfe-commits]

https://github.com/maflcko created https://github.com/llvm/llvm-project/pull/78028

The evaluation order of function or constructor parameters is unspecified, which may be unknown to non-expert C++ developers, and can lead to bugs. Thus, add a clang-tidy check to catch presumed suspect code.

### Implementation

* The check prints a warning, when at least two parameters in a call are sourced from any method of a class, and at least one of those method calls is non-`const`. A fix is not offered.
* C++11 constructor list-initialization `[dcl.init.list]` has an evaluation order and can be used to silence this check.
* In case of false positives, the check can be suppressed, or disabled completely.
* `static` methods and global functions may also lead to issues, but they are not considered in this check, for now.

### Rationale

The check is limited to member functions, because presumably the most common issue in real code is some kind of reader object:

```cpp
struct Reader {
    char buf[4]{};
    int pos{};
    char Pop() { return buf[pos++]; }
};

int Calc(char byte1, char byte2) { return byte1 > byte2; };

int main() {
    Reader r{{1, 2, 3}};
    return Calc(r.Pop(), r.Pop());  // May return 0 or 1
}
```

https://godbolt.org/z/rY5MMnPE3

>From e2a080eece7477be512d56b8e49e64060de3b526 Mon Sep 17 00:00:00 2001
From: MarcoFalke <*~=`'#}+{/-|&$^_ at 721217.xyz>
Date: Fri, 12 Jan 2024 17:38:57 +0100
Subject: [PATCH] clang-tidy: Add bugprone-eval-order

---
 .../bugprone/BugproneTidyModule.cpp           |  2 +
 .../clang-tidy/bugprone/CMakeLists.txt        |  1 +
 .../clang-tidy/bugprone/EvalOrderCheck.cpp    | 59 +++++++++++++++++++
 .../clang-tidy/bugprone/EvalOrderCheck.h      | 32 ++++++++++
 clang-tools-extra/docs/ReleaseNotes.rst       |  5 ++
 .../clang-tidy/checks/bugprone/eval-order.rst | 40 +++++++++++++
 .../docs/clang-tidy/checks/list.rst           |  1 +
 .../checkers/bugprone/eval-order.cpp          | 46 +++++++++++++++
 8 files changed, 186 insertions(+)
 create mode 100644 clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp
 create mode 100644 clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h
 create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst
 create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp

diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 435cb1e3fbcff3..3d84b6138ad635 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -23,6 +23,7 @@
 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
 #include "EmptyCatchCheck.h"
+#include "EvalOrderCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
@@ -119,6 +120,7 @@ class BugproneModule : public ClangTidyModule {
     CheckFactories.registerCheck<EasilySwappableParametersCheck>(
         "bugprone-easily-swappable-parameters");
     CheckFactories.registerCheck<EmptyCatchCheck>("bugprone-empty-catch");
+    CheckFactories.registerCheck<EvalOrderCheck>("bugprone-eval-order");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
     CheckFactories.registerCheck<FoldInitTypeCheck>("bugprone-fold-init-type");
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 70e7fbc7ec0c14..624a4f5cd95fe4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -18,6 +18,7 @@ add_clang_library(clangTidyBugproneModule
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
   EmptyCatchCheck.cpp
+  EvalOrderCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp
new file mode 100644
index 00000000000000..e9f1c0f3c3c7d0
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp
@@ -0,0 +1,59 @@
+//===--- EvalOrderCheck.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 "EvalOrderCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+using ::clang::ast_matchers::internal::Matcher;
+
+namespace clang::tidy::bugprone {
+
+EvalOrderCheck::EvalOrderCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context) {}
+
+bool EvalOrderCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus;
+}
+
+void EvalOrderCheck::registerMatchers(MatchFinder *Finder) {
+  auto Ctor = cxxConstructExpr(unless(isListInitialization())).bind("ctor");
+  auto Fun = callExpr().bind("fun");
+  auto Mut = unless(hasDeclaration(cxxMethodDecl(isConst())));
+  auto Mc1 = hasDescendant(
+      cxxMemberCallExpr(
+          Mut, callee(cxxMethodDecl(hasParent(recordDecl().bind("rd1")))))
+          .bind("mc1"));
+  auto Mc2 = hasDescendant(
+      cxxMemberCallExpr(
+          unless(equalsBoundNode("mc1")),
+          callee(cxxMethodDecl(hasParent(recordDecl(equalsBoundNode("rd1"))))))
+          .bind("mc2"));
+  Finder->addMatcher(expr(anyOf(Ctor, Fun), allOf(Mc1, Mc2)), this);
+}
+
+void EvalOrderCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Ctor = Result.Nodes.getNodeAs<Expr>("ctor");
+  const auto *Fun = Result.Nodes.getNodeAs<Expr>("fun");
+
+  if (Ctor) {
+    diag(Ctor->getExprLoc(), "Order of evaluation of constructor "
+                             "arguments is unspecified.");
+  } else {
+    diag(Fun->getExprLoc(), "Order of evaluation of function "
+                            "arguments is unspecified. ");
+  }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h
new file mode 100644
index 00000000000000..a7e96a4afa0a4f
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h
@@ -0,0 +1,32 @@
+//===--- EvalOrderCheck.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_BUGPRONE_EVALORDERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EVALORDERCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include <vector>
+
+namespace clang::tidy::bugprone {
+
+/// Detects and suggests addressing issues with unspecified evaluation order of
+/// constructor and function parameters.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/eval-order.html
+class EvalOrderCheck : public ClangTidyCheck {
+public:
+  EvalOrderCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_EVALORDERCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3437b6cf9b59e9..9a684c18be70d0 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -149,6 +149,11 @@ New checks
   Detects equality comparison between pointer to member virtual function and
   anything other than null-pointer-constant.
 
+- New :doc:`bugprone-eval-order <clang-tidy/checks/bugprone/eval-order>` check.
+
+  Detects suspect code, which is assumed to rely on unspecified order of
+  evaluation of function and constructor parameters.
+
 - New :doc:`bugprone-inc-dec-in-conditions
   <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check.
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst
new file mode 100644
index 00000000000000..b91766365887d6
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst
@@ -0,0 +1,40 @@
+.. title:: clang-tidy - bugprone-eval-order
+
+bugprone-eval-order
+===================
+
+Order of evaluation is unspecified for function and constructor parameters,
+unless the constructor uses C++11 list-initialization.
+
+This may lead to issues in code that is written with the assumption of a
+specified evaluation order.
+
+This tidy rule will print a warning when at least two parameters in a call are
+sourced from any method of a class, and at least one of those method calls is
+non-``const``. A fix is not offered.
+
+C++11 constructor list-initialization are evaluated in order of appearance and
+can be used as a fix.
+
+In case of false positives, the check can be suppressed, or disabled completely.
+
+``static`` methods and global functions may also lead to issues, but they are
+not considered in this check, for now.
+
+The check is limited to member functions, because presumably the most common
+issue in real code is some kind of reader object:
+
+.. code-block:: c++
+
+  struct Reader {
+      char buf[4]{};
+      int pos{};
+      char Pop() { return buf[pos++]; }
+  };
+
+  int Calc(char byte1, char byte2) { return byte1 > byte2; };
+
+  int main() {
+      Reader r{{1, 2, 3}};
+      return Calc(r.Pop(), r.Pop());  // May return 0 or 1
+  }
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 2f86121ad87299..f24e3240d6d23a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -89,6 +89,7 @@ Clang-Tidy Checks
    :doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`,
    :doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`,
    :doc:`bugprone-empty-catch <bugprone/empty-catch>`,
+   :doc:`bugprone-eval-order <bugprone/eval-order>`,
    :doc:`bugprone-exception-escape <bugprone/exception-escape>`,
    :doc:`bugprone-fold-init-type <bugprone/fold-init-type>`,
    :doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp
new file mode 100644
index 00000000000000..2369b02f0d1bf2
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-eval-order %t
+
+struct Food {
+    int cnt{};
+    int i() { return ++cnt; }
+    bool b() { return ++cnt > 1; }
+    int ci() const { return cnt; }
+    int cb() const { return cnt > 1; }
+    static int I() {return {};}
+};
+
+int I() {
+    static int cnt{};
+    return ++cnt;
+}
+
+struct Cat {
+    Cat(int, bool) {}
+};
+
+void Dog(int, bool) {}
+
+int copy(int i) { return i; }
+
+void function() {
+    Food f;
+    const Food cf;
+    const Food& crf{cf};
+    Food& mutf{f};
+    // Assume const member functions are fine
+    Cat a(cf.ci(), crf.cb());
+    Cat(f.ci(), f.cb());
+    Dog(f.cb(), f.ci());
+    // Assume static and global functions are fine
+    Dog(f.I(), f.b());
+    Dog(I(), I());
+    // C++11 list-init is fine
+    Cat{f.i(), f.b()};
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: Order of evaluation of constructor arguments is unspecified.
+    Cat(f.i(), f.b());
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: Order of evaluation of function arguments is unspecified.
+    Dog(copy(mutf.i()), f.b());
+    // ... even if one is const
+    // CHECK-MESSAGES: :[[@LINE+1]]:5: warning: Order of evaluation of function arguments is unspecified.
+    Dog(f.i(), f.cb());
+}