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

[Eugene.Zelenko]

docs/ReleaseNotes.rst

91

Please use two not one ` for language constructs.

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

11

Will be good idea to always use absl:: prefix. Same in other places.

25

` doesn't make sense in comments. Same below.

[JonasToth]

clang-tidy/abseil/DurationRewriter.cpp

92

nit: stringref should be used as value all the time (as its a pointer itself).

clang-tidy/abseil/TimeSubtractionCheck.cpp

66

wouldn't that be rather a bug condition? I think assert is better in that case

73

please highlight the types with ''

98

Could you please split this function up into smaller ones. There are three or four distinct cases that are easier to comprehend in isolation.

99

nit: InverseName

109

please elimate the multiple blanks

clang-tidy/abseil/TimeSubtractionCheck.h

20

nit: 'Time' domain

test/clang-tidy/abseil-time-subtraction.cpp

13

please add tests where x itself is a calculation with different precedence of its operators (multiplication, addition) to ensure these cases are transformed properly as well.

79

please add tests for templates and macros.

[Eugene.Zelenko]

clang-tidy/abseil/TimeSubtractionCheck.cpp

73

But do we do this in comments? Isn't it's Sphinx syntax?

[JonasToth]

clang-tidy/abseil/TimeSubtractionCheck.cpp

73

We don't do it consistently, but its common to highlight direct usage of types in comments as well.
Sphinx uses backticks, but in comments any form of ticks are ok.

[hwright]

clang-tidy/abseil/TimeSubtractionCheck.cpp

98

The actual bodies of these if-statements are only one or two separate statements themselves. Moving those to separate functions seems like it would just obfuscate things a bit.

clang-tidy/abseil/TimeSubtractionCheck.h

20

This doesn't refer to a type, but a library system, so it probably isn't appropriate to quote it.

(Just has how one wouldn't quote "frequency" when talking about "the frequency domain" of a Fourier transform.)

test/clang-tidy/abseil-time-subtraction.cpp

13

This doesn't actually matter in this case: x will be wrapped in a function call.

It does matter in the case where we unwrap the first argument (below) and I've already got a test which uses multiplication in this case. I've also added one for division.

79

I've add tests for macros, though I'm not sure what cases you have in mind regarding templates.

[JonasToth]

clang-tidy/abseil/TimeSubtractionCheck.cpp

98

IMHO they are complicated statements and hide what is being done. Wrapping them in a function with a name that states what is done seems appropriate.

clang-tidy/abseil/TimeSubtractionCheck.h

20

ah true, but then time would be written small i guess.

test/clang-tidy/abseil-time-subtraction.cpp

13

Yes, it should not matter if x is an expr itself or just a variable. Thats why it should be tested its actually true.

[hwright]

clang-tidy/abseil/TimeSubtractionCheck.cpp

98

I would agree that they are complicated statements, which is why there are multi-line comments explaining what is being doing. Moving a two-line compound statement into a separate function which is only called once seems more confusing than simplifying. More information can be expressed in a prose comment than a single concise function name, no?

test/clang-tidy/abseil-time-subtraction.cpp

13

Added, though this seems more a test of the matcher infrastructure than the tool itself.

[hwright]

clang-tidy/abseil/TimeSubtractionCheck.cpp

98

I've pulled out the common duplicated functionality which actually emits the diagnostic, and I think that's simplified these branches a bit more.

[JonasToth]

I am happy now :)
Thank you for the patch, LGTM

test/clang-tidy/abseil-time-subtraction.cpp

13

True, but there are always some inconsistencies, so better to test what we want :)

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "DurationSubtractionCheck.h"
 #include "DurationUnnecessaryConversionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
+#include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
CheckFactories.registerCheck<NoInternalDependenciesCheck>(
         "abseil-no-internal-dependencies");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
     CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
         "abseil-redundant-strcat-calls");
     CheckFactories.registerCheck<StrCatAppendCheck>(
         "abseil-str-cat-append");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
