Fix disambiguation of file-selected components and scripts

In order to choose the right file-selected components and scripts when
resolving types we need to drop all but the "base" ones from the qmldir.
This has to be done right after parsing the qmldir, but only at run
time. At compile time we need the extra information to determine that we
cannot compile any access to such files to C++.

Pick-to: 6.5 6.6
Fixes: QTBUG-113940
Change-Id: I734b865202b6241d5dd60c134214bdbe09ea10f4
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/qml/qml/qqmlimport.cpp

@@ -987,26 +987,6 @@ QString QQmlImports::resolvedUri(const QString &dir_arg, QQmlImportDatabase *dat
     return stableRelativePath;
 }
 
-/* removes all file selector occurrences in path
-   firstPlus is the position of the initial '+' in the path
-   which we always have as we check for '+' to decide whether
-   we need to do some work at all
-*/
-static QString pathWithoutFileSelectors(QString path, // we want a copy of path
-                                        qsizetype firstPlus)
-{
-    do {
-        Q_ASSERT(path.at(firstPlus) == u'+');
-        const auto eos = path.size();
-        qsizetype terminatingSlashPos = firstPlus + 1;
-        while (terminatingSlashPos != eos && path.at(terminatingSlashPos) != u'/')
-            ++terminatingSlashPos;
-        path.remove(firstPlus, terminatingSlashPos - firstPlus + 1);
-        firstPlus = path.indexOf(u'+', firstPlus);
-    } while (firstPlus != -1);
-    return path;
-}
-
 /*!
   \internal
 
@@ -1053,47 +1033,17 @@ QTypeRevision QQmlImports::matchingQmldirVersion(
     typedef QQmlDirComponents::const_iterator ConstIterator;
     const QQmlDirComponents &components = qmldir.components();
 
-    QMultiHash<QString, ConstIterator> baseFileName2ConflictingComponents;
-
     ConstIterator cend = components.constEnd();
     for (ConstIterator cit = components.constBegin(); cit != cend; ++cit) {
         for (ConstIterator cit2 = components.constBegin(); cit2 != cit; ++cit2) {
             if (cit2->typeName == cit->typeName && cit2->version == cit->version) {
-                // ugly heuristic to deal with file selectors
-                const auto comp2PotentialFileSelectorPos = cit2->fileName.indexOf(u'+');
-                const bool comp2MightHaveFileSelector = comp2PotentialFileSelectorPos != -1;
-                /* If we detect conflicting paths, we check if they agree when we remove anything looking like a
-                   file selector.
-                   We need to create copies of the filenames, otherwise QString::replace would modify the
-                   existing file-names
-                 */
-                QString compFileName1 = cit->fileName;
-                QString compFileName2 = cit2->fileName;
-                if (auto fileSelectorPos1 = compFileName1.indexOf(u'+'); fileSelectorPos1 != -1) {
-                    // existing entry was file selector entry, fix it up
-                    // it could also be the case that _both_ are using file selectors
-                    QString baseName =  comp2MightHaveFileSelector ? pathWithoutFileSelectors(compFileName2,
-                                                                                         comp2PotentialFileSelectorPos)
-                                                                   : compFileName2;
-                    if (pathWithoutFileSelectors(compFileName1, fileSelectorPos1) == baseName) {
-                        baseFileName2ConflictingComponents.insert(baseName, cit);
-                        baseFileName2ConflictingComponents.insert(baseName, cit2);
-                        continue;
-                    }
-                    // fall through to error case
-                } else if (comp2MightHaveFileSelector) {
-                    // new entry contains file selector (and we now that cit did not)
-                    if (pathWithoutFileSelectors(compFileName2, comp2PotentialFileSelectorPos) == compFileName1) {
-                        baseFileName2ConflictingComponents.insert(compFileName1, cit2);
-                        continue;
-                    }
-                    // fall through to error case
-                }
                 // This entry clashes with a predecessor
                 QQmlError error;
