procore-oss / blueprinter

Simple, Fast, and Declarative Serialization Library for Ruby
MIT License
1.14k stars 109 forks source link

"LocalJumpError: unexpected return" after update to 1.0 #388

Closed jhollinger closed 10 months ago

jhollinger commented 10 months ago

I have a BP project with many field blocks using returns, and they're all breaking after the 1.0 update. Here's a contrived, yet illustrative, example:

field :widget_ids do |vendor, options|
  return [] unless options[:include_widget_ids] # <--- error!

  vendor.widget_ids
end

While 1.0 has breaking changes, I don't believe this was intended as one of them. And while it's "easy" to fix by changing the returns to nexts, the risk of missing some and breaking production is high.

I've been looking over the diff from v0.23.4 to 1.0.1 and I haven't yet found the culprit. (fwiw I suspect it's in the recent transformer changes.) I'll keep looking, but I'd be glad for any help.

njbbaer commented 10 months ago

The issue is not in the transformer changes, I saw it when bumping to 0.30.0.

jhollinger commented 10 months ago

I was going to use git bisect to find the changes, but I've gone all the way back to v0.15.0 and I see the same error...

njbbaer commented 10 months ago

I was going to use git bisect to find the changes, but I've gone all the way back to v0.15.0 and I see the same error...

Are you sure? It doesn't appear to be present in 0.23.4 on the Procore monolith.

jhollinger commented 10 months ago

I know, it doesn't make sense. I've got this BP test file locally:

# spec/units/return_spec.rb

require 'json'
require 'ostruct'

describe 'Return spec' do
  class BP < Blueprinter::Base
    fields :id, :name

    # Test that return works/doesn't work
    field :foo do |obj, options|
      return "foo!?"
    end
  end

  it 'should work with return' do
    BP.render(OpenStruct.new({
      id: 42,
      name: "Barp",
    }))
  end
end

Then I checked out the tag and that test still fails:

git checkout v0.23.4
bundle exec rspec

...
Failures:

  1) Return spec should work with return
     Failure/Error: return "foo!?"

     LocalJumpError:
       unexpected return
...

Beginning to wonder if some transformer or other config we have was papering over the issue, and some BP internal refactor made that ineffective?

njbbaer commented 10 months ago

@jhollinger I'm pretty sure this is not a Blueprinter issue. As your test shows, return in field blocks was never allowed by blueprinter and what you have was always a bug. This is the correct behavior IMO because returning from a Ruby block usually returns from the parent method rather than the block.

ritikesh commented 10 months ago

I think in some ruby version, block returns had a change. Can't pin-point to what exactly chamged/when. @jhollinger have you changed your ruby version recently?

jhollinger commented 10 months ago

Closing since this apparently never worked outside of our particular app.