Skip to content
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

Inconsistency between CRuby and TruffleRuby with pattern matching #3678

Open
Th3-M4jor opened this issue Oct 1, 2024 · 2 comments
Open

Inconsistency between CRuby and TruffleRuby with pattern matching #3678

Th3-M4jor opened this issue Oct 1, 2024 · 2 comments
Assignees

Comments

@Th3-M4jor
Copy link
Contributor

Th3-M4jor commented Oct 1, 2024

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:

  • HashWithIndifferentAccess extends Hash
  • Hash#deconstruct_keys returns self
  • The resulting iseq calls .key? followed by .[] on the returned object
@eregon
Copy link
Member

eregon commented Oct 1, 2024

Thank you for the report.

This is because

RubyNode valueOrUndefined = HashGetOrUndefinedNodeGen.create(key, readTemp);
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
Copy link
Member

eregon commented Oct 1, 2024

We should fix this for compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants