QmlCompiler: Reject ambiguous and inaccessible types

If a type is not accessible in the imported version of a module, exclude
it. If we find multiple types for the same name and version, drop them
all and warn.

Pick-to: 6.2 6.3
Fixes: QTBUG-99113
Change-Id: I91d99d603ada6cf87f84afdbb01a543880d9aa76
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/qmlcompiler/qqmljsimporter.cpp

@@ -129,6 +129,19 @@ void QQmlJSImporter::readQmltypes(
     }
 }
 
+static QString internalName(const QQmlJSScope::ConstPtr &scope)
+{
+    if (const auto *factory = scope.factory())
+        return factory->internalName();
+    return scope->internalName();
+}
+
+static bool isComposite(const QQmlJSScope::ConstPtr &scope)
+{
+    // The only thing the factory can do is load a composite type.
+    return scope.factory() || scope->isComposite();
+}
+
 QQmlJSImporter::Import QQmlJSImporter::readQmldir(const QString &path)
 {
     Import result;
@@ -171,7 +184,7 @@ QQmlJSImporter::Import QQmlJSImporter::readQmldir(const QString &path)
             continue;
         }
 
-        auto mo = qmlComponents.find(it.key());
+        auto mo = qmlComponents.find(it->fileName);
         if (mo == qmlComponents.end()) {
             QQmlJSScope::Ptr imported = localFile2ScopeTree(filePath);
             if (it->singleton) {
@@ -180,7 +193,7 @@ QQmlJSImporter::Import QQmlJSImporter::readQmldir(const QString &path)
                 else
                     imported->setIsSingleton(true);
             }
-            mo = qmlComponents.insert(it.key(), {imported, QList<QQmlJSScope::Export>() });
+            mo = qmlComponents.insert(it->fileName, {imported, QList<QQmlJSScope::Export>() });
         }
 
         mo->exports.append(QQmlJSScope::Export(reader.typeNamespace(), it.key(), it->version));
@@ -191,7 +204,12 @@ QQmlJSImporter::Import QQmlJSImporter::readQmldir(const QString &path)
     const auto scripts = reader.scripts();
     for (const auto &script : scripts) {
         const QString filePath = path + QLatin1Char('/') + script.fileName;
-        result.scripts.insert(script.nameSpace, { localFile2ScopeTree(filePath), {}});
+        auto mo = result.scripts.find(script.fileName);
+        if (mo == result.scripts.end())
+            mo = result.scripts.insert(script.fileName, { localFile2ScopeTree(filePath), {} });
+
+        mo->exports.append(QQmlJSScope::Export(
+                               reader.typeNamespace(), script.nameSpace, script.version));
     }
     return result;
 }
@@ -213,17 +231,11 @@ void QQmlJSImporter::importDependencies(const QQmlJSImporter::Import &import,
     }
 }
 