-                error.setDescription(QQmlImportDatabase::tr("\"%1\" version %2.%3 is defined more than once in module \"%4\"")
-                                     .arg(cit->typeName).arg(cit->version.majorVersion())
-                                     .arg(cit->version.minorVersion()).arg(uri));
+                error.setDescription(
+                        QQmlImportDatabase::tr(
+                                "\"%1\" version %2.%3 is defined more than once in module \"%4\"")
+                                .arg(cit->typeName).arg(cit->version.majorVersion())
+                                .arg(cit->version.minorVersion()).arg(uri));
                 errors->prepend(error);
                 return QTypeRevision();
             }
@@ -1102,14 +1052,6 @@ QTypeRevision QQmlImports::matchingQmldirVersion(
         addVersion(cit->version);
     }
 
-    // ensure that all components point to the actual base URL, and let the file selectors resolve them correctly during URL resolution
-    for (auto keyIt = baseFileName2ConflictingComponents.keyBegin(); keyIt != baseFileName2ConflictingComponents.keyEnd(); ++keyIt) {
-        const QString& baseFileName = *keyIt;
-        const auto conflictingComponents = baseFileName2ConflictingComponents.values(baseFileName);
-        for (ConstIterator component: conflictingComponents)
-            component->fileName = baseFileName;
-    }
-
     typedef QList<QQmlDirParser::Script>::const_iterator SConstIterator;
     const QQmlDirScripts &scripts = qmldir.scripts();
 

src/qml/qml/qqmltypeloaderqmldircontent.cpp

@@ -29,6 +29,7 @@ void QQmlTypeLoaderQmldirContent::setContent(const QString &location, const QStr
     m_hasContent = true;
     m_location = location;
     m_parser.parse(content);
+    m_parser.disambiguateFileSelectors();
 }
 
 void QQmlTypeLoaderQmldirContent::setError(const QQmlError &error)

src/qml/qmldirparser/qqmldirparser.cpp

@@ -376,6 +376,129 @@ bool QQmlDirParser::parse(const QString &source)
     return hasError();
 }
 
