[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

13

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 ↗

(On Diff #140180)

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 ↗

(On Diff #140180)

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!

unittests/clangd/ClangdTests.cpp

 //===-- ClangdTests.cpp - Clangd unit tests ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
+#include "Printers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "URI.h"
 #include "clang/Config/config.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"

unittests/clangd/ClangdUnitTests.cpp

 //===-- ClangdUnitTests.cpp - ClangdUnit tests ------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "ClangdUnit.h"
+#include "Printers.h"
 #include "TestFS.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 

unittests/clangd/CodeCompleteTests.cpp

 //===-- CodeCompleteTests.cpp -----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "ClangdServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
 #include "Matchers.h"
+#include "Printers.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #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/CodeCompletionStringsTests.cpp

 //===-- CodeCompletionStringsTests.cpp --------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeCompletionStrings.h"
+#include "Printers.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 

unittests/clangd/ContextTests.cpp

 //===-- ContextTests.cpp - Context tests ------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Context.h"
+#include "Printers.h"
 
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 
 TEST(ContextTests, Simple) {
   Key<int> IntParam;

unittests/clangd/FileIndexTests.cpp

 //===-- FileIndexTests.cpp  ---------------------------*- C++ -*-----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Printers.h"
 #include "TestFS.h"
 #include "index/FileIndex.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 

unittests/clangd/FuzzyMatchTests.cpp

 //===-- FuzzyMatchTests.cpp - String fuzzy matcher tests --------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "FuzzyMatch.h"
+#include "Printers.h"
 
 #include "llvm/ADT/StringExtras.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {

unittests/clangd/HeadersTests.cpp

 //===-- HeadersTests.cpp - Include headers unit tests -----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Headers.h"
+#include "Printers.h"
 #include "TestFS.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 

unittests/clangd/IndexTests.cpp

 //===-- IndexTests.cpp  -------------------------------*- C++ -*-----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Printers.h"
 #include "index/Index.h"
 #include "index/MemIndex.h"
 #include "index/Merge.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using testing::UnorderedElementsAre;
 using testing::Pointee;

unittests/clangd/JSONExprTests.cpp

 //===-- JSONExprTests.cpp - JSON expression unit tests ----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "JSONExpr.h"
-
+#include "Printers.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/Matchers.h

 //===-- Matchers.h ----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // GMock matchers that aren't specific to particular tests.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_MATCHERS_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_MATCHERS_H
+#include "Printers.h"
 #include "Protocol.h"
 #include "gmock/gmock.h"
 
 namespace clang {
 namespace clangd {
 using ::testing::Matcher;
 
 // EXPECT_IFF expects matcher if condition is true, and Not(matcher) if false.

unittests/clangd/Printers.h

+//===-- Printers.h ----------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Custom GTest Printers. This should be included in any test that prints
+// objects on failure. Otherwise, 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.
+// See UniversalPrint::Print and UniversalPrinter::Print.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_PRINTERS_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_PRINTERS_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "llvm/Support/raw_os_ostream.h"
+
+namespace clang {
+namespace clangd {
+
+namespace json {
+inline void PrintTo(const Expr &E, std::ostream *OS) {
+  llvm::raw_os_ostream(*OS) << llvm::formatv("{0:2}", E);
+}
+} // namespace json
+
+inline 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)";
+}
+
+inline void PrintTo(const CompletionItem &I, std::ostream *O) {
+  llvm::raw_os_ostream OS(*O);
+  OS << I.label << " - " << toJSON(I);
+}
+
+inline 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 << "}";
+}
+
+inline void PrintTo(const SignatureInformation &I, std::ostream *O) {
+  llvm::raw_os_ostream OS(*O);
+  OS << I.label << " - " << toJSON(I);
+}
+
+inline 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 clangd
+} // namespace clang
+
+#endif

unittests/clangd/SourceCodeTests.cpp

 //===-- SourceCodeTests.cpp  ------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
+#include "Printers.h"
 #include "SourceCode.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang{

unittests/clangd/SymbolCollectorTests.cpp

 //===-- SymbolCollectorTests.cpp  -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "Printers.h"
 #include "TestFS.h"
 #include "index/SymbolCollector.h"
 #include "index/SymbolYAML.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"

unittests/clangd/TUSchedulerTests.cpp

 //===-- TUSchedulerTests.cpp ------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Context.h"
+#include "Printers.h"
 #include "TUScheduler.h"
 #include "TestFS.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <utility>
 
 namespace clang {

unittests/clangd/TestFS.cpp

 //===-- TestFS.cpp ----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #include "TestFS.h"
+#include "Printers.h"
 #include "llvm/Support/Errc.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
 IntrusiveRefCntPtr<vfs::FileSystem>

unittests/clangd/TraceTests.cpp

 //===-- TraceTests.cpp - Tracing unit tests ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Printers.h"
 #include "Trace.h"
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/YAMLParser.h"
 #include "gmock/gmock.h"

unittests/clangd/URITests.cpp

 //===-- URITests.cpp  ---------------------------------*- C++ -*-----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Printers.h"
 #include "TestFS.h"
 #include "URI.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {

unittests/clangd/XRefsTests.cpp

 //===-- XRefsTests.cpp  ---------------------------*- C++ -*--------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #include "Annotations.h"
 #include "ClangdUnit.h"
 #include "Compiler.h"
 #include "Matchers.h"
+#include "Printers.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "XRefs.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
 #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 {