Repository navigation
Keep the comments around a list pattern's only spread when formatting - #9127
Merged
Merged
Conversation
Why:
`list{...r}` parses to the pattern `r` itself, but the parser gave that
pattern the location of the whole `list{...}`. A comment around the
spread was then inside the location of the leaf `r`, where comment
attachment leaves it unattached, so formatting dropped it. The pattern
now keeps the location of `r`, like `list{...e}` in expressions, and the
comments become leading or trailing comments of `r`.
Format modified (comment around a list pattern's only spread):
Example:
let f = x =>
switch x {
| list{/* d */ ...r} => r
}
let g = (list{...r /* t */}) => r
Before (rescript):
let f = x =>
switch x {
| r => r
}
let g = r => r
After (rescript):
let f = x =>
switch x {
/* d */ | r => r
}
let g = r /* t */ => r
Review: Why is the comment printed before `|`? That is where the formatter
already puts a leading comment of a case pattern, as for `| /* d */ r`.
Fixes: #9123
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kbf3gHnoR8Goys4AMxET4q
Signed-off-by: Christoph Knittel <ck@cca.io>
Changelog🐛 Bug fix
Semantic Diff1 commit · Full page Keep the comments around a list pattern's only spread when formatting939f26b · cknitt + Claude · Oct 29, 2026, 17:59 UTC
Format modified (comment around a list pattern's only spread) let f = x =>
switch x {
| list{/* d */ ...r} => r
}
let g = (list{...r /* t */}) => r
let f = x =>
switch x {
- | r => r
+ /* d */ | r => r
}
-let g = r => r
+let g = r /* t */ => rFixes: #9123 Review
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9127 +/- ##
=======================================
Coverage 81.71% 81.71%
=======================================
Files 460 460
Lines 64761 64762 +1
=======================================
+ Hits 52918 52919 +1
Misses 11843 11843
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9127 |
cknitt
marked this pull request as ready for review
October 29, 2026 19:48
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cknitt
enabled auto-merge (squash)
October 29, 2026 19:56
cristianoc
approved these changes
Oct 29, 2026
cristianoc
left a comment
cristianoc
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me. Thanks for the fix.
This was referenced Oct 30, 2026
cknitt
added a commit
that referenced
this pull request
Oct 30, 2026
…ment pattern snapshot (#9137) Add the blank line before a multi-line let to the comment pattern snapshot #9127 added list-pattern cases to printer/comments/pattern.res, and its snapshot was made before #8961, which puts a blank line around multi-line module items, was merged. Together they print a blank line before `let f = x => switch x {…}`, and the syntax test of master has failed on it since #9127. Claude-Session: https://claude.ai/code/session_01Kbf3gLpf7RN1MiRcubPgRn Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
aspeddro
pushed a commit
to aspeddro/rescript-compiler
that referenced
this pull request
Oct 31, 2026
…rescript-lang#9127) Why: `list{...r}` parses to the pattern `r` itself, but the parser gave that pattern the location of the whole `list{...}`. A comment around the spread was then inside the location of the leaf `r`, where comment attachment leaves it unattached, so formatting dropped it. The pattern now keeps the location of `r`, like `list{...e}` in expressions, and the comments become leading or trailing comments of `r`. Format modified (comment around a list pattern's only spread): Example: let f = x => switch x { | list{/* d */ ...r} => r } let g = (list{...r /* t */}) => r Before (rescript): let f = x => switch x { | r => r } let g = r => r After (rescript): let f = x => switch x { /* d */ | r => r } let g = r /* t */ => r Review: Why is the comment printed before `|`? That is where the formatter already puts a leading comment of a case pattern, as for `| /* d */ r`. Fixes: rescript-lang#9123 Claude-Session: https://claude.ai/code/session_01Kbf3gHnoR8Goys4AMxET4q Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Pedro Castro <aspeddro@gmail.com>
aspeddro
pushed a commit
to aspeddro/rescript-compiler
that referenced
this pull request
Oct 31, 2026
…ts to the comment pattern snapshot (rescript-lang#9137) Add the blank line before a multi-line let to the comment pattern snapshot rescript-lang#9127 added list-pattern cases to printer/comments/pattern.res, and its snapshot was made before rescript-lang#8961, which puts a blank line around multi-line module items, was merged. Together they print a blank line before `let f = x => switch x {…}`, and the syntax test of master has failed on it since rescript-lang#9127. Claude-Session: https://claude.ai/code/session_01Kbf3gLpf7RN1MiRcubPgRn Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Pedro Castro <aspeddro@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
list{...r}parses to the patternritself, but the parser gave that pattern the location of the wholelist{...}. A comment around the spread therefore lay inside the location of the leafr, where comment attachment leaves it unattached, and formatting dropped it:was formatted as
| r => r.The parser now keeps the location of
rfor a list pattern that is only a spread, as it already does forlist{...e}in expressions, so the comments become leading or trailing comments ofr. The output is/* d */ | r => r, which is where the formatter already puts a leading comment of a case pattern (| /* d */ r).Tests: new cases in
tests/syntax_tests/data/printer/comments/pattern.resfor a comment before and after the spread, several comments, a nestedlist{...list{...r}}, and the spread inlet, function parameter andswitchpatterns.Fixes #9123
🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gHnoR8Goys4AMxET4q