Skip to content

implement AS::Duration#send and #try to avoid unexpected behavior #320

Closed
wants to merge 1 commit into from

6 participants

@misfo
misfo commented Apr 27, 2011

since Duration inherits from ::BasicObject when using Ruby 1.9, send and try aren't implemented so they're delegated to the Integer value by method_missing. This causes unexpected behavior:

d = 1.day
d.inspect                #=> "1 day"
d.try(:inspect) || "N/A" #=> "86400"
# ^^^ standard null checking goes pear-shaped
@misfo misfo implement AS::Duration#send and #try to avoid unexpected behavior
since Duration inherits from BasicObject, send and try aren't implemented so
they're delegated to the Integer value by method_missing.  This causes
unexpected behavior:

  d = 1.day
  d.inspect                #=> "1 day"
  d.try(:inspect) || "N/A" #=> "86400"
  # ^^^ standard null checking goes pear-shaped
b508200
@henrikhodne

The code you submitted for try is not compliant with what try is supposed to do though (prevent whiny nils). Also, is there a reason why Duration inherits from BasicObject and not Object?

@misfo
misfo commented Apr 28, 2011

The commit won't affect NilClass#try at all, so it won't change how try "prevents whiny nils".

That is a good point about Duration's superclass, though. It's not clear to me why it inherits from BasicObject in the first place. Does anyone here on the Githubs know why that is?

@henrikhodne

Doi, I had a brain fart there about NilClass#try. You're absolutely right, this is the expected way of doing try. To make it even better, is there a way you can just mix in the try and send from Object? That way you don't have to update it in two places if it changes (which might not be that plausible at all).

@misfo
misfo commented Apr 28, 2011

Turns out making Duration simply inherit from Object fixes all the worlds problems. Thanks for questioning that @dvyjones

I made a new pull request here:
#334

@misfo misfo closed this Apr 28, 2011
@sikachu sikachu reopened this Jun 15, 2011
@sikachu
Ruby on Rails member
sikachu commented Jun 15, 2011

Reopen after some discussion in #334

Turned out that making it inherited from Object actually have more side effect than we want. So, @misfo if you would please rebase your patch, and make it as a module that we can reuse it later, that would be awesome.

Thank you for the hard work. I'm target this one for 3.2.x

@misfo
misfo commented Jun 16, 2011

Sure will. Thanks for taking a look at this. I'll comment on this pull when I've created a new one...

@jeremy
Ruby on Rails member
jeremy commented Oct 9, 2011

Ping. Did you make progress on this?

@misfo
misfo commented Oct 11, 2011

@jeremy, I looked at this again and I still think Duration should subclass Object as it did in the other pull. Duration#tap doesn't work as expected either when ActiveSupport::BasicObject is subclassed:

irb> 1.days.is_a?(ActiveSupport::Duration)
=> true
irb> 1.days.tap {|d| puts d.is_a?(ActiveSupport::Duration) }
false
=> 86400
irb> 1.days.tap {|d| d }.is_a?(ActiveSupport::Duration)
=> false

I understand that Duration should proxy a lot of its methods to its @value, but doesn't it make more sense to explicitly delegate the methods in Object.instance_methods - ActiveSupport::BasicObject.instance_methods that we want proxied. That way everything we want to be proxied is, but basic assumptions about try, send, tap, etc aren't violated.

@jeremy
Ruby on Rails member
jeremy commented Oct 11, 2011

As a proxy, it's clearer to be explicit about what we don't want to be proxied, rather than what we do. It's easier to maintain, too.

This could be a good case for a better supporting class, like ActiveSupport::ProxyObject, though.

@mhfs
mhfs commented Feb 28, 2012

@jeremy I was looking into this and it seems ActiveSupport::BasicObject (which ActiveSupport::Duration inherits from) is used only for Duration and an active resource spec. Also BasicObjects docs says "Used for proxy classes". Would be the case of renaming it into ProxyObject and placing send and try in there?

Or do you prefer Duration < ProxyObject < BasicObject?

Would be happy to provide a patch.

/cc @josevalim

@isaacsanders

Is this still an issue @misfo?

@misfo misfo added a commit to misfo/rails that referenced this pull request Apr 29, 2012
@misfo misfo Don't proxy Duration#object_id, #send, #tap, and #try
Proxying these methods cause unexpected behavior:

  d = 1.day
  d.inspect                #=> "1 day"
  d.try(:inspect) || "N/A" #=> "86400"
  # ^^^ standard null checking goes pear-shaped

related to #320, #334
966e55b
@misfo
misfo commented Apr 29, 2012

Yup, it's still an issue. I made another pull request that resolves @jeremy's concerns, though. It uses an Array of methods inherited from Object that we don't wanna proxy, but avoids reimplementing methods from Object like this pull does. Check it out: #6055

@isaacsanders

Is this okay to close?

@misfo
misfo commented Apr 29, 2012

Surely

@misfo misfo closed this Apr 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.