Skip to content

Remove obsolete attributes from the formatter's filters and merge them - #8740

Merged
cknitt merged 3 commits into
masterfrom
remove-obsolete-attribute-filters
Oct 8, 2026
Merged

cknitt merged 3 commits into
masterfrom
remove-obsolete-attribute-filters

Conversation

@cknitt

@cknitt cknitt commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Stacked on #8738.

The formatter keeps lists of internal attributes it doesn't print. Two of the entries were obsolete, and two of the lists disagreed:

  • res.ternary and JSX are only v0 wire markers now. The parser produces Pexp_ternary and Pexp_jsx_element, and the v0 bridge turns both markers back into those nodes. They're removed from is_parsing_attr, has_attributes and is_printable_attribute, like res.await in Represent await on module expressions with a Pmod_await node #8738.
  • is_printable_attribute and is_parsing_attr disagreed. That mismatch caused the @JSX review finding in Fix module expressions losing attributes, await or parens when formatting #8735: an attribute that was printed but not counted for parens. After removing the obsolete entries they differed only in res.patVariantSpread and res.dictPattern, which the parser only puts on patterns. The printable helpers are only used on expressions and module expressions, so they're now defined in terms of is_parsing_attr, leaving one list. filter_printable_attributes was the same as filter_parsing_attrs and is removed.

has_attributes stays separate because of its res.iflet / @warning("-4") rule; it only loses res.ternary.

Behavior

Tests

  • printer/expr/markerAttributes.res: @JSX and @res.ternary written in source, with @foo for comparison, on operands of binary operators, pipes, unary operators, field and array access, calls, ternaries, arrays and records.

🤖 Generated with Claude Code

cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt added this pull request to stack #8739 October 8, 2026 06:47
@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.97%. Comparing base (23a4197) to head (1f3b6d6).

Files with missing lines Patch % Lines
compiler/syntax/src/res_parsetree_viewer.ml 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8740   +/-   ##
=======================================
  Coverage   79.97%   79.97%           
=======================================
  Files         464      464           
  Lines       63236    63232    -4     
=======================================
- Hits        50573    50571    -2     
+ Misses      12663    12661    -2     
Files with missing lines Coverage Δ
compiler/syntax/src/res_printer.ml 93.51% <100.00%> (ø)
compiler/syntax/src/res_parsetree_viewer.ml 93.00% <66.66%> (+0.51%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch from 581fc5b to a07fd90 Compare October 8, 2026 07:17
@pkg-pr-new

pkg-pr-new Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8740

@rescript/belt

npm i https://pkg.pr.new/@rescript/belt@8740

@rescript/darwin-arm64

npm i https://pkg.pr.new/@rescript/darwin-arm64@8740

@rescript/darwin-x64

npm i https://pkg.pr.new/@rescript/darwin-x64@8740

@rescript/linux-arm64

npm i https://pkg.pr.new/@rescript/linux-arm64@8740

@rescript/linux-x64

npm i https://pkg.pr.new/@rescript/linux-x64@8740

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8740

@rescript/win32-x64

npm i https://pkg.pr.new/@rescript/win32-x64@8740

commit: 1f3b6d6

cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch 2 times, most recently from dbe3e47 to c27a87f Compare October 8, 2026 08:28
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch 2 times, most recently from 26396f3 to 7ee1f60 Compare October 8, 2026 09:39
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch from 7ee1f60 to 76f4916 Compare October 8, 2026 09:47
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch from 76f4916 to 98af7f8 Compare October 8, 2026 10:04
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch 2 times, most recently from e934c71 to 42197a5 Compare October 8, 2026 12:09
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt marked this pull request as ready for review October 8, 2026 14:26
@cknitt

cknitt commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-08T16:35:22.918919Z c3f4386 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42197a5a4d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Comment thread compiler/syntax/src/res_parsetree_viewer.ml
@cknitt

cknitt commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: c3f4386b75

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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
cknitt force-pushed the remove-obsolete-attribute-filters branch from c3f4386 to 09be9c3 Compare October 8, 2026 16:35
cknitt added a commit that referenced this pull request Oct 8, 2026
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Base automatically changed from module-await-node to master October 8, 2026 16:58
cknitt and others added 3 commits October 8, 2026 18:58
`res.ternary` and `JSX` are now only part of the v0 PPX wire format:
the parser produces `Pexp_ternary` and `Pexp_jsx_element`, and the v0
bridge converts both markers back into those nodes. Drop them from
`is_parsing_attr`, `has_attributes` and `is_printable_attribute`.

`is_printable_attribute` then only differed from `is_parsing_attr` in
`res.patVariantSpread` and `res.dictPattern`, which the parser only puts
on patterns, while the printable helpers are only used on expressions and
module expressions. Define the printable helpers in terms of
`is_parsing_attr`, so the printer has one list of internal attributes
instead of two that disagreed (the cause of the `@JSX` review finding in
#8735). `filter_printable_attributes` was the same as
`filter_parsing_attrs` and is removed.

A `@JSX` attribute written in source now gets the same parens as any
other attribute, e.g. `(@jsx x) + 1`. Formatting of the repo's sources is
unchanged.

Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Christoph Knittel <christoph@knittel.cc>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Printer cases for both marker names, and an ordinary attribute, on operands
of binary operators, pipes, unary operators, field and array access, calls,
ternaries and containers. Before the previous commit, @jsx lost its parens,
e.g. @jsx x + 1, and @res.ternary was dropped.

Signed-off-by: Christoph Knittel <christoph@knittel.cc>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cknitt
cknitt force-pushed the remove-obsolete-attribute-filters branch from 09be9c3 to 1f3b6d6 Compare October 8, 2026 16:58
@cknitt
cknitt requested a review from cristianoc October 8, 2026 16:58
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

@cknitt
cknitt removed this pull request from stack #8739 October 8, 2026 17:55
@cknitt
cknitt added this pull request to stack #8743 October 8, 2026 17:55
@cknitt
cknitt merged commit 18f3233 into master Oct 8, 2026
35 of 36 checks passed
@cknitt
cknitt deleted the remove-obsolete-attribute-filters branch October 8, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants