[WIP] Use leave for next instead of catch table - #62
Conversation
| expect(ensure_called_for[0].data).toEqual(1); | ||
| expect(ensure_called_for[1].data).toEqual(2); | ||
| expect(ensure_called_for[2].data).toEqual(3); | ||
| }); |
There was a problem hiding this comment.
This test actually fails... 2 questions...
-
Is testing this via jest the correct path? Or should this be attacked via the ruby tests?
-
Should I attempt to fix ensure first since it might be a more broad bug? Or should I just attempt to fix in the process of eliminating the catch table for
nextin a block?
There was a problem hiding this comment.
Is testing this via jest the correct path? Or should this be attacked via the ruby tests?
Jest (now vitest) is really intended for smoke tests. There's a bit more than just smokes in the jest suite at the moment though because I set it up long before I set up ruby/spec. In this case I would suggest writing some jest tests at first so it's easier to iterate on a solution (ruby/spec comes with a lot of overhead), then remove them in favor of the corresponding ruby/spec tests later.
Should I attempt to fix ensure first since it might be a more broad bug? Or should I just attempt to fix in the process of eliminating the catch table for
nextin a block?
I would focus on eliminating the catch table for next in a block. I say that because dumping the instructions for doubled = [1, 2, 3].map do |x|; next x * 2; ensure; puts 'ensure'; end appears to use a catch table with an ensure entry, which makes sense but isn't the problem you're trying to solve.
| expect(ensure_called_for[2].data).toEqual(3); | ||
| }); | ||
|
|
||
| // TODO: add tests for while/until/for using next |
There was a problem hiding this comment.
Seems like next in a while is a different animal than next in a block despite using the same Ruby keyword.
I wonder if I ought to add coverage for the while/for/until paths first and address any bugs there first before attempting to eliminate using the catch table for next in a block.
Basically, unclear which strategy is better because it would kind of hinge on whether fixing bugs with the current implementation would just be thrown away when abandoning the catch table for implementing next in a block, in which case it seems like I ought to just add all the tests and try to get them green with the non-catch approach instead of fixing them independently in code that would be deleted in the follow-up optimization PR.
There was a problem hiding this comment.
Seems like
nextin awhileis a different animal thannextin a block despite using the same Ruby keyword.
Yes, that's what I was trying to say in Slack. next in a while (or any control structure that doesn't push a frame) can't use leave to exit the loop because leave exits the current frame. Using a leave in a while loop will cause the enclosing frame - eg. the enclosing method, block, or whatever - to exit, which is probably not what you want in the majority of cases.
I wonder if I ought to add coverage for the while/for/until paths first and address any bugs there first before attempting to eliminate using the catch table for
nextin a block.
Yeah, I think that will be easier to start with. Exiting a while loop is a matter of jumping back to the beginning of the loop via a well-placed label, which I think is easier than dealing with the complexities of the catch table. One possible snag though will be keeping track of when the compiler encounters a next within a while (or other non-frame-generating control structure) as opposed to a block. I don't believe the compiler currently has or tracks that information.
Basically, unclear which strategy is better because it would kind of hinge on whether fixing bugs with the current implementation would just be thrown away when abandoning the catch table for implementing next in a block, in which case it seems like I ought to just add all the tests and try to get them green with the non-catch approach instead of fixing them independently in code that would be deleted in the follow-up optimization PR.
I think they are necessarily two different implementations. next in a while will jump; next in a block can use leave. I believe they can both be developed independently of each other.
| // this.iseq.throw(ThrowType.NEXT); | ||
| // TODO: we need a totally different operation that jumps for while/until/for | ||
| // TODO: we should probably to fix the ensure bug in next.test.ts first | ||
| this.iseq.leave(); |
There was a problem hiding this comment.
This works! Well, it gives the same results in the test suite as the throw implementation, so in that sense, it works.
There was a problem hiding this comment.
Nice! This is probably all that's necessary for block contexts 👍 Now we just need to know if we're inside a while.
| this.elements.pop(); | ||
| } | ||
|
|
||
| // eliminate this |
There was a problem hiding this comment.
Obviously I'd remove any comments like these during a rebase so can just ignore them. Stuff like this is in here to help me quickly find interesting spots in the code by looking at diffs/history.
| @@ -0,0 +1,73 @@ | |||
| import {beforeAll, describe, expect, test} from '@jest/globals'; | |||
There was a problem hiding this comment.
Marco just switched us over to vitest 😄
| import {beforeAll, describe, expect, test} from '@jest/globals'; | |
| import {beforeAll, describe, expect, test} from 'vitest'; |
| expect(ensure_called_for[0].data).toEqual(1); | ||
| expect(ensure_called_for[1].data).toEqual(2); | ||
| expect(ensure_called_for[2].data).toEqual(3); | ||
| }); |
There was a problem hiding this comment.
Is testing this via jest the correct path? Or should this be attacked via the ruby tests?
Jest (now vitest) is really intended for smoke tests. There's a bit more than just smokes in the jest suite at the moment though because I set it up long before I set up ruby/spec. In this case I would suggest writing some jest tests at first so it's easier to iterate on a solution (ruby/spec comes with a lot of overhead), then remove them in favor of the corresponding ruby/spec tests later.
Should I attempt to fix ensure first since it might be a more broad bug? Or should I just attempt to fix in the process of eliminating the catch table for
nextin a block?
I would focus on eliminating the catch table for next in a block. I say that because dumping the instructions for doubled = [1, 2, 3].map do |x|; next x * 2; ensure; puts 'ensure'; end appears to use a catch table with an ensure entry, which makes sense but isn't the problem you're trying to solve.
| expect(ensure_called_for[2].data).toEqual(3); | ||
| }); | ||
|
|
||
| // TODO: add tests for while/until/for using next |
There was a problem hiding this comment.
Seems like
nextin awhileis a different animal thannextin a block despite using the same Ruby keyword.
Yes, that's what I was trying to say in Slack. next in a while (or any control structure that doesn't push a frame) can't use leave to exit the loop because leave exits the current frame. Using a leave in a while loop will cause the enclosing frame - eg. the enclosing method, block, or whatever - to exit, which is probably not what you want in the majority of cases.
I wonder if I ought to add coverage for the while/for/until paths first and address any bugs there first before attempting to eliminate using the catch table for
nextin a block.
Yeah, I think that will be easier to start with. Exiting a while loop is a matter of jumping back to the beginning of the loop via a well-placed label, which I think is easier than dealing with the complexities of the catch table. One possible snag though will be keeping track of when the compiler encounters a next within a while (or other non-frame-generating control structure) as opposed to a block. I don't believe the compiler currently has or tracks that information.
Basically, unclear which strategy is better because it would kind of hinge on whether fixing bugs with the current implementation would just be thrown away when abandoning the catch table for implementing next in a block, in which case it seems like I ought to just add all the tests and try to get them green with the non-catch approach instead of fixing them independently in code that would be deleted in the follow-up optimization PR.
I think they are necessarily two different implementations. next in a while will jump; next in a block can use leave. I believe they can both be developed independently of each other.
| // this.iseq.throw(ThrowType.NEXT); | ||
| // TODO: we need a totally different operation that jumps for while/until/for | ||
| // TODO: we should probably to fix the ensure bug in next.test.ts first | ||
| this.iseq.leave(); |
There was a problem hiding this comment.
Nice! This is probably all that's necessary for block contexts 👍 Now we just need to know if we're inside a while.
| NONE = 0x0, | ||
| RETURN = 0x1, | ||
| BREAK = 0x2, | ||
| // eliminate this with leave instead... |
There was a problem hiding this comment.
Oh interesting yeah, I suppose we can remove the NEXT catch type entirely since neither blocks nor whiles will need to use it for handling nexts anymore.
just opening a wip/draft PR to ask questions...