ClangTools: Fix applying fixits one by one

If a file had multiple fixits and only a subset was applied, the
remaining fixits in that file were invalidated. Fix this by adjusting
the locations of the not yet applied fixits.

Change-Id: I2c190412e871e9011c4d4a62ed938e76ad4cdf72
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/plugins/clangtools/clangfixitsrefactoringchanges.cpp

@@ -0,0 +1,148 @@
+/****************************************************************************
+**
+** Copyright (C) 2018 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of Qt Creator.
+**
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+****************************************************************************/
+
+#include "clangfixitsrefactoringchanges.h"
+
+#include <coreplugin/editormanager/editormanager.h>
+
+#include <QDebug>
+#include <QFileInfo>
+#include <QLoggingCategory>
+#include <QTextBlock>
+#include <QTextCursor>
+
+#include <utils/qtcassert.h>
+
+Q_LOGGING_CATEGORY(fixitsLog, "qtc.clangtools.fixits");
+
+using namespace Utils;
+
+namespace ClangTools {
+namespace Internal {
+
+int FixitsRefactoringFile::position(unsigned line, unsigned column) const
+{
+    QTC_ASSERT(line != 0, return -1);
+    QTC_ASSERT(column != 0, return -1);
+    return document()->findBlockByNumber(line - 1).position() + column - 1;
+}
+
+static QDebug operator<<(QDebug debug, const ReplacementOperation &op)
+{
+    debug.nospace() << "ReplacementOperation("
+                    << op.pos << ", "
+                    << op.length << ", "
+                    << op.text << ", "
+                    << op.apply
+                    << ")"
+                    ;
+
+    return debug;
+}
+
+bool FixitsRefactoringFile::apply()
+{
+    qCDebug(fixitsLog) << "Applying fixits for" << m_filePath;
+
+    if (m_replacementOperations.isEmpty())
+        return false; // Error nothing to apply TODO: Is this correct to return?
+
+    QTC_ASSERT(!m_filePath.isEmpty(), return false);
+
+    // Check for permissions
+    if (!QFileInfo(m_filePath).isWritable())
+        return false; // Error file not writable
+
+    // Apply changes
+    QTextDocument *doc = document();
+    QTextCursor cursor(doc);
+
+    for (int i=0; i < m_replacementOperations.size(); ++i) {
+        ReplacementOperation &op = *m_replacementOperations[i];
+        if (op.apply) {
+            qCDebug(fixitsLog) << " " << i << "Applying" << op;
+
+            // Shift subsequent operations that are affected
+            shiftAffectedReplacements(op, i + 1);
+
+            // Apply
+            cursor.setPosition(op.pos);
+            cursor.setPosition(op.pos + op.length, QTextCursor::KeepAnchor);
+            cursor.insertText(op.text);
+        }
+    }
+
+    // Write file
+    if (!m_textFileFormat.codec)
+        return false; // Error reading file
+
+    QString error;
+    if (!m_textFileFormat.writeFile(m_filePath, doc->toPlainText(), &error)) {
+        qCDebug(fixitsLog) << "ERROR: Could not write file" << m_filePath << ":" << error;
+        return false; // Error writing file
+    }
+
+    return true;
+}
+
+QTextDocument *FixitsRefactoringFile::document() const
+{
+    if (!m_document) {
+        QString fileContents;
+        if (!m_filePath.isEmpty()) {
+            QString error;
+            QTextCodec *defaultCodec = Core::EditorManager::defaultTextCodec();
+            TextFileFormat::ReadResult result = TextFileFormat::readFile(
+                        m_filePath, defaultCodec,
+                        &fileContents, &m_textFileFormat,
+                        &error);
+            if (result != TextFileFormat::ReadSuccess) {
+                qCDebug(fixitsLog) << "ERROR: Could not read " << m_filePath << ":" << error;
+                m_textFileFormat.codec = nullptr;
+            }
+        }
+        // always make a QTextDocument to avoid excessive null checks
+        m_document = new QTextDocument(fileContents);
+    }
+    return m_document;
+}
+
+void FixitsRefactoringFile::shiftAffectedReplacements(const ReplacementOperation &op, int startIndex)
+{
+    for (int i = startIndex; i < m_replacementOperations.size(); ++i) {
+        ReplacementOperation &current = *m_replacementOperations[i];
+        ReplacementOperation before = current;
+
+        if (op.pos <= current.pos)
+            current.pos += op.text.size();
+        if (op.pos < current.pos)
+            current.pos -= op.length;
+
+        qCDebug(fixitsLog) << "    shift:" << i << before << " ====> " << current;
+    }
+}
+
+} // namespace Internal
+} // namespace ClangTools

