[clangd] Fix an assertion failure in Selection.

[hokein]

Summary

The assertion is triggered when the Decl is null. I didn't have a small
reproduce testcase, but this failure happened internally.

Details for the assertion:

F0207 09:55:09.069385 47308 logging.cc:84] assert.h assertion failed at llvm/include/llvm/Support/Casting.h:105 in static bool llvm::isa_impl_cl<clang::TranslationUnitDecl, const clang:: Decl *>::doit(const From *) [To = clang::TranslationUnitDecl, From = const clang::Decl *]: Val && "isa<> used on a null pointer"
15 * Check failure stack trace: *
19 @ 0x55615c1f7e06 __assert_fail
20 @ 0x55615a6297d8 clang::clangd::(anonymous namespace)::SelectionVisitor::TraverseDecl()
21 @ 0x55615a62f48d clang::RecursiveASTVisitor<>::TraverseTemplateTemplateParmDecl()
22 @ 0x55615a62b264 clang::RecursiveASTVisitor<>::TraverseDecl()
23 @ 0x55615a62979c clang::clangd::(anonymous namespace)::SelectionVisitor::TraverseDecl()
24 @ 0x55615a63060c clang::RecursiveASTVisitor<>::TraverseClassTemplatePartialSpecializationDecl()
25 @ 0x55615a62ae45 clang::RecursiveASTVisitor<>::TraverseDecl()

[ilya-biryukov]

Repro:

template <class T>
struct Foo {};

template <template<class> class /*cursor here*/U>
struct Foo<U<int>*> {};

I'm not sure how easy is that, but this should probably be fixed in the RecursiveASTVisitor<>. There's really no point in calling TraverseDecl(null)

[sammccall]

In D57879#1388789, @ilya-biryukov wrote:

Repro:

template <class T>
struct Foo {};

template <template<class> class /*cursor here*/U>
struct Foo<U<int>*> {};

I'm not sure how easy is that, but this should probably be fixed in the RecursiveASTVisitor<>. There's really no point in calling TraverseDecl(null)

Ultimately, but the current code has null-checks everywhere except this spot, so I think we should land this first before boiling the ocean.

[hokein]

Add reproduce testcase.

[hokein]

Remove an unexpected change.

clang-tools-extra/trunk/clangd/Selection.cpp

public:
   //  - hit testing: is intersection(node, selection) - union(children) empty?
   //  - attach it to the tree if it or any children hit the selection
   //
   // Two categories of nodes are not "well-behaved":
   //  - those without source range information, we don't record those
   //  - those that can't be stored in DynTypedNode.
   // We're missing some interesting things like Attr due to the latter.
   bool TraverseDecl(Decl *X) {
-    if (isa<TranslationUnitDecl>(X))
+    if (X && isa<TranslationUnitDecl>(X))
       return Base::TraverseDecl(X); // Already pushed by constructor.
     return traverseNode(X, [&] { return Base::TraverseDecl(X); });
   }
   bool TraverseTypeLoc(TypeLoc X) {
     return traverseNode(&X, [&] { return Base::TraverseTypeLoc(X); });
   }
   bool TraverseTypeNestedNameSpecifierLoc(NestedNameSpecifierLoc X) {
     return traverseNode(

clang-tools-extra/trunk/unittests/clangd/SelectionTests.cpp

Case Cases[] = {
 
       // FIXME: Ideally we'd get a declstmt or the VarDecl itself here.
       // This doesn't happen now; the RAV doesn't traverse a node containing ;.
       {"int x = 42;^", nullptr},
       {"int x = 42^;", nullptr},
 
       // Node types that have caused problems in the past.
       {"template <typename T> void foo() { [[^T]] t; }", "TypeLoc"},
+
+      // No crash
+      {
+          R"cpp(
+            template <class T> struct Foo {};
+            template <[[template<class> class /*cursor here*/^U]]>
+             struct Foo<U<int>*> {};
+          )cpp",
+          "TemplateTemplateParmDecl"
+      },
   };
   for (const Case &C : Cases) {
     Annotations Test(C.Code);
     auto AST = TestTU::withCode(Test.code()).build();
     auto T = makeSelectionTree(C.Code, AST);
 
     if (Test.ranges().empty()) {
       // If no [[range]] is marked in the example, there should be no selection.