Page MenuHomePhabricator

[clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting
Needs ReviewPublic

Authored by nridge on Sep 28 2022, 1:00 PM.

Diff Detail

Event Timeline

nridge created this revision.Sep 28 2022, 1:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 1:00 PM
Herald added a subscriber: arphaman. · View Herald Transcript
nridge requested review of this revision.Sep 28 2022, 1:00 PM
nridge edited the summary of this revision. (Show Details)Sep 28 2022, 1:01 PM

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 accepted this revision.Sep 28 2022, 7:30 PM

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.

This revision is now accepted and ready to land.Sep 28 2022, 7:30 PM

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 requested review of this revision.Oct 30 2022, 12:56 AM

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