[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

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

391

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();
 
+  auto Tokens = syntax::spelledTokensTouching(Loc, AST.getTokens());
+  if (Tokens.size() != 1)
+    return {};
+  syntax::Token Tok = Tokens[0];
+
+  // Only consider comment and identifier tokens.
+  if (!(Tok.kind() == tok::TokenKind::comment ||
+        Tok.kind() == tok::TokenKind::identifier))
+    return {};
+
   // 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);
 
-  // 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.
+  // If this is an identifier token, do not consider if it it 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()))
+  if (Tok.kind() == tok::TokenKind::identifier &&
+      tokenSurvivedPreprocessing(WordStart, AST.getTokens())) {
     return {};
+  }
 
   // Additionally filter for signals that the word is likely to be an
   // identifier. This avoids triggering on e.g. random words in a comment.
   if (!isLikelyToBeIdentifier(Word))
     return {};
 
   // Look up the selected word in the index.
   FuzzyFindRequest Req;

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*);
     };

clang/lib/Tooling/Syntax/Tokens.cpp

std::vector<syntax::Token> syntax::tokenize(const FileRange &FR,
   };
 
   auto SrcBuffer = SM.getBufferData(FR.file());
   Lexer L(SM.getLocForStartOfFile(FR.file()), LO, SrcBuffer.data(),
           SrcBuffer.data() + FR.beginOffset(),
           // We can't make BufEnd point to FR.endOffset, as Lexer requires a
           // null terminated buffer.
           SrcBuffer.data() + SrcBuffer.size());
+  L.SetCommentRetentionState(true);
 
   clang::Token T;
   while (!L.LexFromRawLexer(T) && L.getCurrentBufferOffset() < FR.endOffset())
     AddToken(T);
   // LexFromRawLexer returns true when it parses the last token of the file, add
   // it iff it starts within the range we are interested in.
   if (SM.getFileOffset(T.getLocation()) < FR.endOffset())
     AddToken(T);