Formatter drops comments in dict patterns, variant spread patterns and dict literal spreads
Maintainers usually reply within 1 day
A pull request for this has already been merged.
- #8850 by @cknitt — merged
Assessment
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Newbie friendliness
- 35/100
Research direction
Start with res_comments_table.ml and res_printer.ml at the locations linked in the issue, then review the comment fixtures under tests/syntax_tests/data/printer/comments/. Reproduce with the provided cm.res command and add fixture cases for dict patterns and variant spread patterns. Done means formatting preserves every listed comment and the fixtures catch losses on the first print.
Written by the indexing model from the issue text.
Description
Found by adversarial experiments on semantic analysts.
What is broken
The formatter deletes comments in these positions:
- Dict pattern key:
| dict{"a" /* k */: b}→| dict{"a": b} - Dict pattern value:
| dict{"a": /* c */ a}→| dict{"a": a}(the record pattern{a: /* c */ a}keeps its comment) - Empty dict pattern:
| dict{/* e */}→| dict{} - Variant spread pattern:
| .../* c */ t→| ...t - Polymorphic variant type pattern:
| #.../* c */ t→| #...t - Dict literal with a spread: an own-line comment before the first row after a spread is dropped (
/* x */on the same line is kept)
Reproduction
cm.res:
let f = d =>
switch d {
| dict{"a" /* k */: b} => b
| dict{"a": /* c */ a} => a
| dict{/* e */} => 0
| _ => 1
}
type t = A | B
type u = | ...t | C
let g = x =>
switch x {
| .../* c */ t => 0
| #.../* c */ t => 0
| C => 1
}
let b = dict{
...base,
// first
"a": 1,
}
./_build/default/compiler/syntax/cli/res_cli.exe cm.res
Observed: every comment above is missing from the output. Expected: each comment is printed.
Where
Ppat_typeandPpat_variant_spreadattach no comments:
https://github.com/rescript-lang/rescript/blob/8d51b9ce7/compiler/syntax/src/res_comments_table.ml#L2056- Dict pattern rows are walked as record rows, so key comments are attached to the key location:
https://github.com/rescript-lang/rescript/blob/8d51b9ce7/compiler/syntax/src/res_comments_table.ml#L2057-L2063 print_pattern_dict_rowprints the key withprint_longident, which does not consult the comment table. The record row printer usesprint_lident_path ... cmt_tbl.
https://github.com/rescript-lang/rescript/blob/8d51b9ce7/compiler/syntax/src/res_printer.ml#L3051-L3076- The
Ppat_dictcase has noprint_comments_inside:
https://github.com/rescript-lang/rescript/blob/8d51b9ce7/compiler/syntax/src/res_printer.ml#L2845-L2864 - In a dict literal with spreads, each run of rows becomes a synthetic expression. Leading comments attach to that expression (https://github.com/rescript-lang/rescript/blob/8d51b9ce7/compiler/syntax/src/res_comments_table.ml#L1402-L1410), but
print_spread_dict_exprprints the run without printing them (https://github.com/rescript-lang/rescript/blob/8d51b9ce7/compiler/syntax/src/res_printer.ml#L1782-L1786). This cause is from reading the code.
No comment fixture covers dict patterns or variant spread patterns (tests/syntax_tests/data/printer/comments/). The roundtrip test compares the first formatting with the second, so a comment lost on the first print passes. The fix should add these cases to the comment fixtures.
Introduced by
The same output is produced at ece8b148, before the dict pattern and variant spread nodes existed. The current code comes from b081cd19c (#8749) and bffd18e7b (#8742).
Impact
Running the formatter silently deletes user comments.
- Dominant language
- ReScript
- Stars
- 7.5k
- Forks
- 483
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 164
Getting set up
Starts the project's dev container in your browser, under your own GitHub account.
- No Dockerfile or Docker Compose file
- Has a pull request template
- Read the contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from rescript-lang/rescript
-
Difficulty 4/5 3-5 days Newbie friendliness 48/100
rescript-lang/rescript#8938 ·
Maintainers usually reply within 1 day
-
discussion enhancement
Difficulty 5/5 Over a week Newbie friendliness 35/100
rescript-lang/rescript#8934 · 1 reaction ·
Maintainers usually reply within 1 day
-
Partial application moves its arguments into the generated closurePossibly taken @cknitt claimed this 1 day ago. Open
Difficulty 4/5 3-5 days Newbie friendliness 25/100
rescript-lang/rescript#8932 ·
Maintainers usually reply within 1 day
-
Difficulty 5/5 Over a week Newbie friendliness 25/100
rescript-lang/rescript#8910 ·
Maintainers usually reply within 1 day
-
Nested JSX components under React Server Components: derive flat exports from the signaturePossibly taken @cknitt claimed this 3 days ago. Open
Difficulty 5/5 Over a week Newbie friendliness 25/100
rescript-lang/rescript#8901 ·
Maintainers usually reply within 1 day
All issues in rescript-lang/rescript
Similar issues
-
good first issue hacktoberfest help wanted python
Difficulty 2/5 1-3 hours Newbie friendliness 73/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 77/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
maester365/maester#2482 ·
Maintainers usually reply within 1 day
-
Difficulty 1/5 Under an hour Newbie friendliness 88/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 63/100
Maintainers usually reply within 1 day