[clang-tools-extra] [clang-tidy] introduce a must use check (PR #76101)
Tyler Rockwood via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 21 09:21:42 PST 2023
https://github.com/rockwotj updated https://github.com/llvm/llvm-project/pull/76101
>From 07158b0396be8eb1d551bc98b538c5d23c42d9e9 Mon Sep 17 00:00:00 2001
From: Tyler Rockwood <rockwood at redpanda.com>
Date: Wed, 20 Dec 2023 15:05:48 -0600
Subject: [PATCH] clang-tidy/misc: introduce a must use check
Introduce a new (off by default) clang tidy check to ensure that
variables of a specific type are always used even if -Wunused-variables
wouldn't generate a warning.
This check has already caught a couple of different bugs on the codebase
I work on, where not handling a future means that lifetimes may not be
kept alive properly as an async chunk of code may run after a class has
been destroyed, etc.
I would like to upstream it because I believe there could be other
applications of this check that would be useful in different contexts.
The check itself is quite simple.
Signed-off-by: Tyler Rockwood <rockwood at redpanda.com>
---
.../clang-tidy/misc/CMakeLists.txt | 1 +
.../clang-tidy/misc/MiscTidyModule.cpp | 2 +
.../clang-tidy/misc/MustUseCheck.cpp | 49 +++++++++++++++++++
.../clang-tidy/misc/MustUseCheck.h | 40 +++++++++++++++
clang-tools-extra/docs/ReleaseNotes.rst | 7 +++
.../docs/clang-tidy/checks/list.rst | 1 +
.../docs/clang-tidy/checks/misc/must-use.rst | 28 +++++++++++
.../clang-tidy/checkers/misc/must-use.cpp | 48 ++++++++++++++++++
8 files changed, 176 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/misc/MustUseCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/misc/MustUseCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc/must-use.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/misc/must-use.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index d9ec268650c053..374b4fc049c452 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -27,6 +27,7 @@ add_clang_library(clangTidyMiscModule
MisleadingBidirectional.cpp
MisleadingIdentifier.cpp
MisplacedConstCheck.cpp
+ MustUseCheck.cpp
NewDeleteOverloadsCheck.cpp
NoRecursionCheck.cpp
NonCopyableObjects.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index d8a88324ee63e0..651c859c153755 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -18,6 +18,7 @@
#include "MisleadingBidirectional.h"
#include "MisleadingIdentifier.h"
#include "MisplacedConstCheck.h"
+#include "MustUseCheck.h"
#include "NewDeleteOverloadsCheck.h"
#include "NoRecursionCheck.h"
#include "NonCopyableObjects.h"
@@ -54,6 +55,7 @@ class MiscModule : public ClangTidyModule {
CheckFactories.registerCheck<MisleadingIdentifierCheck>(
"misc-misleading-identifier");
CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
+ CheckFactories.registerCheck<MustUseCheck>("misc-must-use");
CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
"misc-new-delete-overloads");
CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
diff --git a/clang-tools-extra/clang-tidy/misc/MustUseCheck.cpp b/clang-tools-extra/clang-tidy/misc/MustUseCheck.cpp
new file mode 100644
index 00000000000000..31870994e4a9f8
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/MustUseCheck.cpp
@@ -0,0 +1,49 @@
+//===--- MustUseCheck.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 "MustUseCheck.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+namespace {
+AST_MATCHER(VarDecl, isLocalVar) { return Node.isLocalVarDecl(); }
+AST_MATCHER(VarDecl, isReferenced) { return Node.isReferenced(); }
+} // namespace
+
+MustUseCheck::MustUseCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ Types(utils::options::parseStringList(Options.get("Types", ""))) {}
+
+void MustUseCheck::registerMatchers(MatchFinder *Finder) {
+ if (Types.empty()) {
+ return;
+ }
+ Finder->addMatcher(
+ varDecl(isLocalVar(), unless(isReferenced()),
+ hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+ recordDecl(matchers::matchesAnyListedName(Types)))))))
+ .bind("var"),
+ this);
+}
+
+void MustUseCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedDecl = Result.Nodes.getNodeAs<VarDecl>("var");
+ diag(MatchedDecl->getLocation(), "variable %0 must be used") << MatchedDecl;
+}
+
+void MustUseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "Types", utils::options::serializeStringList(Types));
+}
+
+} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/misc/MustUseCheck.h b/clang-tools-extra/clang-tidy/misc/MustUseCheck.h
new file mode 100644
index 00000000000000..5ac61295975c69
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/MustUseCheck.h
@@ -0,0 +1,40 @@
+//===--- MustUseCheck.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_MUSTUSECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MUSTUSECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::misc {
+
+/// Warns when not using a variable of a specific type within a function. This
+/// enforces a stronger check than clang's unused-variable warnings, as in C++
+/// this warning is not fired if the class has a custom destructor, or in
+/// templates. This check allows re-enabling unused variable warnings in all
+/// situations for specific classes.
+///
+/// The check supports this option:
+/// - 'Types': a semicolon-separated list of types to ensure must be used.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc/must-use.html
+class MustUseCheck : public ClangTidyCheck {
+public:
+ MustUseCheck(StringRef Name, ClangTidyContext *Context);
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const std::vector<StringRef> Types;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MUSTUSECHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6d91748e4cef18..d86b0394eb9413 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -187,6 +187,13 @@ New checks
points in a coroutine. Such hostile types include scoped-lockable types and
types belonging to a configurable denylist.
+- New :doc:`misc-must-use
+ <clang-tidy/checks/misc/must-use>` check.
+
+ Allow strictly enforcing that variables are used for specific classes,
+ even with they would not be normally warned using `-Wunused-variable` due
+ templates or custom destructors.
+
- New :doc:`modernize-use-constraints
<clang-tidy/checks/modernize/use-constraints>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 31f0e090db1d7d..b68c09842d9686 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -250,6 +250,7 @@ Clang-Tidy Checks
:doc:`misc-misleading-bidirectional <misc/misleading-bidirectional>`,
:doc:`misc-misleading-identifier <misc/misleading-identifier>`,
:doc:`misc-misplaced-const <misc/misplaced-const>`,
+ :doc:`misc-must-use <misc/must-use>`,
:doc:`misc-new-delete-overloads <misc/new-delete-overloads>`,
:doc:`misc-no-recursion <misc/no-recursion>`,
:doc:`misc-non-copyable-objects <misc/non-copyable-objects>`,
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/must-use.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/must-use.rst
new file mode 100644
index 00000000000000..c9db406437ddfc
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/must-use.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - misc-must-use
+
+misc-must-use
+=============
+
+Allow strictly enforcing that variables are used for specific classes,
+even with they would not be normally warned using `-Wunused-variable` due
+templates or custom destructors.
+
+In the following example, `future2` normally would not trigger any unused variable checks,
+but using `{key: "misc-must-use.Types", value: "std::future"}` would cause `future2` to have
+a warning triggered that it is unused.
+
+.. code-block:: c++
+
+ std::future<MyObject> future1;
+ std::future<MyObject> future2;
+ // ...
+ MyObject foo = future1.get();
+ // future2 is not used.
+
+Options
+-------
+
+.. option:: Types
+
+ Semicolon-separated list of fully qualified names of classes to check.
+ By default it is an empty list, which disables the check.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/must-use.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/must-use.cpp
new file mode 100644
index 00000000000000..436d768a0e606a
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/must-use.cpp
@@ -0,0 +1,48 @@
+// RUN: %check_clang_tidy %s misc-must-use %t -- \
+// RUN: -config="{CheckOptions: [{key: 'misc-must-use.Types', value: '::async::Future'}]}"
+
+namespace async {
+template<typename T>
+class Future {
+public:
+ T get() {
+ return Pending;
+ }
+private:
+ T Pending;
+};
+
+
+} // namespace async
+
+// Warning is still emitted if there are type aliases.
+namespace a {
+template<typename T>
+using Future = async::Future<T>;
+} // namespace a
+
+void releaseUnits();
+struct Units {
+ ~Units() {
+ releaseUnits();
+ }
+};
+a::Future<Units> acquireUnits();
+
+template<typename T>
+T qux(T Generic) {
+ async::Future<Units> PendingA = acquireUnits();
+ auto PendingB = acquireUnits();
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: variable 'PendingB' must be used [misc-must-use]
+ PendingA.get();
+ return Generic;
+}
+
+int bar(int Num) {
+ a::Future<Units> PendingA = acquireUnits();
+ a::Future<Units> PendingB = acquireUnits(); // not used at all, unused variable not fired because of destructor side effect
+ // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: variable 'PendingB' must be used [misc-must-use]
+ auto Num2 = PendingA.get();
+ auto Num3 = qux(Num);
+ return Num * Num3;
+}
More information about the cfe-commits
mailing list