qmllint: Catch JS variable declarations in QML elements

JS variable declarations in a QML element are not allowed.
The engine rejects them in the IR builder. As the linter doesn't run
this part of the compilation pipeline, it would never report the issue.

This commit fixes the issue by adding the necessary check.
It also avoids an assert in insertJSIdentifier when assertions are
enabled.

Fixes: QTBUG-137030
Pick-to: 6.10 6.9 6.8
Change-Id: Iff7a6e02bf852cf3a9affcac6ec40b7bd98cda83
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

src/qmlcompiler/qqmljsimportvisitor.cpp

@@ -82,6 +82,36 @@ static bool causesImplicitComponentWrapping(const QQmlJSMetaProperty &property,
     return propertyVerdict && assignedTypeVerdict;
 }
 
+/*!
+  \internal
+  A guarded version of insertJSIdentifier. If the scope is a QML scope,
+  it will log a syntax error instead.
+  Returns true if insertion was successful, otherwise false
+ */
+bool QQmlJSImportVisitor::safeInsertJSIdentifier(QQmlJSScope::Ptr &scope, const QString &name,
+                                   const QQmlJSScope::JavaScriptIdentifier &identifier)
+{
+  /* The grammar currently allows putting a variable declaration into a UiObjectMember
+     and we only complain about it in the IRBbuilder. It is unclear whether we should change
+     the grammar, as the linter would need to handle invalid programs anyway, so we'd need
+     to add some recovery rule to the grammar in any case.
+     We use this method instead to avoid an assertion in insertJSIdentifier
+  */
+    if (scope->scopeType() != QQmlSA::ScopeType::QMLScope) {
+        scope->insertJSIdentifier(name, identifier);
+        return true;
+    } else {
+        const QQmlJSScope *scopePtr = scope.get();
+        std::pair<const QQmlJSScope*, QString> misplaced { scopePtr, name };
+        if (misplacedJSIdentifiers.contains(misplaced))
+            return false; // we only want to warn once
+        misplacedJSIdentifiers.insert(misplaced);
+        m_logger->log(u"JavaScript declarations are not allowed in QML elements"_s, qmlSyntax,
+                      identifier.location);
+        return false;
+    }
+}
+
 /*!
   \internal
   Sets the name of \a scope to \a name based on \a type.
@@ -1501,9 +1531,9 @@ void QQmlJSImportVisitor::flushPendingSignalParameters()
 {
     const QQmlJSMetaSignalHandler handler = m_signalHandlers[m_pendingSignalHandler];
     for (const QString &parameter : handler.signalParameters) {
-        m_currentScope->insertJSIdentifier(parameter,
-                                           { QQmlJSScope::JavaScriptIdentifier::Injected,
-                                             m_pendingSignalHandler, std::nullopt, false });
+        safeInsertJSIdentifier(m_currentScope, parameter,
+                               { QQmlJSScope::JavaScriptIdentifier::Injected,
+                                 m_pendingSignalHandler, std::nullopt, false });
     }
     m_pendingSignalHandler = QQmlJS::SourceLocation();
 }
@@ -1993,10 +2023,10 @@ void QQmlJSImportVisitor::visitFunctionExpressionHelper(QQmlJS::AST::FunctionExp
             const QQmlJS::SourceLocation functionLocation = fexpr->identifierToken.isValid()
                     ? fexpr->identifierToken
                     : fexpr->functionToken;
-            m_currentScope->insertJSIdentifier(name,
-                                               { QQmlJSScope::JavaScriptIdentifier::LexicalScoped,
-                                                 functionLocation, method.returnTypeName(),
-                                                 false });
+            safeInsertJSIdentifier(m_currentScope, name,
+                                   { QQmlJSScope::JavaScriptIdentifier::LexicalScoped,
+                                     functionLocation, method.returnTypeName(),
+                                     false });
         }
         enterEnvironment(QQmlSA::ScopeType::JSFunctionScope, name, fexpr->firstSourceLocation());
     } else {
@@ -2817,7 +2847,7 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::Catch *catchStatement)
 {
     enterEnvironment(QQmlSA::ScopeType::JSLexicalScope, QStringLiteral("catch"),
                      catchStatement->firstSourceLocation());
-    m_currentScope->insertJSIdentifier(
+    safeInsertJSIdentifier(m_currentScope,
             catchStatement->patternElement->bindingIdentifier.toString(),
             { QQmlJSScope::JavaScriptIdentifier::LexicalScoped,
               catchStatement->patternElement->firstSourceLocation(), std::nullopt,
@@ -2872,7 +2902,9 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::VariableDeclarationList *vdl)
         }
 
         const bool isConst = vdl->declaration->scope == QQmlJS::AST::VariableScope::Const;
-        m_currentScope->insertJSIdentifier(name, { kind, location, typeName, isConst });
+        // Break if insertion failed to avoid multiple warnings at the same location
+        if (!safeInsertJSIdentifier(m_currentScope, name,  { kind, location, typeName, isConst }))
+            break;
         vdl = vdl->next;
     }
     return true;
@@ -2887,7 +2919,7 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::FormalParameterList *fpl)
         if (TypeAnnotation *annotation = boundName.typeAnnotation.data())
             if (Type *type = annotation->type)
                 typeName = type->toString();
-        m_currentScope->insertJSIdentifier(boundName.id,
+        safeInsertJSIdentifier(m_currentScope, boundName.id,
                                            { QQmlJSScope::JavaScriptIdentifier::Parameter,
                                              boundName.location, typeName, false });
     }
@@ -3130,13 +3162,15 @@ bool QQmlJSImportVisitor::visit(QQmlJS::AST::PatternElement *element)
             if (TypeAnnotation *annotation = name.typeAnnotation.data())
                 if (Type *type = annotation->type)
                     typeName = type->toString();
-            m_currentScope->insertJSIdentifier(
+            const bool couldInsert = safeInsertJSIdentifier(m_currentScope,
                     name.id,
                     { (element->scope == QQmlJS::AST::VariableScope::Var)
                               ? QQmlJSScope::JavaScriptIdentifier::FunctionScoped
                               : QQmlJSScope::JavaScriptIdentifier::LexicalScoped,
                       name.location, typeName,
                       element->scope == QQmlJS::AST::VariableScope::Const });
+            if (!couldInsert)
+                break;
         }
     }
 

src/qmlcompiler/qqmljsimportvisitor_p.h

@@ -194,6 +194,7 @@ protected:
     QSet<QString> m_usedTypes;
 
     QList<UnfinishedBinding> m_bindings;
+    QSet<std::pair<const QQmlJSScope *, QString>> misplacedJSIdentifiers;
 
     // stores JS functions and Script bindings per scope (only the name). mimics
     // the content of QmlIR::Object::functionsAndExpressions
@@ -410,6 +411,9 @@ private:
     void enterRootScope(QQmlJSScope::ScopeType type, const QString &name,
                            const QQmlJS::SourceLocation &location);
 
+    bool safeInsertJSIdentifier(QQmlJSScope::Ptr &scope, const QString &name,
+                                const QQmlJSScope::JavaScriptIdentifier &identifier);
+
     QList<QQmlJS::DiagnosticMessage> importFromHost(
             const QString &path, const QString &prefix, const QQmlJS::SourceLocation &location);
     QList<QQmlJS::DiagnosticMessage> importFromQrc(

tests/auto/qml/qmllint/data/jsdeclInQmlScope.qml

@@ -0,0 +1,5 @@
+import QtQml
+QtObject {
+
+        var stuff = [1, 2, 3, 5]
+   }

tests/auto/qml/qmllint/tst_qmllint.cpp

@@ -1160,6 +1160,10 @@ expression: \${expr} \${expr} \\\${expr} \\\${expr}`)"_L1, 16, 27 } },
                          { uR"("D" was not found for the type of parameter "d" in method "i".)"_s, 7, 17 },
                          { uR"("G" was not found for the type of parameter "g" in method "i".)"_s, 7, 26 },
                          }};
+
+    QTest::newRow("jsdeclInQmlScope")
+            << QStringLiteral("jsdeclInQmlScope.qml")
+            << Result{ { { "JavaScript declarations are not allowed in QML elements"_L1 , 4, 13 } } };
 }
 
 void TestQmllint::dirtyQmlCode()