FWD » Core Development » Conversion Tools

This task is about improving/fixing comments in general. There will generally be 2 causes:

1. orphaned comments where we cannot calculate an attachment point for a given comment
2. comments that are placed in the wrong location

Both sets of problems should be resolved here.

We should start by collecting testcases (both successful and failing). I will link related customer bug reports here. Those need to be "mined" for failing testcases.

I created 3711a and checked in some improvements to comments conversion. I hit the comments conversion issues while working on #10539.

Limitations and gaps¶

The testcases are attached, cmt_stale_*.p for the stale peerid and tree reload problem, cmt_removed_*.p for the comments of removed code.

Dropping of comments of removed code¶

The rule drops a comment when its code was removed from the tree. It has these problems:

• It treats every Progress node removed during the conversion as dead code, not only unreachable CASE branches. The VALUE target of GET-KEY-VALUE is removed by language_statements.rules, so the end-of-line comment of a live GET-KEY-VALUE statement is dropped (cmt_removed_get_key_value.p).
• The code a comment belongs to is picked wrong in some cases:
  • end case. /* c */ after a dead last WHEN is dropped. END and CASE are hidden tokens, so the node in front of the comment is the last node of the dead branch (cmt_removed_case_after_dead.p).
  • An own-line comment in front of an end. is classified by the code after the end.. In a nested CASE a comment of a live branch followed by a dead outer WHEN is dropped, and a comment at the end of a dead outer branch followed by a live WHEN is kept (cmt_removed_case_nested.p).
• The expression exclusion (a node rewritten inside a live expression doesn't count as removed code) can't be reached. Folding runs in the annotations, before the tree is persisted, so the folded operands are missing in the persisted copy too and getRemovedRoot finds nothing.

Placement¶

• The ancestor walk of getPeer can end high up. For a NO-ERROR statement directly in the main block the closest ancestor with a live peer is the main block, so its comments are emitted in front of externalProcedure(...) or after the closing brace of the block body (cmt_stale_assign_no_error.p).
• In a plain DO block the end-of-line comment of a NO-ERROR statement is emitted after the block, followed by a stray ; (cmt_stale_do_block_no_error.p).
• An end-of-line comment of the last live WHEN in front of a dead WHEN is emitted in front of the whole switch (cmt_removed_case_int_eol.p, cmt_removed_case_position.p, cmt_removed_case_nested.p).
• The placement patterns of the existing comments.rules logic are untouched and apply to the restored comments too. From a sample of 128 restored comments of the big GUI customer project about 11% are misplaced:
  • end-of-line comments of parameter, temp-table field and prototype definitions piled in the lambda header (new Block((Body) () /* ... */ /* ... */ ->) or on a later closing brace
  • main block comments emitted above unrelated class fields
  • procedure headers and the first comments of a body emitted after the method, reading as the header of the next method
  • end-of-line comments of the last statement of a block pushed after the closing brace, often as /* ... */;
  • the end-of-line comment of a variable definition moved in front of the next variable
  • the end-of-line comment of the last statement in front of a function emitted as a /** */ javadoc of the function

Comments still orphaned¶

Comments of code removed in the annotations (statements after RETURN or LEAVE, if false then, do while false, the operands of a folded concatenation) are still orphaned, as in trunk. The code is missing in the persisted copy of the tree too, so there is nothing to attach the comment to (cmt_removed_unreachable_other.p, cmt_removed_expr_fold.p, cmt_stale_dead_code.p, cmt_stale_folded_concat.p). The big GUI customer project still reports 27,395 orphaned comments.

Coverage¶

• The null guard of the javadoc peer lookup in comments.rules can't be triggered by generic code. It needs a PROCEDURE or FUNCTION keyword whose parent carries a stale peerid, and these parents (PROCEDURE, FUNCTION, SUBSCRIBE, the root block) never lose their peer.
• The getAst() guard for a Progress id of a processed tree is not reached from comments.rules any more, all its Progress lookups go through getPersistedAst() or getAst(id, false). It still protects the other rule sets.
• The dropping of comments of removed code was tested on 13 files of the big GUI customer project and the testcases only, not in a full conversion.

Scope¶

• The getAst() guard covers the trees being processed only (the source, copy and target AST). A rule which calls getAst() for a stale id of another tree still gets the persisted copy, and its changes to that copy are lost (#10850).
• The fix tolerates stale ids, it doesn't remove them. They come from:
  • NO-ERROR statements (RUN, ASSIGN, method calls) in the main block, procedures, functions and triggers: beautification.rules drops the single statement silent(() -> ...) wrapper after it was built, the STATEMENT keeps the peerid of the dropped Java node
  • repeated FIND NEXT or FIND PREV of the same buffer in a loop, the record phrase of the second FIND points to a removed Java node
  • Progress nodes removed during the conversion (unreachable CASE branches, the GET-KEY-VALUE target, CREATE VALUE(...))
• CREATE VALUE("button") h. fails the conversion with an NPE in AstSymbolResolver.getAst() (null artifact) when i18n is enabled, in trunk and in the branch.

Further improvements¶

• Restrict the dropping of comments of removed code to removed CASE branches (a removed KW_WHEN, KW_THEN or KW_OTHERWISE under a live KW_CASE). Take the code of an end-of-line comment from the statement the comment ends, including end case., and the code of an own-line comment in front of an end. from the enclosing block.
• Drop the unreachable expression exclusion, or keep the folded operands in the persisted tree if it is ever needed.
• Use the peer of the first live child of a statement with a stale peerid before climbing to its ancestors, so the comments of a NO-ERROR statement stay next to it.
• Clear or update the peerid annotation where a rule removes or replaces the Java node, starting with the silent(...) unwrapping in beautification.rules and the record phrase of the repeated FIND. Report stale peerids at conversion time, the FINE log of AstSymbolResolver lists them.
• Record the position of Progress nodes removed in the annotations (e.g. an artificial marker node), so their comments can be dropped with the code or attached to the enclosing block instead of being orphaned.
• Fix the general getAst() copy problem as proposed in #10850: make an AST returned from a reloaded tree read-only and report any change to it.
• Fix the placement patterns, see #3711 and its related issues: attach the end-of-line comments of definitions to the generated declaration or DMO field, keep the main block comments inside execute(), attach procedure headers in front of their method, keep the end-of-line comment of the last statement of a block inside the block.
• Fix the CREATE VALUE(...) NPE.
• Add the testcases to the conversion regression tests, comparing the emitted comments and their placement, not only the code.