[clang-tidy] Ignore cxxRewrittenBinaryOperator in defaulted function decls in modernize-use-nullptr.

[massberg]

Summary

The check has produced false positives when checking the default implementation of the spaceship operator.
The default implementation should be skipped by the check.

Modified the existing test so that the check runs into the bug without this fix and add another test case.

Fixes #53961

[ilya-biryukov]

LGTM, thanks!
Initially I thought that traversing with TK_IgnoreUnlessSpelledInSource might be appropriate. But that idea does not seem to work as we do need to match implicit casts to find the nullptr.

I am also not sure if we should show this warning when the generated code was written by hand. The check will keep suggesting to replace (a <=> b) < 0 with (a <=> b) < nullptr.
Technically this will compile, but the C++ Standard explicitly mentions the comparisons must be implicitly rewritten to (a <=> b) < 0, not nullptr.
But let's not solve this problem until someone actually reports it.

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

66

NIT: maybe add a comment mentioning that this tries to find defaulted comparison operators.
Pre-C++20 one could only put = default constructors, destructors and assignment operators. So I suspect this might be a useful clarification for readers of the code.

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

6

NIT: add a comment mentioning that this mocks how STL defined the parameter.

[massberg]

Add comments.

[massberg]

Thanks for the comments!

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

return traverse(
                 // 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))))));
+                        .bind(CastSequence))),
+                // Skip defaulted comparison operators.
+                unless(hasAncestor(functionDecl(isDefaulted()))))));
 }
 
 bool isReplaceableRange(SourceLocation StartLoc, SourceLocation EndLoc,
                         const SourceManager &SM) {
   return SM.isWrittenInSameFile(StartLoc, EndLoc);
 }
 
 /// Replaces the provided range with the text "nullptr", but only if

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

 // RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
 
 namespace std {
+class strong_ordering;
+
+// Mock how STD defined unspecified parameters for the operators below.
+struct _CmpUnspecifiedParam {
+  consteval
+  _CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {}
+};
+
 struct strong_ordering {
-  int n;
-  constexpr operator int() const { return n; }
+  signed char value;
+
+  friend constexpr bool operator==(strong_ordering v,
+                                   _CmpUnspecifiedParam) noexcept {
+    return v.value == 0;
+  }
+  friend constexpr bool operator<(strong_ordering v,
+                                  _CmpUnspecifiedParam) noexcept {
+    return v.value < 0;
+  }
+  friend constexpr bool operator>(strong_ordering v,
+                                  _CmpUnspecifiedParam) noexcept {
+    return v.value > 0;
+  }
+  friend constexpr bool operator>=(strong_ordering v,
+                                   _CmpUnspecifiedParam) noexcept {
+    return v.value >= 0;
+  }
   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 {
+  int a;
 public:
   auto operator<=>(const A &other) const = default;
+  // CHECK-FIXES: 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));
 }
+
+template<class T1, class T2>
+struct P {
+  T1 x1;
+  T2 x2;
+  friend auto operator<=>(const P&, const P&) = default;
+  // CHECK-FIXES: friend auto operator<=>(const P&, const P&) = default;
+};
+
+bool foo(P<int,int> x, P<int, int> y) { return x < y; }
+// CHECK-FIXES: bool foo(P<int,int> x, P<int, int> y) { return x < y; }