ClangTools: Improve fix-its handling

Two areas are touched:
1. Select multiple diagnostics if they have the same set
   of fix-its.
2. Watch the files and invalidate diagnostics if corresponding
   file was edited.

Change-Id: If4487ba91f45c25d1aed1a98990dd9b6df9d7fe2
Reviewed-by: Marco Bubke <marco.bubke@qt.io>

src/plugins/clangtools/clangtidyclazytool.cpp

@@ -102,6 +102,9 @@ public:
     static void addFixitOperations(DiagnosticItem *diagnosticItem,
                                    const FixitsRefactoringFile &file, bool apply)
     {
+        if (!diagnosticItem->hasNewFixIts())
+            return;
+
         // Did we already created the fixit operations?
         ReplacementOperations currentOps = diagnosticItem->fixitOperations();
         if (!currentOps.isEmpty()) {
@@ -135,7 +138,7 @@ public:
         diagnosticItem->setFixitOperations(replacements);
     }
 
-    void apply()
+    void apply(ClangToolsDiagnosticModel *model)
     {
         for (auto it = m_refactoringFileInfos.begin(); it != m_refactoringFileInfos.end(); ++it) {
             RefactoringFileInfo &fileInfo = it.value();
@@ -167,18 +170,24 @@ public:
             for (DiagnosticItem *item : itemsScheduledOrSchedulable)
                 ops += item->fixitOperations();
 
+            if (ops.empty())
+                continue;
+
             // Apply file
             QVector<DiagnosticItem *> itemsApplied;
             QVector<DiagnosticItem *> itemsFailedToApply;
             QVector<DiagnosticItem *> itemsInvalidated;
 
             fileInfo.file.setReplacements(ops);
+
+            model->removeWatchedPath(ops.first()->fileName);
             if (fileInfo.file.apply()) {
                 itemsApplied = itemsScheduled;
             } else {
                 itemsFailedToApply = itemsScheduled;
                 itemsInvalidated = itemsSchedulable;
             }
+            model->addWatchedPath(ops.first()->fileName);
 
             // Update DiagnosticItem state
             for (DiagnosticItem *diagnosticItem : itemsScheduled)
@@ -262,7 +271,7 @@ ClangTidyClazyTool::ClangTidyClazyTool()
             diagnosticItems += static_cast<DiagnosticItem *>(item);
         });
 
-        ApplyFixIts(diagnosticItems).apply();
+        ApplyFixIts(diagnosticItems).apply(m_diagnosticModel);
     });
 
     ActionContainer *menu = ActionManager::actionContainer(Debugger::Constants::M_DEBUG_ANALYZER);
@@ -354,7 +363,9 @@ void ClangTidyClazyTool::startTool(bool askUserForFileSelection)
 
     m_perspective.select();
 
+    m_diagnosticModel->clearAndSetupCache();
     m_diagnosticModel->clear();
+
     setToolBusy(true);
     m_diagnosticFilterModel->setProject(project);
     m_running = true;

src/plugins/clangtools/clangtoolsdiagnostic.h

@@ -27,9 +27,9 @@
 
 #include <debugger/analyzer/diagnosticlocation.h>
 
-#include <QList>
 #include <QMetaType>
 #include <QString>
+#include <QVector>
 
 namespace ClangTools {
 namespace Internal {
@@ -38,11 +38,15 @@ class ExplainingStep
 {
 public:
     bool isValid() const;
+    bool operator<(const ExplainingStep &other) const {
+        return std::tie(location, ranges, message)
+               < std::tie(other.location, other.ranges, other.message);
+    }
 
     QString message;
     QString extendedMessage;
     Debugger::DiagnosticLocation location;
-    QList<Debugger::DiagnosticLocation> ranges;
+    QVector<Debugger::DiagnosticLocation> ranges;
     int depth = 0;
     bool isFixIt = false;
 };
@@ -58,7 +62,7 @@ public:
     QString issueContextKind;
     QString issueContext;
     Debugger::DiagnosticLocation location;
-    QList<ExplainingStep> explainingSteps;
+    QVector<ExplainingStep> explainingSteps;
     bool hasFixits = false;
 };
 

src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp

@@ -54,8 +54,10 @@ private:
 
 ClangToolsDiagnosticModel::ClangToolsDiagnosticModel(QObject *parent)
     : Utils::TreeModel<>(parent)
+    , m_filesWatcher(std::make_unique<QFileSystemWatcher>())
 {
     setHeader({tr("Issue"), tr("Location"), tr("Fixit Status")});
+    connectFileWatcher();
 }
 
 void ClangToolsDiagnosticModel::addDiagnostics(const QList<Diagnostic> &diagnostics)
@@ -68,8 +70,11 @@ void ClangToolsDiagnosticModel::addDiagnostics(const QList<Diagnostic> &diagnost
         emit fixItsToApplyCountChanged(m_fixItsToApplyCount);
     };
 
+    if (!diagnostics.empty())
+        addWatchedPath(diagnostics.front().location.filePath);
+
     for (const Diagnostic &d : diagnostics)
-        rootItem()->appendChild(new DiagnosticItem(d, onFixitStatusChanged));
+        rootItem()->appendChild(new DiagnosticItem(d, onFixitStatusChanged, this));
 }
 
 QList<Diagnostic> ClangToolsDiagnosticModel::diagnostics() const
@@ -85,6 +90,49 @@ int ClangToolsDiagnosticModel::diagnosticsCount() const
     return rootItem()->childCount();
 }
 
+void ClangToolsDiagnosticModel::updateItems(const DiagnosticItem *changedItem)
+{
+    for (auto item : stepsToItemsCache[changedItem->diagnostic().explainingSteps]) {
+        if (item != changedItem)
+            item->setFixItStatus(changedItem->fixItStatus());
+    }
+}
+
+void ClangToolsDiagnosticModel::connectFileWatcher()
+{
+    connect(m_filesWatcher.get(),
+            &QFileSystemWatcher::fileChanged,
+            this,
+            &ClangToolsDiagnosticModel::onFileChanged);
+}
+
+void ClangToolsDiagnosticModel::clearAndSetupCache()
+{
+    m_filesWatcher = std::make_unique<QFileSystemWatcher>();
+    connectFileWatcher();
+    stepsToItemsCache.clear();
+}
+
+void ClangToolsDiagnosticModel::onFileChanged(const QString &path)
+{
+    for (Utils::TreeItem * const item : *rootItem()) {
+        auto diagnosticItem = static_cast<DiagnosticItem *>(item);
+        if (diagnosticItem->diagnostic().location.filePath == path)
+            diagnosticItem->setFixItStatus(FixitStatus::Invalidated);
+    }
+    removeWatchedPath(path);
+}
+
+void ClangToolsDiagnosticModel::removeWatchedPath(const QString &path)
+{
+    m_filesWatcher->removePath(path);
+}
+
+void ClangToolsDiagnosticModel::addWatchedPath(const QString &path)
+{
+    m_filesWatcher->addPath(path);
+}
+
 static QString createDiagnosticToolTipString(const Diagnostic &diagnostic)
 {
     using StringPair = QPair<QString, QString>;
@@ -216,10 +264,12 @@ static QString fullText(const Diagnostic &diagnostic)
     return text;
 }
 
-
+DiagnosticItem::DiagnosticItem(const Diagnostic &diag,
-DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged)
+                               const OnFixitStatusChanged &onFixitStatusChanged,
+                               ClangToolsDiagnosticModel *parent)
     : m_diagnostic(diag)
     , m_onFixitStatusChanged(onFixitStatusChanged)
+    , m_parentModel(parent)
 {
     if (diag.hasFixits)
         m_fixitStatus = FixitStatus::NotScheduled;
@@ -231,6 +281,9 @@ DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChange
             return;
     }
 
+    if (!diag.explainingSteps.isEmpty())
+        m_parentModel->stepsToItemsCache[diag.explainingSteps].push_back(this);
+
     foreach (const ExplainingStep &s, diag.explainingSteps)
         appendChild(new ExplainingStepItem(s));
 }
@@ -336,6 +389,7 @@ bool DiagnosticItem::setData(int column, const QVariant &data, int role)
                                           : FixitStatus::NotScheduled;
 
         setFixItStatus(newStatus);
+        m_parentModel->updateItems(this);
         return true;
     }
 
