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

#each's return takes precendence over inner block's return with a lambda #3143

Closed
robin850 opened this Issue Jul 16, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@robin850
Contributor

robin850 commented Jul 16, 2015

Hello,

There's a behavioral difference between JRuby and MRI when calling #each. If a lambda with a return statement is called inside #each, the result of the latter will be returned rather than the lambda's one:

require 'minitest/autorun'

class LambdaTest < Minitest::Test
  def bar(&b)
    [1].each { -> { self.instance_exec(&b) }.call }
  end

  def test_return_with_lambda
    assert_equal "foo", bar { return "foo" }
  end
end

Have a nice day!

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

Works in 1.7, does not work in 9k HEAD.

Member

headius commented Jul 16, 2015

Works in 1.7, does not work in 9k HEAD.

@headius headius added this to the JRuby 9.0.0.0 milestone Jul 16, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

A workaround for you until we fix this: use "break" or "next" instead of "return". Break returns from the method receiving the block (bar) and next returns from the block itself (allowing the lambda to finish normally).

I would guess we're not being strict enough about non-local returns that pass through a containing lambda.

Member

headius commented Jul 16, 2015

A workaround for you until we fix this: use "break" or "next" instead of "return". Break returns from the method receiving the block (bar) and next returns from the block itself (allowing the lambda to finish normally).

I would guess we're not being strict enough about non-local returns that pass through a containing lambda.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

Output on 9k:

[] ~/projects/jruby $ jruby return_test.rb 
Run options: --seed 52562

# Running:

F

Finished in 0.029812s, 33.5440 runs/s, 33.5440 assertions/s.

  1) Failure:
LambdaTest#test_return_with_lambda [return_test.rb:9]:
Expected: "foo"
  Actual: [1]

1 runs, 1 assertions, 1 failures, 0 errors, 0 skips
Member

headius commented Jul 16, 2015

Output on 9k:

[] ~/projects/jruby $ jruby return_test.rb 
Run options: --seed 52562

# Running:

F

Finished in 0.029812s, 33.5440 runs/s, 33.5440 assertions/s.

  1) Failure:
LambdaTest#test_return_with_lambda [return_test.rb:9]:
Expected: "foo"
  Actual: [1]

1 runs, 1 assertions, 1 failures, 0 errors, 0 skips
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

The problem is actually that the lambda (->) is capturing the return. As a result, each's return value is returned from the bar method.

It passes on MRI and JRuby 1.7 because the assert never fires. The return bubbles all the way out of the test method, as it should.

Simplest repro I can come up with:

def foo
  ->{ yield }.call
  return 1
end

def bar
  foo { return 2 }
end

p bar
Member

headius commented Jul 16, 2015

The problem is actually that the lambda (->) is capturing the return. As a result, each's return value is returned from the bar method.

It passes on MRI and JRuby 1.7 because the assert never fires. The return bubbles all the way out of the test method, as it should.

Simplest repro I can come up with:

def foo
  ->{ yield }.call
  return 1
end

def bar
  foo { return 2 }
end

p bar
@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Jul 16, 2015

Contributor

I think clearly speccing this behavior will help fix this

  • lambda { m1 { m2 ... { .. return } ... } } <-- should capture return .. correct?
  • lambda { m1 { m2 ... { .. no return here .. } ... } } <-- should not capture returns .. correct?

If this is indeed the right thing, then lexical scoping checks in IRRuntimeHelpers while the return is bubbling might work. Will require twidding some things in there (not sure if the IRReturnJump object needs more state).

Contributor

subbuss commented Jul 16, 2015

I think clearly speccing this behavior will help fix this

  • lambda { m1 { m2 ... { .. return } ... } } <-- should capture return .. correct?
  • lambda { m1 { m2 ... { .. no return here .. } ... } } <-- should not capture returns .. correct?

If this is indeed the right thing, then lexical scoping checks in IRRuntimeHelpers while the return is bubbling might work. Will require twidding some things in there (not sure if the IRReturnJump object needs more state).

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

I have a fix locally. I think we've determined the logic is that return always goes to the nearest return-capturing lexical enclosure.

The fix makes the original case pass along with every other case we've formulated, and appears to pass tests. I'll push momentarily.

Member

headius commented Jul 16, 2015

I have a fix locally. I think we've determined the logic is that return always goes to the nearest return-capturing lexical enclosure.

The fix makes the original case pass along with every other case we've formulated, and appears to pass tests. I'll push momentarily.

@headius headius closed this in eff3dcc Jul 16, 2015

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

@subbuss I'd appreciate your review if you get a chance!

Member

headius commented Jul 16, 2015

@subbuss I'd appreciate your review if you get a chance!

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Jul 17, 2015

Contributor

Seems reasonable to me.

Contributor

subbuss commented Jul 17, 2015

Seems reasonable to me.

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Jul 17, 2015

Contributor

Awesome, thanks for the quick fix! ❤️

Contributor

robin850 commented Jul 17, 2015

Awesome, thanks for the quick fix! ❤️

headius added a commit to ruby/spec that referenced this issue Jul 27, 2015

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