[clang-tidy] Adding Fuchsia checker for trailing returns

[juliehockett]

Summary

Adds a check to the Fuchsia module to warn if a function has a trailing return.

See https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md for reference.

[aaron.ballman]

Can you give some background on what problem the coding standard is trying to avoid by banning this? For instance, if trailing return types are bad, are deduced return types similarly bad, or are those fine?

clang-tidy/fuchsia/TrailingReturnCheck.cpp

33–34

I think more work needs to be done here. The coding standard explicitly mentions that trailing return types are allowed when the type is un-utterable without the trailing return type.

[juliehockett]

Updating checker to not warn for trailing returns on lambdas.

[juliehockett]

In D42116#977347, @aaron.ballman wrote:

Can you give some background on what problem the coding standard is trying to avoid by banning this? For instance, if trailing return types are bad, are deduced return types similarly bad, or are those fine?

The main concern is that of readability. Deduced return types can help readability, so those are okay (within reason).

[aaron.ballman]

In D42116#977588, @juliehockett wrote:

In D42116#977347, @aaron.ballman wrote:

Can you give some background on what problem the coding standard is trying to avoid by banning this? For instance, if trailing return types are bad, are deduced return types similarly bad, or are those fine?

The main concern is that of readability. Deduced return types can help readability, so those are okay (within reason).

Thanks for the explanation. I'm a bit worried that this is going to diagnose reasonable C++11 code:

template <typename T1, typename T2>
auto fn(const T1 &lhs, const T2 &rhs) -> decltype(lhs + rhs) {
  return lhs + rhs;
}

Would it be a reasonable exception to the rule to allow a trailing return type so long as it's a decltype type specifier?

[juliehockett]

Good point -- updating checker to allow trailing returns if the decltype specifier is used.

[aaron.ballman]

clang-tidy/fuchsia/TrailingReturnCheck.cpp

24

I'd rewrite this as: return Node.getType()->castAs<FunctionProtoType>()->hasTrailingReturnType();

27–29

Can you use something like this instead?

extern const internal::VariadicDynCastAllOfMatcher<Type, DecltypeType> decltypeType;

31

The matchers should only be registered for C++11 and higher. This also fixes a bug where the hasTrailingReturn() would assert if given a K&R C function.

32

s/which/that

46

The "trailing returns" isn't worded like our other diagnostics. How about "a trailing return type is disallowed for this declaration" or something along those lines?

clang-tidy/fuchsia/TrailingReturnCheck.h

20

s/which/that

docs/ReleaseNotes.rst

77

s/which/that

docs/clang-tidy/checks/fuchsia-trailing-return.rst

7

s/which/that

[juliehockett]

Updating matchers & fixing comments

[aaron.ballman]

clang-tidy/fuchsia/TrailingReturnCheck.cpp

26

Good try, but if you rebase, you'll find that this macro has gone away entirely in r322687. I believe this macro was causing issues with ODR violations from its use in header files. I'd expand the macro manually.

[juliehockett]

Rebasing and updating decltypeType matcher

[aaron.ballman]

LGTM! If you felt so inclined, I would say those two AST matchers would both be reasonable candidates to add to ASTMatchers.h if you wanted to do that in a follow-up patch.

clang-tidy/fuchsia/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyFuchsiaModule
   DefaultArgumentsCheck.cpp
   FuchsiaTidyModule.cpp
   OverloadedOperatorCheck.cpp
   StaticallyConstructedObjectsCheck.cpp
+  TrailingReturnCheck.cpp
   VirtualInheritanceCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

 //===--- FuchsiaTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefaultArgumentsCheck.h"
 #include "OverloadedOperatorCheck.h"
 #include "StaticallyConstructedObjectsCheck.h"
+#include "TrailingReturnCheck.h"
 #include "VirtualInheritanceCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace fuchsia {
 
 /// This module is for Fuchsia-specific checks.
 class FuchsiaModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "fuchsia-default-arguments");
     CheckFactories.registerCheck<OverloadedOperatorCheck>(
         "fuchsia-overloaded-operator");
     CheckFactories.registerCheck<StaticallyConstructedObjectsCheck>(
         "fuchsia-statically-constructed-objects");
+    CheckFactories.registerCheck<TrailingReturnCheck>(
+        "fuchsia-trailing-return");
     CheckFactories.registerCheck<VirtualInheritanceCheck>(
         "fuchsia-virtual-inheritance");
   }
 };
 // Register the FuchsiaTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<FuchsiaModule>
     X("fuchsia-module", "Adds Fuchsia platform checks.");
 } // namespace fuchsia
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the FuchsiaModule.
 volatile int FuchsiaModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/fuchsia/TrailingReturnCheck.h

+//===--- TrailingReturnCheck.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_FUCHSIA_TRAILING_RETURN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_TRAILING_RETURN_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+/// Functions that have trailing returns are disallowed, except for those 
+/// using decltype specifiers and lambda with otherwise unutterable 
+/// return types.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-trailing-return.html
+class TrailingReturnCheck : public ClangTidyCheck {
+public:
+  TrailingReturnCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_TRAILING_RETURN_H

clang-tidy/fuchsia/TrailingReturnCheck.cpp

+//===--- TrailingReturnCheck.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 "TrailingReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchersInternal.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace ast_matchers {
+
+const internal::VariadicDynCastAllOfMatcher<Type, DecltypeType> decltypeType;
+
+} // namespace ast_matchers
+
+namespace tidy {
+namespace fuchsia {
+
+AST_MATCHER(FunctionDecl, hasTrailingReturn) {
+  return Node.getType()->castAs<FunctionProtoType>()->hasTrailingReturn();
+}
+
+void TrailingReturnCheck::registerMatchers(MatchFinder *Finder) {
+
+  // Requires C++11 or later.
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  // Functions that have trailing returns are disallowed, except for those
+  // using decltype specifiers and lambda with otherwise unutterable
+  // return types.
+  Finder->addMatcher(
+      functionDecl(allOf(hasTrailingReturn(),
+                         unless(anyOf(returns(decltypeType()),
+                                      hasParent(cxxRecordDecl(isLambda()))))))
+          .bind("decl"),
+      this);
+}
+
+void TrailingReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<Decl>("decl"))
+    diag(D->getLocStart(),
+         "a trailing return type is disallowed for this type of declaration");
+}
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

- New `cppcoreguidelines-avoid-goto
   only allowed in nested loops.
 
 - New `fuchsia-statically-constructed-objects
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-statically-constructed-objects.html>`_ check
 
   Warns if global, non-trivial objects with static storage are constructed, unless the 
   object is statically initialized with a ``constexpr`` constructor or has no 
   explicit constructor.
   
+- New `fuchsia-trailing-return
+  <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-trailing-return.html>`_ check
+
+  Functions that have trailing returns are disallowed, except for those 
+  using decltype specifiers and lambda with otherwise unutterable 
+  return types.
+    
 - New alias `hicpp-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/hicpp-avoid-goto.html>`_ to 
   `cppcoreguidelines-avoid-goto <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_
   added.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/fuchsia-trailing-return.rst

+.. title:: clang-tidy - fuchsia-trailing-return
+
+fuchsia-trailing-return
+=======================
+
+Functions that have trailing returns are disallowed, except for those using 
+decltype specifiers and lambda with otherwise unutterable return types.
+
+For example:
+
+.. code-block:: c++
+
+  // No warning
+  int add_one(const int arg) { return arg; }
+
+  // Warning
+  auto get_add_one() -> int (*)(const int) {
+    return add_one;
+  }
+
+Exceptions are made for lambdas and decltype specifiers:
+
+.. code-block:: c++
+  
+  // No warning
+  auto lambda = [](double x, double y) -> double {return x + y;};
+  
+  // No warning
+  template <typename T1, typename T2>
+  auto fn(const T1 &lhs, const T2 &rhs) -> decltype(lhs + rhs) {
+    return lhs + rhs;
+  }
+
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
    fuchsia-default-arguments
    fuchsia-overloaded-operator
    fuchsia-statically-constructed-objects
+   fuchsia-trailing-return
    fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-objc-avoid-throwing-exception

test/clang-tidy/fuchsia-trailing-return.cpp

+// RUN: %check_clang_tidy %s fuchsia-trailing-return %t
+
+int add_one(const int arg) { return arg; }
+
+auto get_add_one() -> int (*)(const int) {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: a trailing return type is disallowed for this type of declaration
+  // CHECK-NEXT: auto get_add_one() -> int (*)(const int) {
+  return add_one;
+}
+
+auto lambda = [](double x, double y) {return x + y;};
+
+auto lambda2 = [](double x, double y) -> double {return x + y;};
+
+int main() {
+  get_add_one()(5);
+  return 0;
+}
+
+template <typename T1, typename T2>
+auto fn(const T1 &lhs, const T2 &rhs) -> decltype(lhs + rhs) {
+  return lhs + rhs;
+}