Repository navigation
Conversation
SubtypingDiscoverer::visitRefEq checked curr->type != Type::unreachable and read only curr->left->type to determine whether its operands must be subtypes of shared or unshared eqref. When one operand of a RefEq is unreachable and the other operand is a shared reference, curr->type is Type::unreachable, causing visitRefEq to constrain the shared operand to unshared eqref and trip the Type::isSubType(curr->type, type) assertion in TranslateToFuzzReader::mutate. Check both operands of RefEq for shared reference types when determining the eqref shareability constraint. Add unit tests as well as new assertions that allow an Unsbutyping regression test to catch the problem.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where a ref.eq expression with an unreachable operand and a shared reference operand did not correctly constrain its operands to shared eqref. The fix updates visitRefEq in subtype-exprs.h to check both operands for shareability, adds assertions to Unsubtyping.cpp to verify subtype relationships, and introduces both a gtest and a lit regression test to cover this scenario. As there are no review comments, I have no feedback to provide.
|
Unfortunately there does not seem to be a way to disable the GCA comments in the event of "I have no feedback to provide." |
| } | ||
|
|
||
| void noteNonFlowSubtype(Expression* sub, Type super) { | ||
| assert(Type::isSubType(sub->type, super)); |
There was a problem hiding this comment.
Why are these added? They don't look specific to this pass?
There was a problem hiding this comment.
I mean, if these make sense, why not put them in the general utility so they apply to all passes?
There was a problem hiding this comment.
They're added because there's no way to add an unsubtyping regression test otherwise. Unsubtyping takes the early return just below this.
Adding the assertions to the general utility makes sense, though. I'll do that.
There was a problem hiding this comment.
Oh, the assertions cannot be simply moved to the shared utility because the shared utility is doing CRTP calls out to subtypes for these functions. We could have the CRTP utility call a chokepoint function that does the assertion and then calls the subtype, but that would be a much larger change and doesn't seem worth it to me.
This branch has not been deployed
2 participants
SubtypingDiscoverer::visitRefEq checked curr->type != Type::unreachable and read only curr->left->type to determine whether its operands must be subtypes of shared or unshared eqref. When one operand of a RefEq is unreachable and the other operand is a shared reference, curr->type is Type::unreachable, causing visitRefEq to constrain the shared operand to unshared eqref and trip the Type::isSubType(curr->type, type) assertion in TranslateToFuzzReader::mutate.
Check both operands of RefEq for shared reference types when determining the eqref shareability constraint. Add unit tests as well as new assertions that allow an Unsbutyping regression test to catch the problem.