Clojure

In reader, don't ignore explicit :line :column meta

Details

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

Description

Unfortunately, when read in using a LineNumberingPushbackReader, code like this has its :line metadata squashed by the line numbers coming from that. A REPL-friendly example would be:

=> (meta (read (clojure.lang.LineNumberingPushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 1, :column 1}
=> (meta (read (java.io.PushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 66, :column 1}

The latter seems more correct to me (and is equivalent to read-string).

Proposed: Retain existing :line/:column meta on seqs and lists that are read with meta rather than overriding with the values read from the reader.

Additionally, use :file meta as a standard way for the REPL to allow external tool using the repl to specify the source context:

user=> ^{:file "hi.clj" :line 100 :column 10} (def 5)
Syntax error compiling def at (hi.clj:100:10).
First argument to def must be a Symbol

The repl is finding :file meta and binding SOURCE_PATH if it sees it.

Patch: clj-1079-2.patch

  1. CLJ-1079.diff
    07/Oct/12 3:57 PM
    5 kB
    Chas Emerick
  2. clj-1079-2.patch
    12/Oct/18 1:51 PM
    4 kB
    Alex Miller

Activity

Hide
Chas Emerick added a comment -

{{CLJ-1097.diff}} contains a fix for this issue, as well as a separate commit that eliminates a series of casts in order to improve readability in the area.

Show
Chas Emerick added a comment - {{CLJ-1097.diff}} contains a fix for this issue, as well as a separate commit that eliminates a series of casts in order to improve readability in the area.
Hide
Andy Fingerhut added a comment -

Chas, your patch doesn't apply cleanly to latest Clojure master as of Oct 5 2012. I'm not sure, but I think some recent commits to Clojure on Oct 4 2012 caused that. I also take it as evidence of your awesomeness that you can write patches for tickets that haven't been filed yet

Show
Andy Fingerhut added a comment - Chas, your patch doesn't apply cleanly to latest Clojure master as of Oct 5 2012. I'm not sure, but I think some recent commits to Clojure on Oct 4 2012 caused that. I also take it as evidence of your awesomeness that you can write patches for tickets that haven't been filed yet
Hide
Chas Emerick added a comment - - edited

"patches for tickets that haven't been filed yet?"

Anyway, tweaking up this patch is a small price to pay for having column meta. New {{CLJ-1097.diff}} patch attached, applies clean on master as of now. Otherwise same contents as in the original patch, except:

  • the same dynamic is also applied to :column metadata, now that it's available
  • the changes have been rebased into a single commit (including the elimination of the casts in MetaReader, which were becoming so numerous that the code was less readable than most
Show
Chas Emerick added a comment - - edited "patches for tickets that haven't been filed yet?" Anyway, tweaking up this patch is a small price to pay for having column meta. New {{CLJ-1097.diff}} patch attached, applies clean on master as of now. Otherwise same contents as in the original patch, except:
  • the same dynamic is also applied to :column metadata, now that it's available
  • the changes have been rebased into a single commit (including the elimination of the casts in MetaReader, which were becoming so numerous that the code was less readable than most
Hide
Nicola Mometto added a comment -

"patches for tickets that haven't been filed yet?"

He was referring to the fact that you uploaded "CLJ-1097.diff" while the ticket is #1079

Show
Nicola Mometto added a comment - "patches for tickets that haven't been filed yet?" He was referring to the fact that you uploaded "CLJ-1097.diff" while the ticket is #1079
Hide
Chas Emerick added a comment -

Oh, hah! Twice now, even! One more data point recommending my having slight dyslexia or somesuch. :-P

I've replaced the attached patch with one that is named properly to avoid any later confusion.

Show
Chas Emerick added a comment - Oh, hah! Twice now, even! One more data point recommending my having slight dyslexia or somesuch. :-P I've replaced the attached patch with one that is named properly to avoid any later confusion.
Hide
Chas Emerick added a comment -

Refreshed patch to apply cleanly to master after the recent off by one patch for :column metadata.

Show
Chas Emerick added a comment - Refreshed patch to apply cleanly to master after the recent off by one patch for :column metadata.
Hide
Stuart Halloway added a comment -

This feels backwards to me. If a special purpose tool wants to convey information via metadata, why does it use names that collide with those used by LispReader?

Show
Stuart Halloway added a comment - This feels backwards to me. If a special purpose tool wants to convey information via metadata, why does it use names that collide with those used by LispReader?
Hide
Chas Emerick added a comment -

The information being conveyed is the same :line and :column metadata conveyed by LispReader — in fact, that's where it comes from in the first place.

Kibit (and cljx) is essentially an out-of-band source transformation tool. Given an input like this:

(ns com.foo.hosty)

(defn ^:clj system-hash
  [x]
 (System/identityHashCode x))

(defn ^:cljs system-hash
  [x]
  (goog/getUid x))

…it produces two files, a .clj for Clojure, and a .cljs for ClojureScript. (The first code listing in the ticket description is the former.)

However, because there's no way to transform Clojure code/data without losing formatting, anything that depends on line/column numbers (stack traces, stepping debuggers) is significantly degraded. If LispReader were to defer to :line and :column metadata already available on the loaded forms (there when the two generated files are spit out with *print-meta* on), this would not be the case.

Show
Chas Emerick added a comment - The information being conveyed is the same :line and :column metadata conveyed by LispReader — in fact, that's where it comes from in the first place. Kibit (and cljx) is essentially an out-of-band source transformation tool. Given an input like this:
(ns com.foo.hosty)

(defn ^:clj system-hash
  [x]
 (System/identityHashCode x))

(defn ^:cljs system-hash
  [x]
  (goog/getUid x))
…it produces two files, a .clj for Clojure, and a .cljs for ClojureScript. (The first code listing in the ticket description is the former.) However, because there's no way to transform Clojure code/data without losing formatting, anything that depends on line/column numbers (stack traces, stepping debuggers) is significantly degraded. If LispReader were to defer to :line and :column metadata already available on the loaded forms (there when the two generated files are spit out with *print-meta* on), this would not be the case.
Hide
Andy Fingerhut added a comment -

clj-1079-patch-v2.txt dated Feb 7 2013 is identical to Chas's CLJ-1079.diff dated Oct 7 2012, except it applies cleanly to latest master. I believe the only difference is that some white space in the context lines is updated.

Show
Andy Fingerhut added a comment - clj-1079-patch-v2.txt dated Feb 7 2013 is identical to Chas's CLJ-1079.diff dated Oct 7 2012, except it applies cleanly to latest master. I believe the only difference is that some white space in the context lines is updated.
Hide
Andy Fingerhut added a comment -

Sorry for the noise. I've removed clj-1079-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1079.diff dated Oct 7 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1079.diff

I will update the JIRA workflow page instructions for applying patches to mention this, too, because there are multiple patches that fail without --ignore-whitespace, but apply cleanly with that option. That will eliminate the need to update patches merely for whitespace changes.

Show
Andy Fingerhut added a comment - Sorry for the noise. I've removed clj-1079-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1079.diff dated Oct 7 2012 applies cleanly to latest master and passes all tests if you use this command to apply it. % git am --keep-cr -s --ignore-whitespace < CLJ-1079.diff I will update the JIRA workflow page instructions for applying patches to mention this, too, because there are multiple patches that fail without --ignore-whitespace, but apply cleanly with that option. That will eliminate the need to update patches merely for whitespace changes.

People

Vote (2)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: