Clojure

Exceptions thrown in the top level ns form are reported without file or line number

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Ok

Description

If there is an error in the `ns` form, an exception is thrown, which is not caught in `load`.

For example, with an invalid :only clause;

(ns clj14.myns
  (:use
   [clojure.core :only seq]))

With the latest Clojure master as of Aug 24 2013, this generates the following exception, with no source file or line number except the one shown in clojure.core:

Exception :only/:refer value must be a sequential collection of symbols  clojure.core/refer (core.clj:3854)

You can find a source file in your project if you painstakingly search through the stack trace, but it would be nice if it jumped out at you in the exception itself.

Patch: clj-939-v4.diff

Approach: The latest patch does not modify the behavior of any other exceptions thrown by the compiler. The throw-if function is only used in the load-related functions: this patch changes it to throw CompilerException instead of Exception. As a result, exceptions which occur while loading files will be decorated with file names and line/column numbers. The line/column numbers are not always accurate (see notes in attachment screen-clj-939.org) but the file names are correct.

This is an incremental improvement to compile-time error messages, but it does not solve some of the fundamental problems with reporting correct line/column numbers from errors thrown by the compiler.

Screened by: Stuart Sierra (re-screened by Alex for the specific comments by Rich)

Activity

Hide
Hugo Duncan added a comment -

Corrected patch

Show
Hugo Duncan added a comment - Corrected patch
Hide
Andy Fingerhut added a comment -

Patch 0001-report-load-exception-with-file-and-line.diff fails build. Patch 0002-report-load-exception-with-file-and-line.diff applies, builds, and tests cleanly as of March 9, 2012. Hugo has signed a CA.

Show
Andy Fingerhut added a comment - Patch 0001-report-load-exception-with-file-and-line.diff fails build. Patch 0002-report-load-exception-with-file-and-line.diff applies, builds, and tests cleanly as of March 9, 2012. Hugo has signed a CA.
Hide
Andy Fingerhut added a comment -

clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt dated Oct 5 2012 is intended to be an update to Hugo Duncan's patch 0002-report-load-exceptions-with-file-and-line.diff dated Feb 25 2012. Because of Brandon Bloom's recently commited patch adding column numbers in addition to line numbers, this is not simply updating some lines of context, but I think it is correct. It would be good if Hugo could take a look at it and confirm.

Show
Andy Fingerhut added a comment - clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt dated Oct 5 2012 is intended to be an update to Hugo Duncan's patch 0002-report-load-exceptions-with-file-and-line.diff dated Feb 25 2012. Because of Brandon Bloom's recently commited patch adding column numbers in addition to line numbers, this is not simply updating some lines of context, but I think it is correct. It would be good if Hugo could take a look at it and confirm.
Hide
Stuart Sierra added a comment -

Screened.

The error messages are better than what we had before. The line/column numbers are not particularly informative, probably because ns is a macro.

Show
Stuart Sierra added a comment - Screened. The error messages are better than what we had before. The line/column numbers are not particularly informative, probably because ns is a macro.
Hide
Rich Hickey added a comment -

This patch doesn't change the reporting on any other (e.g. nested) exceptions? It looks like it might.

Show
Rich Hickey added a comment - This patch doesn't change the reporting on any other (e.g. nested) exceptions? It looks like it might.
Hide
Alex Miller added a comment -

This issue has somewhat strange history. Starting over with Triaged so it can go through release assignment and patch process again.

Show
Alex Miller added a comment - This issue has somewhat strange history. Starting over with Triaged so it can go through release assignment and patch process again.
Hide
Andy Fingerhut added a comment -

Patch clj-939-report-load-exceptions-with-file-and-line-patch-v3.txt dated Aug 25 2013 started based upon Hugo Duncan's
clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt patch.

It was enhanced to avoid wrapping a CompilerException within another CompilerException first. That always gave line 1 and column 1 for all load/require/use exceptions, so I modified clojure.core/throw-if to throw a CompilerException instead. The line/column numbers with this approach are now usually those of the beginning of the enclosing ns form, if these operations are caused by an ns form.

I also added a little extra info to one exception message, and a new condition in which an exception is thrown if a require/use operand is neither a libspec nor a prefix list.

Show
Andy Fingerhut added a comment - Patch clj-939-report-load-exceptions-with-file-and-line-patch-v3.txt dated Aug 25 2013 started based upon Hugo Duncan's clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt patch. It was enhanced to avoid wrapping a CompilerException within another CompilerException first. That always gave line 1 and column 1 for all load/require/use exceptions, so I modified clojure.core/throw-if to throw a CompilerException instead. The line/column numbers with this approach are now usually those of the beginning of the enclosing ns form, if these operations are caused by an ns form. I also added a little extra info to one exception message, and a new condition in which an exception is thrown if a require/use operand is neither a libspec nor a prefix list.
Hide
Stuart Sierra added a comment -

Screening in progress.

Show
Stuart Sierra added a comment - Screening in progress.
Hide
Stuart Sierra added a comment -

Screened. Notes in attachment screen-clj-939.org

Show
Stuart Sierra added a comment - Screened. Notes in attachment screen-clj-939.org
Hide
Rich Hickey added a comment -

this (surreptitiously? adds a new test for vector or list. Please submit patches that address one specific issue. There's no reason for this concrete check, but if you think there is make another ticket.

Show
Rich Hickey added a comment - this (surreptitiously? adds a new test for vector or list. Please submit patches that address one specific issue. There's no reason for this concrete check, but if you think there is make another ticket.
Hide
Andy Fingerhut added a comment -

To: Operative Sierra
From: Chaos

Agent Hickey thwarted world domination plans yet again. Recommend plan omicron. InfoSec dept provided clj-939-v4.diff - should enable our insidious changes to slip past his Detect-O-Tron. Good luck, operative. Message ends.

Show
Andy Fingerhut added a comment - To: Operative Sierra From: Chaos Agent Hickey thwarted world domination plans yet again. Recommend plan omicron. InfoSec dept provided clj-939-v4.diff - should enable our insidious changes to slip past his Detect-O-Tron. Good luck, operative. Message ends.
Hide
Alex Miller added a comment -

Seems like the (declare list?) can go away too?

Show
Alex Miller added a comment - Seems like the (declare list?) can go away too?
Hide
Andy Fingerhut added a comment -

Good catch, Alex. More coffee required here. Uploaded new clj-939-v4.diff in place of the previous one.

Show
Andy Fingerhut added a comment - Good catch, Alex. More coffee required here. Uploaded new clj-939-v4.diff in place of the previous one.
Hide
Alex Miller added a comment -

Verified questioned bits have been removed and patch still applies cleanly and tests pass. Re-marking Screened with new patch.

Show
Alex Miller added a comment - Verified questioned bits have been removed and patch still applies cleanly and tests pass. Re-marking Screened with new patch.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: