[clang-tidy] introduce a unused local non trival variable check

[rockwotj]

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.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Tyler Rockwood (rockwotj)

Changes

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 --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; +} 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[PiotrZSL]

Simple check, not bad. My main blocker for this check is a name.

[dotnwat]

This is a very nice check for us to have @rockwotj.

[PiotrZSL]

Minor tuning needed, some changes to documentation, options, default configuration.

[PiotrZSL]

Overall looks fine.

[PiotrZSL]

LGTM.
Unless you want this to be merged by rockwotj@users.noreply.github.com please change your github privacy settings.

[rockwotj]

Changed