Handle function expressions as signal handlers

There are two ways to use function expressions on the right-hand side
of bindings:

  property var somethingPressed
  somethingPressed: function() { /* ..press something else.. */ }

  signal buttonPressed
  onButtonPressed: function() { /* ..handle buttonPress.. */ }

In the former case, it declares a property that holds a function. So on
initialization, the right-hand side of the binding returns a closure
that gets assigned to the property 'somethingPressed'.

In the latter case, the signal handler is explicitly marked as a
function for clarity. So, the handler should not be returning the
closure, but the handler should *be* the closure.

In general, it is not possible to detect if the left-hand side is a
property or a signal handler when generating QML cache files ahead of
time. So for this case, we mark the function as only returning a
closure. Then when instantiating the object, we check if it is a signal
handler, and if the handler is marked as only returning a closure. If
so, we set that closure to be the signal handler.

This patch also handles function expressions in SignalTransition and
function expressions as signal handlers.

Task-number: QTBUG-57043
Task-number: QTBUG-50328
Task-number: QTBUG-50328
(cherry picked from commit 22b13921f8067f8a93164875a4ad59bed85b0400)
(cherry picked from commit dc4d6293f9473c0f03c570430d08867d2d01c6e2)
(cherry picked from commit 21301c1dbb00f4a2cd991e520423ed039b297ffb)
Change-Id: I3008ddd847e30b7d0adef07344a326f84d85f1ba
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

10 files changed, 45 insertions, 9 deletions

diff --git a/src/imports/statemachine/signaltransition.cpp b/src/imports/statemachine/signaltransition.cpp
index 0f88ec641b..7057fd0621 100644
--- a/src/imports/statemachine/signaltransition.cpp
+++ b/src/imports/statemachine/signaltransition.cpp
@@ -176,9 +176,10 @@ void SignalTransition::connectTriggered()
     QMetaMethod metaMethod = target->metaObject()->method(qobjectSignal->methodIndex());
     int signalIndex = QMetaObjectPrivate::signalIndex(metaMethod);
 
-    QQmlBoundSignalExpression *expression = ctxtdata ?
-                new QQmlBoundSignalExpression(target, signalIndex,
-                                              ctxtdata, this, m_compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex]) : 0;
+    auto f = m_compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex];
+    QQmlBoundSignalExpression *expression =
+            ctxtdata ? new QQmlBoundSignalExpression(target, signalIndex, ctxtdata, this, f)
+                     : nullptr;
     if (expression)
         expression->setNotifyOnValueChanged(false);
     m_signalExpression = expression;
diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index b07e9f55f5..b609a06382 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -2102,6 +2102,15 @@ int Codegen::defineFunction(const QString &name, AST::Node *ast,
     IR::BasicBlock *exitBlock = function->newBasicBlock(0, IR::Function::DontInsertBlock);
     function->hasDirectEval = _env->hasDirectEval || _env->compilationMode == EvalCode
             || _module->debugMode; // Conditional breakpoints are like eval in the function
+
+    // When a user writes the following QML signal binding:
+    //    onSignal: function() { doSomethingUsefull }
+    // we will generate a binding function that just returns the closure. However, that's not useful
+    // at all, because if the onSignal is a signal handler, the user is actually making it explicit
+    // that the binding is a function, so we should execute that. However, we don't know that during
+    // AOT compilation, so mark the surrounding function as only-returning-a-closure.
+    function->returnsClosure = cast<ExpressionStatement *>(ast) && cast<FunctionExpression *>(cast<ExpressionStatement *>(ast)->expression);
+
     function->usesArgumentsObject = _env->parent && (_env->usesArgumentsObject == Environment::ArgumentsObjectUsed);
     function->usesThis = _env->usesThis;
     function->maxNumberOfArguments = qMax(_env->maxNumberOfArguments, (int)QV4::Global::ReservedArgumentCount);
diff --git a/src/qml/compiler/qv4compileddata_p.h b/src/qml/compiler/qv4compileddata_p.h
index 188a571394..b15e4ade60 100644
--- a/src/qml/compiler/qv4compileddata_p.h
+++ b/src/qml/compiler/qv4compileddata_p.h
@@ -228,6 +228,7 @@ struct Function
     LEUInt32 nLocals;
     LEUInt32 localsOffset;
     LEUInt32 nInnerFunctions;
+    LEUInt32 nestedFunctionIndex; // for functions that only return a single closure, used in signal handlers
     Location location;
 
     // Qml Extensions Begin
@@ -248,6 +249,7 @@ struct Function
     quint8 flags;
     quint8 padding1;
     LEUInt16 padding2;
+    LEUInt32 padding3;
 
     const LEUInt32 *formalsTable() const { return reinterpret_cast<const LEUInt32 *>(reinterpret_cast<const char *>(this) + formalsOffset); }
     const LEUInt32 *localsTable() const { return reinterpret_cast<const LEUInt32 *>(reinterpret_cast<const char *>(this) + localsOffset); }
@@ -266,7 +268,7 @@ struct Function
         return (sizeof(Function) + (nFormals + nLocals + nInnerfunctions + nIdObjectDependencies + 2 * nPropertyDependencies) * sizeof(quint32) + 7) & ~0x7;
     }
 };
-static_assert(sizeof(Function) == 72, "Function structure needs to have the expected size to be binary compatible on disk when generated by host compiler and loaded by target");
+static_assert(sizeof(Function) == 80, "Function structure needs to have the expected size to be binary compatible on disk when generated by host compiler and loaded by target");
 
 // Qml data structures
 
diff --git a/src/qml/compiler/qv4compiler.cpp b/src/qml/compiler/qv4compiler.cpp
index 0dc40d9698..e11e0bbcdd 100644
--- a/src/qml/compiler/qv4compiler.cpp
+++ b/src/qml/compiler/qv4compiler.cpp
@@ -298,6 +298,9 @@ void QV4::Compiler::JSUnitGenerator::writeFunction(char *f, QV4::IR::Function *i
     if (irFunction->hasTry || irFunction->hasWith)
         function->flags |= CompiledData::Function::HasCatchOrWith;
     function->nFormals = irFunction->formals.size();
+    function->nestedFunctionIndex =
+            irFunction->returnsClosure ? quint32(irModule->functions.indexOf(irFunction->nestedFunctions.first()))
+                                       : std::numeric_limits<uint32_t>::max();
     function->formalsOffset = currentOffset;
     currentOffset += function->nFormals * sizeof(quint32);
 
diff --git a/src/qml/compiler/qv4jsir.cpp b/src/qml/compiler/qv4jsir.cpp
index a4038f827a..7741a64667 100644
--- a/src/qml/compiler/qv4jsir.cpp
+++ b/src/qml/compiler/qv4jsir.cpp
@@ -374,6 +374,7 @@ Function::Function(Module *module, Function *outer, const QString &name)
     , hasTry(false)
     , hasWith(false)
     , isQmlBinding(false)
+    , returnsClosure(false)
     , unused(0)
     , line(0)
     , column(0)
diff --git a/src/qml/compiler/qv4jsir_p.h b/src/qml/compiler/qv4jsir_p.h
index 317abd09bb..35b6abaa58 100644
--- a/src/qml/compiler/qv4jsir_p.h
+++ b/src/qml/compiler/qv4jsir_p.h
@@ -1313,7 +1313,8 @@ struct Function {
     uint hasTry: 1;
     uint hasWith: 1;
     uint isQmlBinding: 1;
-    uint unused : 24;
+    uint returnsClosure: 1;
+    uint unused : 23;
 
     // Location of declaration in source code (0 if not specified)
     uint line;
diff --git a/src/qml/jsruntime/qv4function_p.h b/src/qml/jsruntime/qv4function_p.h
index b212b3d027..fb8f1400a2 100644
--- a/src/qml/jsruntime/qv4function_p.h
+++ b/src/qml/jsruntime/qv4function_p.h
@@ -50,6 +50,7 @@
 // We mean it.
 //
 
+#include <stdint.h>
 #include "qv4global_p.h"
 #include <private/qqmlglobal_p.h>
 #include <private/qv4compileddata_p.h>
@@ -100,6 +101,12 @@ struct Q_QML_EXPORT Function {
         return QQmlSourceLocation(sourceFile(), compiledFunction->location.line, compiledFunction->location.column);
     }
 
+    Function *nestedFunction() const
+    {
+        if (compiledFunction->nestedFunctionIndex == std::numeric_limits<uint32_t>::max())
+            return nullptr;
+        return compilationUnit->runtimeFunctions[compiledFunction->nestedFunctionIndex];
+    }
 };
 
 
diff --git a/src/qml/qml/qqmlboundsignal.cpp b/src/qml/qml/qqmlboundsignal.cpp
index 19ece44beb..efe0cccc8d 100644
--- a/src/qml/qml/qqmlboundsignal.cpp
+++ b/src/qml/qml/qqmlboundsignal.cpp
@@ -110,6 +110,12 @@ QQmlBoundSignalExpression::QQmlBoundSignalExpression(QObject *target, int index,
       m_index(index),
       m_target(target)
 {
+    // If the function is marked as having a nested function, then the user wrote:
+    //   onSomeSignal: function() { /*....*/ }
+    // So take that nested function:
+    if (auto closure = function->nestedFunction())
+        function = closure;
+
     setupFunction(scope, function);
     init(ctxt, scopeObject);
 }
@@ -122,6 +128,12 @@ QQmlBoundSignalExpression::QQmlBoundSignalExpression(QObject *target, int index,
     // It's important to call init first, because m_index gets remapped in case of cloned signals.
     init(ctxt, scope);
 
+    // If the function is marked as having a nested function, then the user wrote:
+    //   onSomeSignal: function() { /*....*/ }
+    // So take that nested function:
+    if (auto closure = runtimeFunction->nestedFunction())
+        runtimeFunction = closure;
+
     QV4::ExecutionEngine *engine = QQmlEnginePrivate::getV4Engine(ctxt->engine);
 
     QList<QByteArray> signalParameters = QMetaObjectPrivate::signal(m_target->metaObject(), m_index).parameterNames();
diff --git a/src/qml/types/qqmlconnections.cpp b/src/qml/types/qqmlconnections.cpp
index 7607c19374..2554dcf2b6 100644
--- a/src/qml/types/qqmlconnections.cpp
+++ b/src/qml/types/qqmlconnections.cpp
@@ -289,9 +289,10 @@ void QQmlConnections::connectSignals()
                 new QQmlBoundSignal(target, signalIndex, this, qmlEngine(this));
             signal->setEnabled(d->enabled);
 
-            QQmlBoundSignalExpression *expression = ctxtdata ?
-                new QQmlBoundSignalExpression(target, signalIndex,
-                                              ctxtdata, this, d->compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex]) : 0;
+            auto f = d->compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex];
+            QQmlBoundSignalExpression *expression =
+                    ctxtdata ? new QQmlBoundSignalExpression(target, signalIndex, ctxtdata, this, f)
+                             : nullptr;
             signal->takeExpression(expression);
             d->boundsignals += signal;
         } else {
diff --git a/tests/auto/qmltest/statemachine/tst_triggeredArguments2.qml b/tests/auto/qmltest/statemachine/tst_triggeredArguments2.qml
index 73bc653404..f23cc14152 100644
--- a/tests/auto/qmltest/statemachine/tst_triggeredArguments2.qml
+++ b/tests/auto/qmltest/statemachine/tst_triggeredArguments2.qml
@@ -67,7 +67,6 @@ TestCase {
 
         // Emit the signalTrans.signal
         testCase.mysignal("test1", true, 2)
-        expectFail("", "QTBUG-50328")
         compare(testCase.mystr, "test1")
         compare(testCase.mybool, true)
         compare(testCase.myint, 2)