[clang-tidy] added cppcoreguidelines-use-raii-locks check

[MyDeveloperDay]

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

45

Nit: by and large this list looks to be in alphabetical based on the checker name (except the last one), not sure if thats by accident or design

[lewmpk]

fixed ordering of cppcoreguidelines module checks

[MyDeveloperDay]

This looks good to me but I would wait for one of @JonasToth or @alexfh to perhaps take a look,

maybe you should add some nested scope example too using the same name, I know the compiler should warn about a shadow variable anyway but

std::mutex m;
m.lock();
{
    std::mutex m;
    m.lock();
    m.unlock();
}
m_unlock();

and what about

std::mutex m1;
std::mutex m2;

m1.lock();
m1.unlock();
m2.lock();
m2.unlock();

and

std::mutex m1;
std::mutex m2;

m1.lock();
m2.lock();
m2.unlock();
m1.unlock();

std::mutex m1;
std::mutex m2;

m1.lock();
m1.unlock();
m2.lock();
m2.unlock();

std::mutex m1;
std::mutex m2;

m1.lock();
m2.lock();
m1.unlock();
m2.unlock();

[lewmpk]

I'll improve the tests :)

[lebedev.ri]

+1 i was just going to comment - this needs much more tests.
Also, it would be really great if you could supply the differential's description.T

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp

26

This should be a config param

27

Terminology: *this* doesn't match anything.
It's a matcher, yes, but it's just a lambda.
The actual match happens at the end.

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h

14–15

Separate with newline

[lewmpk]

fixed nested use case

[lewmpk]

removed unneccesary include

[lewmpk]

handle other basiclockable types

[lewmpk]

renamed option for cppcoreguidelines-use-raii-locks

[lewmpk]

fixed documentation

[lewmpk]

fixed documentation again

[lewmpk]

fixed documentation formatting

[lewmpk]

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp

27

I was trying to describe its intent, not its action. Does anyone have any suggestions for a clearer comment?

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h

14–15

it seems that most checks are this way. it was autogenerated by the `add_new_check.py` script.

[lewmpk]

fixed documentation formatting

[lewmpk]

improved tests

[lewmpk]

support try_lock_for and try_lock_until

[lewmpk]

remove support for try_lock_for and try_lock_until

[riccibruno]

Ugh, could you please avoid doing lots a tiny changes every 5 minutes ? This causes spam on cfe-commits /:

[JonasToth]

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp

36

If we allow boost, pre c++11 is ok as well.
In general, plz use proper grammar, punctuation and full sentences in comments.

42

please add proper punctutation in comments. we aim for correct text you can read and understand, with proper spelling and grammar.

66

please add the * for pointers to emphasize the difference between values and pointers.
In general we do not add const to values (as i believe is done in later lines), but only for pointers and references.

70

Please don't retrieve the name like this. Too error prone and compilatcated.

