#537 ✓resolved
Betelgeuse

`nil._?.any_method` should return `nil` without calling the method

Reported by Betelgeuse | November 15th, 2009 @ 07:47 AM | in Hobo 1.0 - Final

>> ENV['FOOBAR']._?.to_i
=> 0
>> ENV['FOOBAR']._?.length
=> nil

Intuitively these should both behave the same way. Either the code should be fixed or the documentation here changed to mention that _? calls the nil public instance methods.

http://cookbook.hobocentral.net/manual/hobosupport

Comments and changes to this ticket

  • Bryan Larsen

    Bryan Larsen November 16th, 2009 @ 09:12 PM

    According to the documentation, this appears to be the crucial difference between ._?. and .try. so yes, this behaviour is surprising.

  • Tom Locke

    Tom Locke November 17th, 2009 @ 03:50 PM

    • Milestone set to Hobo 1.0 - Final

    Agreed, nil._?.to_i should definitely return nil not 0

  • Tom Locke

    Tom Locke November 17th, 2009 @ 03:50 PM

    • Tag changed from defect, doc, hobosupport to defect, hobosupport
  • Tom Locke

    Tom Locke November 17th, 2009 @ 04:04 PM

    • State changed from “new” to “open”
  • Bryan Larsen

    Bryan Larsen November 17th, 2009 @ 05:07 PM

    Current behaviour:

    SafeNil responds to SafeNil.instance.methods as well as nil.methods. Should we redefine all/most SafeNil methods? Should we stop responding to nil.methods?

    >> SafeNil.instance.methods.sort
    => ["==", "===", "=~", "_?", "__id__", "__is_a__", "__metaclass__", "__send__", "_dump", "`", "acts_like?", "b64encode", "blank?", "breakpoint", "class", "class_eval", "classy_module", "clone", "copy_instance_variables_from", "daemonize", "dbg", "dclone", "debugger", "decode64", "decode_b", "display", "dup", "duplicable?", "enable_warnings", "encode64", "enum_for", "eql?", "equal?", "expects", "extend", "extend_with_included_modules_from", "extended_by", "freeze", "frozen?", "hash", "html_escape", "id", "in?", "inspect", "instance_eval", "instance_exec", "instance_of?", "instance_values", "instance_variable_defined?", "instance_variable_get", "instance_variable_names", "instance_variable_set", "instance_variables", "is_a?", "is_one_of?", "it", "its", "kind_of?", "load_with_new_constant_marking", "meta_def", "meta_eval", "metaclass", "metaclass_eval", "method", "method_exists?", "method_missing", "methods", "mocha", "mocha_inspect", "nil?", "not_in?", "object_id", "orig_pretty_print", "present?", "pretty_inspect", "pretty_print", "pretty_print_cycle", "pretty_print_inspect", "pretty_print_instance_variables", "private_methods", "protected_methods", "public_methods", "remove_subclasses_of", "require", "require_association", "require_dependency", "require_library_or_gem", "require_or_load", "reset_mocha", "respond_to?", "returning", "send", "silence_stderr", "silence_stream", "silence_warnings", "singleton_methods", "stubba_method", "stubba_object", "stubs", "subclasses_of", "suppress", "taguri", "taguri=", "taint", "tainted?", "tap", "to_a", "to_enum", "to_json", "to_matcher", "to_param", "to_query", "to_s", "to_yaml", "to_yaml_properties", "to_yaml_style", "try", "type", "unloadable", "untaint", "with_options"]
    >> nil.methods.sort
    => ["&", "==", "===", "=~", "^", "_?", "__id__", "__is_a__", "__metaclass__", "__send__", "`", "acts_like?", "b64encode", "blank?", "breakpoint", "class", "class_eval", "classy_module", "clone", "copy_instance_variables_from", "daemonize", "dbg", "dclone", "debugger", "decode64", "decode_b", "display", "dup", "duplicable?", "enable_warnings", "encode64", "enum_for", "eql?", "equal?", "expects", "extend", "extend_with_included_modules_from", "extended_by", "freeze", "frozen?", "hash", "html_escape", "id", "in?", "inspect", "instance_eval", "instance_exec", "instance_of?", "instance_values", "instance_variable_defined?", "instance_variable_get", "instance_variable_names", "instance_variable_set", "instance_variables", "is_a?", "is_one_of?", "it", "its", "kind_of?", "load_with_new_constant_marking", "meta_def", "meta_eval", "metaclass", "metaclass_eval", "method", "method_exists?", "methods", "mocha", "mocha_inspect", "nil?", "not_in?", "object_id", "orig_pretty_print", "present?", "pretty_inspect", "pretty_print", "pretty_print_cycle", "pretty_print_inspect", "pretty_print_instance_variables", "private_methods", "protected_methods", "public_methods", "remove_subclasses_of", "require", "require_association", "require_dependency", "require_library_or_gem", "require_or_load", "reset_mocha", "respond_to?", "returning", "send", "silence_stderr", "silence_stream", "silence_warnings", "singleton_methods", "stubba_method", "stubba_object", "stubs", "subclasses_of", "suppress", "taguri", "taguri=", "taint", "tainted?", "tap", "to_a", "to_enum", "to_f", "to_i", "to_json", "to_matcher", "to_param", "to_query", "to_s", "to_yaml", "to_yaml_properties", "to_yaml_style", "try", "type", "unloadable", "untaint", "with_options", "|"]
    >> nil.methods.sort - SafeNil.instance.methods.sort
    => ["&", "^", "to_f", "to_i", "|"]
    >> SafeNil.instance.methods.sort - nil.methods.sort
    => ["_dump", "method_missing"]
    
  • Bryan Larsen
  • Tom Locke

    Tom Locke November 18th, 2009 @ 03:13 PM

    Look good - more efficient too : )

    I would have said those should be class_eval and not eval, but I'm
    assuming your tests pass so I must be wrong about that.

  • Matt Jones

    Matt Jones November 18th, 2009 @ 07:14 PM

    I don't think we should be redefining to_i to return nil; it returns a number for nearly anything else (including nil) passed in. It belongs (IMHO) to the same category as to_s and the others.

  • Matt Jones

    Matt Jones November 18th, 2009 @ 07:17 PM

    One other note - returning 'nil' from method_missing on SafeNil means that any extension that defines extra methods on NilClass will behave oddly depending on whether _? is used or not. I can't think of any obvious uses at the moment, but it seems unhelpful to break other people's code, especially when it worked before this change.

  • Bryan Larsen

    Bryan Larsen November 18th, 2009 @ 07:50 PM

    AFAICT The original bug report wanted _?.to_i to return nil. More accurately, he wanted the documentation to tell him what it would return. So I don't mind adding it to the whitelist.

    Look at it this way, if somebody wants to call a function that exists on nil, there is no need to use _?. ENV['FOOBAR'].to_i will always return an integer, whether ENV['FOOBAR'] returns nil or a string. The old behaviour for _? was useless in this case, just slowing down the method call.

    If you want to retain standard nil behaviour, don't use _? or use .try. If a library adds methods to Nil, I can't see why they would expect them to remain on SafeNil.

    Caveat: I've never actually used _? before. I use try quite frequently, and didn't understand why there were two different implementations of two very similar things. This new behaviour and documentation makes the difference much more clear to me.

    What's the use case for the old behavior of _?

    Because you're right -- using a whitelist like I do here does kind of give me the jeebies. There's a reason I haven't pushed it yet.

  • Matt Jones

    Matt Jones November 18th, 2009 @ 09:02 PM

    I've always just read _? as a nice version of the 'rescue nil' idiom that could be chained and didn't swallow other exceptions (which is exactly what the current implementation does). In other words:

    result = x._?.some_method._?.some_other_method
    
    is equivalent to 
    
    t1 = if x.nil?
           x.some_method rescue nil
         else
           x.some_method
         end
    result = if t1.nil?
               t2.some_other_method rescue nil
             else
               t2.some_other_method
             end
    

    Note that the big difference between .try. and .?. in this scenario is that .?. will throw a NoMethodError if the target isn't nil but the method doesn't exist. .try. will swallow the exception in every case.

    In thinking about this, there's also some nasty mess from overriding send - this is no longer true:

    
    x._?.to_s == x._?.send(:to_s)
    

    Again, either expression is pretty silly, but could appear in some metaprogrammed / generated code.

    Maybe it would be preferable to fix the documentation?

    Sorry for harping on this - I think too much time spent on rails-core has turned me into a backwards-compatibility nazi... :)

  • Betelgeuse

    Betelgeuse November 18th, 2009 @ 10:19 PM

    I don't think we must nail ourself to some apis before 1.0 is released if it makes sense.

  • Tom Locke

    Tom Locke November 19th, 2009 @ 10:05 AM

    I've always just read _? as a nice version of the 'rescue nil' idiom that could be chained and didn't swallow other exceptions (which is exactly what the current implementation does).

    OK that's a communication / documentation problem then. _? was definitely never intended to swallow exceptions. It's supposed to be a short hand for "only if not nil", so e.g.

    result = x._?.some_method._?.some_other_method
    

    Is equivalent to

    tmp = (x != nil) && x.some_method
    result = (tmp != nil) && tmp.some_other_method
    

    With that understanding, we definitely should be redefining to_i to return nil, because _? is a "special operator" that messes with the method that comes next. In the same way, foo.*.to_i doesn't return an int either - it returns an array of ints.

    An example of using _?.to_i would be:

    my_score = get_score._?.to_i
    

    Where get_score is some method for fetching my score but happens to return a string. If I don't have a score (i.e. get_score returns nil), that's definitely not the same as having a score of zero. I don't want to do

    my_score = get_score && get_score.to_i
    

    Because get_score happens to be expensive, and I don't want to do

    my_score = (s = get_score and s.to_i)
    

    Because it's ugly :-) OK it's only a bit ugly but it's a very common pattern and I wanted a way to express it clearly and succinctly.

    What I really wanted was object.?method, which a language I created back in the day used to have, and I think Groovy also has it.

    I guess these issues are why a lot of folks would prefer to see _?(:to_i) and try(:to_i), (as in ActiveSupport)

  • Bryan Larsen

    Bryan Larsen November 19th, 2009 @ 04:56 PM

    • State changed from “open” to “resolved”

    (from [a13c22064661c294a91ede7ab387a00925fab7cd]) [#537 state:resolved]

    This changes the behaviour of the ._?. "dot". It now returns 'nil'
    rather than calling the trailing function more often.
    http://github.com/tablatom/hobo/commit/a13c22064661c294a91ede7ab387...

  • Bryan Larsen

    Bryan Larsen December 3rd, 2009 @ 03:30 PM

    Colin,

    your new implementation is very close in spirit to our implementation. Do you have any issues with our new implementation or is your comment an endorsement of it?

    The biggest difference is that we define to_s, to_json and to_yaml. We did find a couple of instances of breakage without the to_s -- defining that let's people use safe nil inside of an erb block cleanly.

    Also we use an eval def block instead of undef, which was silly. I'm not sure why I did that. :)

  • Tom Locke

    Tom Locke December 5th, 2009 @ 12:39 PM

    I agree that Colin's equivalent captures the semantics nicely. We should put that in the docs. Although I think it should be

    (nil == x ? nil : x.some_method)
    

    As that avoid issues with people messing with #nil? and #equals

    Colin's implementation seems to be good too - we should definitely avoid the "wrong number of arguments" problem. Shall we switch to this implementation?

  • Tom Locke

    Tom Locke December 5th, 2009 @ 12:40 PM

    #equals? Wow I regressed into Java : )

    #== I mean

  • Bryan Larsen

    Bryan Larsen December 10th, 2009 @ 08:55 AM

    Colin,

    You shouldn't get the "wrong number of arguments" error with the new definition. We swallow arguments (and even a block, which is why we use eval rather than define_method).

    I can't remember where the _? was showing up in code, because I fixed it. However, the idiom was something like

    <%= model._?.function %>
    

    <%= implicitly calls to_s on the inner portion, so having SafeNil return an empty string for to_s just like nil does is useful.

    OTOH, your example is also useful.

    The workaround for my example would be to append || "", which is a shorter and clearer workaround than your example. So it sounds like you're winning that part of the argument.

    Tom:

    In terms of implementation, if you look at our implementation, I think you'll agree that we're covering a few more bases better. Specifically our first eval block is necessary, I think. Our second eval block should be an undef, and Colin's suggesting we remove to_s and friends from NIL_RESPONSE_METHODS. I'm inclined to agree, although there's certainly merit on both sides of the argument for this one. Another question would be whether object_id should belong to DONT_REDEFINE_METHODS or NIL_RESPONSE_METHODS...

  • Tom Locke

    Tom Locke December 10th, 2009 @ 04:45 PM

    Bryan if you are happy with the implementation that's fine by me.

    having SafeNil return an empty string for to_s just like nil does is useful.

    I'm not sure if I'm reading this right, so can I just double check that the following holds?

    x._?.any_method
    

    Is equivalent to

    (nil == x ? nil : x.any_method)
    
  • Bryan Larsen

    Bryan Larsen December 11th, 2009 @ 04:29 PM

    • State changed from “resolved” to “open”

    Doh! You're right. We will take to_s out of NIL_RESPONSE_METHODS for RC3.

  • Tom Locke

    Tom Locke December 11th, 2009 @ 05:13 PM

    I guess I missed this earlier, because you did send me the code for review Bryan and I OKd it, but on closer inspection I'm not so sure.

    The semantics are that

    x._?.any_method
    

    is a shorthand for

    (nil == x ? nil : x.any_method)
    

    Notice that in the case that nil == x, we don't call any method at all. I don't think we should be calling nil.foo in any situation. i.e. we should never do nil.send(...), we should just return nil.

  • Bryan Larsen

    Bryan Larsen December 12th, 2009 @ 09:16 PM

    rdoctest files are valid markdown, so you can use any markdown
    formatter, including the dingus:
    http://daringfireball.net/projects/markdown/dingus

    Bryan

  • Bryan Larsen

    Bryan Larsen December 14th, 2009 @ 10:56 PM

    • State changed from “open” to “resolved”
  • Bryan Larsen
  • Bryan Larsen

    Bryan Larsen December 17th, 2009 @ 04:42 AM

    • State changed from “resolved” to “open”

    Note that ruby1.9.1 gives this complaint with the most recent patch:

    /home/blarsen/work/hobo-system-test/hobo/hobosupport/lib/hobo_support/methodcall.rb:59: warning: undefining `object_id' may cause serious problem
    

    Should we stop undef'ing this method just to silence the warning?

  • Matt Jones

    Matt Jones December 17th, 2009 @ 09:55 PM

    Note that both AssociationProxy and Scope in activerecord explicitly tiptoe around undef-ing or delegating object_id; not sure exactly what it breaks if you do, but leaving it alone would probably be a good idea.

  • Tom Locke

    Tom Locke December 18th, 2009 @ 10:08 AM

    Since the use of SafeNil is always transitory, I think this it's probably safe to go with silence_warnings and undef object_id. I would expect problems only to come up if someone does

    x = foo._?
    

    and hangs on to one of these. We should mention in the docs that this is a bad bad idea.

    OTOH maybe it's like crossing the beams in Ghostbusters, and you just don't do it and you don't ask why : )

  • Tom Locke

    Tom Locke December 18th, 2009 @ 10:13 AM

    • Title changed from “SafeNil with nil public methods as in ._?.to_i ” to “`nil._?.any_method` should return `nil` without calling the method”

    I just tried this in 0.9.103 and the behavior is still wrong

    >> nil._?.to_s
    => ""
    

    nil._?.any_method should return nil without calling the method.

    I will update the ticket title to avoid any confusion.

  • Bryan Larsen

    Bryan Larsen December 29th, 2009 @ 06:40 PM

    • State changed from “open” to “resolved”

    I would rather have just left object_id alone rather than silencing the warnings, but your year of experience carries some weight, plus the fact you also wrote the patch, plus Tom's endorsement. So you win. I've applied your fork and will push as soon as my tests complete.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

Referenced by

Pages