Clojure

Deref throws an unhelpful error message when used on something not dereferencable

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Duplicate
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

Consider the following code:

(def x 1)
(def y (ref 2))

(+ @x y)

Clojure throws a ClassCastException on cast to Future. This is a very unhelpful error message; why a Future, why not Ref, Atom etc. It would be nice if this failed more gracefully.

  1. deref.patch
    15/Sep/14 2:08 PM
    2 kB
    Tobias Kortkamp
  2. tests-patch.diff
    17/Sep/14 5:44 AM
    1.0 kB
    Mark Nutter

Activity

Alex Miller made changes -
Field Original Value New Value
Approval Triaged [ 10120 ]
Description Consider the following code:

(def x 1)
(def y (ref 2))

(+ @x y)


Clojure throws a ClassCastException on cast to Future. This is a very unhelpful error message; why a Future, why not Ref, Atom etc. It would be nice if this failed more gracefully.
Consider the following code:

{code}
(def x 1)
(def y (ref 2))

(+ @x y)
{code}

Clojure throws a ClassCastException on cast to Future. This is a very unhelpful error message; why a Future, why not Ref, Atom etc. It would be nice if this failed more gracefully.
Issue Type Defect [ 1 ] Enhancement [ 4 ]
Alex Miller made changes -
Labels errormsgs errormsgs newbie
Hide
Tobias Kortkamp added a comment -

Attached a patch with better error messages for deref. The above example now throws:

IllegalArgumentException class java.lang.Long is not derefable  clojure.core/deref (core.clj:2211)

and e.g.

(deref (delay 1) 500 :foo)

throws

IllegalArgumentException class clojure.lang.Delay is not derefable with a timeout  clojure.core/deref (core.clj:2222)
Show
Tobias Kortkamp added a comment - Attached a patch with better error messages for deref. The above example now throws:
IllegalArgumentException class java.lang.Long is not derefable  clojure.core/deref (core.clj:2211)
and e.g.
(deref (delay 1) 500 :foo)
throws
IllegalArgumentException class clojure.lang.Delay is not derefable with a timeout  clojure.core/deref (core.clj:2222)
Tobias Kortkamp made changes -
Attachment deref.patch [ 13341 ]
Hide
Mark Nutter added a comment - - edited

Patch file clj-1436-patch-2014-09-16.diff updates the deref function so that it checks whether its arg is a future before sending it to deref-future. It also updates the deref function to provide clearer error messages. If the arg is not a future, and does not implement IDeref, the patched version of deref throws an IllegalArgumentException with a message that the arg cannot be dereferenced because it is not a ref/future/etc.

Show
Mark Nutter added a comment - - edited Patch file clj-1436-patch-2014-09-16.diff updates the deref function so that it checks whether its arg is a future before sending it to deref-future. It also updates the deref function to provide clearer error messages. If the arg is not a future, and does not implement IDeref, the patched version of deref throws an IllegalArgumentException with a message that the arg cannot be dereferenced because it is not a ref/future/etc.
Mark Nutter made changes -
Attachment clj-1436-patch-2014-09-16.diff [ 13344 ]
Hide
Mark Nutter added a comment -

Oops, I had this page open from yesterday and didn't see the patch submitted by Tobias. His has everything mine does, so I'll withdraw mine.

Show
Mark Nutter added a comment - Oops, I had this page open from yesterday and didn't see the patch submitted by Tobias. His has everything mine does, so I'll withdraw mine.
Mark Nutter made changes -
Attachment clj-1436-patch-2014-09-16.diff [ 13344 ]
Hide
Mark Nutter added a comment -

One suggestion: the error message might sound better as "IllegalArgumentException cannot dereference clojure.lang.Delay; not a future or reference type".

Show
Mark Nutter added a comment - One suggestion: the error message might sound better as "IllegalArgumentException cannot dereference clojure.lang.Delay; not a future or reference type".
Hide
Mark Nutter added a comment -

Tobias' patch does not contain the tests I had in mine, so I'm re-submitting just the tests as tests-patch.diff. If you install the tests patch without installing the deref patch, the tests will fail with the error message "Wrong exception type when passing non-IDeref/non-future to deref/@". Applying the deref patch as well will allow the tests to pass.

Show
Mark Nutter added a comment - Tobias' patch does not contain the tests I had in mine, so I'm re-submitting just the tests as tests-patch.diff. If you install the tests patch without installing the deref patch, the tests will fail with the error message "Wrong exception type when passing non-IDeref/non-future to deref/@". Applying the deref patch as well will allow the tests to pass.
Mark Nutter made changes -
Attachment tests-patch.diff [ 13345 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Hide
Alex Miller added a comment -

This patch does not seem to consider performance implications of adding this check and its impact on inlining.

Show
Alex Miller added a comment - This patch does not seem to consider performance implications of adding this check and its impact on inlining.
Alex Miller made changes -
Approval Triaged [ 10120 ]
Hide
Nicola Mometto added a comment -

Dupe of CLJ-1162

Show
Nicola Mometto added a comment - Dupe of CLJ-1162
Alex Miller made changes -
Resolution Duplicate [ 3 ]
Status Open [ 1 ] Closed [ 6 ]
Hide
Andy Fingerhut added a comment -

Maybe it shouldn't be the deciding factor when selecting which duplicate ticket to close, but CLJ-1162 has 1 vote, this one has 3. Closing this one as the duplicate is discarding those votes, unless (a) they are copied over to the older ticket, or (b) consider closing the older ticket as a duplicate of this one instead.

Show
Andy Fingerhut added a comment - Maybe it shouldn't be the deciding factor when selecting which duplicate ticket to close, but CLJ-1162 has 1 vote, this one has 3. Closing this one as the duplicate is discarding those votes, unless (a) they are copied over to the older ticket, or (b) consider closing the older ticket as a duplicate of this one instead.
Hide
Alex Miller added a comment -

I pondered that myself - I usually look at votes, current status, and patch. In this case, I liked the patch on CLJ-1162 better (although they were pretty similar).

Show
Alex Miller added a comment - I pondered that myself - I usually look at votes, current status, and patch. In this case, I liked the patch on CLJ-1162 better (although they were pretty similar).

People

Vote (3)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: