llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Tyler Rockwood (rockwotj) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/76101.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/misc/CMakeLists.txt (+1) - (modified) clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp (+3) - (added) clang-tools-extra/clang-tidy/misc/MustUseCheck.cpp (+49) - (added) clang-tools-extra/clang-tidy/misc/MustUseCheck.h (+40) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+7) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/docs/clang-tidy/checks/misc/must-use.rst (+28) - (added) clang-tools-extra/test/clang-tidy/checkers/misc/must-use.cpp (+48) ``````````diff 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..9f7cf912fbc0d5 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,8 @@ 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..33b48910f46745 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. + + Add the ability to strictly enforce 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..9d5e15f2716c17 --- /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 +============= + +Allows for strictly enforce variables are used for specific classes, even with +they would not be normally warned using -Wunused-variable due to 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; +} `````````` </details> https://github.com/llvm/llvm-project/pull/76101 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits