[CodeCompletion] Make sure callback is always called from performOperation

We had some situations left that neither returned an error, nor called the callback with results in `performOperation`. Return an error in these and adjust the tests to correctly match the error.

Author: ahoppen

include/swift/IDE/CancellableResult.h

@@ -78,7 +78,7 @@ class CancellableResult {
           ResultType(std::move(Other.Result));
       break;
     case CancellableResultKind::Failure:
-      Error = std::move(Other.Error);
+      ::new ((void *)std::addressof(Error)) std::string(std::move(Other.Error));
       break;
     case CancellableResultKind::Cancelled:
       break;

include/swift/IDE/CompletionInstance.h

@@ -42,6 +42,10 @@ struct CompletionInstanceResult {
   CompilerInstance &CI;
   /// Whether an AST was reused.
   bool DidReuseAST;
+  /// Whether the CompletionInstance found a code completion token in the source
+  /// file. If this is \c false, the user will most likely want to return empty
+  /// results.
+  bool DidFindCodeCompletionToken;
 };
 
 /// Manages \c CompilerInstance for completion like operations.

lib/IDE/CompletionInstance.cpp

@@ -506,7 +506,7 @@ bool CompletionInstance::performCachedOperationIfPossible(
       CI.addDiagnosticConsumer(DiagC);
 
     Callback(CancellableResult<CompletionInstanceResult>::success(
-        {CI, /*reusingASTContext=*/true}));
+        {CI, /*reusingASTContext=*/true, /*DidFindCodeCompletionToken=*/true}));
 
     if (DiagC)
       CI.removeDiagnosticConsumer(DiagC);
@@ -535,6 +535,7 @@ void CompletionInstance::performNewOperation(
   Invocation.getFrontendOptions().IntermoduleDependencyTracking =
       IntermoduleDepTrackingMode::ExcludeSystem;
 
+  bool DidFindCodeCompletionToken = false;
   {
     auto &CI = *TheInstance;
     if (DiagC)
@@ -561,24 +562,26 @@ void CompletionInstance::performNewOperation(
     // failed to load, let's bail early and hand back an empty completion
     // result to avoid any downstream crashes.
     if (CI.loadStdlibIfNeeded()) {
-      /// FIXME: Callback is not being called on this path.
+      Callback(CancellableResult<CompletionInstanceResult>::failure(
+          "failed to load the standard library"));
       return;
     }
 
     CI.performParseAndResolveImportsOnly();
 
-    // If we didn't find a code completion token, bail.
-    auto *state = CI.getCodeCompletionFile()->getDelayedParserState();
-    if (!state->hasCodeCompletionDelayedDeclState())
-      /// FIXME: Callback is not being called on this path.
-      return;
+    DidFindCodeCompletionToken = CI.getCodeCompletionFile()
+                                     ->getDelayedParserState()
+                                     ->hasCodeCompletionDelayedDeclState();
 
     Callback(CancellableResult<CompletionInstanceResult>::success(
-        {CI, /*ReuisingASTContext=*/false}));
+        {CI, /*ReuisingASTContext=*/false, DidFindCodeCompletionToken}));
   }
 
   // Cache the compiler instance if fast completion is enabled.
-  if (isCachedCompletionRequested)
+  // If we didn't find a code compleiton token, we can't cache the instance
+  // because performCachedOperationIfPossible wouldn't have an old code
+  // completion state to compare the new one to.
+  if (isCachedCompletionRequested && DidFindCodeCompletionToken)
     cacheCompilerInstance(std::move(TheInstance), *ArgsHash);
 }
 

test/SourceKit/CodeComplete/complete_without_stdlib.swift

