From 40181057cd6ada17d7b4eba05830e16f66e7fee5 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Tue, 1 Jun 2021 18:14:12 +0200 Subject: [PATCH] ClangCodeModel: Use clangd for local renaming Change-Id: I1536265a8d46c9840e722bdfcb8638906d3f45cf Reviewed-by: David Schulz --- .../clangcodemodel/clangcodemodelplugin.cpp | 1 + src/plugins/clangcodemodel/clangdclient.cpp | 132 ++++++++++++- src/plugins/clangcodemodel/clangdclient.h | 4 + .../clangcodemodel/clangrefactoringengine.cpp | 8 + .../clangcodemodel/test/clangdtests.cpp | 130 ++++++++++++- src/plugins/clangcodemodel/test/clangdtests.h | 11 ++ .../test/data/clangtestdata.qrc | 2 + .../local-references/local-references.pro | 5 + .../test/data/local-references/references.cpp | 174 ++++++++++++++++++ src/plugins/cppeditor/cppeditorwidget.cpp | 2 +- 10 files changed, 461 insertions(+), 8 deletions(-) create mode 100644 src/plugins/clangcodemodel/test/data/local-references/local-references.pro create mode 100644 src/plugins/clangcodemodel/test/data/local-references/references.cpp diff --git a/src/plugins/clangcodemodel/clangcodemodelplugin.cpp b/src/plugins/clangcodemodel/clangcodemodelplugin.cpp index 89949ab688a..2886f637cb3 100644 --- a/src/plugins/clangcodemodel/clangcodemodelplugin.cpp +++ b/src/plugins/clangcodemodel/clangcodemodelplugin.cpp @@ -207,6 +207,7 @@ QVector ClangCodeModelPlugin::createTestObjects() const new Tests::ClangCodeCompletionTest, new Tests::ClangdTestFindReferences, new Tests::ClangdTestFollowSymbol, + new Tests::ClangdTestLocalReferences, }; } #endif diff --git a/src/plugins/clangcodemodel/clangdclient.cpp b/src/plugins/clangcodemodel/clangdclient.cpp index 356bef8f51f..4df643a086e 100644 --- a/src/plugins/clangcodemodel/clangdclient.cpp +++ b/src/plugins/clangcodemodel/clangdclient.cpp @@ -25,6 +25,7 @@ #include "clangdclient.h" +#include #include #include #include @@ -71,10 +72,11 @@ class AstParams : public JsonObject { public: AstParams() {} - AstParams(const TextDocumentIdentifier &document, const Range &range) + AstParams(const TextDocumentIdentifier &document, const Range &range = {}) { setTextDocument(document); - setRange(range); + if (range.isValid()) + setRange(range); } using JsonObject::JsonObject; @@ -575,6 +577,28 @@ public: Utils::optional ast; }; +class LocalRefsData { +public: + LocalRefsData(quint64 id, TextEditor::TextDocument *doc, const QTextCursor &cursor, + CppTools::RefactoringEngineInterface::RenameCallback &&callback) + : id(id), document(doc), cursor(cursor), callback(std::move(callback)), + uri(DocumentUri::fromFilePath(doc->filePath())), revision(doc->document()->revision()) + {} + + ~LocalRefsData() + { + if (callback) + callback({}, {}, revision); + } + + const quint64 id; + const QPointer document; + const QTextCursor cursor; + CppTools::RefactoringEngineInterface::RenameCallback callback; + const DocumentUri uri; + const int revision; +}; + class ClangdClient::Private { @@ -600,14 +624,18 @@ public: void handleDeclDefSwitchReplies(); + QString searchTermFromCursor(const QTextCursor &cursor) const; + ClangdClient * const q; QHash runningFindUsages; Utils::optional followSymbolData; Utils::optional switchDeclDefData; + Utils::optional localRefsData; Utils::optional versionNumber; quint64 nextFindUsagesKey = 0; quint64 nextFollowSymbolId = 0; quint64 nextSwitchDeclDefId = 0; + quint64 nextLocalRefsId = 0; bool isFullyIndexed = false; bool isTesting = false; }; @@ -694,9 +722,8 @@ void ClangdClient::closeExtraFile(const Utils::FilePath &filePath) void ClangdClient::findUsages(TextEditor::TextDocument *document, const QTextCursor &cursor, const Utils::optional &replacement) { - QTextCursor termCursor(cursor); - termCursor.select(QTextCursor::WordUnderCursor); - const QString searchTerm = termCursor.selectedText(); // TODO: This will be wrong for e.g. operators. Use a Symbol info request to get the real symbol string. + // TODO: This will be wrong for e.g. operators. Use a Symbol info request to get the real symbol string. + const QString searchTerm = d->searchTermFromCursor(cursor); if (searchTerm.isEmpty()) return; @@ -1054,6 +1081,94 @@ void ClangdClient::switchDeclDef(TextEditor::TextDocument *document, const QText } +void ClangdClient::findLocalUsages(TextEditor::TextDocument *document, const QTextCursor &cursor, + CppTools::RefactoringEngineInterface::RenameCallback &&callback) +{ + QTC_ASSERT(documentOpen(document), openDocument(document)); + + qCDebug(clangdLog) << "local references requested" << document->filePath() + << (cursor.blockNumber() + 1) << (cursor.positionInBlock() + 1); + + d->localRefsData.emplace(++d->nextLocalRefsId, document, cursor, std::move(callback)); + const QString searchTerm = d->searchTermFromCursor(cursor); + if (searchTerm.isEmpty()) { + d->localRefsData.reset(); + return; + } + + // Step 1: Go to definition + const auto gotoDefCallback = [this, id = d->localRefsData->id](const Utils::Link &link) { + qCDebug(clangdLog) << "received go to definition response" << link.targetFilePath + << link.targetLine << (link.targetColumn + 1); + if (!d->localRefsData || id != d->localRefsData->id) + return; + if (!link.hasValidTarget()) { + d->localRefsData.reset(); + return; + } + + // Step 2: Get AST and check whether it's a local variable. + AstRequest astRequest(AstParams(TextDocumentIdentifier(d->localRefsData->uri))); + astRequest.setResponseCallback([this, link, id](const AstRequest::Response &response) { + qCDebug(clangdLog) << "received ast response"; + if (!d->localRefsData || id != d->localRefsData->id) + return; + const auto result = response.result(); + if (!result || !d->localRefsData->document) { + d->localRefsData.reset(); + return; + } + + const Position linkPos(link.targetLine - 1, link.targetColumn); + const QList astPath = getAstPath(*result, Range(linkPos, linkPos)); + bool isVar = false; + for (auto it = astPath.rbegin(); it != astPath.rend(); ++it) { + if (it->role() == "declaration" && it->kind() == "Function") { + if (!isVar) + break; + + // Step 3: Find references. + qCDebug(clangdLog) << "finding references for local var"; + symbolSupport().findUsages(d->localRefsData->document, + d->localRefsData->cursor, + [this, id](const QList &locations) { + qCDebug(clangdLog) << "found" << locations.size() << "local references"; + if (!d->localRefsData || id != d->localRefsData->id) + return; + ClangBackEnd::SourceLocationsContainer container; + for (const Location &loc : locations) { + container.insertSourceLocation({}, loc.range().start().line() + 1, + loc.range().start().character() + 1); + } + + // The callback only uses the symbol length, so we just create a dummy. + // Note that the calculation will be wrong for identifiers with + // embedded newlines, but we've never supported that. + QString symbol; + if (!locations.isEmpty()) { + const Range r = locations.first().range(); + symbol = QString(r.end().character() - r.start().character(), 'x'); + } + d->localRefsData->callback(symbol, container, d->localRefsData->revision); + d->localRefsData->callback = {}; + d->localRefsData.reset(); + }); + return; + } + if (!isVar && it->role() == "declaration" + && (it->kind() == "Var" || it->kind() == "ParmVar")) { + isVar = true; + } + } + d->localRefsData.reset(); + }); + qCDebug(clangdLog) << "sending ast request for link"; + sendContent(astRequest); + + }; + symbolSupport().findLinkAt(document, cursor, std::move(gotoDefCallback), true); +} + void ClangdClient::Private::handleGotoDefinitionResult() { QTC_ASSERT(followSymbolData->defLink.hasValidTarget(), return); @@ -1285,6 +1400,13 @@ void ClangdClient::Private::handleDeclDefSwitchReplies() switchDeclDefData.reset(); } +QString ClangdClient::Private::searchTermFromCursor(const QTextCursor &cursor) const +{ + QTextCursor termCursor(cursor); + termCursor.select(QTextCursor::WordUnderCursor); + return termCursor.selectedText(); +} + void ClangdClient::VirtualFunctionAssistProcessor::cancel() { resetData(); diff --git a/src/plugins/clangcodemodel/clangdclient.h b/src/plugins/clangcodemodel/clangdclient.h index be1fb6ba120..b07de7e95ce 100644 --- a/src/plugins/clangcodemodel/clangdclient.h +++ b/src/plugins/clangcodemodel/clangdclient.h @@ -25,6 +25,7 @@ #pragma once +#include #include #include #include @@ -66,6 +67,9 @@ public: CppTools::CppEditorWidgetInterface *editorWidget, Utils::ProcessLinkCallback &&callback); + void findLocalUsages(TextEditor::TextDocument *document, const QTextCursor &cursor, + CppTools::RefactoringEngineInterface::RenameCallback &&callback); + void enableTesting(); signals: diff --git a/src/plugins/clangcodemodel/clangrefactoringengine.cpp b/src/plugins/clangcodemodel/clangrefactoringengine.cpp index 779a26fbb32..662a0894e13 100644 --- a/src/plugins/clangcodemodel/clangrefactoringengine.cpp +++ b/src/plugins/clangcodemodel/clangrefactoringengine.cpp @@ -42,6 +42,14 @@ void RefactoringEngine::startLocalRenaming(const CppTools::CursorInEditor &data, CppTools::ProjectPart *, RenameCallback &&renameSymbolsCallback) { + ClangdClient * const client + = ClangModelManagerSupport::instance()->clientForFile(data.filePath()); + if (client && client->reachable()) { + client->findLocalUsages(data.textDocument(), data.cursor(), + std::move(renameSymbolsCallback)); + return; + } + ClangEditorDocumentProcessor *processor = ClangEditorDocumentProcessor::get( data.filePath().toString()); const int startRevision = data.cursor().document()->revision(); diff --git a/src/plugins/clangcodemodel/test/clangdtests.cpp b/src/plugins/clangcodemodel/test/clangdtests.cpp index 3e2bf4d4fb2..d27c5be03a1 100644 --- a/src/plugins/clangcodemodel/test/clangdtests.cpp +++ b/src/plugins/clangcodemodel/test/clangdtests.cpp @@ -30,6 +30,7 @@ #include "../clangdclient.h" #include "../clangmodelmanagersupport.h" +#include #include #include #include @@ -47,6 +48,8 @@ #include #include +#include + using namespace CPlusPlus; using namespace Core; using namespace CppTools::Tests; @@ -119,8 +122,10 @@ void ClangdTest::initTestCase() QSKIP("clangd is too old"); // Wait for index to build. - if (!m_client->isFullyIndexed()) - QVERIFY(waitForSignalOrTimeout(m_client, &ClangdClient::indexingFinished, timeOutInMs())); + if (!m_client->isFullyIndexed()) { + QVERIFY(waitForSignalOrTimeout(m_client, &ClangdClient::indexingFinished, + clangdIndexingTimeout())); + } QVERIFY(m_client->isFullyIndexed()); // Open cpp documents. @@ -359,6 +364,127 @@ void ClangdTestFollowSymbol::test() QCOMPARE(actualLink.targetColumn + 1, targetColumn); } + +ClangdTestLocalReferences::ClangdTestLocalReferences() +{ + setProjectFileName("local-references.pro"); + setSourceFileNames({"references.cpp"}); + setMinimumVersion(13); +} + +using Range = std::tuple; + +// We currently only support local variables, but if and when clangd implements +// the linkedEditingRange request, we can change the expected values for +// the file-scope test cases from empty ranges to the actual locations. +void ClangdTestLocalReferences::test_data() +{ + QTest::addColumn("sourceLine"); + QTest::addColumn("sourceColumn"); + QTest::addColumn>("expectedRanges"); + + QTest::newRow("cursor not on identifier") << 3 << 5 << QList(); + QTest::newRow("local variable, one use") << 3 << 9 << QList{{3, 9, 3}}; + QTest::newRow("local variable, two uses") << 10 << 9 + << QList{{10, 9, 3}, {11, 12, 3}}; + QTest::newRow("class name") << 16 << 7 << QList() + /* QList{{16, 7, 3}, {19, 5, 3}} */; + QTest::newRow("namespace") << 24 << 11 << QList() + /* QList{{24, 11, 1}, {25, 11, 1}, {26, 1, 1}} */; + QTest::newRow("class name via using") << 30 << 21 << QList() + /* QList{{30, 21, 3}, {31, 10, 3}} */; + QTest::newRow("forward-declared class") << 35 << 7 << QList() + /* QList{{35, 7, 3}, {36, 14, 3}} */; + QTest::newRow("class name and new expression") << 40 << 7 << QList() + /* QList{{40, 7, 3}, {43, 9, 3}} */; + QTest::newRow("instantiated template object") << 52 << 19 + << QList{{52, 19, 3}, {53, 5, 3}}; + QTest::newRow("variable in template") << 62 << 13 << QList() + /* QList{{62, 13, 3}, {63, 11, 3}} */; + QTest::newRow("member in template") << 67 << 7 << QList() + /* QList{{64, 16, 3}, {67, 7, 3}} */; + QTest::newRow("template type") << 58 << 19 << QList() + /* QList{{58, 19, 1}, {60, 5, 1}, {67, 5, 1}} */; + QTest::newRow("template parameter member access") << 76 << 9 << QList(); + QTest::newRow("constructor as type") << 82 << 5 << QList() + /* QList{{81, 8, 3}, {82, 5, 3}, {83, 6, 3}} */; + QTest::newRow("freestanding overloads") << 88 << 5 << QList() + /* QList{{88, 5, 3}, {89, 5, 3}} */; + QTest::newRow("member function overloads") << 94 << 9 << QList() + /* QList{{94, 9, 3}, {95, 9, 3}} */; + QTest::newRow("function and function template") << 100 << 26 << QList() + /* QList{{100, 26, 3}, {101, 5, 3}} */; + QTest::newRow("function and function template as member") << 106 << 30 << QList() + /* QList{{106, 30, 3}, {107, 9, 3}} */; + QTest::newRow("enum type") << 112 << 6 << QList() + /* QList{{112, 6, 2}, {113, 8, 2}} */; + QTest::newRow("captured lambda var") << 122 << 15 + << QList{{122, 15, 3}, {122, 33, 3}}; + QTest::newRow("lambda initializer") << 122 << 19 + << QList{{121, 19, 3}, {122, 19, 3}}; + QTest::newRow("template specialization") << 127 << 25 << QList() + /* QList{{127, 5, 3}, {128, 25, 3}, {129, 18, 3}} */; + QTest::newRow("dependent name") << 133 << 34 << QList() + /* QList{{133, 34, 3} */; + QTest::newRow("function call and definition") << 140 << 5 << QList() + /* QList{{140, 5, 3}, {142, 25, 3}} */; + QTest::newRow("object-like macro") << 147 << 9 << QList() + /* QList{{147, 9, 3}, {150, 12, 3}} */; + QTest::newRow("function-like macro") << 155 << 9 << QList() + /* QList{{155, 9, 3}, {158, 12, 3}} */; + QTest::newRow("argument to function-like macro") << 156 << 27 + << QList{{156, 27, 3}, {158, 16, 3}}; + QTest::newRow("overloaded bracket operator argument") << 172 << 7 + << QList{{171, 7, 1}, {172, 7, 1}, {172, 12, 1}, + {173, 7, 1}, {173, 10, 1}}; + QTest::newRow("overloaded call operator second argument") << 173 << 10 + << QList{{171, 7, 1}, {172, 7, 1}, {172, 12, 1}, + {173, 7, 1}, {173, 10, 1}}; + QTest::newRow("overloaded operators arguments from outside") << 171 << 7 + << QList{{171, 7, 1}, {172, 7, 1}, {172, 12, 1}, + {173, 7, 1}, {173, 10, 1}}; +} + +void ClangdTestLocalReferences::test() +{ + QFETCH(int, sourceLine); + QFETCH(int, sourceColumn); + QFETCH(QList, expectedRanges); + + TextEditor::TextDocument * const doc = document("references.cpp"); + QVERIFY(doc); + + QTimer timer; + timer.setSingleShot(true); + QEventLoop loop; + QObject::connect(&timer, &QTimer::timeout, &loop, &QEventLoop::quit); + QList actualRanges; + const auto handler = [&actualRanges, &loop](const QString &symbol, + const ClangBackEnd::SourceLocationsContainer &container, int) { + for (const ClangBackEnd::SourceLocationContainer &c + : container.m_sourceLocationContainers) { + actualRanges << Range(c.line, c.column, symbol.length()); + } + loop.quit(); + }; + + QTextCursor cursor(doc->document()); + const int pos = Utils::Text::positionInText(doc->document(), sourceLine, sourceColumn); + cursor.setPosition(pos); + client()->findLocalUsages(doc, cursor, std::move(handler)); + timer.start(10000); + loop.exec(); + QEXPECT_FAIL("cursor not on identifier", "clangd bug: go to definition does not return", Abort); + QEXPECT_FAIL("template parameter member access", + "clangd bug: go to definition does not return", Abort); + QVERIFY(timer.isActive()); + timer.stop(); + + QCOMPARE(actualRanges, expectedRanges); +} + } // namespace Tests } // namespace Internal } // namespace ClangCodeModel + +Q_DECLARE_METATYPE(ClangCodeModel::Internal::Tests::Range) diff --git a/src/plugins/clangcodemodel/test/clangdtests.h b/src/plugins/clangcodemodel/test/clangdtests.h index 9a730793bda..e794350d7eb 100644 --- a/src/plugins/clangcodemodel/test/clangdtests.h +++ b/src/plugins/clangcodemodel/test/clangdtests.h @@ -104,6 +104,17 @@ private slots: void test(); }; +class ClangdTestLocalReferences : public ClangdTest +{ + Q_OBJECT +public: + ClangdTestLocalReferences(); + +private slots: + void test_data(); + void test(); +}; + } // namespace Tests } // namespace Internal } // namespace ClangCodeModel diff --git a/src/plugins/clangcodemodel/test/data/clangtestdata.qrc b/src/plugins/clangcodemodel/test/data/clangtestdata.qrc index 9d09b7060fd..5357195bc1f 100644 --- a/src/plugins/clangcodemodel/test/data/clangtestdata.qrc +++ b/src/plugins/clangcodemodel/test/data/clangtestdata.qrc @@ -37,5 +37,7 @@ follow-symbol/follow-symbol.pro follow-symbol/header.h follow-symbol/main.cpp + local-references/local-references.pro + local-references/references.cpp diff --git a/src/plugins/clangcodemodel/test/data/local-references/local-references.pro b/src/plugins/clangcodemodel/test/data/local-references/local-references.pro new file mode 100644 index 00000000000..c3b335aad8d --- /dev/null +++ b/src/plugins/clangcodemodel/test/data/local-references/local-references.pro @@ -0,0 +1,5 @@ +TEMPLATE = app + +CONFIG -= qt + +SOURCES = references.cpp diff --git a/src/plugins/clangcodemodel/test/data/local-references/references.cpp b/src/plugins/clangcodemodel/test/data/local-references/references.cpp new file mode 100644 index 00000000000..32ff1b90753 --- /dev/null +++ b/src/plugins/clangcodemodel/test/data/local-references/references.cpp @@ -0,0 +1,174 @@ +void variableSingleReference() +{ + int foo; +} + + + +int variableMultipleReferences() +{ + int foo = 0; + return foo; +} + + + +class Foo {}; +void bla() +{ + Foo foo; +} + + + +namespace N { class Bar {}; } +namespace N { class Baz {}; } +N::Bar bar; + + + +namespace G { class App {}; } +using G::App; + + + +class Hoo; +void f(const Hoo &); + + + +class Moo {}; +void x() +{ + new Moo; +} + + + +class Element {}; +template struct Wrap { T member; }; +void g() +{ + Wrap con; + con.member; +} + + + +template +struct Wrapper { + T f() + { + int foo; + ++foo; + return mem; + } + + T mem; +}; + + + +template +void f() +{ + T mem; + mem.foo(); +} + + + +struct Woo { + Woo(); + ~Woo(); +}; + + + +int muu(); +int muu(int); + + + +struct Doo { + int muu(); + int muu(int); +}; + + + +template int tuu(); +int tuu(int); + + + +struct Xoo { + template int tuu(); + int tuu(int); +}; + + + +enum ET { E1 }; +bool e(ET e) +{ + return e == E1; +} + + + +struct LData { int member; }; +void lambda(LData foo) { + auto l = [bar=foo] { return bar.member; }; +} + + + +template class Coo; +template class Coo; +template<> class Coo; + + + +template typename T::foo n() +{ + typename T::bla hello; +} + + + +int rec(int n = 100) +{ + return n == 0 ? 0 : rec(--n); +} + + + +#define FOO 3 +int objectLikeMacro() +{ + return FOO; +} + + + +#define BAR(x) x +int functionLikeMacro(int foo) +{ + return BAR(foo); +} + +template +class Container +{ +public: + T &operator[](int); T &operator()(int, int); +}; + +int testOperator() { + Container vec; + + int n = 10; + vec[n] = n * 100; + vec(n, n) = 100; +} diff --git a/src/plugins/cppeditor/cppeditorwidget.cpp b/src/plugins/cppeditor/cppeditorwidget.cpp index 7083707b6d7..ec232f63677 100644 --- a/src/plugins/cppeditor/cppeditorwidget.cpp +++ b/src/plugins/cppeditor/cppeditorwidget.cpp @@ -691,7 +691,7 @@ void CppEditorWidget::renameSymbolUnderCursor() viewport()->setCursor(Qt::BusyCursor); d->m_modelManager->startLocalRenaming(CppTools::CursorInEditor{textCursor(), textDocument()->filePath(), - this}, + this, textDocument()}, projPart, std::move(renameSymbols)); }