Clojure

Undefined symbols raise exceptions with line/column number of enclosing expression

Details

  • Type: Defect Defect
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Screened

Description

Certain kinds of errors in loaded source files are coming back tagged with the correct source file, but an incorrect line:column number. This seems to happen when unknown symbols occur by themselves, not called as a function.

The general pattern appears to be that an undefined symbol is reported with a line number of the beginning of its nearest enclosing expression. If the undefined symbol appears at the top level of a file, it is reported with line:column number 0:0, or line:column number of REPL input, if loaded from a REPL. The behavior is different in a Leiningen REPL. If the undefined symbol appears at the top level of a file, it is reported with line:column number 1:1.

$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1)

Patch: CLJ-420-3.patch

Approach: Capture line and column metadata for symbols in the LispReader. A few tests were adjusted to ignore line and col metadata for protocol symbols which now have them.

Screened by: Alex Miller

Background: Clojure Google group thread when this issue was originally reported in 2010 against Clojure 1.2: http://groups.google.com/group/clojure/browse_frm/thread/beb36e7228eabd69/a7ef16dcc45834bc?hl=en#a7ef16dcc45834bc

  1. CLJ-420.patch
    19/Oct/13 9:31 PM
    3 kB
    Paavo Parkkinen
  2. CLJ-420-2.patch
    20/Oct/13 5:05 PM
    3 kB
    Alex Miller
  3. CLJ-420-3.patch
    21/Oct/13 8:29 AM
    3 kB
    Alex Miller
  4. CLJ-420-4.patch
    28/Oct/13 7:44 AM
    3 kB
    Paavo Parkkinen

Activity

Hide
Assembla Importer added a comment -
Show
Assembla Importer added a comment - Converted from http://www.assembla.com/spaces/clojure/tickets/420
Hide
Assembla Importer added a comment -

