Implicit blocks from Symbol#to_proc does surprising and wrong things (with --1.9) #274

Closed
bjeanes opened this Issue Aug 29, 2012 · 8 comments

Projects

None yet

3 participants

@bjeanes
bjeanes commented Aug 29, 2012

I've had trouble articulating this, so hopefully the following code will make it pretty explanatory:

Code

(Variants also available https://gist.github.com/3519907 and https://gist.github.com/3519564)

class Hash
  def map_values
    map { |k, v| [k, yield(v)] }
  end
end

p data = {"x"=>[{"y"=>42}]}

p(data.map_values { |x| x.first })
p data

p data.map_values(&:first)
p data

Correct output (jruby-1.6.7.2 --1.8, jruby-1.7.0-preview2 --1.8, mri 1.8.7, mri 1.9.2-p290)

{"x"=>[{"y"=>42}]}
[["x", {"y"=>42}]]
{"x"=>[{"y"=>42}]}
[["x", {"y"=>42}]]
{"x"=>[{"y"=>42}]}

Incorrect output (jruby-1.6.7.2 --1.9, jruby-1.7.0-preview2 --1.9)

{"x"=>[{"y"=>42}]}
[["x", {"y"=>42}]]
{"x"=>[{"y"=>42}]}
[["x", ["y", 42]]]
{"x"=>[]}

In my specific case, not only does my method (contrived — simplified from a real world project) very much return the wrong thing, it mutates the original Hash, even though no mutative methods were used.

@bjeanes
bjeanes commented Aug 29, 2012

I believe @lopex and @qmx have both replicated this bug in IRC

@bjeanes
bjeanes commented Aug 29, 2012

Note that using this as the method definition does work correctly:

class Hash
  def map_values(&block)
    map { |k, v| [k, block.call(v)] }
  end
end
@headius
Member
headius commented Aug 30, 2012

This is likely the same as some other block/proc dispatch issues where we're unwrapping too much or not unwrapping enough. Sigh.

@bjeanes
bjeanes commented Aug 30, 2012

Even adding a dup doesn't stop the mutation of the original hash. This is horrendously surprising behavior :(

class Hash
  def map_values
    dup.map { |k, v| [k, yield(v)] }
  end
end
@soulcutter

I think I have struck upon this problem as well... except I am getting an NPE.

    parser.for_tag(:blurb).inject([]) { |result, value| result << value } # this works
    parser.for_tag(:blurb).inject([], :<<) # NPE (see gist)

Example of NPE generated by inject with :<<
The spec generating the NPE

Not sure if it has something to do with the fact that I mix in Enumerable with my own each implementation (which uses block.call(value) fwiw), because I wasn't able to reproduce this with a simple Array#inject

I see this behavior only in jruby-1.6.7.2 --1.9 and jruby-1.7.0-preview2 --1.9

@soulcutter

Tried to boil down my inject issue to a much simpler test case

class Foo
  include Enumerable

  def initialize(*stuff)
    @stuff = stuff
  end

  def each(&block)
    @stuff.each { |thing| block.call(thing) }
  end
end

puts Foo.new(1, 2, 3).inject([]) { |result, value| result << value } # works
puts Foo.new(1, 2, 3).inject([], :<<) # NPE
@headius
Member
headius commented Sep 8, 2012

The original issue appears to be fixed now, probably due to the change I made to fix Symbol#to_proc argument handling earlier today (a964aa4).

The additional issue with index and :<< is probably something different, but I'll look into that too.

@headius
Member
headius commented Sep 8, 2012

Ok, so the new case with << actually works if you procify the symbol first, a la inject([], :<<). It only fails if you pass the symbol directly in, which should do the to_proc coercing for you exactly the same way. So inject is probably coercing wrong.

@headius headius added a commit that closed this issue Sep 8, 2012
@headius headius Fix #274 (second part)
Our Java-based #inject block did not report it wanted any args,
and it seems we were not giving it any. Modified to be "OPTIONAL"
and all cases seem to work.
bbe2e8f
@headius headius closed this in bbe2e8f Sep 8, 2012
@eregon eregon added a commit that referenced this issue Jul 18, 2016
@eregon eregon Squashed 'spec/ruby/' changes from 9b80404..ced79c8
ced79c8 Remove spec depending on platform-dependent behavior from ungetc/lseek.
26689c1 Added Exception#cause spec
36f0116 Add a Hash spec
1a7edec Import specs extracted from Rubinius' merged Codedb ffi io branch
4c86731 [Truffle] Add rb_gc_register_address and rb_ary_new4.
9f34a94 [Truffle] Pass all cext complex specs.
51b1b08 [Truffle] Pass all cext rational specs.
ec7a0fe [Truffle] Pass all cext mutex specs.
e9cb231 [Truffle] Pass all cext float specs.
3bce77e [Truffle] Pass all cext fixnum specs.
a9bdd5b [Truffle] rb_gc_enable and _disable
064fdca Add sort_by! spec for single-element array.
440c00f [Truffle] Use jt cextc as the cext compiler.
b223b45 Add an example for Array#slice!
87a550a Add missing require in shared Range specs
be39236 Add a case for nil&.attr+=val to the safe navigation operator specs
cba3695 Merge pull request #274 from iliabylich/add-specs-for-safe-navigator
ffabc7f Add specs for a safe navigator.
0a65ffe Merge pull request #272 from wied03/def_specs
88343ce Test return value of defined setters and []= methods and multiple arguments provided.
8e65d52 Merge pull request #273 from jrafanie/add_2_3_tests_for_reject_bang
e68a19a Add specs for Array#reject!, delete_if 2.3 change
147b216 Merge pull request #271 from odaira/myContribution
d02b3ac Skip specs using getsockopt, which is known to return a wrong length in AIX
c9634e9 Add ensure to the begin/rescue/else permutations for rescue_spec
7e05c6d Prevent regression of outer scope return value with begin/else logic
e1f7b0d Test rescue begin/else on methods as well as blocks
d4ced96 feature guard for fork

git-subtree-dir: spec/ruby
git-subtree-split: ced79c84680f8a82e099eb14c164a57ce56b44b0
9c36da2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment