oracle / truffleruby

A high performance implementation of the Ruby programming language, built on GraalVM.
https://www.graalvm.org/ruby/
Other
3.02k stars 185 forks source link

Incorrect `class.singleton_class.prepend` behaviour. #2558

Open ioquatix opened 2 years ago

ioquatix commented 2 years ago

https://github.com/ioquatix/sus/runs/4726484058?check_suite_focus=true

This failure means there is probably something wrong with singleton_class and prepend in TruffleRuby.

Line one is what we got and line two is what we want.

describe Sus::With with nested contexts it can print context name test/sus/with.rb:7 it a test variable test/sus/with.rb:13
describe Sus::With with nested contexts it can print context name test/sus/with.rb:7 with a test variable

The tests pass on 2.7, 3.0 and 3.1

This is the method that formats the output: https://github.com/ioquatix/sus/blame/main/lib/sus/with.rb#L31-L38

This line should cause the class singleton method to use that implementation: https://github.com/ioquatix/sus/blame/main/lib/sus/with.rb#L13

eregon commented 2 years ago

@bjfish Could you take a look?

eregon commented 2 years ago

@bjfish found that this is caused by prepending twice the same module in the same ancestor chain, in different classes. TruffleRuby currently has the traditional Ruby behavior to only allow a module to be once in ancestors, but it seems this changed at some point in CRuby, and is now allowed for prepend but not for include.

That change is actually pretty drastic as it makes super a lot more complicated and probably slower, so it seems a lot of work to support it. For example, it's not possible by knowing the current method and the receiver to know the super method, e.g. M#foo could be twice in the super chain and there is no easy way to know which one we are in.

I would recommend to avoid prepending the same module multiple times in the same ancestry chain, that's really hard to reason about and I wouldn't be surprised if that might trigger some bugs on CRuby as well.

We'll eventually implement this, but this is rather low priority for now because it's a large effort and it seems very rarely used.

ioquatix commented 2 years ago

I see. Thanks for the explanation.