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

Inconsistency between CRuby and TruffleRuby with pattern matching #3678

Open Th3-M4jor opened 3 weeks ago

Th3-M4jor commented 3 weeks ago

When using pattern matching on a HashWithIndifferentAccess from ActiveSupport there appears to be a difference in behavior.

Given the below example, using ActiveSupport 7.2.1

require 'active_support/all'

test = HashWithIndifferentAccess.new('a' => true)

case test
in {a: b}
  b
else
  false
end

When run on CRuby 3.3.5 it will return true, while on TruffleRuby 24.2.0-dev-512427a6 it'll return false

This appears to work on CRuby because of three things:

eregon commented 3 weeks ago

Thank you for the report.

This is because https://github.com/oracle/truffleruby/blob/5aa58980aefa26688671dc1aa58752907b81f475/src/main/java/org/truffleruby/parser/YARPPatternMatchingTranslator.java#L313 uses the original Hash#[] definition and does not do a dynamic call to []/key?. It's a lot more efficient to not call both of these methods.

I think this should be fixed in the semantics of pattern matching in CRuby, i.e. to not call methods dynamically as it's significantly more expensive and makes overall pattern matching slower. (and the same thing for array pattern matching) But I realize that's an uphill battle to change that.

Is this important in practice, does this get used in applications?

From https://github.com/rails/rails/blob/cca4db3db4a3a122ca3d9090f4cfd6754aed6487/activesupport/lib/active_support/hash_with_indifferent_access.rb#L388-L390 HashWithIndifferentAccess stores keys as Strings. It seems Hash pattern matching does not currently accept String keys unfortunately (it's a SyntaxError).

I suppose one approach here would be to check if the receiver class is Hash and in that case use HashGetOrUndefinedNode, and if it's not then use key? + [].

eregon commented 3 weeks ago

We should fix this for compatibility.