Improved RT errors

[Reid McKenzie]

During a twitter conversation last night on the subject of improving
Clojure's error messages, I got to thinking about the friction point(s)
I encounter on a regular basis and what could be done to clean them up.

Clojure takes a "garbage in, garbage out" stance on things most of the
time. This means that garbage values will flow through a program until
they cause a concrete NullPointerException, ClassCastException or
something equally nasty somewhere. This is fine. What is undesirable if
not flatly unfriendly is allowing those exceptions to propagate back to
a user when there is more information in calling context which can be
used to explain what went wrong and help a user fix the issue.

Particularly, I thought it would be an improvement if the RT methods
which are used to implement Clojure's collections support would indicate
to a user in terms of Clojure's abstractions rather than of the Java
implementation what the violated expectation was. By using only the Java
exception machinery such improved errors could have very low overhead
rather than attempting to implement meaningful validation in the normal
execution flow at the cost of code bloat.

These thoughts in mind, I wrote exactly such wrappers for RT which I
think do indeed yield some improvement. I propose that similar try/catch
blocks used elsewhere in core.clj could yield additional improvements
along the same lines, but I thought the concept merited broader
discussion before I sunk a bunch more time into it.

Linked [1] find my patch for RT, I'm curious what you make of it.

Reid

[1]
https://github.com/arrdem/clojure/commit/e1e7c0864586fc7693d340c17e58efc38d0ad373

[Bozhidar Batsov]

I'm guessing this article sparkled the discussion - http://elm-lang.org/blog/compilers-as-assistants

I'd love to see Clojure's team pay more attention to end user experience with respect to errors and your suggestion looks like a simple but solid step in the right direction. 

--
You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clojure-dev...@googlegroups.com.
To post to this group, send email to cloju...@googlegroups.com.
Visit this group at http://groups.google.com/group/clojure-dev.
For more options, visit https://groups.google.com/d/optout.

[Alex Miller]

Can you list example calls and before / after behavior (basically the user experience)?

[Robin Heggelund Hansen]

This update looks clean (and very much wanted) to me.

[Nicola Mometto]

While I'm all for better error messages, I don't see how the ones you propose improve the current state in any way.

You're proposing changing:
ClassCastException java.lang.Long cannot be cast to clojure.lang.Associative clojure.lang.RT.assoc (RT.java:792)
to:
IllegalArgumentException assoc needs an associative, got java.lang.Long clojure.lang.RT.assoc (RT.java:820)

I don't see the additional value. The new exception message actually has less information

[Reid McKenzie]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Alex:

c.c/count, RT.countFrom previously did not report the full name of a
class on which counting is not supported.

c.c/cons, RT.cons previously generated

> IllegalArgumentException Don't know how to create ISeq from:
java.lang.Object clojure.lang.RT.seqFrom (RT.java:528)

now generates

> IllegalArgumentException cons needs a sequable, got
> java.lang.Object

c.c/{first,second,third,fourth,next,more} and corresponding RT methods
previously generated

> IllegalArgumentException Don't know how to create ISeq from:
java.lang.Object

sourced typically from either RT.seq or RT.seqFrom. Now generates

> IllegalArgumentException $METHOD needs a sequable, got
> java.lang.Object

for $METHOD as appropriate.

On c.c/{pop,assoc,dissoc} or rather the corresponding methods there
are actually breaking changes. Previously raw ClassCastExceptions
would be generated, now IllegalArgumentExceptions are generated.
Previously

> ClassCastException java.lang.Object cannot be cast to
clojure.lang.Associative

now generates

> IllegalArgumentException assoc needs an associative, got

java.lang.Object

Similarly c.c/find, RT.find previously

> ClassCastException java.lang.Object cannot be cast to
> java.util.Map

now generates

> IllegalArgumentException find needs a map or associative, got a
java.lang.Object

As hinted below, maybe nested exceptions would be a better way to
model this without loosing information, but the idea is that if we
take this approach and extend it, we can add some more useful bits of
information at every level of the call stack by adding similar
reporting into clojure.core and then maybe we can get somewhere.

Nicola:

Yes that is precisely the point. These changes won't meaningfully help
anyone reading this list now, and nor were they intended to. You or I
can look up in the source if we don't just remember what's going on,
but that's an expectation I don't think we can or should have of
newcomers. Looking up Associative or some other interface they'll find
what is officially an undocumented implementation detail or a bunch of
StackOverflow posts explaining how this cryptic error message relates
to what it is they thought they were doing. The point of this sketch
is not that it makes a huge UX improvement, but that it does
_something_ which could be expanded upon.

Maybe it would be better to throw exceptions with causes rather than
masking nested exceptions as this patch does. Maybe there's more we
can say in error messages. That's why I started this thread instead of
whining and disappearing into background radiation.

In response to your performance concerns on twitter, if you can come
up with something more useful or show how to achieve anything along
these lines more efficiently than by using the existing exception
infrastructure I welcome the lesson. This is the best prototype in
both those dimensions which I could devise for the purpose of starting
a discussion on better errors.

