[clang-tidy] Enhance modernize-use-auto to templated function casts

[malcolm.parsons]

Summary

Use auto when declaring variables that are initialized by calling a templated
function that returns its explicit first argument.

Fixes PR26763.

[Eugene.Zelenko]

It'll be worth to mention enhancement in Release Notes.

[alexfh]

clang-tidy/modernize/UseAutoCheck.cpp

173–177

Ideally, this should go to ASTMatchers.h (with a proper test and documentation).

259

It might make sense to use a more specific identifier than "a" to avoid collisions.

[malcolm.parsons]

Handle templated member functions too.

[malcolm.parsons]

In D27166#606772, @Eugene.Zelenko wrote:

It'll be worth to mention enhancement in Release Notes.

They already say this:

• The modernize-use-auto <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>_ check now warns about variable declarations that are initialized with a cast.

Does that cover it?

[Eugene.Zelenko]

In D27166#608339, @malcolm.parsons wrote:

In D27166#606772, @Eugene.Zelenko wrote:

It'll be worth to mention enhancement in Release Notes.

They already say this:

• The modernize-use-auto <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>_ check now warns about variable declarations that are initialized with a cast.

Does that cover it?

I'm not very sure, because cats may be interpreted as C++ language constructs only, but not framework provided ones.

[malcolm.parsons]

Add to release notes

[malcolm.parsons]

ping.

[malcolm.parsons]

Remove hasReplacementType matcher.

[Prazek]

gosh, I found it too late. Jakub started to work on the same problem and he have made initial check for it.
One of the problems he got was multideclarations.
Does it work for cases like?

long a = lexical_cast<long>(z), b = lexical_cast<int>(y);

Other test:

Base *p = dyn_cast<Derived>(b);

[malcolm.parsons]

In D27166#617621, @Prazek wrote:.

Does it work for cases like?

Yes; replaceExpr() checks that the variable has the same unqualified type as the initializer and the same canonical type as other variables in the declaration.

[Prazek]

In D27166#617696, @malcolm.parsons wrote:

In D27166#617621, @Prazek wrote:.

Does it work for cases like?

Yes; replaceExpr() checks that the variable has the same unqualified type as the initializer and the same canonical type as other variables in the declaration.

Can you add this small test?

clang-tidy/modernize/UseAutoCheck.cpp

173–177

I agree

262–270

Can you split this matcher into 3 matchers? It is so large that even clang-format doesn't help with it.
And also because of that I can't come up with good auto variables to use here, because I don't know where the parens ends.

docs/clang-tidy/checks/modernize-use-auto.rst

165

I would add to that what functions are considered (functions returning type chosen as first template parameter or something similar). Now someone could think that there is hardcoded list somewhere.

[malcolm.parsons]

clang-tidy/modernize/UseAutoCheck.cpp

173–177

@Prazek Are you talking about hasReplacementType or hasExplicitTemplateArgs?

[malcolm.parsons]

Split up matcher.
Add tests.
Improve doc.

[Prazek]

Cool!
Have you run this check on LLVM & Clang to check if it works?
I just runned it and I got this weird behavior

lib/Analysis/AssumptionCache.cpp:120
-    for (const BasicBlock &B : cast<Function>(*I.first))
+    for (const BasicBlock &B : auto<Function>(*I.first))

[malcolm.parsons]

In D27166#622072, @Prazek wrote:

Have you run this check on LLVM & Clang to check if it works?

I ran it on some of clang-tools-extra:

unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp:17
-    const VarDecl *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+    const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");

lib/Analysis/AssumptionCache.cpp:120
-    for (const BasicBlock &B : cast<Function>(*I.first))
+    for (const BasicBlock &B : auto<Function>(*I.first))

The check should be ignoring implicit VarDecls.

[malcolm.parsons]

Ignore implicit VarDecls

[Prazek]

There is still one more problem:

/home/prazek/llvm/lib/Analysis/ScalarEvolution.cpp:2442:11: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
  const auto **O = SCEVAllocator.Allocate<const SCEV *>(Ops.size());
        ^
        auto

The same thing for normal casts, so I guess it is not only problem with this patch

/home/prazek/llvm/lib/IR/User.cpp:156:3: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
auto **HungOffOperandList = static_cast<Use **>(Storage);
^
auto

There is also problem with function pointers

/home/prazek/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp:520:9: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
      int (*PF)(int, char **, const char **) =
      ^
      auto

