better error message when passing non-seq to seq

Question

Design discussion [here|http://dev.clojure.org/display/design/Better+Error+Messages].

This patch improves Clojure's error message for a single common error: passing a non-seq where a seq is neede. More importantly, it is intended as a prototype for other similar improvements in the future.

Error message before:


(cons 1 2)
=> IllegalArgumentException Don't know how to create ISeq from: java.lang.Long


Error message after:


user=> (cons 1 2)
ExceptionInfo Don't know how to create ISeq from: java.lang.Long
user=> (ex-data *e)
{:instance 2}


*Patch:* better-error-message-for-seq.patch
NOTE: This patch was reverted as it affected the inlining of RT.seqFrom().

Answer 1

Comment made by: michaelklishin

Wouldn't it be better to make it read "Don't know how to create ISeq from: 2 (java.lang.Long)"? How many beginners will figure
out ex-data exists and how to use it?

Answer 2

Comment made by: stu

Hi Michael,

ex-info messages should not, in general, pr-str things into their bodies. This raises the question of print-length and print-level in a place where the user doesn't have good control, while the whole point of ex-info is to be in the data business, not the string business. Users can control printing from ex-data any way they like.

There are two possible ways to make beginners aware of ex-data: Tell them about it in one (or a few places) in docs, or in an infinite number of places saying "This would have been useful here, but we didn't use it because you might not know about it." I prefer the former.

That said, I think it would be great to increase the visibility of ex-info and ex-data early on in documentation for beginners, and to make sure that things like exception printing in logs are flexible enough not to lose the benefits of ex-info.

Answer 3

Comment made by: jafingerhut

Just a comment that this fix was committed before release 1.6.0, and then reverted very shortly before release 1.6.0. I believe the reason for reverting was due to concerns that this change made performance about 5% slower in some relatively common cases, with a suspicion that it could have affected inlining of the seqFrom method.

Not sure whether the ticket should be reopened or not.

Answer 4

Comment made by: gshayban

I think we can now revisit this. The original patch changed the size of the seqFrom method from 133 bytes to 152 bytes, (link: https://groups.google.com/d/msg/clojure/K6W7DbxV0Cw/N3onXKkFVX8J text: affecting inlining as discussed here). If we move the exception construction out to a method as in (link: ^CLJ-1099-better-error-message.txt), the method becomes 100 bytes.

There are also a whole class of (link: https://github.com/clojure/clojure/blob/8c402a8c9695a4eddc07cbbe0d95d44e1372f0bf/src/jvm/clojure/lang/RT.java#L853 text: "faux protocol" invokes with similar failing branches ) where extracting the ex constructor in a similar way could lead to 1) better error with error data 2) smaller method bodies

Answer 5

Comment made by: admin

Would be happy to revisit - new patch causes many test failures so that needs to be fixed to be screened. Test changes that make them less brittle to message text preferred.

Answer 6

Reference: https://clojure.atlassian.net/browse/CLJ-1099 (reported by stu)