Clojure

Don't squash explicit :line and :column metadata in the MetaReader

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.4, Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

I have been experimenting with using cljx to produce Clojure and ClojureScript source from a single file. This has gone well so far, with the exception that, due to the way the source transformation works, all of the linebreaks and other formatting is gone from the output. There is an option to include the original :line metadata in the output though, like so:

;;This file autogenerated from 
;;
;;  src/cljx/com/foo/hosty.cljx
;;
^{:line 1} (ns com.foo.hosty)
^{:line 3} (defn ^{:clj true} system-hash [x] ^{:line 5} (System/identityHashCode x))

(Hopefully, such hackery won't be necessary in the future with sjacket or something like it...)

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}
=> (meta (read (java.io.PushbackReader.
                 (java.io.StringReader. "^{:line 66} ()"))))
{:line 66}

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

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: