Skip to content

Remove Pconst_json and represent fixed external arguments in the parsetree - #8875

Merged
cknitt merged 3 commits into
masterfrom
fixed-external-args-parsetree
Oct 16, 2026
Merged

cknitt merged 3 commits into
masterfrom
fixed-external-args-parsetree

Conversation

@cknitt

@cknitt cknitt commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

First part of splitting #8609, so that the syntax for fixed external arguments can be discussed separately.

Summary

This PR removes Pconst_json from the parsetree and gives fixed external arguments (@as(value) _) a dedicated parsetree node instead of an attribute on _.

Removing Pconst_json

json templates no longer have their own constant:

  • json...`` is an ordinary tagged template. It used to be rejected everywhere except in @as, and is now accepted like any other tagged template.
  • As the payload of a fixed external argument, @as(json`...`) _, it keeps working as before and becomes Pfixed_json below.
  • The v0 PPX encoding of that payload is unchanged (a json-delimited string).
  • The json rejection code is gone: Ast_payload.reject_json_literal*, the post-PPX unconsumed_json_iterator, the parser's json template special case, and Typecore.Json_literal_outside_external.

Fixed external arguments

and arg =
  | Parg_type of {attrs; lbl; typ}
  | Parg_fixed of {attrs; lbl; value: fixed_value loc; loc}

and fixed_value =
  | Pfixed_int of string loc
  | Pfixed_string of String_literal.string_literal
  | Pfixed_template of string loc
  | Pfixed_json of string loc
  • The parser produces Parg_fixed for the top-level arguments of externals, keeping the value and its locations as written. The formatter prints it back as @as(value) _.
  • The v0 PPX encoding is unchanged (_ [@as value]), and Ast_mapper_from0 reads an external's @as(value) _ arguments the same way the parser does.
  • External processing decodes the value with the existing @as reader, so the generated code and the error messages stay the same.
  • Arguments that the node could not print back exactly as written stay attributes and go through the old path: a _ with other attributes besides @as, a braced payload such as @as({1}), a second @as, and invalid payloads.

The .cmi, .cmt and AST magic numbers are bumped, since removing Pconst_json changes the marshalled constant layout.

Tests

  • The json fixtures now check that json templates are ordinary tagged templates (tagged_template_test.res, the parser and super-errors snapshots).
  • printer/typexpr/fixedExternal.res and ast-mapping/FixedExternal.res cover the fixed-argument forms, comments and attributes.
  • ounit_ast_mapper0_tests: a fixed argument reaches v0 as _ [@as 1] and comes back with its value and locations, and only externals get fixed arguments.

Compared with master's compiler, apart from json templates outside fixed arguments, formatter output and the v0 tree a PPX receives are identical for every .res/.resi file in the repository. Compiled output and errors are identical for a few hundred fixed-argument forms in plain, @obj, @send and @variadic externals, both directly and through a PPX.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NP47qMfjMbjUTKZDsLthDo

@cknitt
cknitt force-pushed the fixed-external-args-parsetree branch from 910b6bf to 19a5a20 Compare October 16, 2026 08:02
@cknitt
cknitt added this pull request to stack #8877 October 16, 2026 08:03
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.13793% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.01%. Comparing base (dcc7e35) to head (0b64bf2).

Files with missing lines Patch % Lines
compiler/ml/printast.ml 0.00% 15 Missing ⚠️
tests/ounit_tests/ounit_ast_mapper0_tests.ml 84.50% 11 Missing ⚠️
compiler/ml/ast_fixed_arg.ml 90.24% 4 Missing ⚠️
compiler/ml/pprintast.ml 62.50% 3 Missing ⚠️
compiler/frontend/ast_external_process.ml 91.66% 2 Missing ⚠️
analysis/src/completion_front_end.ml 0.00% 1 Missing ⚠️
analysis/src/document_symbol.ml 0.00% 1 Missing ⚠️
analysis/src/signature_help.ml 90.90% 1 Missing ⚠️
compiler/ml/ast_helper.ml 66.66% 1 Missing ⚠️
compiler/ml/ast_iterator.ml 83.33% 1 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8875      +/-   ##
==========================================
- Coverage   81.02%   81.01%   -0.01%     
==========================================
  Files         453      454       +1     
  Lines       63030    63187     +157     
==========================================
+ Hits        51070    51193     +123     
- Misses      11960    11994      +34     
Files with missing lines Coverage Δ
analysis/src/dump_ast.ml 43.82% <ø> (+0.18%) ⬆️
compiler/ext/config.ml 100.00% <ø> (ø)
compiler/frontend/ast_attributes.ml 91.78% <ø> (ø)
compiler/frontend/ast_config.ml 87.87% <ø> (-0.36%) ⬇️
compiler/frontend/ast_derive_projector.ml 90.16% <100.00%> (ø)
compiler/frontend/ast_exp_extension.ml 100.00% <ø> (ø)
compiler/frontend/ast_exp_handle_external.ml 82.50% <100.00%> (ø)
compiler/frontend/bs_builtin_ppx.ml 90.49% <100.00%> (+0.23%) ⬆️
compiler/frontend/ppx_entry.ml 91.66% <ø> (+1.04%) ⬆️
compiler/ml/ast_mapper.ml 98.41% <100.00%> (+0.03%) ⬆️
... and 26 more
🚀 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.

@pkg-pr-new

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

Copy link
Copy Markdown

Open in StackBlitz

rescript

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

@rescript/belt

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/runtime

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

@rescript/win32-x64

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

commit: 0b64bf2

@github-actions

Copy link
Copy Markdown

`@as(value) _` arguments of externals become `Parg_fixed`, holding the value
as written. `Pconst_json` is gone: `json` templates are ordinary tagged
templates except as fixed values, whose v0 encoding is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NP47qMfjMbjUTKZDsLthDo
Signed-off-by: Christoph Knittel <ck@cca.io>
@cknitt
cknitt force-pushed the fixed-external-args-parsetree branch from 19a5a20 to b5d7c86 Compare October 16, 2026 09:01
@cknitt cknitt changed the title Represent fixed external arguments in the parsetree Remove Pconst_json and represent fixed external arguments in the parsetree Oct 16, 2026
@cknitt

cknitt commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 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-16T13:27:02.566114Z 0b64bf2 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: b5d7c862f4

ℹ️ 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/ml/ast_mapper_to0.ml Outdated
Comment thread compiler/ml/ast_mapper.ml Outdated
Relocating mappers now reach every location of a fixed argument, including
the ones `Ast_mapper_to0` synthesizes for the v0 spelling.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NP47qMfjMbjUTKZDsLthDo
Signed-off-by: Christoph Knittel <ck@cca.io>
@cknitt

cknitt commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@codex review

@cknitt
cknitt marked this pull request as ready for review October 16, 2026 13:06

@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: 99b7a84575

ℹ️ 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/ml/ast_fixed_arg.ml Outdated
`@as(@foo 1) _` stays an attribute, so `@foo` is not dropped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NP47qMfjMbjUTKZDsLthDo
Signed-off-by: Christoph Knittel <ck@cca.io>
@cknitt

cknitt commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 0b64bf2fa0

ℹ️ 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 requested a review from cristianoc October 16, 2026 13:35
@cknitt
cknitt merged commit 735a485 into master Oct 16, 2026
26 checks passed
@cknitt
cknitt deleted the fixed-external-args-parsetree branch October 16, 2026 14:49
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