stu said: Updating tickets (#427, #426, #421, #420, #397)

Show
Assembla Importer added a comment - stu said: Updating tickets (#427, #426, #421, #420, #397)
Hide
Assembla Importer added a comment -

stu said: Updating tickets (#429, #437, #397, #420)

Show
Assembla Importer added a comment - stu said: Updating tickets (#429, #437, #397, #420)
Hide
Alex Miller added a comment -

Based on the updated information (which is really a totally different issue), I have reduced priority from Major to Minor, removed the fix version and sent this back down to Triaged for Rich to take another look.

Show
Alex Miller added a comment - Based on the updated information (which is really a totally different issue), I have reduced priority from Major to Minor, removed the fix version and sent this back down to Triaged for Rich to take another look.
Hide
Paavo Parkkinen added a comment -

Just noticed that when I reproduce this with current code from Github, I get the same behaviour that was in the original report.

$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp target/clojure-1.6.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)    
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 
Show
Paavo Parkkinen added a comment - Just noticed that when I reproduce this with current code from Github, I get the same behaviour that was in the original report.
$ cat test1.clj 

bla
$ cat test2.clj 

(bla)
$ java -cp target/clojure-1.6.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)    
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 
Hide
Paavo Parkkinen added a comment -

I also get the original behaviour with 1.5.1.

$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 

Could be lein is mixing it up. I also get the behaviour in the description if I do it with lein.

Show
Paavo Parkkinen added a comment - I also get the original behaviour with 1.5.1.
$ java -cp ../../opt/clojure/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:1:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test2)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test2.clj:2:1) 
user=> (require 'test1)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: bla in this context, compiling:(test1.clj:5:1) 
user=> 
Could be lein is mixing it up. I also get the behaviour in the description if I do it with lein.
Hide
Andy Fingerhut added a comment -

Paavo, I think you are correct that I was getting different behavior than the original description because I was using Leiningen, rather than straight Clojure, and that the original description's behavior is still true with the latest Clojure master when Leiningen is not used. My bad for changing the description unnecessarily. Would you be willing to correct it?

Show
Andy Fingerhut added a comment - Paavo, I think you are correct that I was getting different behavior than the original description because I was using Leiningen, rather than straight Clojure, and that the original description's behavior is still true with the latest Clojure master when Leiningen is not used. My bad for changing the description unnecessarily. Would you be willing to correct it?
Hide
Paavo Parkkinen added a comment -

Corrected description.

Show
Paavo Parkkinen added a comment - Corrected description.
Hide
Paavo Parkkinen added a comment -

I have a fix for This bug. I will attach it to the ticket, but it is not supposed to be considered for approval, as it is clearly not finished (i.e. cleaned up) yet. If there is a better way to solicit comments for a proposed fix, please let me know.

For the fix, I simply copied the line:column number tracking code from ListReader.invoke (also used in others) into LispReader.read for Symbols. Only issues I see is that some test cases get confused because some meta data contains line:column info where it didn't use to.

I also tried to fix the line:column numbering for files like test3 and test4 below, and the fix is in the patch file, but commented out, because the fix for that causes some compile problems. I have not yet tracked down what the cause of the problems is. I suppose the correct procedure is to leave it out for now, and possibly open a new ticket for it?

$ cat test3.clj 

(
bla)
$ cat test4.clj 

(print
bla)

If anyone has any comments or suggestions, I would be very happy to hear them. In the mean time I'll start checking out the failing test cases, and seeing if I can correct them to work with the fix. I'm not sure how I should go about testing the fix itself.

Also, if the comments here is not the correct place for this discussion, please let me know and I will move it elsewhere (clojure-dev).

P.s. I suppose for test2, the column number should be 2, not 1. Not sure if that's a big deal or not.

Show
Paavo Parkkinen added a comment - I have a fix for This bug. I will attach it to the ticket, but it is not supposed to be considered for approval, as it is clearly not finished (i.e. cleaned up) yet. If there is a better way to solicit comments for a proposed fix, please let me know. For the fix, I simply copied the line:column number tracking code from ListReader.invoke (also used in others) into LispReader.read for Symbols. Only issues I see is that some test cases get confused because some meta data contains line:column info where it didn't use to. I also tried to fix the line:column numbering for files like test3 and test4 below, and the fix is in the patch file, but commented out, because the fix for that causes some compile problems. I have not yet tracked down what the cause of the problems is. I suppose the correct procedure is to leave it out for now, and possibly open a new ticket for it?
$ cat test3.clj 

(
bla)
$ cat test4.clj 

(print
bla)
If anyone has any comments or suggestions, I would be very happy to hear them. In the mean time I'll start checking out the failing test cases, and seeing if I can correct them to work with the fix. I'm not sure how I should go about testing the fix itself. Also, if the comments here is not the correct place for this discussion, please let me know and I will move it elsewhere (clojure-dev). P.s. I suppose for test2, the column number should be 2, not 1. Not sure if that's a big deal or not.
Hide
Alex Miller added a comment -

A few things here:

1) I think you should drop the commented Compiler changes and just focus on the reader changes.
2) This patch has a number of test failures. In particular, protocol symbols now have line and column metadata. That's potentially quite useful but I haven't fully contemplated the ramifications.

Moving to Incomplete for now.

Show
Alex Miller added a comment - A few things here: 1) I think you should drop the commented Compiler changes and just focus on the reader changes. 2) This patch has a number of test failures. In particular, protocol symbols now have line and column metadata. That's potentially quite useful but I haven't fully contemplated the ramifications. Moving to Incomplete for now.
Hide
Paavo Parkkinen added a comment -

I attached a new version of patch, which gets rid of the Compiler changes, and fixes the test cases by simply ignoring the additional line/column metadata. I'm not aware of any way to reliably know what the values are going to be so they could be checked.

Or course there might be code that relies on the metadata not including line/column info, and that code will break with the patch. I can't really think of any reason why anyone would do that, and can't estimate how many people might be affected. After the patch people will probably start using the new metadata, and after that it will be difficult to get rid of, if needed in the future.

There is, I think, also potential performance effects. I believe the metadata is generated at compile time, but will be available at runtime, which will mean at least a minor increase in footprint, and possibly CPU as well. Again, I'm not that familiar with how the runtime works that I could really estimate that.

Of course there may also be other effects that I can't imagine right now.

Show
Paavo Parkkinen added a comment - I attached a new version of patch, which gets rid of the Compiler changes, and fixes the test cases by simply ignoring the additional line/column metadata. I'm not aware of any way to reliably know what the values are going to be so they could be checked. Or course there might be code that relies on the metadata not including line/column info, and that code will break with the patch. I can't really think of any reason why anyone would do that, and can't estimate how many people might be affected. After the patch people will probably start using the new metadata, and after that it will be difficult to get rid of, if needed in the future. There is, I think, also potential performance effects. I believe the metadata is generated at compile time, but will be available at runtime, which will mean at least a minor increase in footprint, and possibly CPU as well. Again, I'm not that familiar with how the runtime works that I could really estimate that. Of course there may also be other effects that I can't imagine right now.
Hide
Alex Miller added a comment -

I updated with a new patch that fixes some whitespace errors and removes the CLJ-420 comments which don't seem very helpful to me. Marking screened.

Show
Alex Miller added a comment - I updated with a new patch that fixes some whitespace errors and removes the CLJ-420 comments which don't seem very helpful to me. Marking screened.
Hide
Rich Hickey added a comment -

this breaks tests, and will also break a ton of code not expecting this new metadata

Show
Rich Hickey added a comment - this breaks tests, and will also break a ton of code not expecting this new metadata
Hide
Paavo Parkkinen added a comment -

Attached a patch (CLJ-420-4.patch) which fixes this without introducing the new metadata into symbols. Instead of using metadata to pass the line/column info from the reader to the compiler, I just store it in fields in the Symbol class.

Show
Paavo Parkkinen added a comment - Attached a patch (CLJ-420-4.patch) which fixes this without introducing the new metadata into symbols. Instead of using metadata to pass the line/column info from the reader to the compiler, I just store it in fields in the Symbol class.
Hide
Andy Fingerhut added a comment -

Paavo, I am not a Clojure screener, and cannot say any of this with authority to back it up, but Rich chose to close this ticket as declined, meaning that it would take some convincing for him to consider a ticket for it again. I am not saying you can't attach all of the patches you want to this ticket, but they might not go anywhere.

Show
Andy Fingerhut added a comment - Paavo, I am not a Clojure screener, and cannot say any of this with authority to back it up, but Rich chose to close this ticket as declined, meaning that it would take some convincing for him to consider a ticket for it again. I am not saying you can't attach all of the patches you want to this ticket, but they might not go anywhere.
Hide
Alex Miller added a comment -

+1 to Andy's comment.

Further, I don't think the replacement patch respects the immutability and immutability concerns of interned Symbols - setting the line and column is a big race condition in the patch as is. IF this path were going to be acceptable, line and column would need to be final fields set in the Symbol constructor. However, it seems to me that you could easily have two symbols sharing the same interned Symbol that perhaps were created in different locations.

An additional item to consider is the additional memory overhead of carrying two additional ints on every Symbol. In general, it does not seem to me that this path provides enough value per overhead/effort so I would think I would recommend letting it go OR filing a new ticket.

Show
Alex Miller added a comment - +1 to Andy's comment. Further, I don't think the replacement patch respects the immutability and immutability concerns of interned Symbols - setting the line and column is a big race condition in the patch as is. IF this path were going to be acceptable, line and column would need to be final fields set in the Symbol constructor. However, it seems to me that you could easily have two symbols sharing the same interned Symbol that perhaps were created in different locations. An additional item to consider is the additional memory overhead of carrying two additional ints on every Symbol. In general, it does not seem to me that this path provides enough value per overhead/effort so I would think I would recommend letting it go OR filing a new ticket.
Hide
Paavo Parkkinen added a comment -

I didn't realize the ticket was closed. Thanks for pointing that out.

And thanks for taking the time to still take a look at and comment on my patch, Alex. It's good advice.

Show
Paavo Parkkinen added a comment - I didn't realize the ticket was closed. Thanks for pointing that out. And thanks for taking the time to still take a look at and comment on my patch, Alex. It's good advice.

People

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: