qiurui
/
qtdeclarative
mirror of https://github.com/qt/qtdeclarative.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

qmlformat: add custom comment anchoring for case 

Add a way to customize where comments are anchored. Prior to this
commit, comments could only be anchored to an AST::Node
firstSourceLocation() or lastSourceLocation().

This commit extends the current functionality to be able to anchor
comments somewhere between firstSourceLocation() and
lastSourceLocation().

This allows to fix the bug where comments would be printed before the
":" in case statements, because they would get attached to the
expression inside the case-statement, instead of getting attached to
the colon of the case-statement.

Extend AstRangesVisitor to add the sourcelocation of the ":" in case-
statements to the list of sourcelocations where comments can be
attached to.

The anchors enum is not meant to be extended: instead, an anchor is
either DefaultAnchor for comments attached to AST::Node*, or
token.begin() for some QQmlJS::SourceLocation token inside an
AST::Node*.

Pick-to: 6.9 6.8 6.5
Fixes: QTBUG-132886
Change-Id: I0a18f09b3631b8798b6697298e532b47ddc7cb20
Reviewed-by: Dmitrii Akshintsev <dmitrii.akshintsev@qt.io>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
This commit is contained in:
Sami Shalayel 2025-01-20 14:29:58 +01:00
parent 30f030d03d
commit b91ca3e0bc
7 changed files with 177 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
src/qmldom/qqmldomcomments.cpp
View File

