[clangd] Use operator<< to prevent printers issues in Gtest

[malaperle]

Summary

It is possible that there will be two different instantiations of
the printer template for a given type and some tests could end up calling the
wrong (default) one. For example, it was seen in CodeCompleteTests.cpp when
printing CompletionItems that it would use the wrong printer because the default
is also instantiated in ClangdTests.cpp.

With this change, objects that were previously printed with a custom Printer now
get printed through the operator<< which is declared alongside the class.
This rule of the thumb should make it less error-prone.

[ilya-biryukov]

LGTM.
I wonder if there's a mechanism to always include the printers when including gtest.h, but having a convention to always include them seems ok for now.

[simark]

We could create a file `ClangdTesting.h" which includes everything tested related (gtest, gmock, printers, syncapi, etc). The convention would be that test files would just include that.

[ilya-biryukov]

In D44764#1045648, @simark wrote:

We could create a file `ClangdTesting.h" which includes everything tested related (gtest, gmock, printers, syncapi, etc). The convention would be that test files would just include that.

Yeah, sounds good. I would exclude syncapi from the list, though. Not all tests should necessarily depend on ClangdServer.
@sammccall , @ioeric , @hokein, @bkramer are you on board with the idea to have an umbrella header for gtest+gmock+printers?

[malaperle]

In D44764#1045682, @ilya-biryukov wrote:

In D44764#1045648, @simark wrote:

We could create a file `ClangdTesting.h" which includes everything tested related (gtest, gmock, printers, syncapi, etc). The convention would be that test files would just include that.

Yeah, sounds good. I would exclude syncapi from the list, though. Not all tests should necessarily depend on ClangdServer.
@sammccall , @ioeric , @hokein, @bkramer are you on board with the idea to have an umbrella header for gtest+gmock+printers?

Sounds good to me too. I can update the patch once there is an agreement.

[malaperle]

Use a header for common inclusions for tests

[simark]

unittests/clangd/Printers.h

12 ↗

(On Diff #139598)

To be more precise, I think it's linker keeping only one of the two versions, so it's _all_ tests, not some, that may end up calling the wrong (default) one.

[sammccall]

In D44764#1045992, @malaperle wrote:

In D44764#1045682, @ilya-biryukov wrote:

In D44764#1045648, @simark wrote:

We could create a file `ClangdTesting.h" which includes everything tested related (gtest, gmock, printers, syncapi, etc). The convention would be that test files would just include that.

Yeah, sounds good. I would exclude syncapi from the list, though. Not all tests should necessarily depend on ClangdServer.
@sammccall , @ioeric , @hokein, @bkramer are you on board with the idea to have an umbrella header for gtest+gmock+printers?

Sounds good to me too. I can update the patch once there is an agreement.

(Sorry, have been OOO)
I understand the template instantiations here are a mess and we need to solve the problem. However I'm not sure this is the right direction:

• it violates IWYU, and we expect people consistently to get gmock transitively via this header. This goes against common practice, is inconsistent with the rest of LLVM, and confuses tools (including clangd itself!). It would need to be carefully documented, but I don't think this is enough.
• it's fragile - as soon as one test includes gmock directly we have this problem again
• printers.h takes a horizontal slice of the types we have - it seems to suffer from the same problem as "not all tests should depend on clangdserver".

I'd suggest instead:

• where there's a fairly nice general-purpose debugging representation of a type, we should name that operator<< and declare it in the header alongside the type. That way it's clear that it's available, there's no risk of ODR violation, and it can be used for debugging as well as by gtest.
  • where there isn't one (or it's insufficiently detailed), we should use something more primitive like EXPECT_THAT(foo, ...) << dump(foo) with dump defined locally in the cpp file. We can't share these, but these aren't really sharable anyway. This isn't ideal (nested matcher messages) but should be good enough in practice.

[ilya-biryukov]

In D44764#1046766, @sammccall wrote:

You raise a good point. If we can get away with using operator << everywhere, it is definitely the best approach.
Then our rule of thumb is: never define PrintTo overload, define operator << for a type instead, right?

[malaperle]

In D44764#1046766, @sammccall wrote:

I understand the template instantiations here are a mess and we need to solve the problem. However I'm not sure this is the right direction:

• it violates IWYU, and we expect people consistently to get gmock transitively via this header. This goes against common practice, is inconsistent with the rest of LLVM, and confuses tools (including clangd itself!). It would need to be carefully documented, but I don't think this is enough.

I didn't know LLVM code was IWYU, is this mentioned in the guide-line? I would think for tests this can be more lax maybe. I'm curious what you mean though by confusing the tools?

• it's fragile - as soon as one test includes gmock directly we have this problem again
• printers.h takes a horizontal slice of the types we have - it seems to suffer from the same problem as "not all tests should depend on clangdserver".

I'd suggest instead:

• where there's a fairly nice general-purpose debugging representation of a type, we should name that operator<< and declare it in the header alongside the type. That way it's clear that it's available, there's no risk of ODR violation, and it can be used for debugging as well as by gtest.
  • where there isn't one (or it's insufficiently detailed), we should use something more primitive like EXPECT_THAT(foo, ...) << dump(foo) with dump defined locally in the cpp file. We can't share these, but these aren't really sharable anyway. This isn't ideal (nested matcher messages) but should be good enough in practice.

Yes, it is fragile but even using operator<<, we can still have the problem of anyone adding a Printer, which is normally how gtest is used. So I think either way is going to be fragile. But I don't mind either way, so I'll change the patch for operator<<

[malaperle]

Use operator<< where we can

[malaperle]

unittests/clangd/JSONExprTests.cpp

19

This one I couldn't change to operator<< because there was already one defined with raw_ostream. But this just means losing indentation so perhaps that's not too bad?

[sammccall]

clangd/Protocol.h

673

I think raw_ostream should work fine here, it's what we've done elsewhere.
Is there a reason to use std::ostream?

(llvm's gtest is specially modified so raw_ostream printers can be used)

unittests/clangd/JSONExprTests.cpp

19

yeah, I think this is fine.

[sammccall]

(Thanks for sorting this out BTW!)

[malaperle]

Use raw_ostream instead.

[malaperle]

clangd/Protocol.h

673

No reason, I changed it.

[malaperle]

@sammccall Are you OK with the latest version? Thanks!

[sammccall]

Sorry I lost track of this. LGTM!

[malaperle]

Thanks!

clangd/Protocol.h

struct CompletionItem {
   llvm::Optional<Command> command;
   // TODO(krasimir): The following optional fields defined by the language
   // server protocol are unsupported:
   //
   // data?: any - A data entry field that is preserved on a completion item
   //              between a completion and a completion resolve request.
 };
 json::Expr toJSON(const CompletionItem &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const CompletionItem &);
 
 bool operator<(const CompletionItem &, const CompletionItem &);
 
 /// Represents a collection of completion items to be presented in the editor.
 struct CompletionList {
   /// The list is not complete. Further typing should result in recomputing the
   /// list.
   bool isIncomplete = false;
struct SignatureInformation {
 
   /// The documentation of this signature. Optional.
   std::string documentation;
 
   /// The parameters of this signature.
   std::vector<ParameterInformation> parameters;
 };
 json::Expr toJSON(const SignatureInformation &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &,
+                              const SignatureInformation &);
 
 /// Represents the signature of a callable.
 struct SignatureHelp {
 
   /// The resulting signatures.
   std::vector<SignatureInformation> signatures;
 
   /// The active signature.
struct DocumentHighlight {
   }
 
   friend bool operator==(const DocumentHighlight &LHS,
                          const DocumentHighlight &RHS) {
     return LHS.kind == RHS.kind && LHS.range == RHS.range;
   }
 };
 json::Expr toJSON(const DocumentHighlight &DH);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DocumentHighlight &);
 
 } // namespace clangd
 } // namespace clang
 
 namespace llvm {
 template <> struct format_provider<clang::clangd::Position> {
   static void format(const clang::clangd::Position &Pos, raw_ostream &OS,
                      StringRef Style) {
     assert(Style.empty() && "style modifiers for this type are not supported");
     OS << Pos;
   }
 };
 } // namespace llvm
 
 #endif

clangd/Protocol.cpp

