[clang-tidy] Add the abseil-duration-conversion-cast check

[hwright]

Summary

This suggests simplifying expressions which are casting conversion functions, such as static_cast<int>(absl::ToDoubleSeconds(...))

[MyDeveloperDay]

nit's come from running running validate_check.py on the rst file from D55523: [clang-tidy] Linting .rst documentation

$ ../clang-tidy/validate_check.py --rst abseil-duration-conversion-cast.rst
Checking abseil-duration-conversion-cast.rst...
warning: line 6 maximize 80 characters by joining:'[Checks for casts of `absl::Duration` conversion functions, and recommends]' and '[the...]

warning: line 20 multiple blank lines
warning: line 28 multiple blank lines

docs/clang-tidy/checks/abseil-duration-conversion-cast.rst

21

nit: double blank line

29

nit: double blank line

32

nit: I believe that 'instead.' can come onto this line and still be <= 80 characters (which might actually be a bug in validate_check.py that it doesn't detect it)

[hwright]

Update documentation line wrapping.

docs/clang-tidy/checks/abseil-duration-conversion-cast.rst

21

This is intentional (and consistent with other examples in other tools).

29

This is intentional (and consistent with other documentation).

[Eugene.Zelenko]

clang-tidy/abseil/DurationConversionCastCheck.cpp

41

Please run Clang-format.

[hwright]

Run clang-format

[hwright]

Ping.

[aaron.ballman]

clang-tidy/abseil/DurationComparisonCheck.cpp

47–49

I think this change (and the related removal) should be landed separately as a NFC commit once this one lands, as it's logically separate from the new check.

clang-tidy/abseil/DurationConversionCastCheck.cpp

47

You can drop the llvm:: qualifiers here and elsewhere.

53

Add a full stop to the end of the comment (same below).

58

This diagnostic message does not tell the user what they did wrong with their code. How about duration should be converted directly to integer|floating-point rather than through a type cast or something along those lines?

test/clang-tidy/abseil-duration-conversion-cast.cpp

11

Can you add an example like:

typedef int FancyInt;

FancyInt j = static_cast<FancyInt>(absl::ToDoubleHours(d1));

to make sure the cast is looking through types as expected? Please add a similar test for floating-point casts.

[hwright]

Address reviewer comments

clang-tidy/abseil/DurationComparisonCheck.cpp

47–49

I've gone ahead and done the change separately, which should clean this up.

[aaron.ballman]

LGTM!

clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.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 "DurationComparisonCheck.h"
+#include "DurationConversionCastCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
 #include "DurationSubtractionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DurationComparisonCheck>(
         "abseil-duration-comparison");
+    CheckFactories.registerCheck<DurationConversionCastCheck>(
+        "abseil-duration-conversion-cast");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");
     CheckFactories.registerCheck<DurationFactoryFloatCheck>(
         "abseil-duration-factory-float");
     CheckFactories.registerCheck<DurationFactoryScaleCheck>(
         "abseil-duration-factory-scale");
     CheckFactories.registerCheck<DurationSubtractionCheck>(
         "abseil-duration-subtraction");

clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
   DurationComparisonCheck.cpp
+  DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp

clang-tidy/abseil/DurationComparisonCheck.cpp

 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
-/// Return `true` if `E` is a either: not a macro at all; or an argument to
-/// one. In the latter case, we should still transform it.
-static bool IsValidMacro(const MatchFinder::MatchResult &Result,
-                         const Expr *E) {
-  if (!E->getBeginLoc().isMacroID())
-    return true;
-
-  SourceLocation Loc = E->getBeginLoc();
-  // We want to get closer towards the initial macro typed into the source only
-  // if the location is being expanded as a macro argument.
-  while (Result.SourceManager->isMacroArgExpansion(Loc)) {
-    // We are calling getImmediateMacroCallerLoc, but note it is essentially
-    // equivalent to calling getImmediateSpellingLoc in this context according
-    // to Clang implementation. We are not calling getImmediateSpellingLoc
-    // because Clang comment says it "should not generally be used by clients."
-    Loc = Result.SourceManager->getImmediateMacroCallerLoc(Loc);
-  }
-  return !Loc.isMacroID();
-}
-
 void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
   auto Matcher =
       binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
                            hasOperatorName("=="), hasOperatorName("<="),
                            hasOperatorName("<")),
                      hasEitherOperand(ignoringImpCasts(callExpr(
                          callee(functionDecl(DurationConversionFunction())
                                     .bind("function_decl"))))))
