ClangCodeModel: Force clangd restart on external changes

Neither we nor clangd can afford to watch all source files, which means
that after e.g. a branch switch we can easily end up in an inconsistent
state.
We alleviate this problem by restarting clangd if at least one open file
was changed externally.

Change-Id: I7e0d14835e3afbd7a64c3233614f2161282dddc0
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/clangcodemodel/clangcodemodelplugin.cpp

@@ -211,6 +211,7 @@ QVector<QObject *> ClangCodeModelPlugin::createTestObjects() const
     return {
         new Tests::ClangCodeCompletionTest,
         new Tests::ClangdTestCompletion,
+        new Tests::ClangdTestExternalChanges,
         new Tests::ClangdTestFindReferences,
         new Tests::ClangdTestFollowSymbol,
         new Tests::ClangdTestHighlighting,

src/plugins/clangcodemodel/clangdclient.cpp

@@ -845,7 +845,11 @@ ClangdClient::ClangdClient(Project *project, const Utils::FilePath &jsonDbDir)
     setDocumentChangeUpdateThreshold(d->settings.documentUpdateThreshold);
 
     const auto textMarkCreator = [this](const Utils::FilePath &filePath,
-            const Diagnostic &diag) { return new ClangdTextMark(filePath, diag, this); };
+            const Diagnostic &diag) {
+        if (d->isTesting)
+            emit textMarkCreated(filePath);
+        return new ClangdTextMark(filePath, diag, this);
+    };
     const auto hideDiagsHandler = []{ ClangDiagnosticManager::clearTaskHubIssues(); };
     setDiagnosticsHandlers(textMarkCreator, hideDiagsHandler);
 

src/plugins/clangcodemodel/clangdclient.h

@@ -88,6 +88,7 @@ signals:
     void highlightingResultsReady(const TextEditor::HighlightingResults &results,
                                   const Utils::FilePath &file);
     void proposalReady(TextEditor::IAssistProposal *proposal);
+    void textMarkCreated(const Utils::FilePath &file);
 
 private:
     void handleDiagnostics(const LanguageServerProtocol::PublishDiagnosticsParams &params) override;

src/plugins/clangcodemodel/clangmodelmanagersupport.cpp

@@ -38,6 +38,7 @@
 #include "clangrefactoringengine.h"
 #include "clangutils.h"
 
+#include <coreplugin/documentmanager.h>
 #include <coreplugin/editormanager/documentmodel.h>
 #include <coreplugin/editormanager/editormanager.h>
 #include <coreplugin/icore.h>
@@ -46,6 +47,7 @@
 #include <cppeditor/cppcodemodelsettings.h>
 #include <cppeditor/cppfollowsymbolundercursor.h>
 #include <cppeditor/cppmodelmanager.h>
+#include <cppeditor/cppprojectfile.h>
 #include <cppeditor/cpptoolsreuse.h>
 #include <cppeditor/editordocumenthandle.h>
 
@@ -54,6 +56,7 @@
 #include <texteditor/quickfix.h>
 
 #include <projectexplorer/buildconfiguration.h>
+#include <projectexplorer/buildsystem.h>
 #include <projectexplorer/project.h>
 #include <projectexplorer/projectnodes.h>
 #include <projectexplorer/session.h>
@@ -108,6 +111,7 @@ ClangModelManagerSupport::ClangModelManagerSupport()
     QTC_CHECK(!m_instance);
     m_instance = this;
 
+    watchForExternalChanges();
     CppEditor::CppModelManager::instance()->setCurrentDocumentFilter(
                 std::make_unique<ClangCurrentDocumentFilter>());
     cppModelManager()->setLocatorFilter(std::make_unique<ClangGlobalSymbolFilter>());
@@ -410,6 +414,69 @@ void ClangModelManagerSupport::claimNonProjectSources(ClangdClient *fallbackClie
     }
 }
 
