[PATCH] D95714: clang-tidy: modernize-use-nullptr mistakenly fixes rewritten spaceship operator comparisons

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc created this revision.
poelmanc added reviewers: aaron.ballman, angelgarcia, hokein.
poelmanc added a project: clang-tools-extra.
Herald added a subscriber: yaxunl.
poelmanc requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

`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);

Oops!

The supplied patch fixes this by adding just one comment and one line of code to `UseNullptrCheck.cpp`:

  // Skip implicit casts to 0 generated by operator<=> rewrites.
  unless(hasAncestor(cxxRewrittenBinaryOperator())),

I looked for a `hasGrandparent` matcher which would be more precise, but none exists. This fix slightly overly-aggressively eliminates matches, so even with this patch `modernize-use-nullptr` will not convert

  result = (a1 > (ptr == 0 ? a1 : a2));

to

  result = (a1 > (ptr == nullptr ? a1 : a2));

because `ptr == 0` has an ancestor that is a rewritten spaceship operator. Changing the proposed one-line fix to:

  unless(hasGrandparent(cxxRewrittenBinaryOperator())),

resolves this, but requires another 57 lines of code across AstMatchers.h, AstMatchersInternal.h, and AstMatchFinder.cpp to create `hasGrandparent`. I'd be happy to supply that patch instead if folks think it worthwhile? Maybe others in the future can benefit from `hasGrandparent`?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95714

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


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-nullptr-cxx20.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy -std=c++20 %s modernize-use-nullptr %t
+
+#include <compare>
+
+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);
+}
Index: clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp
@@ -45,6 +45,8 @@
       TK_AsIs,
       castExpr(anyOf(ImplicitCastToNull,
                      explicitCastExpr(hasDescendant(ImplicitCastToNull))),
+               // Skip implicit casts to 0 generated by operator<=> rewrites.
+               unless(hasAncestor(cxxRewrittenBinaryOperator())),
                unless(hasAncestor(explicitCastExpr())))
           .bind(CastSequence));
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95714.320254.patch
Type: text/x-patch
Size: 1384 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210130/6d56a8a1/attachment.bin>