You can compare use DeclRefExpr (https://clang.llvm.org/doxygen/classclang_1_1DeclRefExpr.html) for your MutexExpr instead.
From there you go to the Decl and compare on pointer equality.

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h

29

It might be a good idea to add the boost types as well? I believe they are interface-compatible, given the std version is derived from them.

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

5

Please add more tests

What happens for this?

void foo() {
  std::mutex m;
  m.lock();
  m.unlock();
  m.lock();
  m.unlock();
  m.try_lock();
  m.lock();
  m.unlock();
}

• Please add tests for templates, where the lock-type is a template parameter
• please add tests where the locking happens within macros
• please add tests for usage within loops
• where cases like std::mutex m1; std::mutex &m2 = m1; // usage. This should not be diagnosed, right?

[lewmpk]

refactor and improved tests

[lewmpk]

added support for boost lockable types

[lewmpk]

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

5

I've added a test case for your example, templates, macros and loops.
I can't catch the case std::mutex m1; std::mutex &m2 = m1; // usage, but i can catch trivial cases.

[JonasToth]

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

5

Yes, your not supposed to catch those. But i feel things like this should be documented. In theory catching this particular case is possible (we do similar analysis for const.
But it is totally acceptable to leave as is!

[JonasToth]

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

91

I got another one that I think defeats the analysis:

while (true)
{

my_label:
  m.lock();

  if (some_condition)
    goto my_label;
  
  m.unlock();
}

Usage of goto can interfer and make the situation more complicated. I am not asking you to implement that correctly (not sure if even possible with pure clang-tidy) but I think a pathological example should be documented for the user.

[Eugene.Zelenko]

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp

30

Please use static for functions instead of anonymous namespaces. See LLVM coding guidelines.

31

Please don't use auto where return type is not obvious.

45

Unnecessary empty line.

73

Unnecessary empty line.

78

Please don't use auto where return type is not obvious.

79

Please don't use auto where return type is not obvious.

83

Please don't use auto where return type is not obvious.

84

Please don't use auto where return type is not obvious.

90

Please don't use auto where return type is not obvious.

92

Please don't use auto where return type is not obvious.

docs/ReleaseNotes.rst

97

Missing space before ``std::mutex

docs/clang-tidy/checks/cppcoreguidelines-use-raii-locks.rst

7

Please synchronize first statement with Release Notes.

19

Please use single ` for options values.

[lewmpk]

added example in docs and explicitly specified types for some variables

[lewmpk]

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

91

why would this defeat the analysis?

[JonasToth]

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

91

Because goto allows you to reorder you code-locations, so the ordering of what comes before what is not so ez.

[lewmpk]

match lock() and unlock() calls by decl

[JonasToth]

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

91

let me restate: You are comparing the occurences of lock and unlock by linenumber, so physical appearance in the source code. goto allows you to jump wildly through your code, so that physical appearance does not match actual control flow.
I am not saying that you need to resolve that (not easily done within clang-tidy), but documenting it is worth it.
And if you mix goto and locking mechanisms, you get what you deserve, which is no help from tooling ;)

[JonasToth]

IMHO the check is close to being finished. please address the notes and mark them as done if finished with it. So its clear to see whats outstanding.
In my opinion the user-facing documentation misses a "Limitations" sections that shows the artificial goto example, that would show that the used mechanism doesn't handle it.

[ccotter]

bump - I know this is really old, but @lewmpk do you plan on finishing up the last remaining comments?

clang-tidy/cppcoreguidelines/CMakeLists.txt

add_clang_library(clangTidyCppCoreGuidelinesModule
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
   SlicingCheck.cpp
   SpecialMemberFunctionsCheck.cpp
+  UseRaiiLocksCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangSerialization
   clangTidy
   clangTidyMiscModule
   clangTidyModernizeModule
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling
   )

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 #include "SlicingCheck.h"
 #include "SpecialMemberFunctionsCheck.h"
+#include "UseRaiiLocksCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");
     CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
     CheckFactories.registerCheck<SpecialMemberFunctionsCheck>(
         "cppcoreguidelines-special-member-functions");
     CheckFactories.registerCheck<SlicingCheck>("cppcoreguidelines-slicing");
+    CheckFactories.registerCheck<UseRaiiLocksCheck>(
+        "cppcoreguidelines-use-raii-locks");
     CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
 

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h

+//===--- UseRaiiLocksCheck.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_CPPCOREGUIDELINES_USERAIILOCKSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_USERAIILOCKSCHECK_H
+
+#include "../ClangTidy.h"
+#include <iostream>
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Check for instances of explicit std::mutex lock() and unlock() calls
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-use-raii-locks.html
+class UseRaiiLocksCheck : public ClangTidyCheck {
+public:
+  UseRaiiLocksCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_USERAIILOCKSCHECK_H

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp

+//===--- UseRaiiLocksCheck.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 "UseRaiiLocksCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void UseRaiiLocksCheck::registerMatchers(MatchFinder *Finder) {
+  // lock_guards require c++11 or later
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  const auto MutexDecl = type(hasUnqualifiedDesugaredType(
+      recordType(hasDeclaration(cxxRecordDecl(hasName("::std::mutex"))))));
+  // Match expressions of type mutex or mutex pointer
+  const auto MutexExpr =
+      expr(anyOf(hasType(MutexDecl), hasType(qualType(pointsTo(MutexDecl)))));
+
+  // Match a call to mutex 'lock' or 'try_lock'
+  const auto MutexLockCallExpr =
+      cxxMemberCallExpr(on(MutexExpr), callee(memberExpr()),
+                        callee(cxxMethodDecl(hasAnyName("lock", "try_lock"))));
+
+  // Match a call to mutex 'unlock'
+  const auto MutexUnlockCallExpr =
+      cxxMemberCallExpr(on(MutexExpr), callee(memberExpr()),
+                        callee(cxxMethodDecl(hasAnyName("unlock"))));
+
+  // Match a scope with a calls to both 'lock' and 'unlock'
+  const auto ScopedProblem =
+      compoundStmt(forEach(MutexLockCallExpr.bind("lock")),
+                   forEach(MutexUnlockCallExpr.bind("unlock")));
+  Finder->addMatcher(ScopedProblem.bind("func"), this);
+}
+
+void UseRaiiLocksCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto LockCallExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("lock");
+  const auto UnlockCallExpr =
+      Result.Nodes.getNodeAs<CXXMemberCallExpr>("unlock");
+
+  const auto LockObjectName = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(
+          LockCallExpr->getImplicitObjectArgument()->getSourceRange()),
+      *Result.SourceManager, getLangOpts());
+  const auto UnlockObjectName = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(
+          UnlockCallExpr->getImplicitObjectArgument()->getSourceRange()),
+      *Result.SourceManager, getLangOpts());
+
+  // Ignore m.lock(); m2.unlock()
+  if (LockObjectName != UnlockObjectName)
+    return;
+
+  const auto LockLineNum =
+      Result.SourceManager->getPresumedLoc(LockCallExpr->getBeginLoc())
+          .getLine();
+  const auto UnlockLineNum =
+      Result.SourceManager->getPresumedLoc(UnlockCallExpr->getBeginLoc())
+          .getLine();
+
+  // Ignore unlock before lock
+  if (UnlockLineNum < LockLineNum)
+    return;
+
+  diag(LockCallExpr->getBeginLoc(),
+       "use RAII-style locking mechanisms such as 'std::lock_guard' "
+       "or 'std::unique_lock'");
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New :doc:`abseil-duration-unnecessary-conversion
   numeric types and back again.
 
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 
   Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
   in the Time domain instead of the numeric domain.
 
+- New :doc:`cppcoreguidelines-use-raii-locks
+  <clang-tidy/checks/cppcoreguidelines-use-raii-locks>` check.
+
+  Checks for explicit usage of ``std::mutex`` functions ``lock()`` and ``unlock``.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 - New alias :doc:`cppcoreguidelines-explicit-virtual-functions

docs/clang-tidy/checks/cppcoreguidelines-use-raii-locks.rst

+.. title:: clang-tidy - cppcoreguidelines-use-raii-locks
+
+cppcoreguidelines-use-raii-locks
+================================
+
+The explicit use of std::mutex functions ``lock()`` and ``unlock()`` is error 
+prone and should be avoided. It's recommended to use RAII-style locking 
+mechanisms such as ``std::lock_guard`` or ``std::unique_lock``.

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
+   cppcoreguidelines-use-raii-locks
    fuchsia-default-arguments
    fuchsia-header-anon-namespaces (redirects to google-build-namespaces) <fuchsia-header-anon-namespaces>
    fuchsia-multiple-inheritance
    fuchsia-overloaded-operator
    fuchsia-restrict-system-includes
    fuchsia-statically-constructed-objects
    fuchsia-trailing-return
    fuchsia-virtual-inheritance

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-use-raii-locks %t
+
+// Mock implementation of std::mutex
+namespace std {
+struct mutex {
+  void lock();
+  void unlock();
+};
+} // namespace std
+
+void warn_me1() {
+  std::mutex m;
+  m.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m.unlock();
+}
+
+void warn_me2() {
+  std::mutex m;
+  m.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  {
+    std::mutex m;
+    m.lock();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use RAII
+    m.unlock();
+  }
+  m.unlock();
+}
+
+void warn_me3() {
+  std::mutex m1;
+  std::mutex m2;
+
+  m1.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m1.unlock();
+  m2.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m2.unlock();
+}
+
+void warn_me4() {
+  std::mutex m1;
+  std::mutex m2;
+
+  m1.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m1.unlock();
+  m2.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m2.unlock();
+}
+
+void warn_me5() {
+  std::mutex m1;
+  std::mutex m2;
+
+  m1.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m2.lock();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use RAII
+  m1.unlock();
+  m2.unlock();
+}
+
+void ignore_me1() {
+  std::mutex m;
+  m.unlock();
+  m.lock();
+  // CHECK-MESSAGES-NOT: warning:
+}
+
+void ignore_me2() {
+  std::mutex m1;
+  std::mutex m2;
+  m1.lock();
+  // CHECK-MESSAGES-NOT: warning:
+  m2.unlock();
+}