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

[hwright]

Summary

This is an analog of the abseil-duration-comparison check, but for the absl::Time domain. It has a similar implementation and tests.

[Eugene.Zelenko]

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

7

Please synchronize with Release Notes.

11

Please fix double spaces. Same below.

[ioeric]

clang-tidy/abseil/TimeComparisonCheck.cpp

24

DurationComparisonCheck.cpp has a very similar matcher pattern.

Maybe pull out a common matcher for this? E.g. comparisonOperatorWithCallee(...)?

43

Move this comment closer the replacement logic below?

47

nit: double negation is a bit hard to read. maybe !(isNotInMacro(LSH) && isNotInMacro(RHS))? Ideally, the helper should be isInMacro instead of isNotInMacro.

[hwright]

clang-tidy/abseil/TimeComparisonCheck.cpp

24

My one concern about doing so is that it would move the name bindings into a separate location away from the callback consuming those bindings.

Since this is only the second instance of this pattern, would it be reasonable to wait until there's a third to combine them?

[ioeric]

clang-tidy/abseil/TimeComparisonCheck.cpp

24

My one concern about doing so is that it would move the name bindings into a separate location away from the callback consuming those bindings.

I think the name binding can stay in the same file. comparisonOperatorWithCallee is a matcher for a FunctionDecl, so, on the call site, you could do comparisonOperatorWithCallee(functionDecl(TimeConversionFunction()).bind("function_decl")).bind("binop").

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 "TimeComparisonCheck.h"
 #include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
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<TimeComparisonCheck>(
+        "abseil-time-comparison");
     CheckFactories.registerCheck<TimeSubtractionCheck>(
         "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
         "abseil-upgrade-duration-conversions");
   }
 };
 
 // Register the AbseilModule using this statically initialized variable.

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
+  TimeComparisonCheck.cpp
   TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/abseil/DurationComparisonCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
-  auto Matcher =
-      binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
-                           hasOperatorName("=="), hasOperatorName("<="),
-                           hasOperatorName("<")),
-                     hasEitherOperand(ignoringImpCasts(callExpr(
-                         callee(functionDecl(DurationConversionFunction())
-                                    .bind("function_decl"))))))
+  auto Matcher = expr(comparisonOperatorWithCallee(functionDecl(
+                          functionDecl(DurationConversionFunction())
+                              .bind("function_decl"))))
                      .bind("binop");
 
   Finder->addMatcher(Matcher, this);
 }
 
 void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
 
   llvm::Optional<DurationScale> Scale = getScaleForDurationInverse(

clang-tidy/abseil/DurationRewriter.h

 AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<FunctionDecl>,
                      TimeConversionFunction) {
   using namespace clang::ast_matchers;
   return functionDecl(hasAnyName(
       "::absl::ToUnixHours", "::absl::ToUnixMinutes", "::absl::ToUnixSeconds",
       "::absl::ToUnixMillis", "::absl::ToUnixMicros", "::absl::ToUnixNanos"));
 }
 
+AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher<Stmt>,
+                       comparisonOperatorWithCallee,
+                       ast_matchers::internal::Matcher<Decl>, funcDecl) {
+  using namespace clang::ast_matchers;
+  return binaryOperator(
+      anyOf(hasOperatorName(">"), hasOperatorName(">="), hasOperatorName("=="),
+            hasOperatorName("<="), hasOperatorName("<")),
+      hasEitherOperand(ignoringImpCasts(callExpr(callee(funcDecl)))));
+}
+
 } // namespace abseil
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_DURATIONCOMPARISONCHECK_H

clang-tidy/abseil/TimeComparisonCheck.h

+//===--- TimeComparisonCheck.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_TIMECOMPARECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_TIMECOMPARECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Prefer comparison in the `absl::Time` domain instead of the numeric
+/// domain.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-time-comparison.html
+class TimeComparisonCheck : public ClangTidyCheck {
+public:
+  TimeComparisonCheck(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_TIMECOMPARECHECK_H

clang-tidy/abseil/TimeComparisonCheck.cpp

+//===--- TimeComparisonCheck.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 "TimeComparisonCheck.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 TimeComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  auto Matcher =
+      expr(comparisonOperatorWithCallee(functionDecl(
+               functionDecl(TimeConversionFunction()).bind("function_decl"))))
+          .bind("binop");
+
+  Finder->addMatcher(Matcher, this);
+}
+
+void TimeComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Binop = Result.Nodes.getNodeAs<BinaryOperator>("binop");
+
+  llvm::Optional<DurationScale> Scale = getScaleForTimeInverse(
+      Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
+  if (!Scale)
+    return;
+
+  if (isInMacro(Result, Binop->getLHS()) || isInMacro(Result, Binop->getRHS()))
+    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.
+  std::string LhsReplacement =
+      rewriteExprFromNumberToTime(Result, *Scale, Binop->getLHS());
+  std::string RhsReplacement =
+      rewriteExprFromNumberToTime(Result, *Scale, Binop->getRHS());
+
+  diag(Binop->getBeginLoc(), "perform comparison in the time domain")
+      << FixItHint::CreateReplacement(Binop->getSourceRange(),
+                                      (llvm::Twine(LhsReplacement) + " " +
+                                       Binop->getOpcodeStr() + " " +
+                                       RhsReplacement)
+                                          .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-comparison
+  <clang-tidy/checks/abseil-time-comparison>` check.
+
+  Prefer comparisons in the ``absl::Time`` domain instead of the integer
+  domain.
+
 - 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>`

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

+.. title:: clang-tidy - abseil-time-comparison
+
+abseil-time-comparison
+======================
+
+Prefer comparisons in the ``absl::Time`` domain instead of the integer domain.
+
+N.B.: In cases where an ``absl::Time`` is being converted to an integer,
+alignment may occur. If the comparison depends on this alingment, doing the
+comparison in the ``absl::Time`` domain may yield a different result. In
+practice this is very rare, and still indicates a bug which should be fixed.
+
+Examples:
+
+.. code-block:: c++
+
+  // Original - Comparison in the integer domain
+  int x;
+  absl::Time t;
+  if (x < absl::ToUnixSeconds(t)) ...
+
+  // Suggested - Compare in the absl::Time domain instead
+  if (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-comparison
    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

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

+// RUN: %check_clang_tidy %s abseil-time-comparison %t -- -- -I%S/Inputs
+
+#include "absl/time/time.h"
+
+void f() {
+  double x;
+  absl::Duration d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) > t1;
+  b = x >= absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) >= t1;
+  b = x == absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) == t1;
+  b = x <= absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) <= t1;
+  b = x < absl::ToUnixSeconds(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) < t1;
+  b = x == absl::ToUnixSeconds(t1 - d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixSeconds(x) == t1 - d2;
+  b = absl::ToUnixSeconds(t1) > absl::ToUnixSeconds(t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 > t2;
+
+  // Check against the LHS
+  b = absl::ToUnixSeconds(t1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 < absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 <= absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) == x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 == absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) >= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 >= absl::FromUnixSeconds(x);
+  b = absl::ToUnixSeconds(t1) > x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 > absl::FromUnixSeconds(x);
+
+  // Comparison against zero
+  b = absl::ToUnixSeconds(t1) < 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 < absl::UnixEpoch();
+  b = absl::ToUnixSeconds(t1) < 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: t1 < absl::UnixEpoch();
+
+  // Scales other than Seconds
+  b = x > absl::ToUnixMicros(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMicros(x) > t1;
+  b = x >= absl::ToUnixMillis(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMillis(x) >= t1;
+  b = x == absl::ToUnixNanos(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixNanos(x) == t1;
+  b = x <= absl::ToUnixMinutes(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMinutes(x) <= t1;
+  b = x < absl::ToUnixHours(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixHours(x) < t1;
+
+  // A long expression
+  bool some_condition;
+  int very_very_very_very_long_variable_name;
+  absl::Time SomeTime;
+  if (some_condition && very_very_very_very_long_variable_name
+     < absl::ToUnixSeconds(SomeTime)) {
+  // CHECK-MESSAGES: [[@LINE-2]]:25: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: if (some_condition && absl::FromUnixSeconds(very_very_very_very_long_variable_name) < SomeTime) {
+    return;
+  }
+
+  // A complex expression
+  int y;
+  b = (y + 5) * 10 > absl::ToUnixMillis(t1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: absl::FromUnixMillis((y + 5) * 10) > t1;
+
+  // We should still transform the expression inside this macro invocation
+#define VALUE_IF(v, e) v ? (e) : 0
+  int a = VALUE_IF(1, 5 > absl::ToUnixSeconds(t1));
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: VALUE_IF(1, absl::FromUnixSeconds(5) > t1);
+#undef VALUE_IF
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0)
+  int a2 = VALUE_IF(1, 5 > absl::ToUnixSeconds(t1));
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: perform comparison in the time domain [abseil-time-comparison]
+  // CHECK-FIXES: VALUE_IF(1, absl::FromUnixSeconds(5) > t1);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
+  int a3 = VALUE_IF(1, t1, Unix);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+#define VALUE_IF_2(e) (e)
+#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)
+  int a4 = VALUE_IF(1, t1, Unix);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
+  // These should not match
+  b = 6 < 4;
+
+#define TODOUBLE(x) absl::ToUnixSeconds(x)
+  b = 5.0 > TODOUBLE(t1);
+#undef TODOUBLE
+#define THIRTY 30.0
+  b = THIRTY > absl::ToUnixSeconds(t1);
+#undef THIRTY
+}