Remove Int.Ref and the %incr, %decr and %refget builtins - #8616
Remove Int.Ref and the %incr, %decr and %refget builtins#8616cristianoc wants to merge 1 commit into
Conversation
![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: a4e7ed8111
ℹ️ 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".
| @@ -268,7 +266,6 @@ let primitive_builtins : (string * Lambda.builtin) array = | |||
| (* BEGIN Triples for ref data type *) | |||
| ("%makeref", Pmakeblock Lambda.ref_tag_info); | |||
| ("%refset", Psetfield (0, Lambda.ref_field_set_info)); | |||
There was a problem hiding this comment.
When make test builds the tests/tests project in scripts/test.js:81-84, tests/tests/src/test_per.res:158-161 is included by that project's recursive src configuration and still declares %refget, %incr, and %decr. With these table entries removed, translation raises Unknown builtin primitive "%refget" before the test suite can run; remove or migrate these declarations, or move them into an explicit negative-error fixture.
AGENTS.md reference: AGENTS.md:L41-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed those three declarations in the same commit — thanks, they were stale and nothing else in the repo declares them.
One correction on the predicted failure, so the same finding doesn't get raised at P1 again: test_per.res compiles fine with them. Declaring an external whose primitive no longer exists is not an error — the external is optimized away when unused — and this file only declares %refget, %incr and %decr, never uses them. Unknown builtin primitive is raised at the use site, which is why make test passes on the branch as it stood. Verified directly:
type ref<'a> = {mutable contents: 'a}
external incr: ref<int> => unit = "%incr"
// compiles; adding `let f = r => incr(r)` is what errorsThe neighbouring %makeref and %refset declarations stay, since those primitives still exist.
None of these were designed for ReScript. %incr, %decr and %refget arrived with OCaml's Pervasives in the 2016 initial export and were carried unexamined through every stdlib reshuffle since. Int.Ref itself was created in April 2025 (#7371) not because anyone wanted it, but as somewhere for the Pervasives.incr deprecation to point; the primitives it wrapped were removed for v13 two weeks ago. Outside this repository, GitHub code search finds no user of either the externals or the API. What the primitive bought was unboxing: expanding at the call site kept the field write syntactically visible, so Lam_pass_eliminate_ref could still turn a local ref into a mutable variable. A call through an ordinary function cannot - the reference appears as a bare Lvar and the pass gives up. That is not special to increment. Its body is six nodes against a small_inline_size of five, and cross-module inlining is off, so the inliner cannot reach it. Writing the update directly does keep the unboxing, and is shorter than the call it replaces: Int.Ref.increment(v) -> v.contents = v.contents + 1 53 call sites across 30 test files change that way, and their generated JavaScript is byte-identical. Only two outputs move: Stdlib_Int loses an empty Ref object and its export, and test_incr_ref loses onExpression - added to pin that the primitive bound its argument before mentioning it twice, which has nothing left to test now that no expansion happens. Lambda.offset_ref and the Offset_ref builtin go with them. Nothing in lambda.ml now builds a term outside the constructors and the traversals. Int.Ref.t went too. It was a type alias for ref<int> introduced alongside the two functions, and with them gone the module held nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W8g8qwBARAcvW9MyuKQq8H
a4e7ed8 to
ac3542b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8616 +/- ##
==========================================
- Coverage 77.32% 77.32% -0.01%
==========================================
Files 467 467
Lines 63313 63318 +5
==========================================
+ Hits 48957 48960 +3
- Misses 14356 14358 +2
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
@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. |
|
I agree that the usefulness of But they are not deprecated yet in v12. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac3542b815
ℹ️ 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".
| - Correct the structured function details produced by `rescript-tools doc` and exposed by `RescriptTools.Docgen`: parameters now retain labels and optionality, nested functions, tuples, variables, and generic arguments retain their type structure, return types are identified correctly, and non-function values no longer receive fake function details. This changes the published docgen detail schema. https://github.com/rescript-lang/rescript/pull/8576 | ||
| - Make object-field mutability part of the type. A property has one type for reading and writing. Assignment requires `@set`, except on an inferred open row, where assignment makes the field settable. Private rows are not inferred open rows, so a field in `type t = private {.."x": int}` is writable only when annotated with `@set`. Coercions never grant or widen write capability. Previously, getter and setter types were tracked independently, allowing a property to be written at a different type than it was read and allowing writes through a value coerced to a type without `@set`. https://github.com/rescript-lang/rescript/pull/8597 | ||
| - Remove the undocumented object-field attribute forms `@get` (bare or with a `null`/`undefined`/`nullable` payload) and `@set({no_get: ...})` on object types. Only bare `@set` marks a field settable; nullable getter types are written directly (`null<t>`, `undefined<t>`, `nullable<t>`). https://github.com/rescript-lang/rescript/pull/8597 | ||
| - Remove `Int.Ref` and the `%incr`, `%decr` and `%refget` builtins behind it. Write `r.contents = r.contents + 1` instead; an `external` declared with one of the removed names is now rejected. https://github.com/rescript-lang/rescript/pull/8616 |
There was a problem hiding this comment.
When the argument is not already a variable—such as the deleted regression case Int.Ref.increment(mkRef())—the advertised substitution becomes mkRef().contents = mkRef().contents + 1, which invokes mkRef() twice and can read from and write to different references. Document that expression receivers must first be bound once, and retain an end-to-end regression test for this migration pattern.
AGENTS.md reference: AGENTS.md:L41-L41
Useful? React with 👍 / 👎.
2 participants
Int.Refexisted only as the destination of a v13 deprecation, and the%incr,%decrand%refgetbuiltins behind it were inherited from OCaml'sPervasivesrather than designed for ReScript.r.contents = r.contents + 1is shorter than the call it replaces and compiles to the same JavaScript.
Removing the builtins also removes
Lambda.offset_ref, the last primitivethat lowered to a hand-built assignment. An
externaldeclared with one ofthe removed builtin names is now rejected as unknown, which is what already
happens for any other unrecognised name.
Part of #8573. Stacked on #8615.