Improved compiler error messages, part 2

[Allen Rohner]

After feedback on my previous compiler error message patch,
I've started looking into the problem more. My goal is to have the
file and line number printed on every user-visible stack trace.

An example of my desired output is:

java.lang.IllegalArgumentException: /Users/arohner/Programming/clojure/
broken-arity.clj Line 6: Wrong number of args passed to foo
at clojure.lang.AFn.throwArity(AFn.java:461)
at clojure.lang.AFn.invoke(AFn.java:68)
at user.eval__2291.invoke(broken-arity.clj:6)
at clojure.lang.Compiler.eval(Compiler.java:3891)
at clojure.lang.Compiler.load(Compiler.java:4192)
at clojure.lang.Compiler.loadFile(Compiler.java:4159)
at clojure.lang.Repl.main(Repl.java:48)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:
39)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:
25)
at java.lang.reflect.Method.invoke(Method.java:585)
at jline.ConsoleRunner.main(ConsoleRunner.java:69)

Right now, there are 308 places where new exceptions are thrown in the
clojure source. Rather than modify the message of each exception, I am
thinking that it would be a good idea to make a hierarchy of clojure
exceptions, like Clojure.SyntaxError, Clojure.RuntimeError,
etc. The Clojure exception's getMessage() would be extended to contain
Compiler.SOURCE_PATH and Compiler.LINE_AFTER.

Then, replace all of the exceptions in the source that use 'standard'
java exceptions like java.lang.ArrayIndexOutOfBounds and
java.lang.IllegalArgument with one of the exceptions from the Clojure
hierarchy.

Thoughts?

Allen

p.s. last night I was writing Clojure code that exercises the
different kinds of Compiler errors that can be thrown. It wouldn't
take too much effor to turn those into a test suite. Rich, is this
something you would be interested in having?

p.p.s My CA is in the mail

[Rich Hickey]

It seems to me you need to distinguish runtime errors from compilation
errors. For runtime errors, the file and line numbers are already in
the stack trace, as Clojure emits that information in the bytecode.
For example, in the above trace:

> at user.eval__2291.invoke(broken-arity.clj:6)

I'm not going to make any changes for people unwilling to look a few
lines down in a stack trace.

A new exception class hierarchy that duplicates these values seems
redundant. The world does not need more exception types, or a
reinvention of stack traces, IMO.

There are cases where macros mess up the line numbers, and I'll look
into reports of that.

It seems most people are having problems either:

With a runtime error in code entered in the repl (or sent from an
editor buffer), rather than loaded, where there are no relevant source
line numbers to use.

With a compile-time error where the source code is unacceptable to
the compiler or a macro.

It is this latter case where I see the most room for improvement,
because the stack there is the stack of the compiler execution, not
user code.

> p.s. last night I was writing Clojure code that exercises the
> different kinds of Compiler errors that can be thrown. It wouldn't
> take too much effor to turn those into a test suite. Rich, is this
> something you would be interested in having?
>

Sure. That is absolutely first priority in this case. I'm not going to
fix or accept a fix for any problem I haven't seen isolated, and agree
is, in fact, a problem.

> p.p.s My CA is in the mail

Great!

Rich

[Allen Rohner]

> It seems to me you need to distinguish runtime errors from compilation
> errors. For runtime errors, the file and line numbers are already in
> the stack trace, as Clojure emits that information in the bytecode.
> For example, in the above trace:
>
> >         at user.eval__2291.invoke(broken-arity.clj:6)
>
> I'm not going to make any changes for people unwilling to look a few
> lines down in a stack trace.

I don't think that unwillingness is the only criteria here. One reason
is for consistency. We're already heading down the track of making
compiler error messages easier to read. It doesn't seem very friendly
to make the compiler error messages easier and then intentionally
leave the runtime errors obfuscated. Another reason is ease of
understanding. I can read the stack trace just fine, but I will
happily write the patch just because it makes my edit-compile-test
cycle easier and faster. It's much easier to read the problem at the
top than to filter through the stack trace.

>
> A new exception class hierarchy that duplicates these values seems
> redundant. The world does not need more exception types, or a
> reinvention of stack traces, IMO.

I've seen cases where the default Java exception.printStackTrace()
doesn't include the entire stack and the part that I care about is
hidden in that "and 60 more lines" message. I think there it's
entirely appropriate to write our own printStackTrace() that does
display everything. I'll post to the group the next time I see one of
those.

My goal in creating a new exception type was to avoid copy&paste code.
At the point where you want to throw a new exception, I see a few
possible choices here.

1) throw new java.lang.IllegalArgumentException("Too many arguments to
def");
2) throw new
java.lang.IllegalArgumentException(Compiler.SOURCE_PATH.get() + " line
" + Compiler.LINE_AFTER.get() + "Too many arguments to def");
3) throw new
java.lang.IllegalArgumentException(Compiler.exceptionPrefix() + "Too
many arguments to def");
4) throw new Clojure.CompileError("too many arguments to def");

where Compiler.exceptionPrefix() is

String exceptionPrefix()
{
return Compiler.SOURCE_PATH.get() + " line " +
Compiler.LINE_AFTER.get();
}

1 is the current code. 2,3,4 have the same behavior, with increasing
levels of encapsulation. I assume you are in favor of 3) then?

Allen

[Raoul Duke]

On Wed, Sep 17, 2008 at 11:55 AM, Allen Rohner <aro...@gmail.com> wrote:
> I don't think that unwillingness is the only criteria here. One reason

yes! +N for usability through extreme clarity. signal-to-noise is important.

sincerely.

[Rich Hickey]

On Sep 17, 2:55 pm, Allen Rohner <aroh...@gmail.com> wrote:
> > It seems to me you need to distinguish runtime errors from compilation
> > errors. For runtime errors, the file and line numbers are already in
> > the stack trace, as Clojure emits that information in the bytecode.
> > For example, in the above trace:
>
> > > at user.eval__2291.invoke(broken-arity.clj:6)
>
> > I'm not going to make any changes for people unwilling to look a few
> > lines down in a stack trace.
>
> I don't think that unwillingness is the only criteria here. One reason
> is for consistency. We're already heading down the track of making
> compiler error messages easier to read. It doesn't seem very friendly
> to make the compiler error messages easier and then intentionally
> leave the runtime errors obfuscated.

Ok, let's tone down the rhetoric please. The runtime errors are not
obfuscated. In the specific case you gave, an arity error, a _runtime_
error will always look like this:

java.lang.IllegalArgumentException: Wrong number of args passed to:
foo
at clojure.lang.AFn.throwArity(AFn.java:460)
at clojure.lang.AFn.invoke(AFn.java:71)
at somens.somefn__2291.invoke(your-file.clj:6)
...

It reports the function being called, and 3 lines down, the point of
call. The point of call is not first because fns are implemented on
top of abstract classes, and the abstract class uses a helper to
report the error. The solutions you are proposing for compile-time
errors cannot work here, as sourcepath and line are not bound to
anything useful at runtime.

>
> > A new exception class hierarchy that duplicates these values seems
> > redundant. The world does not need more exception types, or a
> > reinvention of stack traces, IMO.
>
> I've seen cases where the default Java exception.printStackTrace()
> doesn't include the entire stack and the part that I care about is
> hidden in that "and 60 more lines" message.

Again, more rhetoric. Let's focus on concrete cases. Currently the
repl recursively seeks the cause and puts it first.

> My goal in creating a new exception type was to avoid copy&paste code.
> At the point where you want to throw a new exception, I see a few
> possible choices here.
>
> 1) throw new java.lang.IllegalArgumentException("Too many arguments to
> def");
> 2) throw new
> java.lang.IllegalArgumentException(Compiler.SOURCE_PATH.get() + " line
> " + Compiler.LINE_AFTER.get() + "Too many arguments to def");
> 3) throw new
> java.lang.IllegalArgumentException(Compiler.exceptionPrefix() + "Too
> many arguments to def");
> 4) throw new Clojure.CompileError("too many arguments to def");
>
> where Compiler.exceptionPrefix() is
>
> String exceptionPrefix()
> {
> return Compiler.SOURCE_PATH.get() + " line " +
> Compiler.LINE_AFTER.get();
>
> }
>
> 1 is the current code. 2,3,4 have the same behavior, with increasing
> levels of encapsulation. I assume you are in favor of 3) then?
>

Compiler.analyzeSeq already wraps the cause with an exception
containing the file and line. I'll need to see examples of where
that's not working.

Let's talk about concrete examples of the problem before we go on
further about the solution. And no more rhetoric please.

Thanks,

Rich

[Allen Rohner]

Ok, concrete.

Here's one mistake I made the other day. I created a ref, and then
forgot to access it using @. The example code is

(def my_map (ref {:a 1, :b 2}))
(def map_vals (vals my_map))

$ java -cp clojure-clean.jar clojure.lang.Repl bad-ref.clj
java.lang.IllegalArgumentException: Don't know how to create ISeq
from: Ref
at clojure.lang.RT.seqFrom(RT.java:465)
at clojure.lang.RT.seq(RT.java:448)
at clojure.lang.RT.vals(RT.java:473)
at clojure.vals__470.invoke(boot.clj:893)
at clojure.lang.AFn.applyToHelper(AFn.java:184)
at clojure.lang.AFn.applyTo(AFn.java:175)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:2589)
at clojure.lang.Compiler$DefExpr.eval(Compiler.java:282)
at clojure.lang.Compiler.eval(Compiler.java:3896)

at clojure.lang.Compiler.load(Compiler.java:4192)
at clojure.lang.Compiler.loadFile(Compiler.java:4159)
at clojure.lang.Repl.main(Repl.java:48)

