[clangd] Report diagnostics even if WantDiags::No AST was reused

Summary:
After r338256, clangd stopped reporting diagnostics if WantDiags::No request
is followed by a WantDiags::Yes request but the AST can be reused.

Reviewers: ioeric

Reviewed By: ioeric

Subscribers: javed.absar, MaskRay, jkorous, arphaman, jfb, cfe-commits

Differential Revision: https://reviews.llvm.org/D50045

llvm-svn: 338361

clang-tools-extra/clangd/TUScheduler.cpp

@@ -228,6 +228,9 @@ class ASTWorker {
   Semaphore &Barrier;
   /// Inputs, corresponding to the current state of AST.
   ParseInputs FileInputs;
+  /// Whether the diagnostics for the current FileInputs were reported to the
+  /// users before.
+  bool DiagsWereReported = false;
   /// Size of the last AST
   /// Guards members used by both TUScheduler and the worker thread.
   mutable std::mutex Mutex;
@@ -330,7 +333,9 @@ void ASTWorker::update(
         std::tie(Inputs.CompileCommand, Inputs.Contents);
 
     tooling::CompileCommand OldCommand = std::move(FileInputs.CompileCommand);
+    bool PrevDiagsWereReported = DiagsWereReported;
     FileInputs = Inputs;
+    DiagsWereReported = false;
 
     log("Updating file {0} with command [{1}] {2}", FileName,
         Inputs.CompileCommand.Directory,
@@ -366,34 +371,44 @@ void ASTWorker::update(
     OldPreamble.reset();
     PreambleWasBuilt.notify();
 
-    if (CanReuseAST) {
-      // Take a shortcut and don't build the AST if neither the inputs nor the
-      // preamble have changed.
-      // Note that we do not report the diagnostics, since they should not have
-      // changed either. All the clients should handle the lack of OnUpdated()
-      // call anyway to handle empty result from buildAST.
-      // FIXME(ibiryukov): the AST could actually change if non-preamble
-      // includes changed, but we choose to ignore it.
-      // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
-      // current file at this point?
-      log("Skipping rebuild of the AST for {0}, inputs are the same.",
-          FileName);
-      return;
+    if (!CanReuseAST) {
+      IdleASTs.take(this); // Remove the old AST if it's still in cache.
+    } else {
+      // Since we don't need to rebuild the AST, we might've already reported
+      // the diagnostics for it.
+      if (PrevDiagsWereReported) {
+        DiagsWereReported = true;
+        // Take a shortcut and don't report the diagnostics, since they should
+        // not changed. All the clients should handle the lack of OnUpdated()
+        // call anyway to handle empty result from buildAST.
+        // FIXME(ibiryukov): the AST could actually change if non-preamble
+        // includes changed, but we choose to ignore it.
+        // FIXME(ibiryukov): should we refresh the cache in IdleASTs for the
+        // current file at this point?
+        log("Skipping rebuild of the AST for {0}, inputs are the same.",
+            FileName);
+        return;
+      }
+    }
+
+    // Get the AST for diagnostics.
+    llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
+    if (!AST) {
+      llvm::Optional<ParsedAST> NewAST =
+          buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
+      AST = NewAST ? llvm::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr;
     }
-    // Remove the old AST if it's still in cache.
-    IdleASTs.take(this);
 
-    // Build the AST for diagnostics.
-    llvm::Optional<ParsedAST> AST =
-        buildAST(FileName, std::move(Invocation), Inputs, NewPreamble, PCHs);
     // We want to report the diagnostics even if this update was cancelled.
     // It seems more useful than making the clients wait indefinitely if they
     // spam us with updates.
-    if (WantDiags != WantDiagnostics::No && AST)
-      OnUpdated(AST->getDiagnostics());
+    // Note *AST can be still be null if buildAST fails.
+    if (WantDiags != WantDiagnostics::No && *AST) {
+      OnUpdated((*AST)->getDiagnostics());
+      DiagsWereReported = true;
+    }
     // Stash the AST in the cache for further use.
-    IdleASTs.put(this,
-                 AST ? llvm::make_unique<ParsedAST>(std::move(*AST)) : nullptr);
+    IdleASTs.put(this, std::move(*AST));
   };
 
   startTask("Update", Bind(Task, std::move(OnUpdated)), WantDiags);

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

@@ -390,5 +390,39 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
 }
 
+TEST_F(TUSchedulerTests, NoChangeDiags) {
+  TUScheduler S(
+      /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
+      /*StorePreambleInMemory=*/true, PreambleParsedCallback(),
+      /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+      ASTRetentionPolicy());
+
+  auto FooCpp = testPath("foo.cpp");
+  auto Contents = "int a; int b;";
+
+  S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::No,
+           [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
+  S.runWithAST("touchAST", FooCpp, [](llvm::Expected<InputsAndAST> IA) {
+    // Make sure the AST was actually built.
+    cantFail(std::move(IA));
+  });
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+
+  // Even though the inputs didn't change and AST can be reused, we need to
+  // report the diagnostics, as they were not reported previously.
+  std::atomic<bool> SeenDiags(false);
+  S.update(FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
+           [&](std::vector<Diag>) { SeenDiags = true; });
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+  ASSERT_TRUE(SeenDiags);
+
+  // Subsequent request does not get any diagnostics callback because the same
+  // diags have previously been reported and the inputs didn't change.
+  S.update(
+      FooCpp, getInputs(FooCpp, Contents), WantDiagnostics::Auto,
+      [&](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
+  ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(1)));
+}
+
 } // namespace clangd
 } // namespace clang