clang-tidy: Add bugprone-eval-order #78028
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tidy Author: None (maflcko) ChangesThe 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
RationaleThe check is limited to member functions, because presumably the most common issue in real code is some kind of reader object: 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:
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());
+}
|
|
I guess this check has too many false-positives (mostly due to methods returning mutable iterators or pointers), so reading from a user-supplied list of class names to check, as a config setting, would make more sense? |
| bugprone-eval-order | ||
| =================== | ||
|
|
||
| Order of evaluation is unspecified for function and constructor parameters, |
There was a problem hiding this comment.
Please synchronize with statement in Release Notes.
| 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 |
|
Closing for now, but happy to reopen. I don't see a way to implement this without a user-provided list of class names to match, in which case the check will likely not be used/discovered. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
const. A fix is not offered.[dcl.init.list]has an evaluation order and can be used to silence this check.staticmethods 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:
https://godbolt.org/z/rY5MMnPE3