-static QString internalName(const QQmlJSScope::ConstPtr &scope)
+static bool isVersionAllowed(const QQmlJSScope::Export &exportEntry,
+                             const QQmlJSScope::Import &importDescription)
 {
-    if (const auto *factory = scope.factory())
+    return !importDescription.version().isValid()
-        return factory->internalName();
+            || exportEntry.version() <= importDescription.version();
-    return scope->internalName();
-}
-
-static bool isComposite(const QQmlJSScope::ConstPtr &scope)
-{
-    // The only thing the factory can do is load a composite type.
-    return scope.factory() || scope->isComposite();
 }
 
 void QQmlJSImporter::processImport(const QQmlJSScope::Import &importDescription,
@@ -233,11 +245,78 @@ void QQmlJSImporter::processImport(const QQmlJSScope::Import &importDescription,
     const QString anonPrefix = QStringLiteral("$anonymous$");
     QHash<QString, QList<QQmlJSScope::Export>> seenExports;
 
+    const auto insertExports = [&](const QQmlJSExportedScope &val) {
+        // Resolve conflicting qmlNames within an import
+        for (const auto &valExport : val.exports) {
+            const QString name = prefixedName(importDescription.prefix(), valExport.type());
+            if (!isVersionAllowed(valExport, importDescription))
+                continue;
+
+            const auto it = types->qmlNames.constFind(name);
+            if (it != types->qmlNames.constEnd()) {
+
+                // The same set of exports can declare the same name multiple times for different
+                // versions. That's the common thing and we would just continue here when we hit
+                // it again after having inserted successfully once.
+                // However, it can also declare *different* names. Then we need to do the whole
+                // thing again.
+                if (*it == val.scope)
+                    continue;
+
+                const auto existingExports = seenExports.value(name);
+                enum { LowerVersion, SameVersion, HigherVersion } seenVersion = LowerVersion;
+                for (const QQmlJSScope::Export &entry : existingExports) {
+                    if (entry.type() != name || !isVersionAllowed(entry, importDescription))
+                        continue;
+
+                    if (valExport.version() < entry.version()) {
+                        seenVersion = HigherVersion;
+                        break;
+                    }
+
+                    if (seenVersion == LowerVersion && valExport.version() == entry.version())
+                        seenVersion = SameVersion;
+                }
+
+                switch (seenVersion) {
+                case LowerVersion:
+                    break;
+                case SameVersion: {
+                    m_warnings.append({
+                        QStringLiteral("Ambiguous type detected. "
+                                       "%1 %2.%3 is defined multiple times.")
+                            .arg(name)
+                            .arg(valExport.version().majorVersion())
+                            .arg(valExport.version().minorVersion()),
+                        QtCriticalMsg,
+                        QQmlJS::SourceLocation()
+                    });
+
+                    // Remove the name. We don't know which one to use.
+                    types->qmlNames.remove(name);
+                    continue;
+                }
+                case HigherVersion:
+                    continue;
+                }
+            }
+
+            types->qmlNames.insert(name, val.scope);
+
+            // We replace the exports for name, as the new set is clearly superior when we get here.
+            seenExports.insert(name, val.exports);
+        }
+    };
+
     if (!importDescription.prefix().isEmpty())
         types->qmlNames.insert(importDescription.prefix(), {}); // Empty type means "this is the prefix"
 
-    for (auto it = import.scripts.begin(); it != import.scripts.end(); ++it)
+    for (auto it = import.scripts.begin(); it != import.scripts.end(); ++it) {
-        types->qmlNames.insert(prefixedName(importDescription.prefix(), it.key()), it->scope);
+        types->cppNames.insert(prefixedName(anonPrefix, internalName(it->scope)), it->scope);
+        // You cannot have a script without an export
+        Q_ASSERT(!it->exports.isEmpty());
+        insertExports(*it);
+    }
 
     // add objects
     for (auto it = import.objects.begin(); it != import.objects.end(); ++it) {
@@ -249,40 +328,12 @@ void QQmlJSImporter::processImport(const QQmlJSScope::Import &importDescription,
         types->cppNames.insert(name, val.scope);
 
         if (val.exports.isEmpty()) {
+            // Insert an unresolvable dummy name
             types->qmlNames.insert(
                         prefixedName(importDescription.prefix(), prefixedName(
                                          anonPrefix, internalName(val.scope))), val.scope);
-        }
+        } else {
-
+            insertExports(val);
-        for (const auto &valExport : val.exports) {
-            const QString &name = prefixedName(importDescription.prefix(), valExport.type());
-            // Resolve conflicting qmlNames within an import
-            if (types->qmlNames.contains(name)) {
-                const auto &existing = types->qmlNames[name];
-
-                if (existing == val.scope)
-                    continue;
-
-                if (valExport.version() > importDescription.version())
-                    continue;
-
-                const auto existingExports = seenExports.value(name);
-                auto betterExport =
-                        std::find_if(
-                            existingExports.constBegin(), existingExports.constEnd(),
-                            [&](const QQmlJSScope::Export &exportEntry) {
-                                return exportEntry.type() == name
-                                        // Ensure that the entry isn't newer than the module version
-                                        && exportEntry.version() <= importDescription.version()
-                                        && valExport.version() < exportEntry.version();
-                            });
-
-                if (betterExport != existingExports.constEnd())
-                    continue;
-            }
-
-            types->qmlNames.insert(name, val.scope);
-            seenExports.insert(name, val.exports);
         }
     }
 

tests/auto/qml/qmlcppcodegen/data/failures.qml

@@ -1,5 +1,6 @@
 import QtQml
 import TestTypes
+import Ambiguous 1.2
 
 QtObject {
     property string attachedForNonObject: objectName.Component.objectName
@@ -12,4 +13,12 @@ QtObject {
     onFooBar: console.log("nope")
 
     function doesReturnValue() { return 5; }
+
+    property Thing thing: Thing {
+        property int b: a + 1
+    }
+
+    property NotHere here: NotHere {
+        property int c: b + 1
+    }
 }

tests/auto/qml/qmlcppcodegen/data/imports/Ambiguous/Here.qml

@@ -0,0 +1,5 @@
+import QtQml
+
+QtObject {
+    property int b: 12
+}

tests/auto/qml/qmlcppcodegen/data/imports/Ambiguous/Thing1.qml

@@ -0,0 +1,5 @@
+import QtQml
+
+QtObject {
+    property int a: 5
+}

tests/auto/qml/qmlcppcodegen/data/imports/Ambiguous/Thing2.qml

@@ -0,0 +1,5 @@
+import QtQml
+
+QtObject {
+    property int a: 10
+}

tests/auto/qml/qmlcppcodegen/data/imports/Ambiguous/qmldir

@@ -0,0 +1,5 @@
+module Ambiguous
+
+Thing 1.0 Thing1.qml
+Thing 1.0 Thing2.qml
+NotHere 1.12 Here.qml