From 224cebe0d96af0f3f4de27aff03918fc319687fa Mon Sep 17 00:00:00 2001 From: Maximilian Goldstein Date: Thu, 10 Dec 2020 15:58:36 +0100 Subject: qmlformat: Remove import sorting Remove import sorting due to the fact that sorting imports can break code. [ChangeLog][QML Tooling][qmlformat] Remove import sorting and the (now obsolete) -n parameter to disable it Fixes: QTBUG-89295 Change-Id: I5ff13d0ae3c715db7645b412152aadb31811ce5a Reviewed-by: Ulf Hermann (cherry picked from commit cf3e0559b01b249a6ec06f8826e8e05aca1301ec) --- .../data/Annotations.formatted.nosort.qml | 106 --------------- .../qml/qmlformat/data/Annotations.formatted.qml | 4 +- .../qmlformat/data/Example1.formatted.nosort.qml | 151 --------------------- .../auto/qml/qmlformat/data/Example1.formatted.qml | 12 +- tests/auto/qml/qmlformat/tst_qmlformat.cpp | 63 ++++----- tools/qmlformat/main.cpp | 19 +-- tools/qmlformat/restructureastvisitor.cpp | 39 +----- tools/qmlformat/restructureastvisitor.h | 5 +- 8 files changed, 45 insertions(+), 354 deletions(-) delete mode 100644 tests/auto/qml/qmlformat/data/Annotations.formatted.nosort.qml delete mode 100644 tests/auto/qml/qmlformat/data/Example1.formatted.nosort.qml diff --git a/tests/auto/qml/qmlformat/data/Annotations.formatted.nosort.qml b/tests/auto/qml/qmlformat/data/Annotations.formatted.nosort.qml deleted file mode 100644 index a05c2125dc..0000000000 --- a/tests/auto/qml/qmlformat/data/Annotations.formatted.nosort.qml +++ /dev/null @@ -1,106 +0,0 @@ -/**************************************************************************** -** -** Copyright (C) 2016 The Qt Company Ltd. -** Contact: https://www.qt.io/licensing/ -** -** This file is part of the Qt Charts module of the Qt Toolkit. -** -** $QT_BEGIN_LICENSE:GPL$ -** 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 or (at your option) any later version -** approved by the KDE Free Qt Foundation. The licenses are as published by -** the Free Software Foundation and appearing in the file LICENSE.GPL3 -** 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. -** -** $QT_END_LICENSE$ -** -****************************************************************************/ - -//![2] -import QtQuick 2.0 -//![2] -import QtCharts 2.0 - -@Pippo { - atg1: 3 -} -@Annotation2 { -} -Item { - //![1] - - @AnnotateMore { - property int x: 5 - } - @AnnotateALot { - } - - property variant othersSlice: 0 - @Annotate { - } - - anchors.fill: parent - @SuperComplete { - binding: late - } - Component.onCompleted: { - // You can also manipulate slices dynamically, like append a slice or set a slice exploded - othersSlice = pieSeries.append("Others", 52); - pieSeries.find("Volkswagen").exploded = true; - } - //![1] - ChartView { - id: chart - - title: "Top-5 car brand shares in Finland" - anchors.fill: parent - legend.alignment: Qt.AlignBottom - antialiasing: true - - @ExtraAnnotation { - signal pippo() - } - PieSeries { - id: pieSeries - - PieSlice { - label: "Volkswagen" - value: 13.5 - } - - PieSlice { - label: "Toyota" - value: 10.9 - } - - PieSlice { - label: "Ford" - value: 8.6 - } - - PieSlice { - label: "Skoda" - value: 8.2 - } - - PieSlice { - label: "Volvo" - value: 6.8 - } - - } - - } - -} diff --git a/tests/auto/qml/qmlformat/data/Annotations.formatted.qml b/tests/auto/qml/qmlformat/data/Annotations.formatted.qml index a142d4cb74..a05c2125dc 100644 --- a/tests/auto/qml/qmlformat/data/Annotations.formatted.qml +++ b/tests/auto/qml/qmlformat/data/Annotations.formatted.qml @@ -27,10 +27,10 @@ ** ****************************************************************************/ -//![2] -import QtCharts 2.0 //![2] import QtQuick 2.0 +//![2] +import QtCharts 2.0 @Pippo { atg1: 3 diff --git a/tests/auto/qml/qmlformat/data/Example1.formatted.nosort.qml b/tests/auto/qml/qmlformat/data/Example1.formatted.nosort.qml deleted file mode 100644 index fdc0b1f68c..0000000000 --- a/tests/auto/qml/qmlformat/data/Example1.formatted.nosort.qml +++ /dev/null @@ -1,151 +0,0 @@ -/* This file is licensed under the not a license license - 1. You may not comply - 2. Goodbye -*/ - -// Importing this is very important -import QtQuick 5.15 -// Muddling the waters! -import QtQuick.Models 3.14 as muddle -// Importing that is important too -import Z -import That -import This // THIS IS VERY IMPORTANT! -import Y -import X.Z -import X.Y -import A.LLOHA -import A.B.B.A - -// This comment is related to Item -Item { - // Orphan comment - // Another orphan - // More orphans - - // This to id - // Also id. (line 2) - // This is the third id - // fourth id comment - id: foo - - // This to enum - enum Foo { - A = 3, // This is A - B, // This is B - C = 4, // This is C - D // This is D - } - - property bool some_bool: false - property variant some_array_literal: [30, 20, Math["PI"], [4, 3, 2], "foo", 0.3] - property bool something_computed: function(x) { - // This is an orphan inside something_computed - // Are these getting duplicated? - // Another orphan inside something_computed - - const PI = 3, DAYS_PER_YEAR = 365.25; - var x = 3 + 2; - x["bla"] = 50; - // This one to var few! - var few = new WhatEver(); - x += Math.sin(3); - x--; - --x; - x++; - ++x; - for (var x = 0; x < 100; x++) { - x++; - console.log("Foo"); - } - for (var x in [3, 2, 1]) { - y++; - console.log("Bar"); - } - while (true) - console.log("Wee"); - - with (foo) { - bar; - x += 5; - } // This is related to with! - x3: - do { - console.log("Hello"); - } while (3 == 0); - try { - dangerous(); - } catch (e) { - console.log(e); - } finally { - dangerous(); - } - switch (x) { - case 0: - x = 1; - break; - case 1: - x = 5; - break; - case 4: - x = 100; - break; - } - if (x == 50) - console.log("true"); - else if (x == 50) - console.log("other thing"); - else - console.log("false"); - if (x == 50) { - console.log("true"); - } else if (x == 50) { - console.log("other thing"); - x--; - } else { - console.log("false"); - } - return "foobar"; - }() - default property bool some_default_bool: 500 % 5 !== 0 // some_default_bool - // some_read_only_bool - readonly property bool some_read_only_bool: Math.sin(3) && (aFunc()[30] + 5) | 2 != 0 - - signal say(string name, bool caps) - - // This one to aFunc() - function aFunc() { - var x = 3; - return x; - } - - x: 3 // Very cool - Component.onCompleted: console.log("Foo!") - myFavouriteThings: [ - // This is an orphan - - // This is a cool text - Text { - }, - // This is a cool rectangle - Rectangle { - } - ] - - Text { - required property string batman - - signal boo(int count, int times, real duration) - - text: "Bla" - } - - // This comment is related to the property animation - PropertyAnimation on x { - id: foo - - x: 3 - y: x + 3 - } - -} diff --git a/tests/auto/qml/qmlformat/data/Example1.formatted.qml b/tests/auto/qml/qmlformat/data/Example1.formatted.qml index 5f12517781..fdc0b1f68c 100644 --- a/tests/auto/qml/qmlformat/data/Example1.formatted.qml +++ b/tests/auto/qml/qmlformat/data/Example1.formatted.qml @@ -3,19 +3,19 @@ 2. Goodbye */ -import A.B.B.A -import A.LLOHA // Importing this is very important import QtQuick 5.15 // Muddling the waters! import QtQuick.Models 3.14 as muddle +// Importing that is important too +import Z import That import This // THIS IS VERY IMPORTANT! -import X.Y -import X.Z import Y -// Importing that is important too -import Z +import X.Z +import X.Y +import A.LLOHA +import A.B.B.A // This comment is related to Item Item { diff --git a/tests/auto/qml/qmlformat/tst_qmlformat.cpp b/tests/auto/qml/qmlformat/tst_qmlformat.cpp index 9e91ff8569..cb3c39aaff 100644 --- a/tests/auto/qml/qmlformat/tst_qmlformat.cpp +++ b/tests/auto/qml/qmlformat/tst_qmlformat.cpp @@ -53,7 +53,7 @@ private Q_SLOTS: private: QString readTestFile(const QString &path); - QString runQmlformat(const QString &fileToFormat, bool sortImports, bool shouldSucceed, const QString &newlineFormat = "native"); + QString runQmlformat(const QString &fileToFormat, bool shouldSucceed, const QString &newlineFormat = "native"); QString m_qmlformatPath; QStringList m_excludedDirs; @@ -179,16 +179,16 @@ QString TestQmlformat::readTestFile(const QString &path) void TestQmlformat::testLineEndings() { // macos - const QString macosContents = runQmlformat(testFile("Example1.formatted.qml"), false, true, "macos"); + const QString macosContents = runQmlformat(testFile("Example1.formatted.qml"), true, "macos"); QVERIFY(!macosContents.contains("\n")); QVERIFY(macosContents.contains("\r")); // windows - const QString windowsContents = runQmlformat(testFile("Example1.formatted.qml"), false, true, "windows"); + const QString windowsContents = runQmlformat(testFile("Example1.formatted.qml"), true, "windows"); QVERIFY(windowsContents.contains("\r\n")); // unix - const QString unixContents = runQmlformat(testFile("Example1.formatted.qml"), false, true, "unix"); + const QString unixContents = runQmlformat(testFile("Example1.formatted.qml"), true, "unix"); QVERIFY(unixContents.contains("\n")); QVERIFY(!unixContents.contains("\r")); } @@ -197,58 +197,54 @@ void TestQmlformat::testFormat_data() { QTest::addColumn("file"); QTest::addColumn("fileFormatted"); - QTest::addColumn("sortImports"); QTest::addColumn("shouldSucceed"); - QTest::newRow("example1 (sorted)") << "Example1.qml" - << "Example1.formatted.qml" << true << true; - QTest::newRow("example1 (not sorted)") << "Example1.qml" - << "Example1.formatted.nosort.qml" << false << true; - QTest::newRow("annotation (sorted)") << "Annotations.qml" - << "Annotations.formatted.qml" << true << true; - QTest::newRow("annotation (not sorted)") << "Annotations.qml" - << "Annotations.formatted.nosort.qml" << false << true; + QTest::newRow("example1") + << "Example1.qml" + << "Example1.formatted.qml" << true; + QTest::newRow("annotation") + << "Annotations.qml" + << "Annotations.formatted.qml" << true; QTest::newRow("front inline") << "FrontInline.qml" - << "FrontInline.formatted.qml" << false << true; + << "FrontInline.formatted.qml" << true; QTest::newRow("if blocks") << "IfBlocks.qml" - << "IfBlocks.formatted.qml" << false << true; + << "IfBlocks.formatted.qml" << true; QTest::newRow("read-only properties") << "readOnlyProps.qml" - << "readOnlyProps.formatted.qml" << false << true; + << "readOnlyProps.formatted.qml" << true; QTest::newRow("states and transitions") << "statesAndTransitions.qml" - << "statesAndTransitions.formatted.qml" << false << true; + << "statesAndTransitions.formatted.qml" << true; QTest::newRow("large bindings") << "largeBindings.qml" - << "largeBindings.formatted.qml" << false << true; + << "largeBindings.formatted.qml" << true; QTest::newRow("verbatim strings") << "verbatimString.qml" - << "verbatimString.formatted.qml" << false << true; + << "verbatimString.formatted.qml" << true; QTest::newRow("inline components") << "inlineComponents.qml" - << "inlineComponents.formatted.qml" << false << true; + << "inlineComponents.formatted.qml" << true; QTest::newRow("nested ifs") << "nestedIf.qml" - << "nestedIf.formatted.qml" << false << true; + << "nestedIf.formatted.qml" << true; QTest::newRow("QTBUG-85003") << "QtBug85003.qml" - << "QtBug85003.formatted.qml" << false << true; + << "QtBug85003.formatted.qml" << true; QTest::newRow("nested functions") << "nestedFunctions.qml" - << "nestedFunctions.formatted.qml" << false << true; + << "nestedFunctions.formatted.qml" << true; QTest::newRow("multiline comments") << "multilineComment.qml" - << "multilineComment.formatted.qml" << false << true; + << "multilineComment.formatted.qml" << true; QTest::newRow("for of") << "forOf.qml" - << "forOf.formatted.qml" << false << true; + << "forOf.formatted.qml" << true; QTest::newRow("property names") << "propertyNames.qml" - << "propertyNames.formatted.qml" << false << true; + << "propertyNames.formatted.qml" << true; QTest::newRow("empty object") << "emptyObject.qml" - << "emptyObject.formatted.qml" << false << true; + << "emptyObject.formatted.qml" << true; QTest::newRow("arrow functions") << "arrowFunctions.qml" - << "arrowFunctions.formatted.qml" << false << true; + << "arrowFunctions.formatted.qml" << true; } void TestQmlformat::testFormat() { QFETCH(QString, file); QFETCH(QString, fileFormatted); - QFETCH(bool, sortImports); QFETCH(bool, shouldSucceed); - QCOMPARE(runQmlformat(testFile(file), sortImports, shouldSucceed), readTestFile(fileFormatted)); + QCOMPARE(runQmlformat(testFile(file), shouldSucceed), readTestFile(fileFormatted)); } #if !defined(QTEST_CROSS_COMPILED) // sources not available when cross compiled @@ -273,14 +269,14 @@ void TestQmlformat::testExample() { QFETCH(QString, file); const bool isInvalid = isInvalidFile(QFileInfo(file)); - QString output = runQmlformat(file, true, !isInvalid); + QString output = runQmlformat(file, !isInvalid); if (!isInvalid) QVERIFY(!output.isEmpty()); } #endif -QString TestQmlformat::runQmlformat(const QString &fileToFormat, bool sortImports, bool shouldSucceed, const QString &newlineFormat) +QString TestQmlformat::runQmlformat(const QString &fileToFormat, bool shouldSucceed, const QString &newlineFormat) { // Copy test file to temporary location QTemporaryDir tempDir; @@ -291,9 +287,6 @@ QString TestQmlformat::runQmlformat(const QString &fileToFormat, bool sortImport args << "-i"; args << tempFile; - if (!sortImports) - args << "-n"; - args << "-l" << newlineFormat; auto verify = [&]() { diff --git a/tools/qmlformat/main.cpp b/tools/qmlformat/main.cpp index f9afd10a2c..1461fea357 100644 --- a/tools/qmlformat/main.cpp +++ b/tools/qmlformat/main.cpp @@ -44,7 +44,8 @@ #include "dumpastvisitor.h" #include "restructureastvisitor.h" -bool parseFile(const QString& filename, bool inplace, bool verbose, bool sortImports, bool force, const QString& newline) +bool parseFile(const QString &filename, bool inplace, bool verbose, bool force, + const QString &newline) { QFile file(filename); @@ -90,11 +91,8 @@ bool parseFile(const QString& filename, bool inplace, bool verbose, bool sortImp qWarning().noquote() << orphaned << "comments are orphans."; } - if (verbose && sortImports) - qWarning().noquote() << "Sorting imports"; - // Do the actual restructuring - RestructureAstVisitor restructure(parser.rootNode(), sortImports); + RestructureAstVisitor restructure(parser.rootNode()); // Turn AST back into source code if (verbose) @@ -182,9 +180,6 @@ int main(int argc, char *argv[]) parser.addOption(QCommandLineOption({"V", "verbose"}, QStringLiteral("Verbose mode. Outputs more detailed information."))); - parser.addOption(QCommandLineOption({"n", "no-sort"}, - QStringLiteral("Do not sort imports."))); - parser.addOption(QCommandLineOption({"i", "inplace"}, QStringLiteral("Edit file in-place instead of outputting to stdout."))); @@ -225,15 +220,15 @@ int main(int argc, char *argv[]) if (file.isEmpty()) continue; - if (!parseFile(file, true, parser.isSet("verbose"), !parser.isSet("no-sort"), - parser.isSet("force"), parser.value("newline"))) + if (!parseFile(file, true, parser.isSet("verbose"), + parser.isSet("force"), + parser.value("newline"))) success = false; } } else { for (const QString &file : parser.positionalArguments()) { if (!parseFile(file, parser.isSet("inplace"), parser.isSet("verbose"), - !parser.isSet("no-sort"), parser.isSet("force"), - parser.value("newline"))) + parser.isSet("force"), parser.value("newline"))) success = false; } } diff --git a/tools/qmlformat/restructureastvisitor.cpp b/tools/qmlformat/restructureastvisitor.cpp index 7cce0e8034..45957230d8 100644 --- a/tools/qmlformat/restructureastvisitor.cpp +++ b/tools/qmlformat/restructureastvisitor.cpp @@ -30,7 +30,7 @@ #include -RestructureAstVisitor::RestructureAstVisitor(Node *rootNode, bool sortImports) : m_sortImports(sortImports) +RestructureAstVisitor::RestructureAstVisitor(Node *rootNode) { rootNode->accept(this); } @@ -69,43 +69,6 @@ static QString parseUiQualifiedId(UiQualifiedId *id) return name; } -void RestructureAstVisitor::endVisit(UiHeaderItemList *node) -{ - QList correctOrder; - - auto imports = findKind(node); - - if (!m_sortImports) - return; - - // Sort imports - std::sort(imports.begin(), imports.end(), [](UiImport *a, UiImport *b) - { - auto nameA = a->fileName.isEmpty() ? parseUiQualifiedId(a->importUri) - : a->fileName.toString(); - auto nameB = b->fileName.isEmpty() ? parseUiQualifiedId(b->importUri) - : b->fileName.toString(); - - return nameA < nameB; - }); - - // Add imports - for (auto *import : imports) - correctOrder.append(import); - - // Add all the other items - for (auto *item = node; item != nullptr; item = item->next) { - if (!correctOrder.contains(item->headerItem)) - correctOrder.append(item->headerItem); - } - - // Rebuild member list from correctOrder - for (auto *item = node; item != nullptr; item = item->next) { - item->headerItem = correctOrder.front(); - correctOrder.pop_front(); - } -} - void RestructureAstVisitor::endVisit(UiObjectMemberList *node) { QList correctOrder; diff --git a/tools/qmlformat/restructureastvisitor.h b/tools/qmlformat/restructureastvisitor.h index a2195c8c2e..7b3573300f 100644 --- a/tools/qmlformat/restructureastvisitor.h +++ b/tools/qmlformat/restructureastvisitor.h @@ -37,14 +37,11 @@ using namespace QQmlJS::AST; class RestructureAstVisitor : protected Visitor { public: - RestructureAstVisitor(Node *rootNode, bool sortImports); + RestructureAstVisitor(Node *rootNode); void throwRecursionDepthError() override {} void endVisit(UiObjectMemberList *node) override; - void endVisit(UiHeaderItemList *node) override; -private: - bool m_sortImports = false; }; #endif // RESTRUCTUREASTVISITOR_H -- cgit v1.2.3