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

@headius my comments on #2198 indicates that I opened a new task for the JIT. Not sure if that is resolved or not. In any case, I assume you got this one.

@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:

• Attribute readers, like foo.bar, just compile like normal calls now. Normal calls use standard on-stack mechanisms for checking caller and visibility, so those mechanisms should be ok for attribute writers.
• I failed to notice that AttrAssignInstr used Helpers.invoke, which does indeed require a frame to do visibility checking. It also means that all attr assignment from IR is doing those calls uncached.

My patch does the following:

• Modifies the base CallType for AttrAssignInstr to NORMAL so the base class's callSite field gets initialized properly.
• Adds a second CallSite for functional (i.e. visibility-free) invocation in the cases where object == self. This may not be necessary if our "normal" call site is doing this visibility check correctly.
• Modifies the interpret method to use the appropriate site to make the call, which eliminates the need for a frame and introduces call site caching.

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

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.

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).

        compileCallCommon(
                jvmMethod(),
                attrAssignInstr.getName(),
                callArgs,
                attrAssignInstr.getReceiver(),
                callArgs.length,
                null,
                false,
                attrAssignInstr.getReceiver() instanceof Self ? CallType.FUNCTIONAL : CallType.NORMAL,
                null,
                attrAssignInstr.isPotentiallyRefined());

This may simplify my patch, but I'll let the PR's CI run its course before trying that.

See #4139 for the deprecations that go along with #4138.

Wow, thanks for the fast fix