<< Back to previous view

[CLJS-176] Track line & column in generated JS (needed for SourceMaps) Created: 09/Apr/12  Updated: 27/Jul/13  Resolved: 15/Apr/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch,

Attachments: File track-pos-1.diff     Text File track-pos-2.patch     Text File uninit-var-print.patch    
Patch: Code


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.

Comment by Brandon Bloom [ 12/Apr/12 12:03 AM ]

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.

Comment by David Nolen [ 12/Apr/12 11:03 AM ]

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.

Comment by Brandon Bloom [ 12/Apr/12 1:39 PM ]

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.

Comment by David Nolen [ 12/Apr/12 2:42 PM ]

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.

Comment by Brandon Bloom [ 12/Apr/12 5:50 PM ]

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")
(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.

Comment by David Nolen [ 13/Apr/12 8:23 AM ]

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.

Comment by Brandon Bloom [ 13/Apr/12 2:16 PM ]

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.

Comment by Brandon Bloom [ 13/Apr/12 2:29 PM ]

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.

Comment by David Nolen [ 13/Apr/12 3:01 PM ]

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.

Comment by Brandon Bloom [ 14/Apr/12 7:49 PM ]

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

Comment by Brandon Bloom [ 14/Apr/12 7:54 PM ]

You want attachment 11052, not 11051

Comment by David Nolen [ 15/Apr/12 2:12 PM ]

Fixed, https://github.com/clojure/clojurescript/commit/e4c6ae4e0639efaa648efa351c366a2f686be51e

Comment by Brandon Bloom [ 16/Apr/12 11:52 PM ]

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.

Comment by David Nolen [ 17/Apr/12 12:57 PM ]

Fixed in master, thanks.

Generated at Tue Jan 27 02:34:15 CST 2015 using JIRA 4.4#649-r158309.