Repository navigation
Treat an empty record literal with mutable fields as expansive - #9152
Conversation
Changelog🐛 Bug fix
Semantic Diff1 commit · Full page Treat an empty record literal with mutable fields as expansived0537fc · cknitt + Claude · Oct 31, 2026, 08:21 UTC
Diagnostic added (empty record literal with a mutable field) type cell<'a> = {tag?: int, mutable value?: 'a}
let cell: cell<_> = {}
cell.value = Some(1)
let s: option<string> = cell.value
-none; `s` holds the number 1 at run time
+This has type: option<int>
+But it's expected to have type: option<string>
+
+The incompatible parts:
+ int vs stringReview
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9152 +/- ##
==========================================
- Coverage 81.74% 81.74% -0.01%
==========================================
Files 460 460
Lines 64940 64944 +4
==========================================
+ Hits 53088 53091 +3
- Misses 11852 11853 +1
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
Why:
The value restriction looks for mutable fields among a record
literal's entries, but `{}` of an all-optional record has none, so
it was always non-expansive and its type variables were generalized
even when a field is mutable (#9150). It now looks up the record's
declaration and requires every field to be immutable.
Diagnostic added (empty record literal with a mutable field):
Example:
type cell<'a> = {tag?: int, mutable value?: 'a}
let cell: cell<_> = {}
cell.value = Some(1)
let s: option<string> = cell.value
Before (diagnostic): none; `s` holds the number 1 at run time
After (diagnostic):
This has type: option<int>
But it's expected to have type: option<string>
The incompatible parts:
int vs string
Review: Does `{}` of an immutable record still generalize? Yes; the
super_errors fixture uses one such value at both option<int> and
option<string> before reaching the error.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe
Signed-off-by: Christoph Knittel <ck@cca.io>
30ccdfd to
d0537fc
Compare
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-9152 |
|
@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. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
cristianoc
left a comment
There was a problem hiding this comment.
Please check for potential breaking changes.
This is bug fix so it's fine no matter what.
But one should set expectaions right.
|
Checked: master and this PR, each with the same files. What newly fails. All three conditions must hold:
The binding's type then gets a weak variable instead of being generalized. That surfaces in three ways: type handlers<'a> = {mutable onChange?: 'a => unit}
// 1. Left open at the top level:
let empty: handlers<'a> = {}
// error: type variables that cannot be generalized: handlers<'_a>
// 2. Used at two types:
let empty: handlers<_> = {}
let forInts: handlers<int> = empty
let forStrings: handlers<string> = empty // error: handlers<int> vs handlers<string>
// 3. Declared polymorphic in a .resi as `let empty: handlers<'a>`:
// error: the implementation has handlers<'_weak1>All three compile on master. Cases 2 and 3 are exactly the unsound pattern: one mutable object shared at several types. What still compiles:
Migration: make the shared value a function ( In this repo nothing hit it: the stdlib, Belt, the tests and the rewatch test repo with its dependencies all build unchanged. Records with optional |
…ipt-lang#9152) Why: The value restriction looks for mutable fields among a record literal's entries, but `{}` of an all-optional record has none, so it was always non-expansive and its type variables were generalized even when a field is mutable (rescript-lang#9150). It now looks up the record's declaration and requires every field to be immutable. Diagnostic added (empty record literal with a mutable field): Example: type cell<'a> = {tag?: int, mutable value?: 'a} let cell: cell<_> = {} cell.value = Some(1) let s: option<string> = cell.value Before (diagnostic): none; `s` holds the number 1 at run time After (diagnostic): This has type: option<int> But it's expected to have type: option<string> The incompatible parts: int vs string Review: Does `{}` of an immutable record still generalize? Yes; the super_errors fixture uses one such value at both option<int> and option<string> before reaching the error. Claude-Session: https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe Signed-off-by: Christoph Knittel <ck@cca.io> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Pedro Castro <aspeddro@gmail.com>
Why:
The value restriction checked only the fields a spread literal writes,
so `{...base, tag: 1}` was non-expansive even when it copied a mutable
field from `base`, and its type was generalized (#9156). A record
literal allocates every field of its type, whether written, copied
from the spread or left out, so the check now requires all of them to
be immutable, with or without a spread.
Diagnostic added (record spread copying a mutable field):
Example:
type cell<'a> = {tag: int, mutable value: option<'a>}
@Val external base: cell<'a> = "base"
let copy = {...base, tag: 1}
copy.value = Some(1)
let s: option<string> = copy.value
Before (diagnostic): none; `s` holds the number 1 at run time
After (diagnostic):
This has type: option<int>
But it's expected to have type: option<string>
The incompatible parts:
int vs string
Review: Does a spread of an immutable record still generalize? Yes; the
super_errors fixture uses one at both option<int> and option<string>
first.
Review: Why touch the {} changelog entry? #9152 merged before its
migration note was pushed; this adds it.
Claude-Session: https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe
Signed-off-by: Christoph Knittel <ck@cca.io>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #9150.
The value restriction (
Typecore.is_nonexpansive) treats a record literal as non-expansive when none of its fields is mutable. An empty literal{}of an all-optional record has no field entries in the typed tree, so the check always passed. Its type variables were then generalized even when the record has a mutable field, and a value could be stored at one type and read at another:For
{}without a spread, the check now looks up the record's declaration through the expression's type and requires every field to be immutable.{}of an immutable record still generalizes.The new
super_errorsfixturerecord_empty_mutable_does_not_generalize.rescovers both: it first uses an immutable{}at two types, then reaches the expected error for the mutable one.#9149, which stopped storing omitted optional fields in the typed tree, added the matching check for literals that write some fields but leave out a mutable one. This PR fills in the remaining
{}arm of that check.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gKyiYMf54wPRxFdUWe