-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Rails new fails with 9.1.3.0 and "--dev" option #4134
Comments
The issue in #2198 seems to have been that it was supposed to present an error and the JIT did not, probably due to relaxed visibility-checking. That glossed over the actual problem, which is that attribute accessors are not checking visibility correctly. @subbuss fixed that in c0f90f4. I believe the JIT still may be skipping the visibility check. The problem now is that I reverted his change, not realizing why attribute accessors required visibility checking. That commit was 7bed12f. The proper fix would be to get visibility-checking working properly without requiring the frame self. This needs to be addressed for a quick 9.1.5.0 release. |
cc @enebo |
@subbuss Do you remember what issue you opened? :-) And yes, I'll try to fix it and let you know if I need assistance. |
I know ... I was kicking myself for not mentioning it in the comment. I'll have to look at github issues I created and find it. |
#2201 maybe? |
@subbuss And it looks like it's fixed...but now we have this issue. Thanks, I'll see what I can do. |
I'm working up a patch and have a few observations:
My patch does the following:
The JIT may work properly here because it already does use the actual caller to check visibility, without requiring a frame. |
I think I might also remove visibility-checking from Helpers.invoke*. Even a pro like me will miss that these methods require a Ruby frame to be set up, and I suspect usually we want an "invoke" from Java code to skip visibility. In any case, having it do the check and expect a frame seems very error-prone. |
Added PR with my changes: #4138 |
Good thing! This is also one of the reasons why I wanted the IR to be explicit about all the previously implicit work that was happening. I wanted it for 2 reasons: (a) clarity about operational semantics of ruby code and identifying where some of the complexity is inherent Ruby complexity vs. being accidental JRuby complexity (b) identifying scenarios where some of these operations can be eliminated. However, we have walked back on this design in a few places since the explicit nature of the operations means the interpreter has additional overhead. But, I think that we should largely follow this approach / principle. |
Note, it appears the JIT is choosing its call type the old way, at compile time by checking for literal "self". However, I think even in cases where the receiver is not literally "self", the normal site will do the right thing (checking if the caller is an instance of the receiver).
This may simplify my patch, but I'll let the PR's CI run its course before trying that. |
This has worked for the JIT and it runs almost all the same tests as the interpreter. See jruby#4134.
This is in response to callers not seting up frame and breaking visibility checks in jruby#4134. This goes along with jruby#4138.
This is in response to callers not seting up frame and breaking visibility checks in jruby#4134. This goes along with jruby#4138.
Use a CallSite for invocation of attr writers. Fixes #4134.
Wow, thanks for the fast fix |
Environment
JRUBY_OPTS='--dev'
Expected Behavior
$ rails new blankapp
Generates a new Rails app.
It is doing so with JRuby 9.1.2.0 (including '--dev') or with JRuby 9.1.3.0 excluding the '--dev' flag
Actual Behavior
Follow up ticket from #4127, probably related to #2198
The text was updated successfully, but these errors were encountered: