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

Warn about using `return` inside inline callback blocks [ci skip] #13271

Merged
merged 1 commit into from Dec 12, 2013

Conversation

Projects
None yet
5 participants
@chancancode
Member

chancancode commented Dec 11, 2013

The Problem

In previous versions of Rails, you used to be able to do this:

class MyModel < ActiveRecord::Base
  before_save do
    return if special_snow_flake?
    # ...more checks...
  end
end

...or...

class MyController < ApplicationController
  before_action do
    if maintenance_mode?
      render text: "<h1>Maintenance Mode</h1>" and return
    end
  end
end

This is no longer possible. Using a return (or break) inside a inline callback block now raises a LocalJumpError.

This behaviour was never intentionally supported.

The Solution

If you are currently using return statements inside an inline callback block, it is recommended that you explicitly define it as a method and pass a symbol to the callback macro instead (i.e. before_save :check_xyz).

The Details

The short version: it was working before because internally Rails used to turn the inline callbacks into actual methods on the class. It doesn't do that anymore, and returning from a block like this is not something you can do.

The long version requires a bit more explanation:

proc vs lambda

There are two flavours of Proc objects in Ruby - procs and lambdas. The rule of thumb is, proc behaves like a block and lambda behaves like a method. So you can return out of a lambda but not in a block:

# You cannot return from a proc
>> p = proc { return 1 }
=> #<Proc:0x007fbf9888a270@(irb):1>
>> p.lambda?
=> false
>> p.call
LocalJumpError: unexpected return
    from (irb):1:in `block in irb_binding'
    from (irb):3:in `call'
    from (irb):3
    from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/bin/irb:12:in `<main>'

# But you can return from a lambda
>> l = lambda { return 1 }
=> #<Proc:0x007fbf9886b140@(irb):4 (lambda)>
>> l.lambda?
=> true
>> l.call
=> 1

# Block arguments are converted into procs
>> def extract_block(&block); block; end
=> nil
>> e = extract_block { return 1 }
=> #<Proc:0x007fbf9883b328@(irb):10>
>> e.lambda?
=> false
>> e.call
LocalJumpError: unexpected return
    from (irb):10:in `block in irb_binding'
    from (irb):12:in `call'
    from (irb):12
    from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/bin/irb:12:in `<main>'

# While methods are converted into lambdas
>> class A; def a; return 1; end; end
=> nil
>> a = A.new.method(:a).to_proc
=> #<Proc:0x007fbf9a9dbed8 (lambda)>
>> a.lambda?
=> true
>> a.call
=> 1

As you can see, returning from a proc block causes an error, while returning from a lambda block does not. Also note that when you use an & to extract a block argument, it's converted into a proc block for you, meaning that you normally wouldn't be able to return from an inline block.

(Okay, I lied, you can in fact return from a proc, but it almost certainly does not do what you expect, so you should just never do that. @fxn gave an excellent explanation of that here)

Why was it working before?

In previous versions of Rails, inline callback blocks are internally being defined as methods on the class. Therefore you were allowed to do anything you can normally do inside a method, including using return.

As of 2b1500d, these inline blocks are being instance_exec-ed directly rather than being defined into methods. Therefore, this stopped working (again, because block arguments are extracted into procs not lambdas).

What should I do moving forward?

As mentioned above, you should just explicitly define the callback blocks as methods and pass a symbol to the callback "macros". This also has a side-effect of making these methods easily testable should the need arise.

However, there is one scenario that this might be challenging – dynamic callbacks.

Let's say you are writing a library that supplies (dynamic) default values for ActiveRecord models:

module HasDefaultValueFor
  extend ActiveSupport::Concern

  module ClassMethods
    def has_default_value_for(field, value = nil, &block)
      before_create do
        return unless self.send(field).present?

        if block
          self.send("#{field}=", block.call(self))
        else
          self.send("#{field}=", value)
        end

        # Always return true, otherwise when value is false or the block
        # evaluates to false, then the record won't be saved
        return true
      end
    end
  end
end

ActiveRecord::Base.send(:include, HasDefaultValueFor)

# Usage
# 
#  class User < ActiveRecord::Base
#    has_default_value_for :display_name { |u| u.first_name }
#    has_default_value_for :receive_marketing_emails, true
#  end

In this case, since has_default_value_for can be called multiple times on the same model class, it would be a little annoying having to define explicit methods and deal with potential name conflicts, etc. Using the inline block form of before_create here seems quite apt.

While it's possible to unfold the conditionals so that an explicit return isn't necessary, but it would be quite difficult in other cases especially when you are trying to explicitly return false to halt the callback chain.

In these cases you can consider using the following trick:

before_create(&->{
  return unless self.send(field).present?

  if block
    self.send("#{field}=", block.call(self))
  else
    self.send("#{field}=", value)
  end

  # Always return true, otherwise when value is false or the block
  # evaluates to false, then the record won't be saved
  return true
})

It takes advantage of a few things:

  1. ->{ } is the new syntax for lambda{ ... }

  2. &some_block in a method call expands a proc or lambda into a block argument

  3. Ruby tries to preserve the lambda/proc property of a Proc object whenever possible:

    >> l = lambda { return 1 }
    => #<Proc:0x007f931b879cf8@(irb):1 (lambda)>
    >> p = proc { return 1 }
    => #<Proc:0x007f931b86b0e0@(irb):2>
    >> lambda(&p).lambda?
    => false
    >> proc(&l).lambda?
    => true
    >> def extract_block(&block); block; end
    => nil
    >> extract_block(&l).lambda?
    => true
    >> extract_block(&p).lambda?
    => false
    >> def call_block(&block); block.call; end
    => nil
    >> call_block { return 1 }
    LocalJumpError: unexpected return
       from (irb):10:in `block in irb_binding'
       from (irb):9:in `call'
       from (irb):9:in `call_block'
       from (irb):10
       from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/bin/irb:12:in `<main>'
    >> call_block &p
    LocalJumpError: unexpected return
       from (irb):3:in `block in irb_binding'
       from (irb):9:in `call'
       from (irb):9:in `call_block'
       from (irb):11
       from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/bin/irb:12:in `<main>'
    >> call_block &l
    => 1

    This is JustRuby(tm), and it's documented behaviour, no black magic involved. The only case this won't help you is when you try to yield a block:

    >> def a; yield; end
    => nil
    >> a { return 1 }
    LocalJumpError: unexpected return
       from (irb):2:in `block in irb_binding'
       from (irb):1:in `a'
       from (irb):2
       from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/bin/irb:12:in `<main>'
    >> a &->{ return 1 }
    LocalJumpError: unexpected return
       from (irb):3:in `block in irb_binding'
       from (irb):1:in `a'
       from (irb):3
       from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/bin/irb:12:in `<main>'

    However I think this is unlikely to become a problem for ActiveSupport::Callbacks.

Further Reading

http://readruby.io/closures (Hint: if you are wondering why break doesn't work, you should read this)

Closes #12981

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode
Member

chancancode commented Dec 11, 2013

@rafaelfranca

View changes

Show outdated Hide outdated guides/source/upgrading_ruby_on_rails.md Outdated
@rafaelfranca

View changes

Show outdated Hide outdated guides/source/upgrading_ruby_on_rails.md Outdated
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Dec 11, 2013

Member

@rafaelfranca fixed, thanks!

Member

chancancode commented Dec 11, 2013

@rafaelfranca fixed, thanks!

rafaelfranca added a commit that referenced this pull request Dec 12, 2013

Merge pull request #13271 from chancancode/warn_about_using_return_in…
…_as_callbacks

Warn about using `return` inside inline callback blocks [ci skip]

@rafaelfranca rafaelfranca merged commit a9b3c8f into rails:master Dec 12, 2013

@fxn

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Dec 12, 2013

Member

❤️ sorry for not giving feedback couldn't yet have a look at the patch other than a quick scan, I'll do it soon anyway.

Member

fxn commented Dec 12, 2013

❤️ sorry for not giving feedback couldn't yet have a look at the patch other than a quick scan, I'll do it soon anyway.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 12, 2013

Member

👍 I merged since I'm trying to get the beta out this week, but please feel free to review

Member

rafaelfranca commented Dec 12, 2013

👍 I merged since I'm trying to get the beta out this week, but please feel free to review

This change applies to most places in Rails where callbacks are used, including
Active Record and Active Model callbacks, as well as "filters" in Action
Controller (e.g. `before_action`). See [this pull request](https://github.com/rails/rails/pull/13271)

This comment has been minimized.

@chancancode

chancancode Dec 12, 2013

Member

Meta pull request 🤘

@chancancode

chancancode Dec 12, 2013

Member

Meta pull request 🤘

This comment has been minimized.

@lucascaton

lucascaton Apr 25, 2014

Contributor

Haha, "meta pull request" -- awesome 😄

@lucascaton

lucascaton Apr 25, 2014

Contributor

Haha, "meta pull request" -- awesome 😄

coolo added a commit to coolo/open-build-service that referenced this pull request May 12, 2014

coolo added a commit to coolo/open-build-service that referenced this pull request May 12, 2014

coolo added a commit to coolo/open-build-service that referenced this pull request May 14, 2014

coolo added a commit to coolo/open-build-service that referenced this pull request May 15, 2014

coolo added a commit to openSUSE/open-build-service that referenced this pull request May 16, 2014

@rywall

This comment has been minimized.

Show comment
Hide comment
@rywall

rywall Dec 16, 2014

Contributor

Just a small point. It looks like you can use next to return from a proc (or lambda) instead of using the more verbose trick mentioned above.

>> a = proc { return false; puts 'nope' }
=> #<Proc:0x007fabcd9c9250@(irb):1>
>> a.call
LocalJumpError: unexpected return
  from (irb):1:in `block in irb_binding'
  from (irb):2:in `call'
  from (irb):2
  from /Users/rywall/.rbenv/versions/2.1.5/bin/irb:11:in `<main>'
>> b = proc { next false; puts 'nope' }
=> #<Proc:0x007fabcda1b758@(irb):3>
>> b.call
=> false
Contributor

rywall commented Dec 16, 2014

Just a small point. It looks like you can use next to return from a proc (or lambda) instead of using the more verbose trick mentioned above.

>> a = proc { return false; puts 'nope' }
=> #<Proc:0x007fabcd9c9250@(irb):1>
>> a.call
LocalJumpError: unexpected return
  from (irb):1:in `block in irb_binding'
  from (irb):2:in `call'
  from (irb):2
  from /Users/rywall/.rbenv/versions/2.1.5/bin/irb:11:in `<main>'
>> b = proc { next false; puts 'nope' }
=> #<Proc:0x007fabcda1b758@(irb):3>
>> b.call
=> false

@esambo esambo referenced this pull request Mar 20, 2017

Closed

Do not require db/seeds.sql #20

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