Skip to content

Fix custom operator dispatch missing receiver#295

Open
khoadng wants to merge 2 commits into
ethanblake4:masterfrom
khoadng:fix/custom-operator-dispatch
Open

Fix custom operator dispatch missing receiver#295
khoadng wants to merge 2 commits into
ethanblake4:masterfrom
khoadng:fix/custom-operator-dispatch

Conversation

@khoadng
Copy link
Copy Markdown
Contributor

@khoadng khoadng commented Mar 24, 2026

Custom operators on eval-defined classes ([], []=, +, *, /, %, etc.) crash because InvokeDynamic doesn't pass the instance as the first arg.

@Zverik
Copy link
Copy Markdown
Collaborator

Zverik commented Mar 24, 2026

  1. Will the two skipped tests in operator_test work with this change?
  2. This change seems to be a bit too far-reaching, like the idempotent boxing-unboxing. Do we know the core reason for this not being prepended? Maybe it's operator-specific?

@khoadng
Copy link
Copy Markdown
Contributor Author

khoadng commented Mar 24, 2026

  1. They are still failed
  2. Operator-specific. The receiver was missing because the operator path in the compiler never pushed it, unlike the regular method call path. We can fix it at the compiler by pushing the receiver for known instance classes, and encode flag on InvokeDynamic so the runtime only fills it in for dynamic calls where the compiler can't know. But this would change the bytecode format and need version bump.

@khoadng
Copy link
Copy Markdown
Contributor Author

khoadng commented Mar 24, 2026

Changed to compiler fixed.

@ethanblake4
Copy link
Copy Markdown
Owner

I'm a bit confused about this one. If we have enough information at compile-time to know whether to set _hasReceiver to true or false why couldn't we just PushArg() the receiver instead of making a change to the runtime?

@khoadng
Copy link
Copy Markdown
Contributor Author

khoadng commented Mar 25, 2026

When the type is known, the compiler pushes the receiver. But for dynamic, the same InvokeDynamic op could resolve to either a $InstanceImpl (needs receiver in args for PushScope) or a bridge $Instance (receiver passed separately via method.call). The flag lets the runtime know which case it's in.

@ethanblake4
Copy link
Copy Markdown
Owner

ethanblake4 commented Mar 25, 2026

I see, next question.

You added these isBridge === false checks to determine how to set the _hasReceiver flag. But if the receiver is a bridge class then it would also necessarily not be an $InstanceImpl, right? Thus, it would not even reach the codepath in InvokeDynamic that reads said flag, so I don't see how that check does anything.

If that's accurate, then the only time this is useful is when the receiver type is dynamic. And maybe this is still the best solution for that, but it does irk me somewhat to have to add the overhead of another property on the opcode just for those few cases.

@khoadng
Copy link
Copy Markdown
Contributor Author

khoadng commented Mar 25, 2026

Yeah, the flag only matters for dynamic. For known bridges the $InstanceImpl branch isn't reached anyway. If the bytecode overhead isn't worth it for that edge case, we could drop the flag and use the previous identical check, it only runs in the $InstanceImpl branch so it's already scoped correctly. The compiler-side fix for known instance classes is the main fix either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants