qmlformat: don't make comments jump around functions

Add more comment anchors around functions to avoid comments from
"jumping through" parenthesis, the "function" keyword or through
commas.

Add missing sourcelocations to AST::FunctionExpression in the parser
for arrow functions. This requires building the FunctionExpression*
early so that it can access the location of the parentheses.
Add the comma location in FormalParameterList in the parser.

This allows qmlformat to properly attach comments to commas, function
keywords, parentheses in lambdas and arrow functions instead of making
the comments "jump" through them.

Fix a domcomparison method to actually ignore parentheses during
qmlformat checks: this is because qmlformat tends to remove parentheses
of lambdas like `(x) => x` that become `x => x`. Now that we actually
save the location of the parentheses, the domcomparison method sees
that the parentheses disappear and complains wrongfully. Therefore,
ignore the newly added parentheses sourcelocations when doing
qmlformat checks. We have plenty of other tests that make sure the
parentheses are the way they should be.

Pick-to: 6.9 6.8
Task-number: QTBUG-123386
Change-Id: I08a3536cfb27fa1cdc2a78cc9849f609a8113a07
Reviewed-by: Ulf Hermann <[email protected]>

8 files changed, 128 insertions, 40 deletions

diff --git a/src/qml/parser/qqmljs.g b/src/qml/parser/qqmljs.g
index 94f54417ea..a825c01092 100644
--- a/src/qml/parser/qqmljs.g
+++ b/src/qml/parser/qqmljs.g
@@ -188,6 +188,7 @@ public:
       AST::Finally *Finally;
       AST::FormalParameterList *FormalParameterList;
       AST::FunctionDeclaration *FunctionDeclaration;
+      AST::FunctionExpression *FunctionExpression;
       AST::Node *Node;
       AST::PropertyName *PropertyName;
       AST::Statement *Statement;
@@ -1777,7 +1778,11 @@ CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN Expression_In T_RPAR
 CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN T_RPAREN;
 /.
     case $rule_number: {
-        sym(1).Node = nullptr;
+        AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), nullptr, nullptr);
+        f->functionToken = loc(1).startZeroLengthLocation();
+        f->lparenToken = loc(1);
+        f->rparenToken = loc(2);
+        sym(1).Node = f;
         coverExpressionErrorLocation = loc(2);
         coverExpressionType = CE_FormalParameterList;
     } break;
@@ -1786,8 +1791,12 @@ CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN T_RPAREN;
 CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN BindingRestElement T_RPAREN;
 /.
     case $rule_number: {
-        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(2).PatternElement))->finish(pool);
-        sym(1).Node = node;
+        AST::FormalParameterList *list = (new (pool) AST::FormalParameterList(nullptr, sym(2).PatternElement))->finish(pool);
+        AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), list, nullptr);
+        f->functionToken = loc(1).startZeroLengthLocation();
+        f->lparenToken = loc(1);
+        f->rparenToken = loc(3);
+        sym(1).FunctionExpression = f;
         coverExpressionErrorLocation = loc(2);
         coverExpressionType = CE_FormalParameterList;
     } break;
@@ -1801,12 +1810,18 @@ CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN Expression_In T_COMM
             syntaxError(loc(1), "Invalid Arrow parameter list.");
             return false;
         }
+        list->commaToken = loc(3);
         if (sym(4).Node) {
             list = new (pool) AST::FormalParameterList(list, sym(4).PatternElement);
         }
+
+        AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), list->finish(pool), nullptr);
+        f->functionToken = loc(1).startZeroLengthLocation();
+        f->lparenToken = loc(1);
+        f->rparenToken = loc(5);
         coverExpressionErrorLocation = loc(4);
         coverExpressionType = CE_FormalParameterList;
-        sym(1).Node = list->finish(pool);
+        sym(1).FunctionExpression = f;
     } break;
 ./
 
@@ -4076,6 +4091,7 @@ FormalParameterList: BindingElement;
 FormalParameterList: FormalParameterList T_COMMA BindingElement;
 /.
     case $rule_number: {
+        sym(1).FormalParameterList->commaToken = loc(2);
         AST::FormalParameterList *node = new (pool) AST::FormalParameterList(sym(1).FormalParameterList, sym(3).PatternElement);
         sym(1).Node = node;
     } break;
