[clang-tidy] Update abseil-duration-comparison to handle macro arguments

[hwright]

Summary

This change relaxes the requirements on the utility rewriteExprFromNumberToDuration function, so that the abseil-duration-comparison check transform expressions which are arguments of macros.

[aaron.ballman]

clang-tidy/abseil/DurationComparisonCheck.cpp

25

expr doesn't follow the usual naming conventions, I'd go with E (and fix the comment to match).

29

loc -> Loc

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

131

Can you add another example that has one more level of macro arg expansion? e.g.,

#define VALUE_IF_2(e) (e)
#define VALUE_IF(v, e) v ? VALUE_IF_2(e) : VALUE_IF_2(0)
int a = VALUE_IF(1, 5 > absl::ToDoubleSeconds(d1));

Which makes me wonder -- is this transformation valid in all cases? For instance, how do the fixes look with this abomination?

#define VALUE_IF_2(e) (e)
#define VALUE_IF(v, e, type) (v ? VALUE_IF_2(absl::To##type##Seconds(e)) : 0)
int a = VALUE_IF(1, d1, Double);

I would expect these shenanigans to be vanishingly rare, so if this turns out to be a bad FIXIT you may want to document it as a known deficiency if you don't feel like handling such a case.

[hwright]

Update tests

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

131

I've added both these as test cases.

The first one works as expected, and I'm pleasantly surprised to see that the second one does as well (which is to say that it doesn't transform).

[aaron.ballman]

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

131

Great, thank you!

146

What if this is changed to:

#define VALUE_IF(v, e, type) (v ? (5 > VALUE_IF_2(absl::To##type##Seconds(e))) : 0)

[hwright]

Another test.

[hwright]

@aaron.ballman I am both grateful and sad that I don't possess the same macro creativity as you do. :)

(I'm also sad that the various clang APIs aren't as well documented as I hoped. The current solution works well, but it took a while and a bit of consultation with some other llvm'ers to get it right.)

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

146

This also doesn't transform.

[aaron.ballman]

In D55784#1334929, @hwright wrote:

@aaron.ballman I am both grateful and sad that I don't possess the same macro creativity as you do. :)

LOL

(I'm also sad that the various clang APIs aren't as well documented as I hoped. The current solution works well, but it took a while and a bit of consultation with some other llvm'ers to get it right.)

As you dig into APIs like that, feel free to propose or commit new documentation that helps clarify things -- more/better docs are always appreciated!

LGTM with a minor commenting nit.

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

146

Fantastic, thank you! I wouldn't expect a fix-it there due to the macro shenanigans, but I'm surprised it does not diagnose given that it expands to basically the same thing as a and a2 above: https://godbolt.org/z/D1MvGX (ignore the errors but look at the macro expansion from the diagnostics).

Granted, this situation is uncommon enough, so feel free to add a FIXME comment to the a4 test case to say that it probably should be diagnosed without a fix-it hint (assuming you agree).

[hwright]

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

146

I actually prefer not to diagnose here, mostly because I'm not sure what value we'd actually give without a fix-it hint. "You are holding it wrong, but we can't tell you how to hold it right."

Ultimately, though, I think you are right that the number of cases this occurs are vanishingly small, and I'd expect folks doing these kinds of craziness to know what they are doing (and take responsibility thereof). Perhaps that's optimistic.

clang-tidy/abseil/DurationComparisonCheck.cpp

 #include "clang/Tooling/FixIt.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
+/// Return `true` if `E` is a either: not a macro at all; or an argument to
+/// one. In the latter case, we should still transform it.
+static bool IsValidMacro(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)) {
+    // We are calling getImmediateMacroCallerLoc, but note it is essentially
+    // equivalent to calling getImmediateSpellingLoc in this context according
+    // to Clang implementation. We are not calling getImmediateSpellingLoc
+    // because Clang comment says it "should not generally be used by clients."
+    Loc = Result.SourceManager->getImmediateMacroCallerLoc(Loc);
+  }
+  return !Loc.isMacroID();
+}
+
 void DurationComparisonCheck::registerMatchers(MatchFinder *Finder) {
   auto Matcher =
       binaryOperator(anyOf(hasOperatorName(">"), hasOperatorName(">="),
                            hasOperatorName("=="), hasOperatorName("<="),
                            hasOperatorName("<")),
                      hasEitherOperand(ignoringImpCasts(callExpr(
                          callee(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");
 
-  // Don't try to replace things inside of macro definitions.
-  if (Binop->getExprLoc().isMacroID())
-    return;
-
   llvm::Optional<DurationScale> Scale = getScaleForInverse(
       Result.Nodes.getNodeAs<FunctionDecl>("function_decl")->getName());
   if (!Scale)
     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.
-  llvm::Optional<std::string> LhsReplacement =
+  if (!IsValidMacro(Result, Binop->getLHS()) ||
+      !IsValidMacro(Result, Binop->getRHS()))
+    return;
+  std::string LhsReplacement =
       rewriteExprFromNumberToDuration(Result, *Scale, Binop->getLHS());
-  llvm::Optional<std::string> RhsReplacement =
+  std::string RhsReplacement =
       rewriteExprFromNumberToDuration(Result, *Scale, Binop->getRHS());
 
-  if (!(LhsReplacement && RhsReplacement))
-    return;
-
   diag(Binop->getBeginLoc(), "perform comparison in the duration domain")
       << FixItHint::CreateReplacement(Binop->getSourceRange(),
-                                      (llvm::Twine(*LhsReplacement) + " " +
+                                      (llvm::Twine(LhsReplacement) + " " +
                                        Binop->getOpcodeStr() + " " +
-                                       *RhsReplacement)
+                                       RhsReplacement)
                                           .str());
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

clang-tidy/abseil/DurationRewriter.h

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(
+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;
   return functionDecl(
       hasAnyName("::absl::ToDoubleHours", "::absl::ToDoubleMinutes",

clang-tidy/abseil/DurationRewriter.cpp

llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
 
   auto ScaleIter = ScaleMap.find(std::string(Name));
   if (ScaleIter == ScaleMap.end())
     return llvm::None;
 
   return ScaleIter->second;
 }
 
-llvm::Optional<std::string> rewriteExprFromNumberToDuration(
+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

clang-tidy/abseil/DurationSubtractionCheck.cpp

void DurationSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
   // 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 =
+  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 + ")")
+              RhsReplacement + ")")
                  .str());
 }
 
 } // namespace abseil
 } // namespace tidy
 } // namespace clang

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

void f() {
   }
 
   // A complex expression
   int y;
   b = (y + 5) * 10 > absl::ToDoubleMilliseconds(d1);
   // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
   // CHECK-FIXES: absl::Milliseconds((y + 5) * 10) > d1;
 
+  // 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::ToDoubleSeconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:23: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: VALUE_IF(1, absl::Seconds(5) > d1);
+#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::ToDoubleSeconds(d1));
+  // CHECK-MESSAGES: [[@LINE-1]]:24: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: VALUE_IF(1, absl::Seconds(5) > d1);
+#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, d1, Double);
+#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, d1, Double);
+#undef VALUE_IF
+#undef VALUE_IF_2
+
   // These should not match
   b = 6 < 4;
 
 #define TODOUBLE(x) absl::ToDoubleSeconds(x)
   b = 5.0 > TODOUBLE(d1);
 #undef TODOUBLE
 #define THIRTY 30.0
   b = THIRTY > absl::ToDoubleSeconds(d1);
 #undef THIRTY
 }