llvm::Optional<DurationScale> Scale = getScaleForInverse(
       Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
   if (!Scale)
     return;
 
   // In most cases, we'll only need to rewrite one of the sides, but we also
   // want to handle the case of rewriting both sides. This is much simpler if
   // we unconditionally try and rewrite both, and let the rewriter determine
   // if nothing needs to be done.
-  if (!IsValidMacro(Result, Binop->getLHS()) ||
-      !IsValidMacro(Result, Binop->getRHS()))
+  if (!isNotInMacro(Result, Binop->getLHS()) ||
+      !isNotInMacro(Result, Binop->getRHS()))
     return;
   std::string LhsReplacement =
       rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
   std::string RhsReplacement =
       rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
 
   diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
       << FixItHint::CreateReplacement(Binop->getSourceRange(),
                                       (llvm::Twine(LhsReplacement) + " " +
                                        Binop->getOpcodeStr() + " " +
                                        RhsReplacement)
                                           .str());
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

clang-tidy/abseil/DurationConversionCastCheck.h

+//===--- DurationConversionCastCheck.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_ABSEIL_DURATIONCONVERSIONCASTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCONVERSIONCASTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Checks for casts of ``absl::Duration`` conversion functions, and recommends
+/// the right conversion function instead.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-conversion-cast.html
+class DurationConversionCastCheck : public ClangTidyCheck {
+public:
+  DurationConversionCastCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCONVERSIONCASTCHECK_H

clang-tidy/abseil/DurationConversionCastCheck.cpp

+//===--- DurationConversionCastCheck.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 "DurationConversionCastCheck.h"
+#include "DurationRewriter.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void DurationConversionCastCheck::registerMatchers(MatchFinder *Finder) {
+  auto CallMatcher = ignoringImpCasts(callExpr(
+      callee(functionDecl(DurationConversionFunction()).bind("func_decl")),
+      hasArgument(0, expr().bind("arg"))));
+
+  Finder->addMatcher(
+      expr(anyOf(
+          cxxStaticCastExpr(hasSourceExpression(CallMatcher)).bind("cast_expr"),
+          cStyleCastExpr(hasSourceExpression(CallMatcher)).bind("cast_expr"),
+          cxxFunctionalCastExpr(hasSourceExpression(CallMatcher))
+              .bind("cast_expr"))),
+      this);
+}
+
+void DurationConversionCastCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCast =
+      Result.Nodes.getNodeAs<ExplicitCastExpr>("cast_expr");
+
+  if (!isNotInMacro(Result, MatchedCast)) return;
+
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("func_decl");
+  const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+  llvm::StringRef ConversionFuncName = FuncDecl->getName();
+
+  llvm::Optional<DurationScale> Scale = getScaleForInverse(ConversionFuncName);
+  if (!Scale)
+    return;
+
+  // Casting a double to an integer
+  if (MatchedCast->getTypeAsWritten()->isIntegerType() &&
+      ConversionFuncName.contains("Double")) {
+    llvm::StringRef NewFuncName = getInverseForScale(*Scale).second;
+
+    diag(MatchedCast->getBeginLoc(), "convert duration directly to integer")
+        << FixItHint::CreateReplacement(
+               MatchedCast->getSourceRange(),
+               (llvm::Twine(NewFuncName.substr(2)) + "(" +
+                tooling::fixit::getText(*Arg, *Result.Context) + ")")
+                   .str());
+  }
+
+  // Casting an integer to a double
+  if (MatchedCast->getTypeAsWritten()->isRealFloatingType() &&
+      ConversionFuncName.contains("Int64")) {
+    llvm::StringRef NewFuncName = getInverseForScale(*Scale).first;
+
+    diag(MatchedCast->getBeginLoc(), "convert duration directly to double")
+        << FixItHint::CreateReplacement(
+               MatchedCast->getSourceRange(),
+               (llvm::Twine(NewFuncName.substr(2)) + "(" +
+                tooling::fixit::getText(*Arg, *Result.Context) + ")")
+                   .str());
+  }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/DurationRewriter.h

 getInverseForScale(DurationScale Scale);
 
 /// Assuming `Node` has type `double` or `int` representing a time interval of
 /// `Scale`, return the expression to make it a suitable `Duration`.
 std::string rewriteExprFromNumberToDuration(
     const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
     const Expr *Node);
 
+/// Return `true` if `E` is a either: not a macro at all; or an argument to
+/// one.  In the both cases, we often want to do the transformation.
+bool isNotInMacro(const ast_matchers::MatchFinder::MatchResult &Result,
+                  const Expr *E);
+
 AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
                      DurationConversionFunction) {
   using namespace clang::ast_matchers;
   return functionDecl(
       hasAnyName("::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
                  "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
                  "::absl::ToDoubleMicroseconds", "::absl::ToDoubleNanoseconds",
                  "::absl::ToInt64Hours", "::absl::ToInt64Minutes",

clang-tidy/abseil/DurationRewriter.cpp

std::string rewriteExprFromNumberToDuration(
   if (IsLiteralZero(Result, RootNode))
     return std::string("absl::ZeroDuration()");
 
   return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
           simplifyDurationFactoryArg(Result, RootNode) + ")")
       .str();
 }
 
+/// Return `true` if `E` is a either: not a macro at all; or an argument to
+/// one. In the latter case, we should still transform it.
+bool isNotInMacro(const MatchFinder::MatchResult &Result, const Expr *E) {
+  if (!E->getBeginLoc().isMacroID())
+    return true;
+
+  SourceLocation Loc = E->getBeginLoc();
+  // We want to get closer towards the initial macro typed into the source only
+  // if the location is being expanded as a macro argument.
+  while (Result.SourceManager->isMacroArgExpansion(Loc)) {
+    // We are calling getImmediateMacroCallerLoc, but note it is essentially
+    // equivalent to calling getImmediateSpellingLoc in this context according
+    // to Clang implementation. We are not calling getImmediateSpellingLoc
+    // because Clang comment says it "should not generally be used by clients."
+    Loc = Result.SourceManager->getImmediateMacroCallerLoc(Loc);
+  }
+  return !Loc.isMacroID();
+}
+
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

 --------------------------
 
 - New :doc:`abseil-duration-comparison
   <clang-tidy/checks/abseil-duration-comparison>` check.
 
   Checks for comparisons which should be done in the ``absl::Duration`` domain
   instead of the float of integer domains.
 
+- New :doc:`abseil-duration-conversion-cast
+  <clang-tidy/checks/abseil-duration-conversion-cast>` check.
+
+  Checks for casts of ``absl::Duration`` conversion functions, and recommends
+  the right conversion function instead.
+
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   floating-point context, and recommends the use of a function that
   returns a floating-point value.
 
 - New :doc:`abseil-duration-factory-float

docs/clang-tidy/checks/abseil-duration-conversion-cast.rst

+.. title:: clang-tidy - abseil-duration-conversion-cast
+
+abseil-duration-conversion-cast
+===============================
+
+Checks for casts of ``absl::Duration`` conversion functions, and recommends
+the right conversion function instead.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Cast from a double to an integer
+  absl::Duration d;
+  int i = static_cast<int>(absl::ToDoubleSeconds(d));
+
+  // Suggested - Use the integer conversion function directly.
+  int i = absl::ToInt64Seconds(d);
+
+
+  // Original - Cast from a double to an integer
+  absl::Duration d;
+  double x = static_cast<double>(absl::ToInt64Seconds(d));
+
+  // Suggested - Use the integer conversion function directly.
+  double x = absl::ToDoubleSeconds(d);
+
+
+Note: In the second example, the suggested fix could yield a different result,
+as the conversion to integer could truncate.  In practice, this is very rare,
+and you should use ``absl::Trunc`` to perform this operation explicitly instead.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-comparison
+   abseil-duration-conversion-cast
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-duration-subtraction
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls

test/clang-tidy/abseil-duration-conversion-cast.cpp

+// RUN: %check_clang_tidy %s abseil-duration-conversion-cast %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  absl::Duration d1;
+  double x;
+  int i;
+
+  i = static_cast<int>(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  x = static_cast<float>(absl::ToInt64Hours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleHours(d1);
+  i = static_cast<int>(absl::ToDoubleMinutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Minutes(d1);
+  x = static_cast<float>(absl::ToInt64Minutes(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMinutes(d1);
+  i = static_cast<int>(absl::ToDoubleSeconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Seconds(d1);
+  x = static_cast<float>(absl::ToInt64Seconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleSeconds(d1);
+  i = static_cast<int>(absl::ToDoubleMilliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Milliseconds(d1);
+  x = static_cast<float>(absl::ToInt64Milliseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMilliseconds(d1);
+  i = static_cast<int>(absl::ToDoubleMicroseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Microseconds(d1);
+  x = static_cast<float>(absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+  i = static_cast<int>(absl::ToDoubleNanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Nanoseconds(d1);
+  x = static_cast<float>(absl::ToInt64Nanoseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleNanoseconds(d1);
+
+  // Functional-style casts
+  i = int(absl::ToDoubleHours(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  x = float(absl::ToInt64Microseconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+
+  // C-style casts
+  i = (int) absl::ToDoubleHours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToInt64Hours(d1);
+  x = (float) absl::ToInt64Microseconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: convert duration directly to double [abseil-duration-conversion-cast]
+  // CHECK-FIXES: absl::ToDoubleMicroseconds(d1);
+
+  // Macro handling
+  // We want to transform things in macro arguments
+#define EXTERNAL(x) (x) + 5
+  i = EXTERNAL(static_cast<int>(absl::ToDoubleSeconds(d1)));
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: convert duration directly to integer [abseil-duration-conversion-cast]
+  // CHECK-FIXES: EXTERNAL(absl::ToInt64Seconds(d1));
+#undef EXTERNAL
+
+  // We don't want to transform this which get split across macro boundaries
+#define SPLIT(x) static_cast<int>((x)) + 5
+  i = SPLIT(absl::ToDoubleSeconds(d1));
+#undef SPLIT
+
+  // We also don't want to transform things inside of a macro definition
+#define INTERNAL(x) static_cast<int>(absl::ToDoubleSeconds((x))) + 5
+  i = INTERNAL(d1);
+#undef INTERNAL
+
+  // These shouldn't be converted
+  i = static_cast<int>(absl::ToInt64Seconds(d1));
+  i = static_cast<float>(absl::ToDoubleSeconds(d1));
+}