[clang-tools-extra] clang-tidy: Add bugprone-eval-order (PR #78028)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 13 03:51:51 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tidy
Author: None (maflcko)
<details>
<summary>Changes</summary>
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
---
Full diff: https://github.com/llvm/llvm-project/pull/78028.diff
8 Files Affected:
- (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+2)
- (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1)
- (added) clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.cpp (+59)
- (added) clang-tools-extra/clang-tidy/bugprone/EvalOrderCheck.h (+32)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
- (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/eval-order.rst (+40)
- (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
- (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/eval-order.cpp (+46)
``````````diff
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());
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/78028
More information about the cfe-commits
mailing list