Lint/EnsureReturn cop - why ensure must not return

[ckornaros]

In my code I have an ensure like that:

ensure
   return false unless successfully_updated(result)
end

And rubocop show me the following warning : Lint/EnsureReturn: Do not return from an ensure block.

But if I removed the return the result will not be the same.

Exemple:

def test_ensure_without_return
  true
ensure
  false  
end

test_ensure_without_return
=> true

def test_ensure_with_return
  true
ensure
  return false  
end

test_ensure_with_return
=> false

So is there a reason that ensure must not return a value ?

RuboCop version

0.54.0 (using Parser 2.5.0.5, running on ruby 2.3.1 x86_64-linux)

[mikegee]

Google lead me to this blog post. Apparently, ensure with explicit return also silently rescues.

[bbatsov]

I see an opportunity to improve the documentation of this cop. 😉

…

On Wed, 6 Jun 2018 at 2:53, Michael Gee ***@***.***> wrote: Google lead me to this blog post <http://blog.leshill.org/blog/2009/11/17/ensure-with-explicit-return.html>. Apparently, ensure with explicit return also silently rescues. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5949 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGVyhQbOS0aCbJTRBlQNouNUIdsB08sks5t5sV_gaJpZM4Ua4Eq> .

[Drenmi]

Care to open a PR with documentation improvement @ckornaros? 🙂

[ckornaros]

Thanks for the link @mikegee ! I didn't know that ensure with return have this strange behavior.
And I didn't notice this behavior in my code because I try something like that (see example bellow) to check if ActiveRecord::Rollback is raised even if return is used on ensure:

  def test_ensure_without_return
    Order.transaction do
      order = Order.last
      order.user_id = 1000008
      order.save
      p 'raise now!'
      raise ActiveRecord::Rollback
    end
  ensure
    p 'ensure'
    false
  end

Result:

(0.4ms) BEGIN
Order Load (0.3ms) SELECT "orders".* FROM "orders" ORDER BY "orders"."id" DESC LIMIT $1 [["LIMIT", 1]]
SQL (0.9ms) UPDATE "orders" SET "user_id" = $1, "updated_at" = $2 WHERE "orders"."id" = $3
[["user_id", 1000008], ["updated_at", "2018-06-06 10:15:17.737387"], ["id", 100000036]]
"raise now!"
(0.3ms) ROLLBACK
"ensure"
=> nil

  def test_ensure_with_return
    Order.transaction do
      order = Order.last
      order.user_id = 1000008
      order.save
      p 'raise now!'
      raise ActiveRecord::Rollback
    end
  ensure
    p 'ensure'
    return false
  end

Result:

(0.3ms) BEGIN
Order Load (0.3ms) SELECT "orders".* FROM "orders" ORDER BY "orders"."id" DESC LIMIT $1 [["LIMIT", 1]]
SQL (0.8ms) UPDATE "orders" SET "user_id" = $1, "updated_at" = $2 WHERE "orders"."id" = $3
[["user_id", 1000008], ["updated_at", "2018-06-06 10:16:54.228939"], ["id", 100000036]]
"raise now!"
(0.4ms) ROLLBACK
"ensure"
=> false

But because of the transaction, ActiveRecord::Rollback will always be raised in my case but not in all other case without transactions.
So I definitely agree that it's a good practice to avoid return in ensure.
@bbatsov @Drenmi It'll be good to document that and I'll do it if I have the time.

[amrrbakry]

Hey guys, I'd like to help with this issue. Is there anyone currently working on it?

[OmriSama]

Glad I found this thread; It's good to know more about how ensure works. We had an issue in production today because of a specific Rubocop fix that was made to our code.

[sbezugliy]

I have similar scenario, but I need to redirect every error/exception to the own handler. Which will log and process data. And in case of error I sending further some specific value for unification reasons as here is empty Array. Receiver method expects actually Array.

def rescue_empty_result
  raise EmptyValueError.new("Empty value happened..")
rescue => error
  ::Log.error(error, **debug_params)
ensure
  return []
end

If I will remove return from the ensure section I will have a true as result at the calling method.

It this case it is even sounds as wrong. )))