Repository navigation
Draw commit records as semantic diffs of the compiler's objects - #8882
cristianoc wants to merge 1 commit into
Conversation
Why:
A record states its change as prose sections and a JavaScript
Before/After pair. The objects most compiler changes touch, a typing
rule, a parsetree node, a match compiler's clause matrix, a Lambda
rewrite, an error message, have no form of their own in it: the reviewer
reconstructs them from the code diff of typecore.ml or matching.ml.
Code added (scripts/recordRender.js, the drawing of a record):
A pure module, with no file system and no git: it parses a record
(sections, change blocks, git trailers) and draws it as HTML. Each kind of
object has its drawing: a typing rule as an inference rule, an AST or
typed tree fragment as a tree, a clause matrix as a matrix, Lambda and
JavaScript as highlighted code with Before and After side by side, a
runtime representation as `ReScript ↦ JavaScript` lines, a diagnostic as
its text. The kinds are a typed table, so a kind without a drawing does
not type-check. A modified block's two versions of code, a tree or text
are drawn as aligned rows: a line both have faces itself, the changed
lines between are padded with gaps, and on a narrow screen the rows read
as a unified diff. An added or removed block's one version spans the
block. A long line wraps, its continuation indented past the line's own
indentation or tree connectors. It checks each record against its diff:
changelog fragments, the required trailers, the Tests trailer against
the test files both ways (every listed path is in the diff; every test
file in the diff other than a generated snapshot is listed, the missing
ones named), generated JavaScript against Output, magic numbers, the v0
PPX bridge, super_errors fixtures and tests/ERROR_VARIANTS.md.
Code modified (scripts/commitRecords.js, the Semantic Diff page):
Reads each commit's record, files, magic-number diff and changes/
fragments from git and hands them to recordRender.js, which parses,
checks and draws them. Its options are --out, --pr, --title and
--fragment.
Code added (scripts/recordFacts.js, the facts a record abstracts):
Prints, for one example and one compiler build, the parser's tree, the
tree after the built-in PPX, the JSX transform's output, the
formatter's output, the typed tree, the inferred interface, Lambda as
translated and after each pass that changes it, the generated ES module
and the diagnostic, each as the compiler prints it, under a line naming
the compiler and the commit its checkout is at. Run with --bsc on a
build of the parent commit, it gives a modified block's Before.
Code added (scripts/commitRecordHook.sh, a commit-msg hook, not installed):
Refuses a message without the Area, Output, Tests and Signed-off-by
trailers, with a trailer after Signed-off-by, or with a malformed Output
or Tests; for a commit that changes compiler, runtime, build system or
editor tooling sources, also one without Why: or a change block.
Docs modified (.claude/skills/records/SKILL.md, the record format):
The skill defines the kinds of object, each with the code that owns
it, how to extract its exact form, its drawing and what a change of it
brings; and the trailers, each with the check the card makes. A change
that replaces one shape by another is one modified block on one
example, Before extracted from a build of the parent commit and After
from this commit's, the Source line naming both builds; added and
removed blocks are for objects with no counterpart.
Before (text):
<subject>
<paragraph>
Example (rescript), Before (js), After (js), Source, Breaking, Review
Area, Output, Formatting, Formats, PPX, Editor, Errors, Tests,
Measured, PR, Signed-off-by
After (text):
<subject>
Why
<Kind> <added|modified|removed> (<object>), with Before and After
extracted from the parent commit's build and this commit's
Example, Effect, Source, Breaking, Review
Area, Output, Tests, Measured, PR, Signed-off-by
Command (sh):
node scripts/recordFacts.js -e 'let f = (c, a, b) => c ? a : b' parse lambda js
node scripts/commitRecords.js --out review.html --pr 1234 --title "Title" master..my-branch
ln -sf "$PWD/scripts/commitRecordHook.sh" "$(git rev-parse --git-path hooks)/commit-msg"
Review: Do records in the earlier format still render? Yes: a top-level
`Before (js):` and `After (js):` pair draws side by side, and trailers
outside the format (Formats, Errors, ...) draw as facts.
Review: Where do Formatting, Formats, PPX and Editor go? A formatter or
editor change is a Format or Editor change block; the card reads the
magic numbers, the v0 bridge fixtures and the error catalogue from the
diff.
Area: scripts docs tests
Output: unchanged
Tests: ~scripts/commitRecords.test.js
PR: #8882
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
18bff7c to
6aed21f
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@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-8882 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aed21f3a8
ℹ️ 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".
| if git diff --cached --name-only | | ||
| grep -Eq '^(compiler|rewatch/src|analysis|tools/src|packages/@rescript/(runtime|belt))/'; then |
There was a problem hiding this comment.
Include rewatch-ocaml/ in this source-path predicate. When a commit stages a file such as rewatch-ocaml/build_state.ml, the condition is false, so the new hook accepts the commit without the required Why: section or change block even though this directory contains the OCaml build-system implementation.
AGENTS.md reference: AGENTS.md:L35-L38
Useful? React with 👍 / 👎.
| const missing = listed.filter( | ||
| t => !r.files.some(f => f.path.startsWith(t.path)), | ||
| ); |
There was a problem hiding this comment.
Require either an exact path match or a / boundary after a listed directory. With the current prefix check, Tests: ~tests/foo is considered present when the diff changes only tests/foobar.res; the reciprocal unlisted-file check uses the same prefix logic, so both checks report success even though the changed test was never listed.
Useful? React with 👍 / 👎.
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. |
Proposal: records per PRMotivation#8882 makes every commit message a structured record, with Before/After extracted from a build of the parent commit. Trying it on the
ProposalOne record per PR, in a file, with Before/After generated by CI. Per commitA short message: subject, one to three lines of why, sign-off. No required structure or trailers. Per PR, written by the author
Per PR, generated by CI
Rules
What changes in #8882
Example: #8879No commit rewriting. One file: CI would render the |
|
One more thing, separate from the proposal: the It fails any commit that adds a For example, #8876 adds fixtures for two Could the check apply only to fixtures that exercise a catalogued variant? Alternatively, it could be a hint rather than a failure. |
|
A follow-up on the format of the record file: it could be YAML, validated against a JSON Schema, instead of a custom plain-text format. Why a schema
Why YAML rather than JSON #8879's record as YAML # changes/raw-extension-nodes.record.yaml
why: |
%raw, %ffi and %%raw were extensions whose string payload the frontend had
to recognise. Through a PPX, template payloads lost their decoding.
changes:
- kind: AST
change: modified
object: raw extensions
examples:
- views: [parse]
code: |
let f = %raw(`(a) => a`)
%%raw("var x = 1")
breaking: none
review:
- question: Do PPXs see a different tree?
answer: |
No. The v0 bridge maps the nodes to the same extensions as before and
reads them back.CI would add the generated parts to this: Before/After for each example and view from the base and head builds, plus the facts computed from the diff. |
|
One more idea for later, once the record format has settled: the changelog entries could move into the record file, so a PR has a single # changes/raw-extension-nodes.yaml
changelog:
- category: internal
entry: Represent `%raw`, `%ffi` and `%%raw` with parsetree nodes. https://github.com/rescript-lang/rescript/pull/8879
- category: fix
entry: Fix template and braced payloads of `%raw` and `%ffi` compiling differently when a PPX is used. https://github.com/rescript-lang/rescript/pull/8879
why: |
…
changes:
- …
For the transition, |
This branch has not been deployed
2 participants
Commit records name the objects a compiler change touches, and the Semantic Diff page draws each of them: a parsetree or typed tree as a tree, a typing rule as an inference rule, the match compiler's clause matrix as a matrix, Lambda and JavaScript as highlighted code. A change that replaces one shape by another is one block on one example, with the parent commit's and this commit's versions side by side on aligned rows.
scripts/recordRender.jsparses a record and draws it. It is a pure module, with no file system and no git. It also checks the record against its diff: changelog fragments, the required trailers,Testsagainst the test files in both directions, generated JavaScript againstOutput, magic numbers, the v0 PPX bridge,super_errorsfixtures andERROR_VARIANTS.md.scripts/recordFacts.jsextracts what a record shows from a compiler build, so Before and After come from the compiler rather than being written by hand. That covers the parser's tree, the tree after the built-in PPX, the typed tree, Lambda, the generated JS, formatter output and diagnostics.scripts/commitRecords.jsreads git and writes the page. Its options (--out,--pr,--title,--fragment), the Semantic Diff workflow andmake semantic-diffwork as before.scripts/commitRecordHook.shis an optionalcommit-msghook that refuses a message without the required trailers..claude/skills/records/SKILL.mdexplains how to enable it.changes/fragments are the changelog,Testsis required (ornone, because …), there are noRanorDependstrailers, andSigned-off-bycomes last.Example. #8879's diff, re-committed on this branch by an agent following the skill, renders as one
AST modifiedblock onlet f = %raw(`(a) => a`)and%%raw("var x = 1"). The left side is the parent compiler's tree (Pexp_extension "raw"…Pexp_template,Pstr_extension "raw"…PConst_raw_source), and the right side is the new one (Pexp_raw/Raw_template,Pstr_raw/Raw_string). Both come frombsc -bs-no-builtin-ppx -only-parse -dparsetree. The parsetree type excerpts sit side by side in the same way. All checks pass on that card.Test plan
node --test scripts/commitRecords.test.js: 9 passing. The tests cover a full record, added and removed blocks at full width, aligned modified rows, the--pr/--titleheader with escaping, both directions of theTestschecks (generated snapshots are exempt), two changelog fragments in one commit, and the hook accepting and refusing messages.npx biome check scripts/andnpx tsc: clean.master's recent commits flags only the squash merges that carry no record.🤖 Generated with Claude Code