So these problems occur for cast like functions and for cast. Do you see simple fix for it?

[malcolm.parsons]

In D27166#622103, @Prazek wrote:

There is still one more problem:

/home/prazek/llvm/lib/Analysis/ScalarEvolution.cpp:2442:11: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
  const auto **O = SCEVAllocator.Allocate<const SCEV *>(Ops.size());
        ^
        auto

Any suggestions for rewriting this matcher?

// Skip declarations that are already using auto.
unless(has(varDecl(anyOf(hasType(autoType()),
                         hasType(pointerType(pointee(autoType()))),
                         hasType(referenceType(pointee(autoType())))))))

There is also problem with function pointers

/home/prazek/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp:520:9: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
      int (*PF)(int, char **, const char **) =
      ^
      auto

The warning is correct, but the fixit is wrong.
Suppress fixit for function pointers?

[malcolm.parsons]

Use qualType(hasDescendant(autoType())) to fix skipping of declarations that
are already using auto.

[Prazek]

In D27166#622108, @malcolm.parsons wrote:

In D27166#622103, @Prazek wrote:

There is still one more problem:

/home/prazek/llvm/lib/Analysis/ScalarEvolution.cpp:2442:11: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
  const auto **O = SCEVAllocator.Allocate<const SCEV *>(Ops.size());
        ^
        auto

Any suggestions for rewriting this matcher?

// Skip declarations that are already using auto.
unless(has(varDecl(anyOf(hasType(autoType()),
                         hasType(pointerType(pointee(autoType()))),
                         hasType(referenceType(pointee(autoType())))))))

There is also problem with function pointers

/home/prazek/llvm/lib/ExecutionEngine/MCJIT/MCJIT.cpp:520:9: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
      int (*PF)(int, char **, const char **) =
      ^
      auto

The warning is correct, but the fixit is wrong.
Suppress fixit for function pointers?

Yep, it is not worth fixing it. Have you add test cases to the cases that we discussed?

clang-tidy/modernize/UseAutoCheck.cpp

173–177

It is good now.

[malcolm.parsons]

In D27166#622127, @Prazek wrote:

In D27166#622108, @malcolm.parsons wrote:

In D27166#622103, @Prazek wrote:

There is also problem with function pointers

The warning is correct, but the fixit is wrong.
Suppress fixit for function pointers?

Yep, it is not worth fixing it.

OK.

Have you add test cases to the cases that we discussed?

I'll try to add some test cases tonight.

[malcolm.parsons]

Add tests for implicit vars and pointers to pointers to auto.

[Prazek]

LGTM, thanks for all the fixes.
Can you leave FIXIT comment somewhere in the code near the FixitHint about the function pointers?

[malcolm.parsons]

Add FIXME comment.

