Skip to content

Conversation

@eightbitraptor
Copy link
Contributor

Properly fixes ruby/prism#2053 (after a partial fix in #9445)

This PR refactors the methods prism was using to look for local variables. It does a few things:

  • Binds together the depth of the required local table, and the index into the table into a tuple pm_local_index_t, so that we don't have to pass pointers to manipulate into the lookup functions.

  • Reduces the API surface - previously we had 3 different ways for looking up local variables depending on whether we wanted to recurse or not, or start from a specific depth. Now we have a single function, pm_lookup_local_index, that always starts from the current depth and recurses if it doesn't find a local. This matches Ruby semantics where variables defined in outer scope are always visible from inner scopes.

  • Removes scope_node->local_depth_offset. This method of selecting the local table hasn't worked out as well as I'd hoped. Because for deeply nested scopes it's not possible to accurately set the offset such that it'll work in all cases. Starting from the current scope and walking the tree until a local is found means that we don't need this functionality at all anymore.

@eightbitraptor eightbitraptor force-pushed the mvh-refactor-local-index branch from deddb90 to f547c89 Compare January 16, 2024 08:58
Copy link
Contributor

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly, we're basically using the depth of the various localvariable*node nodes to provide a starting level, and then recursing up from there by doing a hash lookup at every level. From some experimentation, I think this works because prism always introduces fewer scopes than CRuby. If that were ever to change, this would be buggy.

I'm happy to merge this as is - it looks like it gets us closer on the btests so I think that's a win. I do have two things I'd like to change about this, but they can be done at a later time or now, whichever you'd prefer.

The first is I think we should avoid making this recursive. It's tail recursive at the moment, which helps, but I think we can do better by looking up the scope chain in a loop within the function.

The second is that I think we're doing unnecessary hash lookups with this approach. For example with foo = 1; END { foo } it's going to think it's at depth 0, which means it's going to check 3 hash tables before it finds the local. The same is going to happen for anything nested under rescues, ensures, or for loops. I think we can avoid this by introducing a new field onto scope nodes that indicates whether or not this function should check for locals on them. Then, when it hits one of those scopes, it would skip over it instead of looking up the local in the hash.

As I said, happy to merge this as is though.

@eightbitraptor
Copy link
Contributor Author

Thanks @kddnewton - those changes make sense to me. I'll merge this as is for now as it gets us a chunk closer on btest. I'll then immediately make issues for those two outstanding problems and start working on them.

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.

[COMPILER] Seg fault with locals within blocks within rescues

2 participants