[clang-tidy] check for __func__/__FUNCTION__ in lambdas

[brycel]

Summary

Add a clang-tidy check for using func__/FUNCTION__ inside lambdas. This evaluates to the string operator(), which is almost never useful and almost certainly not what the author intended.

[lebedev.ri]

1. What about __PRETTY_FUNCTION__ ?
2. Consider following generic error handling macro:

(ThrowException is some template function)

#undef STR
#define STR(a) XSTR(a)

#define ThrowExceptionHelper(CLASS, fmt, ...) ThrowException<CLASS>(__FILE__ ":" STR(__LINE__) ": %s: " fmt, __PRETTY_FUNCTION__, ##__VA_ARGS__)
#endif

Which is called like if(somethig) ThrowException(std::exception, "%s", "bar");
Even though the function name may be useless, file/line info is still there and is(?) correct.
Perhaps there should not be a warning in such a case?

[brycel]

In D33497#763307, @lebedev.ri wrote:

1. What about __PRETTY_FUNCTION__ ?

__PRETTY_FUNCTION__ is a little more useful, it at least tells you which function the lambda is defined in:

#include <stdio.h>

int main() {
  auto f = [] {
    printf("__func__: %s\n", __func__);
    printf("__FUNCTION__: %s\n", __FUNCTION__);
    printf("__PRETTY_FUNCTION__: %s\n", __PRETTY_FUNCTION__);
  };
  f();
}

results in the output:

__func__: operator()
__FUNCTION__: operator()
__PRETTY_FUNCTION__: auto main()::(anonymous class)::operator()() const

I think __PRETTY_FUNCTION__ inside a lambda is useful enough not to warn about.

2. Consider following generic error handling macro:

(ThrowException is some template function)

#undef STR
#define STR(a) XSTR(a)

#define ThrowExceptionHelper(CLASS, fmt, ...) ThrowException<CLASS>(__FILE__ ":" STR(__LINE__) ": %s: " fmt, __PRETTY_FUNCTION__, ##__VA_ARGS__)
#endif

Which is called like if(somethig) ThrowException(std::exception, "%s", "bar");
Even though the function name may be useless, file/line info is still there and is(?) correct.
Perhaps there should not be a warning in such a case?

That's a good point. I'll look into suppressing the warning if we're in a macro definition where __FILE__ and __LINE__ are both used. It may suppress some warnings that would be legitimate (such as if __FILE__/__LINE__ are written but not actually output anywhere), but in those cases it's probably impossible to figure out the author's intention.

[lebedev.ri]

In D33497#763376, @brycel wrote:

In D33497#763307, @lebedev.ri wrote:

1. What about __PRETTY_FUNCTION__ ?

I think __PRETTY_FUNCTION__ inside a lambda is useful enough not to warn about.

Then that should be checked by the test

2. Consider following generic error handling macro:

(ThrowException is some template function)

That's a good point. I'll look into suppressing the warning if we're in a macro definition where __FILE__ and __LINE__ are both used. It may suppress some warnings that would be legitimate (such as if __FILE__/__LINE__ are written but not actually output anywhere), but in those cases it's probably impossible to figure out the author's intention.

Makes sense

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[brycel]

Addressed the following review comments:

• Added a test to make sure we don't warn on PRETTY_FUNCTION
• Suppressed warnings when in a macro that also uses FILE and LINE
• Updated docs/ReleaseNotes.rst.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

83 ↗

