diff options
author | Nikolai Kosjar <nikolai.kosjar@qt.io> | 2018-05-24 09:03:44 +0200 |
---|---|---|
committer | Nikolai Kosjar <nikolai.kosjar@qt.io> | 2018-05-28 13:54:54 +0000 |
commit | bae61e08ea670aa083b9bc76e7744f0240fba423 (patch) | |
tree | 01a72b4488401bb6c0c87ce77c5945f5a6cbb264 | |
parent | 79b958b02ec0c8750b69310d84e0c444094b4ce0 (diff) | |
download | qt-creator-bae61e08ea670aa083b9bc76e7744f0240fba423.tar.gz |
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>
-rw-r--r-- | src/plugins/clangtools/clangfixitsrefactoringchanges.cpp | 148 | ||||
-rw-r--r-- | src/plugins/clangtools/clangfixitsrefactoringchanges.h | 73 | ||||
-rw-r--r-- | src/plugins/clangtools/clangtidyclazytool.cpp | 101 | ||||
-rw-r--r-- | src/plugins/clangtools/clangtools.pro | 2 | ||||
-rw-r--r-- | src/plugins/clangtools/clangtools.qbs | 2 | ||||
-rw-r--r-- | src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp | 10 | ||||
-rw-r--r-- | src/plugins/clangtools/clangtoolsdiagnosticmodel.h | 11 |
7 files changed, 306 insertions, 41 deletions
diff --git a/src/plugins/clangtools/clangfixitsrefactoringchanges.cpp b/src/plugins/clangtools/clangfixitsrefactoringchanges.cpp new file mode 100644 index 0000000000..dce0b9665a --- /dev/null +++ b/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 ¤t = *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 diff --git a/src/plugins/clangtools/clangfixitsrefactoringchanges.h b/src/plugins/clangtools/clangfixitsrefactoringchanges.h new file mode 100644 index 0000000000..c19d14aa02 --- /dev/null +++ b/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 diff --git a/src/plugins/clangtools/clangtidyclazytool.cpp b/src/plugins/clangtools/clangtidyclazytool.cpp index 71865abada..cf4a4ea5a4 100644 --- a/src/plugins/clangtools/clangtidyclazytool.cpp +++ b/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 bool addChanges(TextEditor::RefactoringFilePtr file, - const Diagnostic &diagnostic, - ChangeSet &changeSet) + static void addFixitOperations(DiagnosticItem *diagnosticItem, + const FixitsRefactoringFile &file, bool apply) { - 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), - file->position(end.line, end.column), - step.message); - if (!operationAdded) - return false; + const int startPos = file.position(start.line, start.column); + const int endPos = file.position(end.line, end.column); + + auto op = new ReplacementOperation; + 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) { - const RefactoringFileInfo &fileInfo = i.value(); + for (auto it = m_refactoringFileInfos.begin(); it != m_refactoringFileInfos.end(); ++it) { + RefactoringFileInfo &fileInfo = it.value(); - QVector<DiagnosticItem *> itemsSucceeded; - QVector<DiagnosticItem *> itemsFailed; - QVector<DiagnosticItem *> itemsInvalidated; + QVector<DiagnosticItem *> itemsScheduledOrSchedulable; + QVector<DiagnosticItem *> itemsScheduled; + 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(); - if (addChanges(fileInfo.file, diagnosticItem->diagnostic(), changeSet)) - itemsSucceeded += diagnosticItem; - else // Ops, some fixits might have overlapping ranges. - itemsFailed += diagnosticItem; - fileInfo.file->setChangeSet(changeSet); - } else if (fileInfo.hasScheduledFixits && fixItStatus == FixitStatus::NotScheduled) { - itemsInvalidated += diagnosticItem; + + const bool isScheduled = fixItStatus == FixitStatus::Scheduled; + const bool isSchedulable = fileInfo.hasScheduledFixits + && fixItStatus == FixitStatus::NotScheduled; + + if (isScheduled || isSchedulable) { + addFixitOperations(diagnosticItem, fileInfo.file, isScheduled); + itemsScheduledOrSchedulable += 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()) { - itemsSucceeded.clear(); - itemsFailed = fileInfo.diagnosticItems; + QVector<DiagnosticItem *> itemsApplied; + QVector<DiagnosticItem *> itemsFailedToApply; + 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; }; diff --git a/src/plugins/clangtools/clangtools.pro b/src/plugins/clangtools/clangtools.pro index ce0f000254..5f855290e0 100644 --- a/src/plugins/clangtools/clangtools.pro +++ b/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 \ diff --git a/src/plugins/clangtools/clangtools.qbs b/src/plugins/clangtools/clangtools.qbs index f947fb6115..71df19430c 100644 --- a/src/plugins/clangtools/clangtools.qbs +++ b/src/plugins/clangtools/clangtools.qbs @@ -43,6 +43,8 @@ QtcPlugin { files: [ "clangfileinfo.h", + "clangfixitsrefactoringchanges.cpp", + "clangfixitsrefactoringchanges.h", "clangselectablefilesdialog.cpp", "clangselectablefilesdialog.h", "clangselectablefilesdialog.ui", diff --git a/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp b/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp index fdb36c7ec9..db7ab4e14a 100644 --- a/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp +++ b/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) diff --git a/src/plugins/clangtools/clangtoolsdiagnosticmodel.h b/src/plugins/clangtools/clangtoolsdiagnosticmodel.h index c8e782fa0d..4c6a7e191c 100644 --- a/src/plugins/clangtools/clangtoolsdiagnosticmodel.h +++ b/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<> |