Reid

[Nicola Mometto]

Reid,
I don't think that any of those error messages require any knowledge of the internals of clojure.
Given that functions like `seq?` already reference interfaces like `clojure.lang.ISeq`, I don't see why `seqable` would be more appropriate.

If anything, I would propose that we once and for all document those interfaces that have been effectively public but never made into the official "public api".

With regards to my performance concerns, I raised them mindful of previous instances like http://dev.clojure.org/jira/browse/CLJ-1099 where error messages improvements introduced performance regressions.
As I already said when I asked for performance metrics on the direct-linking changes, I don't think it's fair to ask users/other developers/contributors to provide benchmarks demonstrating that a change they aren't the author of has caused a performance regression.
It should rather be the burden of the author of the improvement itself (or a supporter of it) that should provide a compelling argument for its inclusion and metrics demonstrating either a performance improvement (in the case of direct linking) or demonstrating that no perf regressions happened.

[Bozhidar Batsov]

I strongly feel that better UX is more important than marginal performance gains. The errors people get from the Clojure compiler have often been pointed as a serious issue for newcomers. 

Consider this:

user> (1 2 3 4)

ClassCastException java.lang.Long cannot be cast to clojure.lang.IFn  user/eval7922 (form-init2946881538932206850.clj:2)

For a newcomer something like this is totally absurd. It'd be much better if it read "The first element of a list must be a function." (or something in general vicinity). I'd also add some tip like the Elm compiler tips for newcomers that if someone wanted a list that won't be treated a function/method call they should quote it or whatever. Tips should be something you can turn on/off, the nice errors will always be around. 

A "ClassCastException" means pretty much nothing - except that we're simply assuming people will know what to do. I'd say an IllegalArgumentException carries way more information. Soon or later Clojure should start paying more attention to small things like these, otherwise it risks alienating a ton of potential new users who'd opt for something with "more user-friendly" experience. 

 

[Timothy Baldridge]

The problem I see with all these approaches is that they require modification of Clojure and specifically the compiler. And they still will be confusing for users. Using Bozhidar's example:

(map 42 [1 2 3 4])

"The first element of a list must be a function"

Hrm....

Seems like a more preferable solution would be to hook into the Clojure compiler/runtime somehow in order to get enough diagnostic information to improve messages for users. Something like what was demoed in Colin's Conj talk this year. That way, people (like me) who want the more exact, but obtuse error messages can have them, while those who want something more user friendly are free to install tools that provide those messages for them. 

Cause in the end...both of these runtime errors really are cast exceptions, it's just that the beginner clojure users doesn't understand what these interfaces are. 

Timothy

“One of the main causes of the fall of the Roman Empire was that–lacking zero–they had no way to indicate successful termination of their C programs.”
(Robert Firth)

[Nicola Mometto]

Agreed.

And tools like that already exist, see for example dynalint.

The issue with those tools AFAIK is that they can only work at the Var level, so anything that is inlined can't have their error messages improved, this is something that would be solved by having a more fine-grained control of what optimizations are applied/build profiles (I would love for an option to disable inlining, just as we have an option to disable direct linking, plus a DL+inline and distribution of clojure and one fully dynamic)

Nicola

[Ambrose Bonnaire-Sergeant]

FWIW inlining can be changed via alter-meta! monkeypatching in :inline. It requires re-evaluating the forms though, which might be what you meant.

eg. Dynalint adds a warning on unchecked-inc overflow:

user=> (unchecked-inc Long/MAX_VALUE)

WARNING (Dynalint id 23):  clojure.core/unchecked-inc (inlining) overflow detected : 9223372036854775807 (class java.lang.Long) -> -9223372036854775808 (class java.lang.Long)

-9223372036854775808

Thanks,

Ambrose

[Nicola Mometto]

Yes but that won't affect already defined defns using those functions.

[Ambrose Bonnaire-Sergeant]

Right, I agree a knob for implicit inlining would be useful.

[Ryan Fowler]

Much like Donny, I'm out of my element here. But...

The biggest problem with Clojure Exceptions isn't the type or the message. It's that you can't tell what part of your program is screwed up.

Somewhere in the following stack trace, there should be a `foo`. Maybe that's not how the internals work, but it's how people think

% java -jar ~/.m2/repository/org/clojure/clojure/1.8.0-RC1/clojure-1.8.0-RC1.jar

Clojure 1.8.0-RC1

user=> (defn foo [] (map 1 [1 2]))

#'user/foo

user=> (foo)

ClassCastException java.lang.Long cannot be cast to clojure.lang.IFn  clojure.core/map/fn--4781 (core.clj:2646)

user=> (pst)

ClassCastException java.lang.Long cannot be cast to clojure.lang.IFn

        clojure.core/map/fn--4781 (core.clj:2646)

        clojure.lang.LazySeq.sval (LazySeq.java:40)

        clojure.lang.LazySeq.seq (LazySeq.java:49)

        clojure.lang.RT.seq (RT.java:521)

        clojure.core/seq--4352 (core.clj:137)

        clojure.core/print-sequential (core_print.clj:46)

        clojure.core/fn--6068 (core_print.clj:153)

        clojure.core/fn--6068 (core_print.clj:-1)

        clojure.lang.MultiFn.invoke (MultiFn.java:233)

        clojure.core/pr-on (core.clj:3574)

        clojure.core/pr (core.clj:3577)

        clojure.core/pr (core.clj:-1)

