`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.
Comments and changes to this ticket
-
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 November 17th, 2009 @ 03:50 PM
- Milestone set to Hobo 1.0 - Final
Agreed,
nil._?.to_i
should definitely returnnil
not0
-
Tom Locke November 17th, 2009 @ 03:50 PM
- Tag changed from defect, doc, hobosupport to defect, hobosupport
-
Tom Locke November 17th, 2009 @ 04:04 PM
- State changed from new to open
-
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"]
-
Tom Locke November 18th, 2009 @ 03:13 PM
Look good - more efficient too : )
I would have said those should be
class_eval
and noteval
, but I'm
assuming your tests pass so I must be wrong about that. -
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 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 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 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 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 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 domy_score = get_score && get_score.to_i
Because
get_score
happens to be expensive, and I don't want to domy_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)
andtry(:to_i)
, (as in ActiveSupport) -
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 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 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?
-
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 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 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 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 callingnil.foo
in any situation. i.e. we should never donil.send(...)
, we should just returnnil
. -
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/dingusBryan
-
Bryan Larsen December 14th, 2009 @ 10:56 PM
- State changed from open to resolved
-
Bryan Larsen December 14th, 2009 @ 11:57 PM
(from [88c284bd0d04130e26386baf49b87407bdd86d50]) [#537 state:resolved] update CHANGES.txt http://github.com/tablatom/hobo/commit/88c284bd0d04130e26386baf49b8...
-
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 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 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 undefobject_id
. I would expect problems only to come up if someone doesx = 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 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 returnnil
without calling the method.I will update the ticket title to avoid any confusion.
-
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.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป
People watching this ticket
Attachments
Tags
Referenced by
- 537 SafeNil with nil public methods as in ._?.to_i (from [a13c22064661c294a91ede7ab387a00925fab7cd]) [#537 s...
- 537 SafeNil with nil public methods as in ._?.to_i (from [88c284bd0d04130e26386baf49b87407bdd86d50]) [#537 s...