+    CheckFactories.registerCheck<TimeSubtractionCheck>(
+        "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
         "abseil-upgrade-duration-conversions");
   }
 };
 
 // Register the AbseilModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AbseilModule> X("abseil-module",
                                                     "Add Abseil checks.");

clang-tidy/abseil/CMakeLists.txt

add_clang_library(clangTidyAbseilModule
   DurationSubtractionCheck.cpp
   DurationUnnecessaryConversionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
+  TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/abseil/DurationRewriter.h

enum class DurationScale : std::uint8_t {
   Microseconds,
   Nanoseconds,
 };
 
 /// Given a `Scale`, return the appropriate factory function call for
 /// 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);
 
 /// Possibly strip a floating point cast expression.
 ///
 /// If `Node` represents an explicit cast to a floating point type, return
 /// the textual context of the cast argument, otherwise `None`.

 llvm::StringRef getTimeInverseForScale(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);
 
+/// 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,
                   const Expr *E);
 
 AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
                      DurationConversionFunction) {
   using namespace clang::ast_matchers;

clang-tidy/abseil/DurationRewriter.cpp

if (const auto *MaybeCallArg = selectFirst<const Expr>(
                          hasArgument(0, expr().bind("e"))),
                 Node, *Result.Context))) {
     return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
   }
 
   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) {
+  const 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) {
   case DurationScale::Hours:
     return "absl::Hours";
   case DurationScale::Minutes:
     return "absl::Minutes";
   case DurationScale::Seconds:
     return "absl::Seconds";
   case DurationScale::Milliseconds:
     return "absl::Milliseconds";
   case DurationScale::Microseconds:
     return "absl::Microseconds";
   case DurationScale::Nanoseconds:
     return "absl::Nanoseconds";
   }
   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) {
   case DurationScale::Hours:
     return "absl::ToUnixHours";
   case DurationScale::Minutes:
     return "absl::ToUnixMinutes";
   case DurationScale::Seconds:
std::string rewriteExprFromNumberToDuration(
   if (IsLiteralZero(Result, RootNode))
     return std::string("absl::ZeroDuration()");
 
   return (llvm::Twine(getDurationFactoryForScale(Scale)) + "(" +
           simplifyDurationFactoryArg(Result, RootNode) + ")")
       .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;
 
   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)) {

clang-tidy/abseil/TimeSubtractionCheck.h

+//===--- 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;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMESUBTRACTIONCHECK_H

clang-tidy/abseil/TimeSubtractionCheck.cpp

+//===--- 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 {
+
+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::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);
+    if (!Scale)
+      continue;
+
+    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 inverse_name =
+      Result.Nodes.getNodeAs<FunctionDecl>("func_decl")->getNameAsString();
+
+  llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(inverse_name);
+  if (!Scale)
+    return;
+
+  const auto *OuterCall = Result.Nodes.getNodeAs<CallExpr>("outer_call");
+  if (OuterCall) {
+    // 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);
+
+    diag(OuterCall->getBeginLoc(), "perform subtraction in the time domain")
+        << FixItHint::CreateReplacement(
+               OuterCall->getSourceRange(),
+               (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) {
+      bool NeedParens = parensRequired(Result, MaybeCallArg);
+
+      diag(MaybeCallArg->getBeginLoc(),
+           "perform subtraction in the time domain")
+          << FixItHint::CreateReplacement(
+                 MaybeCallArg->getSourceRange(),
+                 (llvm::Twine(NeedParens ? "(" : "") +
+                  rewriteExprFromNumberToTime(Result, *Scale, BinOp->getLHS()) +
+                  " - " +
+                  rewriteExprFromNumberToTime(Result, *Scale, BinOp->getRHS()) +
+                  (NeedParens ? ")" : ""))
+                     .str());
+    } else {
+      diag(BinOp->getBeginLoc(), "perform subtraction in the time domain")
+          << FixItHint::CreateReplacement(
+                 BinOp->getSourceRange(),
+                 (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

docs/ReleaseNotes.rst

- New :doc:`abseil-duration-conversion-cast
   the right conversion function instead.
 
 - New :doc:`abseil-duration-unnecessary-conversion
   <clang-tidy/checks/abseil-duration-unnecessary-conversion>` check.
 
   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.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 - The :doc:`bugprone-argument-comment

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

+.. 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 a ``Duration`` and the first argument is a ``Time``.
+ - When the second argument is a ``Time``.
+
+In the first case, we must know the result of the operation, since without that
+the second operand could be either a ``Time`` or a ``Duration``.  In the second
+case, the first operand *must* be a ``Time``, because subtracting a ``Time``
+from a ``Duration`` is not defined.
+
+Examples:
+
+.. code-block:: c++
+  int x;
+  absl::Time t;
+
+  // Original - `Duration` result and first operand is a `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 a `Time`.
+  int i = x - absl::ToUnixSeconds(t);
+
+  // Suggestion - Perform subtraction in the Time domain instead.
+  int i = absl::ToInt64Seconds(absl::FromUnixSeconds(x) - t);

docs/clang-tidy/checks/list.rst

.. toctree::
    abseil-duration-subtraction
    abseil-duration-unnecessary-conversion
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-str-cat-append
    abseil-string-find-startswith
+   abseil-time-subtraction
    abseil-upgrade-duration-conversions
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen

test/clang-tidy/abseil-time-subtraction.cpp

+// RUN: %check_clang_tidy %s abseil-time-subtraction %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void g(absl::Duration d);
+
+void f() {
+  absl::Time t;
+  int x, y;
+  absl::Duration d;
+
+  d = absl::Hours(absl::ToUnixHours(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = (t - absl::FromUnixHours(x));
+  d = absl::Minutes(absl::ToUnixMinutes(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = (t - absl::FromUnixMinutes(x));
+  d = absl::Seconds(absl::ToUnixSeconds(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = (t - absl::FromUnixSeconds(x));
+  d = absl::Milliseconds(absl::ToUnixMillis(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = (t - absl::FromUnixMillis(x));
+  d = absl::Microseconds(absl::ToUnixMicros(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = (t - absl::FromUnixMicros(x));
+  d = absl::Nanoseconds(absl::ToUnixNanos(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = (t - absl::FromUnixNanos(x));
+
+  y = x - absl::ToUnixHours(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: y = absl::ToInt64Hours(absl::FromUnixHours(x) - t);
+  y = x - absl::ToUnixMinutes(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: y = absl::ToInt64Minutes(absl::FromUnixMinutes(x) - t);
+  y = x - absl::ToUnixSeconds(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: y = absl::ToInt64Seconds(absl::FromUnixSeconds(x) - t);
+  y = x - absl::ToUnixMillis(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: y = absl::ToInt64Milliseconds(absl::FromUnixMillis(x) - t);
+  y = x - absl::ToUnixMicros(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: y = absl::ToInt64Microseconds(absl::FromUnixMicros(x) - t);
+  y = x - absl::ToUnixNanos(t);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: y = absl::ToInt64Nanoseconds(absl::FromUnixNanos(x) - t);
+
+  // Check parenthesis placement
+  d = 5 * absl::Seconds(absl::ToUnixSeconds(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: d = 5 * (t - absl::FromUnixSeconds(x));
+
+  // No extra parens around arguments
+  g(absl::Seconds(absl::ToUnixSeconds(t) - x));
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: g(t - absl::FromUnixSeconds(x));
+  g(absl::Seconds(x - absl::ToUnixSeconds(t)));
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: g(absl::FromUnixSeconds(x) - t);
+
+  // These should not trigger; they are likely bugs
+  d = absl::Milliseconds(absl::ToUnixSeconds(t) - x);
+  d = absl::Seconds(absl::ToUnixMicros(t) - x);
+}
+
+absl::Duration parens_in_return() {
+  absl::Time t;
+  int x;
+
+  return absl::Seconds(absl::ToUnixSeconds(t) - x);
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: return t - absl::FromUnixSeconds(x);
+  return absl::Seconds(x - absl::ToUnixSeconds(t));
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: perform subtraction in the time domain [abseil-time-subtraction]
+  // CHECK-FIXES: return absl::FromUnixSeconds(x) - t;
+}