Clojure

Incorrect error locations reported in the stacktrace

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.6
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

The following code produces an incorrect stacktrace:

(ns clojure-demo.core)

(defn foo
  "I don't do a whole lot."
  [x]
  (println x "Hello, World!"))

(/ 1 0)
Exception in thread "main" java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31)

The problem is actually on the 8th line. As a matter of fact - there's nothing at location 6:31.
This is a pretty serious problem as many tools parse stacktraces for error locations.
Here's a related discussion in cider's issue tracker.

Patch: clj-1568.patch
Screened by: Alex Miller

Activity

Hide
Alex Miller added a comment -

Maybe a dupe of CLJ-1561 ?

Show
Alex Miller added a comment - Maybe a dupe of CLJ-1561 ?
Hide
Andy Fingerhut added a comment -

I tried out the example given in the description, with the latest Clojure master as of today plus the patch for CLJ-1561 called 0002-Mark-line-number-after-emitting-children.patch, dated Oct 10 2014.

The line:column number 6:31 is the same for that patched version as it is in the ticket description, which is for Clojure 1.6.0.

The issue of misleading line:column numbers is common between the two tickets, but at least the proposed improvement in CLJ-1561's patch is not effective for improving this issue.

Show
Andy Fingerhut added a comment - I tried out the example given in the description, with the latest Clojure master as of today plus the patch for CLJ-1561 called 0002-Mark-line-number-after-emitting-children.patch, dated Oct 10 2014. The line:column number 6:31 is the same for that patched version as it is in the ticket description, which is for Clojure 1.6.0. The issue of misleading line:column numbers is common between the two tickets, but at least the proposed improvement in CLJ-1561's patch is not effective for improving this issue.
Hide
Bozhidar Batsov added a comment -

I know that the issue list for 1.7 is pretty much finalised, but I think that this issue and and CLJ-1561 should be fixed as soon as possible.
Correct error reporting is extremely important IMO.

Show
Bozhidar Batsov added a comment - I know that the issue list for 1.7 is pretty much finalised, but I think that this issue and and CLJ-1561 should be fixed as soon as possible. Correct error reporting is extremely important IMO.
Hide
Nicola Mometto added a comment -

Attached a patch that fixes the issue by consuming all the whitespaces before retrieving line/column info for the next form.

Show
Nicola Mometto added a comment - Attached a patch that fixes the issue by consuming all the whitespaces before retrieving line/column info for the next form.
Hide
Alex Miller added a comment -

Are there possible downsides to more eagerly consuming whitespace as done in the patch?

Show
Alex Miller added a comment - Are there possible downsides to more eagerly consuming whitespace as done in the patch?
Hide
Nicola Mometto added a comment -

I can't think of any

Show
Nicola Mometto added a comment - I can't think of any
Hide
Paul Stadig added a comment -

The defect on master does not have effect when using compile:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

With the patch applied all the line numbers are the same in all cases:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

Agreed that this seems to be orthogonal to CLJ-1561.

Show
Paul Stadig added a comment - The defect on master does not have effect when using compile:
user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 
With the patch applied all the line numbers are the same in all cases:
user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 
Agreed that this seems to be orthogonal to CLJ-1561.
Hide
Bozhidar Batsov added a comment - - edited

Seems we need to add tests before this can be merged - https://groups.google.com/forum/#!topic/clojure-dev/7pFhG8LMvGo

Show
Bozhidar Batsov added a comment - - edited Seems we need to add tests before this can be merged - https://groups.google.com/forum/#!topic/clojure-dev/7pFhG8LMvGo
Hide
Daniel Solano Gómez added a comment -

Well, I've been looking into adding tests to this patch, and I've made some interesting discoveries. The additions to Compiler.load() seem to work just fine. However, I'm not seeing much coming out of the changes to Compiler.compile. You can see in Paul's comment above that the compile call actually reports the correct location. I created a test that throws a NullPointerException during compilation, and in the case of compile, it is never wrapped in a CompilerException. I'll attach my test patch that contains the example code I have been working with.

Show
Daniel Solano Gómez added a comment - Well, I've been looking into adding tests to this patch, and I've made some interesting discoveries. The additions to Compiler.load() seem to work just fine. However, I'm not seeing much coming out of the changes to Compiler.compile. You can see in Paul's comment above that the compile call actually reports the correct location. I created a test that throws a NullPointerException during compilation, and in the case of compile, it is never wrapped in a CompilerException. I'll attach my test patch that contains the example code I have been working with.
Hide
Daniel Solano Gómez added a comment -

Patch with tests for code paths in the fix. The tests for the Compiler.compile changes are not showing what I had expected.

Show
Daniel Solano Gómez added a comment - Patch with tests for code paths in the fix. The tests for the Compiler.compile changes are not showing what I had expected.
Hide
Alex Miller added a comment -

Whenever people feel this is ready for screening, just switch the Approval from Incomplete to Vetted.

Show
Alex Miller added a comment - Whenever people feel this is ready for screening, just switch the Approval from Incomplete to Vetted.
Hide
Nicola Mometto added a comment -

Updated patch removing the changes to Compiler.compile as they seem to be useless, by Daniel's tests

Show
Nicola Mometto added a comment - Updated patch removing the changes to Compiler.compile as they seem to be useless, by Daniel's tests
Hide
Daniel Solano Gómez added a comment -

I have cleaned up my test code a bit and put together a combined patch that includes both the fix and the tests.

Show
Daniel Solano Gómez added a comment - I have cleaned up my test code a bit and put together a combined patch that includes both the fix and the tests.
Hide
Bozhidar Batsov added a comment -

Great! Looking forward to seeing this merged.

Show
Bozhidar Batsov added a comment - Great! Looking forward to seeing this merged.
Hide
Bozhidar Batsov added a comment -

Seems we can finally merge this!

Show
Bozhidar Batsov added a comment - Seems we can finally merge this!

People

Vote (19)
Watch (7)

Dates

  • Created:
    Updated:
    Resolved: