Clang: Avoid waiting plugin side if (re)parse fails

If a parse or reparse operation fails, the corresponding translation
unit is "not intact" anymore and further job requests for that
translation unit will be silently discarded without notifying the
plugin. This is especially problematic when the plugin expects/waits
(for) a certain message.

A parse/reparse can fail due to e.g. an invalid commandline or an
internal crash.

In such a case, ensure to send some dummy results.

Task-number: QTCREATORBUG-18864
Change-Id: Ida9b8066fd55e64027a9b0d8bd5cf365b16a7356
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/tools/clangbackend/ipcsource/clangjobqueue.cpp

@@ -53,6 +53,19 @@ bool JobQueue::add(const JobRequest &job)
         return false;
     }
 
+    if (!m_documents.hasDocument(job.filePath, job.projectPartId)) {
+        qCDebug(jobsLog) << "Not adding / cancelling due to already closed document:" << job;
+        cancelJobRequest(job);
+        return false;
+    }
+
+    const Document document = m_documents.document(job.filePath, job.projectPartId);
+    if (!document.isIntact()) {
+        qCDebug(jobsLog) << "Not adding / cancelling due not intact document:" << job;
+        cancelJobRequest(job);
+        return false;
+    }
+
     qCDebug(jobsLog) << "Adding" << job;
     m_queue.append(job);
 
@@ -90,7 +103,7 @@ void JobQueue::removeExpiredRequests()
     m_queue = cleanedRequests;
 }
 
-bool JobQueue::isJobRequestExpired(const JobRequest &jobRequest) const
+bool JobQueue::isJobRequestExpired(const JobRequest &jobRequest)
 {
     const JobRequest::ExpirationReasons expirationReasons = jobRequest.expirationReasons;
     const UnsavedFiles unsavedFiles = m_documents.unsavedFiles();
@@ -120,7 +133,8 @@ bool JobQueue::isJobRequestExpired(const JobRequest &jobRequest) const
         const Document document
                 = m_documents.document(jobRequest.filePath, jobRequest.projectPartId);
         if (!document.isIntact()) {
-            qCDebug(jobsLog) << "Removing due to not intact translation unit:" << jobRequest;
+            qCDebug(jobsLog) << "Removing/Cancelling due to not intact document:" << jobRequest;
+            cancelJobRequest(jobRequest);
             return true;
         }
 
@@ -174,6 +188,12 @@ void JobQueue::prioritizeRequests()
     std::stable_sort(m_queue.begin(), m_queue.end(), lessThan);
 }
 
+void JobQueue::cancelJobRequest(const JobRequest &jobRequest)
+{
+    if (m_cancelJobRequest)
+        m_cancelJobRequest(jobRequest);
+}
+
 static bool passesPreconditions(const JobRequest &request, const Document &document)
 {
     using Condition = JobRequest::Condition;
@@ -280,6 +300,11 @@ void JobQueue::setIsJobRunningForJobRequestHandler(
     m_isJobRunningForJobRequestHandler = isJobRunningHandler;
 }
 
+void JobQueue::setCancelJobRequest(const JobQueue::CancelJobRequest &cancelJobRequest)
+{
+    m_cancelJobRequest = cancelJobRequest;
+}
+
 JobRequests &JobQueue::queue()
 {
     return m_queue;

src/tools/clangbackend/ipcsource/clangjobqueue.h

@@ -51,17 +51,21 @@ public:
     void setIsJobRunningForJobRequestHandler(
             const IsJobRunningForJobRequestHandler &isJobRunningHandler);
 
+    using CancelJobRequest = std::function<void(const JobRequest &)>;
+    void setCancelJobRequest(const CancelJobRequest &cancelJobRequest);
+
 public: // for tests
     JobRequests &queue();
     int size() const;
     void prioritizeRequests();
 
 private:
+    void cancelJobRequest(const JobRequest &jobRequest);
     bool isJobRunningForTranslationUnit(const Utf8String &translationUnitId);
     bool isJobRunningForJobRequest(const JobRequest &jobRequest);
     JobRequests takeJobRequestsToRunNow();
     void removeExpiredRequests();
-    bool isJobRequestExpired(const JobRequest &jobRequest) const;
+    bool isJobRequestExpired(const JobRequest &jobRequest);
 
 private:
     Documents &m_documents;
@@ -69,6 +73,7 @@ private:
 
     IsJobRunningForTranslationUnitHandler m_isJobRunningForTranslationUnitHandler;
     IsJobRunningForJobRequestHandler m_isJobRunningForJobRequestHandler;
+    CancelJobRequest m_cancelJobRequest;
 
     JobRequests m_queue;
 };

src/tools/clangbackend/ipcsource/clangjobs.cpp

@@ -29,6 +29,9 @@
 #include "clangiasyncjob.h"
 #include "projects.h"
 
+#include <clangbackendipc/cmbcodecompletedmessage.h>
+#include <clangbackendipc/referencesmessage.h>
+
 #include <QDebug>
 #include <QFutureSynchronizer>
 #include <QLoggingCategory>
@@ -54,6 +57,9 @@ Jobs::Jobs(Documents &documents,
     m_queue.setIsJobRunningForJobRequestHandler([this](const JobRequest &jobRequest) {
         return isJobRunningForJobRequest(jobRequest);
     });
+    m_queue.setCancelJobRequest([this](const JobRequest &jobRequest) {
+        return cancelJobRequest(jobRequest);
+    });
 }
 
 Jobs::~Jobs()
@@ -197,4 +203,29 @@ bool Jobs::isJobRunningForJobRequest(const JobRequest &jobRequest) const
     return Utils::anyOf(m_running.values(), hasJobRequest);
 }
 
+void Jobs::cancelJobRequest(const JobRequest &jobRequest)
+{
+    // TODO: Consider to refactor this. Jobs should not know anything about
+    // concrete messages. On the other hand, having this here avoids
+    // duplication in multiple job classes.
+
+    // If a job request with a ticket number is cancelled, the plugin side
+    // must get back some results in order to clean up the state there.
+    switch (jobRequest.type) {
+    case JobRequest::Type::RequestReferences:
+        m_client.references(ReferencesMessage(FileContainer(),
+                                              QVector<SourceRangeContainer>(),
+                                              false,
+                                              jobRequest.ticketNumber));
+        break;
+    case JobRequest::Type::CompleteCode:
+        m_client.codeCompleted(CodeCompletedMessage(CodeCompletions(),
+                                                    CompletionCorrection::NoCorrection,
+                                                    jobRequest.ticketNumber));
+        break;
+    default:
+        break;
+    }
+}
+
 } // namespace ClangBackEnd

src/tools/clangbackend/ipcsource/clangjobs.h

@@ -80,6 +80,7 @@ public /*for tests*/:
     bool isJobRunningForJobRequest(const JobRequest &jobRequest) const;
 
 private:
+    void cancelJobRequest(const JobRequest &jobRequest);
     JobRequests runJobs(const JobRequests &jobRequest);
     bool runJob(const JobRequest &jobRequest);
     void onJobFinished(IAsyncJob *asyncJob);

tests/unit/unittest/clangjobqueue-test.cpp

@@ -118,6 +118,41 @@ TEST_F(JobQueue, DoNotAddDuplicateForWhichAJobIsAlreadyRunning)
     ASSERT_FALSE(added);
 }
 
+TEST_F(JobQueue, DoNotAddForNotExistingDocument)
+{
+    jobQueue.setCancelJobRequest([](const JobRequest &) {
+       return true;
+    });
+
+    const bool added = jobQueue.add(createJobRequest(Utf8StringLiteral("notExistingDocument.cpp"),
+                                                     JobRequest::Type::UpdateDocumentAnnotations));
+
+    ASSERT_FALSE(added);
+}
+
+TEST_F(JobQueue, DoNotAddForNotIntactDocument)
+{
+    document.setHasParseOrReparseFailed(true);
+    const bool added = jobQueue.add(createJobRequest(filePath1,
+                                                     JobRequest::Type::UpdateDocumentAnnotations));
+
+    ASSERT_FALSE(added);
+}
+
+TEST_F(JobQueue, CancelDuringAddForNotIntactDocument)
+{
+    document.setHasParseOrReparseFailed(true);
+    bool canceled = false;
+    jobQueue.setCancelJobRequest([&canceled](const JobRequest &) {
+        canceled = true;
+    });
+
+
+    jobQueue.add(createJobRequest(filePath1, JobRequest::Type::UpdateDocumentAnnotations));
+
+    ASSERT_TRUE(canceled);
+}
+
 TEST_F(JobQueue, ProcessEmpty)
 {
     jobQueue.processQueue();
@@ -218,6 +253,20 @@ TEST_F(JobQueue, RemoveRequestsForNotIntactDocuments)
     ASSERT_THAT(jobsToRun.size(), Eq(0));
 }
 
+TEST_F(JobQueue, CancelRequestsForNotIntactDocuments)
+{
+    jobQueue.add(createJobRequest(filePath1, JobRequest::Type::UpdateDocumentAnnotations));
+    document.setHasParseOrReparseFailed(true);
+    bool canceled = false;
+    jobQueue.setCancelJobRequest([&canceled](const JobRequest &) {
+        canceled = true;
+    });
+
+    jobQueue.processQueue();
+
+    ASSERT_TRUE(canceled);
+}
+
 TEST_F(JobQueue, PrioritizeCurrentDocumentOverNotCurrent)
 {
     resetVisibilityAndCurrentEditor();