reasonml / reasonml/reason

Remaining issues in comments interleaving

Open
#664 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

KIND: BUG Printer
Dominant language
OCaml
Stars
10.3k
Forks
438
PR merge metrics
No merged PRs in 30d

Description

This issue is for documenting some known issues that we've found in our new comments interleaving logic.

  • Formatting some code with comments are not idempotent
let a = IList.partition /* comment */ (fun | X => 1 | y => 2)

is not idempotent.

The problem here is that : in this expression, the keyword "fun" has a wrongly estimated SourceMap location (the estimated location begins at the start of the expression, which is the location of "(").

  • Numeric literals don't have a location for comments to be attached to: #399
  • Shrink then enlarge formatting width problem :

This is a problem exists in theory but we haven't been able to produce yet.

One invariant we want to maintain is when the user changes width of formatting and changes it back ,the resulted code still look the same:

format(format(format(src, x), y), x) == format(src, x)

We have different logic of formatting for different types of comments:

  1. end-of-line comments are currently being formatted as end-of-line comments no matter how the width change.
  2. we also want to prevent an inline comment form being formatted to an end-of-line comment. Since if an inline-comment somehow becomes end-of-line comment when shrink we shrink the width, it would stay as end-of-line comment even when we increase the width later, therefore violating the invariant.

Consider this fake example:

let a () /* inline comment */ => 1;

If we shrink the width, we may get:

let a () /* inline comment */
    => 1;

At this time, the comment has become an end-of-line comment, and will stay as an end-of-line comment in our algorithm, thus violates the invariant.

The above example is just an illustration of how this issue would happen. It is not the case in reality. In reality, the token " =>" and "()" will always be formatted on the same line.

To solve this issue in theory, we would need another invariant:

If a node in our PrintTree meets the following condition:
  the node doesn't have SourceMap location info attached to it (mostly likely it doesn't show up in the AST, in this case, "=>"). 
then we need to make sure:
  If there exists a formatted code `c` where the node is placed on the right side of another node on the same line, with any possible formatting width `x`, formatting `c` on `x` will yield formatted code where the two nodes are *still on the same line*. 

This invariants basically says if there is a node where comment cannot be attached to, the node will stick to the node on the left side of it. Thus, if we add an inline comment between these two nodes, it is guaranteed that the the inline comment will stay as an inline comment.

Currently I don't find any case in our PrintTree that would violate this variant. But it is possible.

  • Comments being formatted into "{}"

This code below:

class stackWithAttributes init => 
/* Before class */
{
  method a = 1;
};

is formatted as:

class stackWithAttributes init => {
  /* Before class */
  method a = 1;
};

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading the comments interleaving logic, including its SourceMap handling and PrintTree invariants, then reproduce the marked examples and the numeric-literal case in #399. Done means the listed comment-placement problems are fixed and formatting remains idempotent when widths change and change back.

Written by the indexing model from the issue text.

Assessment

Tech stack
ocaml
Domain
tooling
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.