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

[ilya-biryukov]

Summary

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

[ioeric]

Overall LG, just a suggestion to make the code a bit easier to follow.

clangd/TUScheduler.cpp

379 ↗

(On Diff #158197)

The implicit condition here is that we can reuse AST and diagnostics were not reported. I think we can be more explicit here by moving this early return into the CanReuseAST if branch. By doing this, we could also get rid of PrevDiagsWereReported. An additional state variable seems to have made the flow less clear.

405 ↗

(On Diff #158197)

This was preexisting, but it might worth adding a comment explaining when *AST would be false.

[ilya-biryukov]

• Added a comment

[ilya-biryukov]

clangd/TUScheduler.cpp

379 ↗

(On Diff #158197)

The problem with that would be that the value of DiagsWereReported would be stale for ~40 lines between the assignment to FileInputs and this point.
So I'm not sure about this trade-off. WDYT?

[ilya-biryukov]

• Moved early return into the CanReuseAST branch

[ilya-biryukov]

clangd/TUScheduler.cpp

379 ↗

(On Diff #158197)

Update after chatting offline: we agreed to keep the PrevDiagsWereReported and move the early return into the original if branch to make the dependencies between DiagsWereReported and CanReuseAST more explicit. This comment should be done now.

[ioeric]

clangd/TUScheduler.cpp

381 ↗

(On Diff #158216)

I think checking PrevDiagsWereReported here would be more straightforward.

[ilya-biryukov]

clangd/TUScheduler.cpp

381 ↗

(On Diff #158216)

Good point. Done.

[ilya-biryukov]

• Check for PrevDiagsWereReported

clang-tools-extra/trunk/clangd/TUScheduler.cpp

private:
   /// Callback, passed to the preamble builder.
   const PreambleParsedCallback PreambleCallback;
   /// Helper class required to build the ASTs.
   const std::shared_ptr<PCHContainerOperations> PCHs;
 
   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;
   std::shared_ptr<const PreambleData> LastBuiltPreamble; /* GUARDED_BY(Mutex) */
   /// Becomes ready when the first preamble build finishes.
   Notification PreambleWasBuilt;
   /// Set to true to signal run() to finish processing.
   bool Done;                    /* GUARDED_BY(Mutex) */
void ASTWorker::update(
     llvm::unique_function<void(std::vector<Diag>)> OnUpdated) {
   auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
     // Will be used to check if we can avoid rebuilding the AST.
     bool InputsAreTheSame =
         std::tie(FileInputs.CompileCommand, FileInputs.Contents) ==
         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,
         llvm::join(Inputs.CompileCommand.CommandLine, " "));
     // Rebuild the preamble and the AST.
     std::unique_ptr<CompilerInvocation> Invocation =
         buildCompilerInvocation(Inputs);
     if (!Invocation) {
bool CanReuseAST = InputsAreTheSame && (OldPreamble == NewPreamble);
         LastBuiltPreamble = NewPreamble;
     }
     // Before doing the expensive AST reparse, we want to release our reference
     // to the old preamble, so it can be freed if there are no other references
     // to it.
     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()
+    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;
       }
-    // Remove the old AST if it's still in cache.
-    IdleASTs.take(this);
+    }
 
-    // Build the AST for diagnostics.
-    llvm::Optional<ParsedAST> AST =
+    // 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;
+    }
+
     // 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);
 }
 
 void ASTWorker::runWithAST(
     llvm::StringRef Name,
     llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {

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

TEST_F(TUSchedulerTests, NoopOnEmptyChanges) {
   ASSERT_FALSE(DoUpdate(getInputs(Source, OtherSourceContents)));
 
   // Update to the compile commands should also cause a rebuild.
   CDB.ExtraClangFlags.push_back("-DSOMETHING");
   ASSERT_TRUE(DoUpdate(getInputs(Source, OtherSourceContents)));
   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