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

Fix exception during Hash#each and Hash#each_key if keys get deleted during loop #2403

Merged
merged 3 commits into from
Mar 9, 2022

Conversation

janbiedermann
Copy link
Member

Given the following ruby:

o = { a: 1, b: 2, c: 3 }
h = o.dup
h.each { |k,v| h.delete(k) }
i = o.dup
i.each_key { |k| i.delete(k) }
puts "o: #{o}\nh: #{h}\ni: #{i}"

and executed with matz ruby results in the following expected output:

o: {:a=>1, :b=>2, :c=>3}
h: {}
i: {}

However, using opal master the following exceptions are thrown (need to comment out the .each ain above code to get the second exception):

<internal:corelib/hash.rb>:429:1:in `each': Cannot read properties of undefined (reading '$$is_string') (Exception)
        from <internal:corelib/runtime.js>:1900:5:in `Opal.send2'
        from <internal:corelib/runtime.js>:1888:5:in `each'
        from test.rb:3:2:in `undefined'
        from <internal:corelib/runtime.js>:2760:7:in `<main>'
        from test.rb:1:1:in `null'
        from node:internal/modules/cjs/loader:1103:14:in `Module._compile'
        from node:internal/modules/cjs/loader:1155:10:in `Module._extensions..js'
        from node:internal/modules/cjs/loader:981:32:in `Module.load'
        from node:internal/modules/cjs/loader:822:12:in `Module._load'
        
<internal:corelib/hash.rb>:450:1:in `each_key': Cannot read properties of undefined (reading '$$is_string') (Exception)
        from <internal:corelib/runtime.js>:1900:5:in `Opal.send2'
        from <internal:corelib/runtime.js>:1888:5:in `each_key'
        from test.rb:5:2:in `undefined'
        from <internal:corelib/runtime.js>:2760:7:in `<main>'
        from test.rb:1:1:in `null'
        from node:internal/modules/cjs/loader:1103:14:in `Module._compile'
        from node:internal/modules/cjs/loader:1155:10:in `Module._extensions..js'
        from node:internal/modules/cjs/loader:981:32:in `Module.load'
        from node:internal/modules/cjs/loader:822:12:in `Module._load'

This happens because the original $$keys aray is used for the loop. If that gets modified during the loop, the indexes fail to match during the loop, resulting in null keys and failing.

The patch copies the $$keys using .slice() (the supposedly fastest way to copy arrays) before entering the loop:

o: {"a"=>1, "b"=>2, "c"=>3}
h: {}
i: {}

@hmdne
Copy link
Member

hmdne commented Mar 9, 2022

Thanks! Let's merge!

@hmdne hmdne merged commit 8310df1 into opal:master Mar 9, 2022
@janbiedermann janbiedermann deleted the hash_each_delete branch March 13, 2022 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants