[clang-tidy] add new check: modernize-use-scoped-lock #126434

vbvictor · 2025-02-09T19:58:33Z

Add new clang-tidy that finds uses of std::lock_guard and suggests replacing them with C++17's more flexible and safer alternative std::scoped_lock.

Here is a small description of how it works for better understanding of the code:
Two separate AST matchers are registered:

The first one matches declarations of std::lock_guard that are single in their scope (only one std::lock_guard in CompoundStmt). It's an easy case, we can emit warning right away.
The second one matches CompoundStmt's that have multiple std::lock_guard declarations, which means that we may have consecutive declarations of std::lock_guard that can be replaced by a single std::scoped_lock. In order to ensure that declarations are consecutive, we need to loop over Stmt's in CompoundStmt. Here is a small example:

{
  std::mutex m1, m2;
  std::lock(m1, m2);
  std::lock_guard<std::mutex> l1(m, std::adopt_lock); // first declaration of 'std::lock_guard'
  std::lock_guard<std::mutex> l2(m, std::adopt_lock); // second declaration of 'std::lock_guard' that can be merged with first using 'scoped_lock'
}

If there is an easier way to find consecutive declarations of std::lock_guard in AST matchers, I'm happy to learn and adjust my code.

This PR closes #107839.

llvmbot · 2025-02-09T19:59:03Z

@llvm/pr-subscribers-clang-tools-extra

Author: Baranov Victor (vbvictor)

Changes

Add new clang-tidy that finds uses of std::lock_guard and suggests replacing them with C++17's more flexible and safer alternative std::scoped_lock.

Here is a small description of how it works for better understanding of the code:
Two separate AST matchers are registered:

The first one matches declarations of std::lock_guard that are single in their scope (only one std::lock_guard in CompoundStmt). It's an easy case, we can emit warning right away.
The second one matches CompoundStmt's that have multiple std::lock_guard declarations, which means that we may have consecutive declarations of std::lock_guard that can be replaced by a single std::scoped_lock. In order to ensure that declarations are consecutive, we need to loop over Stmt's in CompoundStmt. Here is a small example:

{
  std::mutex m1, m2;
  std::lock(m1, m2);
  std::lock_guard&lt;std::mutex&gt; l1(m, std::adopt_lock); // first declaration of 'std::lock_guard'
  std::lock_guard&lt;std::mutex&gt; l2(m, std::adopt_lock); // second declaration of 'std::lock_guard' that can be merged with first using 'scoped_lock'
}

If there is an easier way to find consecutive declarations of std::lock_guard in AST matchers, I'm happy to learn and adjust my code.

This PR closes #107839.

Patch is 29.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126434.diff

9 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
(modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+3)
(added) clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp (+234)
(added) clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h (+50)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
(modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
(added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst (+81)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp (+122)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock.cpp (+290)

diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index bab1167fb15ff20..619a27b2f9bb6b2 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
@@ -42,6 +42,7 @@ add_clang_library(clangTidyModernizeModule STATIC
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseRangesCheck.cpp
+  UseScopedLockCheck.cpp
   UseStartsEndsWithCheck.cpp
   UseStdFormatCheck.cpp
   UseStdNumbersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index fc46c72982fdce8..b2d4ddd6675022a 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -43,6 +43,7 @@
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseRangesCheck.h"
+#include "UseScopedLockCheck.h"
 #include "UseStartsEndsWithCheck.h"
 #include "UseStdFormatCheck.h"
 #include "UseStdNumbersCheck.h"
@@ -80,6 +81,8 @@ class ModernizeModule : public ClangTidyModule {
     CheckFactories.registerCheck<UseIntegerSignComparisonCheck>(
         "modernize-use-integer-sign-comparison");
     CheckFactories.registerCheck<UseRangesCheck>("modernize-use-ranges");
+    CheckFactories.registerCheck<UseScopedLockCheck>(
+        "modernize-use-scoped-lock");
     CheckFactories.registerCheck<UseStartsEndsWithCheck>(
         "modernize-use-starts-ends-with");
     CheckFactories.registerCheck<UseStdFormatCheck>("modernize-use-std-format");
diff --git a/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp
new file mode 100644
index 000000000000000..af2fea5ad310e6d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp
@@ -0,0 +1,234 @@
+//===--- UseScopedLockCheck.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 "UseScopedLockCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Twine.h"
+#include <iterator>
+#include <optional>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::modernize {
+
+namespace {
+
+bool isLockGuard(const QualType &Type) {
+  if (const auto *RecordTy = Type->getAs<RecordType>()) {
+    if (const auto *RecordDecl = RecordTy->getDecl()) {
+      return RecordDecl->getQualifiedNameAsString() == "std::lock_guard";
+    }
+  }
+
+  if (const auto *TemplateSpecType =
+          Type->getAs<TemplateSpecializationType>()) {
+    if (const auto *TemplateDecl =
+            TemplateSpecType->getTemplateName().getAsTemplateDecl()) {
+      return TemplateDecl->getQualifiedNameAsString() == "std::lock_guard";
+    }
+  }
+
+  return false;
+}
+
+std::vector<const VarDecl *> getLockGuardsFromDecl(const DeclStmt *DS) {
+  std::vector<const VarDecl *> LockGuards;
+
+  for (const auto *Decl : DS->decls()) {
+    if (const auto *VD = dyn_cast<VarDecl>(Decl)) {
+      const QualType Type = VD->getType().getCanonicalType();
+      if (isLockGuard(Type)) {
+        LockGuards.push_back(VD);
+      }
+    }
+  }
+
+  return LockGuards;
+}
+
+// Scans through the statements in a block and groups consecutive
+// 'std::lock_guard' variable declarations together.
+std::vector<std::vector<const VarDecl *>>
+findLocksInCompoundStmt(const CompoundStmt *Block,
+                        const ast_matchers::MatchFinder::MatchResult &Result) {
+  // store groups of consecutive 'std::lock_guard' declarations
+  std::vector<std::vector<const VarDecl *>> LockGuardGroups;
+  std::vector<const VarDecl *> CurrentLockGuardGroup;
+
+  auto AddAndClearCurrentGroup = [&]() {
+    if (!CurrentLockGuardGroup.empty()) {
+      LockGuardGroups.push_back(CurrentLockGuardGroup);
+      CurrentLockGuardGroup.clear();
+    }
+  };
+
+  for (const auto *Stmt : Block->body()) {
+    if (const auto *DS = dyn_cast<DeclStmt>(Stmt)) {
+      std::vector<const VarDecl *> LockGuards = getLockGuardsFromDecl(DS);
+
+      if (!LockGuards.empty()) {
+        CurrentLockGuardGroup.insert(
+            CurrentLockGuardGroup.end(),
+            std::make_move_iterator(LockGuards.begin()),
+            std::make_move_iterator(LockGuards.end()));
+      }
+
+      if (LockGuards.empty()) {
+        AddAndClearCurrentGroup();
+      }
+    } else {
+      AddAndClearCurrentGroup();
+    }
+  }
+
+  AddAndClearCurrentGroup();
+
+  return LockGuardGroups;
+}
+
+// Find the exact source range of the 'lock_guard<...>' token
+std::optional<SourceRange> getLockGuardRange(const VarDecl *LockGuard,
+                                             SourceManager &SM) {
+  const auto *SourceInfo = LockGuard->getTypeSourceInfo();
+  const auto TypeLoc = SourceInfo->getTypeLoc();
+
+  const auto ElaboratedLoc = TypeLoc.getAs<ElaboratedTypeLoc>();
+  if (!ElaboratedLoc)
+    return std::nullopt;
+
+  const auto TemplateLoc =
+      ElaboratedLoc.getNamedTypeLoc().getAs<TemplateSpecializationTypeLoc>();
+  if (!TemplateLoc)
+    return std::nullopt;
+
+  const SourceLocation LockGuardBeginLoc = TemplateLoc.getTemplateNameLoc();
+  const SourceLocation LockGuardRAngleLoc = TemplateLoc.getRAngleLoc();
+
+  return SourceRange(LockGuardBeginLoc, LockGuardRAngleLoc);
+}
+
+} // namespace
+
+void UseScopedLockCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOnlyMultipleLocks", WarnOnlyMultipleLocks);
+}
+
+void UseScopedLockCheck::registerMatchers(MatchFinder *Finder) {
+  auto LockGuardType = qualType(hasDeclaration(namedDecl(
+      hasName("::std::lock_guard"),
+      anyOf(classTemplateDecl(), classTemplateSpecializationDecl()))));
+  auto LockVarDecl = varDecl(hasType(LockGuardType));
+
+  // Match CompoundStmt with only one 'std::lock_guard'
+  if (!WarnOnlyMultipleLocks) {
+    Finder->addMatcher(
+        compoundStmt(has(declStmt(has(LockVarDecl.bind("lock-decl-single")))),
+                     unless(hasDescendant(declStmt(has(varDecl(
+                         hasType(LockGuardType),
+                         unless(equalsBoundNode("lock-decl-single")))))))),
+        this);
+  }
+
+  // Match CompoundStmt with multiple 'std::lock_guard'
+  Finder->addMatcher(
+      compoundStmt(has(declStmt(has(LockVarDecl.bind("lock-decl-multiple")))),
+                   hasDescendant(declStmt(has(varDecl(
+                       hasType(LockGuardType),
+                       unless(equalsBoundNode("lock-decl-multiple")))))))
+          .bind("block-multiple"),
+      this);
+}
+
+void UseScopedLockCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Decl = Result.Nodes.getNodeAs<VarDecl>("lock-decl-single")) {
+    emitDiag(Decl, Result);
+  }
+
+  if (const auto *Compound =
+          Result.Nodes.getNodeAs<CompoundStmt>("block-multiple")) {
+    emitDiag(findLocksInCompoundStmt(Compound, Result), Result);
+  }
+}
+
+void UseScopedLockCheck::emitDiag(const VarDecl *LockGuard,
+                                  const MatchFinder::MatchResult &Result) {
+  auto Diag = diag(LockGuard->getBeginLoc(),
+                   "use 'std::scoped_lock' instead of 'std::lock_guard'");
+
+  const std::optional<SourceRange> LockGuardTypeRange =
+      getLockGuardRange(LockGuard, *Result.SourceManager);
+
+  if (!LockGuardTypeRange) {
+    return;
+  }
+
+  // Create Fix-its only if we can find the constructor call to handle
+  // 'std::lock_guard l(m, std::adopt_lock)' case
+  const auto *CtorCall = dyn_cast<CXXConstructExpr>(LockGuard->getInit());
+  if (!CtorCall) {
+    return;
+  }
+
+  switch (CtorCall->getNumArgs()) {
+  case 1:
+    Diag << FixItHint::CreateReplacement(LockGuardTypeRange.value(),
+                                         "scoped_lock");
+    return;
+  case 2:
+    const auto *CtorArgs = CtorCall->getArgs();
+
+    const Expr *MutexArg = CtorArgs[0];
+    const Expr *AdoptLockArg = CtorArgs[1];
+
+    const StringRef MutexSourceText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(MutexArg->getSourceRange()),
+        *Result.SourceManager, Result.Context->getLangOpts());
+    const StringRef AdoptLockSourceText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(AdoptLockArg->getSourceRange()),
+        *Result.SourceManager, Result.Context->getLangOpts());
+
+    Diag << FixItHint::CreateReplacement(LockGuardTypeRange.value(),
+                                         "scoped_lock")
+         << FixItHint::CreateReplacement(
+                SourceRange(CtorArgs[0]->getBeginLoc(),
+                            CtorArgs[1]->getEndLoc()),
+                (llvm::Twine(AdoptLockSourceText) + ", " + MutexSourceText)
+                    .str());
+    return;
+  }
+
+  llvm_unreachable("Invalid argument number of std::lock_guard constructor");
+}
+
+void UseScopedLockCheck::emitDiag(
+    const std::vector<std::vector<const VarDecl *>> &LockGuardGroups,
+    const MatchFinder::MatchResult &Result) {
+  for (const auto &Group : LockGuardGroups) {
+    if (Group.size() == 1 && !WarnOnlyMultipleLocks) {
+      emitDiag(Group[0], Result);
+    } else {
+      diag(Group[0]->getBeginLoc(),
+           "use single 'std::scoped_lock' instead of multiple "
+           "'std::lock_guard'");
+
+      for (size_t I = 1; I < Group.size(); ++I) {
+        diag(Group[I]->getLocation(),
+             "additional 'std::lock_guard' declared here", DiagnosticIDs::Note);
+      }
+    }
+  }
+}
+
+} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h
new file mode 100644
index 000000000000000..0e1fd8067ddd1e5
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h
@@ -0,0 +1,50 @@
+//===--- UseScopedLockCheck.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_MODERNIZE_USESCOPEDLOCKCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USESCOPEDLOCKCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Stmt.h"
+#include <optional>
+
+namespace clang::tidy::modernize {
+
+/// Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's
+/// more flexible and safer alternative ``std::scoped_lock``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-scoped-lock.html
+class UseScopedLockCheck : public ClangTidyCheck {
+public:
+  UseScopedLockCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        WarnOnlyMultipleLocks(Options.get("WarnOnlyMultipleLocks", false)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus17;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+
+private:
+  void emitDiag(const VarDecl *LockGuard,
+                const ast_matchers::MatchFinder::MatchResult &Result);
+  void emitDiag(const std::vector<std::vector<const VarDecl *>> &LockGroups,
+                const ast_matchers::MatchFinder::MatchResult &Result);
+
+  const bool WarnOnlyMultipleLocks;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USESCOPEDLOCKCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3bddeeda06e06b9..6aefbc473727634 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -91,6 +91,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`use-scoped-lock
+  <clang-tidy/checks/modernize/use-scoped-lock>` check.
+
+  Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's
+  more flexible and safer alternative ``std::scoped_lock``.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 7b9b905ef76715e..b21633459a95cd7 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -308,6 +308,7 @@ Clang-Tidy Checks
    :doc:`modernize-use-nullptr <modernize/use-nullptr>`, "Yes"
    :doc:`modernize-use-override <modernize/use-override>`, "Yes"
    :doc:`modernize-use-ranges <modernize/use-ranges>`, "Yes"
+   :doc:`modernize-use-scoped-lock <modernize/use-scoped-lock>`, "Yes"
    :doc:`modernize-use-starts-ends-with <modernize/use-starts-ends-with>`, "Yes"
    :doc:`modernize-use-std-format <modernize/use-std-format>`, "Yes"
    :doc:`modernize-use-std-numbers <modernize/use-std-numbers>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst
new file mode 100644
index 000000000000000..04b35d0081b3c55
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst
@@ -0,0 +1,81 @@
+.. title:: clang-tidy - modernize-use-scoped-lock
+
+modernize-use-scoped-lock
+=========================
+
+Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's more
+flexible and safer alternative ``std::scoped_lock``. The check will
+automatically transform only single declarations of ``std::lock_guard`` and
+emit warnings for multiple declarations of ``std::lock_guard`` that can be
+replaced with a single declaration of ``std::scoped_lock``.
+
+Examples
+--------
+
+Single ``std::lock_guard`` declaration:
+
+.. code-block:: c++
+
+  std::mutex M;
+  std::lock_guard<std::mutex> L(M);
+
+
+Transforms to:
+
+.. code-block:: c++
+
+  std::mutex m;
+  std::scoped_lock L(M);
+
+Single ``std::lock_guard`` declaration with ``std::adopt_lock``:
+
+.. code-block:: c++
+
+  std::mutex M;
+  std::lock(M);
+  std::lock_guard<std::mutex> L(M, std::adopt_lock);
+
+
+Transforms to:
+
+.. code-block:: c++
+
+  std::mutex M;
+  std::lock(M);
+  std::scoped_lock L(std::adopt_lock, M);
+
+Multiple ``std::lock_guard`` declarations only emit warnings:
+
+.. code-block:: c++
+
+  std::mutex M1, M2;
+  std::lock(M1, M2);
+  std::lock_guard Lock1(M, std::adopt_lock); // warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  std::lock_guard Lock2(M, std::adopt_lock); // note: additional 'std::lock_guard' declared here
+
+
+Limitations
+-----------
+
+The check will not emit warnings if ``std::lock_guard`` is used implicitly via
+``using``, ``typedef`` or ``template``.
+
+.. code-block:: c
+
+  template <template <typename> typename Lock>
+  void TemplatedLock() {
+    std::mutex m;
+    Lock<std::mutex> l(m); // no warning
+  }
+
+  void UsingLock() {
+    using Lock = std::lock_guard<std::mutex>;
+    std::mutex m;
+    Lock l(m); // no warning
+  }
+
+.. option:: WarnOnlyMultipleLocks
+
+  When `true`, the check will only emit warnings if the there are multiple
+  consecutive ``std::lock_guard`` declarations that can be replaced with a
+  single ``std::scoped_lock`` declaration. Default is `false`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp
new file mode 100644
index 000000000000000..6d46e43bc2be477
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s modernize-use-scoped-lock %t -- \
+// RUN:   -config="{CheckOptions: {modernize-use-scoped-lock.WarnOnlyMultipleLocks: true}}"
+
+namespace std {
+
+struct mutex {
+  void lock() {}
+  void unlock() {}
+};
+
+template<class Lockable1, class Lockable2, class... LockableN >
+void lock(Lockable1& lock1, Lockable2& lock2, LockableN&... lockn );
+
+struct adopt_lock_t { };
+std::adopt_lock_t adopt_lock {};
+
+template <typename Mutex>
+struct lock_guard {
+  lock_guard(Mutex &m) { }
+  lock_guard(Mutex &m, std::adopt_lock_t t) {}
+  lock_guard( const lock_guard& ) = delete;
+};
+
+template <typename... MutexTypes>
+struct scoped_lock {
+  scoped_lock(MutexTypes&... m) {}
+  scoped_lock(std::adopt_lock_t t, MutexTypes&... m) {}
+};
+
+} // namespace std
+
+
+void Positive() {
+  std::mutex m;
+
+  {
+    std::lock_guard<std::mutex> l1(m);
+    std::lock_guard<std::mutex> l2(m);
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+    // CHECK-MESSAGES: :[[@LINE-2]]:33: note: additional 'std::lock_guard' declared here
+  }
+
+  {
+    std::lock_guard<std::mutex> l1(m), l2(m), l3(m);
+    std::lock_guard<std::mutex> l4(m);
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+    // CHECK-MESSAGES: :[[@LINE-3]]:40: note: additional 'std::lock_guard' declared here
+    // CHECK-MESSAGES: :[[@LINE-4]]:47: note: additional 'std::lock_guard' declared here
+    // CHECK-MESSAGES: :[[@LINE-4]]:33: note: additional 'std::lock_guard' declared here
+  }
+  
+  { 
+    std::lock(m, m);
+    std::lock_guard<std::mutex> l1(m, std::adopt_lock);
+    std::lock_guard<std::mutex> l2(m, std::adopt_lock);
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+    // CHECK-MESSAGES: :[[@LINE-2]]:33: note: additional 'std::lock_guard' declared here
+  } 
+}
+
+void Negative() {
+  std::mutex m;
+  {
+    std::lock_guard<std::mutex> l(m);
+  }
+
+  {
+    std::lock_guard<std::mutex> l(m, std::adopt_lock);
+  }
+}
+
+void PositiveInsideArg(std::mutex &m1, std::mutex &m2, std::mutex &m3) {
+  std::lock_guard<std::mutex> l1(m1);
+  std::lock_guard<std::mutex> l2(m2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  // CHECK-MESSAGES: :[[@LINE-2]]:31: note: additional 'std::lock_guard' declared here
+}
+
+
+void NegativeInsideArg(std::mutex &m1, std::mutex &m2, std::mutex &m3) {
+  std::lock_guard<std::mutex> l3(m3);
+}
+
+template <typename T>
+void PositiveTemplated() {
+  std::mutex m1, m2;
+  
+  std::lock_guard<std::mutex> l1(m1);
+  std::lock_guard<std::mutex> l2(m2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  // CHECK-MESSAGES: :[[@LINE-2]]:31: note: additional 'std::lock_guard' declared here  
+}
+
+template <typename T>
+void NegativeTemplated() {
+  std::mutex m1, m2, m3;
+  std::lock_guard<std::mutex> l(m1);
+}
+
+template <typename Mutex>
+void PositiveTemplatedMutex() {
+  Mutex m1, m2;
+
+  std::lock_guard<Mutex> l1(m1);
+  std::lock_guard<Mutex> l2(m2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  // CHECK-MESSAGES: :[[@LINE-2]]:26: note: additional 'std::lock_guard' declared here
+}
+
+template <typename Mutex>
+void NegativeTemplatedMutex() {
+  Mutex m1;
+  std::lock_guard<Mutex> l(m1);
+}
+
+struct NegativeClass {
+  void Negative() {
+    std::lock_guard<std::mutex> l(m1);
+  }
+  
+  std::mutex m1;
+};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use...
[truncated]

llvmbot · 2025-02-09T19:59:05Z

@llvm/pr-subscribers-clang-tidy

Author: Baranov Victor (vbvictor)

Changes

Add new clang-tidy that finds uses of std::lock_guard and suggests replacing them with C++17's more flexible and safer alternative std::scoped_lock.

Here is a small description of how it works for better understanding of the code:
Two separate AST matchers are registered:

The first one matches declarations of std::lock_guard that are single in their scope (only one std::lock_guard in CompoundStmt). It's an easy case, we can emit warning right away.
The second one matches CompoundStmt's that have multiple std::lock_guard declarations, which means that we may have consecutive declarations of std::lock_guard that can be replaced by a single std::scoped_lock. In order to ensure that declarations are consecutive, we need to loop over Stmt's in CompoundStmt. Here is a small example:

{
  std::mutex m1, m2;
  std::lock(m1, m2);
  std::lock_guard&lt;std::mutex&gt; l1(m, std::adopt_lock); // first declaration of 'std::lock_guard'
  std::lock_guard&lt;std::mutex&gt; l2(m, std::adopt_lock); // second declaration of 'std::lock_guard' that can be merged with first using 'scoped_lock'
}

If there is an easier way to find consecutive declarations of std::lock_guard in AST matchers, I'm happy to learn and adjust my code.

This PR closes #107839.

Patch is 29.95 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126434.diff

9 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
(modified) clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp (+3)
(added) clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp (+234)
(added) clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h (+50)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+6)
(modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
(added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst (+81)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp (+122)
(added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock.cpp (+290)

diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index bab1167fb15ff20..619a27b2f9bb6b2 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
@@ -42,6 +42,7 @@ add_clang_library(clangTidyModernizeModule STATIC
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
   UseRangesCheck.cpp
+  UseScopedLockCheck.cpp
   UseStartsEndsWithCheck.cpp
   UseStdFormatCheck.cpp
   UseStdNumbersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index fc46c72982fdce8..b2d4ddd6675022a 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -43,6 +43,7 @@
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 #include "UseRangesCheck.h"
+#include "UseScopedLockCheck.h"
 #include "UseStartsEndsWithCheck.h"
 #include "UseStdFormatCheck.h"
 #include "UseStdNumbersCheck.h"
@@ -80,6 +81,8 @@ class ModernizeModule : public ClangTidyModule {
     CheckFactories.registerCheck<UseIntegerSignComparisonCheck>(
         "modernize-use-integer-sign-comparison");
     CheckFactories.registerCheck<UseRangesCheck>("modernize-use-ranges");
+    CheckFactories.registerCheck<UseScopedLockCheck>(
+        "modernize-use-scoped-lock");
     CheckFactories.registerCheck<UseStartsEndsWithCheck>(
         "modernize-use-starts-ends-with");
     CheckFactories.registerCheck<UseStdFormatCheck>("modernize-use-std-format");
diff --git a/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp
new file mode 100644
index 000000000000000..af2fea5ad310e6d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp
@@ -0,0 +1,234 @@
+//===--- UseScopedLockCheck.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 "UseScopedLockCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Twine.h"
+#include <iterator>
+#include <optional>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::modernize {
+
+namespace {
+
+bool isLockGuard(const QualType &Type) {
+  if (const auto *RecordTy = Type->getAs<RecordType>()) {
+    if (const auto *RecordDecl = RecordTy->getDecl()) {
+      return RecordDecl->getQualifiedNameAsString() == "std::lock_guard";
+    }
+  }
+
+  if (const auto *TemplateSpecType =
+          Type->getAs<TemplateSpecializationType>()) {
+    if (const auto *TemplateDecl =
+            TemplateSpecType->getTemplateName().getAsTemplateDecl()) {
+      return TemplateDecl->getQualifiedNameAsString() == "std::lock_guard";
+    }
+  }
+
+  return false;
+}
+
+std::vector<const VarDecl *> getLockGuardsFromDecl(const DeclStmt *DS) {
+  std::vector<const VarDecl *> LockGuards;
+
+  for (const auto *Decl : DS->decls()) {
+    if (const auto *VD = dyn_cast<VarDecl>(Decl)) {
+      const QualType Type = VD->getType().getCanonicalType();
+      if (isLockGuard(Type)) {
+        LockGuards.push_back(VD);
+      }
+    }
+  }
+
+  return LockGuards;
+}
+
+// Scans through the statements in a block and groups consecutive
+// 'std::lock_guard' variable declarations together.
+std::vector<std::vector<const VarDecl *>>
+findLocksInCompoundStmt(const CompoundStmt *Block,
+                        const ast_matchers::MatchFinder::MatchResult &Result) {
+  // store groups of consecutive 'std::lock_guard' declarations
+  std::vector<std::vector<const VarDecl *>> LockGuardGroups;
+  std::vector<const VarDecl *> CurrentLockGuardGroup;
+
+  auto AddAndClearCurrentGroup = [&]() {
+    if (!CurrentLockGuardGroup.empty()) {
+      LockGuardGroups.push_back(CurrentLockGuardGroup);
+      CurrentLockGuardGroup.clear();
+    }
+  };
+
+  for (const auto *Stmt : Block->body()) {
+    if (const auto *DS = dyn_cast<DeclStmt>(Stmt)) {
+      std::vector<const VarDecl *> LockGuards = getLockGuardsFromDecl(DS);
+
+      if (!LockGuards.empty()) {
+        CurrentLockGuardGroup.insert(
+            CurrentLockGuardGroup.end(),
+            std::make_move_iterator(LockGuards.begin()),
+            std::make_move_iterator(LockGuards.end()));
+      }
+
+      if (LockGuards.empty()) {
+        AddAndClearCurrentGroup();
+      }
+    } else {
+      AddAndClearCurrentGroup();
+    }
+  }
+
+  AddAndClearCurrentGroup();
+
+  return LockGuardGroups;
+}
+
+// Find the exact source range of the 'lock_guard<...>' token
+std::optional<SourceRange> getLockGuardRange(const VarDecl *LockGuard,
+                                             SourceManager &SM) {
+  const auto *SourceInfo = LockGuard->getTypeSourceInfo();
+  const auto TypeLoc = SourceInfo->getTypeLoc();
+
+  const auto ElaboratedLoc = TypeLoc.getAs<ElaboratedTypeLoc>();
+  if (!ElaboratedLoc)
+    return std::nullopt;
+
+  const auto TemplateLoc =
+      ElaboratedLoc.getNamedTypeLoc().getAs<TemplateSpecializationTypeLoc>();
+  if (!TemplateLoc)
+    return std::nullopt;
+
+  const SourceLocation LockGuardBeginLoc = TemplateLoc.getTemplateNameLoc();
+  const SourceLocation LockGuardRAngleLoc = TemplateLoc.getRAngleLoc();
+
+  return SourceRange(LockGuardBeginLoc, LockGuardRAngleLoc);
+}
+
+} // namespace
+
+void UseScopedLockCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WarnOnlyMultipleLocks", WarnOnlyMultipleLocks);
+}
+
+void UseScopedLockCheck::registerMatchers(MatchFinder *Finder) {
+  auto LockGuardType = qualType(hasDeclaration(namedDecl(
+      hasName("::std::lock_guard"),
+      anyOf(classTemplateDecl(), classTemplateSpecializationDecl()))));
+  auto LockVarDecl = varDecl(hasType(LockGuardType));
+
+  // Match CompoundStmt with only one 'std::lock_guard'
+  if (!WarnOnlyMultipleLocks) {
+    Finder->addMatcher(
+        compoundStmt(has(declStmt(has(LockVarDecl.bind("lock-decl-single")))),
+                     unless(hasDescendant(declStmt(has(varDecl(
+                         hasType(LockGuardType),
+                         unless(equalsBoundNode("lock-decl-single")))))))),
+        this);
+  }
+
+  // Match CompoundStmt with multiple 'std::lock_guard'
+  Finder->addMatcher(
+      compoundStmt(has(declStmt(has(LockVarDecl.bind("lock-decl-multiple")))),
+                   hasDescendant(declStmt(has(varDecl(
+                       hasType(LockGuardType),
+                       unless(equalsBoundNode("lock-decl-multiple")))))))
+          .bind("block-multiple"),
+      this);
+}
+
+void UseScopedLockCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Decl = Result.Nodes.getNodeAs<VarDecl>("lock-decl-single")) {
+    emitDiag(Decl, Result);
+  }
+
+  if (const auto *Compound =
+          Result.Nodes.getNodeAs<CompoundStmt>("block-multiple")) {
+    emitDiag(findLocksInCompoundStmt(Compound, Result), Result);
+  }
+}
+
+void UseScopedLockCheck::emitDiag(const VarDecl *LockGuard,
+                                  const MatchFinder::MatchResult &Result) {
+  auto Diag = diag(LockGuard->getBeginLoc(),
+                   "use 'std::scoped_lock' instead of 'std::lock_guard'");
+
+  const std::optional<SourceRange> LockGuardTypeRange =
+      getLockGuardRange(LockGuard, *Result.SourceManager);
+
+  if (!LockGuardTypeRange) {
+    return;
+  }
+
+  // Create Fix-its only if we can find the constructor call to handle
+  // 'std::lock_guard l(m, std::adopt_lock)' case
+  const auto *CtorCall = dyn_cast<CXXConstructExpr>(LockGuard->getInit());
+  if (!CtorCall) {
+    return;
+  }
+
+  switch (CtorCall->getNumArgs()) {
+  case 1:
+    Diag << FixItHint::CreateReplacement(LockGuardTypeRange.value(),
+                                         "scoped_lock");
+    return;
+  case 2:
+    const auto *CtorArgs = CtorCall->getArgs();
+
+    const Expr *MutexArg = CtorArgs[0];
+    const Expr *AdoptLockArg = CtorArgs[1];
+
+    const StringRef MutexSourceText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(MutexArg->getSourceRange()),
+        *Result.SourceManager, Result.Context->getLangOpts());
+    const StringRef AdoptLockSourceText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(AdoptLockArg->getSourceRange()),
+        *Result.SourceManager, Result.Context->getLangOpts());
+
+    Diag << FixItHint::CreateReplacement(LockGuardTypeRange.value(),
+                                         "scoped_lock")
+         << FixItHint::CreateReplacement(
+                SourceRange(CtorArgs[0]->getBeginLoc(),
+                            CtorArgs[1]->getEndLoc()),
+                (llvm::Twine(AdoptLockSourceText) + ", " + MutexSourceText)
+                    .str());
+    return;
+  }
+
+  llvm_unreachable("Invalid argument number of std::lock_guard constructor");
+}
+
+void UseScopedLockCheck::emitDiag(
+    const std::vector<std::vector<const VarDecl *>> &LockGuardGroups,
+    const MatchFinder::MatchResult &Result) {
+  for (const auto &Group : LockGuardGroups) {
+    if (Group.size() == 1 && !WarnOnlyMultipleLocks) {
+      emitDiag(Group[0], Result);
+    } else {
+      diag(Group[0]->getBeginLoc(),
+           "use single 'std::scoped_lock' instead of multiple "
+           "'std::lock_guard'");
+
+      for (size_t I = 1; I < Group.size(); ++I) {
+        diag(Group[I]->getLocation(),
+             "additional 'std::lock_guard' declared here", DiagnosticIDs::Note);
+      }
+    }
+  }
+}
+
+} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h
new file mode 100644
index 000000000000000..0e1fd8067ddd1e5
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.h
@@ -0,0 +1,50 @@
+//===--- UseScopedLockCheck.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_MODERNIZE_USESCOPEDLOCKCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USESCOPEDLOCKCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Stmt.h"
+#include <optional>
+
+namespace clang::tidy::modernize {
+
+/// Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's
+/// more flexible and safer alternative ``std::scoped_lock``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-scoped-lock.html
+class UseScopedLockCheck : public ClangTidyCheck {
+public:
+  UseScopedLockCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        WarnOnlyMultipleLocks(Options.get("WarnOnlyMultipleLocks", false)) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus17;
+  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+
+private:
+  void emitDiag(const VarDecl *LockGuard,
+                const ast_matchers::MatchFinder::MatchResult &Result);
+  void emitDiag(const std::vector<std::vector<const VarDecl *>> &LockGroups,
+                const ast_matchers::MatchFinder::MatchResult &Result);
+
+  const bool WarnOnlyMultipleLocks;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USESCOPEDLOCKCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3bddeeda06e06b9..6aefbc473727634 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -91,6 +91,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`use-scoped-lock
+  <clang-tidy/checks/modernize/use-scoped-lock>` check.
+
+  Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's
+  more flexible and safer alternative ``std::scoped_lock``.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 7b9b905ef76715e..b21633459a95cd7 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -308,6 +308,7 @@ Clang-Tidy Checks
    :doc:`modernize-use-nullptr <modernize/use-nullptr>`, "Yes"
    :doc:`modernize-use-override <modernize/use-override>`, "Yes"
    :doc:`modernize-use-ranges <modernize/use-ranges>`, "Yes"
+   :doc:`modernize-use-scoped-lock <modernize/use-scoped-lock>`, "Yes"
    :doc:`modernize-use-starts-ends-with <modernize/use-starts-ends-with>`, "Yes"
    :doc:`modernize-use-std-format <modernize/use-std-format>`, "Yes"
    :doc:`modernize-use-std-numbers <modernize/use-std-numbers>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst
new file mode 100644
index 000000000000000..04b35d0081b3c55
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst
@@ -0,0 +1,81 @@
+.. title:: clang-tidy - modernize-use-scoped-lock
+
+modernize-use-scoped-lock
+=========================
+
+Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's more
+flexible and safer alternative ``std::scoped_lock``. The check will
+automatically transform only single declarations of ``std::lock_guard`` and
+emit warnings for multiple declarations of ``std::lock_guard`` that can be
+replaced with a single declaration of ``std::scoped_lock``.
+
+Examples
+--------
+
+Single ``std::lock_guard`` declaration:
+
+.. code-block:: c++
+
+  std::mutex M;
+  std::lock_guard<std::mutex> L(M);
+
+
+Transforms to:
+
+.. code-block:: c++
+
+  std::mutex m;
+  std::scoped_lock L(M);
+
+Single ``std::lock_guard`` declaration with ``std::adopt_lock``:
+
+.. code-block:: c++
+
+  std::mutex M;
+  std::lock(M);
+  std::lock_guard<std::mutex> L(M, std::adopt_lock);
+
+
+Transforms to:
+
+.. code-block:: c++
+
+  std::mutex M;
+  std::lock(M);
+  std::scoped_lock L(std::adopt_lock, M);
+
+Multiple ``std::lock_guard`` declarations only emit warnings:
+
+.. code-block:: c++
+
+  std::mutex M1, M2;
+  std::lock(M1, M2);
+  std::lock_guard Lock1(M, std::adopt_lock); // warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  std::lock_guard Lock2(M, std::adopt_lock); // note: additional 'std::lock_guard' declared here
+
+
+Limitations
+-----------
+
+The check will not emit warnings if ``std::lock_guard`` is used implicitly via
+``using``, ``typedef`` or ``template``.
+
+.. code-block:: c
+
+  template <template <typename> typename Lock>
+  void TemplatedLock() {
+    std::mutex m;
+    Lock<std::mutex> l(m); // no warning
+  }
+
+  void UsingLock() {
+    using Lock = std::lock_guard<std::mutex>;
+    std::mutex m;
+    Lock l(m); // no warning
+  }
+
+.. option:: WarnOnlyMultipleLocks
+
+  When `true`, the check will only emit warnings if the there are multiple
+  consecutive ``std::lock_guard`` declarations that can be replaced with a
+  single ``std::scoped_lock`` declaration. Default is `false`.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp
new file mode 100644
index 000000000000000..6d46e43bc2be477
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock-only-warn-on-multiple.cpp
@@ -0,0 +1,122 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s modernize-use-scoped-lock %t -- \
+// RUN:   -config="{CheckOptions: {modernize-use-scoped-lock.WarnOnlyMultipleLocks: true}}"
+
+namespace std {
+
+struct mutex {
+  void lock() {}
+  void unlock() {}
+};
+
+template<class Lockable1, class Lockable2, class... LockableN >
+void lock(Lockable1& lock1, Lockable2& lock2, LockableN&... lockn );
+
+struct adopt_lock_t { };
+std::adopt_lock_t adopt_lock {};
+
+template <typename Mutex>
+struct lock_guard {
+  lock_guard(Mutex &m) { }
+  lock_guard(Mutex &m, std::adopt_lock_t t) {}
+  lock_guard( const lock_guard& ) = delete;
+};
+
+template <typename... MutexTypes>
+struct scoped_lock {
+  scoped_lock(MutexTypes&... m) {}
+  scoped_lock(std::adopt_lock_t t, MutexTypes&... m) {}
+};
+
+} // namespace std
+
+
+void Positive() {
+  std::mutex m;
+
+  {
+    std::lock_guard<std::mutex> l1(m);
+    std::lock_guard<std::mutex> l2(m);
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+    // CHECK-MESSAGES: :[[@LINE-2]]:33: note: additional 'std::lock_guard' declared here
+  }
+
+  {
+    std::lock_guard<std::mutex> l1(m), l2(m), l3(m);
+    std::lock_guard<std::mutex> l4(m);
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+    // CHECK-MESSAGES: :[[@LINE-3]]:40: note: additional 'std::lock_guard' declared here
+    // CHECK-MESSAGES: :[[@LINE-4]]:47: note: additional 'std::lock_guard' declared here
+    // CHECK-MESSAGES: :[[@LINE-4]]:33: note: additional 'std::lock_guard' declared here
+  }
+  
+  { 
+    std::lock(m, m);
+    std::lock_guard<std::mutex> l1(m, std::adopt_lock);
+    std::lock_guard<std::mutex> l2(m, std::adopt_lock);
+    // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+    // CHECK-MESSAGES: :[[@LINE-2]]:33: note: additional 'std::lock_guard' declared here
+  } 
+}
+
+void Negative() {
+  std::mutex m;
+  {
+    std::lock_guard<std::mutex> l(m);
+  }
+
+  {
+    std::lock_guard<std::mutex> l(m, std::adopt_lock);
+  }
+}
+
+void PositiveInsideArg(std::mutex &m1, std::mutex &m2, std::mutex &m3) {
+  std::lock_guard<std::mutex> l1(m1);
+  std::lock_guard<std::mutex> l2(m2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  // CHECK-MESSAGES: :[[@LINE-2]]:31: note: additional 'std::lock_guard' declared here
+}
+
+
+void NegativeInsideArg(std::mutex &m1, std::mutex &m2, std::mutex &m3) {
+  std::lock_guard<std::mutex> l3(m3);
+}
+
+template <typename T>
+void PositiveTemplated() {
+  std::mutex m1, m2;
+  
+  std::lock_guard<std::mutex> l1(m1);
+  std::lock_guard<std::mutex> l2(m2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  // CHECK-MESSAGES: :[[@LINE-2]]:31: note: additional 'std::lock_guard' declared here  
+}
+
+template <typename T>
+void NegativeTemplated() {
+  std::mutex m1, m2, m3;
+  std::lock_guard<std::mutex> l(m1);
+}
+
+template <typename Mutex>
+void PositiveTemplatedMutex() {
+  Mutex m1, m2;
+
+  std::lock_guard<Mutex> l1(m1);
+  std::lock_guard<Mutex> l2(m2);
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use single 'std::scoped_lock' instead of multiple 'std::lock_guard'
+  // CHECK-MESSAGES: :[[@LINE-2]]:26: note: additional 'std::lock_guard' declared here
+}
+
+template <typename Mutex>
+void NegativeTemplatedMutex() {
+  Mutex m1;
+  std::lock_guard<Mutex> l(m1);
+}
+
+struct NegativeClass {
+  void Negative() {
+    std::lock_guard<std::mutex> l(m1);
+  }
+  
+  std::mutex m1;
+};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use...
[truncated]

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

PiotrZSL

Few nits, but overall not bad.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst

clang-tools-extra/docs/ReleaseNotes.rst

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

clang-tools-extra/test/clang-tidy/checkers/modernize/use-scoped-lock.cpp

vbvictor · 2025-02-14T09:21:58Z

@PiotrZSL, I fixed all your comments and improved check by adding new WarnOnUsingAndTypedef option.

vbvictor · 2025-02-22T08:26:43Z

I changed option name from WarnOnlyOnMultipleLocks to WarnOnSingleLocks. I think it will be more clear for users what they turn on and off. When having WarnOnlyOnMultipleLocks together with WarnOnUsingAndTypedef option, it is confusing whether first option will disable warnings on using and typedef or not. Now it is clear that options will not affect each other.

vbvictor · 2025-02-26T12:28:39Z

Ping

vbvictor · 2025-03-03T07:03:13Z

Ping @PiotrZSL @HerrCai0907

clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

HerrCai0907

I try to use it. The matcher for single and multiple is unclear. There are some cases for multiple to match single lock when the other lock is in another sub compound stmt.

It can give more clear matcher results.

AST_MATCHER_P(CompoundStmt, hasMultiple, ast_matchers::internal::Matcher<Stmt>,
              InnerMatcher) {
  size_t Cnt = 0;
  for (const Stmt *Stmt : Node.body()) {
    if (InnerMatcher.matches(*Stmt, Finder, Builder))
      Cnt++;
  }
  return Cnt > 1;
}

AST_MATCHER_P(CompoundStmt, hasSingle, ast_matchers::internal::Matcher<Stmt>,
              InnerMatcher) {
  ast_matchers::internal::BoundNodesTreeBuilder Result;
  size_t Cnt = 0;
  for (const Stmt *Stmt : Node.body()) {
    ast_matchers::internal::BoundNodesTreeBuilder TB(*Builder);
    if (InnerMatcher.matches(*Stmt, Finder, &TB)) {
      Cnt++;
      Result.addMatch(TB);
    }
  }
  if (Cnt == 1) {
    *Builder = std::move(Result);
    return true;
  }
  Builder->removeBindings([](const ast_matchers::internal::BoundNodesMap &) { return true; });
  return false;
}

HerrCai0907

LGTM with nits

follow LLVM coding guideline to remove {} for only one lines if / for / ...
personal I prefer to use different function name instead of emitDiag for everythings.

vbvictor · 2025-03-19T13:58:19Z

@PiotrZSL Ping

PiotrZSL

Few nits

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

PiotrZSL · 2025-03-19T14:04:31Z

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

+bool isLockGuard(const QualType &Type) {
+  if (const auto *Record = Type->getAs<RecordType>())
+    if (const RecordDecl *Decl = Record->getDecl())
+      return Decl->getName() == "lock_guard" && Decl->isInStdNamespace();


would be good to check if we have "isIdentifier" or we will get assert for unnamed declarations.

added assert for isIdentifier

There is assert in getName already, and this is a reason why I didn't want assert but proper handling of anonumous classes. Handle this correctly instead of crasing.

added new function isLockGuardDecl with needed logic

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

PiotrZSL · 2025-03-19T14:16:05Z

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

+  }
+
+  Finder->addMatcher(compoundStmt(unless(isExpansionInSystemHeader()),
+                                  hasMultiple(declStmt(has(LockVarDecl))))


this hasMultiple is useless.

simply use:
has(declStmt(has(LockVarDecl)).bind("first")),
has(declStmt(unless(equalsBoundNode("first"), has(LockVarDecl)))

I'm afraid there is controversial opinion about "cleanness" of matchers. Previously I had matchers with equalsBoundNode but rewrote them according to #126434 (review).
I believe that current version with for-loop is a bit readable than version with equalsBoundNode (that was the reason for rewriting).
I can try to make new variables with proper names hasSingle and hasMultiple that will satisfy everyone.

FYI @HerrCai0907

Another reason to rewrite matchers was

There are some cases for multiple to match single lock when the other lock is in another sub compound stmt.

For this situation I have test-case PositiveNested() which passed with both versions, maybe I didn't cover something?

I posted this match because the old version is incorrect. Maybe you can try the matchers from @PiotrZSL and check the results. It looks workable.

Added new matchers, seem to work fine.
Previous matchers had little bug, I added test-case for that in the latest commit.
Thank everyone for help

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp

clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst

vbvictor · 2025-03-28T21:58:52Z

@PiotrZSL, Ping

llvmbot added clang-tools-extra clang-tidy labels Feb 9, 2025

vbvictor changed the title ~~[clang-tidy] add new modernize-use-scoped-lock check~~ [clang-tidy] add new check: modernize-use-scoped-lock Feb 9, 2025

EugeneZelenko reviewed Feb 10, 2025

View reviewed changes

EugeneZelenko requested review from PiotrZSL, carlosgalvezp, 5chmidti and HerrCai0907 February 10, 2025 15:47

PiotrZSL reviewed Feb 10, 2025

View reviewed changes

vbvictor requested a review from PiotrZSL February 19, 2025 20:15

vbvictor force-pushed the modernize-use-scoped-lock branch 2 times, most recently from 2c27387 to 45d8029 Compare March 3, 2025 06:26

HerrCai0907 reviewed Mar 3, 2025

View reviewed changes

clang-tools-extra/docs/clang-tidy/checks/modernize/use-scoped-lock.rst Outdated Show resolved Hide resolved

HerrCai0907 reviewed Mar 3, 2025

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp Outdated Show resolved Hide resolved

vbvictor requested a review from HerrCai0907 March 9, 2025 21:37

vbvictor added 4 commits March 11, 2025 14:18

[clang-tidy] add scoped-lock-check

bc1b4ad

add newlines to test files

fc9c1b5

Add UnqualifiedDesugaredType to match more cases

f34c9ae

add more tests to WarnOnSingleLock opt

be8cfb1

vbvictor force-pushed the modernize-use-scoped-lock branch from e6402ed to be8cfb1 Compare March 11, 2025 11:18

HerrCai0907 reviewed Mar 12, 2025

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp Show resolved Hide resolved

HerrCai0907 reviewed Mar 12, 2025

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseScopedLockCheck.cpp Outdated Show resolved Hide resolved

add lambda to tests

a7fec18

vbvictor requested a review from HerrCai0907 March 14, 2025 13:14

HerrCai0907 requested changes Mar 14, 2025

View reviewed changes

made more readable matchers

c69993e

vbvictor requested a review from HerrCai0907 March 15, 2025 09:11

HerrCai0907 approved these changes Mar 17, 2025

View reviewed changes

vbvictor added 2 commits March 17, 2025 10:48

clean unnecessary braces

2f3d5d3

make diag function more readable

d55d427

PiotrZSL reviewed Mar 19, 2025

View reviewed changes

vbvictor added 3 commits March 19, 2025 19:54

fix comments

53a0781

modernize docs

218fb62

feat: new matchers, fix: comments

5a3118e

vbvictor requested a review from PiotrZSL March 20, 2025 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-tidy] add new check: modernize-use-scoped-lock #126434

[clang-tidy] add new check: modernize-use-scoped-lock #126434

vbvictor commented Feb 9, 2025

llvmbot commented Feb 9, 2025

llvmbot commented Feb 9, 2025

PiotrZSL left a comment

vbvictor commented Feb 14, 2025 •

edited

Loading

vbvictor commented Feb 22, 2025 •

edited

Loading

vbvictor commented Feb 26, 2025

vbvictor commented Mar 3, 2025

HerrCai0907 left a comment •

edited

Loading

HerrCai0907 left a comment

vbvictor commented Mar 19, 2025

PiotrZSL left a comment

PiotrZSL Mar 19, 2025

vbvictor Mar 19, 2025

PiotrZSL Mar 20, 2025

vbvictor Mar 20, 2025

PiotrZSL Mar 19, 2025

vbvictor Mar 19, 2025 •

edited

Loading

vbvictor Mar 19, 2025 •

edited

Loading

HerrCai0907 Mar 20, 2025

vbvictor Mar 20, 2025 •

edited

Loading

vbvictor commented Mar 28, 2025

[clang-tidy] add new check: modernize-use-scoped-lock #126434

Are you sure you want to change the base?

[clang-tidy] add new check: modernize-use-scoped-lock #126434

Conversation

vbvictor commented Feb 9, 2025

llvmbot commented Feb 9, 2025

llvmbot commented Feb 9, 2025

PiotrZSL left a comment

Choose a reason for hiding this comment

vbvictor commented Feb 14, 2025 • edited Loading

vbvictor commented Feb 22, 2025 • edited Loading

vbvictor commented Feb 26, 2025

vbvictor commented Mar 3, 2025

HerrCai0907 left a comment • edited Loading

Choose a reason for hiding this comment

HerrCai0907 left a comment

Choose a reason for hiding this comment

vbvictor commented Mar 19, 2025

PiotrZSL left a comment

Choose a reason for hiding this comment

PiotrZSL Mar 19, 2025

Choose a reason for hiding this comment

vbvictor Mar 19, 2025

Choose a reason for hiding this comment

PiotrZSL Mar 20, 2025

Choose a reason for hiding this comment

vbvictor Mar 20, 2025

Choose a reason for hiding this comment

PiotrZSL Mar 19, 2025

Choose a reason for hiding this comment

vbvictor Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

vbvictor Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

HerrCai0907 Mar 20, 2025

Choose a reason for hiding this comment

vbvictor Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

vbvictor commented Mar 28, 2025

vbvictor commented Feb 14, 2025 •

edited

Loading

vbvictor commented Feb 22, 2025 •

edited

Loading

HerrCai0907 left a comment •

edited

Loading

vbvictor Mar 19, 2025 •

edited

Loading

vbvictor Mar 19, 2025 •

edited

Loading

vbvictor Mar 20, 2025 •

edited

Loading