[clangd] Fix a nullptr-dereference crash in computeIncludeCleanerFindings.

[hokein]

Summary

Be more robust, we shuold not crash when we cannot find the corresponding token from the
tokenbuffer.

[hokein]

add a comment.

[kadircet]

could you give some more details about the crash, so that we actually now where to fix it? e.g. is it token buffers missing the spelled token at that offset somehow ? or is it AST reporting a weird source locations for user-defined literals and we need to do some sort of post processing (or maybe it should've been filtered all together). as doing it like this will imply us completely eating such diagnostics.

[hokein]

In D147686#4248330, @kadircet wrote:

could you give some more details about the crash, so that we actually now where to fix it? e.g. is it token buffers missing the spelled token at that offset somehow ? or is it AST reporting a weird source locations for user-defined literals and we need to do some sort of post processing (or maybe it should've been filtered all together). as doing it like this will imply us completely eating such diagnostics.

sorry, for not giving more details. See the other comment.

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

421

here is the UserDefinedLiteral AST node:

`-UserDefinedLiteral 0x5556682e4500 <line:46:3> 'int'
      |-ImplicitCastExpr 0x5556682e44e8 <col:4> 'int (*)(unsigned long long)' <FunctionToPointerDecay>
      | `-DeclRefExpr 0x5556682e44a0 <col:4> 'int (unsigned long long)' lvalue Function 0x5556682e4290 'operator""s' 'int (unsigned long long)'
      `-IntegerLiteral 0x5556682e4480 <col:3> 'unsigned long long' 1

The source location of the UserDefinedLiteral points to ^1s, while the source location of the inner DeclRefExpr points to 1^s (1 offset shift). In the token buffer, we only have a single spelled token 1s, thus AST.getTokens().spelledTokenAt can not find any token that starts at 1^s source loc.

429

T

[kadircet]

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

421

thanks, that makes sense!

then instead of "suppressing" these, can we use spelledTokensTouching().front() (and asserting that it isn't empty?)

[hokein]

use syntax::spelledTokensTouching.

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

421

Thanks, this is a good idea.

But we have to use the back(), for the case ns::^abc (we want to point to abc, not the ::).

clang-tools-extra/clangd/IncludeCleaner.cpp

include_cleaner::walkUsed(
         // file.
         // FIXME: Use presumed locations to map such usages back to patched
         // locations safely.
         auto Loc = SM.getFileLoc(Ref.RefLocation);
         // File locations can be outside of the main file if macro is expanded
         // through an #include.
         while (SM.getFileID(Loc) != SM.getMainFileID())
           Loc = SM.getIncludeLoc(SM.getFileID(Loc));
-        const auto *Token = AST.getTokens().spelledTokenAt(Loc);
-        MissingIncludeDiagInfo DiagInfo{Ref.Target, Token->range(SM),
-                                        Providers};
+        auto TouchingTokens =
+            syntax::spelledTokensTouching(Loc, AST.getTokens());
+        assert(!TouchingTokens.empty());
+        // Loc points to the start offset of the ref token, here we use the last
+        // element of the TouchingTokens, e.g. avoid getting the "::" for
+        // "ns::^abc".
+        MissingIncludeDiagInfo DiagInfo{
+            Ref.Target, TouchingTokens.back().range(SM), Providers};
         MissingIncludes.push_back(std::move(DiagInfo));
       });
   std::vector<const Inclusion *> UnusedIncludes =
       getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
   return {std::move(UnusedIncludes), std::move(MissingIncludes)};
 }
 
 std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,

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

)cpp");
   auto RefRange = Findings.front().SymRefRange;
   auto &SM = AST.getSourceManager();
   EXPECT_EQ(RefRange.file(), SM.getMainFileID());
   // FIXME: Point at the spelling location, rather than the include.
   EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), MainFile.range());
   EXPECT_EQ(RefRange, Findings[1].SymRefRange);
 }
 
+TEST(IncludeCleaner, NoCrash) {
+  TestTU TU;
+  Annotations MainCode(R"cpp(
+    #include "all.h"
+    void test() {
+      [[1s]];
+    }
+    )cpp");
+  TU.Code = MainCode.code();
+  TU.AdditionalFiles["foo.h"] =
+      guard("int operator\"\"s(unsigned long long) { return 0; }");
+  TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\"");
+  ParsedAST AST = TU.build();
+  const auto &MissingIncludes =
+      computeIncludeCleanerFindings(AST).MissingIncludes;
+  EXPECT_THAT(MissingIncludes, testing::SizeIs(1));
+  auto &SM = AST.getSourceManager();
+  EXPECT_EQ(
+      halfOpenToRange(SM, MissingIncludes.front().SymRefRange.toCharRange(SM)),
+      MainCode.range());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang