Clang: Use line and column instead of offset for diagnostics

Offsets can be get wrong because of the utf8 <-> utf16 differences. Line
and columns are not that sensitive to it.

Change-Id: I2e3e8c000621b6d694a4ada6df176f29427794f5
Reviewed-by: Nikolai Kosjar <nikolai.kosjar@theqtcompany.com>

src/libs/clangbackendipc/sourcelocationcontainer.cpp

@@ -39,10 +39,10 @@ namespace ClangBackEnd {
 
 SourceLocationContainer::SourceLocationContainer(const Utf8String &filePath,
                                                  uint line,
-                                                 uint offset)
+                                                 uint column)
     : filePath_(filePath),
       line_(line),
-      offset_(offset)
+      column_(column)
 {
 }
 
@@ -57,16 +57,16 @@ uint SourceLocationContainer::line() const
 }
 
 
-uint SourceLocationContainer::offset() const
+uint SourceLocationContainer::column() const
 {
-    return offset_;
+    return column_;
 }
 
 QDataStream &operator<<(QDataStream &out, const SourceLocationContainer &container)
 {
     out << container.filePath_;
     out << container.line_;
-    out << container.offset_;
+    out << container.column_;
 
     return out;
 }
@@ -75,7 +75,7 @@ QDataStream &operator>>(QDataStream &in, SourceLocationContainer &container)
 {
     in >> container.filePath_;
     in >> container.line_;
-    in >> container.offset_;
+    in >> container.column_;
 
     return in;
 }
@@ -87,14 +87,16 @@ bool operator==(const SourceLocationContainer &first, const SourceLocationContai
 
 bool operator!=(const SourceLocationContainer &first, const SourceLocationContainer &second)
 {
-    return first.offset_ != second.offset_
+    return first.line_ != second.line_
+        || first.column_ != second.column_
         || first.filePath_ != second.filePath_;
 }
 
 bool operator<(const SourceLocationContainer &first, const SourceLocationContainer &second)
 {
     return first.filePath_ < second.filePath_
-        || (first.filePath_ == second.filePath_ && first.offset_ < second.offset_);
+        || (first.filePath_ == second.filePath_ && first.line_ < second.line_)
+        || (first.filePath_ == second.filePath_ && first.line_ == second.line_ && first.column_ < second.column_);
 }
 
 QDebug operator<<(QDebug debug, const SourceLocationContainer &container)
@@ -102,7 +104,7 @@ QDebug operator<<(QDebug debug, const SourceLocationContainer &container)
     debug.nospace() << "SourceLocationContainer("
                     << container.filePath() << ", "
                     << container.line() << ", "
-                    << container.offset()
+                    << container.column()
                     << ")";
     return debug;
 }
@@ -112,7 +114,7 @@ void PrintTo(const SourceLocationContainer &container, ::std::ostream* os)
     *os << "["
         << container.filePath().constData() << ", "
         << container.line() << ", "
-        << container.offset()
+        << container.column()
         << "]";
 }
 } // namespace ClangBackEnd

src/libs/clangbackendipc/sourcelocationcontainer.h

@@ -51,16 +51,16 @@ public:
     SourceLocationContainer() = default;
     SourceLocationContainer(const Utf8String &filePath,
                             uint line,
-                            uint offset);
+                            uint column);
 
     const Utf8String &filePath() const;
     uint line() const;
-    uint offset() const;
+    uint column() const;
 
 private:
     Utf8String filePath_;
     uint line_;
-    uint offset_;
+    uint column_;
 };
 
 CMBIPC_EXPORT QDataStream &operator<<(QDataStream &out, const SourceLocationContainer &container);

src/plugins/clangcodemodel/clangdiagnosticmanager.cpp

@@ -38,6 +38,8 @@
 #include <utils/fileutils.h>
 #include <utils/qtcassert.h>
 
