[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

62–63

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

68–69

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

74–75

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-tidy/misc/CMakeLists.txt

   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
+  LambdaFunctionNameCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp

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:
+  LambdaFunctionNameCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_LAMBDA_FUNCTION_NAME_H

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"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void LambdaFunctionNameCheck::registerMatchers(MatchFinder *Finder) {
+  // Match on PredefinedExprs inside a lambda.
+  Finder->addMatcher(predefinedExpr(hasAncestor(lambdaExpr())).bind("E"),
+                     this);
+}
+
+void LambdaFunctionNameCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getNodeAs<PredefinedExpr>("E");
+  if (E->getIdentType() == PredefinedExpr::Func ||
+      E->getIdentType() == PredefinedExpr::Function) {
+    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());
+  }
+  // We don't care about other PredefinedExprs.
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
+#include "LambdaFunctionNameCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedConstCheck.h"

         "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");

docs/clang-tidy/checks/list.rst

    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
+   misc-lambda-function-name
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const

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

test/clang-tidy/misc-lambda-function-name.cpp

+// RUN: %check_clang_tidy %s misc-lambda-function-name %t
+
+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]
+}
+
+void Negatives() {
+  __func__;
+  __FUNCTION__;
+}