@@ -4109,9 +4125,9 @@ ArrowFunction_In: ArrowParameters T_ARROW ConciseBodyLookahead AssignmentExpress
         ret->returnToken = sym(4).Node->firstSourceLocation().startZeroLengthLocation();
         ret->semicolonToken = sym(4).Node->lastSourceLocation().endZeroLengthLocation(driver->code());
         AST::StatementList *statements = (new (pool) AST::StatementList(ret))->finish();
-        AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), sym(1).FormalParameterList, statements);
+        AST::FunctionExpression *f = sym(1).FunctionExpression;
+        f->body = statements;
         f->isArrowFunction = true;
-        f->functionToken = sym(1).Node ? sym(1).Node->firstSourceLocation().startZeroLengthLocation() : loc(1).startZeroLengthLocation();
         f->lbraceToken = sym(4).Node->firstSourceLocation().startZeroLengthLocation();
         f->rbraceToken = sym(4).Node->lastSourceLocation().endZeroLengthLocation(driver->code());
         sym(1).Node = f;
@@ -4123,9 +4139,9 @@ ArrowFunction: ArrowParameters T_ARROW ConciseBodyLookahead T_FORCE_BLOCK Functi
 ArrowFunction_In: ArrowParameters T_ARROW ConciseBodyLookahead T_FORCE_BLOCK FunctionLBrace FunctionBody FunctionRBrace;
 /.
     case $rule_number: {
-        AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), sym(1).FormalParameterList, sym(6).StatementList);
+        AST::FunctionExpression *f = sym(1).FunctionExpression;
+        f->body = sym(6).StatementList;
         f->isArrowFunction = true;
-        f->functionToken = sym(1).Node ? sym(1).Node->firstSourceLocation().startZeroLengthLocation() : loc(1).startZeroLengthLocation();
         f->lbraceToken = loc(5);
         f->rbraceToken = loc(7);
         sym(1).Node = f;
@@ -4137,7 +4153,10 @@ ArrowParameters: BindingIdentifier;
     case $rule_number: {
         AST::PatternElement *e = new (pool) AST::PatternElement(stringRef(1), /*type annotation*/nullptr, nullptr, AST::PatternElement::Binding);
         e->identifierToken = loc(1);
-        sym(1).FormalParameterList = (new (pool) AST::FormalParameterList(nullptr, e))->finish(pool);
+        AST::FormalParameterList *list = (new (pool) AST::FormalParameterList(nullptr, e))->finish(pool);
+        AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), list, nullptr);
+        f->functionToken = loc(1).startZeroLengthLocation();
+        sym(1).FunctionExpression = f;
     } break;
 ./
 
@@ -4153,7 +4172,11 @@ ArrowParameters: CoverParenthesizedExpressionAndArrowParameterList;
                 syntaxError(loc(1), "Invalid Arrow parameter list.");
                 return false;
             }
-            sym(1).Node = list->finish(pool);
+            AST::FunctionExpression *f = new (pool) AST::FunctionExpression(QStringView(), list->finish(pool), nullptr);
+            f->functionToken = loc(1).startZeroLengthLocation();
+            f->lparenToken = ne->lparenToken;
+            f->rparenToken = ne->rparenToken;
+            sym(1).FunctionExpression = f;
         }
     } break;
 ./
diff --git a/src/qml/parser/qqmljsast.cpp b/src/qml/parser/qqmljsast.cpp
index 6dc3812e84..f20fe016ff 100644
--- a/src/qml/parser/qqmljsast.cpp
+++ b/src/qml/parser/qqmljsast.cpp
@@ -134,6 +134,7 @@ FormalParameterList *ExpressionNode::reparseAsFormalParameterList(MemoryPool *po
         if (!f)
             return nullptr;
 
+        f->commaToken = commaExpr->commaToken;
         expr = commaExpr->right;
     }
 
@@ -158,6 +159,7 @@ FormalParameterList *ExpressionNode::reparseAsFormalParameterList(MemoryPool *po
     }
     if (!binding)
         return nullptr;
+
     return new (pool) AST::FormalParameterList(f, binding);
 }
 
diff --git a/src/qml/parser/qqmljsast_p.h b/src/qml/parser/qqmljsast_p.h
index 0a4793a05a..a6a0f3a5c0 100644
--- a/src/qml/parser/qqmljsast_p.h
+++ b/src/qml/parser/qqmljsast_p.h
@@ -2522,6 +2522,7 @@ public:
 // attributes
     PatternElement *element = nullptr;
     FormalParameterList *next;
+    SourceLocation commaToken;
 };
 
 class QML_PARSER_EXPORT ClassExpression : public ExpressionNode
diff --git a/src/qmldom/qqmldomastdumper.cpp b/src/qmldom/qqmldomastdumper.cpp
index c56bf4bbc8..afe9412fd7 100644
--- a/src/qmldom/qqmldomastdumper.cpp
+++ b/src/qmldom/qqmldomastdumper.cpp
@@ -849,15 +849,21 @@ public:
     void endVisit(AST::FunctionDeclaration *) override { stop(u"FunctionDeclaration"); }
 
     bool visit(AST::FunctionExpression *el) override {
+
+        QString parentheses = options & AstDumperOption::NoLocations
+                ? QLatin1String()
+                : QLatin1String(" lparenToken=%1 rparenToken=%2")
+                          .arg(loc(el->lparenToken), loc(el->rparenToken));
+
         start(QLatin1String("FunctionExpression name=%1 isArrowFunction=%2 isGenerator=%3 "
                             "functionToken=%4 "
-                            "identifierToken=%5 lparenToken=%6 rparenToken=%7 lbraceToken=%8 "
-                            "rbraceToken=%9")
+                            "identifierToken=%5%6 lbraceToken=%7 "
+                            "rbraceToken=%8")
                       .arg(quotedString(el->name), boolStr(el->isArrowFunction),
                            boolStr(el->isGenerator),
                            loc(el->functionToken, options & AstDumperOption::SloppyCompare),
-                           loc(el->identifierToken), loc(el->lparenToken), loc(el->rparenToken),
-                           loc(el->lbraceToken), loc(el->rbraceToken)));
+                           loc(el->identifierToken), parentheses, loc(el->lbraceToken),
+                           loc(el->rbraceToken)));
         return true;
     }
     void endVisit(AST::FunctionExpression *) override { stop(u"FunctionExpression"); }
diff --git a/src/qmldom/qqmldomcomments.cpp b/src/qmldom/qqmldomcomments.cpp
index ff9893a54d..26d9ee15fd 100644
--- a/src/qmldom/qqmldomcomments.cpp
+++ b/src/qmldom/qqmldomcomments.cpp
@@ -402,6 +402,33 @@ public:
         return true;
     }
 
+    bool visit(FormalParameterList *list) override
+    {
+        if (!list->commaToken.isValid())
+            return true;
+
+        // Comments are first attached to some previous element, if possible. Therefore, attach
+        // comments in FormalParameterList to comments so that they don't "jump over commas" during
+        // formatting.
+        addSourceLocations(list, list->commaToken);
+        return true;
+    }
+
+    bool visit(FunctionExpression *fExpr) override
+    {
+        addSourceLocations(fExpr, fExpr->identifierToken);
+        addSourceLocations(fExpr, fExpr->lparenToken);
+        addSourceLocations(fExpr, fExpr->rparenToken);
+        addSourceLocations(fExpr, fExpr->lbraceToken);
+        addSourceLocations(fExpr, fExpr->rbraceToken);
+        return true;
+    }
+
+    bool visit(FunctionDeclaration *fExpr) override
+    {
+        return visit(static_cast<FunctionExpression *>(fExpr));
+    }
+
     QMap<qsizetype, ElementRef> starts;
     QMap<qsizetype, ElementRef> ends;
 };
@@ -672,6 +699,10 @@ private:
         } else {
             --preStart;
         }
+        if (m_endElement == m_ranges.ends.end()) {
+            m_commentedElement = preStart.value();
+            return;
+        }
         // we are inside a node, actually inside both n1 and n2 (which might be the same)
         // add to pre of the smallest between n1 and n2.
         // This is needed because if there are multiple nodes starting/ending at the same
diff --git a/src/qmldom/qqmldomreformatter.cpp b/src/qmldom/qqmldomreformatter.cpp
index 313dd17875..9576e3c8cb 100644
--- a/src/qmldom/qqmldomreformatter.cpp
+++ b/src/qmldom/qqmldomreformatter.cpp
@@ -791,29 +791,25 @@ bool ScriptFormatter::visit(FunctionExpression *ast)
             out("function");
             lw.lineWriter.ensureSpace();
         }
-        if (!ast->name.isNull())
-            out(ast->identifierToken);
+        outWithComments(ast->identifierToken, ast);
     }
-    out(ast->lparenToken);
-    const bool needParentheses = ast->formals
-            && (ast->formals->next
-                || (ast->formals->element && ast->formals->element->bindingTarget));
-    if (ast->isArrowFunction && needParentheses)
-        out("(");
+
+    const bool removeParentheses = ast->isArrowFunction && ast->formals && !ast->formals->next
+            && (ast->formals->element && !ast->formals->element->bindingTarget);
+
+    // note: qmlformat removes the parentheses for "(x) => x". In that case, we still need
+    // to print potential comments attached to `(` or `)` via `OnlyComments` option.
+    outWithComments(ast->lparenToken, ast, removeParentheses ? OnlyComments : NoSpace);
     int baseIndent = lw.increaseIndent(1);
     accept(ast->formals);
     lw.decreaseIndent(1, baseIndent);
-    if (ast->isArrowFunction && needParentheses)
-        out(")");
-    out(ast->rparenToken);
-    if (ast->isArrowFunction && !ast->formals)
-        out("()");
+    outWithComments(ast->rparenToken, ast, removeParentheses ? OnlyComments : NoSpace);
     lw.lineWriter.ensureSpace();
     if (ast->isArrowFunction) {
         out("=>");
         lw.lineWriter.ensureSpace();
     }
-    out(ast->lbraceToken);
+    outWithComments(ast->lbraceToken, ast);
     if (ast->lbraceToken.length != 0)
         ++expressionDepth;
     if (ast->body) {
@@ -829,7 +825,7 @@ bool ScriptFormatter::visit(FunctionExpression *ast)
     }
     if (ast->lbraceToken.length != 0)
         --expressionDepth;
-    out(ast->rbraceToken);
+    outWithComments(ast->rbraceToken, ast);
     return false;
 }
 
@@ -922,15 +918,8 @@ bool ScriptFormatter::visit(CaseClauses *ast)
 bool ScriptFormatter::visit(FormalParameterList *ast)
 {
     for (FormalParameterList *it = ast; it; it = it->next) {
-        // compare FormalParameterList::finish
-        if (auto id = it->element->bindingIdentifier.toString(); !id.isEmpty())
-            out(id);
-        if (it->element->bindingTarget)
-            accept(it->element->bindingTarget);
-        if (it->next) {
-            out(",");
-            lw.lineWriter.ensureSpace();
-        }
+        accept(it->element);
+        outWithComments(it->commaToken, it, SpaceBeforePostComment);
     }
     return false;
 }
diff --git a/src/qmldom/qqmldomreformatter_p.h b/src/qmldom/qqmldomreformatter_p.h
index 8fabc2a237..538a8bce0d 100644
--- a/src/qmldom/qqmldomreformatter_p.h
+++ b/src/qmldom/qqmldomreformatter_p.h
@@ -46,12 +46,18 @@ protected:
         if (loc.length != 0)
             out(loc2Str(loc));
     }
-    void outWithComments(const SourceLocation &loc, AST::Node *node)
+    enum CommentOption { NoSpace, SpaceBeforePostComment, OnlyComments };
+    void outWithComments(const SourceLocation &loc, AST::Node *node, CommentOption option = NoSpace)
     {
+        if (!loc.isValid())
+            return;
         const CommentedElement *c = comments->commentForNode(node, CommentAnchor::from(loc));
         if (c)
             c->writePre(lw, nullptr);
-        out(loc);
+        if (option != OnlyComments)
+            out(loc);
+        if (option == SpaceBeforePostComment)
+            lw.ensureSpace();
         if (c)
             c->writePost(lw, nullptr);
     }
diff --git a/tests/auto/qmldom/reformatter/tst_reformatter.h b/tests/auto/qmldom/reformatter/tst_reformatter.h
index 4360464c61..afa37e8c3a 100644
--- a/tests/auto/qmldom/reformatter/tst_reformatter.h
+++ b/tests/auto/qmldom/reformatter/tst_reformatter.h
@@ -807,6 +807,36 @@ private slots:
                                         << QStringLiteral(u"/*1*/class /*2*/ A/*3*/ {/*4*/}/*5*/");
         QTest::newRow("classBody") << QStringLiteral(u"/*1*/class/*2*/ A/*3*/{/*4*/constructor(){}/*5*/}/*6*/")
                                    << QStringLiteral(u"/*1*/class /*2*/ A/*3*/ {/*4*/\nconstructor() {}/*5*/\n}/*6*/");
+
+        QTest::newRow("AroundEmptyFunction")
+                << u"/*1*/function/*2*/ a/*3*/(/*4*/a/*5*/,/*6*/b/*7*/)/*8*/{/*9*/}/*10*/"_s
+                << u"/*1*/function /*2*/ a/*3*/(/*4*/a/*5*/, /*6*/b/*7*/)/*8*/ {/*9*/}/*10*/"_s;
+        QTest::newRow("AroundFunction")
+                << u"/*1*/function/*2*/ a/*3*/(/*4*/a/*5*/,/*6*/b/*7*/)/*8*/{/*9*/ "
+                   u"return 42 /*10*/}/*11*/"_s
+                << u"/*1*/function /*2*/ a/*3*/(/*4*/a/*5*/, /*6*/b/*7*/)/*8*/ "
+                   u"{/*9*/\nreturn 42; /*10*/\n}/*11*/"_s;
+        QTest::newRow("AroundFunctionDouble")
+                << u"/*1a*//*1b*/function/*2a*//*2b*/ "
+                   u"a/*3a*//*3b*/(/*4a*//*4b*/a/*5a*//*5b*/,/*6a*//*6b*/b/*7a*//*7b*/)/"
+                   u"*8a*//*8b*/{/*9a*//*9b*/ return 42 /*10a*//*10b*/}/*11a*//*11b*/"_s
+                << u"/*1a*//*1b*/function /*2a*//*2b*/ "
+                   u"a/*3a*//*3b*/(/*4a*//*4b*/a/*5a*//*5b*/, "
+                   u"/*6a*//*6b*/b/*7a*//*7b*/)/*8a*//*8b*/ {/*9a*//*9b*/\nreturn "
+                   u"42; /*10a*//*10b*/\n}/*11a*//*11b*/"_s;
+
+        QTest::newRow("AroundAnonymous")
+                << u"const x = /*1*/function/*2*/(/*3*/a/*4*/,/*5*/b/*6*/)/*7*/{/*8*/ "
+                   u"return 42 /*9*/}/*10*/"_s
+                << u"const x = /*1*/function /*2*/(/*3*/a/*4*/, /*5*/b/*6*/)/*7*/ {/*8*/\nreturn 42; /*9*/\n}/*10*/"_s;
+
+        QTest::newRow("AroundArrowFunction")
+                << u"const x = /*1*/(/*2*/a/*3*/,/*4*/b/*5*/)/*6*/ => /*7*/ {/*8*/ return 42 /*9*/}/*10*/"_s
+                << u"const x = /*1*/(/*2*/a/*3*/, /*4*/b/*5*/)/*6*/ => /*7*/ {/*8*/\nreturn 42; /*9*/\n}/*10*/"_s;
+
+        QTest::newRow("AroundArrowFunction2")
+                << u"const x = /*1*/(/*2*/a/*3*/, /*4*/b/*5*/)/*6*/ => /*7*/42/*8*/"_s
+                << u"const x = /*1*/(/*2*/a/*3*/, /*4*/b/*5*/)/*6*/ => /*7*/42/*8*/"_s;
     }
 
     void comments()