+#include <QTextBlock>
+
 namespace  {
 
 QTextEdit::ExtraSelection createExtraSelections(const QTextCharFormat &mainformat,
@@ -53,6 +55,14 @@ QTextEdit::ExtraSelection createExtraSelections(const QTextCharFormat &mainforma
     return extraSelection;
 }
 
+int positionInText(QTextDocument *textDocument,
+                   const ClangBackEnd::SourceLocationContainer &sourceLocationContainer)
+{
+    auto textBlock = textDocument->findBlockByNumber(int(sourceLocationContainer.line()) - 1);
+
+    return textBlock.position() + int(sourceLocationContainer.column()) - 1;
+}
+
 void addRangeSelections(const ClangBackEnd::DiagnosticContainer &diagnostic,
                         QTextDocument *textDocument,
                         const QTextCharFormat &contextFormat,
@@ -61,8 +71,8 @@ void addRangeSelections(const ClangBackEnd::DiagnosticContainer &diagnostic,
 {
     for (auto &&range : diagnostic.ranges()) {
         QTextCursor cursor(textDocument);
-        cursor.setPosition(int(range.start().offset()));
-        cursor.setPosition(int(range.end().offset()), QTextCursor::KeepAnchor);
+        cursor.setPosition(positionInText(textDocument, range.start()));
+        cursor.setPosition(positionInText(textDocument, range.end()), QTextCursor::KeepAnchor);
 
         auto extraSelection = createExtraSelections(contextFormat, cursor, diagnosticText);
 
@@ -70,14 +80,15 @@ void addRangeSelections(const ClangBackEnd::DiagnosticContainer &diagnostic,
     }
 }
 
-QTextCursor createSelectionCursor(QTextDocument *textDocument, uint position)
+QTextCursor createSelectionCursor(QTextDocument *textDocument,
+                                  const ClangBackEnd::SourceLocationContainer &sourceLocationContainer)
 {
     QTextCursor cursor(textDocument);
-    cursor.setPosition(int(position));
+    cursor.setPosition(positionInText(textDocument, sourceLocationContainer));
     cursor.movePosition(QTextCursor::EndOfWord, QTextCursor::KeepAnchor);
 
     if (!cursor.hasSelection()) {
-        cursor.setPosition(int(position) - 1);
+        cursor.setPosition(positionInText(textDocument, sourceLocationContainer) - 1);
         cursor.movePosition(QTextCursor::Right, QTextCursor::KeepAnchor, 2);
     }
 
@@ -122,7 +133,7 @@ void addSelections(const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics
                    QList<QTextEdit::ExtraSelection> &extraSelections)
 {
     for (auto &&diagnostic : diagnostics) {
-        auto cursor = createSelectionCursor(textDocument, diagnostic.location().offset());
+        auto cursor = createSelectionCursor(textDocument, diagnostic.location());
 
         auto text = diagnosticText(diagnostic);
         auto extraSelection = createExtraSelections(mainFormat, cursor, text);

src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp

@@ -170,18 +170,37 @@ void ClangEditorDocumentProcessor::updateCodeWarnings(const QVector<ClangBackEnd
         emit codeWarningsUpdated(revision(), codeWarnings);
     }
 }
+namespace {
 
-static QList<TextEditor::BlockRange>
-toTextEditorBlocks(const QVector<ClangBackEnd::SourceRangeContainer> &ifdefedOutRanges)
+int positionInText(QTextDocument *textDocument,
+                   const ClangBackEnd::SourceLocationContainer &sourceLocationContainer)
+{
+    auto textBlock = textDocument->findBlockByNumber(int(sourceLocationContainer.line()) - 1);
+
+    return textBlock.position() + int(sourceLocationContainer.column()) - 1;
+}
+
+TextEditor::BlockRange
+toTextEditorBlock(QTextDocument *textDocument,
+                  const ClangBackEnd::SourceRangeContainer &sourceRangeContainer)
+{
+    return TextEditor::BlockRange(positionInText(textDocument, sourceRangeContainer.start()),
+                                  positionInText(textDocument, sourceRangeContainer.end()));
+}
+
+QList<TextEditor::BlockRange>
+toTextEditorBlocks(QTextDocument *textDocument,
+                   const QVector<ClangBackEnd::SourceRangeContainer> &ifdefedOutRanges)
 {
     QList<TextEditor::BlockRange> blockRanges;
     blockRanges.reserve(ifdefedOutRanges.size());
 
     for (const auto &range : ifdefedOutRanges)
-        blockRanges.append(TextEditor::BlockRange(range.start().offset(),range.end().offset()));
+        blockRanges.append(toTextEditorBlock(textDocument, range));
 
     return blockRanges;
 }
+}
 
 void ClangEditorDocumentProcessor::updateHighlighting(
         const QVector<ClangBackEnd::HighlightingMarkContainer> &highlightingMarks,
@@ -189,7 +208,7 @@ void ClangEditorDocumentProcessor::updateHighlighting(
         uint documentRevision)
 {
     if (documentRevision == revision()) {
-        const auto skippedPreprocessorBlocks = toTextEditorBlocks(skippedPreprocessorRanges);
+        const auto skippedPreprocessorBlocks = toTextEditorBlocks(textDocument(), skippedPreprocessorRanges);
         emit ifdefedOutBlocksUpdated(documentRevision, skippedPreprocessorBlocks);
 
         m_semanticHighlighter.setHighlightingRunner(

src/plugins/clangcodemodel/clangfixitoperation.cpp

@@ -32,6 +32,8 @@
 
 #include <texteditor/refactoringchanges.h>
 
+#include <QTextDocument>
+
 namespace ClangCodeModel {
 
 ClangFixItOperation::ClangFixItOperation(const Utf8String &filePath,
@@ -56,19 +58,26 @@ QString ClangCodeModel::ClangFixItOperation::description() const
 void ClangFixItOperation::perform()
 {
     const TextEditor::RefactoringChanges refactoringChanges;
-    TextEditor::RefactoringFilePtr refactoringFile = refactoringChanges.file(filePath.toString());
+    refactoringFile = refactoringChanges.file(filePath.toString());
     refactoringFile->setChangeSet(changeSet());
     refactoringFile->apply();
 }
 
+QString ClangFixItOperation::refactoringFileContent_forTestOnly() const
+{
+    return refactoringFile->document()->toPlainText();
+}
+
 Utils::ChangeSet ClangFixItOperation::changeSet() const
 {
     Utils::ChangeSet changeSet;
 
     for (const auto &fixItContainer : fixItContainers) {
         const auto range = fixItContainer.range();
-        changeSet.replace(range.start().offset(),
-                          range.end().offset(),
+        const auto start = range.start();
+        const auto end = range.end();
+        changeSet.replace(refactoringFile->position(start.line(), start.column()),
+                          refactoringFile->position(end.line(), end.column()),
                           fixItContainer.text());
     }
 

src/plugins/clangcodemodel/clangfixitoperation.h

@@ -34,9 +34,16 @@
 #include <texteditor/quickfix.h>
 
 #include <clangbackendipc/fixitcontainer.h>
+
 #include <utils/changeset.h>
 
 #include <QVector>
+#include <QSharedPointer>
+
+namespace TextEditor
+{
+class RefactoringFile;
+}
 
 namespace ClangCodeModel {
 
@@ -51,11 +58,15 @@ public:
     QString description() const override;
     void perform() override;
 
+    QString refactoringFileContent_forTestOnly() const;
+
+private:
     Utils::ChangeSet changeSet() const;
 
 private:
     Utf8String filePath;
     Utf8String fixItText;
+    QSharedPointer<TextEditor::RefactoringFile> refactoringFile;
     QVector<ClangBackEnd::FixItContainer> fixItContainers;
 };
 

src/tools/clangbackend/ipcsource/sourcelocation.cpp

@@ -67,7 +67,7 @@ uint SourceLocation::offset() const
 
 SourceLocationContainer SourceLocation::toSourceLocationContainer() const
 {
-    return SourceLocationContainer(filePath_, line_, offset_);
+    return SourceLocationContainer(filePath_, line_, column_);
 }
 
 SourceLocation::SourceLocation(CXSourceLocation cxSourceLocation)
@@ -94,7 +94,8 @@ SourceLocation::SourceLocation(CXTranslationUnit cxTranslationUnit,
                                          line,
                                          column)),
       filePath_(filePath),
-      line_(line)
+      line_(line),
+      column_(column)
 {
 }
 

tests/unit/mockup/texteditor/refactoringchanges.h

@@ -31,7 +31,20 @@
 #ifndef REFACTORINGCHANGES_H
 #define REFACTORINGCHANGES_H
 
+#include <QFile>
 #include <QSharedPointer>
+#include <QTextBlock>
+#include <QTextCursor>
+#include <QTextDocument>
+
+#include <utils/changeset.h>
+
+#include <memory>
+
+#include "gtest/gtest.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest-qt-printing.h"
 
 QT_BEGIN_NAMESPACE
 class QString;
@@ -47,20 +60,68 @@ class RefactoringFile;
 class RefactoringChangesData;
 typedef QSharedPointer<RefactoringFile> RefactoringFilePtr;
 
+using testing::NotNull;
+
 class RefactoringFile
 {
 public:
-    void setChangeSet(const Utils::ChangeSet &) {}
-    void apply() {}
+    RefactoringFile(std::unique_ptr<QTextDocument> &&textDocument)
+        : textDocument(std::move(textDocument))
+    {
+    }
+
+    const QTextDocument *document() const
+    {
+        return textDocument.get();
+    }
+
+    void setChangeSet(const Utils::ChangeSet &changes)
+    {
+        this->changes = changes;
+    }
+
+    void apply()
+    {
+        QTextCursor textCursor(textDocument.get());
+        changes.apply(&textCursor);
+        changes.clear();
+    }
+
+    int position(uint line, uint column)
+    {
+        return textDocument->findBlockByNumber(uint(line) - 1).position() + int(column) - 1;
+    }
+
+private:
+    std::unique_ptr<QTextDocument> textDocument;
+    Utils::ChangeSet changes;
 };
 
+QString readFile(const QString &filePath)
+{
+    EXPECT_FALSE(filePath.isEmpty());
+
+    QFile file(filePath);
+
+    EXPECT_TRUE(file.open(QFile::ReadOnly));
+
+    auto content = file.readAll();
+
+    EXPECT_FALSE(content.isEmpty());
+
+    return QString::fromUtf8(content);
+}
+
 class RefactoringChanges
 {
 public:
     RefactoringChanges() {}
     virtual ~RefactoringChanges() {}
 
-    RefactoringFilePtr file(const QString &) const { return RefactoringFilePtr(); }
+    RefactoringFilePtr file(const QString &filePath) const
+    {
+        return RefactoringFilePtr(new RefactoringFile(std::unique_ptr<QTextDocument>(new QTextDocument(readFile(filePath)))));
+    }
 };
 
 class RefactoringChangesData

tests/unit/unittest/clangfixitoperationtest.cpp

@@ -58,13 +58,12 @@ QString unsavedFileContent(const QString &unsavedFilePath)
     return QString::fromUtf8(unsavedFileContentFile.readAll());
 }
 
-MATCHER_P2(MatchText, errorText, expectedText,
-           std::string(negation ? "hasn't" : "has") + " error text:\n" + PrintToString(errorText) +
-           " and expected text:\n" + PrintToString(expectedText))
+MATCHER_P(MatchText, expectedText,
+          std::string(negation ? "hasn't" : "has")
+          + " expected text:\n" + PrintToString(expectedText))
 {
-    QString resultText = errorText;
-    Utils::ChangeSet changeSet = arg.changeSet();
-    changeSet.apply(&resultText);
+    const ::ClangFixItOperation &operation = arg;
+    QString resultText = operation.refactoringFileContent_forTestOnly();
 
     if (resultText != expectedText) {
         *result_listener << "\n" << resultText.toUtf8().constData();
@@ -77,30 +76,31 @@ MATCHER_P2(MatchText, errorText, expectedText,
 class ClangFixItOperation : public ::testing::Test
 {
 protected:
-    Utf8String filePath;
+    Utf8String semicolonFilePath{TESTDATA_DIR"/diagnostic_semicolon_fixit.cpp", -1};
+    Utf8String compareFilePath{TESTDATA_DIR"/diagnostic_comparison_fixit.cpp", -1};
     Utf8String diagnosticText{Utf8StringLiteral("expected ';' at end of declaration")};
     FixItContainer semicolonFixItContainer{Utf8StringLiteral(";"),
-                                  {{filePath, 3u, 29u},
-                                   {filePath, 3u, 29u}}};
-    QString semicolonErrorFile{QString::fromUtf8(TESTDATA_DIR "/diagnostic_semicolon_fixit.cpp")};
+                                  {{semicolonFilePath, 3u, 13u},
+                                   {semicolonFilePath, 3u, 13u}}};
+    QString semicolonErrorFile{semicolonFilePath.toString()};
     QString semicolonExpectedFile{QString::fromUtf8(TESTDATA_DIR"/diagnostic_semicolon_fixit_expected.cpp")};
-    QString compareWarningFile{QString::fromUtf8(TESTDATA_DIR"/diagnostic_comparison_fixit.cpp")};