(On Diff #100149)

Please highlight func and FUNCTION with ``.

[brycel]

Fixed ReleaseNotes.rst

[Eugene.Zelenko]

docs/ReleaseNotes.rst

83 ↗

(On Diff #100149)

I meant double `, not single .

[brycel]

Double backticks, not single.

[brycel]

docs/ReleaseNotes.rst

83 ↗

(On Diff #100149)

Sorry, I had Markdown on my brain.

[alexfh]

clang-tidy/misc/LambdaFunctionNameCheck.cpp

61–62 ↗

(On Diff #100161)

llvm::make_unique<MacroExpansionsWithFileAndLine>(...)

67–68 ↗

(On Diff #100161)

nit: I'd use the early return style here.

73–74 ↗

(On Diff #100161)

I'm afraid this can become extremely slow on large files with boilerplate/generated code. Since the code is just looking for exact matches (and not any overlapping ranges, for example), we could replace the vector with a (hash?) map to limit the worst case complexity.

[brycel]

Addressed comments from alexfh. In particular, changed to using a set from a vector.

[brycel]

Move namespace-level types to class-level to avoid potential future name conflicts.

[alexfh]

Looks good!

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
+  LambdaFunctionNameCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.h

+//===--- LambdaFunctionNameCheck.h - clang-tidy------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_LAMBDA_FUNCTION_NAME_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_LAMBDA_FUNCTION_NAME_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect when __func__ or __FUNCTION__ is being used from within a lambda. In
+/// that context, those expressions expand to the name of the call operator
+/// (i.e., `operator()`).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-lambda-function-name.html
+class LambdaFunctionNameCheck : public ClangTidyCheck {
+public:
+  struct SourceRangeLessThan {
+    bool operator()(const SourceRange &L, const SourceRange &R) {
+      if (L.getBegin() == R.getBegin()) {
+        return L.getEnd() < R.getEnd();
+      }
+      return L.getBegin() < R.getBegin();
+    }
+  };
+  using SourceRangeSet = std::set<SourceRange, SourceRangeLessThan>;
+
+  LambdaFunctionNameCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  SourceRangeSet SuppressMacroExpansions;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_LAMBDA_FUNCTION_NAME_H

clang-tools-extra/trunk/clang-tidy/misc/LambdaFunctionNameCheck.cpp

+//===--- LambdaFunctionNameCheck.cpp - clang-tidy--------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "LambdaFunctionNameCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/MacroInfo.h"
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+// Keep track of macro expansions that contain both __FILE__ and __LINE__. If
+// such a macro also uses __func__ or __FUNCTION__, we don't want to issue a
+// warning because __FILE__ and __LINE__ may be useful even if __func__ or
+// __FUNCTION__ is not, especially if the macro could be used in the context of
+// either a function body or a lambda body.
+class MacroExpansionsWithFileAndLine : public PPCallbacks {
+public:
+  explicit MacroExpansionsWithFileAndLine(
+      LambdaFunctionNameCheck::SourceRangeSet *SME)
+      : SuppressMacroExpansions(SME) {}
+
+  void MacroExpands(const Token &MacroNameTok,
+                    const MacroDefinition &MD, SourceRange Range,
+                    const MacroArgs *Args) override {
+    bool has_file = false;
+    bool has_line = false;
+    for (const auto& T : MD.getMacroInfo()->tokens()) {
+      if (T.is(tok::identifier)) {
+        StringRef IdentName = T.getIdentifierInfo()->getName();
+        if (IdentName == "__FILE__") {
+          has_file = true;
+        } else if (IdentName == "__LINE__") {
+          has_line = true;
+        }
+      }
+    }
+    if (has_file && has_line) {
+      SuppressMacroExpansions->insert(Range);
+    }
+  }
+
+private:
+  LambdaFunctionNameCheck::SourceRangeSet* SuppressMacroExpansions;
+};
+
+} // namespace
+
+void LambdaFunctionNameCheck::registerMatchers(MatchFinder *Finder) {
+  // Match on PredefinedExprs inside a lambda.
+  Finder->addMatcher(predefinedExpr(hasAncestor(lambdaExpr())).bind("E"),
+                     this);
+}
+
+void LambdaFunctionNameCheck::registerPPCallbacks(CompilerInstance &Compiler) {
+  Compiler.getPreprocessor().addPPCallbacks(
+      llvm::make_unique<MacroExpansionsWithFileAndLine>(
+          &SuppressMacroExpansions));
+}
+
+void LambdaFunctionNameCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getNodeAs<PredefinedExpr>("E");
+  if (E->getIdentType() != PredefinedExpr::Func &&
+      E->getIdentType() != PredefinedExpr::Function) {
+    // We don't care about other PredefinedExprs.
+    return;
+  }
+  if (E->getLocation().isMacroID()) {
+    auto ER =
+        Result.SourceManager->getImmediateExpansionRange(E->getLocation());
+    if (SuppressMacroExpansions.find(SourceRange(ER.first, ER.second)) !=
+        SuppressMacroExpansions.end()) {
+      // This is a macro expansion for which we should not warn.
+      return;
+    }
+  }
+  diag(E->getLocation(),
+       "inside a lambda, '%0' expands to the name of the function call "
+       "operator; consider capturing the name of the enclosing function "
+       "explicitly")
+      << PredefinedExpr::getIdentTypeName(E->getIdentType());
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
+#include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedConstCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"

 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "misc-forwarding-reference-overload");
+    CheckFactories.registerCheck<LambdaFunctionNameCheck>(
+        "misc-lambda-function-name");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>("misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");

clang-tools-extra/trunk/docs/ReleaseNotes.rst

- Improved `cppcoreguidelines-no-malloc
 
   Allow custom memory management functions to be considered as well.
   
 - New `misc-forwarding-reference-overload
   <http://clang.llvm.org/extra/clang-tidy/checks/misc-forwarding-reference-overload.html>`_ check
 
   Finds perfect forwarding constructors that can unintentionally hide copy or move constructors.
 
+- New `misc-lambda-function-name
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-lambda-function-name.html>`_ check
+
+  Finds uses of ``__func__`` or ``__FUNCTION__`` inside lambdas.
+
 - New `modernize-replace-random-shuffle
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-random-shuffle.html>`_ check
 
   Finds and fixes usage of ``std::random_shuffle`` as the function has been removed from C++17.
 
 - New `modernize-return-braced-init-list
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html>`_ check
 

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    misc-dangling-handle
    misc-definitions-in-headers
    misc-fold-init-type
    misc-forward-declaration-namespace
    misc-forwarding-reference-overload
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
+   misc-lambda-function-name
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const
    misc-misplaced-widening-cast
    misc-move-const-arg
    misc-move-constructor-init
    misc-move-forwarding-reference
    misc-multiple-statement-macro

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-lambda-function-name.rst

+.. title:: clang-tidy - misc-lambda-function-name
+
+misc-lambda-function-name
+=========================
+
+Checks for attempts to get the name of a function from within a lambda
+expression. The name of a lambda is always something like ``operator()``, which
+is almost never what was intended.
+
+Example:
+
+.. code-block:: c++
+                
+  void FancyFunction() {
+    [] { printf("Called from %s\n", __func__); }();
+    [] { printf("Now called from %s\n", __FUNCTION__); }();
+  }
+
+Output::
+
+  Called from operator()
+  Now called from operator()
+
+Likely intended output::
+
+  Called from FancyFunction
+  Now called from FancyFunction

clang-tools-extra/trunk/test/clang-tidy/misc-lambda-function-name.cpp

+// RUN: %check_clang_tidy %s misc-lambda-function-name %t
+
+void Foo(const char* a, const char* b, int c) {}
+
+#define FUNC_MACRO Foo(__func__, "", 0)
+#define FUNCTION_MACRO Foo(__FUNCTION__, "", 0)
+#define EMBED_IN_ANOTHER_MACRO1 FUNC_MACRO
+
+void Positives() {
+  [] { __func__; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__func__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { __FUNCTION__; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__FUNCTION__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { FUNC_MACRO; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__func__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { FUNCTION_MACRO; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__FUNCTION__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+  [] { EMBED_IN_ANOTHER_MACRO1; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: inside a lambda, '__func__' expands to the name of the function call operator; consider capturing the name of the enclosing function explicitly [misc-lambda-function-name]
+}
+
+#define FUNC_MACRO_WITH_FILE_AND_LINE Foo(__func__, __FILE__, __LINE__)
+#define FUNCTION_MACRO_WITH_FILE_AND_LINE Foo(__FUNCTION__, __FILE__, __LINE__)
+#define EMBED_IN_ANOTHER_MACRO2 FUNC_MACRO_WITH_FILE_AND_LINE
+
+void Negatives() {
+  __func__;
+  __FUNCTION__;
+
+  // __PRETTY_FUNCTION__ should not trigger a warning because its value is
+  // actually potentially useful.
+  __PRETTY_FUNCTION__;
+  [] { __PRETTY_FUNCTION__; }();
+
+  // Don't warn if __func__/__FUNCTION is used inside a macro that also uses
+  // __FILE__ and __LINE__, on the assumption that __FILE__ and __LINE__ will
+  // be useful even if __func__/__FUNCTION__ is not.
+  [] { FUNC_MACRO_WITH_FILE_AND_LINE; }();
+  [] { FUNCTION_MACRO_WITH_FILE_AND_LINE; }();
+  [] { EMBED_IN_ANOTHER_MACRO2; }();
+}