Details

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.

Brandon Bloom
added a comment - 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.

David Nolen
added a comment - 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.

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.

Brandon Bloom
added a comment - 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.

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.

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.

Brandon Bloom
added a comment - 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")
(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.

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.

Brandon Bloom
added a comment - 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.

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.

Brandon Bloom
added a comment - 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.

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.

David Nolen
added a comment - 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.

Brandon Bloom
added a comment - 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.