clang-tidy/modernize/UseAutoCheck.cpp

 namespace clang {
 namespace tidy {
 namespace modernize {
 namespace {
 
 const char IteratorDeclStmtId[] = "iterator_decl";
 const char DeclWithNewId[] = "decl_new";
 const char DeclWithCastId[] = "decl_cast";
+const char DeclWithTemplateCastId[] = "decl_template";
 
 /// \brief Matches variable declarations that have explicit initializers that
 /// are not initializer lists.
 ///
 /// Given
 /// \code
 ///   iterator I = Container.begin();
 ///   MyType A(42);
AST_MATCHER(Decl, isFromStdNamespace) {
   if (!D->isNamespace() || !D->getParent()->isTranslationUnit())
     return false;
 
   const IdentifierInfo *Info = cast<NamespaceDecl>(D)->getIdentifier();
 
   return (Info && Info->isStr("std"));
 }
 
+/// Matches the type that was substituted for the template parameter.
+AST_MATCHER_P(SubstTemplateTypeParmType, hasReplacementType,
+              ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
+  return InnerMatcher.matches(Node.getReplacementType(), Finder, Builder);
+}
+
+/// Matches declaration reference or member expressions with explicit template
+/// arguments.
+AST_POLYMORPHIC_MATCHER(hasExplicitTemplateArgs,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(DeclRefExpr,
+                                                        MemberExpr)) {
+  return Node.hasExplicitTemplateArgs();
+}
+
 /// \brief Returns a DeclarationMatcher that matches standard iterators nested
 /// inside records with a standard container name.
 DeclarationMatcher standardIterator() {
   return allOf(
       namedDecl(hasStdIteratorName()),
       hasDeclContext(recordDecl(hasStdContainerName(), isFromStdNamespace())));
 }
 

 }
 
 StatementMatcher makeDeclWithCastMatcher() {
   return declStmt(
              unless(has(varDecl(unless(hasInitializer(explicitCastExpr()))))))
       .bind(DeclWithCastId);
 }
 
+StatementMatcher makeDeclWithTemplateCastMatcher() {
+  auto ST =
+      substTemplateTypeParmType(hasReplacementType(equalsBoundNode("arg")));
+
+  return declStmt(
+             unless(has(varDecl(unless(hasInitializer(ignoringImplicit(callExpr(
+                 anyOf(has(memberExpr(hasExplicitTemplateArgs())),
+                       has(ignoringImpCasts(
+                           declRefExpr(hasExplicitTemplateArgs())))),
+                 callee(functionDecl(
+                     hasTemplateArgument(0,
+                                         refersToType(qualType().bind("arg"))),
+                     returns(anyOf(ST, pointsTo(ST), references(ST)))))))))))))
+      .bind(DeclWithTemplateCastId);
+}
+
 StatementMatcher makeCombinedMatcher() {
   return declStmt(
       // At least one varDecl should be a child of the declStmt to ensure
       // it's a declaration list and avoid matching other declarations,
       // e.g. using directives.
       has(varDecl()),
       // Skip declarations that are already using auto.
       unless(has(varDecl(anyOf(hasType(autoType()),
                                hasType(pointerType(pointee(autoType()))),
                                hasType(referenceType(pointee(autoType()))))))),
       anyOf(makeIteratorDeclMatcher(), makeDeclWithNewMatcher(),
-            makeDeclWithCastMatcher()));
+            makeDeclWithCastMatcher(), makeDeclWithTemplateCastMatcher()));
 }
 
 } // namespace
 
 UseAutoCheck::UseAutoCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       RemoveStars(Options.get("RemoveStars", 0)) {}
 
replaceExpr(Decl, Result.Context,
                  Result.Nodes.getNodeAs<DeclStmt>(DeclWithCastId)) {
     replaceExpr(
         Decl, Result.Context,
         [](const Expr *Expr) {
           return cast<ExplicitCastExpr>(Expr)->getTypeAsWritten();
         },
         "use auto when initializing with a cast to avoid duplicating the type "
         "name");
+  } else if (const auto *Decl =
+                 Result.Nodes.getNodeAs<DeclStmt>(DeclWithTemplateCastId)) {
+    replaceExpr(
+        Decl, Result.Context,
+        [](const Expr *Expr) {
+          return cast<CallExpr>(Expr->IgnoreImplicit())
+              ->getDirectCallee()
+              ->getReturnType();
+        },
+        "use auto when initializing with a template cast to avoid duplicating "
+        "the type name");
   } else {
     llvm_unreachable("Bad Callback. No node provided.");
   }
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

 - `modernize-make-unique
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-unique.html>`_
   and `modernize-make-shared
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-make-shared.html>`_
   now handle calls to the smart pointer's ``reset()`` method.
 
 - The `modernize-use-auto
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html>`_ check
-  now warns about variable declarations that are initialized with a cast.
+  now warns about variable declarations that are initialized with a cast, or by
+  calling a templated function that behaves as a cast.
 
 - New `modernize-use-equals-delete
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-delete.html>`_ check
 
   Adds ``= delete`` to unimplemented private special member functions.
 
 - New `modernize-use-transparent-functors
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-transparent-functors.html>`_ check

docs/clang-tidy/checks/modernize-use-auto.rst

.. code-block:: c++
 
   TypeName *my_pointer = static_cast<TypeName>(my_param);
 
   // becomes
 
   auto *my_pointer = static_cast<TypeName>(my_param);
 
 The check handles ``static_cast``, ``dynamic_cast``, ``const_cast``,
-``reinterpret_cast``, functional casts and C-style casts.
+``reinterpret_cast``, functional casts, C-style casts and function templates
+that behave as casts, such as ``llvm::dyn_cast``, ``boost::lexical_cast`` and
+``gsl::narrow_cast``.
 
 Known Limitations
 -----------------
 
 * If the initializer is an explicit conversion constructor, the check will not
   replace the type specifier even though it would be safe to do so.
 
 * User-defined iterators are not handled at this time.
 
