Details

Description

From my email discussion with dnolon before making this patch:

The current analyzer does some trickery to prepare for emitting to JavaScript. Among this trickery is gensyms for locals (including "this"), the new $ prefix on some namespaces, uniqify on parameters, and more. This must be mildly annoying for people writing alternate compiler backends, but for the most part non-blocking because fewer symbol collisions should never be an additional problem for a target language with different symbol resolution rules.

Simple enough, right? This walks each binding, ANF transforms the init expression, gets the environment in which to analyze the body, and then analyzes a new let (or loop) statement with the modified bindings.

Unfortunately, this doesn't work. When the ana/analyze call happens, body-env contains gensymed locals. The result is that body-env is invalidated by the outer analyze call, which is re-generating the symbols for the local variables. If you take the gensyms out of analyze-let, then analyze becomes pure (modulo def forms) and this code magically becomes correct. I've run into this same problem anywhere gensyms are used in analyze.

Commit message on the patch:

AST Changes
* Anywhere a binding was introduced for a local used to be a symbol,
now it is a map with a :name key and potentially a :shadow key.
* Bindings vectors are no longer alternating symbols, then init maps.
Instead, the are a vector of maps of the shape described for locals
plus an :init key.
* The :gthis key for functions has been replaced with :type, which
is the symbol describing the type name of the enclosing deftype form.
* recur frames now expose :params as binding maps, instead of :names
Benefits:
* Shadowed variables are now visible to downstream AST transforms.
* :tag, :mutable, and other metadata are now uniform across ops
* Eliminates usages of gensym inside the analyzer, which was a source
of state that made the analyzer impossible to use for some
transformations of let, letfn, etc which require re-analyzing forms.
* Removes JavaScript shadowing semantics from the analyze phase.

David Nolen
added a comment - 03/Sep/12 1:13 PM Can we please get the ticket number in the first line of the patch? If you look at the ClojureScript commit history you can see the convention that's been adopted. Thanks!

They are separate patches. One is a enhancement to the compiler. The other one is an enhancement to my simplistic shadowing solution using the improvements to the compiler in the other enhancement. Thanks!

David Nolen
added a comment - 29/Sep/12 12:39 PM They are separate patches. One is a enhancement to the compiler. The other one is an enhancement to my simplistic shadowing solution using the improvements to the compiler in the other enhancement. Thanks!

I'm not sure how I can break this patch down into smaller pieces. All of the gensyms were there before to eliminate potential shadowing; the two issues are tightly related. If you eliminated all the gensyms, the compiler would work fine... unless you shadowed a variable (which several tests cover).

Have you studied the patch? Can you suggest a concrete way to break it up into smaller patches? I'm not sure it's worth the trouble.

Brandon Bloom
added a comment - 30/Sep/12 12:34 PM Are you talking about the namespace shadowing? This patch affects all variable shadowing. For example, (fn [x x] x) will produce `function (x x__$1) { return x__$1; }` instead of `function (x_G12 x_G13) { return x__G13; }` or something like that.
I'm not sure how I can break this patch down into smaller pieces. All of the gensyms were there before to eliminate potential shadowing; the two issues are tightly related. If you eliminated all the gensyms, the compiler would work fine... unless you shadowed a variable (which several tests cover).
Have you studied the patch? Can you suggest a concrete way to break it up into smaller patches? I'm not sure it's worth the trouble.

David Nolen
added a comment - 15/Oct/12 10:43 PM I've finally found some time go through this patch. It's great but it no longer applies to master. If I get an updated patch I';; apply promptly. Thank you very much and sorry for the delay.

Off topic: I like to keep tabs on my open source contributions semi-automatically using the author information stored in git. Sadly, your minor change switched the author to you. If it's not too much to ask, could you please preserve the author info in the future? Either via a separate fixup commit, or by using the --author flag when editing commits. Thanks!

Brandon Bloom
added a comment - 15/Oct/12 11:45 PM Awesome. Glad to see this applied.
Off topic: I like to keep tabs on my open source contributions semi-automatically using the author information stored in git. Sadly, your minor change switched the author to you. If it's not too much to ask, could you please preserve the author info in the future? Either via a separate fixup commit, or by using the --author flag when editing commits. Thanks!