Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support forwarding flags on scopes #3410

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Support forwarding flags on scopes #3410

merged 1 commit into from
Jan 14, 2025

Conversation

kddnewton
Copy link
Collaborator

When parent scopes around an eval are forwarding parameters (like *, **, &, or ...) we need to know that information when we are in the parser. As such, we need to support passing that information into the scopes option. In order to do this, unfortunately we need a bunch of changes.

The scopes option was previously an array of array of strings. These corresponded to the names of the locals in the parent scopes. We still support this, but now additionally support passing in a Prism::Scope instance at each index in the array. This Prism::Scope class holds both the names of the locals as well as an array of forwarding parameter names (symbols corresponding to the forwarding parameters). There is convenience function on the Prism module that creates a Prism::Scope object using Prism.scope.

In JavaScript, we now additionally support an object much the same as the Ruby side. In Java, we now have a ParsingOptions.Scope class that holds that information. In the dump APIs, these objects in all 3 languages will add an additional byte for the forwarding flags in the middle of the scopes serialization.

All of this is in service of properly parsing the following code:

def foo(*) = eval("bar(*)")

@kddnewton kddnewton force-pushed the forwarding-flags branch 3 times, most recently from ea3e0c7 to a51e770 Compare January 14, 2025 18:32
@eregon
Copy link
Member

eregon commented Jan 14, 2025

One thought: what if for simplicity we kept the existing structure (an array of array of strings) and represented that new information by simply adding *, **, &, or ... in those arrays, as "pseudo locals"?
Usages might even choose easily that way if they want to represent those as locals (a valid choice) or something more complicated.

When parent scopes around an eval are forwarding parameters (like
*, **, &, or ...) we need to know that information when we are in
the parser. As such, we need to support passing that information
into the scopes option. In order to do this, unfortunately we need
a bunch of changes.

The scopes option was previously an array of array of strings.
These corresponded to the names of the locals in the parent scopes.
We still support this, but now additionally support passing in a
Prism::Scope instance at each index in the array. This Prism::Scope
class holds both the names of the locals as well as an array of
forwarding parameter names (symbols corresponding to the forwarding
parameters). There is convenience function on the Prism module that
creates a Prism::Scope object using Prism.scope.

In JavaScript, we now additionally support an object much the same
as the Ruby side. In Java, we now have a ParsingOptions.Scope class
that holds that information. In the dump APIs, these objects in all
3 languages will add an additional byte for the forwarding flags in
the middle of the scopes serialization.

All of this is in service of properly parsing the following code:

```ruby
def foo(*) = eval("bar(*)")
```
@kddnewton
Copy link
Collaborator Author

One thought: what if for simplicity we kept the existing structure (an array of array of strings) and represented that new information by simply adding *, **, &, or ... in those arrays, as "pseudo locals"? Usages might even choose easily that way if they want to represent those as locals (a valid choice) or something more complicated.

It ends up being much easier to represent them separately since the then the size of the locals matches up with the size of the locals table, and you know it beforehand so you can allocate the a fixed size array.

@eregon
Copy link
Member

eregon commented Jan 14, 2025

I see, now that #3410 (comment) is clarified I think this approach is good.
I guess CRuby needs some possibly-hidden locals for */**/&/..., but those can then be easily added based on the flags (and might not be 1-1).
There might be some temporary locals too for things like flip-flops which are then added later but that's probably rare, most temp locals are probably just stack slots.
FWIW on TruffleRuby there are quite a few temp locals (at least currently) so predicating the local table size is not that useful.

@kddnewton kddnewton merged commit 00fae31 into main Jan 14, 2025
58 checks passed
@kddnewton kddnewton deleted the forwarding-flags branch January 14, 2025 20:31
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.

2 participants