Repository navigation
Semantic diff: write and draw changes as semantic objects - #8908
Conversation
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-8908 |
|
@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. |
![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: 56c69ba029
ℹ️ 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".
| const lhs = [...g.slice(0, at), ...(left.trim() ? [g[at]] : [])]; | ||
| const rhs = [...(right.trim() ? [g[at]] : []), ...body]; |
There was a problem hiding this comment.
When two rewrite forms have an identical left-hand side but a changed right-hand side, lineDiff marks the entire source line and including g[at] in lhs causes that unchanged left-hand side to receive del/add shading in both versions. For example, the tested (+ x 0) ⇝ (+ x 0) to (+ x 0) ⇝ x rewrite visually reports (+ x 0) itself as changed, making the semantic diff inaccurate; compare and shade the split left and right fragments independently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the terms of a rewrite pair are compared position by position, so (+ x 0) ⇝ (+ x 0) against (+ x 0) ⇝ x shades only the right side. Covered by "a pair of rewrites shades each side where it changes".
cknitt
left a comment
cknitt
left a comment
There was a problem hiding this comment.
Thanks, the forms and drawings read well, and the escaping holds up (I tried HTML in the subject, author, captions, trees, matrices and mappings). I ran recordRender.js on some message shapes outside the tests and found a few parser problems. The first three seem worth fixing before merge.
1. Trailers other than Signed-off-by / Co-Authored-By end the trailer block early (trailerStart, TRAILER)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_x
Signed-off-by: A <a@b>
The scan stops at Claude-Session. The first two lines render as a prose paragraph on the card, and the co-author list comes out empty. Reviewed-by:, Fixes: and Closes: hit the same problem. Could the last paragraph count as trailers when every line is Key: value (or a continuation line), or could it go through git interpret-trailers --parse?
2. Content on the heading line produces a false "Before and After are equal" (forms)
JS modified (output):
Before (js): return x + 0 | 0;
After (js): return x;
Both texts become captions and both bodies are empty, so the pair compares equal. The page then says "Before and After are equal" for a change. Tab-indented bodies end up the same way, because only spaces count as indentation. Two empty bodies shouldn't produce the "equal" claim, or the captions should be part of the comparison.
3. A paragraph after Why: … / Breaking: … merges into that section (parseMessage, the blank-line branch)
Why: the problem.
A separate paragraph.
Breaking: old flag removed.
Another unrelated paragraph.
A blank line resets only unnamed sections. So "A separate paragraph." ends up in the Why box, and "Another unrelated paragraph." ends up in the Breaking banner. This only happens when the heading has text on the same line.
4. Blocks start at 2 spaces of indentation, but the skill says 4 (/^ {2,}\S/)
A wrapped bullet with a 2-space continuation line (the style of changes/ fragments) splits into a paragraph plus a detached block:
- first bullet that wraps
onto a second line
5. Indented code under a plain paragraph renders as prose
The new output for:
let f = x => x + 1
let g = y => y
This renders as <p>let f = x => x + 1 let g = y => y</p>. prose() dedents first, so a uniformly indented block counts as text.
Smaller
- A prose line starting with
Example:(e.g. "Example: when the argument is constant, …") renders as a ReScript code block, even outside an object block. - In
rules, a change anywhere shades the whole figure. The skill says "the changed parts shaded". - In
code,a ⇝ b ⇝ cdrops everything after the second⇝. - Codex's open finding on rewrite shading (
recordRender.js:378) still applies. I haven't repeated it here.
Most of these would make good additions to commitRecords.test.js.
56c69ba to
0308018
Compare
|
Thanks, all fixed in the amended commit. Each case is now a regression test, and each of those tests fails on the previous renderer.
The skill states the trailer, indentation, one-line form and |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8908 +/- ##
=======================================
Coverage 81.03% 81.03%
=======================================
Files 455 455
Lines 63640 63640
=======================================
Hits 51573 51573
Misses 12067 12067 🚀 New features to boost your workflow:
|
Why:
Reviewers reconstruct a change's effect on compiler objects (trees,
Lambda, JavaScript, diagnostics, typing rules) from its code diff. A
record states that effect in each object's own form, Before and After,
and the review page draws it, so a reviewer reads the change at the
level of the object it changes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
cristianoc
changed the title
0308018 to
963328d
Compare
2 participants
A commit states its change as the semantic objects it changes: the parse tree, the typed tree, a typing rule, the match compiler's clause matrix, Lambda, the generated JavaScript, a diagnostic, a runtime representation. Each object is shown Before and After. This PR has two halves.
Writing: the skill.
.claude/skills/records/SKILL.mdtells an author, usually an agent, how to write a commit message this way:AST modified (raw extensions):);A small commit is a subject and a line or two, with no blocks.
Drawing: the page. The Semantic Diff page draws each commit's message:
scripts/recordRender.jsparses and draws a message,scripts/commitRecords.jswrites the page for a range of commits, andmake semantic-diffrendersmaster..HEAD.A generic mechanism, instantiated for ReScript. The mechanism has three parameters: the notion of semantic object, the format each object is written in, and the way each is rendered. This PR fixes them for the compiler: objects are trees, rules, matrices, Lambda, output and diagnostics, written as the blocks above and drawn as the drawings above. Another project instantiates the same mechanism with its own objects, formats and renderings.
Examples. #8909 re-applies six merged changes, each with its message written by the skill. Its Semantic Diff link shows a parse tree, a clause matrix, Lambda, generated JavaScript, formatter output and a diagnostic.
Related
master.Test plan
node --test scripts/commitRecords.test.js: 23 passing.npx biome check scripts/,npx tscandscripts/format_check.sh: clean.🤖 Generated with Claude Code