+// If any open C/C++ source file is changed from outside Qt Creator, we restart the client
+// for the respective project to force re-parsing of open documents and re-indexing.
+// While this is not 100% bullet-proof, chances are good that in a typical session-based
+// workflow, e.g. a git branch switch will hit at least one open file.
+void ClangModelManagerSupport::watchForExternalChanges()
+{
+    const auto projectIsParsing = [](const ProjectExplorer::Project *project) {
+        const ProjectExplorer::BuildSystem * const bs = project && project->activeTarget()
+                ? project->activeTarget()->buildSystem() : nullptr;
+        return bs && (bs->isParsing() || bs->isWaitingForParse());
+    };
+
+    const auto timer = new QTimer(this);
+    timer->setInterval(3000);
+    connect(timer, &QTimer::timeout, this, [this, projectIsParsing] {
+        const auto clients = m_clientsToRestart;
+        m_clientsToRestart.clear();
+        for (ClangdClient * const client : clients) {
+            if (client && client->state() != Client::Shutdown
+                    && client->state() != Client::ShutdownRequested
+                    && !projectIsParsing(client->project())) {
+
+                // FIXME: Lots of const-incorrectness along the call chain of updateLanguageClient().
+                const auto project = const_cast<ProjectExplorer::Project *>(client->project());
+
+                updateLanguageClient(project, CppModelManager::instance()->projectInfo(project));
+            }
+        }
+    });
+
+    connect(Core::DocumentManager::instance(), &Core::DocumentManager::filesChangedExternally,
+            this, [this, timer, projectIsParsing](const QSet<Utils::FilePath> &files) {
+        if (!LanguageClientManager::hasClients<ClangdClient>())
+            return;
+        for (const Utils::FilePath &file : files) {
+            const ProjectFile::Kind kind = ProjectFile::classify(file.toString());
+            if (!ProjectFile::isSource(kind) && !ProjectFile::isHeader(kind))
+                continue;
+            const ProjectExplorer::Project * const project
+                    = ProjectExplorer::SessionManager::projectForFile(file);
+            if (!project)
+                continue;
+
+            // If a project file was changed, it is very likely that we will have to generate
+            // a new compilation database, in which case the client will be restarted via
+            // a different code path.
+            if (projectIsParsing(project))
+                return;
+
+            ClangdClient * const client = clientForProject(project);
+            if (client) {
+                m_clientsToRestart.append(client);
+                timer->start();
+            }
+
+            // It's unlikely that the same signal carries files from different projects,
+            // so we exit the loop as soon as we have dealt with one project, as the
+            // project look-up is not free.
+            return;
+        }
+    });
+}
+
 void ClangModelManagerSupport::onEditorOpened(Core::IEditor *editor)
 {
     QTC_ASSERT(editor, return);

src/plugins/clangcodemodel/clangmodelmanagersupport.h

@@ -35,6 +35,7 @@
 #include <utils/id.h>
 
 #include <QObject>
+#include <QPointer>
 
 #include <memory>
 
@@ -133,6 +134,7 @@ private:
                               const CppEditor::ProjectInfo::ConstPtr &projectInfo);
     ClangdClient *createClient(ProjectExplorer::Project *project, const Utils::FilePath &jsonDbDir);
     void claimNonProjectSources(ClangdClient *fallbackClient);
+    void watchForExternalChanges();
 
 private:
     UiHeaderOnDiskManager m_uiHeaderOnDiskManager;
@@ -144,6 +146,7 @@ private:
 
     QHash<ProjectExplorer::Project *, ClangProjectSettings *> m_projectSettings;
     Utils::FutureSynchronizer m_generatorSynchronizer;
+    QList<QPointer<ClangdClient>> m_clientsToRestart;
 };
 
 class ClangModelManagerSupportProvider : public CppEditor::ModelManagerSupportProvider

src/plugins/clangcodemodel/test/clangdtests.cpp

@@ -51,6 +51,7 @@
 
 #include <QElapsedTimer>
 #include <QEventLoop>
+#include <QFile>
 #include <QFileInfo>
 #include <QPair>
 #include <QScopedPointer>
@@ -1922,6 +1923,50 @@ AssistProposalItemInterface *ClangdTestCompletion::getItem(
     return nullptr;
 }
 
+
+ClangdTestExternalChanges::ClangdTestExternalChanges()
+{
+    setProjectFileName("completion.pro");
+    setSourceFileNames({"mainwindow.cpp", "main.cpp"});
+}
+
+void ClangdTestExternalChanges::test()
+{
+    // Break a header file that is used, but not open in Creator.
+    // Neither we nor the server should notice, and no diagnostics should be shown for the
+    // source file that includes the now-broken header.
+    QFile header(project()->projectDirectory().toString() + "/mainwindow.h");
+    QVERIFY(header.open(QIODevice::WriteOnly));
+    header.write("blubb");
+    header.close();
+    ClangdClient * const oldClient = client();
+    QVERIFY(oldClient);
+    QVERIFY(!waitForSignalOrTimeout(ClangModelManagerSupport::instance(),
+                                    &ClangModelManagerSupport::createdClient, timeOutInMs()));
+    QCOMPARE(client(), oldClient);
+    const TextDocument * const curDoc = document("main.cpp");
+    QVERIFY(curDoc);
+    QVERIFY(curDoc->marks().isEmpty());
+
+    // Now trigger an external change in an open, but not currently visible file and
+    // verify that we get a new client and diagnostics in the current editor.
+    TextDocument * const docToChange = document("mainwindow.cpp");
+    docToChange->setSilentReload();
+    QFile otherSource(filePath("mainwindow.cpp").toString());
+    QVERIFY(otherSource.open(QIODevice::WriteOnly));
+    otherSource.write("blubb");
+    otherSource.close();
+    QVERIFY(waitForSignalOrTimeout(ClangModelManagerSupport::instance(),
+                                   &ClangModelManagerSupport::createdClient, timeOutInMs()));
+    ClangdClient * const newClient = ClangModelManagerSupport::instance()
+            ->clientForFile(filePath("main.cpp"));
+    QVERIFY(newClient);
+    QVERIFY(newClient != oldClient);
+    newClient->enableTesting();
+    if (curDoc->marks().isEmpty())
+        QVERIFY(waitForSignalOrTimeout(newClient, &ClangdClient::textMarkCreated, timeOutInMs()));
+}
+
 } // namespace Tests
 } // namespace Internal
 } // namespace ClangCodeModel

src/plugins/clangcodemodel/test/clangdtests.h

@@ -194,6 +194,17 @@ private:
     QSet<Utils::FilePath> m_documentsWithHighlighting;
 };
 
+class ClangdTestExternalChanges : public ClangdTest
+{
+    Q_OBJECT
+
+public:
+    ClangdTestExternalChanges();
+
+private slots:
+    void test();
+};
+
 } // namespace Tests
 } // namespace Internal
 } // namespace ClangCodeModel

src/plugins/coreplugin/documentmanager.cpp

@@ -1114,6 +1114,7 @@ void DocumentManager::checkForReload()
 
     // clean up. do this before we may enter the main loop, otherwise we would
     // lose consecutive notifications.
+    emit filesChangedExternally(d->m_changedFiles);
     d->m_changedFiles.clear();
 
     // collect information about "expected" file names

src/plugins/coreplugin/documentmanager.h

@@ -153,6 +153,8 @@ signals:
                          const Utils::FilePath &to);
     void projectsDirectoryChanged(const Utils::FilePath &directory);
 
+    void filesChangedExternally(const QSet<Utils::FilePath> &filePaths);
+
 private:
     explicit DocumentManager(QObject *parent);
     ~DocumentManager() override;

src/plugins/languageclient/languageclientmanager.h

@@ -31,6 +31,7 @@
 #include "locatorfilter.h"
 #include "lspinspector.h"
 
+#include <utils/algorithm.h>
 #include <utils/id.h>
 
 #include <languageserverprotocol/diagnostics.h>
@@ -86,6 +87,7 @@ public:
     static Client *clientForFilePath(const Utils::FilePath &filePath);
     static Client *clientForUri(const LanguageServerProtocol::DocumentUri &uri);
     static const QList<Client *> clientsForProject(const ProjectExplorer::Project *project);
+    template<typename T> static bool hasClients();
 
     ///
     /// \brief openDocumentWithClient
@@ -130,4 +132,12 @@ private:
     WorkspaceMethodLocatorFilter m_workspaceMethodLocatorFilter;
     LspInspector m_inspector;
 };
+
+template<typename T> bool LanguageClientManager::hasClients()
+{
+    return Utils::contains(instance()->m_clients, [](const Client *c) {
+        return qobject_cast<const T* >(c);
+    });
+}
+
 } // namespace LanguageClient

src/plugins/texteditor/textdocument.cpp

@@ -106,6 +106,7 @@ public:
     QScopedPointer<Formatter> m_formatter;
 
     int m_autoSaveRevision = -1;
+    bool m_silentReload = false;
 
     TextMarks m_marksCache; // Marks not owned
     Utils::Guard m_modificationChangedGuard;
@@ -426,6 +427,13 @@ QAction *TextDocument::createDiffAgainstCurrentFileAction(
     return diffAction;
 }
 
+#ifdef WITH_TESTS
+void TextDocument::setSilentReload()
+{
+    d->m_silentReload = true;
+}
+#endif
+
 void TextDocument::triggerPendingUpdates()
 {
     if (d->m_fontSettingsNeedsApply)
@@ -707,6 +715,13 @@ void TextDocument::setFilePath(const Utils::FilePath &newName)
     IDocument::setFilePath(newName.absoluteFilePath());
 }
 
+IDocument::ReloadBehavior TextDocument::reloadBehavior(ChangeTrigger state, ChangeType type) const
+{
+    if (d->m_silentReload)
+        return IDocument::BehaviorSilent;
+    return BaseTextDocument::reloadBehavior(state, type);
+}
+
 bool TextDocument::isModified() const
 {
     return d->m_document.isModified();

src/plugins/texteditor/textdocument.h

@@ -120,6 +120,7 @@ public:
     bool isSaveAsAllowed() const override;
     bool reload(QString *errorString, ReloadFlag flag, ChangeType type) override;
     void setFilePath(const Utils::FilePath &newName) override;
+    ReloadBehavior reloadBehavior(ChangeTrigger state, ChangeType type) const override;
 
     QString fallbackSaveAsPath() const override;
     QString fallbackSaveAsFileName() const override;
@@ -155,6 +156,10 @@ public:
     static QAction *createDiffAgainstCurrentFileAction(QObject *parent,
         const std::function<Utils::FilePath()> &filePath);
 
+#ifdef WITH_TESTS
+    void setSilentReload();
+#endif
+
 signals:
     void aboutToOpen(const Utils::FilePath &filePath, const Utils::FilePath &realFilePath);
     void openFinishedSuccessfully();