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

Summary:
This check uses the context of a subtraction expression as well as knowledge
about the Abseil Time types, to infer the type of the second operand of some
subtraction expressions in Duration conversions. For example:

   absl::ToDoubleSeconds(duration) - foo

can become
   absl::ToDoubleSeconds(duration - absl::Seconds(foo))

This ensures that time calculations are done in the proper domain, and also
makes it easier to further deduce the types of the second operands to these
expressions.

Reviewed By: JonasToth

Tags: #clang-tools-extra

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

llvm-svn: 349073

7 files changed, 215 insertions, 116 deletions

diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
index 23fc2cf2770..181400152fa 100644
--- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
+#include "DurationSubtractionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
@@ -37,6 +38,8 @@ public:
         "abseil-duration-factory-float");
     CheckFactories.registerCheck<DurationFactoryScaleCheck>(
         "abseil-duration-factory-scale");
+    CheckFactories.registerCheck<DurationSubtractionCheck>(
+        "abseil-duration-subtraction");
     CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
         "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<NoInternalDependenciesCheck>(
diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
index 34640318a22..bf9d9e849cd 100644
--- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt
@@ -7,6 +7,7 @@ add_clang_library(clangTidyAbseilModule
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
+  DurationSubtractionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp
index d5eb81b0f49..ae368edfe48 100644
--- a/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/DurationComparisonCheck.cpp
@@ -19,101 +19,6 @@ namespace clang {
 namespace tidy {
 namespace abseil {
 
-/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
-/// return its `DurationScale`, or `None` if a match is not found.
-static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
-  static const llvm::DenseMap<llvm::StringRef, DurationScale> ScaleMap(
-      {{"ToDoubleHours", DurationScale::Hours},
-       {"ToInt64Hours", DurationScale::Hours},
-       {"ToDoubleMinutes", DurationScale::Minutes},
-       {"ToInt64Minutes", DurationScale::Minutes},
-       {"ToDoubleSeconds", DurationScale::Seconds},
-       {"ToInt64Seconds", DurationScale::Seconds},
-       {"ToDoubleMilliseconds", DurationScale::Milliseconds},
-       {"ToInt64Milliseconds", DurationScale::Milliseconds},
-       {"ToDoubleMicroseconds", DurationScale::Microseconds},
-       {"ToInt64Microseconds", DurationScale::Microseconds},
-       {"ToDoubleNanoseconds", DurationScale::Nanoseconds},
-       {"ToInt64Nanoseconds", DurationScale::Nanoseconds}});
-
-  auto ScaleIter = ScaleMap.find(std::string(Name));
-  if (ScaleIter == ScaleMap.end())
-    return llvm::None;
-
-  return ScaleIter->second;
-}
-
-/// Given a `Scale` return the inverse functions for it.
-static const std::pair<llvm::StringRef, llvm::StringRef> &
-getInverseForScale(DurationScale Scale) {
-  static const std::unordered_map<DurationScale,
-                                  std::pair<llvm::StringRef, llvm::StringRef>>
-      InverseMap(
-          {{DurationScale::Hours,
-            std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours")},
-           {DurationScale::Minutes, std::make_pair("::absl::ToDoubleMinutes",
-                                                   "::absl::ToInt64Minutes")},
-           {DurationScale::Seconds, std::make_pair("::absl::ToDoubleSeconds",
-                                                   "::absl::ToInt64Seconds")},
-           {DurationScale::Milliseconds,
-            std::make_pair("::absl::ToDoubleMilliseconds",
-                           "::absl::ToInt64Milliseconds")},
-           {DurationScale::Microseconds,
-            std::make_pair("::absl::ToDoubleMicroseconds",
-                           "::absl::ToInt64Microseconds")},
-           {DurationScale::Nanoseconds,
-            std::make_pair("::absl::ToDoubleNanoseconds",
-                           "::absl::ToInt64Nanoseconds")}});
-
-  // We know our map contains all the Scale values, so we can skip the
-  // nonexistence check.
-  auto InverseIter = InverseMap.find(Scale);
-  assert(InverseIter != InverseMap.end() && "Unexpected scale found");
-  return InverseIter->second;
-}
-
-/// If `Node` is a call to the inverse of `Scale`, return that inverse's
-/// argument, otherwise None.
-static llvm::Optional<std::string>
-maybeRewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
-                                DurationScale Scale, const Expr &Node) {
-  const std::pair<std::string, std::string> &InverseFunctions =
-      getInverseForScale(Scale);
-  if (const Expr *MaybeCallArg = selectFirst<const Expr>(
-          "e", match(callExpr(callee(functionDecl(
-                                  hasAnyName(InverseFunctions.first.c_str(),
-                                             InverseFunctions.second.c_str()))),
-                              hasArgument(0, expr().bind("e"))),
-                     Node, *Result.Context))) {
-    return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
-  }
-
-  return llvm::None;
-}
-
-/// Assuming `Node` has type `double` or `int` representing a time interval of
-/// `Scale`, return the expression to make it a suitable `Duration`.
-static llvm::Optional<std::string> rewriteExprFromNumberToDuration(
-    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
-    const Expr *Node) {
-  const Expr &RootNode = *Node->IgnoreParenImpCasts();
-
-  if (RootNode.getBeginLoc().isMacroID())
-    return llvm::None;
-
-  // First check to see if we can undo a complimentary function call.
-  if (llvm::Optional<std::string> MaybeRewrite =
-          maybeRewriteInverseDurationCall(Result, Scale, RootNode))
-    return *MaybeRewrite;
-
-  if (IsLiteralZero(Result, RootNode))
-    return std::string("absl::ZeroDuration()");
-
-  return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
-          simplifyDurationFactoryArg(Result, RootNode) + ")")
-      .str();
-}
-
 void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
   auto Matcher =
       binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
diff --git a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp
index b8a0f535f34..7a4881340e3 100644
--- a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp
+++ b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp
@@ -9,6 +9,7 @@
 
 #include "DurationRewriter.h"
 #include "clang/Tooling/FixIt.h"
+#include "llvm/ADT/IndexedMap.h"
 
 using namespace clang::ast_matchers;
 
@@ -16,6 +17,13 @@ namespace clang {
 namespace tidy {
 namespace abseil {
 
+struct DurationScale2IndexFunctor {
+  using argument_type = DurationScale;
+  unsigned operator()(DurationScale Scale) const {
+    return static_cast<unsigned>(Scale);
+  }
+};
+
 /// Returns an integer if the fractional part of a `FloatingLiteral` is `0`.
 static llvm::Optional<llvm::APSInt>
 truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
@@ -29,6 +37,55 @@ truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
   return llvm::None;
 }
 
+/// Given a `Scale` return the inverse functions for it.
+static const std::pair<llvm::StringRef, llvm::StringRef> &
+getInverseForScale(DurationScale Scale) {
+  static const llvm::IndexedMap<std::pair<llvm::StringRef, llvm::StringRef>,
+                                DurationScale2IndexFunctor>
+      InverseMap = []() {
+        // TODO: Revisit the immediately invoked lamba technique when
+        // IndexedMap gets an initializer list constructor.
+        llvm::IndexedMap<std::pair<llvm::StringRef, llvm::StringRef>,
+                         DurationScale2IndexFunctor>
+            InverseMap;
+        InverseMap.resize(6);
+        InverseMap[DurationScale::Hours] =
+            std::make_pair("::absl::ToDoubleHours", "::absl::ToInt64Hours");
+        InverseMap[DurationScale::Minutes] =
+            std::make_pair("::absl::ToDoubleMinutes", "::absl::ToInt64Minutes");
+        InverseMap[DurationScale::Seconds] =
+            std::make_pair("::absl::ToDoubleSeconds", "::absl::ToInt64Seconds");
+        InverseMap[DurationScale::Milliseconds] = std::make_pair(
+            "::absl::ToDoubleMilliseconds", "::absl::ToInt64Milliseconds");
+        InverseMap[DurationScale::Microseconds] = std::make_pair(
+            "::absl::ToDoubleMicroseconds", "::absl::ToInt64Microseconds");
+        InverseMap[DurationScale::Nanoseconds] = std::make_pair(
+            "::absl::ToDoubleNanoseconds", "::absl::ToInt64Nanoseconds");
+        return InverseMap;
+      }();
+
+  return InverseMap[Scale];
+}
+
+/// If `Node` is a call to the inverse of `Scale`, return that inverse's
+/// argument, otherwise None.
+static llvm::Optional<std::string>
+rewriteInverseDurationCall(const MatchFinder::MatchResult &Result,
+                           DurationScale Scale, const Expr &Node) {
+  const std::pair<llvm::StringRef, llvm::StringRef> &InverseFunctions =
+      getInverseForScale(Scale);
+  if (const auto *MaybeCallArg = selectFirst<const Expr>(
+          "e",
+          match(callExpr(callee(functionDecl(hasAnyName(
+                             InverseFunctions.first, InverseFunctions.second))),
+                         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 getFactoryForScale(DurationScale Scale) {
   switch (Scale) {
@@ -104,6 +161,49 @@ std::string simplifyDurationFactoryArg(const MatchFinder::MatchResult &Result,
   return tooling::fixit::getText(Node, *Result.Context).str();
 }
 
+llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+  static const llvm::StringMap<DurationScale> ScaleMap(
+      {{"ToDoubleHours", DurationScale::Hours},
+       {"ToInt64Hours", DurationScale::Hours},
+       {"ToDoubleMinutes", DurationScale::Minutes},
+       {"ToInt64Minutes", DurationScale::Minutes},
+       {"ToDoubleSeconds", DurationScale::Seconds},
+       {"ToInt64Seconds", DurationScale::Seconds},
+       {"ToDoubleMilliseconds", DurationScale::Milliseconds},
+       {"ToInt64Milliseconds", DurationScale::Milliseconds},
+       {"ToDoubleMicroseconds", DurationScale::Microseconds},
+       {"ToInt64Microseconds", DurationScale::Microseconds},
+       {"ToDoubleNanoseconds", DurationScale::Nanoseconds},
+       {"ToInt64Nanoseconds", DurationScale::Nanoseconds}});
+
+  auto ScaleIter = ScaleMap.find(std::string(Name));
+  if (ScaleIter == ScaleMap.end())
+    return llvm::None;
+
+  return ScaleIter->second;
+}
+
+llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+    const Expr *Node) {
+  const Expr &RootNode = *Node->IgnoreParenImpCasts();
+
+  if (RootNode.getBeginLoc().isMacroID())
+    return llvm::None;
+
+  // First check to see if we can undo a complimentary function call.
+  if (llvm::Optional<std::string> MaybeRewrite =
+          rewriteInverseDurationCall(Result, Scale, RootNode))
+    return *MaybeRewrite;
+
+  if (IsLiteralZero(Result, RootNode))
+    return std::string("absl::ZeroDuration()");
+
+  return (llvm::Twine(getFactoryForScale(Scale)) + "(" +
+          simplifyDurationFactoryArg(Result, RootNode) + ")")
+      .str();
+}
+
 } // namespace abseil
 } // namespace tidy
 } // namespace clang
diff --git a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h
index aac241fb8bc..97103b64785 100644
--- a/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h
+++ b/clang-tools-extra/clang-tidy/abseil/DurationRewriter.h
@@ -19,34 +19,15 @@ namespace tidy {
 namespace abseil {
 
 /// Duration factory and conversion scales
-enum class DurationScale : std::int8_t {
-  Hours,
+enum class DurationScale : std::uint8_t {
+  Hours = 0,
   Minutes,
   Seconds,
   Milliseconds,
   Microseconds,
   Nanoseconds,
 };
-} // namespace abseil
-} // namespace tidy
-} // namespace clang
-
-namespace std {
-template <> struct hash<::clang::tidy::abseil::DurationScale> {
-  using argument_type = ::clang::tidy::abseil::DurationScale;
-  using underlying_type = std::underlying_type<argument_type>::type;
-  using result_type = std::hash<underlying_type>::result_type;
-
-  result_type operator()(const argument_type &arg) const {
-    std::hash<underlying_type> hasher;
-    return hasher(static_cast<underlying_type>(arg));
-  }
-};
-} // namespace std
 
-namespace clang {
-namespace tidy {
-namespace abseil {
 /// Given a `Scale`, return the appropriate factory function call for
 /// constructing a `Duration` for that scale.
 llvm::StringRef getFactoryForScale(DurationScale Scale);
@@ -78,6 +59,16 @@ std::string
 simplifyDurationFactoryArg(const ast_matchers::MatchFinder::MatchResult &Result,
                            const Expr &Node);
 
+/// Given the name of an inverse Duration function (e.g., `ToDoubleSeconds`),
+/// return its `DurationScale`, or `None` if a match is not found.
+llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name);
+
+/// Assuming `Node` has type `double` or `int` representing a time interval of
+/// `Scale`, return the expression to make it a suitable `Duration`.
+llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+    const ast_matchers::MatchFinder::MatchResult &Result, DurationScale Scale,
+    const Expr *Node);
+
 AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
                      DurationConversionFunction) {
   using namespace clang::ast_matchers;
diff --git a/clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.cpp b/clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.cpp
new file mode 100644
index 00000000000..8b85bd2ca79
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.cpp
@@ -0,0 +1,63 @@
+//===--- DurationSubtractionCheck.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 "DurationSubtractionCheck.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 DurationSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      binaryOperator(
+          hasOperatorName("-"),
+          hasLHS(callExpr(callee(functionDecl(DurationConversionFunction())
+                                     .bind("function_decl")),
+                          hasArgument(0, expr().bind("lhs_arg")))))
+          .bind("binop"),
+      this);
+}
+
+void DurationSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("function_decl");
+
+  // Don't try to replace things inside of macro definitions.
+  if (Binop->getExprLoc().isMacroID() || Binop->getExprLoc().isInvalid())
+    return;
+
+  llvm::Optional<DurationScale> Scale = getScaleForInverse(FuncDecl->getName());
+  if (!Scale)
+    return;
+
+  llvm::Optional<std::string> RhsReplacement =
+      rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
+  if (!RhsReplacement)
+    return;
+
+  const Expr *LhsArg = Result.Nodes.getNodeAs<Expr>("lhs_arg");
+
+  diag(Binop->getBeginLoc(), "perform subtraction in the duration domain")
+      << FixItHint::CreateReplacement(
+             Binop->getSourceRange(),
+             (llvm::Twine("absl::") + FuncDecl->getName() + "(" +
+              tooling::fixit::getText(*LhsArg, *Result.Context) + " - " +
+              *RhsReplacement + ")")
+                 .str());
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.h b/clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.h
new file mode 100644
index 00000000000..227dcdb5c20
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/abseil/DurationSubtractionCheck.h
@@ -0,0 +1,36 @@
+//===--- DurationSubtractionCheck.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_DURATIONSUBTRACTIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONSUBTRACTIONCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Checks for cases where subtraction should be performed in the
+/// `absl::Duration` domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-duration-subtraction.html
+class DurationSubtractionCheck : public ClangTidyCheck {
+public:
+  DurationSubtractionCheck(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_DURATIONSUBTRACTIONCHECK_H