Fix scoping of JavaScript function names

Function declarations add their name to the outer scope, but not the
inner scope. Function expressions add their name to the inner scope,
unless the name is actually picked from the outer scope rather than
given after the function token.

We don't add the name to any scope in the case of functions declared in
QML elements because the QML element will receive the function as
appropriately named, and typed, property. It is always better to use
that one than to use a JavaScript local.

This causes some additional ecmascript tests to pass.

Pick-to: 6.2
Fixes: QTBUG-96625
Change-Id: I0b8ee98917d102a99fb6b9bd918037c71867a4a5
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

5 files changed, 113 insertions, 18 deletions

diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index 9dec968a91..ee259b968b 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -382,8 +382,11 @@ bool ScanFunctions::visit(ExpressionStatement *ast)
         if (!_allowFuncDecls)
             _cg->throwSyntaxError(expr->functionToken, QStringLiteral("conditional function or closure declaration"));
 
-        if (!enterFunction(expr, /*enterName*/ true))
+        if (!enterFunction(expr, expr->identifierToken.length > 0
+                                ? FunctionNameContext::Inner
+                                : FunctionNameContext::None)) {
             return false;
+        }
         Node::accept(expr->formals, this);
         Node::accept(expr->body, this);
         leaveEnvironment();
@@ -399,7 +402,9 @@ bool ScanFunctions::visit(ExpressionStatement *ast)
 
 bool ScanFunctions::visit(FunctionExpression *ast)
 {
-    return enterFunction(ast, /*enterName*/ false);
+    return enterFunction(ast, ast->identifierToken.length > 0
+                                ? FunctionNameContext::Inner
+                                : FunctionNameContext::None);
 }
 
 bool ScanFunctions::visit(ClassExpression *ast)
@@ -496,12 +501,12 @@ bool ScanFunctions::visit(ArrayPattern *ast)
     return false;
 }
 
-bool ScanFunctions::enterFunction(FunctionExpression *ast, bool enterName)
+bool ScanFunctions::enterFunction(FunctionExpression *ast, FunctionNameContext nameContext)
 {
     Q_ASSERT(_context);
     if (_context->isStrict && (ast->name == QLatin1String("eval") || ast->name == QLatin1String("arguments")))
         _cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Function name may not be eval or arguments in strict mode"));
-    return enterFunction(ast, ast->name.toString(), ast->formals, ast->body, enterName);
+    return enterFunction(ast, ast->name.toString(), ast->formals, ast->body, nameContext);
 }
 
 void ScanFunctions::endVisit(FunctionExpression *)
@@ -536,7 +541,7 @@ void ScanFunctions::endVisit(PatternProperty *)
 
 bool ScanFunctions::visit(FunctionDeclaration *ast)
 {
-    return enterFunction(ast, /*enterName*/ true);
+    return enterFunction(ast, FunctionNameContext::Outer);
 }
 
 void ScanFunctions::endVisit(FunctionDeclaration *)
@@ -674,7 +679,9 @@ void ScanFunctions::endVisit(WithStatement *)
     leaveEnvironment();
 }
 
-bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParameterList *formals, StatementList *body, bool enterName)
+bool ScanFunctions::enterFunction(
+        Node *ast, const QString &name, FormalParameterList *formals, StatementList *body,
+        FunctionNameContext nameContext)
 {
     Context *outerContext = _context;
     enterEnvironment(ast, ContextType::Function, name);
@@ -685,7 +692,7 @@ bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParamete
     if (outerContext) {
         outerContext->hasNestedFunctions = true;
         // The identifier of a function expression cannot be referenced from the enclosing environment.
-        if (enterName) {
+        if (nameContext == FunctionNameContext::Outer) {
             if (!outerContext->addLocalVar(name, Context::FunctionDefinition, VariableScope::Var, expr)) {
                 _cg->throwSyntaxError(ast->firstSourceLocation(), QStringLiteral("Identifier %1 has already been declared").arg(name));
                 return false;
@@ -711,8 +718,10 @@ bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParamete
     }
 
 
-    if (!enterName && (!name.isEmpty() && (!formals || !formals->containsName(name))))
+    if (nameContext == FunctionNameContext::Inner
+            && (!name.isEmpty() && (!formals || !formals->containsName(name)))) {
         _context->addLocalVar(name, Context::ThisFunctionName, VariableScope::Var);
+    }
     _context->formals = formals;
 
     if (body && !_context->isStrict)
diff --git a/src/qml/compiler/qv4compilerscanfunctions_p.h b/src/qml/compiler/qv4compilerscanfunctions_p.h
index 0336398cac..073ee5c2a4 100644
--- a/src/qml/compiler/qv4compilerscanfunctions_p.h
+++ b/src/qml/compiler/qv4compilerscanfunctions_p.h
@@ -89,9 +89,19 @@ public:
     void leaveEnvironment();
 
     void enterQmlFunction(QQmlJS::AST::FunctionExpression *ast)
-    { enterFunction(ast, false); }
+    { enterFunction(ast, FunctionNameContext::None); }
 
 protected:
+    // Function declarations add their name to the outer scope, but not the
+    // inner scope. Function expressions add their name to the inner scope,
+    // unless the name is actually picked from the outer scope rather than
+    // given after the function token. QML functions don't add their name
+    // anywhere because the name is already recorded in the QML element.
+    // This enum is used to control the behavior of enterFunction().
+    enum class FunctionNameContext {
+        None, Inner, Outer
+    };
+
     using Visitor::visit;
     using Visitor::endVisit;
 
@@ -118,7 +128,8 @@ protected:
     bool visit(QQmlJS::AST::FieldMemberExpression *) override;
     bool visit(QQmlJS::AST::ArrayPattern *) override;
 
-    bool enterFunction(QQmlJS::AST::FunctionExpression *ast, bool enterName);
+    bool enterFunction(QQmlJS::AST::FunctionExpression *ast,
+                       FunctionNameContext nameContext);
 
     void endVisit(QQmlJS::AST::FunctionExpression *) override;
 
@@ -161,7 +172,7 @@ protected:
 protected:
     bool enterFunction(QQmlJS::AST::Node *ast, const QString &name,
                        QQmlJS::AST::FormalParameterList *formals,
-                       QQmlJS::AST::StatementList *body, bool enterName);
+                       QQmlJS::AST::StatementList *body, FunctionNameContext nameContext);
 
     void calcEscapingVariables();
 // fields:
diff --git a/tests/auto/qml/debugger/qv4debugger/tst_qv4debugger.cpp b/tests/auto/qml/debugger/qv4debugger/tst_qv4debugger.cpp
index 610665146b..dd4c28f1d2 100644
--- a/tests/auto/qml/debugger/qv4debugger/tst_qv4debugger.cpp
+++ b/tests/auto/qml/debugger/qv4debugger/tst_qv4debugger.cpp
@@ -489,7 +489,7 @@ void tst_qv4debugger::conditionalBreakPoint()
 
     QVERIFY(m_debuggerAgent->m_capturedScope.size() > 1);
     const TestAgent::NamedRefs &frame0 = m_debuggerAgent->m_capturedScope.at(0);
-    QCOMPARE(frame0.size(), 3);
+    QCOMPARE(frame0.size(), 2);
     QVERIFY(frame0.contains("i"));
     QCOMPARE(frame0.value("i").toInt(), 11);
 }
@@ -545,7 +545,7 @@ void tst_qv4debugger::readArguments()
     QVERIFY(m_debuggerAgent->m_wasPaused);
     QVERIFY(m_debuggerAgent->m_capturedScope.size() > 1);
     const TestAgent::NamedRefs &frame0 = m_debuggerAgent->m_capturedScope.at(0);
-    QCOMPARE(frame0.size(), 5);
+    QCOMPARE(frame0.size(), 4);
     QVERIFY(frame0.contains(QStringLiteral("a")));
     QCOMPARE(frame0.type(QStringLiteral("a")), QStringLiteral("number"));
     QCOMPARE(frame0.value(QStringLiteral("a")).toDouble(), 1.0);
@@ -568,7 +568,7 @@ void tst_qv4debugger::readComplicatedArguments()
     QVERIFY(m_debuggerAgent->m_wasPaused);
     QVERIFY(m_debuggerAgent->m_capturedScope.size() > 1);
     const TestAgent::NamedRefs &frame0 = m_debuggerAgent->m_capturedScope.at(0);
-    QCOMPARE(frame0.size(), 2);
+    QCOMPARE(frame0.size(), 1);
     QVERIFY(frame0.contains(QStringLiteral("a")));
     QCOMPARE(frame0.type(QStringLiteral("a")), QStringLiteral("number"));
     QCOMPARE(frame0.value(QStringLiteral("a")).toInt(), 12);
@@ -591,7 +591,7 @@ void tst_qv4debugger::readLocals()
     QVERIFY(m_debuggerAgent->m_wasPaused);
     QVERIFY(m_debuggerAgent->m_capturedScope.size() > 1);
     const TestAgent::NamedRefs &frame0 = m_debuggerAgent->m_capturedScope.at(0);
-    QCOMPARE(frame0.size(), 7); // locals and parameters
+    QCOMPARE(frame0.size(), 6); // locals and parameters
     QVERIFY(frame0.contains("c"));
     QCOMPARE(frame0.type("c"), QStringLiteral("number"));
     QCOMPARE(frame0.value("c").toDouble(), 3.0);
@@ -619,7 +619,7 @@ void tst_qv4debugger::readObject()
     QVERIFY(m_debuggerAgent->m_wasPaused);
     QVERIFY(m_debuggerAgent->m_capturedScope.size() > 1);
     const TestAgent::NamedRefs &frame0 = m_debuggerAgent->m_capturedScope.at(0);
-    QCOMPARE(frame0.size(), 3);
+    QCOMPARE(frame0.size(), 2);
     QVERIFY(frame0.contains("b"));
     QCOMPARE(frame0.type("b"), QStringLiteral("object"));
     QJsonObject b = frame0.rawValue("b");
@@ -680,7 +680,7 @@ void tst_qv4debugger::readContextInAllFrames()
 
     for (int i = 0; i < 12; ++i) {
         const TestAgent::NamedRefs &scope = m_debuggerAgent->m_capturedScope.at(i);
-        QCOMPARE(scope.size(), 3);
+        QCOMPARE(scope.size(), 2);
         QVERIFY(scope.contains("n"));
         QCOMPARE(scope.type("n"), QStringLiteral("number"));
         QCOMPARE(scope.value("n").toDouble(), i + 1.0);
diff --git a/tests/auto/qml/ecmascripttests/TestExpectations b/tests/auto/qml/ecmascripttests/TestExpectations
index 6b743ba433..82d80f7f03 100644
--- a/tests/auto/qml/ecmascripttests/TestExpectations
+++ b/tests/auto/qml/ecmascripttests/TestExpectations
@@ -600,7 +600,6 @@ language/statements/class/definition/methods-gen-yield-as-literal-property-name.
 language/statements/class/definition/methods-gen-yield-as-property-name.js fails
 language/statements/class/definition/methods-named-eval-arguments.js fails
 language/statements/class/definition/prototype-property.js fails
-language/statements/class/definition/setters-prop-desc.js fails
 language/statements/class/definition/setters-restricted-ids.js fails
 language/statements/class/definition/this-access-restriction-2.js fails
 language/statements/class/definition/this-access-restriction.js fails
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index f42086ea9f..83d7a5ee03 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -424,6 +424,8 @@ private slots:
     void optionalChainNull();
 
     void asCast();
+    void functionNameInFunctionScope();
+
 private:
 //    static void propertyVarWeakRefCallback(v8::Persistent<v8::Value> object, void* parameter);
     static void verifyContextLifetime(const QQmlRefPointer<QQmlContextData> &ctxt);
@@ -9874,6 +9876,80 @@ void tst_qqmlecmascript::asCast()
     QCOMPARE(qvariant_cast<QObject *>(root->property("rectangleAsRectangle")), rectangle);
 }
 
+void tst_qqmlecmascript::functionNameInFunctionScope()
+{
+    QJSEngine engine;
+    QJSValue result = engine.evaluate(R"(
+        var a = {};
+        var foo = function foo() {
+            return foo;
+        }
+        a.foo = foo();
+
+        function bar() {
+            bar = 2;
+        }
+        bar()
+        a.bar = bar;
+
+        var baz = function baz() {
+            baz = 3;
+        }
+        baz()
+        a.baz = baz;
+
+        var foo2 = function() {
+            return foo2;
+        }
+        a.foo2 = foo2();
+
+        var baz2 = function() {
+            baz2 = 3;
+        }
+        baz2()
+        a.baz2 = baz2;
+        a
+
+    )");
+
+    QVERIFY(!result.isError());
+    const QJSManagedValue m(result, &engine);
+
+    QVERIFY(m.property("foo").isCallable());
+    QCOMPARE(m.property("bar").toInt(), 2);
+    QVERIFY(m.property("baz").isCallable());
+    QVERIFY(m.property("foo2").isCallable());
+    QCOMPARE(m.property("baz2").toInt(), 3);
+
+    const QJSValue getterInClass = engine.evaluate(R"(
+        class Tester {
+            constructor () {
+                this.a = 1;
+                this.b = 1;
+            }
+
+            get sum() {
+                const sum = this.a + this.b;
+                return sum;
+            }
+        }
+    )");
+
+    QVERIFY(!getterInClass.isError());
+
+    const QJSValue innerName = engine.evaluate(R"(
+        const a = 2;
+        var b = function a() { return a };
+        ({a: a, b: b, c: b()})
+    )");
+
+    QVERIFY(!innerName.isError());
+    const QJSManagedValue m2(innerName, &engine);
+    QCOMPARE(m2.property("a").toInt(), 2);
+    QVERIFY(m2.property("b").isCallable());
+    QVERIFY(m2.property("c").isCallable());
+}
+
 QTEST_MAIN(tst_qqmlecmascript)
 
 #include "tst_qqmlecmascript.moc"