And then here's the stack trace from the real code:
java.lang.IllegalArgumentException: Don't know how to create ISeq
from: Ref
at clojure.lang.RT.seqFrom(RT.java:465)
at clojure.lang.RT.seq(RT.java:448)
at clojure.lang.RT.vals(RT.java:473)
at clojure.vals__470.invoke(boot.clj:893)
at clojure.lang.AFn.applyToHelper(AFn.java:184)
at clojure.lang.AFn.applyTo(AFn.java:175)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:2589)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:2588)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:2588)
at clojure.lang.Compiler$DefExpr.eval(Compiler.java:282)
at clojure.lang.Compiler.eval(Compiler.java:3896)
at clojure.lang.Compiler.load(Compiler.java:4196)
at clojure.lang.RT.loadResourceScript(RT.java:360)
at clojure.lang.RT.loadResourceScript(RT.java:347)
at clojure.lang.RT.loadResourceScript(RT.java:339)
at clojure.load__1722$fn__1724.invoke(boot.clj:3195)
at clojure.load__1722.doInvoke(boot.clj:3194)
at clojure.lang.RestFn.invoke(RestFn.java:413)
at clojure.load_one__1685.invoke(boot.clj:3041)
at clojure.load_lib__1705.doInvoke(boot.clj:3078)
at clojure.lang.RestFn.applyTo(RestFn.java:147)
at clojure.apply__135.doInvoke(boot.clj:364)
at clojure.lang.RestFn.invoke(RestFn.java:443)
at clojure.load_libs__1709.doInvoke(boot.clj:3104)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.apply__135.doInvoke(boot.clj:364)
at clojure.lang.RestFn.invoke(RestFn.java:443)
at clojure.require__1713.doInvoke(boot.clj:3163)
at clojure.lang.RestFn.invoke(RestFn.java:413)
at user.eval__3155.invoke(start.clj:5)
at clojure.lang.Compiler.eval(Compiler.java:3891)
at clojure.lang.Compiler.load(Compiler.java:4196)
at clojure.lang.Compiler.loadFile(Compiler.java:4163)

at clojure.lang.Repl.main(Repl.java:48)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:
39)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:
25)
at java.lang.reflect.Method.invoke(Method.java:585)
at jline.ConsoleRunner.main(ConsoleRunner.java:69)

Clojure
user=>

The real code is complicated by the fact that I required several
files. If you pay attention to the stack trace, you see that the code
entered at start.clj. Then three files were required, but you can't
tell which file or line blew up.

[Rich Hickey]

On Sep 17, 9:15 pm, Allen Rohner <aroh...@gmail.com> wrote:
> Ok, concrete.
>
> Here's one mistake I made the other day. I created a ref, and then
> forgot to access it using @. The example code is
>
> (def my_map (ref {:a 1, :b 2}))
> (def map_vals (vals my_map))
>
> $ java -cp clojure-clean.jar clojure.lang.Repl bad-ref.clj
> java.lang.IllegalArgumentException: Don't know how to create ISeq
> from: Ref
> at clojure.lang.RT.seqFrom(RT.java:465)
> at clojure.lang.RT.seq(RT.java:448)
> at clojure.lang.RT.vals(RT.java:473)
> at clojure.vals__470.invoke(boot.clj:893)
> at clojure.lang.AFn.applyToHelper(AFn.java:184)
> at clojure.lang.AFn.applyTo(AFn.java:175)
> at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:2589)
> at clojure.lang.Compiler$DefExpr.eval(Compiler.java:282)
> at clojure.lang.Compiler.eval(Compiler.java:3896)
> at clojure.lang.Compiler.load(Compiler.java:4192)

> The real code is complicated by the fact that I required several
> files. If you pay attention to the stack trace, you see that the code
> entered at start.clj. Then three files were required, but you can't
> tell which file or line blew up.

I've made some enhancements to the location info (rev 1031), see if
that helps.

Rich

[Allen Rohner]

> I've made some enhancements to the location info (rev 1031), see if
> that helps.
>
> Rich

This looks good. Thanks!

[Allen Rohner]

Ok, I have a new one.

Reader errors don't tell you the name of the file that had problems:

eof.clj:

(defn foo [a]
(println "hello")

$ java -cp clojure.jar clojure.lang.Repl eof.clj
java.lang.Exception: ReaderError:(3,1) EOF while reading
at clojure.lang.LispReader.read(LispReader.java:164)
at clojure.lang.Compiler.load(Compiler.java:4253)
at clojure.lang.Compiler.loadFile(Compiler.java:4224)
at clojure.lang.Repl.main(Repl.java:48)
Caused by: java.lang.Exception: EOF while reading
at clojure.lang.LispReader.readDelimitedList(LispReader.java:847)
at clojure.lang.LispReader$ListReader.invoke(LispReader.java:784)
at clojure.lang.LispReader.read(LispReader.java:130)
... 3 more
Clojure
user=>

[Allen Rohner]

Here's my attempt at a patch to fix the above problem

I made the existing LispReader.read() private. I made a new public
LispReader.read() that takes an extra argument, a sourcePath. The new
method pushes the sourcePath into a var and then calls the private
method. Then I modifed all existing calls to LispReader.read() to use
the new method.

http://clojure.googlegroups.com/web/reader-error.diff

Feedback?

Allen

[Rich Hickey]

I've enhanced handling of reader errors during load (rev 1033), see if
that helps.

Rich

[Allen Rohner]

>
> I've enhanced handling of reader errors during load (rev 1033), see if
> that helps.
>
> Rich

Looks good. Thanks!

Allen