Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-123386

QmlFormat. Incorrect handling of some comments

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: P3: Somewhat important P3: Somewhat important
    • 6.11
    • 6.7
    • QML: Tooling
    • None
    • ccc830877 (6.8), a3863b2d8 (dev), 240d2d190 (dev), bc6b4940a (dev), d8a1e840e (6.10), a46ee0635 (6.10), 70ab48778 (6.10), fcd85f201 (6.9), e4cbd81ca (6.9), 6436e8716 (6.9), 68fca3511 (tqtc/lts-6.8), 3848ec04e (tqtc/lts-6.8), ca263ec76 (tqtc/lts-6.8)

      Problems

      Briefly here are the problems:

      1. Comments around MethodParameters are lost. In both "standalone" ES and when ES is used inside QmlObject
      2. Inproper formatting when Comment is between the function keyword and the functionIdentifier (not a common case though, but there might be more common problems from that set or when it manifests it spoils the code)
      3. Commented Body of the Method or Class results in a loss or duplication and wrong placement in the "standalone" ES (relatively bad, but not really a common usecase maybe?), but in case of QmlObject a \n character is added (not bad, but just weird, haven't seen smth similar).
      4. If a Body of the item is commented, it moved up. (almost non-important, but for the sake of completeness of the observation and I personally haven't seen similar formatter behaviors)

      For standalone ES, Prettier (https://prettier.io/) can be used for references. 

      High level architecture (Current State) and Issues

      See https://qtgroup.atlassian.net/wiki/x/F4BdDg 

      Some Dragons with Comment attachment

      1. We probably don't need to Link comments to the `DomItem,FileLocationRegion`-s and to `AST::Node, offset` and can probably work just with SourceLocation-s
      2. CommentLinker lacks encapsulation / modularity. For example CommentLinker takes in a constructor a reference to an integer (lastPostCommentPostEnd) which is used and modified during the linkCommentWithElement, however that integer variable outlives the lifetime of the CommentLinker.[1].
        Another opportunity for encapsulation and modularity is the way CommentLinker stores intermediately processed information.[2]
      3. findSpacesAroundComment is not tested and it's not clear whether there might be a cleaner/more straightforward approach.
      4. The whole algorithm of CommentLinker::linkCommentWithElement is not documented, not covered by unit tests. The high level order is not really clear, but lower level function like checkElementAfterComment, checkElementBeforeComment, checkElementInside are hard to debug, reason about and make changes (if you find it otherwise, pls reach out to me and help me to figure it out). There is no knowledge about the corner cases, but there are many questions whether there could be a leaner solution.

      Refactoring attempts / inspo

      1. Refactor CommentCollector API and introduce more modularity https://codereview.qt-project.org/c/qt/qtdeclarative/+/551774
      2. More "one step at a time" style refactoring of CommentCollector https://codereview.qt-project.org/c/qt/qtdeclarative/+/552710/1
      3. Rough attempt to introduce unit tests and figure out what's going on https://codereview.qt-project.org/c/qt/qtdeclarative/+/552703

      Notes / References

      [1] see the usage of lastPostCommentPostEnd and m_lastPostCommentPostEnd

      [2] Even though some of the coupling most likely imposed by the complexity of how ASTRangesVisitor deals with ranges of "Elements", but it's still might be beneficial to separate handling / storing Comment-specific Info (like CommentLocation, m_spaces) and m_ranges. Currently everything is initialised altogether in the CommentLinker constructor. For me personally it's also quite hard to have Iterators initialized at the constructor stage...

        For Gerrit Dashboard: QTBUG-123386
        # Subject Branch Project Status CR V

            qtqmlteam Qt Qml Team User
            dima.a Dmitrii Akshintsev
            Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:

                There are 5 open Gerrit changes