src/plugins/clangtools/clangfixitsrefactoringchanges.h

@@ -0,0 +1,73 @@
+/****************************************************************************
+**
+** Copyright (C) 2018 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of Qt Creator.
+**
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+****************************************************************************/
+
+#pragma once
+
+#include <utils/changeset.h>
+#include <utils/textfileformat.h>
+
+#include <QString>
+#include <QTextDocument>
+#include <QVector>
+
+namespace ClangTools {
+namespace Internal {
+
+class ReplacementOperation
+{
+public:
+    int pos = -1;
+    int length = -1;
+    QString text;
+    bool apply = false;
+};
+using ReplacementOperations = QVector<ReplacementOperation *>;
+
+/// Simplified version of TextEditor::RefactoringFile that allows
+/// to operate on not owned ReplamentOperations
+class FixitsRefactoringFile
+{
+public:
+    FixitsRefactoringFile() = default;
+    FixitsRefactoringFile(const QString &filePath) : m_filePath(filePath) {}
+
+    bool isValid() const { return !m_filePath.isEmpty(); }
+    int position(unsigned line, unsigned column) const;
+
+    void setReplacements(const ReplacementOperations &ops) { m_replacementOperations = ops; }
+    bool apply();
+
+private:
+    QTextDocument *document() const;
+    void shiftAffectedReplacements(const ReplacementOperation &op, int startIndex);
+
+    QString m_filePath;
+    mutable Utils::TextFileFormat m_textFileFormat;
+    mutable QTextDocument *m_document = nullptr;
+    ReplacementOperations m_replacementOperations; // Not owned.
+};
+
+} // namespace Internal
+} // namespace ClangTools

src/plugins/clangtools/clangtidyclazytool.cpp

@@ -25,6 +25,7 @@
 
 #include "clangtidyclazytool.h"
 
+#include "clangfixitsrefactoringchanges.h"
 #include "clangselectablefilesdialog.h"
 #include "clangtoolsconstants.h"
 #include "clangtoolsdiagnosticmodel.h"
@@ -50,8 +51,6 @@
 #include <projectexplorer/target.h>
 #include <projectexplorer/session.h>
 
-#include <texteditor/refactoringchanges.h>
-
 #include <utils/fancylineedit.h>
 #include <utils/utilsicons.h>
 
@@ -75,9 +74,9 @@ public:
     class RefactoringFileInfo
     {
     public:
-        bool isValid() const { return file; }
+        bool isValid() const { return file.isValid(); }
 
-        TextEditor::RefactoringFilePtr file;
+        FixitsRefactoringFile file;
         QVector<DiagnosticItem *> diagnosticItems;
         bool hasScheduledFixits = false;
     };
@@ -91,7 +90,7 @@ public:
             // Get or create refactoring file
             RefactoringFileInfo &fileInfo = m_refactoringFileInfos[filePath];
             if (!fileInfo.isValid())
-                fileInfo.file = m_changes.file(filePath);
+                fileInfo.file = FixitsRefactoringFile(filePath);
 
             // Append item
             fileInfo.diagnosticItems += diagnosticItem;
@@ -100,61 +99,90 @@ public:
         }
     }
 
-
+    static void addFixitOperations(DiagnosticItem *diagnosticItem,
-    static bool addChanges(TextEditor::RefactoringFilePtr file,
+                                   const FixitsRefactoringFile &file, bool apply)
-                           const Diagnostic &diagnostic,
-                           ChangeSet &changeSet)
     {
-        for (const ExplainingStep &step : diagnostic.explainingSteps) {
+        // Did we already created the fixit operations?
+        ReplacementOperations currentOps = diagnosticItem->fixitOperations();
+        if (!currentOps.isEmpty()) {
+            for (ReplacementOperation *op : currentOps)
+                op->apply = apply;
+            return;
+        }
+
+        // Collect/construct the fixit operations
+        ReplacementOperations replacements;
+
+        for (const ExplainingStep &step : diagnosticItem->diagnostic().explainingSteps) {
             if (!step.isFixIt)
                 continue;
 
             const Debugger::DiagnosticLocation start = step.ranges.first();
             const Debugger::DiagnosticLocation end = step.ranges.last();
-            const bool operationAdded = changeSet.replace(file->position(start.line, start.column),
+            const int startPos = file.position(start.line, start.column);
-                                                          file->position(end.line, end.column),
+            const int endPos = file.position(end.line, end.column);
-                                                          step.message);
+
-            if (!operationAdded)
+            auto op = new ReplacementOperation;
-                return false;
+            op->pos = startPos;
+            op->length = endPos - startPos;
+            op->text = step.message;
+            op->apply = apply;
+
+            replacements += op;
         }
 
-        return true;
+        diagnosticItem->setFixitOperations(replacements);
     }
 
     void apply()
     {
-        for (auto i = m_refactoringFileInfos.begin(); i != m_refactoringFileInfos.end(); ++i) {
+        for (auto it = m_refactoringFileInfos.begin(); it != m_refactoringFileInfos.end(); ++it) {
-            const RefactoringFileInfo &fileInfo = i.value();
+            RefactoringFileInfo &fileInfo = it.value();
 
-            QVector<DiagnosticItem *> itemsSucceeded;
+            QVector<DiagnosticItem *> itemsScheduledOrSchedulable;
-            QVector<DiagnosticItem *> itemsFailed;
+            QVector<DiagnosticItem *> itemsScheduled;
-            QVector<DiagnosticItem *> itemsInvalidated;
+            QVector<DiagnosticItem *> itemsSchedulable;
 
-            // Construct change set
+            // Construct refactoring operations
             for (DiagnosticItem *diagnosticItem : fileInfo.diagnosticItems) {
                 const FixitStatus fixItStatus = diagnosticItem->fixItStatus();
-                if (fixItStatus == FixitStatus::Scheduled) {
+
-                    ChangeSet changeSet = fileInfo.file->changeSet();
+                const bool isScheduled = fixItStatus == FixitStatus::Scheduled;
-                    if (addChanges(fileInfo.file, diagnosticItem->diagnostic(), changeSet))
+                const bool isSchedulable = fileInfo.hasScheduledFixits
-                        itemsSucceeded += diagnosticItem;
+                                           && fixItStatus == FixitStatus::NotScheduled;
-                    else // Ops, some fixits might have overlapping ranges.
+
-                        itemsFailed += diagnosticItem;
+                if (isScheduled || isSchedulable) {
-                    fileInfo.file->setChangeSet(changeSet);
+                    addFixitOperations(diagnosticItem, fileInfo.file, isScheduled);
-                } else if (fileInfo.hasScheduledFixits && fixItStatus == FixitStatus::NotScheduled) {
+                    itemsScheduledOrSchedulable += diagnosticItem;
-                    itemsInvalidated += diagnosticItem;
+                    if (isScheduled)
+                        itemsScheduled += diagnosticItem;
+                    else
+                        itemsSchedulable += diagnosticItem;
                 }
             }
 
+            // Collect replacements
+            ReplacementOperations ops;
+            for (DiagnosticItem *item : itemsScheduledOrSchedulable)
+                ops += item->fixitOperations();
+
             // Apply file
-            if (!fileInfo.file->apply()) {
+            QVector<DiagnosticItem *> itemsApplied;
-                itemsSucceeded.clear();
+            QVector<DiagnosticItem *> itemsFailedToApply;
-                itemsFailed = fileInfo.diagnosticItems;
+            QVector<DiagnosticItem *> itemsInvalidated;
+
+            fileInfo.file.setReplacements(ops);
+            if (fileInfo.file.apply()) {
+                itemsApplied = itemsScheduled;
+            } else {
+                itemsFailedToApply = itemsScheduled;
+                itemsInvalidated = itemsSchedulable;
             }
 
             // Update DiagnosticItem state
-            for (DiagnosticItem *diagnosticItem : itemsSucceeded)
+            for (DiagnosticItem *diagnosticItem : itemsScheduled)
                 diagnosticItem->setFixItStatus(FixitStatus::Applied);
-            for (DiagnosticItem *diagnosticItem : itemsFailed)
+            for (DiagnosticItem *diagnosticItem : itemsFailedToApply)
                 diagnosticItem->setFixItStatus(FixitStatus::FailedToApply);
             for (DiagnosticItem *diagnosticItem : itemsInvalidated)
                 diagnosticItem->setFixItStatus(FixitStatus::Invalidated);
@@ -162,7 +190,6 @@ public:
     }
 
 private:
-    TextEditor::RefactoringChanges m_changes;
     QMap<QString, RefactoringFileInfo> m_refactoringFileInfos;
 };
 

src/plugins/clangtools/clangtools.pro

@@ -9,6 +9,7 @@ LIBS += $$LIBCLANG_LIBS
 INCLUDEPATH += $$LLVM_INCLUDEPATH
 
 SOURCES += \
+    clangfixitsrefactoringchanges.cpp \
     clangselectablefilesdialog.cpp \
     clangtoolsdiagnosticview.cpp \
     clangtoolsprojectsettingswidget.cpp \
@@ -29,6 +30,7 @@ SOURCES += \
 
 HEADERS += \
     clangfileinfo.h \
+    clangfixitsrefactoringchanges.h \
     clangselectablefilesdialog.h \
     clangtoolsdiagnosticview.h \
     clangtoolsprojectsettingswidget.h \

src/plugins/clangtools/clangtools.qbs

@@ -43,6 +43,8 @@ QtcPlugin {
 
     files: [
         "clangfileinfo.h",
+        "clangfixitsrefactoringchanges.cpp",
+        "clangfixitsrefactoringchanges.h",
         "clangselectablefilesdialog.cpp",
         "clangselectablefilesdialog.h",
         "clangselectablefilesdialog.ui",

src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp

@@ -230,6 +230,11 @@ DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChange
         appendChild(new ExplainingStepItem(s));
 }
 
+DiagnosticItem::~DiagnosticItem()
+{
+    setFixitOperations(ReplacementOperations());
+}
+
 Qt::ItemFlags DiagnosticItem::flags(int column) const
 {
     const Qt::ItemFlags itemFlags = TreeItem::flags(column);
@@ -338,9 +343,10 @@ void DiagnosticItem::setFixItStatus(const FixitStatus &status)
         m_onFixitStatusChanged(status);
 }
 
-FixitStatus DiagnosticItem::fixItStatus() const
+void DiagnosticItem::setFixitOperations(const ReplacementOperations &replacements)
 {
-    return m_fixitStatus;
+    qDeleteAll(m_fixitOperations);
+    m_fixitOperations = replacements;
 }
 
 ExplainingStepItem::ExplainingStepItem(const ExplainingStep &step) : m_step(step)

src/plugins/clangtools/clangtoolsdiagnosticmodel.h

@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include "clangfixitsrefactoringchanges.h"
 #include "clangtoolsdiagnostic.h"
 #include "clangtoolsprojectsettings.h"
 
@@ -56,12 +57,16 @@ class DiagnosticItem : public Utils::TreeItem
 public:
     using OnFixitStatusChanged = std::function<void(FixitStatus newStatus)>;
     DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged);
+    ~DiagnosticItem() override;
 
     Diagnostic diagnostic() const { return m_diagnostic; }
 
-    FixitStatus fixItStatus() const;
+    FixitStatus fixItStatus() const { return m_fixitStatus; }
     void setFixItStatus(const FixitStatus &status);
 
+    ReplacementOperations &fixitOperations() { return m_fixitOperations; }
+    void setFixitOperations(const ReplacementOperations &replacements);
+
 private:
     Qt::ItemFlags flags(int column) const override;
     QVariant data(int column, int role) const override;
@@ -69,8 +74,10 @@ private:
 
 private:
     const Diagnostic m_diagnostic;
-    FixitStatus m_fixitStatus = FixitStatus::NotAvailable;
     OnFixitStatusChanged m_onFixitStatusChanged;
+
+    ReplacementOperations  m_fixitOperations;
+    FixitStatus m_fixitStatus = FixitStatus::NotAvailable;
 };
 
 class ClangToolsDiagnosticModel : public Utils::TreeModel<>