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.

Task-number: QTBUG-57043
Task-number: QTBUG-50328
Change-Id: I3008ddd847e30b7d0adef07344a326f84d85f1ba
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

8 files changed, 103 insertions, 5 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index bc4ca5d6f4..a262908960 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -2075,6 +2075,14 @@ int Codegen::defineFunction(const QString &name, AST::Node *ast,
 
     _context->hasDirectEval |= (_context->compilationMode == EvalCode || _context->compilationMode == GlobalCode || _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.
+    _context->returnsClosure = cast<ExpressionStatement *>(ast) && cast<FunctionExpression *>(cast<ExpressionStatement *>(ast)->expression);
+
     BytecodeGenerator bytecode(_context->line, _module->debugMode);
     BytecodeGenerator *savedBytecodeGenerator;
     savedBytecodeGenerator = bytecodeGenerator;
diff --git a/src/qml/compiler/qv4compileddata_p.h b/src/qml/compiler/qv4compileddata_p.h
index f1776f5772..e6c4225bbd 100644
--- a/src/qml/compiler/qv4compileddata_p.h
+++ b/src/qml/compiler/qv4compileddata_p.h
@@ -225,7 +225,7 @@ struct Function
     quint32_le localsOffset;
     quint32_le nLineNumbers;
     quint32_le lineNumberOffset;
-    quint32_le nInnerFunctions;
+    quint32_le nestedFunctionIndex; // for functions that only return a single closure, used in signal handlers
     quint32_le nRegisters;
     Location location;
 
diff --git a/src/qml/compiler/qv4compiler.cpp b/src/qml/compiler/qv4compiler.cpp
index f2e1f4a0de..ccc909c199 100644
--- a/src/qml/compiler/qv4compiler.cpp
+++ b/src/qml/compiler/qv4compiler.cpp
@@ -305,6 +305,9 @@ void QV4::Compiler::JSUnitGenerator::writeFunction(char *f, QV4::Compiler::Conte
         function->flags |= CompiledData::Function::IsStrict;
     if (irFunction->hasTry || irFunction->hasWith)
         function->flags |= CompiledData::Function::HasCatchOrWith;
+    function->nestedFunctionIndex =
+            irFunction->returnsClosure ? quint32(module->functions.indexOf(irFunction->nestedContexts.first()))
+                                       : std::numeric_limits<uint32_t>::max();
     function->nFormals = irFunction->arguments.size();
     function->formalsOffset = currentOffset;
     currentOffset += function->nFormals * sizeof(quint32);
@@ -317,7 +320,6 @@ void QV4::Compiler::JSUnitGenerator::writeFunction(char *f, QV4::Compiler::Conte
     function->lineNumberOffset = currentOffset;
     currentOffset += function->nLineNumbers * sizeof(CompiledData::CodeOffsetToLine);
 
-    function->nInnerFunctions = irFunction->nestedContexts.size();
 
     function->nRegisters = irFunction->registerCount;
 
diff --git a/src/qml/compiler/qv4compilercontext_p.h b/src/qml/compiler/qv4compilercontext_p.h
index a78a66db52..8fabf41c40 100644
--- a/src/qml/compiler/qv4compilercontext_p.h
+++ b/src/qml/compiler/qv4compilercontext_p.h
@@ -144,6 +144,7 @@ struct Context {
     bool usesThis = false;
     bool hasTry = false;
     bool hasWith = false;
+    bool returnsClosure = false;
     mutable bool argumentsCanEscape = false;
 
     enum UsesArgumentsObject {
diff --git a/src/qml/jsruntime/qv4function_p.h b/src/qml/jsruntime/qv4function_p.h
index 4c8c790ca7..59a94e5dde 100644
--- a/src/qml/jsruntime/qv4function_p.h
+++ b/src/qml/jsruntime/qv4function_p.h
@@ -105,6 +105,13 @@ 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/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index 7051fb51da..7c36f59035 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -879,6 +879,14 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
     if (binding->type == QV4::CompiledData::Binding::Type_Script || binding->containsTranslations()) {
         if (binding->flags & QV4::CompiledData::Binding::IsSignalHandlerExpression) {
             QV4::Function *runtimeFunction = compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex];
+
+            // When a user writes the following:
+            //    onSignal: function() { doSomethingUsefull }
+            // then do not run the binding that returns the closure, but run the closure
+            // instead.
+            if (auto closure = runtimeFunction->nestedFunction())
+                runtimeFunction = closure;
+
             int signalIndex = _propertyCache->methodIndexToSignalIndex(bindingProperty->coreIndex());
             QQmlBoundSignal *bs = new QQmlBoundSignal(_bindingTarget, signalIndex, _scopeObject, engine);
             QQmlBoundSignalExpression *expr = new QQmlBoundSignalExpression(_bindingTarget, signalIndex,
diff --git a/src/qml/types/qqmlconnections.cpp b/src/qml/types/qqmlconnections.cpp
index a43562a7b8..d1a7aa9b6f 100644
--- a/src/qml/types/qqmlconnections.cpp
+++ b/src/qml/types/qqmlconnections.cpp
@@ -288,9 +288,17 @@ 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]) : nullptr;
+            auto f = d->compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex];
+
+            // If the function is marked as having a nested function, then the user wrote:
+            //   onSomeSignal: function() { /*....*/ }
+            // So take that nested function:
+            if (auto closure = f->nestedFunction())
+                f = closure;
+
+            QQmlBoundSignalExpression *expression =
+                    ctxtdata ? new QQmlBoundSignalExpression(target, signalIndex, ctxtdata, this, f)
+                             : nullptr;
             signal->takeExpression(expression);
             d->boundsignals += signal;
         } else {
diff --git a/tests/auto/qml/qmlcachegen/tst_qmlcachegen.cpp b/tests/auto/qml/qmlcachegen/tst_qmlcachegen.cpp
index 2f5ff0022e..e36edca699 100644
--- a/tests/auto/qml/qmlcachegen/tst_qmlcachegen.cpp
+++ b/tests/auto/qml/qmlcachegen/tst_qmlcachegen.cpp
@@ -47,6 +47,7 @@ private slots:
     void signalHandlerParameters();
     void errorOnArgumentsInSignalHandler();
     void aheadOfTimeCompilation();
+    void functionExpressions();
 
     void workerScripts();
 };
@@ -292,6 +293,69 @@ void tst_qmlcachegen::workerScripts()
     QTRY_VERIFY(obj->property("success").toBool());
 }
 
+void tst_qmlcachegen::functionExpressions()
+{
+    QTemporaryDir tempDir;
+    QVERIFY(tempDir.isValid());
+
+    const auto writeTempFile = [&tempDir](const QString &fileName, const char *contents) {
+        QFile f(tempDir.path() + '/' + fileName);
+        const bool ok = f.open(QIODevice::WriteOnly | QIODevice::Truncate);
+        Q_ASSERT(ok);
+        f.write(contents);
+        return f.fileName();
+    };
+
+    const QString testFilePath = writeTempFile(
+                "test.qml",
+                "import QtQuick 2.0\n"
+                "Item {\n"
+                "    id: di\n"
+                "    \n"
+                "    property var f\n"
+                "    property bool f_called: false\n"
+                "    f : function() { f_called = true }\n"
+                "    \n"
+                "    signal g\n"
+                "    property bool g_handler_called: false\n"
+                "    onG: function() { g_handler_called = true }\n"
+                "    \n"
+                "    signal h(int i)\n"
+                "    property bool h_connections_handler_called: false\n"
+                "    Connections {\n"
+                "        target: di\n"
+                "        onH: function(magic) { h_connections_handler_called = (magic == 42)\n }\n"
+                "    }\n"
+                "    \n"
+                "    function runTest() { \n"
+                "        f()\n"
+                "        g()\n"
+                "        h(42)\n"
+                "    }\n"
+                "}");
+
+    QVERIFY(generateCache(testFilePath));
+
+    const QString cacheFilePath = testFilePath + QLatin1Char('c');
+    QVERIFY(QFile::exists(cacheFilePath));
+    QVERIFY(QFile::remove(testFilePath));
+
+    QQmlEngine engine;
+    CleanlyLoadingComponent component(&engine, QUrl::fromLocalFile(testFilePath));
+    QScopedPointer<QObject> obj(component.create());
+    QVERIFY(!obj.isNull());
+
+    QCOMPARE(obj->property("f_called").toBool(), false);
+    QCOMPARE(obj->property("g_handler_called").toBool(), false);
+    QCOMPARE(obj->property("h_connections_handler_called").toBool(), false);
+
+    QMetaObject::invokeMethod(obj.data(), "runTest");
+
+    QCOMPARE(obj->property("f_called").toBool(), true);
+    QCOMPARE(obj->property("g_handler_called").toBool(), true);
+    QCOMPARE(obj->property("h_connections_handler_called").toBool(), true);
+}
+
 QTEST_GUILESS_MAIN(tst_qmlcachegen)
 
 #include "tst_qmlcachegen.moc"