@@ -357,6 +411,14 @@ void DiagnosticItem::setFixitOperations(const ReplacementOperations &replacement
     m_fixitOperations = replacements;
 }
 
+bool DiagnosticItem::hasNewFixIts() const
+{
+    if (m_diagnostic.explainingSteps.empty())
+        return false;
+
+    return m_parentModel->stepsToItemsCache[m_diagnostic.explainingSteps].front() == this;
+}
+
 ExplainingStepItem::ExplainingStepItem(const ExplainingStep &step) : m_step(step)
 {
 }

src/plugins/clangtools/clangtoolsdiagnosticmodel.h

@@ -33,10 +33,14 @@
 #include <utils/fileutils.h>
 #include <utils/treemodel.h>
 
+#include <QFileSystemWatcher>
 #include <QPointer>
 #include <QSortFilterProxyModel>
+#include <QVector>
 
 #include <functional>
+#include <map>
+#include <memory>
 
 namespace ProjectExplorer { class Project; }
 
@@ -52,11 +56,14 @@ enum class FixitStatus {
     Invalidated,
 };
 
+class ClangToolsDiagnosticModel;
+
 class DiagnosticItem : public Utils::TreeItem
 {
 public:
     using OnFixitStatusChanged = std::function<void(FixitStatus newStatus)>;
-    DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged);
+    DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged,
+                   ClangToolsDiagnosticModel *parent);
     ~DiagnosticItem() override;
 
     const Diagnostic &diagnostic() const { return m_diagnostic; }
@@ -64,6 +71,7 @@ public:
     FixitStatus fixItStatus() const { return m_fixitStatus; }
     void setFixItStatus(const FixitStatus &status);
 
+    bool hasNewFixIts() const;
     ReplacementOperations &fixitOperations() { return m_fixitOperations; }
     void setFixitOperations(const ReplacementOperations &replacements);
 
@@ -78,12 +86,15 @@ private:
 
     ReplacementOperations  m_fixitOperations;
     FixitStatus m_fixitStatus = FixitStatus::NotAvailable;
+    ClangToolsDiagnosticModel *m_parentModel = nullptr;
 };
 
 class ClangToolsDiagnosticModel : public Utils::TreeModel<>
 {
     Q_OBJECT
 
+    friend class DiagnosticItem;
+
 public:
     ClangToolsDiagnosticModel(QObject *parent = nullptr);
 
@@ -96,10 +107,22 @@ public:
         DiagnosticRole = Debugger::DetailedErrorView::FullTextRole + 1
     };
 
+    void clearAndSetupCache();
+    void removeWatchedPath(const QString &path);
+    void addWatchedPath(const QString &path);
+
 signals:
     void fixItsToApplyCountChanged(int count);
 
 private:
+    void connectFileWatcher();
+    void updateItems(const DiagnosticItem *changedItem);
+    void onFileChanged(const QString &path);
+
+private:
+    std::map<QVector<ExplainingStep>, QVector<DiagnosticItem *>> stepsToItemsCache;
+    std::unique_ptr<QFileSystemWatcher> m_filesWatcher;
+    QVector<QString> m_allowFileWriteOnce;
     int m_fixItsToApplyCount = 0;
 };
 

src/plugins/debugger/analyzer/diagnosticlocation.cpp

@@ -46,6 +46,12 @@ bool operator==(const DiagnosticLocation &first, const DiagnosticLocation &secon
             && first.column == second.column;
 }
 
+bool operator<(const DiagnosticLocation &first, const DiagnosticLocation &second)
+{
+    return std::tie(first.filePath, first.line, first.column)
+           < std::tie(second.filePath, second.line, second.column);
+}
+
 QDebug operator<<(QDebug dbg, const DiagnosticLocation &location)
 {
     dbg.nospace() << "Location(" << location.filePath << ", "

src/plugins/debugger/analyzer/diagnosticlocation.h

@@ -49,6 +49,7 @@ public:
 };
 
 DEBUGGER_EXPORT bool operator==(const DiagnosticLocation &first, const DiagnosticLocation &second);
+DEBUGGER_EXPORT bool operator<(const DiagnosticLocation &first, const DiagnosticLocation &second);
 DEBUGGER_EXPORT QDebug operator<<(QDebug dbg, const DiagnosticLocation &location);
 
 } // namespace Debugger