test(builtin): document physical_equal on a #valtype struct - #4093
Closed
bobzhang wants to merge 1 commit into
Closed
test(builtin): document physical_equal on a #valtype struct#4093bobzhang wants to merge 1 commit into
bobzhang wants to merge 1 commit into
Conversation
`#valtype` changes a type's representation, and `physical_equal` is
`%refeq` -- a comparison *of* the representation. Putting the two
together produces the sharpest case of the warning already on
`physical_equal`, that its result "may not be consistent across
different backends": for a two-field `#valtype struct Point`, the
targets disagree outright.
* native / llvm / wasm -- `Point` is genuinely unboxed, so there is no
identity to compare and `physical_equal` degrades to a field-wise
value comparison. Two independently constructed `Point`s with equal
fields *are* physically equal.
* js / wasm-gc -- a multi-field `#valtype` struct is still a heap
object, so `physical_equal` stays identity and those same two
`Point`s are *not* equal, exactly as for an ordinary struct.
`Some(p)` inverts between the same two groups, for the same underlying
reason: where `Point` is a value it cannot be represented as a pointer,
so `Some` allocates and two `Some(p)`s are distinct; where `Point` is
already a pointer, `Some(p)` is `p` itself and the two are equal.
Arity matters as well. A *single*-field `#valtype` struct is unwrapped
to its field on every target including js and wasm-gc, so a `Double`
wrapper inherits NaN's non-reflexivity straight through the struct and
is not physically equal even to itself. The split above is specifically
about multi-field structs.
The agreeing cases live in one file; the two divergent groups get a file
each, split via `options(targets:)` in `moon.pkg`, so each group's
answer is written down explicitly instead of hidden behind a
conditional. As with the other `physical_equal` tests these document
rather than specify -- nothing outside these files should depend on the
answers; they are pinned so a representation change shows up as a
visible diff.
Verified on all four targets (native, js, wasm, wasm-gc); `moon check`
clean on each, and no `.mbti` change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds builtin tests that document (not specify) how physical_equal behaves for #valtype structs across different backends, including the key divergence between unboxed (native/llvm/wasm) and boxed (js/wasm-gc) representations.
Changes:
- Introduces a shared test file covering
physical_equalcases that are consistent across all targets (reflexivity, unequal fields, array round-trips, and a single-field#valtypewrapper). - Adds two target-specific test files that pin the divergent behaviors for multi-field
#valtypestructs on unboxed vs boxed targets. - Updates
builtin/moon.pkgto include the per-target test files viaoptions(targets:).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| builtin/physical_equal_valtype_test.mbt | Shared, cross-target documentation tests and the #valtype/non-#valtype fixture types. |
| builtin/physical_equal_valtype_unboxed_test.mbt | Pins behavior on targets where multi-field #valtype structs are unboxed (native/llvm/wasm). |
| builtin/physical_equal_valtype_boxed_test.mbt | Pins behavior on targets where multi-field #valtype structs remain boxed (js/wasm-gc). |
| builtin/moon.pkg | Routes the divergent tests to the appropriate targets. |
Suppressed comments (2)
builtin/physical_equal_valtype_test.mbt:50
RefPointappears to be a test-local helper type. Consider making itprivto keep the fixture scoped to the test package and avoid accidental export of an abstract-public type.
struct RefPoint {
builtin/physical_equal_valtype_test.mbt:119
- Same as
Point: sinceDoubleWrapperis just a test fixture, marking itprivkeeps the test surface tidy and avoids exporting an abstract-public type from a_test.mbtfile.
struct DoubleWrapper {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// The struct under test, and an ordinary (reference-represented) struct with | ||
| /// the same fields to contrast against. | ||
| #valtype | ||
| struct Point { |
Collaborator
Coverage Report for CI Build 6141Coverage remained the same at 90.859%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds tests pinning what
physical_equalcurrently does with a#valtype struct Point { x : Int, y : Int }. Separate from #4092 (the NaN cases), though they share a theme.Why
#valtypechanges a type's representation, andphysical_equalis%refeq— a comparison of the representation. Together they produce the sharpest instance of the warning already sitting onphysical_equal, that its result "may not be consistent across different backends": for a two-fieldPoint, the targets disagree outright.physical_equal(p, p)truetruephysical_equal(p, q), distinct constructions, equal fieldstruefalsex, or only inyfalsefalsephysical_equal(Some(p), Some(p))falsetruefn[T]call site#valtype) struct, equal fieldsfalsefalseTwo mechanisms explain the whole table:
Pointis genuinely unboxed (native/llvm/wasm) there is no identity to compare, sophysical_equaldegrades to a field-wise value comparison — and it is a full one, not first-field-only, since a difference inyalone is caught. Where it stays a heap object (js/wasm-gc),physical_equalremains identity and#valtypechanges nothing.Some(p)inverts between the same two groups for the same reason: wherePointis a value it can't be represented as a pointer, soSomemust allocate and twoSome(p)s are distinct; wherePointis already a pointer,Some(p)ispand the two are equal.Arity matters too. A single-field
#valtypestruct is unwrapped to its field on every target, js and wasm-gc included. So aDoublewrapper inherits NaN's non-reflexivity straight through the struct —physical_equal(nan_wrapper, nan_wrapper)isfalseeverywhere. The native/js split above is specifically about multi-field structs.Layout
Cases that agree on every target live in
physical_equal_valtype_test.mbt; the two divergent groups get a file each, split byoptions(targets:)inbuiltin/moon.pkg. That way each group's answer is written down explicitly rather than hidden behind a conditional, and a representation change on either side shows up as a visible diff.Documentation, not specification
Same caveat as #4092, and it bites harder here:
intrinsics.mbtwarns the result may vary by backend and optimization settings, and this PR is a demonstration of exactly that. Nothing outside these files should depend on the answers.Testing
Run on all four targets —
native,js,wasm,wasm-gc— all green.moon checkclean on each;moon infoproduces no.mbtichange (test-only, no public surface).🤖 Generated with Claude Code