Merge remote-tracking branch 'origin/master' into master-next

swift-ci · swift-ci · commit 6ea84f6406ad · 2018-06-19T10:49:58.000-07:00
diff --git a/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp b/tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp
@@ -263,7 +263,14 @@ class ASTProducer : public ThreadSafeRefCountedBase<ASTProducer> {
   SmallVector<BufferStamp, 8> Stamps;
   ThreadSafeRefCntPtr<ASTUnit> AST;
   SmallVector<std::pair<std::string, BufferStamp>, 8> DependencyStamps;
-  std::vector<std::pair<SwiftASTConsumerRef, const void*>> QueuedConsumers;
+
+  struct QueuedConsumer {
+    SwiftASTConsumerRef consumer;
+    std::vector<ImmutableTextSnapshotRef> snapshots;
+    const void *oncePerASTToken;
+  };
+
+  std::vector<QueuedConsumer> QueuedConsumers;
   llvm::sys::Mutex Mtx;
 
 public:
@@ -282,8 +289,13 @@ class ASTProducer : public ThreadSafeRefCountedBase<ASTProducer> {
   bool shouldRebuild(SwiftASTManager::Implementation &MgrImpl,
                      ArrayRef<ImmutableTextSnapshotRef> Snapshots);
 
-  void enqueueConsumer(SwiftASTConsumerRef Consumer, const void *OncePerASTToken);
-  std::vector<SwiftASTConsumerRef> popQueuedConsumers();
+  void enqueueConsumer(SwiftASTConsumerRef Consumer,
+                       ArrayRef<ImmutableTextSnapshotRef> Snapshots,
+                       const void *OncePerASTToken);
+
+  using ConsumerPredicate = llvm::function_ref<bool(
+      SwiftASTConsumer *, ArrayRef<ImmutableTextSnapshotRef>)>;
+  std::vector<SwiftASTConsumerRef> takeConsumers(ConsumerPredicate predicate);
 
   size_t getMemoryCost() const {
     // FIXME: Report the memory cost of the overall CompilerInstance.
@@ -557,19 +569,27 @@ void SwiftASTManager::processASTAsync(SwiftInvocationRef InvokRef,
     }
   }
 
-  Producer->enqueueConsumer(std::move(ASTConsumer), OncePerASTToken);
+  Producer->enqueueConsumer(ASTConsumer, Snapshots, OncePerASTToken);
 
-  Producer->getASTUnitAsync(Impl, Snapshots,
-    [Producer](ASTUnitRef Unit, StringRef Error) {
-      auto Consumers = Producer->popQueuedConsumers();
+  auto handleAST = [this, Producer, ASTConsumer](ASTUnitRef unit,
+                                                 StringRef error) {
+    auto consumers = Producer->takeConsumers(
+        [&](SwiftASTConsumer *consumer,
+            ArrayRef<ImmutableTextSnapshotRef> snapshots) {
+          return consumer == ASTConsumer.get() ||
+                 !Producer->shouldRebuild(Impl, snapshots) ||
+                 (unit && consumer->canUseASTWithSnapshots(snapshots));
+        });
 
-      for (auto &Consumer : Consumers) {
-        if (Unit)
-          Unit->Impl.consumeAsync(std::move(Consumer), Unit);
-        else
-          Consumer->failed(Error);
-      }
-    });
+    for (auto &consumer : consumers) {
+      if (unit)
+        unit->Impl.consumeAsync(std::move(consumer), unit);
+      else
+        consumer->failed(error);
+    }
+  };
+
+  Producer->getASTUnitAsync(Impl, Snapshots, std::move(handleAST));
 }
 
 void SwiftASTManager::removeCachedAST(SwiftInvocationRef Invok) {
@@ -689,30 +709,37 @@ ASTUnitRef ASTProducer::getASTUnitImpl(SwiftASTManager::Implementation &MgrImpl,
   return AST;
 }
 
-void ASTProducer::enqueueConsumer(SwiftASTConsumerRef Consumer,
-                                  const void *OncePerASTToken) {
+void ASTProducer::enqueueConsumer(SwiftASTConsumerRef consumer,
+                                  ArrayRef<ImmutableTextSnapshotRef> snapshots,
+                                  const void *oncePerASTToken) {
   llvm::sys::ScopedLock L(Mtx);
-  if (OncePerASTToken) {
+  if (oncePerASTToken) {
     for (auto I = QueuedConsumers.begin(),
               E = QueuedConsumers.end(); I != E; ++I) {
-      if (I->second == OncePerASTToken) {
-        I->first->cancelled();
+      if (I->oncePerASTToken == oncePerASTToken) {
+        I->consumer->cancelled();
         QueuedConsumers.erase(I);
         break;
       }
     }
   }
-  QueuedConsumers.push_back({ std::move(Consumer), OncePerASTToken });
+  QueuedConsumers.push_back({std::move(consumer), snapshots, oncePerASTToken});
 }
 
-std::vector<SwiftASTConsumerRef> ASTProducer::popQueuedConsumers() {
+std::vector<SwiftASTConsumerRef>
+ASTProducer::takeConsumers(ConsumerPredicate predicate) {
   llvm::sys::ScopedLock L(Mtx);
-  std::vector<SwiftASTConsumerRef> Consumers;
-  Consumers.reserve(QueuedConsumers.size());
-  for (auto &C : QueuedConsumers)
-    Consumers.push_back(std::move(C.first));
-  QueuedConsumers.clear();
-  return Consumers;
+  std::vector<SwiftASTConsumerRef> consumers;
+
+  QueuedConsumers.erase(std::remove_if(QueuedConsumers.begin(),
+      QueuedConsumers.end(), [&](QueuedConsumer &qc) {
+    if (predicate(qc.consumer.get(), qc.snapshots)) {
+      consumers.push_back(std::move(qc.consumer));
+      return true;
+    }
+    return false;
+  }), QueuedConsumers.end());
+  return consumers;
 }
 
 bool ASTProducer::shouldRebuild(SwiftASTManager::Implementation &MgrImpl,
diff --git a/unittests/SourceKit/SwiftLang/CursorInfoTest.cpp b/unittests/SourceKit/SwiftLang/CursorInfoTest.cpp
@@ -30,7 +30,7 @@ static StringRef getRuntimeLibPath() {
 namespace {
 
 class NullEditorConsumer : public EditorConsumer {
-  bool needsSemanticInfo() override { return false; }
+  bool needsSemanticInfo() override { return needsSema; }
 
   void handleRequestError(const char *Description) override {
     llvm_unreachable("unexpected error");
@@ -90,6 +90,9 @@ class NullEditorConsumer : public EditorConsumer {
   bool handleSourceText(StringRef Text) override { return false; }
   bool handleSerializedSyntaxTree(StringRef Text) override { return false; }
   bool syntaxTreeEnabled() override { return false; }
+
+public:
+  bool needsSema = false;
 };
 
 struct TestCursorInfo {
@@ -100,8 +103,9 @@ struct TestCursorInfo {
 };
 
 class CursorInfoTest : public ::testing::Test {
-  SourceKit::Context Ctx{ getRuntimeLibPath(), SourceKit::createSwiftLangSupport };
+  SourceKit::Context &Ctx;
   std::atomic<int> NumTasks;
+  NullEditorConsumer Consumer;
 
 public:
   LangSupport &getLang() { return Ctx.getSwiftLangSupport(); }
@@ -114,20 +118,30 @@ class CursorInfoTest : public ::testing::Test {
     NumTasks = 0;
   }
 
+  CursorInfoTest()
+      : Ctx(*new SourceKit::Context(getRuntimeLibPath(),
+                                    SourceKit::createSwiftLangSupport,
+                                    /*dispatchOnMain=*/false)) {
+    // This is avoiding destroying \p SourceKit::Context because another
+    // thread may be active trying to use it to post notifications.
+    // FIXME: Use shared_ptr ownership to avoid such issues.
+  }
+
   void addNotificationReceiver(DocumentUpdateNotificationReceiver Receiver) {
     Ctx.getNotificationCenter().addDocumentUpdateNotificationReceiver(Receiver);
   }
 
-  void open(StringRef DocName, StringRef Text) {
-    NullEditorConsumer Consumer;
+  void open(const char *DocName, StringRef Text,
+            Optional<ArrayRef<const char *>> CArgs = llvm::None) {
+    auto Args = CArgs.hasValue() ? makeArgs(DocName, *CArgs)
+                                 : std::vector<const char *>{};
     auto Buf = MemoryBuffer::getMemBufferCopy(Text, DocName);
-    getLang().editorOpen(DocName, Buf.get(), /*EnableSyntaxMap=*/false, Consumer,
-                         /*Args=*/{});
+    getLang().editorOpen(DocName, Buf.get(), /*EnableSyntaxMap=*/false,
+                         Consumer, Args);
   }
 
   void replaceText(StringRef DocName, unsigned Offset, unsigned Length,
                    StringRef Text) {
-    NullEditorConsumer Consumer;
     auto Buf = MemoryBuffer::getMemBufferCopy(Text, DocName);
     getLang().editorReplaceText(DocName, Buf.get(), Offset, Length, Consumer);
   }
@@ -159,6 +173,8 @@ class CursorInfoTest : public ::testing::Test {
     return pos;
   }
 
+  void setNeedsSema(bool needsSema) { Consumer.needsSema = needsSema; }
+
 private:
   std::vector<const char *> makeArgs(const char *DocName,
                                      ArrayRef<const char *> CArgs) {
@@ -348,3 +364,32 @@ TEST_F(CursorInfoTest, CursorInfoMustWaitDueToken) {
   EXPECT_EQ(FooOffs, Info.DeclarationLoc->first);
   EXPECT_EQ(strlen("fog"), Info.DeclarationLoc->second);
 }
+
+TEST_F(CursorInfoTest, CursorInfoMustWaitDueTokenRace) {
+  const char *DocName = "/test.swift";
+  const char *Contents = "let value = foo\n"
+                         "let foo = 0\n";
+  const char *Args[] = {"-parse-as-library"};
+
+  auto FooRefOffs = findOffset("foo", Contents);
+  auto FooOffs = findOffset("foo =", Contents);
+
+  // Open with args, kicking off an ast build. The hope of this tests is for
+  // this AST to still be in the process of building when we start the cursor
+  // info, to ensure the ASTManager doesn't try to handle this cursor info with
+  // the wrong AST.
+  setNeedsSema(true);
+  open(DocName, Contents, llvm::makeArrayRef(Args));
+  // Change 'foo' to 'fog' by replacing the last character.
+  replaceText(DocName, FooRefOffs + 2, 1, "g");
+  replaceText(DocName, FooOffs + 2, 1, "g");
+
+  // Should wait for the new AST, because the cursor location points to a
+  // different token.
+  auto Info = getCursor(DocName, FooRefOffs, Args);
+  EXPECT_STREQ("fog", Info.Name.c_str());
+  EXPECT_STREQ("Int", Info.Typename.c_str());
+  ASSERT_TRUE(Info.DeclarationLoc.hasValue());
+  EXPECT_EQ(FooOffs, Info.DeclarationLoc->first);
+  EXPECT_EQ(strlen("fog"), Info.DeclarationLoc->second);
+}
diff --git a/unittests/SourceKit/SwiftLang/EditingTest.cpp b/unittests/SourceKit/SwiftLang/EditingTest.cpp
@@ -142,12 +142,15 @@ class EditTest : public ::testing::Test {
     Ctx->getNotificationCenter().addDocumentUpdateNotificationReceiver(Receiver);
   }
 
-  bool waitForDocUpdate() {
+  bool waitForDocUpdate(bool reset = false) {
     std::chrono::seconds secondsToWait(10);
     std::unique_lock<std::mutex> lk(DocUpdState->Mtx);
     auto when = std::chrono::system_clock::now() + secondsToWait;
-    return !DocUpdState->CV.wait_until(
+    auto result = !DocUpdState->CV.wait_until(
         lk, when, [&]() { return DocUpdState->HasUpdate; });
+    if (reset)
+      DocUpdState->HasUpdate = false;
+    return result;
   }
 
   void open(const char *DocName, StringRef Text, ArrayRef<const char *> CArgs,
@@ -158,6 +161,10 @@ class EditTest : public ::testing::Test {
                          Args);
   }
 
+  void close(const char *DocName) {
+    getLang().editorClose(DocName, /*removeCache=*/false);
+  }
+
   void replaceText(StringRef DocName, unsigned Offset, unsigned Length,
                    StringRef Text, EditorConsumer &Consumer) {
     auto Buf = MemoryBuffer::getMemBufferCopy(Text, DocName);
@@ -177,6 +184,8 @@ class EditTest : public ::testing::Test {
     DocUpdState->HasUpdate = false;
   }
 
+  void doubleOpenWithDelay(useconds_t delay, bool close);
+
 private:
   std::vector<const char *> makeArgs(const char *DocName,
                                      ArrayRef<const char *> CArgs) {
@@ -226,3 +235,65 @@ TEST_F(EditTest, DiagsAfterEdit) {
   }
   EXPECT_EQ(SemaDiagStage, Consumer.DiagStage);
 }
+
+void EditTest::doubleOpenWithDelay(useconds_t delay, bool closeDoc) {
+  const char *DocName = "/test.swift";
+  const char *Contents =
+    "func foo() { _ = unknown_name }\n";
+  const char *Args[] = { "-parse-as-library" };
+
+  DiagConsumer Consumer;
+  open(DocName, Contents, Args, Consumer);
+  ASSERT_EQ(0u, Consumer.Diags.size());
+  // Open again without closing; this reinitializes the semantic info on the doc
+  if (delay)
+    usleep(delay);
+  if (closeDoc)
+    close(DocName);
+  reset(Consumer);
+  open(DocName, Contents, Args, Consumer);
+  ASSERT_EQ(0u, Consumer.Diags.size());
+
+  // Wait for the document update from the second time we open the document. We
+  // may or may not get a notification from the first time it was opened, but
+  // only the second time will there be any semantic information available to
+  // be queried, since the semantic info from the first open is unreachable.
+  for (int i = 0; i < 2; ++i) {
+    bool expired = waitForDocUpdate(/*reset=*/true);
+    ASSERT_FALSE(expired) << "no second notification";
+    replaceText(DocName, 0, 0, StringRef(), Consumer);
+    if (!Consumer.Diags.empty())
+      break;
+    ASSERT_EQ(0, i) << "no diagnostics after second notification";
+  }
+
+  ASSERT_EQ(1u, Consumer.Diags.size());
+  EXPECT_STREQ("use of unresolved identifier 'unknown_name'", Consumer.Diags[0].Description.c_str());
+}
+
+TEST_F(EditTest, DiagsAfterCloseAndReopen) {
+  // Attempt to open the same file twice in a row. This tests (subject to
+  // timing) cases where:
+  // * the 2nd open happens before the first AST starts building
+  // * the 2nd open happens after the first AST starts building
+  // * the 2nd open happens after the AST finishes
+
+  // The middle case in particular verifies the ASTManager is only calling the
+  // correct ASTConsumers.
+
+  doubleOpenWithDelay(0, true);
+  doubleOpenWithDelay(1000, true);   // 1 ms
+  doubleOpenWithDelay(10000, true);  // 10 ms
+  doubleOpenWithDelay(100000, true); // 100 ms
+}
+
+TEST_F(EditTest, DiagsAfterReopen) {
+  // See description of DiagsAfterCloseAndReopen, but in this case we don't
+  // close the original document, causing it to reinitialize instead of create
+  // a fresh document.
+
+  doubleOpenWithDelay(0, false);
+  doubleOpenWithDelay(1000, false);   // 1 ms
+  doubleOpenWithDelay(10000, false);  // 10 ms
+  doubleOpenWithDelay(100000, false); // 100 ms
+}