+/* removes all file selector occurrences in path
+   firstPlus is the position of the initial '+' in the path
+   which we always have as we check for '+' to decide whether
+   we need to do some work at all
+*/
+static QString pathWithoutFileSelectors(QString path, // we want a copy of path
+                                        qsizetype firstPlus)
+{
+    do {
+        Q_ASSERT(path.at(firstPlus) == u'+');
+        const auto eos = path.size();
+        qsizetype terminatingSlashPos = firstPlus + 1;
+        while (terminatingSlashPos != eos && path.at(terminatingSlashPos) != u'/')
+            ++terminatingSlashPos;
+        path.remove(firstPlus, terminatingSlashPos - firstPlus + 1);
+        firstPlus = path.indexOf(u'+', firstPlus);
+    } while (firstPlus != -1);
+    return path;
+}
+
+static bool canDisambiguate(
+        const QString &fileName1, const QString &fileName2, QString *correctedFileName)
+{
+    // If the entries are exactly the same we can delete one without losing anything.
+    if (fileName1 == fileName2)
+        return true;
+
+    // If we detect conflicting paths, we check if they agree when we remove anything
+    // looking like a file selector.
+
+    // ugly heuristic to deal with file selectors
+    const qsizetype file2PotentialFileSelectorPos = fileName2.indexOf(u'+');
+    const bool file2MightHaveFileSelector = file2PotentialFileSelectorPos != -1;
+
+    if (const qsizetype fileSelectorPos1 = fileName1.indexOf(u'+'); fileSelectorPos1 != -1) {
+        // existing entry was file selector entry, fix it up
+        // it could also be the case that _both_ are using file selectors
+        const QString baseName = file2MightHaveFileSelector
+                ? pathWithoutFileSelectors(fileName2, file2PotentialFileSelectorPos)
+                : fileName2;
+
+        if (pathWithoutFileSelectors(fileName1, fileSelectorPos1) != baseName)
+            return false;
+
+        *correctedFileName = baseName;
+        return true;
+    }
+
+    // new entry contains file selector (and we know that fileName1 did not)
+    if (file2MightHaveFileSelector
+            && pathWithoutFileSelectors(fileName2, file2PotentialFileSelectorPos) == fileName1) {
+        *correctedFileName = fileName1;
+        return true;
+    }
+
+    return false;
+}
+
+static void disambiguateFileSelectedComponents(QQmlDirComponents *components)
+{
+    using ConstIterator = QQmlDirComponents::const_iterator;
+
+    // end iterator may get invalidated by the erasing below.
+    // Therefore, refetch it on each iteration.
+    for (ConstIterator cit = components->constBegin(); cit != components->constEnd();) {
+
+        // We can erase and re-assign cit if we immediately forget cit2.
+        // But we cannot erase cit2 without potentially invalidating cit.
+
+        bool doErase = false;
+        const ConstIterator cend = components->constEnd();
+        for (ConstIterator cit2 = ++ConstIterator(cit); cit2 != cend; ++cit2) {
+            if (cit2.key() != cit.key())
+                break;
+
+            Q_ASSERT(cit2->typeName == cit->typeName);
+
+            if (cit2->version != cit->version
+                || cit2->internal != cit->internal
+                || cit2->singleton != cit->singleton) {
+                continue;
+            }
+
+            // The two components may differ only by fileName now.
+
+            if (canDisambiguate(cit->fileName, cit2->fileName, &(cit2->fileName))) {
+                doErase = true;
+                break;
+            }
+        }
+
+        if (doErase)
+            cit = components->erase(cit);
+        else
+            ++cit;
+    }
+}
+
+static void disambiguateFileSelectedScripts(QQmlDirScripts *scripts)
+{
+    using Iterator = QQmlDirScripts::iterator;
+
+    Iterator send = scripts->end();
+
+    for (Iterator sit = scripts->begin(); sit != send; ++sit) {
+        send = std::remove_if(++Iterator(sit), send, [sit](const QQmlDirParser::Script &script2) {
+            if (sit->nameSpace != script2.nameSpace || sit->version != script2.version)
+                return false;
+
+            // The two scripts may differ only by fileName now.
+            return canDisambiguate(sit->fileName, script2.fileName, &(sit->fileName));
+        });
+    }
+
+    scripts->erase(send, scripts->end());
+}
+
+void QQmlDirParser::disambiguateFileSelectors()
+{
+    disambiguateFileSelectedComponents(&_components);
+    disambiguateFileSelectedScripts(&_scripts);
+}
+
 void QQmlDirParser::reportError(quint16 line, quint16 column, const QString &description)
 {
     QQmlJS::DiagnosticMessage error;

src/qml/qmldirparser/qqmldirparser_p.h

@@ -30,6 +30,7 @@ class Q_QML_COMPILER_PRIVATE_EXPORT QQmlDirParser
 public:
     void clear();
     bool parse(const QString &source);
+    void disambiguateFileSelectors();
 
     bool hasError() const { return !_errors.isEmpty(); }
     void setError(const QQmlJS::DiagnosticMessage &);

tests/auto/qml/qqmlfileselector/data/qmldirtest/main.qml

@@ -1,10 +1,8 @@
 import QtQuick
 import qmldirtest
 
-Window {
-    width: 640
-    height: 480
-    visible: true
+Item {
+    objectName: Name.name
     property color color: mybutton.color
 
     MyButton {

tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+linux/Name.js

@@ -0,0 +1 @@
+var name = "linux"

tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+macos/Name.js

@@ -0,0 +1 @@
+var name = "macos"

tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/Name.js

@@ -0,0 +1 @@
+var name = "base"

tests/auto/qml/qqmlfileselector/data/qmldirtest/qmldir

@@ -2,4 +2,7 @@ module qmldirtest
 MyButton 1.0 qml/MyButton.qml
 MyButton 1.0 qml/+linux/MyButton.qml
 MyButton 1.0 qml/+macos/MyButton.qml
+Name 1.0 qml/Name.js
+Name 1.0 qml/+linux/Name.js
+Name 1.0 qml/+macos/Name.js
 

tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/MyButton.qml

@@ -0,0 +1,7 @@
+import QtQuick
+
+Rectangle {
+    width: 300
+    height: 50
+    color: "yellow"
+}

tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/Name.js

@@ -0,0 +1 @@
+var name = "bar"

tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/MyButton.qml

@@ -0,0 +1,7 @@
+import QtQuick
+
+Rectangle {
+    width: 300
+    height: 50
+    color: "blue"
+}

tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/Name.js

@@ -0,0 +1 @@
+var name = "foo"

tests/auto/qml/qqmlfileselector/data/qmldirtest2/MyButton.qml

@@ -0,0 +1,7 @@
+import QtQuick
+
+Rectangle {
+    width: 300
+    height: 50
+    color: "green"
+}

tests/auto/qml/qqmlfileselector/data/qmldirtest2/Name.js

@@ -0,0 +1 @@
+var name = "base"

tests/auto/qml/qqmlfileselector/data/qmldirtest2/main.qml

@@ -0,0 +1,9 @@
+import QtQuick
+
+Item {
+    // TODO: objectName: Name.name
+    property color color: mybutton.color
+    MyButton {
+        id: mybutton
+    }
+}

tests/auto/qml/qqmlfileselector/data/qmldirtest2/qmldir

@@ -0,0 +1,5 @@
+module qmldirtest2
+MyButton 1.0 +foo/MyButton.qml
+MyButton 1.0 MyButton.qml
+MyButton 1.0 +bar/MyButton.qml
+Name 1.0 Name.js

tests/auto/qml/qqmlfileselector/tst_qqmlfileselector.cpp

@@ -72,19 +72,40 @@ void tst_qqmlfileselector::applicationEngineTest()
 
 void tst_qqmlfileselector::qmldirCompatibility()
 {
-    QQmlApplicationEngine engine;
-    engine.addImportPath(dataDirectory());
-    engine.load(testFileUrl("qmldirtest/main.qml"));
-    QVERIFY(!engine.rootObjects().isEmpty());
-    QObject *object = engine.rootObjects().at(0);
-    auto color = object->property("color").value<QColor>();
+    {
+        // No error for multiple files with different selectors, and the matching one is chosen
+        // for +macos and +linux selectors.
+        QQmlApplicationEngine engine;
+        engine.addImportPath(dataDirectory());
+        engine.load(testFileUrl("qmldirtest/main.qml"));
+        QVERIFY(!engine.rootObjects().isEmpty());
+        QObject *object = engine.rootObjects().at(0);
+        auto color = object->property("color").value<QColor>();
 #if defined(Q_OS_LINUX) && !defined(Q_OS_ANDROID)
-    QCOMPARE(color, QColorConstants::Svg::blue);
+        QCOMPARE(object->objectName(), "linux");
+        QCOMPARE(color, QColorConstants::Svg::blue);
 #elif defined(Q_OS_DARWIN)
-    QCOMPARE(color, QColorConstants::Svg::yellow);
+        QCOMPARE(object->objectName(), "macos");
+        QCOMPARE(color, QColorConstants::Svg::yellow);
 #else
-    QCOMPARE(color, QColorConstants::Svg::green);
+        QCOMPARE(object->objectName(), "base");
+        QCOMPARE(color, QColorConstants::Svg::green);
 #endif
+    }
+
+    {
+        // If nothing matches, the _base_ file is chosen, not the first or the last one.
+        // This also holds when using the implicit import.
+        QQmlApplicationEngine engine;
+        engine.addImportPath(dataDirectory());
+        engine.load(testFileUrl("qmldirtest2/main.qml"));
+        QVERIFY(!engine.rootObjects().isEmpty());
+        QObject *object = engine.rootObjects().at(0);
+        QCOMPARE(object->property("color").value<QColor>(), QColorConstants::Svg::green);
+
+        QEXPECT_FAIL("", "scripts in implicit import are not resolved", Continue);
+        QCOMPARE(object->objectName(), "base");
+    }
 }
 
 QTEST_MAIN(tst_qqmlfileselector)