QQmlJSImportVisitor: Handle on binding inside grouped properties

The visitation logic for UiObjectBinding was entering nested scopes for
attached and grouped properties to check if their scopes need to be
resolved. However, the logic to leave those scopes was faulty: It left
the current scope as long as the current scope was a attached or grouped
property scope. That would however break if an on-binding (triggering
this code path) were placed in a grouped property. In that case, we
would leave the grouped property scope, even though it should have still
been the current scope.
Fix the issue by explicitly counting how many scopes we were entering,
and leave the corresponding number of scopes afterwards.

As a drive-by, turn a "verbal assertion" in a Q_ASSERT in endVisit.

Fixes: QTBUG-98125
Change-Id: I270b1bf3fc5b38ad9d437df1ea6c55684d143378
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
(cherry picked from commit 332099038d5fe0020105dffc12550abef9dbd06b)

3 files changed, 19 insertions, 6 deletions

diff --git a/src/qmlcompiler/qqmljsimportvisitor.cpp b/src/qmlcompiler/qqmljsimportvisitor.cpp
index 158ff03e20..edbc8c08d6 100644
--- a/src/qmlcompiler/qqmljsimportvisitor.cpp
+++ b/src/qmlcompiler/qqmljsimportvisitor.cpp
@@ -1507,6 +1507,7 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::UiObjectBinding *uiob)
     name.chop(1);
 
     bool needsResolution = false;
+    int scopesEnteredCounter = 0;
     for (auto group = uiob->qualifiedId; group->next; group = group->next) {
         const QString idName = group->name.toString();
 
@@ -1516,11 +1517,11 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::UiObjectBinding *uiob)
         const auto scopeKind = idName.front().isUpper() ? QQmlJSScope::AttachedPropertyScope
                                                         : QQmlJSScope::GroupedPropertyScope;
         bool exists = enterEnvironmentNonUnique(scopeKind, idName, group->firstSourceLocation());
+        ++scopesEnteredCounter;
         needsResolution = needsResolution || !exists;
     }
 
-    while (m_currentScope->scopeType() == QQmlJSScope::GroupedPropertyScope
-           || m_currentScope->scopeType() == QQmlJSScope::AttachedPropertyScope) {
+    for (int i=0; i < scopesEnteredCounter; ++i) { // leave the scopes we entered again
         leaveEnvironment();
     }
 
@@ -1544,6 +1545,7 @@ void QQmlJSImportVisitor::endVisit(QQmlJS::AST::UiObjectBinding *uiob)
     leaveEnvironment();
 
     auto group = uiob->qualifiedId;
+    int scopesEnteredCounter = 0;
     for (; group->next; group = group->next) {
         const QString idName = group->name.toString();
 
@@ -1553,7 +1555,9 @@ void QQmlJSImportVisitor::endVisit(QQmlJS::AST::UiObjectBinding *uiob)
         const auto scopeKind = idName.front().isUpper() ? QQmlJSScope::AttachedPropertyScope
                                                         : QQmlJSScope::GroupedPropertyScope;
         // definitely exists
-        enterEnvironmentNonUnique(scopeKind, idName, group->firstSourceLocation());
+        [[maybe_unused]] bool exists = enterEnvironmentNonUnique(scopeKind, idName, group->firstSourceLocation());
+        Q_ASSERT(exists);
+        scopesEnteredCounter++;
     }
 
     // on ending the visit to UiObjectBinding, set the property type to the
@@ -1570,10 +1574,8 @@ void QQmlJSImportVisitor::endVisit(QQmlJS::AST::UiObjectBinding *uiob)
                                                   uiob->firstSourceLocation(), uiob->hasOnToken };
     }
 
-    while (m_currentScope->scopeType() == QQmlJSScope::GroupedPropertyScope
-           || m_currentScope->scopeType() == QQmlJSScope::AttachedPropertyScope) {
+    for (int i = 0; i < scopesEnteredCounter; ++i)
         leaveEnvironment();
-    }
 }
 
 bool QQmlJSImportVisitor::visit(ExportDeclaration *)
diff --git a/tests/auto/qml/qmllint/data/onBindingInGroupedProperty.qml b/tests/auto/qml/qmllint/data/onBindingInGroupedProperty.qml
new file mode 100644
index 0000000000..186b8a557f
--- /dev/null
+++ b/tests/auto/qml/qmllint/data/onBindingInGroupedProperty.qml
@@ -0,0 +1,10 @@
+import QtQuick
+
+Rectangle
+{
+    id: root
+    border
+    {
+        ColorAnimation on color { }
+    }
+}
diff --git a/tests/auto/qml/qmllint/tst_qmllint.cpp b/tests/auto/qml/qmllint/tst_qmllint.cpp
index 7006986928..7c434a5635 100644
--- a/tests/auto/qml/qmllint/tst_qmllint.cpp
+++ b/tests/auto/qml/qmllint/tst_qmllint.cpp
@@ -806,6 +806,7 @@ void TestQmllint::cleanQmlCode_data()
     QTest::newRow("QQmlEasingEnums::Type") << QStringLiteral("animationEasing.qml");
     QTest::newRow("required property in Component") << QStringLiteral("requiredPropertyInComponent.qml");
     QTest::newRow("connectionNoParent") << QStringLiteral("connectionNoParent.qml"); // QTBUG-97600
+    QTest::newRow("on binding in grouped property") << QStringLiteral("onBindingInGroupedProperty.qml");
 }
 
 void TestQmllint::cleanQmlCode()