-* Function templates that behave as casts, such as ``llvm::dyn_cast``,
-  ``boost::lexical_cast`` or ``gsl::narrow_cast`` are not handled.
-
 Options
 -------
 
 .. option:: RemoveStars
 
    If the option is set to non-zero (default is `0`), the check will remove
    stars from the non-typedef pointer types when replacing type names with
    ``auto``. Otherwise, the check will leave stars. For example:

test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp

void f_functional_cast() {
   long l = 1;
   int i1 = int(l);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a cast to avoid duplicating the type name
   // CHECK-FIXES: auto  i1 = int(l);
 
   B b;
   A a = A(b);
 }
+
+class StringRef
+{
+public:
+  StringRef(const char *);
+};
+
+template <typename T, typename U>
+T template_value_cast(const U &u);
+
+template <typename T, typename U>
+T *template_pointer_cast(U *u);
+
+template <typename T, typename U>
+T &template_reference_cast(U &u);
+
+template <typename T, typename U>
+const T *template_const_pointer_cast(const U *u);
+
+template <typename T, typename U>
+const T &template_const_reference_cast(const U &u);
+
+template <typename T>
+T template_value_get(StringRef s);
+
+template <typename T>
+T max(T t1, T t2);
+
+void f_template_cast()
+{
+  double d = 0;
+  int i1 = template_value_cast<int>(d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto  i1 = template_value_cast<int>(d);
+
+  A *a = new B();
+  B *b1 = template_value_cast<B *>(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto b1 = template_value_cast<B *>(a);
+  B &b2 = template_value_cast<B &>(*a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto  &b2 = template_value_cast<B &>(*a);
+  B *b3 = template_pointer_cast<B>(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto b3 = template_pointer_cast<B>(a);
+  B &b4 = template_reference_cast<B>(*a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto  &b4 = template_reference_cast<B>(*a);
+  const B *b5 = template_const_pointer_cast<B>(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: const auto b5 = template_const_pointer_cast<B>(a);
+  const B &b6 = template_const_reference_cast<B>(*a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: const auto  &b6 = template_const_reference_cast<B>(*a);
+  B *b7 = template_value_get<B *>("foo");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto b7 = template_value_get<B *>("foo");
+
+  auto i2 = template_value_cast<int>(d);
+  int i3 = max(i1, i2);
+}

test/clang-tidy/modernize-use-auto-cast.cpp

void f_functional_cast() {
   long l = 1;
   int i1 = int(l);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a cast to avoid duplicating the type name
   // CHECK-FIXES: auto i1 = int(l);
 
   B b;
   A a = A(b);
 }
+
+class StringRef
+{
+public:
+  StringRef(const char *);
+};
+
+template <typename T, typename U>
+T template_value_cast(const U &u);
+
+template <typename T, typename U>
+T *template_pointer_cast(U *u);
+
+template <typename T, typename U>
+T &template_reference_cast(U &u);
+
+template <typename T, typename U>
+const T *template_const_pointer_cast(const U *u);
+
+template <typename T, typename U>
+const T &template_const_reference_cast(const U &u);
+
+template <typename T>
+T template_value_get(StringRef s);
+
+struct S {
+  template <typename T>
+  const T *template_member_get();
+};
+
+template <typename T>
+T max(T t1, T t2);
+
+void f_template_cast()
+{
+  double d = 0;
+  int i1 = template_value_cast<int>(d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto i1 = template_value_cast<int>(d);
+
+  A *a = new B();
+  B *b1 = template_value_cast<B *>(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto *b1 = template_value_cast<B *>(a);
+  B &b2 = template_value_cast<B &>(*a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto &b2 = template_value_cast<B &>(*a);
+  B *b3 = template_pointer_cast<B>(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto *b3 = template_pointer_cast<B>(a);
+  B &b4 = template_reference_cast<B>(*a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto &b4 = template_reference_cast<B>(*a);
+  const B *b5 = template_const_pointer_cast<B>(a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: const auto *b5 = template_const_pointer_cast<B>(a);
+  const B &b6 = template_const_reference_cast<B>(*a);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: const auto &b6 = template_const_reference_cast<B>(*a);
+  B *b7 = template_value_get<B *>("foo");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a template cast to avoid duplicating the type name
+  // CHECK-FIXES: auto *b7 = template_value_get<B *>("foo");
+
+  S s;
+  const B *b8 = s.template_member_get<B>();
+
+  auto i2 = template_value_cast<int>(d);
+  int i3 = max(i1, i2);
+}