ClojureScript

Track line & column in generated JS (needed for SourceMaps)

Details

  • Type: Enhancement Enhancement
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

The attached patch modifies the ClojureScript compiler by replacing all calls to print, println, emits, etc, such that all code generation funnels through the new emitx and emitln. The emitx and emitln functions track position as a 2-tuple vector of [line, column], zero-based. The line and column information is a prerequisite for generating correct source maps. NOTE: This is the position in the generated JavaScript source, not the :line and :column metadata from the Clojure reader.

In the emitln function, there are a few commented out lines which constitute a "test". Basically, it causes the compiler to append a line number // comment on each line of the generated source. The comments are horizontally aligned at 120 characters, so you can quickly visually inspect for alignment bugs. Unfortunately, I'm not really sure how better to test this, or to ensure that this keeps working until the rest of SourceMaps gets implemented In theory, as long as no one prints anything directly, always using emitx and emitln, nothing should break.

Attachments

  1. File
    track-pos-1.diff
    09/Apr/12 2:16 AM
    29 kB
    Brandon Bloom
  2. Text File
    track-pos-2.patch
    14/Apr/12 7:49 PM
    30 kB
    Brandon Bloom
  3. Text File
    uninit-var-print.patch
    16/Apr/12 11:49 PM
    0.7 kB
    Brandon Bloom

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Brandon Bloom added a comment -

This patch is now out of date. I'd be happy to update it, but the compiler moves fast enough that I'll wait for general feedback. I don't want to have to constantly re-update this patch.

Show
Brandon Bloom added a comment - This patch is now out of date. I'd be happy to update it, but the compiler moves fast enough that I'll wait for general feedback. I don't want to have to constantly re-update this patch.
Hide
Permalink
David Nolen added a comment -

emitx is a little too ambiguous for my tastes. What about emit-ln and emit-str? Other options? Once we decide on names I'd be happy to merge this in quickly.

Show
David Nolen added a comment - emitx is a little too ambiguous for my tastes. What about emit-ln and emit-str? Other options? Once we decide on names I'd be happy to merge this in quickly.
Hide
Permalink
Brandon Bloom added a comment -

This patch removes a function that was called emits (note the "s"). It simply called emit inside a with-out-string.

The emitx function in this patch isn't that simple. It does some special handling of nil, maps, seqs, functions, etc. It creates a mini DSL for emitting code, so that I didn't have to drastically rewrite all the places that used (print (emit-str ...)) and similar before.

I decided not to call it emit-str to avoid confusion with the old emits function, as well as to differentiate the behavior from prn-str and the others, which return a string without affecting the previously set out. Quite honestly, the best name for it is simply "emit", but that honor goes to the current multi-method, which is probably best named "emit-ast". I don't think it's worth the churn to rename that.

Either way, I'm not too picky. If you have a preferred name, I'll update the patch to whatever you like.

Show
Brandon Bloom added a comment - This patch removes a function that was called emits (note the "s"). It simply called emit inside a with-out-string. The emitx function in this patch isn't that simple. It does some special handling of nil, maps, seqs, functions, etc. It creates a mini DSL for emitting code, so that I didn't have to drastically rewrite all the places that used (print (emit-str ...)) and similar before. I decided not to call it emit-str to avoid confusion with the old emits function, as well as to differentiate the behavior from prn-str and the others, which return a string without affecting the previously set out. Quite honestly, the best name for it is simply "emit", but that honor goes to the current multi-method, which is probably best named "emit-ast". I don't think it's worth the churn to rename that. Either way, I'm not too picky. If you have a preferred name, I'll update the patch to whatever you like.
Hide
Permalink
David Nolen added a comment -

It's a big patch, it's not clear to me why you need emitx to handle all those cases. Can you point out specific lines? Thanks.

Show
David Nolen added a comment - It's a big patch, it's not clear to me why you need emitx to handle all those cases. Can you point out specific lines? Thanks.
Hide
Permalink
Brandon Bloom added a comment -

Any line that used to use 'emits was potentially problematic because it allowed things to be printed out of order. Consider this:

(let [xyz (with-out-str (print-mystery))]
(println "abc")
(println xyz)
(println "123"))

How many newlines were printed? At what column is "123" printed? Without knowing the contents of 'xyz, there is no way to know. You could split by occurrences of
n in 'xyz, but that would negatively affect performance and doesn't alleviate the need to funnel all printing through a single choke point.

Instead, I opted to restructure to something like this:

(let xyz #(emit-mystery)
(emitln "abc")
(xyz)
(emitln "123"))

So that's why 'emitx handles 'fn? and the :else case. 'map? is for handling the ast 'emit multi-method. 'nil? and 'seq? are convinces for common cases. The behavior is similar to expansion of seqs in Hiccup. There are numerous examples of each.