if (CI.textEdit)
     Result["textEdit"] = *CI.textEdit;
   if (!CI.additionalTextEdits.empty())
     Result["additionalTextEdits"] = json::ary(CI.additionalTextEdits);
   if (CI.command)
     Result["command"] = *CI.command;
   return std::move(Result);
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &O, const CompletionItem &I) {
+  O << I.label << " - " << toJSON(I);
+  return O;
+}
+
 bool operator<(const CompletionItem &L, const CompletionItem &R) {
   return (L.sortText.empty() ? L.label : L.sortText) <
          (R.sortText.empty() ? R.label : R.sortText);
 }
 
 json::Expr toJSON(const CompletionList &L) {
   return json::obj{
       {"isIncomplete", L.isIncomplete},
json::obj Result{
       {"label", SI.label},
       {"parameters", json::ary(SI.parameters)},
   };
   if (!SI.documentation.empty())
     Result["documentation"] = SI.documentation;
   return std::move(Result);
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &O,
+                              const SignatureInformation &I) {
+  O << I.label << " - " << toJSON(I);
+  return O;
+}
+
 json::Expr toJSON(const SignatureHelp &SH) {
   assert(SH.activeSignature >= 0 &&
          "Unexpected negative value for number of active signatures.");
   assert(SH.activeParameter >= 0 &&
          "Unexpected negative value for active parameter index");
   return json::obj{
       {"activeSignature", SH.activeSignature},
       {"activeParameter", SH.activeParameter},

 
 json::Expr toJSON(const DocumentHighlight &DH) {
   return json::obj{
       {"range", toJSON(DH.range)},
       {"kind", static_cast<int>(DH.kind)},
   };
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &O,
+                              const DocumentHighlight &V) {
+  O << V.range;
+  if (V.kind == DocumentHighlightKind::Read)
+    O << "(r)";
+  if (V.kind == DocumentHighlightKind::Write)
+    O << "(w)";
+  return O;
+}
+
 bool fromJSON(const json::Expr &Params, DidChangeConfigurationParams &CCP) {
   json::ObjectMapper O(Params);
   return O && O.map("settings", CCP.settings);
 }
 
 bool fromJSON(const json::Expr &Params, ClangdConfigurationParamsChange &CCPC) {
   json::ObjectMapper O(Params);
   return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
 }
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/CodeCompleteTests.cpp

 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "index/MemIndex.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
-// Let GMock print completion items and signature help.
-void PrintTo(const CompletionItem &I, std::ostream *O) {
-  llvm::raw_os_ostream OS(*O);
-  OS << I.label << " - " << toJSON(I);
-}
-void PrintTo(const std::vector<CompletionItem> &V, std::ostream *O) {
-  *O << "{\n";
-  for (const auto &I : V) {
-    *O << "\t";
-    PrintTo(I, O);
-    *O << "\n";
-  }
-  *O << "}";
-}
-void PrintTo(const SignatureInformation &I, std::ostream *O) {
-  llvm::raw_os_ostream OS(*O);
-  OS << I.label << " - " << toJSON(I);
-}
-void PrintTo(const std::vector<SignatureInformation> &V, std::ostream *O) {
-  *O << "{\n";
-  for (const auto &I : V) {
-    *O << "\t";
-    PrintTo(I, O);
-    *O << "\n";
-  }
-  *O << "}";
-}
 
 namespace {
 using namespace llvm;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;

unittests/clangd/JSONExprTests.cpp

 #include "JSONExpr.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace json {
-void PrintTo(const Expr &E, std::ostream *OS) {
-  llvm::raw_os_ostream(*OS) << llvm::formatv("{0:2}", E);
-}
+
 namespace {
 
 std::string s(const Expr &E) { return llvm::formatv("{0}", E).str(); }
 std::string sp(const Expr &E) { return llvm::formatv("{0:2}", E).str(); }
 
 TEST(JSONExprTests, Types) {
   EXPECT_EQ("true", s(true));
   EXPECT_EQ("null", s(nullptr));

unittests/clangd/XRefsTests.cpp

 #include "llvm/Support/Path.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
-void PrintTo(const DocumentHighlight &V, std::ostream *O) {
-  llvm::raw_os_ostream OS(*O);
-  OS << V.range;
-  if (V.kind == DocumentHighlightKind::Read)
-    OS << "(r)";
-  if (V.kind == DocumentHighlightKind::Write)
-    OS << "(w)";
-}
-
 namespace {
 using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {