[clang-tools-extra] 98146c1 - [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

[Nathan James via cfe-commits]

Author: poelmanc
Date: 2021-02-09T14:02:45Z
New Revision: 98146c1f5d0c772aec56149724119d49990f4d0c

URL: https://github.com/llvm/llvm-project/commit/98146c1f5d0c772aec56149724119d49990f4d0c
DIFF: https://github.com/llvm/llvm-project/commit/98146c1f5d0c772aec56149724119d49990f4d0c.diff

LOG: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

`clang-tidy -std=c++20` with `modernize-use-nullptr` mistakenly inserts `nullptr` in place of the comparison operator if the comparison internally expands in the AST to a rewritten spaceship operator. This can be reproduced by running the new `modernize-use-nullptr-cxx20.cpp` test without applying the supplied patch to `UseNullptrCheck.cpp`; the current clang-tidy will mistakenly replace:
```result = (a1 < a2);```
with
```result = (a1 nullptr a2);```

Reviewed By: njames93

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

Added: 
    clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp

Modified: 
    clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
index e32687ba7b35..5ca2be2eb556 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
@@ -41,12 +41,28 @@ StatementMatcher makeCastSequenceMatcher() {
       unless(hasImplicitDestinationType(qualType(substTemplateTypeParmType()))),
       unless(hasSourceExpression(hasType(sugaredNullptrType()))));
 
+  auto IsOrHasDescendant = [](auto InnerMatcher) {
+    return anyOf(InnerMatcher, hasDescendant(InnerMatcher));
+  };
+
   return traverse(
       TK_AsIs,
-      castExpr(anyOf(ImplicitCastToNull,
-                     explicitCastExpr(hasDescendant(ImplicitCastToNull))),
-               unless(hasAncestor(explicitCastExpr())))
-          .bind(CastSequence));
+      anyOf(castExpr(anyOf(ImplicitCastToNull,
+                           explicitCastExpr(hasDescendant(ImplicitCastToNull))),
+                     unless(hasAncestor(explicitCastExpr())),
+                     unless(hasAncestor(cxxRewrittenBinaryOperator())))
+                .bind(CastSequence),
+            cxxRewrittenBinaryOperator(
+                // Match rewritten operators, but verify (in the check method)
+                // that if an implicit cast is found, it is not from another
+                // nested rewritten operator.
+                expr().bind("matchBinopOperands"),
+                hasEitherOperand(IsOrHasDescendant(
+                    implicitCastExpr(
+                        ImplicitCastToNull,
+                        hasAncestor(cxxRewrittenBinaryOperator().bind(
+                            "checkBinopOperands")))
+                        .bind(CastSequence))))));
 }
 
 bool isReplaceableRange(SourceLocation StartLoc, SourceLocation EndLoc,
@@ -480,6 +496,11 @@ void UseNullptrCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *NullCast = Result.Nodes.getNodeAs<CastExpr>(CastSequence);
   assert(NullCast && "Bad Callback. No node provided");
 
+  if (Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>(
+          "matchBinopOperands") !=
+      Result.Nodes.getNodeAs<CXXRewrittenBinaryOperator>("checkBinopOperands"))
+    return;
+
   // Given an implicit null-ptr cast or an explicit cast with an implicit
   // null-to-pointer cast within use CastSequenceVisitor to identify sequences
   // of explicit casts that can be converted into 'nullptr'.

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
new file mode 100644
index 000000000000..b00cb5787ea0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
+
+namespace std {
+struct strong_ordering {
+  int n;
+  constexpr operator int() const { return n; }
+  static const strong_ordering equal, greater, less;
+};
+constexpr strong_ordering strong_ordering::equal = {0};
+constexpr strong_ordering strong_ordering::greater = {1};
+constexpr strong_ordering strong_ordering::less = {-1};
+} // namespace std
+
+class A {
+public:
+  auto operator<=>(const A &other) const = default;
+};
+
+void test_cxx_rewritten_binary_ops() {
+  A a1, a2;
+  bool result;
+  // should not change next line to (a1 nullptr a2)
+  result = (a1 < a2);
+  // CHECK-FIXES: result = (a1 < a2);
+  // should not change next line to (a1 nullptr a2)
+  result = (a1 >= a2);
+  // CHECK-FIXES: result = (a1 >= a2);
+  int *ptr = 0;
+  // CHECK-FIXES: int *ptr = nullptr;
+  result = (a1 > (ptr == 0 ? a1 : a2));
+  // CHECK-FIXES: result = (a1 > (ptr == nullptr ? a1 : a2));
+  result = (a1 > ((a1 > (ptr == 0 ? a1 : a2)) ? a1 : a2));
+  // CHECK-FIXES: result = (a1 > ((a1 > (ptr == nullptr ? a1 : a2)) ? a1 : a2));
+}