From 50a7560b784f16e4d8d697ce62bede8aa56d75f6 Mon Sep 17 00:00:00 2001 From: Sami Shalayel Date: Mon, 28 Aug 2023 11:13:18 +0200 Subject: [PATCH] qmlls: show fix suggestions and enable quick fixes Display the fix suggestions from qmllint in qmlls instead of just showing the warning. For example, 'Unqualified access: xxx is accessible in this scope because you are handling a signal at 18:10. Use a function instead.' sounds so much more helpful than the previous 'Unqualified access'. Add an ending '.' in QQmlJSFixSuggestion's messages where missing. Enable quick-fixes in VS Code and QtC by labeling quickfixes as quickfixes. Everything that is not labelled a quickfix might need a special language client implementation to work. Also, add a test for quickfixes: it seems someone broke the quick fix functionality in qmlls by mistake by relabeling a quickfix to 'refactor.rewrite'. Task-number: QTBUG-115213 Change-Id: I350e23901b97d16a60bb39fdb4ab566b0d7fbdfb Reviewed-by: Semih Yavuz Reviewed-by: Qt CI Bot Reviewed-by: Fabian Kosmale Reviewed-by: Ulf Hermann --- src/qmlcompiler/qqmljsimportvisitor.cpp | 31 ++--- src/qmlcompiler/qqmljstypepropagator.cpp | 10 +- src/qmlls/qqmllintsuggestions.cpp | 14 +- .../data/quickfixes/INeedAQuickFix.qml | 19 +++ .../auto/qmlls/modules/tst_qmlls_modules.cpp | 126 ++++++++++++++++++ tests/auto/qmlls/modules/tst_qmlls_modules.h | 2 + tests/auto/qmlls/qmlls/tst_qmlls.cpp | 3 +- 7 files changed, 179 insertions(+), 26 deletions(-) create mode 100644 tests/auto/qmlls/modules/data/quickfixes/INeedAQuickFix.qml diff --git a/src/qmlcompiler/qqmljsimportvisitor.cpp b/src/qmlcompiler/qqmljsimportvisitor.cpp index 181607d42e..400d2e5c57 100644 --- a/src/qmlcompiler/qqmljsimportvisitor.cpp +++ b/src/qmlcompiler/qqmljsimportvisitor.cpp @@ -872,12 +872,12 @@ void QQmlJSImportVisitor::checkRequiredProperties() if (requiredScope != scope) { if (!prevRequiredScope.isNull()) { auto sourceScope = prevRequiredScope->baseType(); - suggestion = QQmlJSFixSuggestion { - "%1:%2:%3: Property marked as required in %4"_L1 - .arg(sourceScope->filePath()) - .arg(sourceScope->sourceLocation().startLine) - .arg(sourceScope->sourceLocation().startColumn) - .arg(requiredScopeName), + suggestion = QQmlJSFixSuggestion{ + "%1:%2:%3: Property marked as required in %4."_L1 + .arg(sourceScope->filePath()) + .arg(sourceScope->sourceLocation().startLine) + .arg(sourceScope->sourceLocation().startColumn) + .arg(requiredScopeName), sourceScope->sourceLocation() }; suggestion->setFilename(sourceScope->filePath()); @@ -1003,12 +1003,12 @@ void QQmlJSImportVisitor::checkSignal( const qsizetype newLength = m_logger->code().indexOf(u'\n', location.end()) - location.offset; - fix = QQmlJSFixSuggestion { - "Implicitly defining %1 as signal handler in Connections is deprecated. " - "Create a function instead"_L1.arg(handlerName), - QQmlJS::SourceLocation(location.offset, newLength, location.startLine, - location.startColumn), - "function %1(%2) { ... }"_L1.arg(handlerName, handlerParameters.join(u", ")) + fix = QQmlJSFixSuggestion{ + "Implicitly defining %1 as signal handler in Connections is deprecated. " + "Create a function instead."_L1.arg(handlerName), + QQmlJS::SourceLocation(location.offset, newLength, location.startLine, + location.startColumn), + "function %1(%2) { ... }"_L1.arg(handlerName, handlerParameters.join(u", ")) }; } @@ -1411,11 +1411,8 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::StringLiteral *sl) templateString += c; } - QQmlJSFixSuggestion suggestion = { - "Use a template literal instead"_L1, - sl->literalToken, - u"`" % templateString % u"`" - }; + QQmlJSFixSuggestion suggestion = { "Use a template literal instead."_L1, sl->literalToken, + u"`" % templateString % u"`" }; suggestion.setAutoApplicable(); m_logger->log(QStringLiteral("String contains unescaped line terminator which is " "deprecated."), diff --git a/src/qmlcompiler/qqmljstypepropagator.cpp b/src/qmlcompiler/qqmljstypepropagator.cpp index 5eb8c875fd..4bddaf470e 100644 --- a/src/qmlcompiler/qqmljstypepropagator.cpp +++ b/src/qmlcompiler/qqmljstypepropagator.cpp @@ -381,11 +381,11 @@ void QQmlJSTypePropagator::handleUnqualifiedAccess(const QString &name, bool isM QQmlJS::SourceLocation fixLocation = location; fixLocation.length = 0; - suggestion = QQmlJSFixSuggestion { - name + " is a member of a parent element.\n You can qualify the access " - "with its id to avoid this warning:\n"_L1, - fixLocation, - (id.isEmpty() ? u"."_s : (id + u'.')) + suggestion = QQmlJSFixSuggestion{ + name + + " is a member of a parent element.\n You can qualify the access " + "with its id to avoid this warning.\n"_L1, + fixLocation, (id.isEmpty() ? u"."_s : (id + u'.')) }; if (id.isEmpty()) diff --git a/src/qmlls/qqmllintsuggestions.cpp b/src/qmlls/qqmllintsuggestions.cpp index 0084e9d2e4..dda62e65a6 100644 --- a/src/qmlls/qqmllintsuggestions.cpp +++ b/src/qmlls/qqmllintsuggestions.cpp @@ -80,7 +80,8 @@ static void codeActionHandler( edit.documentChanges = edits; CodeAction action; - action.kind = u"refactor.rewrite"_s.toUtf8(); + // VS Code and QtC ignore everything that is not a 'quickfix'. + action.kind = u"quickfix"_s.toUtf8(); action.edit = edit; action.title = message.toUtf8(); @@ -165,7 +166,16 @@ static Diagnostic messageToDiagnostic_helper(AdvanceFunc advancePositionPastLoca position); } range.end = position; - diagnostic.message = message.message.toUtf8(); + if (message.fixSuggestion && !message.fixSuggestion->fixDescription().isEmpty()) { + diagnostic.message = QString(message.message) + .append(u": "_s) + .append(message.fixSuggestion->fixDescription()) + .simplified() + .toUtf8(); + } else { + diagnostic.message = message.message.toUtf8(); + } + diagnostic.source = QByteArray("qmllint"); auto suggestion = message.fixSuggestion; diff --git a/tests/auto/qmlls/modules/data/quickfixes/INeedAQuickFix.qml b/tests/auto/qmlls/modules/data/quickfixes/INeedAQuickFix.qml new file mode 100644 index 0000000000..ccd2a55383 --- /dev/null +++ b/tests/auto/qmlls/modules/data/quickfixes/INeedAQuickFix.qml @@ -0,0 +1,19 @@ +import QtQuick + +Item { + id: hello + + signal s(xxx: string) + property int i: 42 + + // fix me! add '(xxx) =>' in front of console.log() + onS: console.log(xxx) + + Item { + // fix me! prepend 'hello.' to 'i'! + function f() { + return i + } + } + +} diff --git a/tests/auto/qmlls/modules/tst_qmlls_modules.cpp b/tests/auto/qmlls/modules/tst_qmlls_modules.cpp index 864496879e..88b03b9e7f 100644 --- a/tests/auto/qmlls/modules/tst_qmlls_modules.cpp +++ b/tests/auto/qmlls/modules/tst_qmlls_modules.cpp @@ -3,9 +3,11 @@ #include "tst_qmlls_modules.h" #include "QtQmlLS/private/qqmllsutils_p.h" +#include #include #include #include +#include #include #include @@ -1397,6 +1399,130 @@ void tst_qmlls_modules::qmldirImportsFromSource() QTRY_VERIFY_WITH_TIMEOUT(diagnosticOk, 5000); } +void tst_qmlls_modules::quickFixes_data() +{ + QTest::addColumn("filePath"); + QTest::addColumn("codeActionParams"); + QTest::addColumn("diagnosticIndex"); + QTest::addColumn("replacementRange"); + QTest::addColumn("replacementText"); + + const QString filePath = u"quickfixes/INeedAQuickFix.qml"_s; + + QString fileContent; + { + QFile file(testFile(filePath)); + QVERIFY(file.open(QFile::Text | QFile::ReadOnly)); + fileContent = file.readAll(); + } + + CodeActionParams firstCodeAction; + firstCodeAction.range = rangeFrom(fileContent, 10, 23, 1); + firstCodeAction.textDocument.uri = testFileUrl(filePath).toEncoded(); + + const Range firstRange = rangeFrom(fileContent, 10, 10, 0); + const QString firstReplacement = u"(xxx) => "_s; + + QTest::addRow("injectedParameters") + << filePath << firstCodeAction << 0 << firstRange << firstReplacement; + + CodeActionParams secondCodeAction; + secondCodeAction.textDocument.uri = testFileUrl(filePath).toEncoded(); + secondCodeAction.range = rangeFrom(fileContent, 15, 20, 1); + + const Range secondRange = rangeFrom(fileContent, 15, 20, 0); + const QString secondReplacement = u"hello."_s; + + QTest::addRow("parentProperty") + << filePath << secondCodeAction << 1 << secondRange << secondReplacement; +} + +std::tuple rangeAsTuple(const Range &range) +{ + return std::make_tuple(range.start.line, range.start.character, range.end.line, + range.end.character); +} + +void tst_qmlls_modules::quickFixes() +{ + QFETCH(QString, filePath); + QFETCH(CodeActionParams, codeActionParams); + // The index of the diagnostic that the quickFix belongs to. + // diagnostics are sorted by their range (= text position in the current file). + QFETCH(int, diagnosticIndex); + QFETCH(Range, replacementRange); + QFETCH(QString, replacementText); + + const auto uri = openFile(filePath); + QVERIFY(uri); + + bool diagnosticOk = false; + QList diagnostics; + + // run first the diagnostic that proposes a quickfix + m_protocol->registerPublishDiagnosticsNotificationHandler( + [&diagnosticOk, &uri, &diagnostics, + &diagnosticIndex](const QByteArray &, const PublishDiagnosticsParams &p) { + if (p.uri != *uri) + return; + + if constexpr (enable_debug_output) { + for (const auto &x : p.diagnostics) { + qDebug() << x.message; + } + } + QCOMPARE_GE(p.diagnostics.size(), diagnosticIndex); + + QList partially_sorted{ p.diagnostics }; + std::nth_element(partially_sorted.begin(), + std::next(partially_sorted.begin(), diagnosticIndex), + partially_sorted.end(), + [](const Diagnostic &a, const Diagnostic &b) { + return rangeAsTuple(a.range) < rangeAsTuple(b.range); + }); + diagnostics.append(partially_sorted[diagnosticIndex]); + + diagnosticOk = true; + }); + + QTRY_VERIFY_WITH_TIMEOUT(diagnosticOk, 5000); + + codeActionParams.context.diagnostics = diagnostics; + + using InnerT = QList>; + using T = std::variant; + + bool codeActionOk = false; + + // request a quickfix with the obtained diagnostic + m_protocol->requestCodeAction(codeActionParams, [&](const T &result) { + QVERIFY(std::holds_alternative(result)); + InnerT inner = std::get(result); + QCOMPARE(inner.size(), 1); + QVERIFY(std::holds_alternative(inner.front())); + CodeAction codeAction = std::get(inner.front()); + + QCOMPARE(codeAction.kind, "quickfix"); // everything else is ignored by QtC, VS Code, ... + + QVERIFY(codeAction.edit); + QVERIFY(codeAction.edit->documentChanges); + QVERIFY(std::holds_alternative>(*codeAction.edit->documentChanges)); + auto edits = std::get>(*codeAction.edit->documentChanges); + QCOMPARE(edits.size(), 1); + QCOMPARE(edits.front().edits.size(), 1); + QVERIFY(std::holds_alternative(edits.front().edits.front())); + auto textEdit = std::get(edits.front().edits.front()); + + // make sure that the quick fix does something + QCOMPARE(textEdit.newText, replacementText); + QCOMPARE(rangeAsTuple(textEdit.range), rangeAsTuple(replacementRange)); + + codeActionOk = true; + }); + + QTRY_VERIFY_WITH_TIMEOUT(codeActionOk, 5000); +} + QTEST_MAIN(tst_qmlls_modules) #include diff --git a/tests/auto/qmlls/modules/tst_qmlls_modules.h b/tests/auto/qmlls/modules/tst_qmlls_modules.h index 4477442c21..9e589d4275 100644 --- a/tests/auto/qmlls/modules/tst_qmlls_modules.h +++ b/tests/auto/qmlls/modules/tst_qmlls_modules.h @@ -62,6 +62,8 @@ private slots: void rangeFormatting(); void qmldirImportsFromBuild(); void qmldirImportsFromSource(); + void quickFixes_data(); + void quickFixes(); private: QProcess m_server; diff --git a/tests/auto/qmlls/qmlls/tst_qmlls.cpp b/tests/auto/qmlls/qmlls/tst_qmlls.cpp index 49d2de0583..8919c2e133 100644 --- a/tests/auto/qmlls/qmlls/tst_qmlls.cpp +++ b/tests/auto/qmlls/qmlls/tst_qmlls.cpp @@ -214,8 +214,7 @@ void tst_Qmlls::didOpenTextDocument() QString title = QString::fromUtf8(action.title); QVERIFY(action.kind.has_value()); - QCOMPARE(QString::fromUtf8(action.kind.value()), - QLatin1StringView("refactor.rewrite")); + QCOMPARE(QString::fromUtf8(action.kind.value()), QLatin1StringView("quickfix")); QVERIFY(action.edit.has_value()); WorkspaceEdit edit = action.edit.value();