@@ -9,12 +9,11 @@ class Str {
 // RUN: %empty-directory(%t/rsrc)
 // RUN: %empty-directory(%t/sdk)
 
-// RUN: %sourcekitd-test \
+// RUN: not %sourcekitd-test \
 // RUN:   -req=global-config -req-opts=completion_max_astcontext_reuse_count=0 \
-// RUN:   -req=complete -pos=4:1 %s -- %s -resource-dir %t/rsrc -sdk %t/sdk | %FileCheck %s
-// RUN: %sourcekitd-test \
+// RUN:   -req=complete -pos=4:1 %s -- %s -resource-dir %t/rsrc -sdk %t/sdk 2>&1 | %FileCheck %s
+// RUN: not %sourcekitd-test \
 // RUN:   -req=complete -pos=4:1 %s -- %s -resource-dir %t/rsrc -sdk %t/sdk == \
-// RUN:   -req=complete -pos=4:1 %s -- %s -resource-dir %t/rsrc -sdk %t/sdk | %FileCheck %s
+// RUN:   -req=complete -pos=4:1 %s -- %s -resource-dir %t/rsrc -sdk %t/sdk 2>&1 | %FileCheck %s
 
-// CHECK: key.results: [
-// CHECK-NOT: key.description:
+// CHECK: error response (Request Failed): failed to load the standard library

tools/SourceKit/lib/SwiftLang/SwiftCompletion.cpp

@@ -190,8 +190,15 @@ static void swiftCodeCompleteImpl(
               ide::makeCodeCompletionCallbacksFactory(CompletionContext,
                                                       Consumer));
 
-          auto *SF = CI.getCodeCompletionFile();
-          performCodeCompletionSecondPass(*SF, *callbacksFactory);
+          if (!Result->DidFindCodeCompletionToken) {
+            Callback(ResultType::success(
+                {/*HasResults=*/false, &CI.getASTContext(), &CI.getInvocation(),
+                 &CompletionContext, /*RequestedModules=*/{}, /*DC=*/nullptr}));
+            return;
+          }
+
+          performCodeCompletionSecondPass(*CI.getCodeCompletionFile(),
+                                          *callbacksFactory);
           if (!Consumer.HandleResultWasCalled) {
             // If we didn't receive a handleResult call from the second pass,
             // we didn't receive any results. To make sure Callback gets called

tools/SourceKit/lib/SwiftLang/SwiftConformingMethodList.cpp

@@ -81,6 +81,12 @@ static void swiftConformingMethodListImpl(
               ide::makeConformingMethodListCallbacksFactory(ExpectedTypeNames,
                                                             Consumer));
 
+          if (!Result->DidFindCodeCompletionToken) {
+            Callback(ResultType::success(
+                {/*Results=*/nullptr, Result->DidReuseAST}));
+            return;
+          }
+
           performCodeCompletionSecondPass(*Result->CI.getCodeCompletionFile(),
                                           *callbacksFactory);
           if (!Consumer.HandleResultWasCalled) {

tools/SourceKit/lib/SwiftLang/SwiftTypeContextInfo.cpp

@@ -75,6 +75,11 @@ static void swiftTypeContextInfoImpl(
           std::unique_ptr<CodeCompletionCallbacksFactory> callbacksFactory(
               ide::makeTypeContextInfoCallbacksFactory(Consumer));
 
+          if (!Result->DidFindCodeCompletionToken) {
+            Callback(
+                ResultType::success({/*Results=*/{}, Result->DidReuseAST}));
+          }
+
           performCodeCompletionSecondPass(*Result->CI.getCodeCompletionFile(),
                                           *callbacksFactory);
           if (!Consumer.HandleResultsCalled) {

tools/swift-ide-test/swift-ide-test.cpp

@@ -866,28 +866,48 @@ static bool doCodeCompletionImpl(
 
   PrintingDiagnosticConsumer PrintDiags;
   CompletionInstance CompletionInst;
-  // FIXME: Should we count it as an error if we didn't receive a callback?
-  bool HadError = false;
+  std::string CompletionError;
+  bool CallbackCalled = false;
   CompletionInst.performOperation(
       Invocation, /*Args=*/{}, llvm::vfs::getRealFileSystem(), CleanFile.get(),
       Offset, CodeCompletionDiagnostics ? &PrintDiags : nullptr,
       [&](CancellableResult<CompletionInstanceResult> Result) {
+        CallbackCalled = true;
         switch (Result.getKind()) {
         case CancellableResultKind::Success: {
-          HadError = false;
           assert(!Result->DidReuseAST &&
                  "reusing AST context without enabling it");
+
+          if (!Result->DidFindCodeCompletionToken) {
+            // Return empty results by not performing the second pass and never
+            // calling the consumer of the callback factory.
+            return;
+          }
+
           performCodeCompletionSecondPass(*Result->CI.getCodeCompletionFile(),
                                           *callbacksFactory);
           break;
         }
         case CancellableResultKind::Failure:
+          CompletionError = "error: " + Result.getError();
+          break;
         case CancellableResultKind::Cancelled:
-          HadError = true;
+          CompletionError = "request cancelled";
           break;
         }
       });
-  return HadError;
+  assert(CallbackCalled &&
+         "We should always receive a callback call for code completion");
+  if (CompletionError == "error: did not find code completion token") {
+    // Do not consider failure to find the code completion token as a critical
+    // failure that returns a non-zero exit code. Instead, allow the caller to
+    // match the error message.
+    llvm::outs() << CompletionError << '\n';
+  } else if (!CompletionError.empty()) {
+    llvm::errs() << CompletionError << '\n';
+    return true;
+  }
+  return false;
 }
 
 static int doTypeContextInfo(const CompilerInvocation &InitInvok,
@@ -1279,16 +1299,20 @@ static int doBatchCodeCompletion(const CompilerInvocation &InitInvok,
     PrintingDiagnosticConsumer PrintDiags;
     auto completionStart = std::chrono::high_resolution_clock::now();
     bool wasASTContextReused = false;
-    // FIXME: Should we count it as an error if we didn't receive a callback?
-    bool completionDidFail = false;
+    std::string completionError = "";
+    bool CallbackCalled = false;
     CompletionInst.performOperation(
         Invocation, /*Args=*/{}, FileSystem, completionBuffer.get(), Offset,
         CodeCompletionDiagnostics ? &PrintDiags : nullptr,
         [&](CancellableResult<CompletionInstanceResult> Result) {
+          CallbackCalled = true;
           switch (Result.getKind()) {
           case CancellableResultKind::Success: {
-            completionDidFail = false;
-
+            if (!Result->DidFindCodeCompletionToken) {
+              // Return empty results by not performing the second pass and
+              // never calling the consumer of the callback factory.
+              return;
+            }
             wasASTContextReused = Result->DidReuseAST;
 
             // Create a CodeCompletionConsumer.
@@ -1314,15 +1338,15 @@ static int doBatchCodeCompletion(const CompilerInvocation &InitInvok,
             break;
           }
           case CancellableResultKind::Failure:
-            completionDidFail = true;
-            llvm::errs() << "error: " << Result.getError() << "\n";
+            completionError = "error: " + Result.getError();
             break;
           case CancellableResultKind::Cancelled:
-            completionDidFail = true;
-            llvm::errs() << "request cancelled\n";
+            completionError = "request cancelled";
             break;
           }
         });
+    assert(CallbackCalled &&
+           "We should always receive a callback call for code completion");
     auto completionEnd = std::chrono::high_resolution_clock::now();
     auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
         completionEnd - completionStart);
@@ -1350,13 +1374,18 @@ static int doBatchCodeCompletion(const CompilerInvocation &InitInvok,
     }
 
     llvm::raw_fd_ostream fileOut(resultFD, /*shouldClose=*/true);
-    fileOut << ResultStr;
-    fileOut.close();
-
-    if (completionDidFail) {
-      // error message was already emitted above.
+    if (completionError == "error: did not find code completion token") {
+      // Do not consider failure to find the code completion token as a critical
+      // failure that returns a non-zero exit code. Instead, allow the caller to
+      // match the error message.
+      fileOut << completionError;
+    } else if (!completionError.empty()) {
+      llvm::errs() << completionError << '\n';
       return 1;
+    } else {
+      fileOut << ResultStr;
     }
+    fileOut.close();
 
     if (options::SkipFileCheck)
       continue;