[clang-tidy] Add the abseil-time-subtraction check

Differential Revision: https://reviews.llvm.org/D58137

llvm-svn: 355024

clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp

@@ -23,6 +23,7 @@
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
+#include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
@@ -59,6 +60,8 @@ class AbseilModule : public ClangTidyModule {
         "abseil-str-cat-append");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
+    CheckFactories.registerCheck<TimeSubtractionCheck>(
+        "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
         "abseil-upgrade-duration-conversions");
   }

clang-tools-extra/clang-tidy/abseil/CMakeLists.txt

@@ -17,6 +17,7 @@ add_clang_library(clangTidyAbseilModule
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
+  TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 
   LINK_LIBS

clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp

@@ -84,6 +84,22 @@ rewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
   return llvm::None;
 }
 
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+rewriteInverseTimeCall(const MatchFinder::MatchResult &Result,
+                       DurationScale Scale, const Expr &Node) {
+  llvm::StringRef InverseFunction = getTimeInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst<const Expr>(
+          "e", match(callExpr(callee(functionDecl(hasName(InverseFunction))),
+                              hasArgument(0, expr().bind("e"))),
+                     Node, *Result.Context))) {
+    return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+  }
+
+  return llvm::None;
+}
+
 /// Returns the factory function name for a given `Scale`.
 llvm::StringRef getDurationFactoryForScale(DurationScale Scale) {
   switch (Scale) {
@@ -103,6 +119,24 @@ llvm::StringRef getDurationFactoryForScale(DurationScale Scale) {
   llvm_unreachable("unknown scaling factor");
 }
 
+llvm::StringRef getTimeFactoryForScale(DurationScale Scale) {
+  switch (Scale) {
+  case DurationScale::Hours:
+    return "absl::FromUnixHours";
+  case DurationScale::Minutes:
+    return "absl::FromUnixMinutes";
+  case DurationScale::Seconds:
+    return "absl::FromUnixSeconds";
+  case DurationScale::Milliseconds:
+    return "absl::FromUnixMillis";
+  case DurationScale::Microseconds:
+    return "absl::FromUnixMicros";
+  case DurationScale::Nanoseconds:
+    return "absl::FromUnixNanos";
+  }
+  llvm_unreachable("unknown scaling factor");
+}
+
 /// Returns the Time factory function name for a given `Scale`.
 llvm::StringRef getTimeInverseForScale(DurationScale scale) {
   switch (scale) {
@@ -250,6 +284,24 @@ std::string rewriteExprFromNumberToDuration(
       .str();
 }
 
+std::string rewriteExprFromNumberToTime(
+    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+    const Expr *Node) {
+  const Expr &RootNode = *Node->IgnoreParenImpCasts();
+
+  // First check to see if we can undo a complimentary function call.
+  if (llvm::Optional<std::string> MaybeRewrite =
+          rewriteInverseTimeCall(Result, Scale, RootNode))
+    return *MaybeRewrite;
+
+  if (IsLiteralZero(Result, RootNode))
+    return std::string("absl::UnixEpoch()");
+
+  return (llvm::Twine(getTimeFactoryForScale(Scale)) + "(" +
+          tooling::fixit::getText(RootNode, *Result.Context) + ")")
+      .str();
+}
+
 bool isNotInMacro(const MatchFinder::MatchResult &Result, const Expr *E) {
   if (!E->getBeginLoc().isMacroID())
     return true;

clang-tools-extra/clang-tidy/abseil/DurationRewriter.h

@@ -31,6 +31,10 @@ enum class DurationScale : std::uint8_t {
 /// constructing a `Duration` for that scale.
 llvm::StringRef getDurationFactoryForScale(DurationScale Scale);
 
+/// Given a 'Scale', return the appropriate factory function call for
+/// constructing a `Time` for that scale.
+llvm::StringRef getTimeFactoryForScale(DurationScale scale);
+
 // Determine if `Node` represents a literal floating point or integral zero.
 bool IsLiteralZero(const ast_matchers::MatchFinder::MatchResult &Result,
                    const Expr &Node);
@@ -81,6 +85,12 @@ std::string rewriteExprFromNumberToDuration(
     const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
     const Expr *Node);
 
+/// Assuming `Node` has a type `int` representing a time instant of `Scale`
+/// since The Epoch, return the expression to make it a suitable `Time`.
+std::string rewriteExprFromNumberToTime(
+    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,

clang-tools-extra/clang-tidy/abseil/TimeSubtractionCheck.cpp

@@ -0,0 +1,181 @@
+//===--- TimeSubtractionCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TimeSubtractionCheck.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 {
+
+// Returns `true` if `Range` is inside a macro definition.
+static bool InsideMacroDefinition(const MatchFinder::MatchResult &Result,
+                                  SourceRange Range) {
+  return !clang::Lexer::makeFileCharRange(
+              clang::CharSourceRange::getCharRange(Range),
+              *Result.SourceManager, Result.Context->getLangOpts())
+              .isValid();
+}
+
+static bool isConstructorAssignment(const MatchFinder::MatchResult &Result,
+                                    const Expr *Node) {
+  return selectFirst<const Expr>(
+             "e", match(expr(hasParent(materializeTemporaryExpr(hasParent(
+                                 cxxConstructExpr(hasParent(exprWithCleanups(
+                                     hasParent(varDecl()))))))))
+                            .bind("e"),
+                        *Node, *Result.Context)) != nullptr;
+}
+
+static bool isArgument(const MatchFinder::MatchResult &Result,
+                       const Expr *Node) {
+  return selectFirst<const Expr>(
+             "e",
+             match(expr(hasParent(
+                            materializeTemporaryExpr(hasParent(cxxConstructExpr(
+                                hasParent(callExpr()),
+                                unless(hasParent(cxxOperatorCallExpr())))))))
+                       .bind("e"),
+                   *Node, *Result.Context)) != nullptr;
+}
+
+static bool isReturn(const MatchFinder::MatchResult &Result, const Expr *Node) {
+  return selectFirst<const Expr>(
+             "e", match(expr(hasParent(materializeTemporaryExpr(hasParent(
+                                 cxxConstructExpr(hasParent(exprWithCleanups(
+                                     hasParent(returnStmt()))))))))
+                            .bind("e"),
+                        *Node, *Result.Context)) != nullptr;
+}
+
+static bool parensRequired(const MatchFinder::MatchResult &Result,
+                           const Expr *Node) {
+  // TODO: Figure out any more contexts in which we can omit the surrounding
+  // parentheses.
+  return !(isConstructorAssignment(Result, Node) || isArgument(Result, Node) ||
+           isReturn(Result, Node));
+}
+
+void TimeSubtractionCheck::emitDiagnostic(const Expr *Node,
+                                          llvm::StringRef Replacement) {
+  diag(Node->getBeginLoc(), "perform subtraction in the time domain")
+      << FixItHint::CreateReplacement(Node->getSourceRange(), Replacement);
+}
+
+void TimeSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  for (auto ScaleName :
+       {"Hours", "Minutes", "Seconds", "Millis", "Micros", "Nanos"}) {
+    std::string TimeInverse = (llvm::Twine("ToUnix") + ScaleName).str();
+    llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(TimeInverse);
+    assert(Scale && "Unknow scale encountered");
+
+    auto TimeInverseMatcher = callExpr(callee(
+        functionDecl(hasName((llvm::Twine("::absl::") + TimeInverse).str()))
+            .bind("func_decl")));
+
+    // Match the cases where we know that the result is a 'Duration' and the
+    // first argument is a 'Time'. Just knowing the type of the first operand
+    // is not sufficient, since the second operand could be either a 'Time' or
+    // a 'Duration'. If we know the result is a 'Duration', we can then infer
+    // that the second operand must be a 'Time'.
+    auto CallMatcher =
+        callExpr(
+            callee(functionDecl(hasName(getDurationFactoryForScale(*Scale)))),
+            hasArgument(0, binaryOperator(hasOperatorName("-"),
+                                          hasLHS(TimeInverseMatcher))
+                               .bind("binop")))
+            .bind("outer_call");
+    Finder->addMatcher(CallMatcher, this);
+
+    // Match cases where we know the second operand is a 'Time'. Since
+    // subtracting a 'Time' from a 'Duration' is not defined, in these cases,
+    // we always know the first operand is a 'Time' if the second is a 'Time'.
+    auto OperandMatcher =
+        binaryOperator(hasOperatorName("-"), hasRHS(TimeInverseMatcher))
+            .bind("binop");
+    Finder->addMatcher(OperandMatcher, this);
+  }
+}
+
+void TimeSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  std::string InverseName =
+      Result.Nodes.getNodeAs<FunctionDecl>("func_decl")->getNameAsString();
+  if (InsideMacroDefinition(Result, BinOp->getSourceRange()))
+    return;
+
+  llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(InverseName);
+  if (!Scale)
+    return;
+
+  const auto *OuterCall = Result.Nodes.getNodeAs<CallExpr>("outer_call");
+  if (OuterCall) {
+    if (InsideMacroDefinition(Result, OuterCall->getSourceRange()))
+      return;
+
+    // We're working with the first case of matcher, and need to replace the
+    // entire 'Duration' factory call. (Which also means being careful about
+    // our order-of-operations and optionally putting in some parenthesis.
+    bool NeedParens = parensRequired(Result, OuterCall);
+
+    emitDiagnostic(
+        OuterCall,
+        (llvm::Twine(NeedParens ? "(" : "") +
+         rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) + " - " +
+         rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) +
+         (NeedParens ? ")" : ""))
+            .str());
+  } else {
+    // We're working with the second case of matcher, and either just need to
+    // change the arguments, or perhaps remove an outer function call. In the
+    // latter case (addressed first), we also need to worry about parenthesis.
+    const auto *MaybeCallArg = selectFirst<const CallExpr>(
+        "arg", match(expr(hasAncestor(
+                         callExpr(callee(functionDecl(hasName(
+                                      getDurationFactoryForScale(*Scale)))))
+                             .bind("arg"))),
+                     *BinOp, *Result.Context));
+    if (MaybeCallArg && MaybeCallArg->getArg(0)->IgnoreImpCasts() == BinOp &&
+        !InsideMacroDefinition(Result, MaybeCallArg->getSourceRange())) {
+      // Handle the case where the matched expression is inside a call which
+      // converts it from the inverse to a Duration.  In this case, we replace
+      // the outer with just the subtraction expresison, which gives the right
+      // type and scale, taking care again about parenthesis.
+      bool NeedParens = parensRequired(Result, MaybeCallArg);
+
+      emitDiagnostic(
+          MaybeCallArg,
+          (llvm::Twine(NeedParens ? "(" : "") +
+           rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) +
+           " - " +
+           rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) +
+           (NeedParens ? ")" : ""))
+              .str());
+    } else {
+      // In the last case, just convert the arguments and wrap the result in
+      // the correct inverse function.
+      emitDiagnostic(
+          BinOp,
+          (llvm::Twine(
+               getDurationInverseForScale(*Scale).second.str().substr(2)) +
+           "(" + rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) +
+           " - " +
+           rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) + ")")
+              .str());
+    }
+  }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/abseil/TimeSubtractionCheck.h

@@ -0,0 +1,38 @@
+//===--- TimeSubtractionCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMESUBTRACTIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMESUBTRACTIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Finds and fixes `absl::Time` subtraction expressions to do subtraction
+/// in the time domain instead of the numeric domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-time-subtraction.html
+class TimeSubtractionCheck : public ClangTidyCheck {
+public:
+  TimeSubtractionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void emitDiagnostic(const Expr* Node, llvm::StringRef Replacement);
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMESUBTRACTIONCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -85,6 +85,12 @@ Improvements to clang-tidy
   Finds and fixes cases where ``absl::Duration`` values are being converted to
   numeric types and back again.
 
+- New :doc:`abseil-time-subtraction
+  <clang-tidy/checks/abseil-time-subtraction>` check.
+
+  Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
+  in the Time domain instead of the numeric domain.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.

clang-tools-extra/docs/clang-tidy/checks/abseil-time-subtraction.rst

@@ -0,0 +1,37 @@
+.. title:: clang-tidy - abseil-time-subtraction
+
+abseil-time-subtraction
+=======================
+
+Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
+in the Time domain instead of the numeric domain.
+
+There are two cases of Time subtraction in which deduce additional type
+information:
+ - When the result is an ``absl::Duration`` and the first argument is an
+   ``absl::Time``.
+ - When the second argument is a ``absl::Time``.
+
+In the first case, we must know the result of the operation, since without that
+the second operand could be either an ``absl::Time`` or an ``absl::Duration``.
+In the second case, the first operand *must* be an ``absl::Time``, because
+subtracting an ``absl::Time`` from an ``absl::Duration`` is not defined.
+
+Examples:
+
+.. code-block:: c++
+  int x;
+  absl::Time t;
+
+  // Original - absl::Duration result and first operand is a absl::Time.
+  absl::Duration d = absl::Seconds(absl::ToUnixSeconds(t) - x);
+
+  // Suggestion - Perform subtraction in the Time domain instead.
+  absl::Duration d = t - absl::FromUnixSeconds(x);
+
+
+  // Original - Second operand is an absl::Time.
+  int i = x - absl::ToUnixSeconds(t);
+
+  // Suggestion - Perform subtraction in the Time domain instead.
+  int i = absl::ToInt64Seconds(absl::FromUnixSeconds(x) - t);