-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fixes [Bug 21522] eval isolation in Ractors for Prism #14940
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
Conversation
|
Hello, are you a human? There are real humans reviewing PRs here, and it's somewhat rude to waste their time reviewing LLM output. I think you should at least disclose that this is AI generated. |
|
@XrXr I am human, and sorry, I don't know exactly how to respond to that, I made sure to properly understand the problem and solve it in a reasonable way, does my code make no sense somehow? Note: and yes I did use the help of AI, but I definately didn't do it blindly. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
|
The hallucinated bug number in the title was a red flag. Bugs in that syntax link to https://bugs.ruby-lang.org/ and https://bugs.ruby-lang.org/issues/22883 is 404. If it's a bug-report-with-fix-suggestion, the description is not very focused and smelled generated. I apologize for my previous response; it's pretty rude on my part to question whether you're human. I could've just pointed you to https://docs.ruby-lang.org/en/master/contributing/reporting_issues_md.html |
|
Oh, that bug number was totally my bad, I understand your reaction, I fixed with the correct issue: Bug 21522: Accessing outer locals via eval in a Ractor returns false |
445bb45 to
fca814b
Compare
fca814b to
1369d05
Compare
|
@XrXr is this okay for review? |
Earlopain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignoring the AI-generated description, this looks good to me compared to what parse.y does. Can you take a look @kddnewton?
kddnewton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Problem
Prism parser was not properly enforcing eval isolation in Ractors. Tests were added to verify eval behavior in Ractor isolation contexts. Test
eval with outer locals in a Ractor raises SyntaxErrorfailed with Prism but passed with parse.y.Tests Added
Three new test cases were added to verify eval isolation behavior in Ractors:
Behavior Before Fix
With parse.y (legacy parser):
With Prism (default) - BEFORE FIX:
Solution
Added helper functions to compute isolation depth consistently across both parsers:
Both pm_eval_make_iseq() (Prism) and eval_make_iseq() (parse.y) now use the same isolation detection logic.
Testing After Fix
With Prism - AFTER FIX:
Test
eval with outer locals in a Ractor raises SyntaxErrornow correctly raises SyntaxError when eval tries to access outer locals in a Ractor. Both parsers produce identical behavior.Root Cause
Prism was hardcoding 0 for isolated_depth when compiling eval, while parse.y was correctly detecting the isolation context. The fix ensures both parsers use the same detection mechanism.