@ -276,23 +276,34 @@ public:
FileLocationRegion regionName;
};
class NodeRef
{
public:
AST::Node *node = nullptr;
CommentAnchor commentAnchor;
};
// internal class to keep a reference either to an AST::Node* or a region of a DomItem and the
// size of that region
class ElementRef
{
public:
ElementRef(AST::Node *node, qsizetype size) : element(node), size(size) { }
ElementRef(AST::Node *node, qsizetype size, CommentAnchor commentAnchor)
: element(NodeRef{ node, commentAnchor }), size(size)
{
}
ElementRef(const Path &path, FileLocationRegion region, qsizetype size)
: element(RegionRef{ path, region }), size(size)
{
}
operator bool() const
{
return (element.index() == 0 && std::get<0>(element)) || element.index() == 1 || size != 0;
return (element.index() == 0 && std::get<0>(element).node) || element.index() == 1
|| size != 0;
}
ElementRef() = default;
std::variant<AST::Node *, RegionRef> element;
std::variant<NodeRef, RegionRef> element;
qsizetype size = 0;
};
@ -346,19 +357,40 @@ public:
static const QSet<int> kindsToSkip();
static bool shouldSkipRegion(const DomItem &item, FileLocationRegion region);
void addSourceLocations(Node *n, qsizetype start, qsizetype end, CommentAnchor commentAnchor)
{
if (!starts.contains(start))
starts.insert(start, { n, end - start, commentAnchor });
if (!ends.contains(end))
ends.insert(end, { n, end - start, commentAnchor });
}
void addSourceLocations(Node *n)
{
addSourceLocations(n, n->firstSourceLocation().begin(), n->lastSourceLocation().end(),
CommentAnchor{});
}
void addSourceLocations(Node *n, const SourceLocation &sourceLocation)
{
addSourceLocations(n, sourceLocation.begin(), sourceLocation.end(),
CommentAnchor::from(sourceLocation));
}
bool preVisit(Node *n) override
{
if (!kindsToSkip().contains(n->kind)) {
qsizetype start = n->firstSourceLocation().begin();
qsizetype end = n->lastSourceLocation().end();
if (!starts.contains(start))
starts.insert(start, { n, end - start });
if (!ends.contains(end))
ends.insert(end, { n, end - start });
addSourceLocations(n);
}
return true;
}
using VisitAll::visit;
bool visit(CaseClause *caseClause) override
{
// special case: case clauses can have comments attached to their `:` token
addSourceLocations(caseClause, caseClause->colonToken);
return true;
}
QMap<qsizetype, ElementRef> starts;
QMap<qsizetype, ElementRef> ends;
};
@ -662,12 +694,8 @@ bool AstComments::iterateDirectSubpaths(const DomItem &self, DirectVisitor visit
{
// TODO: QTBUG-123645
// Revert this commit to reproduce crash with tst_qmldomitem::doNotCrashAtAstComments
QList<Comment> pre;
QList<Comment> post;
for (const auto &commentedElement : commentedElements().values()) {
pre.append(commentedElement.preComments());
post.append(commentedElement.postComments());
}
auto [pre, post] = collectPreAndPostComments();
if (!pre.isEmpty())
self.dvWrapField(visitor, Fields::preComments, pre);
if (!post.isEmpty())
@ -725,15 +753,17 @@ void CommentCollector::collectComments(
elementToBeLinked.element = RegionRef{ Path(), MainRegion };
elementToBeLinked.size = FileLocations::region(m_fileLocations, MainRegion).length;
} else if (rootNode) {
elementToBeLinked.element = rootNode;
elementToBeLinked.element = NodeRef{ rootNode, CommentAnchor{} };
elementToBeLinked.size = rootNode->lastSourceLocation().end() - rootNode->firstSourceLocation().begin();
}
}
if (const auto *const commentNode = std::get_if<AST::Node *>(&elementToBeLinked.element)) {
auto &commentedElement = astComments->commentedElements()[*commentNode];
commentedElement.addComment(comment);
} else if (const auto * const regionRef = std::get_if<RegionRef>(&elementToBeLinked.element)) {
if (const auto *const commentNode = std::get_if<NodeRef>(&elementToBeLinked.element)) {
auto commentedElement = astComments->ensureCommentForNode(commentNode->node,
commentNode->commentAnchor);
commentedElement->addComment(comment);
} else if (const auto *const regionRef =
std::get_if<RegionRef>(&elementToBeLinked.element)) {
DomItem currentItem = m_rootItem.item().path(regionRef->path);
MutableDomItem regionComments = currentItem.field(Fields::comments);
if (auto *regionCommentsPtr = regionComments.mutableAs<RegionComments>()) {

58
src/qmldom/qqmldomcomments_p.h
View File

@ -72,6 +72,29 @@ public:
QQmlJS::SourceLocation commentLocation;
};
/*!
\internal
Anchor comments at an AST::Node* (DefaultLocation) or at an QQmlJS::SourceLocation inside of the
AST::Node*. In the latter case, the member location takes the value of the anchor's
QQmlJS::SourceLocation::offset, so that the formatter can differentiate between multiple anchors
inside the same AST::Node*.
*/
struct CommentAnchor
{
qsizetype location = -1;
static CommentAnchor from(const SourceLocation &sl) { return CommentAnchor{ sl.begin() }; }
friend bool comparesEqual(const CommentAnchor &a, const CommentAnchor &b) noexcept
{
return a.location == b.location;
}
Q_DECLARE_EQUALITY_COMPARABLE(CommentAnchor)
};
inline size_t qHash(const CommentAnchor &key, size_t seed = 0) noexcept
{
return qHashMulti(seed, key.location);
}
class QMLDOM_EXPORT Comment
{
public:
@ -140,8 +163,8 @@ public:
m_postComments.append(comment);
}
const QList<Comment> &preComments() const { return m_preComments;}
const QList<Comment> &postComments() const { return m_postComments;}
QList<Comment> preComments() const { return m_preComments; }
QList<Comment> postComments() const { return m_postComments; }
private:
QList<Comment> m_preComments;
@ -177,7 +200,7 @@ public:
private:
Path addPreComment(const Comment &comment, FileLocationRegion region)
{
auto &preList = m_regionComments[region].preComments();
const auto preList = m_regionComments[region].preComments();
index_type idx = preList.size();
m_regionComments[region].addComment(comment);
return Path::fromField(Fields::regionComments)
@ -188,7 +211,7 @@ private:
Path addPostComment(const Comment &comment, FileLocationRegion region)
{
auto &postList = m_regionComments[region].postComments();
const auto postList = m_regionComments[region].postComments();
index_type idx = postList.size();
m_regionComments[region].addComment(comment);
return Path::fromField(Fields::regionComments)
@ -208,6 +231,8 @@ protected:
return std::make_shared<AstComments>(*this);
}
using CommentKey = std::pair<AST::Node *, CommentAnchor>;
public:
constexpr static DomType kindValue = DomType::AstComments;
DomType kind() const override { return kindValue; }
@ -224,27 +249,34 @@ public:
{
}
const QHash<AST::Node *, CommentedElement> &commentedElements() const
const CommentedElement *commentForNode(AST::Node *n, CommentAnchor location) const
{
return m_commentedElements;
const auto it = m_commentedElements.constFind(CommentKey{ n, location });
return it == m_commentedElements.end() ? nullptr : &*it;
}
QHash<AST::Node *, CommentedElement> &commentedElements()
std::pair<QList<Comment>, QList<Comment>> collectPreAndPostComments() const
{
return m_commentedElements;
QList<Comment> pre;
QList<Comment> post;
for (const auto &element : m_commentedElements) {
pre.append(element.preComments());
post.append(element.postComments());
}
return std::make_pair(pre, post);
}
CommentedElement *commentForNode(AST::Node *n)
CommentedElement *ensureCommentForNode(AST::Node *n, CommentAnchor location)
{
if (m_commentedElements.contains(n))
return &(m_commentedElements[n]);
return nullptr;
const CommentKey key{ n, location };
return &m_commentedElements[key];
}
QMultiMap<quint32, const QList<Comment> *> allCommentsInNode(AST::Node *n);
private:
std::shared_ptr<Engine> m_engine;
QHash<AST::Node *, CommentedElement> m_commentedElements;
QHash<CommentKey, CommentedElement> m_commentedElements;
};
class CommentCollector

10
src/qmldom/qqmldomreformatter.cpp
View File

@ -22,7 +22,7 @@ using namespace AST;
bool ScriptFormatter::preVisit(Node *n)
{
if (CommentedElement *c = comments->commentForNode(n)) {
if (const CommentedElement *c = comments->commentForNode(n, CommentAnchor{})) {
c->writePre(lw);
postOps[n].append([c, this]() { c->writePost(lw); });
}
@ -716,7 +716,7 @@ bool ScriptFormatter::visit(CaseClause *ast)
out("case"); // ast->caseToken
lw.lineWriter.ensureSpace();
accept(ast->expression);
out(ast->colonToken);
outWithComments(ast->colonToken, ast);
if (ast->statements)
lnAcceptIndented(ast->statements);
return false;
@ -875,8 +875,10 @@ bool ScriptFormatter::visit(StatementList *ast)
// If any of those are present they will take care of
// handling the spacing between the statements so we
// don't need to push any newline.
auto *commentForCurrentStatement = comments->commentForNode(it->statement);
auto *commentForNextStatement = comments->commentForNode(it->next->statement);
auto *commentForCurrentStatement =
comments->ensureCommentForNode(it->statement, CommentAnchor{});
auto *commentForNextStatement =
comments->ensureCommentForNode(it->next->statement, CommentAnchor{});
if (
(commentForCurrentStatement && !commentForCurrentStatement->postComments().empty())

9
src/qmldom/qqmldomreformatter_p.h
View File

@ -46,6 +46,15 @@ protected:
if (loc.length != 0)
out(loc2Str(loc));
}
void outWithComments(const SourceLocation &loc, AST::Node *node)
{
const CommentedElement *c = comments->commentForNode(node, CommentAnchor::from(loc));
if (c)
c->writePre(lw, nullptr);
out(loc);
if (c)
c->writePost(lw, nullptr);
}
inline void newLine(quint32 count = 1) { lw.ensureNewline(count); }
inline void accept(AST::Node *node) { AST::Node::accept(node, this); }

39
tests/auto/qml/qmlformat/data/caseWithComment.formatted.js Normal file
View File

@ -0,0 +1,39 @@
function f() {
switch (something) {
case 0: // Hello World
return;
case /**/ 1 /**/ :
return;
case /**/ 2:
return;
}
}
function g() {
switch (0) {
case /*1*/ 0 /*2*/:
}
switch (0) {
case 0:/*3*/
}
switch (0) {
case 0:/*4*/
return;
}
switch (0) {
case 0:/*5*/
return;
}
switch (0) {
case 0:/*6*/
return;
}
switch (0) {
case 0://7
return;
}
switch (0) {
case 0/*8*/:/*9*/
case 1:/*10*/
}
}

26
tests/auto/qml/qmlformat/data/caseWithComment.js Normal file
View File

@ -0,0 +1,26 @@
function f() {
switch (something) {
case 0: // Hello World
return
case /**/ 1 /**/ :
return
case /**/ 2 :
return
}
}
function g() {
switch(0){case /*1*/ 0 /*2*/:}
switch(0){case 0:/*3*/}
switch(0){case 0:/*4*/ return;}
switch(0){case 0:/*5*/
return}
switch(0){case 0:/*6*/
return;}
switch(0){case 0://7
return;
}
switch(0){case 0/*8*/:/*9*/ case 1:/*10*/}
}

2
tests/auto/qml/qmlformat/tst_qmlformat.cpp
View File

@ -530,6 +530,8 @@ void TestQmlformat::plainJS_data()
<< "noSuperfluousSpaceInsertions.fail_pragma.formatted.js";
QTest::newRow("fromAsIdentifier") << "fromAsIdentifier.js"
<< "fromAsIdentifier.formatted.js";
QTest::newRow("caseWithComment") << "caseWithComment.js"
<< "caseWithComment.formatted.js";
}
void TestQmlformat::plainJS()