<< Back to previous view

[CLJS-369] gensyms break re-analyze; prevents variable shadowing analysis Created: 01/Sep/12  Updated: 27/Jul/13  Resolved: 15/Oct/12

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

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

Attachments: Text File CLJS-369-v2.patch     Text File shadowing.patch    
Patch: Code


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.

[snip lots more text]

Consider what an ANF transform would look like for the :let form:

(defmethod anf :let
  [{:keys [env bindings statements ret form] :as ast}]
  (let [bindings (mapcat (fn [{:keys [sym init] :as binding}]
                           [name (anf init)])
        body-env (-> bindings last :env)]
    (ana/analyze env `(~(first form) [~@(map :form bindings)]
                          ~@(anf-block (assoc ast :env body-env))))))

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
    * 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.

Comment by David Nolen [ 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!

Comment by Brandon Bloom [ 03/Sep/12 2:02 PM ]

Reworded commit message to meet new convention.

Comment by David Nolen [ 28/Sep/12 5:55 PM ]

Can we put the shadow patch in another ticket with the patch referencing the new ticket #. Thanks!

Comment by Brandon Bloom [ 28/Sep/12 6:11 PM ]

Not sure what good a separate ticket would do. How about this new title?

Comment by David Nolen [ 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!

Comment by Brandon Bloom [ 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.

Comment by David Nolen [ 03/Oct/12 10:59 AM ]

Right sorry, makes sense now. I will review both patches more closely. Thanks again.

Comment by David Nolen [ 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.

Comment by David Nolen [ 15/Oct/12 11:01 PM ]

Went ahead and applied it, only needed a minor change.

Comment by David Nolen [ 15/Oct/12 11:02 PM ]

fixed, http://github.com/clojure/clojurescript/commit/19afb31a52504293ba2182c584b1867917316662

Comment by Brandon Bloom [ 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!

Comment by David Nolen [ 17/Oct/12 10:53 AM ]

Brandon, yep of course! I didn't know about the --author flag otherwise I would have used it. Thanks again.

Generated at Sat Oct 21 16:10:27 CDT 2017 using JIRA 4.4#649-r158309.