[clangd] Do not trigger go-to-def textual fallback inside string literals

[nridge]

This is my attempt to avoid triggering the textual fallback for string literals, as per the discussion in the issue.

To classify tokens into the categories discussed in the issue, I resurrected and modified the getTokenFlavor() function that was removed in D75176.

However, it does not seem to be working as I expect -- Lexer::getRawToken() is returning TokenKind::raw_identifier inside the string literal rather than TokenKind::string_literal.

Am I misunderstanding how this API is supposed to work? I thought "raw" meant "no preprocessor", and things like string literals would still be recognized in raw mode.

[kadircet]

clang-tools-extra/clangd/XRefs.cpp

366

you can rather use AST.getTokens().spelledTokenAt(Loc) to get preprocessed token, and pass that into getTokenFlavor rather than a SourceLocation.

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

360

I don't think we need this function, which just maps one enum onto another - just use the token kind directly?

366

for a bit more context: running Lexer::getRawToken runs a raw lex that only sees the piece of text you point at. If you're inside a huge comment, it won't know that.

Using AST.getTokens() uses the results of an earlier raw lex of the whole file.

[nridge]

clang-tools-extra/clangd/XRefs.cpp

366

Hmm, I've tried this and spelledTokenAt() seems to return null for comment tokens.

[nridge]

clang-tools-extra/clangd/XRefs.cpp

366

It looks like there are two reasons for this:

• The lexer that is producing the token buffer is using inKeepCommentMode() == false
• spelledTokenAt() only returns a result if you give it the location at the beginning of the token, not something in the middle

[nridge]

Use TokenBuffer instead of a raw lexer.

Note that getting this to word required enabling comment-retention mode for
the lexer which produces the TokenBuffer. I'm not sure if this is desirable
in general.

[sammccall]

clang-tools-extra/clangd/XRefs.cpp

367

this means you're not going to resolve foo in a.^foo (you're touching two tokens).

The cleanest thing seems to be to use the word you've identified: iterate over the spelledTokensTouching(WordStart) and accept the one where tok.range(SM).touches(WordOffset + Word.size())

clang/lib/Tooling/Syntax/Tokens.cpp

343 ↗

(On Diff #250885)

Yikes, I didn't remember TokenBuffer doesn't currently record comment tokens.
I'm afraid this isn't a trivial change and would definitely need tests to verify it doesn't interfere with translating between spelled/expanded tokens (I'm pretty sure there are tests that comments *aren't* retained now in TokensTest.cpp, part of SyntaxTests)

Given that, the shorter route for this patch would be to blacklist string literals rather than whitelisting comments + identifiers.

[nridge]

Take a blacklisting approach

[nridge]

clang-tools-extra/clangd/XRefs.cpp

381

Whoops, meant to use WordStart rather than Loc here.

[nridge]

Use WordStart

clang-tools-extra/clangd/XRefs.cpp

bool tokenSurvivedPreprocessing(SourceLocation Loc,
                                 const syntax::TokenBuffer &TB) {
   auto WordExpandedTokens =
       TB.expandedTokens(TB.sourceManager().getMacroArgExpandedLocation(Loc));
   return !WordExpandedTokens.empty();
 }
 
 } // namespace
 
 std::vector<LocatedSymbol>
 locateSymbolNamedTextuallyAt(ParsedAST &AST, const SymbolIndex *Index,
                              SourceLocation Loc,
                              const std::string &MainFilePath) {
   const auto &SM = AST.getSourceManager();
 
   // Get the raw word at the specified location.
   unsigned Pos;
   FileID File;
   std::tie(File, Pos) = SM.getDecomposedLoc(Loc);
   llvm::StringRef Code = SM.getBufferData(File);
   llvm::StringRef Word = wordTouching(Code, Pos);
   if (Word.empty())
     return {};
   unsigned WordOffset = Word.data() - Code.data();
   SourceLocation WordStart = SM.getComposedLoc(File, WordOffset);
 
+  // Attempt to determine the kind of token that contains the word,
+  // and bail if it's a string literal. Note that we cannot always
+  // determine the token kind (e.g. comments, for which we do want
+  // to activate, are not retained by TokenBuffer).
+  for (syntax::Token T :
+       syntax::spelledTokensTouching(WordStart, AST.getTokens())) {
+    if (T.range(AST.getSourceManager()).touches(WordOffset + Word.size())) {
+      if (isStringLiteral(T.kind()))
+        return {};
+    }
+  }
+
   // Do not consider tokens that survived preprocessing.
   // We are erring on the safe side here, as a user may expect to get
   // accurate (as opposed to textual-heuristic) results for such tokens.
   // FIXME: Relax this for dependent code.
   if (tokenSurvivedPreprocessing(WordStart, AST.getTokens()))
     return {};
 
   // Additionally filter for signals that the word is likely to be an

clang-tools-extra/clangd/unittests/XRefsTests.cpp

 
 TEST(LocateSymbol, Textual) {
   const char *Tests[] = {
       R"cpp(// Comment
         struct [[MyClass]] {};
         // Comment mentioning M^yClass
       )cpp",
       R"cpp(// String
-        struct [[MyClass]] {};
+        struct MyClass {};
+        // Not triggered for string literal tokens.
         const char* s = "String literal mentioning M^yClass";
       )cpp",
       R"cpp(// Ifdef'ed out code
         struct [[MyClass]] {};
         #ifdef WALDO
           M^yClass var;
         #endif
       )cpp",
for (const char *Test : Tests) {
 
     if (!WantDecl) {
       EXPECT_THAT(Results, IsEmpty()) << Test;
     } else {
       ASSERT_THAT(Results, ::testing::SizeIs(1)) << Test;
       EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test;
     }
   }
-}
+} // namespace
 
 TEST(LocateSymbol, Ambiguous) {
   auto T = Annotations(R"cpp(
     struct Foo {
       Foo();
       Foo(Foo&&);
       $ConstructorLoc[[Foo]](const char*);
     };