Fix regression in tst_qqmlecmascript::signalAssignment

Detect errors in the signal declaration already at compile time, re-introducing
the earlier code in qqmlcompiler.cpp that checked that. This also means that
the parameter string construction can be done once for each signal and not for
each handler.

Change-Id: Icf6242a793939466bbc44d43bf041281164ad1b6
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

7 files changed, 109 insertions, 37 deletions

diff --git a/src/qml/qml/qqmlboundsignal.cpp b/src/qml/qml/qqmlboundsignal.cpp
index 80a06e9761..0c922294b4 100644
--- a/src/qml/qml/qqmlboundsignal.cpp
+++ b/src/qml/qml/qqmlboundsignal.cpp
@@ -69,7 +69,8 @@ static QQmlJavaScriptExpression::VTable QQmlBoundSignalExpression_jsvtable = {
 QQmlBoundSignalExpression::QQmlBoundSignalExpression(QObject *target, int index,
                                                      QQmlContextData *ctxt, QObject *scope, const QString &expression,
                                                      const QString &fileName, quint16 line, quint16 column,
-                                                     const QString &handlerName)
+                                                     const QString &handlerName,
+                                                     const QString &parameterString)
     : QQmlJavaScriptExpression(&QQmlBoundSignalExpression_jsvtable),
       m_fileName(fileName),
       m_line(line),
@@ -81,6 +82,7 @@ QQmlBoundSignalExpression::QQmlBoundSignalExpression(QObject *target, int index,
 {
     init(ctxt, scope);
     m_handlerName = handlerName;
+    m_parameterString = parameterString;
     m_expression = expression;
 }
 
@@ -133,50 +135,35 @@ void QQmlBoundSignalExpression::evaluate(void **a)
     ep->referenceScarceResources(); // "hold" scarce resources in memory during evaluation.
     {
         if (!m_expressionFunctionValid) {
-
-            //TODO: look at using the property cache here (as in the compiler)
-            //      for further optimization
-            QMetaMethod signal = QMetaObjectPrivate::signal(m_target->metaObject(), m_index);
-
             QString expression;
 
             expression = QStringLiteral("(function ");
             expression += m_handlerName;
             expression += QLatin1Char('(');
 
-            QString error;
-
-            bool unnamedParameter = false;
-            const QV4::IdentifierHash<bool> &illegalNames = ep->v8engine()->illegalNames();
-
-            const QList<QByteArray> parameters = signal.parameterNames();
-            for (int i = 0; i < parameters.count(); ++i) {
-                if (i > 0)
-                    expression += QLatin1Char(',');
-                const QByteArray &param = parameters.at(i);
-                if (param.isEmpty())
-                    unnamedParameter = true;
-                else if (unnamedParameter) {
-                    error = QCoreApplication::translate("QQmlRewrite", "Signal uses unnamed parameter followed by named parameter.");
-                    break;
-                } else if (illegalNames.contains(param)) {
-                    error = QCoreApplication::translate("QQmlRewrite", "Signal parameter \"%1\" hides global variable.").arg(QString::fromUtf8(param));
-                    break;
+            if (m_parameterString.isEmpty()) {
+                QString error;
+                //TODO: look at using the property cache here (as in the compiler)
+                //      for further optimization
+                QMetaMethod signal = QMetaObjectPrivate::signal(m_target->metaObject(), m_index);
+                expression += QQmlPropertyCache::signalParameterStringForJS(engine(), signal.parameterNames(), &error);
+
+                if (!error.isEmpty()) {
+                    qmlInfo(scopeObject()) << error;
+                    m_invalidParameterName = true;
+                    ep->dereferenceScarceResources();
+                    return;
                 }
-                expression += QString::fromUtf8(param);
-            }
-
-            if (!error.isEmpty()) {
-                qmlInfo(scopeObject()) << error;
-                m_invalidParameterName = true;
-                ep->dereferenceScarceResources();
-                return;
-            }
+            } else
+                expression += m_parameterString;
 
             expression += QStringLiteral(") { ");
             expression += m_expression;
             expression += QStringLiteral(" })");
+
             m_expression.clear();
+            m_handlerName.clear();
+            m_parameterString.clear();
 
             m_v8function = evalFunction(context(), scopeObject(), expression,
                                         m_fileName, m_line, &m_v8qmlscope);
diff --git a/src/qml/qml/qqmlboundsignal_p.h b/src/qml/qml/qqmlboundsignal_p.h
index 7d3adb17b1..ffb3d06770 100644
--- a/src/qml/qml/qqmlboundsignal_p.h
+++ b/src/qml/qml/qqmlboundsignal_p.h
@@ -72,7 +72,8 @@ public:
     QQmlBoundSignalExpression(QObject *target, int index,
                               QQmlContextData *ctxt, QObject *scope, const QString &expression,
                               const QString &fileName, quint16 line, quint16 column,
-                              const QString &handlerName = QString());
+                              const QString &handlerName = QString(),
+                              const QString &parameterString = QString());
 
 
     // "inherited" from QQmlJavaScriptExpression.
@@ -99,6 +100,7 @@ private:
     QV4::PersistentValue m_v8function;
 
     QString m_handlerName;
+    QString m_parameterString;
     //once m_v8function is valid, we clear expression and
     //extract it from m_v8function if needed.
     QString m_expression;   //only used when expression needs to be rewritten
diff --git a/src/qml/qml/qqmlcompiler.cpp b/src/qml/qml/qqmlcompiler.cpp
index a940bb256b..ddefffe8e2 100644
--- a/src/qml/qml/qqmlcompiler.cpp
+++ b/src/qml/qml/qqmlcompiler.cpp
@@ -1314,6 +1314,7 @@ void QQmlCompiler::genObjectBody(QQmlScript::Object *obj)
 
             Instruction::StoreSignal store;
             store.handlerName = output->indexForString(prop->name().toString());
+            store.parameters = output->indexForString(obj->metatype->signalParameterStringForJS(prop->index));
             store.signalIndex = prop->index;
             store.value = output->indexForString(v->value.asScript());
             store.context = v->signalExpressionContextStack;
@@ -1677,6 +1678,11 @@ bool QQmlCompiler::buildSignal(QQmlScript::Property *prop, QQmlScript::Object *o
             //to ensure all parameters are available (see qqmlboundsignal constructor for more details)
             prop->index = obj->metatype->originalClone(prop->index);
             prop->values.first()->signalExpressionContextStack = ctxt.stack;
+
+            QString errorString;
+            obj->metatype->signalParameterStringForJS(prop->index, &errorString);
+            if (!errorString.isEmpty())
+                COMPILE_EXCEPTION(prop, errorString);
         }
     }
 
diff --git a/src/qml/qml/qqmlinstruction_p.h b/src/qml/qml/qqmlinstruction_p.h
index a157a0bbc8..be27f9069c 100644
--- a/src/qml/qml/qqmlinstruction_p.h
+++ b/src/qml/qml/qqmlinstruction_p.h
@@ -393,6 +393,7 @@ union QQmlInstruction
     struct instr_storeSignal {
         QML_INSTR_HEADER
         int handlerName;
+        int parameters;
         int signalIndex;
         int value;
         short context;
diff --git a/src/qml/qml/qqmlpropertycache.cpp b/src/qml/qml/qqmlpropertycache.cpp
index 903d50e067..aee24bb028 100644
--- a/src/qml/qml/qqmlpropertycache.cpp
+++ b/src/qml/qml/qqmlpropertycache.cpp
@@ -73,7 +73,6 @@ public:
 
     //for signal handler rewrites
     QString *signalParameterStringForJS;
-    int signalParameterCountForJS:30;
     int parameterError:1;
     int argumentsValid:1;
 
@@ -1078,7 +1077,6 @@ QQmlPropertyCacheMethodArguments *QQmlPropertyCache::createArgumentsObject(int a
     args->arguments[0] = argc;
     args->argumentsValid = false;
     args->signalParameterStringForJS = 0;
-    args->signalParameterCountForJS = 0;
     args->parameterError = false;
     args->names = argc ? new QList<QByteArray>(names) : 0;
     args->next = argumentsCache;
@@ -1086,6 +1084,81 @@ QQmlPropertyCacheMethodArguments *QQmlPropertyCache::createArgumentsObject(int a
     return args;
 }
 
+/*! \internal
+    \a index MUST be in the signal index range (see QObjectPrivate::signalIndex()).
+    This is different from QMetaMethod::methodIndex().
+*/
+QString QQmlPropertyCache::signalParameterStringForJS(int index, QString *errorString)
+{
+    QQmlPropertyCache *c = 0;
+    QQmlPropertyData *signalData = signal(index, &c);
+    if (!signalData)
+        return QString();
+
+    typedef QQmlPropertyCacheMethodArguments A;
+
+    if (signalData->arguments) {
+        A *arguments = static_cast<A *>(signalData->arguments);
+        if (arguments->signalParameterStringForJS) {
+            if (arguments->parameterError) {
+                if (errorString)
+                    *errorString = *arguments->signalParameterStringForJS;
+                return QString();
+            }
+            return *arguments->signalParameterStringForJS;
+        }
+    }
+
+    QList<QByteArray> parameterNameList = signalParameterNames(index);
+
+    if (!signalData->arguments) {
+        A *args = c->createArgumentsObject(parameterNameList.count(), parameterNameList);
+        signalData->arguments = args;
+    }
+
+    QString error;
+    QString parameters = signalParameterStringForJS(engine, parameterNameList, &error);
+
+    A *arguments = static_cast<A *>(signalData->arguments);
+    arguments->signalParameterStringForJS = new QString(!error.isEmpty() ? error : parameters);
+    if (!error.isEmpty()) {
+        arguments->parameterError = true;
+        if (errorString)
+            *errorString = *arguments->signalParameterStringForJS;
+        return QString();
+    }
+    return *arguments->signalParameterStringForJS;
+}
+
+QString QQmlPropertyCache::signalParameterStringForJS(QQmlEngine *engine, const QList<QByteArray> &parameterNameList, QString *errorString)
+{
+    QQmlEnginePrivate *ep = QQmlEnginePrivate::get(engine);
+    bool unnamedParameter = false;
+    const QV4::IdentifierHash<bool> &illegalNames = ep->v8engine()->illegalNames();
+    QString error;
+    QString parameters;
+
+    for (int i = 0; i < parameterNameList.count(); ++i) {
+        if (i > 0)
+            parameters += QLatin1Char(',');
+        const QByteArray &param = parameterNameList.at(i);
+        if (param.isEmpty())
+            unnamedParameter = true;
+        else if (unnamedParameter) {
+            if (errorString)
+                *errorString = QCoreApplication::translate("QQmlRewrite", "Signal uses unnamed parameter followed by named parameter.");
+            return QString();
+        } else if (illegalNames.contains(param)) {
+            if (errorString)
+                *errorString = QCoreApplication::translate("QQmlRewrite", "Signal parameter \"%1\" hides global variable.").arg(QString::fromUtf8(param));
+            return QString();
+        }
+        parameters += QString::fromUtf8(param);
+    }
+
+    return parameters;
+}
+
 // Returns an array of the arguments for method \a index.  The first entry in the array
 // is the number of arguments.
 int *QQmlPropertyCache::methodParameterTypes(QObject *object, int index,
diff --git a/src/qml/qml/qqmlpropertycache_p.h b/src/qml/qml/qqmlpropertycache_p.h
index 1f9199cd6d..e70c89ced2 100644
--- a/src/qml/qml/qqmlpropertycache_p.h
+++ b/src/qml/qml/qqmlpropertycache_p.h
@@ -314,6 +314,8 @@ public:
     static int originalClone(QObject *, int index);
 
     QList<QByteArray> signalParameterNames(int index) const;
+    QString signalParameterStringForJS(int index, QString *errorString = 0);
+    static QString signalParameterStringForJS(QQmlEngine *engine, const QList<QByteArray> &parameterNameList, QString *errorString = 0);
 
     const char *className() const;
 
diff --git a/src/qml/qml/qqmlvme.cpp b/src/qml/qml/qqmlvme.cpp
index f7390794ea..e4a996fac6 100644
--- a/src/qml/qml/qqmlvme.cpp
+++ b/src/qml/qml/qqmlvme.cpp
@@ -759,7 +759,8 @@ QObject *QQmlVME::run(QList<QQmlError> *errors,
                 new QQmlBoundSignalExpression(target, instr.signalIndex,
                                               CTXT, context, PRIMITIVES.at(instr.value),
                                               COMP->name, instr.line, instr.column,
-                                              PRIMITIVES.at(instr.handlerName));
+                                              PRIMITIVES.at(instr.handlerName),
+                                              PRIMITIVES.at(instr.parameters));
             bs->takeExpression(expr);
         QML_END_INSTR(StoreSignal)