nil

Ryan

[Ghadi Shayban]

Perhaps this approach is misguided.  Instead, can leiningen/nREPL/REPLy provide some stack-rewriting functionality for better traces?  JRuby uses this to great effect.  `throwable->map` `#error` and `#object` are pretty visionary changes Rich added in 1.7.  Those should enable better traces:

"Exception in Lazy Sequence Rest Thunk"

"Mapping function threw an exception when applied"

(This was all possible before throwable->map, just harder)

I think if code in core actively interferes with building better tooling then a bug/enhancement would be merited and welcomed, but perhaps changing Clojure first isn't a minimal approach.

[Reid McKenzie]

While I think the presented approach to error reporting works for dead
simple cases, after playing with it some more it seems to fall apart
badly under composition. Credit to Tim for presenting the map example,

> (map 42 [1 2 3 4])

This is fine, but what if we generate the same exception inside the
mapping function, eg

> (map (fn [x] (42 x)) [1 2 3 4])

Now it's indeterminate whether a ClassCastException raised in the
evaluation of map is the result of mapping something that isn't a
function, or of the same bug nested in the mapping function. This is an
essential problem to any variation of the bottom up approach to error
message generation I was attempting to play, because it forces us to
make all error handling decisions ahead of time to which mistake
chouser's conj talk applies directly.

While my gut reaction to Ghadi's suggestion that what we need is better
tooling is in the negative since the goal seems to be improving the
stock behavior (we already have several different stacktrace tools) I'm
persuaded that whatever its performance characteristics may turn out to
be my particular sketch is not a solution and that something based on
stack analysis will be able to generate much more meaningful results.

Reid

> clojure-dev...@googlegroups.com <javascript:>.

> > > > To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> > > > Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> > > > For more options, visit
> https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

> > >
> > > --
> > > You received this message because you are subscribed to
> the Google Groups "Clojure Dev" group.
> > > To unsubscribe from this group and stop receiving emails
> from it, send an email to clojure-dev...@googlegroups.com

> <javascript:>.

> > > To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> > > Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> > > For more options, visit
> https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

> > >
> > >
> > > --
> > > You received this message because you are subscribed to
> the Google Groups "Clojure Dev" group.
> > > To unsubscribe from this group and stop receiving emails
> from it, send an email to clojure-dev...@googlegroups.com

> <javascript:>.

> > > To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> > > Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> > > For more options, visit
> https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

> > >
> > >
> > >
> > > --
> > > “One of the main causes of the fall of the Roman Empire
> was that–lacking zero–they had no way to indicate successful
> termination of their C programs.”
> > > (Robert Firth)
> > >
> > > --
> > > You received this message because you are subscribed to
> the Google Groups "Clojure Dev" group.
> > > To unsubscribe from this group and stop receiving emails
> from it, send an email to clojure-dev...@googlegroups.com

> <javascript:>.

> > > To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> > > Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> > > For more options, visit
> https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

> >
> > --
> > You received this message because you are subscribed to
> the Google Groups "Clojure Dev" group.
> > To unsubscribe from this group and stop receiving emails
> from it, send an email to clojure-dev...@googlegroups.com

> <javascript:>.

> > To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> > Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> > For more options, visit https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

> >
> >
> > --
> > You received this message because you are subscribed to
> the Google Groups "Clojure Dev" group.
> > To unsubscribe from this group and stop receiving emails
> from it, send an email to clojure-dev...@googlegroups.com

> <javascript:>.

> > To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> > Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> > For more options, visit https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

>
> --
> You received this message because you are subscribed to the
> Google Groups "Clojure Dev" group.
> To unsubscribe from this group and stop receiving emails
> from it, send an email to clojure-dev...@googlegroups.com

> <javascript:>.

> To post to this group, send email to

> cloju...@googlegroups.com <javascript:>.

> Visit this group at
> http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> For more options, visit https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

>
>
> --
> You received this message because you are subscribed to the
> Google Groups "Clojure Dev" group.
> To unsubscribe from this group and stop receiving emails from

> it, send an email to clojure-dev...@googlegroups.com <javascript:>.

> To post to this group, send email to cloju...@googlegroups.com

> <javascript:>.

> Visit this group at http://groups.google.com/group/clojure-dev

> <http://groups.google.com/group/clojure-dev>.

> For more options, visit https://groups.google.com/d/optout

> <https://groups.google.com/d/optout>.

>
>
> --
> You received this message because you are subscribed to the Google
> Groups "Clojure Dev" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to clojure-dev...@googlegroups.com

> <mailto:clojure-dev...@googlegroups.com>.

> To post to this group, send email to cloju...@googlegroups.com

> <mailto:cloju...@googlegroups.com>.