[clangd] Return earlier when snippet is empty

[tom-anders]

Summary

Fixes github.com/clangd/clangd/issues/1216

If the Snippet string is empty, Snippet.front() would trigger a crash.
Move the Snippet->empty() check up a few lines to avoid this. Should not
break any existing behavior.

[nridge]

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

1017

Does this test trigger the crash for you when run without the fix?

[tom-anders]

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

1017

Yes it does, I double checked. Not sure why it didn't work for you

[kadircet]

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

1017

it doesn't trigger the crash for me either.

[tom-anders]

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

1017

Huh, maybe this is a b

1017

Hmm could be a build type thing? I ran the tests for a Debug build, maybe it's different in Release?

Maybe we could also ask the author of the GitHub issue to check if it fixes the crash for them?

[nridge]

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

1017

Ok, I verified that the test case does produce completion proposals with empty snippets, i.e. it does trigger the problematic code path, it just happens not to crash for me. I think that's fine.

Thank you for tracking this down and fixing!

[tom-anders]

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

1017

np, thanks for the review :)

[sammccall]

It's not surprising that we often don't crash here, the "obvious" lowering of S.front() is *S.data() which is perfectly valid (will be 0 for an empty string).

One way to crash on this is to build with libstdc++'s debug iterators, which assert on this. Fedora seems to be building (some of) their packages in this mode, such that they crash but (sometimes?) do not print the assertion message. This was the cause in https://github.com/clangd/vscode-clangd/issues/400.

[tom-anders]

In D134137#3877726, @sammccall wrote:

It's not surprising that we often don't crash here, the "obvious" lowering of S.front() is *S.data() which is perfectly valid (will be 0 for an empty string).

One way to crash on this is to build with libstdc++'s debug iterators, which assert on this. Fedora seems to be building (some of) their packages in this mode, such that they crash but (sometimes?) do not print the assertion message. This was the cause in https://github.com/clangd/vscode-clangd/issues/400.

Interesting, FYI I was able to reproduce this on Arch Linux.

clang-tools-extra/clangd/CodeComplete.cpp

if (IsUsingDeclaration)
       return "";
     auto *Snippet = onlyValue<&BundledEntry::SnippetSuffix>();
     if (!Snippet)
       // All bundles are function calls.
       // FIXME(ibiryukov): sometimes add template arguments to a snippet, e.g.
       // we need to complete 'forward<$1>($0)'.
       return "($0)";
 
+    if (Snippet->empty())
+      return "";
+
     bool MayHaveArgList = Completion.Kind == CompletionItemKind::Function ||
                           Completion.Kind == CompletionItemKind::Method ||
                           Completion.Kind == CompletionItemKind::Constructor ||
                           Completion.Kind == CompletionItemKind::Text /*Macro*/;
     // If likely arg list already exists, don't add new parens & placeholders.
     //   Snippet: function(int x, int y)
     //   func^(1,2) -> function(1, 2)
     //             NOT function(int x, int y)(1, 2)
if (MayHaveArgList) {
         }
         return "";
       }
     }
     if (EnableFunctionArgSnippets)
       return *Snippet;
 
     // Replace argument snippets with a simplified pattern.
-    if (Snippet->empty())
-      return "";
     if (MayHaveArgList) {
       // Functions snippets can be of 2 types:
       // - containing only function arguments, e.g.
       //   foo(${1:int p1}, ${2:int p2});
       //   We transform this pattern to '($0)' or '()'.
       // - template arguments and function arguments, e.g.
       //   foo<${1:class}>(${2:int p1}).
       //   We transform this pattern to '<$1>()$0' or '<$0>()'.

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

void f() {
       clan^
     }
   )cpp");
 
   EXPECT_THAT(Results.Completions, Contains(labeled("clang")));
   EXPECT_THAT(Results.Completions, Not(Contains(labeled("clang::"))));
 }
 
+TEST(CompletionTests, EmptySnippetDoesNotCrash) {
+    // See https://github.com/clangd/clangd/issues/1216
+    auto Results = completions(R"cpp(
+        int main() {
+          auto w = [&](auto &&f) { return f(f); };
+          auto f = w([&](auto &&f) {
+            return [&](auto &&n) {
+              if (n == 0) {
+                return 1;
+              }
+              return n * ^(f)(n - 1);
+            };
+          })(10);
+        }
+    )cpp");
+}
+
 TEST(CompletionTest, BacktrackCrashes) {
   // Sema calls code completion callbacks twice in these cases.
   auto Results = completions(R"cpp(
       namespace ns {
       struct FooBarBaz {};
       } // namespace ns
 
      int foo(ns::FooBar^