[clangd][c++20]Consider rewritten binary operators in TargetFinder

[massberg]

Summary

In C++20 some binary operations can be rewritten, e.g. a != b
can be rewritten to !(a == b) if != is not explicitly defined.
The TargetFinder hasn't considered the corresponding CXXRewrittenBinaryOperator yet. This resulted that the definition of such operators couldn't be found
when navigating to such a != operator, see https://github.com/clangd/clangd/issues/1476.

In this patch we add support of CXXRewrittenBinaryOperator in FindTarget.
In such a case we redirect to the inner binary operator of the decomposed form.
E.g. in case that a != b has been rewritten to !(a == b) we go to the
== operator. The == operator might be implicitly defined (e.g. by a <=>
operator), but this case is already handled, see the new test.

I'm not sure if I the hover test which is added in this patch is the right one,
but at least is passed with this patch and fails without it :)

Note, that it might be a bit missleading that hovering over a != refers to
"instance method operator==".

[sammccall]

Thanks, this looks pretty good!

I'm not sure if I the hover test which is added in this patch is the right one,
but at least is passed with this patch and fails without it :)

This is nice to have, but you add a unittest to FindTargetTest too? That's the most direct way to test this code.

This won't affect find-refs BTW, that would be handled in refInStmt() in FindTarget.cpp

clang-tools-extra/clangd/unittests/HoverTests.cpp

4051

no need to sign your work :-)

4051

can you add this to HoverTest__All instead? That way we test all details of the hover card

4079

if we're describing this as the spaceship operator, then the type looks wrong

[massberg]

Fix test name

[massberg]

Add test to FindTargetTests and extend test in HoverTests.

[massberg]

Thanks for the comments, I have added an additional test to FindTargetTest. See also my other comments.

clang-tools-extra/clangd/unittests/HoverTests.cpp

4051

Upps, sorry.

4051

can you add this to HoverTest__All instead? That way we test all details of the hover card

The (Hover, All) test tests with std=c++17 while this test tests c++20 features.
We could add an additional field with the version to the struct in the (Hover, All) test.
Or add a (Hover, All_Cpp20) test for testing C++20 (what is probably not worth at the moment with just one test requiring C++20).

4079

I have added a test checking the whole definition.
Actually the following is happening here:
The != operator isn't explicitly defined, so the binary operator is rewritten to !(Foo(1) == Foo(2),
i.e. we are now using the == operator.
However, the == operator is also not explicitly defined, but there is a defaulted spaceship operator.
Thus the == operator is implicitly defined through the <=> operator.
So the type and definition here are from the implicitly defined == operator, while the original source of it is the <=> operator.

[sammccall]

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

648

this template pattern vs instantiation is really surprising, but it's a reasonable analogy, I don't see any particular problems. (Also, operator<=> looks like a template over whatever = is!)

Do you know whether this means go-to-definition gives you two options to navigate to?
(No need to write a test, just curious)

clang-tools-extra/clangd/unittests/HoverTests.cpp

4051

Does anything break if you switch everything to C++20?
The intention of "std=c++17" there was certainly "not 14", rather than "not 20" :-)

[massberg]

Switched the "Hover, All" test to run with C++20 instead of C++17 and moved the
RewrittenBinaryOperatorSpaceship test to it.

[massberg]

clang-tools-extra/clangd/unittests/HoverTests.cpp

4051

Test test passed with C++20. I have switch it to C++20 and moved the new test as a new case in this test.

[massberg]

clang-format code

clang-tools-extra/clangd/FindTarget.cpp

struct Visitor : public ConstStmtVisitor<Visitor> {
         Outer.add(POE->getSyntacticForm(), Flags);
       }
       void VisitCXXNewExpr(const CXXNewExpr *CNE) {
         Outer.add(CNE->getOperatorNew(), Flags);
       }
       void VisitCXXDeleteExpr(const CXXDeleteExpr *CDE) {
         Outer.add(CDE->getOperatorDelete(), Flags);
       }
+      void
+      VisitCXXRewrittenBinaryOperator(const CXXRewrittenBinaryOperator *RBO) {
+        Outer.add(RBO->getDecomposedForm().InnerBinOp, Flags);
+      }
     };
     Visitor(*this, Flags).Visit(S);
   }
 
   void add(QualType T, RelSet Flags) {
     if (T.isNull())
       return;
     debug(T, Flags);

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Code = R"cpp(
 
     awaitable foo() {
       co_await [[executor]]();
     }
   )cpp";
   EXPECT_DECLS("RecordTypeLoc", "struct executor");
 }
 
+TEST_F(TargetDeclTest, RewrittenBinaryOperator) {
+  Flags.push_back("-std=c++20");
+
+  Code = R"cpp(
+  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};
+    }
+
+    struct Foo
+    {
+      int x;
+      auto operator<=>(const Foo&) const = default;
+    };
+
+    bool x = (Foo(1) [[!=]] Foo(2));
+  )cpp";
+  EXPECT_DECLS("CXXRewrittenBinaryOperator",
+               {"std::strong_ordering operator<=>(const Foo &) const = default",
+                Rel::TemplatePattern},
+               {"bool operator==(const Foo &) const noexcept = default",
+                Rel::TemplateInstantiation});
+}
+
 TEST_F(TargetDeclTest, FunctionTemplate) {
   Code = R"cpp(
     // Implicit specialization.
     template<typename T> bool foo(T) { return false; };
     bool x = [[foo]](42);
   )cpp";
   EXPECT_DECLS("DeclRefExpr",
                {"template<> bool foo<int>(int)", Rel::TemplateInstantiation},

clang-tools-extra/clangd/unittests/HoverTests.cpp

#define array_decl(type, name, size) type name left_bracket size right_bracket
   auto AST = TU.build();
   auto H = getHover(AST, C.point(), format::getLLVMStyle(), nullptr);
 
   ASSERT_TRUE(H);
   EXPECT_TRUE(H->Value);
   EXPECT_TRUE(H->Type);
 }
 
+TEST(Hover, RewrittenBinaryOperatorSpaceship) {
+  Annotations T(R"cpp(
+  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};
+  }
+
+  struct Foo
+  {
+    int x;
+    // Foo spaceship
+    auto operator<=>(const Foo&) const = default;
+  };
+
+  static_assert(Foo(1) !^= Foo(2));
+  )cpp");
+
+  TestTU TU = TestTU::withCode(T.code());
+  TU.ExtraArgs.push_back("-std=c++20");
+  auto AST = TU.build();
+  auto HI = getHover(AST, T.point(), format::getLLVMStyle(), nullptr);
+  EXPECT_EQ(HI->Type,
+            HoverInfo::PrintedType("bool (const Foo &) const noexcept"));
+  EXPECT_EQ(HI->Name, "operator==");
+  EXPECT_EQ(HI->Definition,
+            "bool operator==(const Foo &) const noexcept = default");
+  EXPECT_EQ(HI->Documentation, "Foo spaceship");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang