Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ 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 @@ "abseil-str-cat-append"); CheckFactories.registerCheck( "abseil-string-find-startswith"); + CheckFactories.registerCheck( + "abseil-time-subtraction"); CheckFactories.registerCheck( "abseil-upgrade-duration-conversions"); } Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -17,6 +17,7 @@ RedundantStrcatCallsCheck.cpp StrCatAppendCheck.cpp StringFindStartswithCheck.cpp + TimeSubtractionCheck.cpp UpgradeDurationConversionsCheck.cpp LINK_LIBS Index: clang-tidy/abseil/DurationRewriter.h =================================================================== --- clang-tidy/abseil/DurationRewriter.h +++ clang-tidy/abseil/DurationRewriter.h @@ -31,6 +31,10 @@ /// 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 @@ 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, Index: clang-tidy/abseil/DurationRewriter.cpp =================================================================== --- clang-tidy/abseil/DurationRewriter.cpp +++ clang-tidy/abseil/DurationRewriter.cpp @@ -84,6 +84,22 @@ return llvm::None; } +/// If `Node` is a call to the inverse of `Scale`, return that inverse's +/// argument, otherwise None. +static llvm::Optional +rewriteInverseTimeCall(const MatchFinder::MatchResult &Result, + DurationScale Scale, const Expr &Node) { + const llvm::StringRef &InverseFunction = getTimeInverseForScale(Scale); + if (const auto *MaybeCallArg = selectFirst( + "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_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 @@ .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 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; Index: clang-tidy/abseil/TimeSubtractionCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/TimeSubtractionCheck.h @@ -0,0 +1,35 @@ +//===--- 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 Index: clang-tidy/abseil/TimeSubtractionCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/TimeSubtractionCheck.cpp @@ -0,0 +1,163 @@ +//===--- 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( + "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( + "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( + "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 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("binop"); + std::string inverse_name = + Result.Nodes.getNodeAs("func_decl")->getNameAsString(); + + llvm::Optional Scale = getScaleForTimeInverse(inverse_name); + if (!Scale) + return; + + const auto *OuterCall = Result.Nodes.getNodeAs("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( + "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 Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -85,6 +85,12 @@ Finds and fixes cases where ``absl::Duration`` values are being converted to numeric types and back again. +- New :doc:`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 ` check. Index: docs/clang-tidy/checks/abseil-time-subtraction.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-time-subtraction.rst @@ -0,0 +1,36 @@ +.. 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); Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -18,6 +18,7 @@ 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 Index: test/clang-tidy/abseil-time-subtraction.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-time-subtraction.cpp @@ -0,0 +1,78 @@ +// 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; +}