Lower the structural_eq intrinsic wrapper as a standalone procedure#10180
Merged
Conversation
Fixes roc-lang#10156. A generic constrained by `a.is_eq` requires its caller to supply the element's `is_eq` as evidence. When the concrete element type derives equality structurally, that evidence is the structural-equality intrinsic wrapper, which must then lower as a standalone procedure. lowerTemplateBody assumed the wrapper was only reachable from direct `==` sites and hit an invariant: a panic in Debug/ReleaseSafe and undefined behavior (false negatives, segfaults) in ReleaseFast. Instead, synthesize a `|lhs, rhs| lhs == rhs` body, mirroring how str_inspect is lowered.
The wrapper body synthesized in lowerStructuralEqIntrinsic went through lowerEqualityExpr, whose exact-method dispatch resolves is_eq for the wrapper's own type straight back to the wrapper, emitting a body that tail-calls itself: Str.from_utf8([255]) == Str.from_utf8([255]) compiled but looped forever at runtime. Split lowerDerivation's structural expansion into lowerDerivationExpansion and lower the wrapper body through it, so only the top layer skips method dispatch; components still derive through lowerDerivation and dispatch to their exact methods as before. Extend the issue 10156 fixture with expects that actually execute the wrapper (equal and unequal Err(BadUtf8(...)) values) and correct the fixture comments: the trigger is any comparison over a type containing Str.Utf8Problem (the only structural_eq intrinsic), not the generic evidence path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Anton-4
approved these changes
Jul 17, 2026
Anton-4
left a comment
Anton-4
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @niclas-ahden,
The Zig-fu was good :)
I did happen to discover a runtime infinite loop with this code:
Probe := [].{ run = |bytes| Str.from_utf8(bytes) }
expect Probe.run([255]) == Probe.run([255])
So I further altered the solution to fix this and added more tests.
Let me know if you would like me to explain things further.
Anton-4
enabled auto-merge
July 17, 2026 18:21
Contributor
Author
|
@Anton-4 Thanks for the review, and great that you found that issue! I'm just happy to get this merged and I can continue on my merry way of porting code :) |
Contributor
Author
|
Seems we need someone else's approval to merge since Anton was the last to push. |
lukewilliamboswell
approved these changes
Jul 18, 2026
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
3 participants
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.
I needed a fix for #10156 to unblock myself so I gave it a shot. This is my first Zig contribution and it took me a long time to try and figure stuff out, so hopefully I actually did understand and came to a reasonable solution 😅 I used existing code as a guideline as much as I could, and added fitting tests. I didn't want to use Claude for this since it's my first contribution, but I did ask it for a review and it was happy, so fingers crossed. Let me know if I messed up!
See the issue in #10156, and the solution as follows:
A generic constrained by
a.is_eqrequires its caller to supply the element'sis_eqas evidence. When the concrete element type derives equality structurally, that evidence is the structural-equality intrinsic wrapper, which must then lower as a standalone procedure.lowerTemplateBodyassumed the wrapper was only reachable from direct==sites and hit an invariant: a panic inReleaseSafeand undefined behavior (false negatives, segfaults) inReleaseFast.Instead, synthesize a
|lhs, rhs| lhs == rhsbody, mirroring howstr_inspectis lowered.