[clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting

[nridge]

Summary

Fixes https://github.com/clangd/clangd/issues/1313

[nridge]

Speculative fix for an infinite recursion with no testcase. I'm happy to circle back to this if/when we get a testcase, but I don't think there's any downside to this patch in the meantime.

[sammccall]

Based on the stacktrace, I came up with this crashing example, maybe it's a usable testcase?

template <int> class X {
template <int> class Y {
    using Y<0>::xxx;
};
};

(not currently on a machine that can build clangd, so I haven't verified it's the same crash)

Another interesting indirect case

template <int> class X {
template <int> class Z;
template <int> class Y {
    using Z<0>::xxx;
};
template <int> class Z {
    using Y<0>::xxx;
};
};

clang-tools-extra/clangd/SemanticHighlighting.cpp

143

This looks good+safe.

I wonder if the check should be moved inside resolveUsingValueDecl - having it resolve to itself seems unhelpful always.

[nridge]

Thanks for the testcases!

The indirect one is particularly interesting, and suggests that we may want a "seen decls" set (similar to SeenTemplates here) in the kindForDecl() recursion?

(I'd move the check into resolveUsingValueDecl(), but that goes against the grain of putting the more thorough check in the caller -- or should we do both?)

[nridge]

(I'm going to re-request review since I have an outstanding question about the fix approach.)

[nridge]

add test case

[nridge]

clang-tools-extra/clangd/SemanticHighlighting.cpp

143

I wonder if the check should be moved inside resolveUsingValueDecl - having it resolve to itself seems unhelpful always.

I haven't done this because, as mentioned earlier, I think a fix for the mutually-recursive case will need to build on logic here rather than inside resolveUsingValueDecl.

[nridge]

I will go ahead and merge this partial fix because it is an improvement over the current state.

@sammccall I'm still curious to know your thoughts on the approach for fixing the mutually-recursive case. Is there something more general than adding a recursion guard like this to semantic highlighting code that I'm overlooking?

we may want a "seen decls" set (similar to SeenTemplates here) in the kindForDecl() recursion

[thakis]

This breaks tests: http://45.33.8.238/linux/102084/step_9.txt

Please take a look and revert for now if it takes a while to fix.

Did you not run tests before commiting?

[nridge]

My apologies. I did run tests locally but on a slightly older tree. I'll post a fix momentarily.

[nridge]

Should be fixed in https://reviews.llvm.org/rG9d042ed1a4f4.

clang-tools-extra/clangd/SemanticHighlighting.cpp

if (isa<NamespaceDecl>(D) || isa<NamespaceAliasDecl>(D) ||
     return HighlightingKind::Namespace;
   if (isa<TemplateTemplateParmDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
       isa<NonTypeTemplateParmDecl>(D))
     return HighlightingKind::TemplateParameter;
   if (isa<ConceptDecl>(D))
     return HighlightingKind::Concept;
   if (const auto *UUVD = dyn_cast<UnresolvedUsingValueDecl>(D)) {
     auto Targets = Resolver->resolveUsingValueDecl(UUVD);
-    if (!Targets.empty()) {
+    if (!Targets.empty() && Targets[0] != UUVD) {
       return kindForDecl(Targets[0], Resolver);
     }
     return HighlightingKind::Unknown;
   }
   return llvm::None;
 }
 llvm::Optional<HighlightingKind>
 kindForType(const Type *TP, const HeuristicResolver *Resolver) {