[PATCH] D136567: [clangd] Avoid hanging in Selection when PP corrects the token sequence.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Clang's error correction can invent extra tokens, including at the "eof"
position of various FileIDs.
(I thought this only happened at the AST level - I was mistaken).

Currently this can lead to Selection looping forever unable to make progress
because the location is not in range.
Unfortunately after fixing this the selection we get may be garbage: it can
also invent tokens whose locations coincide with real ones. Nevertheless
this is better than asserting/hanging.

Fixes https://github.com/llvm/llvm-project/issues/58482


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136567

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -697,6 +697,24 @@
   EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());
 }
 
+TEST(SelectionTest, MacroInitListCorrection) {
+  // This macro use is broken: the , is unparenthesized and so forms two args.
+  // clang can try to repair it, inserting extra parentheses.
+  // However these parens have dubious locations, we used to infloop here.
+  // https://github.com/llvm/llvm-project/issues/58482
+  const char *Case = R"cpp(
+    struct SS{int x,y;};
+    #define ID(X) X
+    auto s = ID(S^S{1,2}); // error-ok
+  )cpp";
+  auto AST = TestTU::withCode(Annotations(Case).code()).build();
+  auto T = makeSelectionTree(Case, AST);
+  // Ideally we would select RecordLoc, but the token sequence emitted by the
+  // preprocessor is confusing (there's an l_paren with the same loc as SS).
+  // Let's just be happy with selecting something and not crashing.
+  EXPECT_NE(nullptr, T.commonAncestor());
+}
+
 TEST(SelectionTest, IncludedFile) {
   const char *Case = R"cpp(
     void test() {
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -324,7 +324,11 @@
       // Comparing SourceLocations against bounds is cheaper than getFileID().
       SourceLocation Limit = SM.getComposedLoc(FID, SM.getFileIDSize(FID));
       auto Batch = ExpandedTokens.take_while([&](const syntax::Token &T) {
-        return T.location() >= Start && T.location() < Limit;
+        // [Start, Limit) rather than [Start, Limit] seems correct.
+        // However PP error-correction can produce one-past-the-end locations.
+        // (The selections we produce may be dubious, but we won't hang!)
+        // See https://github.com/llvm/llvm-project/issues/58482
+        return T.location() >= Start && T.location() <= Limit;
       });
       assert(!Batch.empty());
       ExpandedTokens = ExpandedTokens.drop_front(Batch.size());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D136567.470017.patch
Type: text/x-patch
Size: 2228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221023/b240a2fa/attachment.bin>