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++.

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>
(cherry picked from commit 269afdcb7cab15a7430487b630b1b4af468d2015)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

18 files changed, 206 insertions, 76 deletions

diff --git a/src/qml/qml/qqmlimport.cpp b/src/qml/qml/qqmlimport.cpp
index 81ddba8a8e..cea8fd9fe5 100644
--- a/src/qml/qml/qqmlimport.cpp
+++ b/src/qml/qml/qqmlimport.cpp
@@ -1035,26 +1035,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
 
@@ -1101,47 +1081,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();
             }
@@ -1150,14 +1100,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();
 
diff --git a/src/qml/qml/qqmltypeloaderqmldircontent.cpp b/src/qml/qml/qqmltypeloaderqmldircontent.cpp
index 57044fcdc3..15767f01ad 100644
--- a/src/qml/qml/qqmltypeloaderqmldircontent.cpp
+++ b/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)
diff --git a/src/qml/qmldirparser/qqmldirparser.cpp b/src/qml/qmldirparser/qqmldirparser.cpp
index a2983995cc..e6a5691d3d 100644
--- a/src/qml/qmldirparser/qqmldirparser.cpp
+++ b/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;
diff --git a/src/qml/qmldirparser/qqmldirparser_p.h b/src/qml/qmldirparser/qqmldirparser_p.h
index 9a04997048..43409b7b41 100644
--- a/src/qml/qmldirparser/qqmldirparser_p.h
+++ b/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 &);
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest/main.qml b/tests/auto/qml/qqmlfileselector/data/qmldirtest/main.qml
index d6dd2c9b90..4e09798a84 100644
--- a/tests/auto/qml/qqmlfileselector/data/qmldirtest/main.qml
+++ b/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 {
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+linux/Name.js b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+linux/Name.js
new file mode 100644
index 0000000000..91ca4f129d
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+linux/Name.js
@@ -0,0 +1 @@
+var name = "linux"
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+macos/Name.js b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+macos/Name.js
new file mode 100644
index 0000000000..12e5058285
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/+macos/Name.js
@@ -0,0 +1 @@
+var name = "macos"
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/Name.js b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/Name.js
new file mode 100644
index 0000000000..916a232eb4
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qml/Name.js
@@ -0,0 +1 @@
+var name = "base"
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest/qmldir b/tests/auto/qml/qqmlfileselector/data/qmldirtest/qmldir
index ac68d9097d..a2efdbf27d 100644
--- a/tests/auto/qml/qqmlfileselector/data/qmldirtest/qmldir
+++ b/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
 
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/MyButton.qml b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/MyButton.qml
new file mode 100644
index 0000000000..5bf632c48d
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/MyButton.qml
@@ -0,0 +1,7 @@
+import QtQuick
+
+Rectangle {
+    width: 300
+    height: 50
+    color: "yellow"
+}
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/Name.js b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/Name.js
new file mode 100644
index 0000000000..8591795d37
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+bar/Name.js
@@ -0,0 +1 @@
+var name = "bar"
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/MyButton.qml b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/MyButton.qml
new file mode 100644
index 0000000000..cc6eb967da
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/MyButton.qml
@@ -0,0 +1,7 @@
+import QtQuick
+
+Rectangle {
+    width: 300
+    height: 50
+    color: "blue"
+}
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/Name.js b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/Name.js
new file mode 100644
index 0000000000..b224ed15ec
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/+foo/Name.js
@@ -0,0 +1 @@
+var name = "foo"
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/MyButton.qml b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/MyButton.qml
new file mode 100644
index 0000000000..32db428c4f
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/MyButton.qml
@@ -0,0 +1,7 @@
+import QtQuick
+
+Rectangle {
+    width: 300
+    height: 50
+    color: "green"
+}
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/Name.js b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/Name.js
new file mode 100644
index 0000000000..916a232eb4
--- /dev/null
+++ b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/Name.js
@@ -0,0 +1 @@
+var name = "base"
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/main.qml b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/main.qml
new file mode 100644
index 0000000000..3ef7df59e0
--- /dev/null
+++ b/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
+    }
+}
diff --git a/tests/auto/qml/qqmlfileselector/data/qmldirtest2/qmldir b/tests/auto/qml/qqmlfileselector/data/qmldirtest2/qmldir
new file mode 100644
index 0000000000..92fefb9806
--- /dev/null
+++ b/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
diff --git a/tests/auto/qml/qqmlfileselector/tst_qqmlfileselector.cpp b/tests/auto/qml/qqmlfileselector/tst_qqmlfileselector.cpp
index 46df20378c..4e746ab10e 100644
--- a/tests/auto/qml/qqmlfileselector/tst_qqmlfileselector.cpp
+++ b/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)