warning: OpenStruct#define_singleton_method accesses caller method's state and should not be aliased

[yamam]

jruby 9.3.3.0 now shows the following warning.
It did not show up in jruby 9.3.2.0.

jruby 9.3.3.0 (2.6.8) 2022-01-19 b26de1f5c5 OpenJDK 64-Bit Server VM 11.0.13+8-Ubuntu-0ubuntu1.20.04 on 11.0.13+8-Ubuntu-0ubuntu1.20.04 +jit [linux-x86_64]
uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/ostruct.rb:464: warning: OpenStruct#define_singleton_method accesses caller method's state and should not be aliased
uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/ostruct.rb:468: warning: OpenStruct#block_given? accesses caller method's state and should not be aliased

[yamam]

I'm sorry, I'll report an Issue to Jruby as I think it's a JRuby problem.

[marcandre]

I'll reopen, I imagine this is related to #30

[headius]

@marcandre This is the same issue for a couple other methods. I'm not sure why they were not reported before.

• define_singleton_method needs to read the caller's frame visibility. It might be possible to eliminate this requirement in JRuby. Aliasing to another method name means we might not prepare that frame, so we warn.
• block_given? also accesses the caller's frame, to get the block it was passed. This might also be possible to eliminate in JRuby, since the only time we can't get the passed block from the incoming arguments is when block_given? is called within a block body, which already forces a frame.

In the short term, though, these should probably be blacklisted for aliasing or overwriting, as they are part of the core plumbing of types and method calls.

[marcandre]

Hmm, except that I actually use define_singleton_method!.

define_singleton_method needs to read the caller's frame visibility

I'm not sure of the technical details, but isn't define_singleton_method always equivalent to singleton_class.define_method?

[headius]

Hmm, except that I actually use define_singleton_method!.

The problem is not the new name, it's that the old name is associated with a method that has special access to the caller frame. We know define_singleton_method' needs a frame. We do not know define-singleton_method!` needs a frame, so we warn.

The warning is because we do not necessarily know if we have already compiled code without a frame that calls define_singleton_method!.

isn't define_singleton_method always equivalent to singleton_class.define_method?

It is, and both require access to the caller's frame to get the current visibility.

[marcandre]

isn't define_singleton_method always equivalent to singleton_class.define_method?

It is, and both require access to the caller's frame to get the current visibility.

My point is I don't understand why define_singleton_method needs the current visibility (by which you mean private/protected/public, right?), since AFAIK the defined method is always public.

I'm even more confused now that you say that singleton_class also does, even though it makes no sense to me, and there is no warning when it is aliased by ostruct.

Final confusion: doesn't define_method need the current visibility and thus aliasing it should issue a warning but it doesn't currently?

[headius]

AFAIK the defined method is always public.

Hmmm, that may be the case. We implement define_singleton_method using define_method.
If you are right we can remove this restriction.

now that you say that singleton_class also does

define_method needs visibility, not singleton_class.

doesn't define_method need the current visibility and thus aliasing it should issue a warning but it doesn't currently

Are you aliasing it? It is defined on Module, not Object, so I would assume OpenStruct does not alias it.

Also, why are you aliasing the private method block_given?? Nobody can normally call it against another object, and if they did it would still act on the caller frame and nothing related to the object. Better to leave it untouched.

[headius]

$ rvm ruby-2.6.8 do ruby -e 'def foo; obj = Object.new; private; obj.define_singleton_method(:bar) {p :ok}; obj.bar; end; foo'
:ok

It does seem to be the case that define_singleton_method always defines public methods (which I suppose makes sense). I think we can remove the frame restriction for that one in 9.3.4 but I will confer with @enebo.

[marcandre]

Are you aliasing it?

Ugh, no, you're right of course.

So, in summary:

1. ostruct will not alias block_given? on JRuby
2. aliasing define_singleton_method should not warn on JRuby (to be confirmed)

Alternative to 2 would be for ostruct to call singleton_class!.define_method instead, but I'd rather avoid that if possible.

[headius]

Confirmed! I will deal with the define_singleton_method issue and report back. Just want a chance to look over the C code and confirm they always force public visibility.

[marcandre]

PR ready

[headius]

Back to work this morning and I will look into the define_singleton_method visibility thing to confirm.

[headius]

I have asked for clarification on ruby-core slack, but I think the answer is that since define_singleton_method switches the context from self (the object against which the call was made) to the singleton class, the scoping (cref scope) will never match so it defaults to public visibility. I have not been able to find a way to cause it to use the scope's visibility so I think we can move forward with making this explicit behavior.

Assuming this was the intent, I will push for documenting and specifying this in CRuby and rubyspec.

[marcandre]

Yes, that's my understanding too. Thanks 👍

[headius]

I've pushed jruby/jruby#7055 which makes public visibility the only visibility for define_singleton_method, which in turn fixes this issue:

$ jruby -v -e 'require "ostruct"'
jruby 9.3.4.0-SNAPSHOT (2.6.8) 2022-01-31 1108febcbb OpenJDK 64-Bit Server VM 11.0.4+11 on 11.0.4+11 +jit [darwin-x86_64]
/Users/headius/projects/jruby-9.3/lib/ruby/stdlib/ostruct.rb:468: warning: OpenStruct#block_given? accesses caller method's state and should not be aliased

I don't like breaking the one edge case I found without full buy-in from ruby-core, though...

$ rvm ruby-2.6.8 do ruby -e '$o = Object.new; class << $o; private; $o.define_singleton_method(:foo){}; end; $o.foo'
-e:1:in `<main>': private method `foo' called for #<Object:0x00007fcf0e00dc98> (NoMethodError)

Edit: fixed issue link

[marcandre]

Right, similarly

$ ruby -e 'class Foo; class << Foo; private; Foo.define_singleton_method(:foo){}; end; end; Foo.foo'
-e:1:in `<main>': private method `foo' called for Foo:Class (NoMethodError)

Feels very odd to me.

[headius]

See https://bugs.ruby-lang.org/issues/18561 for my CRuby issue to codify this behavior of define_singleton_method and def self.foo and eliminate the edge cases.

[headius]

Right, similarly

Much better example of an edge case, and one we might actually see in practice (though I highly doubt anyone would expect it to have different behavior than the unmatched-scoping case).

[headius]

@marcandre Not getting much buy-in for the edge-case fix, so I'm unsure how to proceed. We'd like to have a version of ostruct that does not warn, and I am reluctant to merge the explicit PUBLIC change into JRuby 9.3.4 without some acknowledgement from other ruby-core folks.

[marcandre]

Understood. I guess I'll change the code to use singleton_class!.define_method

[headius]

Just so you are aware, that will of course have effectively the same result as my JRuby PR, since the define_singleton_method! body will always have PUBLIC visibility when calliing define_method!. But it will definitely eliminate the warning.

[headius]

@marcandre After some team discussion, and some analysis and buy-in from @jeremyevans (https://bugs.ruby-lang.org/issues/18561, ruby/ruby#5636), we have decided to go ahead with the change in 9.3.4 (jruby/jruby#7055).

[marcandre]

Perfect, thanks. So avoiding only block_given? like in #41 is the only remaining issue then, right?

[headius]

Correct!

[marcandre]

Thanks for the reminder! Released from Lisboa as 0.5.4 :-)

[headius]

Thank you @marcandre!