Show
Brandon Bloom added a comment - Any line that used to use 'emits was potentially problematic because it allowed things to be printed out of order. Consider this: (let [xyz (with-out-str (print-mystery))] (println "abc") (println xyz) (println "123")) How many newlines were printed? At what column is "123" printed? Without knowing the contents of 'xyz, there is no way to know. You could split by occurrences of
n in 'xyz, but that would negatively affect performance and doesn't alleviate the need to funnel all printing through a single choke point. Instead, I opted to restructure to something like this: (let xyz #(emit-mystery) (emitln "abc") (xyz) (emitln "123")) So that's why 'emitx handles 'fn? and the :else case. 'map? is for handling the ast 'emit multi-method. 'nil? and 'seq? are convinces for common cases. The behavior is similar to expansion of seqs in Hiccup. There are numerous examples of each.
Hide
Permalink
David Nolen added a comment -

K I understand that patch quite a bit better now. Makes sense. I still think emitx is not a great name. Lets stick with emits.

Show
David Nolen added a comment - K I understand that patch quite a bit better now. Makes sense. I still think emitx is not a great name. Lets stick with emits.
Hide
Permalink
Brandon Bloom added a comment -

I just looked through the source and renaming 'emit to 'emit-ast and 'emitx to 'emit wouldn't actually be that much churn. There are only one or two references to 'emit outside of the 'defmethod calls. Also, anyone who calls 'emit, expecting 'emit-ast, will still get the right result. I'll go with that, unless you feel strongly about 'emits.

Hopefully, I'll find time to update the patch this weekend.

Is there a resource regarding testing the compiler? Wiki page or something? I've been diffing the output of the Twitter sample, but it would be nice to have a more rigorous test harness.

Show
Brandon Bloom added a comment - I just looked through the source and renaming 'emit to 'emit-ast and 'emitx to 'emit wouldn't actually be that much churn. There are only one or two references to 'emit outside of the 'defmethod calls. Also, anyone who calls 'emit, expecting 'emit-ast, will still get the right result. I'll go with that, unless you feel strongly about 'emits. Hopefully, I'll find time to update the patch this weekend. Is there a resource regarding testing the compiler? Wiki page or something? I've been diffing the output of the Twitter sample, but it would be nice to have a more rigorous test harness.
Hide
Permalink
Brandon Bloom added a comment -

Sorry, shouldn't write comments before I have my coffee

I know about this page: https://github.com/clojure/clojurescript/wiki/Running-the-tests

But that page mentions "make sure the repl hasn't broken". I'm assuming that the test coverage for the compiler isn't good enough to accept patches without additional manual testing. Is that a correct assumption? What's your test process? I'd like to minimize the effort for you to accept patches in the future.

Show
Brandon Bloom added a comment - Sorry, shouldn't write comments before I have my coffee I know about this page: https://github.com/clojure/clojurescript/wiki/Running-the-tests But that page mentions "make sure the repl hasn't broken". I'm assuming that the test coverage for the compiler isn't good enough to accept patches without additional manual testing. Is that a correct assumption? What's your test process? I'd like to minimize the effort for you to accept patches in the future.
Hide
Permalink
David Nolen added a comment -

I see no compelling reason to switch to emit-ast. Lets just stick with emit, emits, emitln please.

As far as testing, I normally just test using ./script/test. Issues w/ Browser REPL tend to crop up if someone is messing with closure.clj or the REPL source itself. It's simple to test and I don't mind doing that.

Show
David Nolen added a comment - I see no compelling reason to switch to emit-ast. Lets just stick with emit, emits, emitln please. As far as testing, I normally just test using ./script/test. Issues w/ Browser REPL tend to crop up if someone is messing with closure.clj or the REPL source itself. It's simple to test and I don't mind doing that.
Brandon Bloom made changes -
Field Original Value New Value
Attachment track-pos-2.patch [ 11051 ]
Brandon Bloom made changes -
Attachment track-pos-2.patch [ 11051 ]
Brandon Bloom made changes -
Attachment track-pos-2.patch [ 11052 ]
Hide
Permalink
Brandon Bloom added a comment -

Updated with patch 2. Please don't make me merge more optimizations

Show
Brandon Bloom added a comment - Updated with patch 2. Please don't make me merge more optimizations
Hide
Permalink
Brandon Bloom added a comment -

You want attachment 11052, not 11051

Show
Brandon Bloom added a comment - You want attachment 11052, not 11051
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
Brandon Bloom made changes -
Attachment uninit-var-print.patch [ 11059 ]
Hide
Permalink
Brandon Bloom added a comment -

Apparently I missed one println call on line 443. I've attached uninit-var-print.patch to correct it.

I hope you see updates to this issue, even though it's resolved. It seems really heavyweight to use a JIRA ticket for such a trivial change. Let me know if I really should have.

Show
Brandon Bloom added a comment - Apparently I missed one println call on line 443. I've attached uninit-var-print.patch to correct it. I hope you see updates to this issue, even though it's resolved. It seems really heavyweight to use a JIRA ticket for such a trivial change. Let me know if I really should have.
Hide
Permalink
David Nolen added a comment -

Fixed in master, thanks.

Show
David Nolen added a comment - Fixed in master, thanks.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.