<< Back to previous view

[CLJS-740] undeclared-ns warnings are broken Created: 02/Jan/14  Updated: 17/Jan/14  Resolved: 17/Jan/14

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

Type: Defect Priority: Minor
Reporter: Erik Ouchterlony Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler, bug, patch,

Attachments: File fix-undeclared-ns-warnings.diff    
Patch: Code

 Description   

In the recent versions of cljs, the undeclared-ns warnings have stopped working. This patch seems to be the culprit.



 Comments   
Comment by David Nolen [ 02/Jan/14 12:58 PM ]

Great thanks!

Comment by David Nolen [ 07/Jan/14 10:32 AM ]

CLJS-615

Comment by David Nolen [ 17/Jan/14 9:26 AM ]

fixed, https://github.com/clojure/clojurescript/commit/b8cf302b1500e88e0602e72fa6aec6f7328a1a00





[CLJS-661] Support a try/catch-all mechanism Created: 04/Nov/13  Updated: 05/Nov/13  Resolved: 04/Nov/13

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, patch,

Attachments: Text File CLJS-661-v001.patch    
Patch: Code and Test

 Description   

See http://dev.clojure.org/display/design/Platform+Errors



 Comments   
Comment by David Nolen [ 04/Nov/13 10:45 PM ]

fixed, https://github.com/clojure/clojurescript/commit/9feed772a6db1d2697de387f14c05e7e99c9d891

Comment by Brandon Bloom [ 05/Nov/13 9:32 AM ]

See also http://dev.clojure.org/jira/browse/CLJ-1293





[CLJS-541] review inlining macros for subtle evaluation order bugs Created: 15/Jul/13  Updated: 27/Jul/13  Resolved: 16/Jul/13

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-541.patch    
Patch: Code and Test

 Description   

instance? suffers from this problem



 Comments   
Comment by David Nolen [ 16/Jul/13 6:12 AM ]

fixed http://github.com/clojure/clojurescript/commit/37b4ccefb085aac1cc30a90d13cd5bee736e00c1





[CLJS-441] Require implicit 'do nodes for all blocks in AST Created: 12/Dec/12  Updated: 27/Jul/13  Resolved: 21/Dec/12

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

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

Attachments: Text File CLJS-441-v001.patch    
Patch: Code

 Description   

Discussion here: https://groups.google.com/d/topic/clojure-dev/6CVTsW1EQn4/discussion



 Comments   
Comment by Brandon Bloom [ 12/Dec/12 1:18 PM ]

This patch depends on the patch at http://dev.clojure.org/jira/browse/CLJS-440

Comment by Brandon Bloom [ 12/Dec/12 3:18 PM ]

This patch would also address http://dev.clojure.org/jira/browse/CLJS-416

Comment by David Nolen [ 21/Dec/12 5:36 PM ]

Fixed http://github.com/clojure/clojurescript/commit/764565e9e7696379ada5ac8168b20ee5f9cde6a2





[CLJS-440] Replace :is-loop flag in AST with distinct :loop op Created: 12/Dec/12  Updated: 27/Jul/13  Resolved: 21/Dec/12

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

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

Attachments: Text File CLJS-1126-v001.patch     Text File CLJS-440-v001.patch    
Patch: Code

 Description   

Discussion here: https://groups.google.com/d/topic/clojure-dev/6CVTsW1EQn4/discussion



 Comments   
Comment by Brandon Bloom [ 12/Dec/12 1:14 PM ]

Whoops. Accidentally made a Clojure issue instead of a ClojureScript issue. Moved the ticket and reuploaded patch with correct ticket #

Comment by David Nolen [ 21/Dec/12 5:12 PM ]

fixed, http://github.com/clojure/clojurescript/commit/26b2541cd66d3139eca354434a56137218b17d09





[CLJS-437] Missing "Too few arguments to if" check Created: 06/Dec/12  Updated: 27/Jul/13  Resolved: 07/Dec/12

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

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

Attachments: Text File CLJS-437-v001.patch    
Patch: Code

 Description   

(if) and (if 1) both return nil. JVM clojure throws "Too few arguments to if".



 Comments   
Comment by David Nolen [ 07/Dec/12 12:03 AM ]

fixed, https://github.com/clojure/clojurescript/commit/9abeb143f66ad6d92756c2dd702966f63ae76c27





[CLJS-432] Include line and file information in error messages Created: 28/Nov/12  Updated: 27/Jul/13  Resolved: 30/Nov/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, patch,

Attachments: Text File CLJS-432-v001.patch     Text File CLJS-432-v002.patch     Text File CLJS-432-v003.patch     Text File CLJS-432-v004.patch     Text File CLJS-432-v005.patch     Text File CLJS-432-v006.patch    
Patch: Code

 Description   

Just as the ana/warning function did, now errors and assertions include line and file source information. I needed this sorely.



 Comments   
Comment by Brandon Bloom [ 28/Nov/12 6:13 PM ]

Added v2 of patch that will handle any exception during parsing. Similar could be done during code generation. This approach seems much more robust and less invasive.

Comment by Brandon Bloom [ 28/Nov/12 11:15 PM ]

That v2 was made hastily. Upon further thought, it seems broken in the face of recursive analysis. I think the right thing, if we prefer the exception catching approach, is to rethrow as a custom exception type, which would be allowed through un-re-wrapped.

For example:

(try
  (parse-form ...)
  (catch AnalysisError e
    throw e)
  (catch Throwable e
    (throw (AnalysisError. env e))))
Comment by David Nolen [ 29/Nov/12 11:32 AM ]

I don't have a problem with the approach in the first patch. I don't really see how a less invasive patch is even possible - you still need to pass the environment to some assertion check if you are going to throw a custom exception as well.

Comment by Brandon Bloom [ 29/Nov/12 2:28 PM ]

v3 of patch fixes issue with v2

Comment by Brandon Bloom [ 29/Nov/12 2:50 PM ]

v4 of patch as discussed in irc

Comment by Brandon Bloom [ 29/Nov/12 2:57 PM ]

v5 also catches error from parse-invoke

Comment by Brandon Bloom [ 29/Nov/12 5:05 PM ]

v6 improves errors in repl as discussed in IRC

Comment by David Nolen [ 30/Nov/12 11:25 AM ]

fixed, http://github.com/clojure/clojurescript/commit/af4ab91754d30f082b117b40c07ea94d0063c0d6





[CLJS-431] namespace and name fail on '/ Created: 27/Nov/12  Updated: 27/Jul/13  Resolved: 30/Nov/12

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

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

Attachments: Text File CLJS-431-v001.patch    
Patch: Code and Test

 Description   

Both the 'namespace and the 'name functions use .lastIndexOf to search for "/". This fails on the division symbol if you don't provide a starting index to search from.



 Comments   
Comment by David Nolen [ 30/Nov/12 11:56 AM ]

fixed in commit 5ac15039c685af19810dc5b35f06a496be1f3369





[CLJS-424] Internal representation of keywords can not be differentiated by printable characters Created: 19/Nov/12  Updated: 27/Jul/13  Resolved: 22/Dec/12

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

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

Attachments: Text File CLJS-424-v1.patch    
Patch: Code

 Description   

Currently, Symbols are represented by (str \uFDD1 \' name) and keywords are represented by (str \uFDD0 \' name)

The \uFDDX character is not printable, so it appears as a little box when printed in a javascript repl such as the browser's console. The following character is always a \' regardless of whether the object is a symbol or a keyword. This has bitten me during debugging on several occasions. The attached patch changes keywords to be represented internally by (str \uFFD1 \: name)



 Comments   
Comment by David Nolen [ 22/Dec/12 3:45 PM ]

fixed, http://github.com/clojure/clojurescript/commit/21eab986826bd7a9e852712aaeda647a5de59b3b





[CLJS-416] emit-block doesn't use context argument Created: 09/Nov/12  Updated: 21/Dec/12  Resolved: 21/Dec/12

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

Type: Defect Priority: Trivial
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-316-v1.patch    
Patch: Code

 Description   

emit-block currently has arguments [context statements ret] but the context argument is never used and all call sites always draw statements and ret from the same block AST node. The attach patch simplifies emit-block and all call sites to use arguments [{:keys [statement ret]}]



 Comments   
Comment by Brandon Bloom [ 21/Dec/12 5:30 PM ]

Obsoleted by http://dev.clojure.org/jira/browse/CLJS-441





[CLJS-409] Small code cleanup for emitting goog.provide statements Created: 27/Oct/12  Updated: 27/Jul/13  Resolved: 27/Oct/12

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

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

Attachments: Text File CLJS-409-v1.patch    
Patch: Code

 Description   

Just abstracts out a duplicated piece of code.



 Comments   
Comment by David Nolen [ 27/Oct/12 4:52 PM ]

fixed, http://github.com/clojure/clojurescript/commit/d8f490e9b958a43e97b34808e76d977f29fee8c9





[CLJS-408] Include :form on fn :methods in AST Created: 24/Oct/12  Updated: 27/Jul/13  Resolved: 27/Oct/12

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

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

Attachments: Text File CLJS-408-v1.patch    
Patch: Code

 Description   

All core CLJS AST nodes include a :form key. Several quasi-nodes similarly include their originating code forms. Although :fn nodes include their :form, it varies between single and multiple arity functions. I've found it easier to assume multiple arities and always analyze :methods directly. Unfortunately, the methods lack :form keys. This simple patch exposes the form of each method, simplifying function transformations.



 Comments   
Comment by David Nolen [ 27/Oct/12 4:52 PM ]

fixed, http://github.com/clojure/clojurescript/commit/3ea4a9309346a2a2d0983dc535b9b00531bfc90f





[CLJS-387] Add docstring from def and ns definitions to @namespaces metadata map, and make reflect functions make use of that Created: 07/Oct/12  Updated: 27/Jul/13  Resolved: 17/Oct/12

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

Type: Enhancement Priority: Minor
Reporter: Frank Siebenlist Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docs, enhancement, patch,, reflection
Environment:

clojure/clojurescript "0.0-1450"


Attachments: File add-ns-def-doc-patch.diff    
Patch: Code

 Description   

The docstrings were parsed from the definitions-forms for def and ns, but not added to the @namespaces metadata map.
There is no :doc entry used in the ns' metadata in the @namespaces.
No ns's :doc info is communicated to the browser in the reflect functions with reflect/doc.
Patch-file is attached with code-changes that add the :doc info for ns and def to the @namespaces, and enhances the reflect functions to communicate that info to the browser in the reflect/doc call.



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

This patch no longer applies, mind updating it?

Comment by Frank Siebenlist [ 16/Oct/12 12:10 AM ]

This patch should apply to master version on Mon, 15 Oct 2012 22:03:19 -0700 (4defcbcf19112b9be6a4a27b5d8855552bf94948)

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

Excellent, fixed http://github.com/clojure/clojurescript/commit/bef56a74f2eeecabfe0c0a28d89b455dce576ea3

Please at the ticket # to the commit message though, thanks!





[CLJS-370] Incorrect behavior of integer? for integral floating point expressions Created: 03/Sep/12  Updated: 27/Jul/13  Resolved: 05/Sep/12

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

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

Attachments: Text File CLJS-370-v1.patch    
Patch: Code and Test

 Description   

This includes infinity, exponential expressions, etc.

See: http://stackoverflow.com/questions/3885817/how-to-check-if-a-number-is-float-or-integer



 Comments   
Comment by David Nolen [ 03/Sep/12 4:26 PM ]

changing the behavior of integer? outside of a more comprehensive numeric discussion is low priority. The current implementation is already suboptimal in its coercion of numbers to strings.

Comment by Brandon Bloom [ 03/Sep/12 5:31 PM ]

I'm not changing the behavior of integer?, I'm fixing it. Also, regarding the suboptimal nature of string coercion, the following benchmark seems to show this isn't an issue at all:

Benchmarking with V8
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs


Benchmarking with SpiderMonkey
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs


Benchmarking with JavaScriptCore
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs
Comment by David Nolen [ 04/Sep/12 10:02 AM ]

Those benchmarks are not revealing anything. Advanced compilation is eliminating that code. Try with :simple or :whitespace.

Comment by Brandon Bloom [ 04/Sep/12 11:13 PM ]

sigh I've once again shown my profiling ineptitude. Here's results running with whitespace optimizations:

Benchmarking with V8
[n 5], (old-integer? n), 1000000 runs, 924 msecs
[n 5.5], (old-integer? n), 1000000 runs, 1097 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 1009 msecs
[n ""], (old-integer? n), 1000000 runs, 77 msecs
[n 5], (integer? n), 1000000 runs, 210 msecs
[n 5.5], (integer? n), 1000000 runs, 556 msecs
[n 5.0E300], (integer? n), 1000000 runs, 731 msecs
[n ""], (integer? n), 1000000 runs, 78 msecs


Benchmarking with SpiderMonkey
[n 5], (old-integer? n), 1000000 runs, 236 msecs
[n 5.5], (old-integer? n), 1000000 runs, 362 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 2885 msecs
[n ""], (old-integer? n), 1000000 runs, 94 msecs
[n 5], (integer? n), 1000000 runs, 216 msecs
[n 5.5], (integer? n), 1000000 runs, 230 msecs
[n 5.0E300], (integer? n), 1000000 runs, 1360 msecs
[n ""], (integer? n), 1000000 runs, 98 msecs


Benchmarking with JavaScriptCore
[n 5], (old-integer? n), 1000000 runs, 471 msecs
[n 5.5], (old-integer? n), 1000000 runs, 467 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 771 msecs
[n ""], (old-integer? n), 1000000 runs, 212 msecs
[n 5], (integer? n), 1000000 runs, 882 msecs
[n 5.5], (integer? n), 1000000 runs, 756 msecs
[n 5.0E300], (integer? n), 1000000 runs, 851 msecs
[n ""], (integer? n), 1000000 runs, 213 msecs

Seems like a win on V8 & SpiderMonkey, but a small loss on JavaScriptCore.

Comment by David Nolen [ 04/Sep/12 11:40 PM ]

The slow down on JSC gives me pause. WebKit is ubiquitous in the mobile space.

I'm also not convinced we need to handle Infinity or NaN (though others may differ on that point). Perhaps we can in some debug mode intelligently detect and error out precisely where these kinds of mistakes may occur.

Comment by Brandon Bloom [ 05/Sep/12 1:19 AM ]

It seems like a bad precedent to favor micro-optimization over correctness when speed is easily obtainable through platform interop.

Infinity, NaN, and floating point exponentiation expressions are not integers. I changed the function's behavior to match JVM Clojure's. We're going to need a reliable integer predicate if we want to implement ratios and other compound numeric types.

If you need need a fast predicate for numbers, the number? predicate only checks type. It's behavior also matches JVM Clojure's: it accepts infinity, NaN, etc. If you need a fast integer test, but insist that edge cases like 5e300 are exceptional and ignorable, then you can micro-optimize by calling the necessary javascript primitives yourself.

Comment by David Nolen [ 05/Sep/12 9:55 AM ]

On my machine your patch is slower across all JS engines w/ JSC faring the worst at nearly 2X. I do agree that the behavior is better. However moving forward this means that even simple functions like even? are 100X slower than their Clojure JVM counterparts. There's nothing "micro-optimization" about 2 orders of magnitude.

Comment by David Nolen [ 05/Sep/12 10:06 AM ]

Oops after a lot more testing and playing around with your patch I turned out to be very wrong! It looks your patch + a boolean type hint is a win across the board on all engines, both in terms of performance and correctness.

Comment by David Nolen [ 05/Sep/12 10:07 AM ]

fixed, http://github.com/clojure/clojurescript/commit/3c210d9430b30ffeac9600ae4851e1c347c2cced

Comment by Brandon Bloom [ 05/Sep/12 11:40 AM ]

Heh. Performance is hard





[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

 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.

[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)])
                         bindings)
        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
    
    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.


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





[CLJS-360] Analyzer incorrectly merges metadata when redefining vars Created: 26/Aug/12  Updated: 27/Jul/13  Resolved: 31/Aug/12

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

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

Attachments: Text File redef-meta-2.patch     Text File redef-meta.patch    
Patch: Code

 Description   

I'm experimenting with using the analyzer for some more sophisticated macros, including a CPS transform and control constructs. During interactive development, I discovered that the analyzer is incorrectly merging metadata on vars when redefining them. This patch changes redef's to replace, rather than merge, existing var metadata.

The patch does not include a test, since the tests don't currently muck with the analyzer directly. Here's some code you can play with in your repl:

(require '[cljs.analyzer :as ana])
(require '[cljs.compiler :as comp])

(def env (ana/empty-env))

(defn show-foo [form]
  (ana/analyze env form)
  (-> @ana/namespaces (get 'cljs.user) :defs (get 'x) :foo))

(show-foo '(def ^{:foo 1} x 1))

(show-foo '(def ^{:foo 2} x 1))

(show-foo '(def x 1))  ; before patch, this returns 2. After patch, nil.


 Comments   
Comment by Brandon Bloom [ 26/Aug/12 6:27 PM ]

Also, the patch makes the behavior match JVM Clojure:

user=> (def ^:foo x)
#'user/x
user=> (:foo (meta #'x))
true
user=> (def x)
#'user/x
user=> (:foo (meta #'x))
nil

Comment by David Nolen [ 31/Aug/12 9:29 AM ]

Can we get a patch without whitespace changes? Thanks!

Comment by Brandon Bloom [ 31/Aug/12 11:22 AM ]

Updated patch: no longer changes indent/whitespace and resolves conflict with change that happened to master since original patch.

Comment by David Nolen [ 31/Aug/12 11:29 AM ]

fixed, http://github.com/clojure/clojurescript/commit/8355d1eacff667b77551bb60699d5c85b2f15298





[CLJS-326] hash-set and PersistentHashSet/fromArray == faster set construction Created: 24/Jun/12  Updated: 27/Jul/13  Resolved: 25/Jun/12

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

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

Attachments: Text File making-sets.patch     Text File making-sets-squashed.patch    
Patch: Code and Test

 Description   

As discussed with mmarczyk in irc, the attached patch implements Clojure's hash-set function and modifies the compiler to create sets using a new fromArray static method. To test these, I created three new benchmarks. Here's before and after benchmark results:

$ time ./script/benchmark | grep '#{'
[], #{}, 100000 runs, 77 msecs
[], #{1 2 3}, 100000 runs, 582 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 104 msecs
[], #{}, 100000 runs, 740 msecs
[], #{1 2 3}, 100000 runs, 1809 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 221 msecs
[], #{}, 100000 runs, 281 msecs
[], #{1 2 3}, 100000 runs, 1310 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 288 msecs
./script/benchmark 109.41s user 3.82s system 127% cpu 1:28.96 total

$ time ./script/benchmark | grep '#{'
[], #{}, 100000 runs, 1 msecs
[], #{1 2 3}, 100000 runs, 333 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 100 msecs
[], #{}, 100000 runs, 1 msecs
[], #{1 2 3}, 100000 runs, 1670 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 325 msecs
[], #{}, 100000 runs, 1 msecs
[], #{1 2 3}, 100000 runs, 689 msecs
[coll #{1 2 3}], (conj coll 4), 100000 runs, 321 msecs
./script/benchmark 103.94s user 4.89s system 108% cpu 1:39.88 total
grep '#{' 0.00s user 0.00s system 0% cpu 1:39.85 total



 Comments   
Comment by Brandon Bloom [ 24/Jun/12 8:47 PM ]

Slightly better patch with the EMPTY case

Comment by David Nolen [ 24/Jun/12 10:13 PM ]

Getting a weird error when I try to run the tests. Also can we squash the commits? Thanks!

Comment by Brandon Bloom [ 24/Jun/12 10:24 PM ]

Squashed patch.

Comment by Brandon Bloom [ 24/Jun/12 10:28 PM ]

Squashed and rebased on latest master. Is the rule no-multi-commit patches? Or simply just not for such small patches? In this case, I made and tested each each change in sequence. I also wanted the benchmarks to have a different sha1, so that the perf improvement would show up in the graphs. I'll try to provide squashed patches in the future.

What weird error are you getting? Works for me :-P

Comment by Brandon Bloom [ 24/Jun/12 10:33 PM ]

Updating 2nd benchmark in description to include the EMPTY optimization

Comment by David Nolen [ 25/Jun/12 6:33 AM ]

fixed, http://github.com/clojure/clojurescript/commit/c60df78e2fd318109a9338a87277c11565b506ae





[CLJS-321] Support with-out-str Created: 18/Jun/12  Updated: 27/Jul/13  Resolved: 22/Oct/12

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

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

Attachments: File cljs-321-with-out-str.diff     File cljs-321-with-out-str.diff     Text File with-out-str.patch    
Patch: Code and Test

 Description   

Attached patch implements with-out-str macro in terms of goog.string.StringBuffer and print-fn via .append

The attached patch also fixes CLJS-319



 Comments   
Comment by David Nolen [ 18/Jun/12 8:44 AM ]

Patch looks mostly OK. Did you do any basic benchmarking? It would be nice to know that this doesn't slow down printing of Clojure objects.

Comment by Brandon Bloom [ 18/Jun/12 5:03 PM ]

Yes, I ran this:

(time (dotimes [i 5000] (prn-str {:foo [1 "two" 'three]}))

before patch: 29988 msecs
after patch: 28751 msecs

Effectively identical.

Comment by David Nolen [ 18/Jun/12 8:56 PM ]

on V8? JSC? SM?

Comment by Brandon Bloom [ 19/Jun/12 12:35 AM ]

I had run it in a browser repl, but I forget which. I added a benchmark and re-ran it in all three. It was a tiny bit slower, so I went to investigate and discovered that I haven't the slightest clue how to predict or interpret Javascript engine performance numbers. Hell, Chrome's console seems to have completely random performance, where as the Node.js repl seems to be more consistent. I'll get my shit together and see if I can fix this up. Hopefully will have time to look at it tomorrow.

Comment by Laszlo Török [ 19/Jun/12 3:54 AM ]

jsperf.com was a great help to me when prototyping PersistentVector's first CLJS implementation

it's very-very handy and easy to use and keep updated if necessary

e.g.
http://jsperf.com/persistentvector-norecur-js/12

Comment by David Nolen [ 29/Aug/12 8:42 PM ]

Thanks Brandon, let's hold off on this one until we can resolve CLJS-340

Comment by Paul deGrandis [ 22/Oct/12 4:57 PM ]

I attached an updated patch (that I'm using internally for a personal project) of just the with-out-str macro code.

Comment by David Nolen [ 22/Oct/12 5:00 PM ]

Thanks Paul. Could we get a patch with tests included?

Comment by Paul deGrandis [ 22/Oct/12 5:02 PM ]

No problem - on it right now.

Comment by Paul deGrandis [ 22/Oct/12 5:18 PM ]

Updated with one additional piece I needed to bring over from my branch (:dynamic on print-fn) and two tests: one using print and one using the raw print-fn.

Comment by David Nolen [ 22/Oct/12 6:06 PM ]

fixed, http://github.com/clojure/clojurescript/commit/eab6032e6ba22571e5c4f821707bf5de8075e757





[CLJS-320] memfn not supported Created: 18/Jun/12  Updated: 27/Jul/13  Resolved: 18/Jun/12

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

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

Attachments: Text File memfn.patch     Text File memfn-with-tests.patch    
Patch: Code and Test

 Description   

The docs say "it almost always preferable to do this directly now", but this was one more thing that slowed down the porting of a small piece of JVM Clojure code for me. I'm of the opinion that parity with Clojure proper is desirable where it doesn't hurt platform interop or performance.



 Comments   
Comment by David Nolen [ 18/Jun/12 8:53 AM ]

Can we get a simple test with this patch?

Comment by Brandon Bloom [ 18/Jun/12 4:55 PM ]

Updated patch with tests

Comment by David Nolen [ 18/Jun/12 9:01 PM ]

fixed, http://github.com/clojure/clojurescript/commit/8965df74d424ed41beb3990a55d775d2d851aacd





[CLJS-319] missing spaces when printing the same thing more than once Created: 18/Jun/12  Updated: 27/Jul/13  Resolved: 05/Jul/12

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

Type: Defect Priority: Major
Reporter: Tom Jack Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: patch, patch,
Environment:

9ad79e1c


Attachments: Text File cljs-319.patch    
Patch: Code and Test

 Description   

E.g. (println 1 1) prints "11\n" instead of "1 1\n".

Here's the culprit:
https://github.com/clojure/clojurescript/blob/master/src/cljs/cljs/core.cljs#L6136-6137



 Comments   
Comment by Brandon Bloom [ 18/Jun/12 2:14 AM ]

Both pr-with-opts and pr-sb are affected. See:

https://github.com/clojure/clojurescript/blob/master/src/cljs/cljs/core.cljs#L6107

Comment by Brandon Bloom [ 18/Jun/12 3:16 AM ]

Patch here: CLJS-321

Comment by Brandon Bloom [ 21/Jun/12 6:19 PM ]

Attaching a simple patch for this bug instead of fixing it along side with-out-str

Comment by David Nolen [ 05/Jul/12 3:06 PM ]

fixed, http://github.com/clojure/clojurescript/commit/280ea95938883f97be82267d109342178a2e47aa





[CLJS-304] Comparing a js/Date to nil throws Created: 05/Jun/12  Updated: 27/Jul/13  Resolved: 18/Jun/12

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

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

Attachments: Text File date-equiv2.patch     Text File date-equiv3.patch     Text File date-equiv.patch    
Patch: Code

 Description   

(= (js/Date.) nil)



 Comments   
Comment by Brandon Bloom [ 05/Jun/12 10:27 PM ]

D'oh! Sorry, this patch causes a warning:

WARNING: Use of undeclared Var cljs.core/instance? at line 384

Comment by David Nolen [ 17/Jun/12 12:06 PM ]

updated patch please.

Comment by Brandon Bloom [ 17/Jun/12 4:28 PM ]

Sorry, forgot about this ticket. Attached patch has two additional commits: 1st stops calling .getTime with list/EMPTY via unecessary '() and the 2nd moves instance? from the "preds" to "fundamentals" section of core.cljs.

Comment by David Nolen [ 18/Jun/12 9:01 AM ]

Thanks can we collapse the patch into one commit?

Comment by Brandon Bloom [ 18/Jun/12 4:45 PM ]

Same as patch 2 with commits squashed

Comment by David Nolen [ 18/Jun/12 8:59 PM ]

fixed, http://github.com/clojure/clojurescript/commit/80726438d7375eb72cd4ea82a7ea676d3237b6ce





[CLJS-301] Inline instance? Created: 05/Jun/12  Updated: 27/Jul/13  Resolved: 15/Jul/13

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

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

Attachments: Text File CLJS-301-v002.patch     Text File inline-instanceof.patch    
Patch: Code

 Description   

See discussion on http://dev.clojure.org/jira/browse/CLJS-300

This is a low priority patch for inlining instanceof checks. I don't know if it has any significant performance improvement, but the patch was sort of a side effect of my work on CLJS-300, so I figured I'd bundle it up and share it here in case someone wants to test it's perf impact and apply if it is a win.



 Comments   
Comment by David Nolen [ 22/Dec/12 3:29 PM ]

This will probably result in a minor performance boost, but it is nice to get another jsSTAR out of core.cljs. If you update the patch to work on master, I will apply it.

Comment by Brandon Bloom [ 15/Jul/13 4:47 PM ]

Was fixed awhile ago by another patch





[CLJS-300] RegExp objects print incorrectly Created: 04/Jun/12  Updated: 27/Jul/13  Resolved: 04/Jun/12

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

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

Attachments: Text File print-regexp2.patch     Text File print-regex.patch    
Patch: Code and Test

 Description   

#"x" is currently printing as #</x/>

js/RegExp needs an IPrintable implementation: (-pr-seq [re _] (list "#\"" (.-source re) "\""))

Unfortunately, this is blocked by the same GClosure issue as http://dev.clojure.org/jira/browse/CLJS-68



 Comments   
Comment by David Nolen [ 04/Jun/12 3:58 PM ]

The simplest solution is to add a RegExp case to pr-seq.

Comment by Brandon Bloom [ 04/Jun/12 6:09 PM ]

That was surprisingly not that simple...

(instance? js/RegExp obj) compiles to `cljs.core.instance_QMARK_.call(null, RegExp, obj)` which also triggers the GClosure warning. So I thought I could inline `instance?` with a macro, but the instanceof operator's operands are in reverse order of instance?'s arguments. To preserve evaluation order, the expansion would be `(let t# ~t ('js* "({} instanceof ~{})" ~o t#))) however even that triggers the warning because the output leaves RegExp as an expression by itself again. I wound up just special-casing simple instanceof checks against symbols in the macro.

Attached patch fixes printing of RegExp objects and also enables calling (instance? js/RegExp X) without triggering the warning. Sadly, any more complex expression which evaluates to js/RegExp, or any higher order usage of instance? against that type will still trigger the warning.

Comment by Brandon Bloom [ 04/Jun/12 7:15 PM ]

As discussed, will make a separate ticket for inlining 'instance? and will (defn regexp? [o] (js* "~{o} instanceof RegExp"))

Comment by Brandon Bloom [ 04/Jun/12 8:46 PM ]

Updated with simplified patch

Comment by David Nolen [ 04/Jun/12 11:52 PM ]

fixed, http://github.com/clojure/clojurescript/commit/63cd8ce9c7c9a8864deb676f2b855b8018698d57





[CLJS-297] Eliminate :meta, :vector, :set, and :map ops Created: 03/Jun/12  Updated: 27/Jul/13  Resolved: 15/Jul/13

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

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

Attachments: Text File emit-ds-via-macros.patch    
Patch: Code

 Description   

The attached patch eliminates the :meta, :set, :vector, and :map ops.

These four operations can be defined more simply in terms of
calls to with-meta, set, vector, and hash-map respectively.

The compiler was optimizing construction of vectors and maps. Now,
those optimizations are implemented as macros. Additionally, sets
are optimized in much the same way.

3 files changed, 52 insertions, 99 deletions

Also worth mentioning: as macros instead of ops & emit methods, these optimizations can apply to any backend. The macros create ClojureScript forms, rather than manually generating JavaScript.



 Comments   
Comment by Brandon Bloom [ 17/Jun/12 4:40 PM ]

I'd also like to extend this for Symbols and Keywords.

I've been experimenting with fleshed out Symbol and Keyword objects with interning. I've found that I need emitters, macros, and functions. With the approach here, I could eliminate the emitters and instead have the analyzer produce invocation forms.

Comment by Raphaël AMIARD [ 18/Jun/12 1:20 PM ]

I think this is an interesting patch. It would be worth adapting it to the decoupled emitters. It raises the question of how it would be possible to share part of the emitters between backends.

Comment by David Nolen [ 18/Jun/12 3:25 PM ]

I don't see many benefits falling out of this patch. How to best emit language primitives like literals and constants may vary from host to host - perhaps emitting bytecode directly will work best for some implementations.

Comment by Brandon Bloom [ 18/Jun/12 5:18 PM ]

Couldn't macros emit bytecode via a mechanism similar to the js* form?

My goals with this are:

1) Move some optimizations from the emit phase further down the pipeline. For example, consider choosing the best associative data structure to create. Why should {:foo "bar"} be optimized to an ObjMap or ArrayMap but (hash-map :foo "bar") not be? Why should that optimization be implemented in such a way that it can not be reused by alternative backends?

2) Operate at a higher level. Prefer working with Clojure forms over target-language code fragments (either strings or byte codes). This is where the code length savings is coming from.

If we continue with this approach, I see 4 or 5 more places where analyzer & emitter code can be replaced with shorter, simpler macros, which are more readily reused by alternate backends.

The one implication (downside?) this approach has on consumers of the analyzer or API is that they may need to do a little extra work when considering :invoke operations for static analysis and the like. However, that seems likely for most analyzers anyway, so this would be a matter of (defmethod handle-special-form :map) vs (defmethod handle-invoke :hash-map)

Comment by Michał Marczyk [ 18/Jun/12 5:53 PM ]

Re: 1, I don't think we should be "optimizing" hash-map or array-map (or similar) calls. These functions are a documented way of requesting a map of a particular type (see the docstrings) which I think should not be removed. If anything, we might want to introduce an obj-map function to create arbitrarily large ObjMaps on request (in fact I'll look into that, but that is a separate discussion).

Additionally, the fact that {} is optimized to be a ObjMap in CLJS goes to show that any map-emitting macro will need to be rewritten for each target platform (ObjMap only makes sense when targeting JS, so this optimization simply won't be applicable to other backends). If so and assuming hash-map & Co. retain the behaviour advertised in their docstrings, there's not much gain to implementing this in a macro over just writings a bunch of emitters.

As for decoupling emitters – I think it's perfectly fine for them not to be decoupled, they are the layer closest to the platform after all. Certainly if there's some code which turns out to look the same across multiple platforms it might be worth it to move it upwards in the stack (not necessarily, though – moving it sideways, to a utility namespace / library, might turn out to be more appropriate), but I have a feeling this is an issue best decided once there actually are multiple backends in place and the various costs and benefits can be judged properly.

Now, the story might well be different if we were to introduce some generic factory functions – "create a map of some type", "create a set of some type" etc. – if (and only if!) they would be meant for public consumption. Then implementing a bunch of compiler macros around those new factories and letting them handle data structure literals would save some duplicate work. I don't want to pronounce an opinion on the usefulness of such generic factory functions at this time – just pointing out the possibility.

Comment by Brandon Bloom [ 23/Jun/12 5:14 PM ]

> These functions are a documented way of requesting a map of a particular type

D'oh! You're right.

> we might want to introduce an obj-map

I see you did just that with CLJS-322 – nice.

> the story might well be different if we were to introduce some generic factory functions

There are already some generic factory functions. 'set, for example, is documented as "Returns a set of the distinct elements of coll." despite always returning a PersistentHashSet. Similar for vector and some others. It seems like map is the only core data structure that realistically has several reasonable choices for a default representation.

> implementing a bunch of compiler macros around those new factories and letting them handle data structure literals would save some duplicate work

So all this was somewhat inspired by tagged_literals.clj – You'll see that those functions are effectively macros which take a form and, generally, return an invocation form.

In my mind, I see Clojure's sugar syntax as a strict expansion transformation.

For example, ^:m {:x [@y 'z/w true]} is simply a shortcut for:

(with-meta (make-map (keyword "x") (vector (deref y) (symbol "z" "w") Boolean/TRUE)) (make-map (keyword "m") Boolean/TRUE))

This sort of thing already happens for @ derefs, # lambdas, etc.

In theory, this could be implemented at a level lower than the compiler. You could, for instance, define a reader "desugar" mode which only returns lists and primitives instead of vectors, maps, etc. This would greatly reduce the number of special forms in the compiler, since all of these boil down to invocations with macros.

Emit methods could be replaced with macros for at least these things: vars, maps, vectors, sets, nil, bools, regexes, keywords, symbols, metadata, and empty lists.

The result would be a significant reduction in the amount of code in the compiler for a proportionally smaller increase in the amount of code in per-language macros and maybe the reader.

> I have a feeling this is an issue best decided once there actually are multiple backends in place

I'll grant you that.

I've said my piece on the topic and don't feel very strongly about this particular patch. I just wanted to spark the discussion about reusing more bits of the compiler between backends. In my mind, it's almost always preferable to transform lists than it is to emit strings. I tried that, and the result was a reduction in responsibilities for the analyzer and macros that were easier to work with than emit methods.

Comment by Michał Marczyk [ 24/Jun/12 7:41 PM ]

Some further discussion here:

http://clojure-log.n01se.net/date/2012-06-24.html#20:30a

Comment by Brandon Bloom [ 16/Aug/12 9:34 PM ]

One other advantage of function application over special casing maps/sets/etc is that argument evaluation order is well defined for function application (left-to-right). The Clojure reader returns un-ordered maps & sets, so without changing the reader, we have no way of being able to know what order map key-value-pairs or set elements were originally in. I filed a bug on that. I think we need to make the reader extensible to say to create the return values from their children expressions. In the case of the ClojureScript compiler, we do care about order, so we'd want to return either a (make-map ...) form directly, or a sorted-map by read-order. Same goes for sets.

Comment by David Nolen [ 31/Aug/12 9:24 AM ]

There's not enough rationale for this one.

Comment by Brandon Bloom [ 15/Jul/13 4:50 PM ]

David and I resolved this in a different (better) way:

https://github.com/clojure/clojurescript/commit/f80956d90f455810be140cfec1632f55254385a5





[CLJS-294] AST contains munged symbols Created: 02/Jun/12  Updated: 27/Jul/13  Resolved: 03/Jun/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 emit-munge.patch    
Patch: Code

 Description   

All of the symbols in the abstract syntax tree are munged according for use in JavaScript. This is a problem because it means the AST is lossy and new compiler backends may have differing munging rules.

The attached patch removes all munging from the analysis phase and moves it into the emit phase. The emit phase is backend specific, so it's an appropriate place for munging.



 Comments   
Comment by David Nolen [ 03/Jun/12 9:55 AM ]

It may be better to formalize munging as there are some other bits different backend must do, Raphael Amiard has taken some notes on this here: http://github.com/raph-amiard/clojurescript/wiki/How-to-modularize-parse-analyze-phases-in-the-compiler

Comment by Brandon Bloom [ 03/Jun/12 11:33 AM ]

I agree that we'll eventually want to formalize munging. However, whenever that happens, it should still happen after the AST is constructed. Munging shouldn't happen at all if you're doing refactoring or the like, where you query the AST. Munging is code generation concern.

Prior to my patch, the AST needs to support both :name and :name-sym keys to get around the fact that the :name key loses fidelity of the original symbol. This complects the Clojure-specific parse phase with target-specific needs. The patch renames :name-sym to replace :name.

I read Raphael's notes and he and I have a email thread going on too. With that in mind, this is a step towards untangling the architectural layers of the compiler. By better segregating parse from emit, it will be easier to break the analyzer out from the code generation backend.

Comment by David Nolen [ 03/Jun/12 5:12 PM ]

fixed, http://github.com/clojure/clojurescript/commit/a9d1dc6a043e8a6367492a2cf8622f73f2a947f9





[CLJS-289] Represent ast :children as a vector of keys Created: 31/May/12  Updated: 27/Jul/13  Resolved: 05/Jun/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: Declined Votes: 0
Labels: patch, patch,

Attachments: Text File children-keys.patch    
Patch: Code

 Description   

See discussion here: https://groups.google.com/d/topic/clojure-dev/vZLVKmKX0oc/discussion

Summary: Duplication of ast nodes in various keys and in :children is problematic for some users. In particular, it doesn't play nicely with printing. A solution is needed which preserves "The AST is data. Period."

The attached patch makes no changes to how the sub nodes are currently stored outside of the :children key. Instead, it replaces :children values with a vector of keys into the main ast node. This preserves child ordering and allows a children function to be defined as:

(defn children [ast]
(mapcat #(if (coll? %) % [%]) ((apply juxt (:children ast)) ast)))

The attached patch has two caveats: 1) Many (but not all?) blocks will now be present in the children hierarchy as 'do forms. 2) Interleaved forms are intentionally bugged with respect to ordering. The :children vector for those forms match the actual behavior, not the expected behavior. This can be fixed along with http://dev.clojure.org/jira/browse/CLJS-288



 Comments   
Comment by David Nolen [ 03/Jun/12 10:00 AM ]

the result of the discussion did not end with agreement on representing :children as a vector of keys

Comment by Brandon Bloom [ 03/Jun/12 11:24 AM ]

I realize that, but it was one approach suggested and seemed entirely viable, so I tried it. From what I can tell, it seems like a solution that meets all of the criteria that came up during the discussion.

Did I overlook a requirement?

Who is currently using :children and could weigh in on whether or not this still meets their needs?

Comment by Jonas Enlund [ 03/Jun/12 11:52 AM ]

I'm using :children and I don't think this patch (currently) meets my needs. At least {:op :let ...} is broken IMO. (something like `(map :init bes)` is missing from the patch)

Comment by Brandon Bloom [ 03/Jun/12 12:09 PM ]

Ah OK, you're right. It's the same problem there that I ran into with :statements and analyze-block: There is a pseudo AST node that needs to be traversed down into.

The easy fix for that here would be to merge {:children [:init]} into each binding, but the binding hash will be missing :op and :form. For analyze-block, I was able to use :op :do and :form (list 'do ...), but there isn't an obvious analogy here without inventing a :binding op or relaxing the requirement for :op and :form.

Interestingly, each successive binding can be treated as a single nested binding. What if we defined let as a macro which expands to a series of let1 forms? (let [x 1 y 2] (* x y)) --> (let1 [x 1] (let1 [y 2] (* x y))

That may increase the depth of the AST, but it would really simplify traversal into binding clauses. Each let op would have :name, :init, and children as [:init].

In general, it may be worth while to consider doing the same thing with the various analyze-blocks situations, such that 'do is the only place multiple statements can occur.

Comment by Brandon Bloom [ 03/Jun/12 12:16 PM ]

Er, I mean: a hypothetical let1 op would have children [:init :statements :ret]

However, if you also expanded the implicit do blocks, you'd be able to simplify to [:init :body] or something like that.

Comment by Brandon Bloom [ 03/Jun/12 12:34 PM ]

Forgive me for repeatedly commenting, but it also occurs to me that you can change 'do to a macro which expands to a series of 'do2 forms:

(do x y z) --> (do2 x (do2 y z)) or (do2 (do2 x y) z)

That do2 op could have children [:left :right] and all other :statements and :ret children could be simplified down to 'do2 expansions.

With that in mind, the children fn becomes simply (defn children [ast] ((apply juxt (:children ast)) ast)) and a lot of analyze and emit code gets much simpler by not having to map over many collections all over the place.

Kinda a wild idea and probably not the right forum for it, but worth suggesting. Thoughts?

Comment by David Nolen [ 04/Jun/12 11:35 AM ]

I see little benefit to changing core macros. I also see no problem with creating a :binding ast node.

Comment by Jonas Enlund [ 05/Jun/12 12:44 AM ]

Every node (i.e. an map with :op, :form and :env keys, called an "Expression Object" in the docstring for analyze) in ClojureScript corresponds to a self-contained expression. A binding form is not self-contained, it is a part of the let expression.

Another similar example is functions. There is a node {:op :fn ...} but there is no {:op :fn-method} even thou there are maps under the :methods key describing function methods. The same argument goes for this: A function method is not self-contained, it is part of the function and should not be a node.

The nodes (expression objects) which make up the ClojureScript AST seems to be really well thought out and I would be careful in making the proposed change.

Comment by David Nolen [ 05/Jun/12 8:09 AM ]

Agreed I think it's far too early to have a ticket for this. Confluence page first.

Comment by Brandon Bloom [ 29/Aug/12 12:09 AM ]

Design page here: http://dev.clojure.org/display/design/AST+children





[CLJS-260] Add clojure.core/shuffle implementation Created: 17/May/12  Updated: 27/Jul/13  Resolved: 20/May/12

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

Type: Enhancement Priority: Minor
Reporter: Evan Mezeske Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch,, test

Attachments: Text File 0001-Add-clojure.core-shuffle-and-a-test.patch     Text File shuffle.patch     Text File shuffle.v2.patch    
Patch: Code and Test

 Description   

I added a simple implementation of clojure.core/shuffle, which uses goog.array's Fisher-Yates for its implementation. Included in the patch is a test to make sure it works.



 Comments   
Comment by David Nolen [ 17/May/12 7:07 PM ]

Why don't you return a vector like Clojure does?

Comment by Evan Mezeske [ 17/May/12 7:11 PM ]

Return a vector like Clojure.

Comment by David Nolen [ 18/May/12 9:23 PM ]

The patch is not correctly formatted with attribution.

Comment by Evan Mezeske [ 20/May/12 4:24 PM ]

Use git format-patch instead of git diff, to hopefully provide the right patch format.

Comment by David Nolen [ 20/May/12 10:28 PM ]

fixed, http://github.com/clojure/clojurescript/commit/b06905863a9b0ce3618634d8fe0effb3aefd4063





[CLJS-215] js-obj function broken Created: 29/Apr/12  Updated: 27/Jul/13  Resolved: 29/Apr/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,

Attachments: Text File js-obj-fn.patch    
Patch: Code and Test

 Description   

The js-obj macro is fine, but the js-obj function was incorrectly using "{foo: bar}" expecting "foo" to be evaluated as a variable, instead of as a string key.

Since we have the js-obj macro for speed, I just deleted the broken overloads, so now all the non-zero arity cases simply call gobject



 Comments   
Comment by David Nolen [ 29/Apr/12 5:08 PM ]

fixed, https://github.com/clojure/clojurescript/commit/f4a3ef43dc70aed4f15045836d709989a95ef6f7





[CLJS-211] apply does not work on IFn objects in CLJS Created: 27/Apr/12  Updated: 27/Jul/13  Resolved: 28/Apr/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,

Attachments: Text File apply1.patch    
Patch: Code and Test

 Description   

http://dev.clojure.org/jira/browse/CLJS-135 fixed this for multimethods, but the same issue applies to anyone who implements IFn.

The attached patch fixes this by modifying extend-type to emit an apply method on the extended object's prototype.



 Comments   
Comment by Brandon Bloom [ 28/Apr/12 2:31 AM ]

Updated with patch!

Worth noting: String has an implementation of IFn and manually defines prototype.apply. I contemplated deleting it, since this makes it unnecessary, however, my general purpose implementation is probably a tad slower. Maybe, maybe not. Depends on how Google's inlining does. I figured: make it work, then make it fast. So I didn't bother profiling them and left the existing implementation there.

Comment by David Nolen [ 28/Apr/12 12:50 PM ]

fixed, https://github.com/clojure/clojurescript/commit/ddd8199b324997cfe0537be66372162b41ce8293





[CLJS-210] Implement Var form, var-get, and var? in CLJS Created: 27/Apr/12  Updated: 28/Oct/12

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

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch,

Attachments: Text File vars1.patch    
Patch: Code and Test

 Description   

See discussion of Vars in CLJS here: http://dev.clojure.org/display/design/Dynamic+Binding
My goal is to eventually implement dynamic scope capture, via bound-fn and friends. The attached patch is a step in that direction.

This patch adds support for parsing the (var foo) form. The #' reader macro is provided by JVM Clojure.

#'foo emits code to construct a Var object. In this patch, each invocation of 'var will create a unique Var object. This means they are '= by fully qualified symbol, but not 'identical?. Simple memoization would fix that, but I'm not going to bother until I get to Dynamic Var objects.

The main advantage of this level of Var support is for the interactive development convenience of being able to defer dereferencing top-levels. For example, (def fog (comp f #'g)) will pick up redefinitions of 'g, but not of 'f.



 Comments   
Comment by Brandon Bloom [ 28/Apr/12 10:07 PM ]

I found two issues with this patch:

1) I never used the IVar that I declared :-P

2) (apply #'+ (range 100)) throws "Invalid arity: 101" – this is related to http://dev.clojure.org/jira/browse/CLJS-211

I'll look into fixing both. However, we should discuss supporting variable arity protocol methods...

Comment by David Nolen [ 17/Jun/12 12:17 PM ]

any support for reified vars is a low priority for the foreseeable future.

Comment by Brandon Bloom [ 01/Sep/12 7:46 PM ]

Reified vars (and binding frames!) are a requirement for a CPS transform to be correct with respect to dynamic bindings.

Comment by Tom Jack [ 10/Sep/12 5:11 PM ]

+1 towards bound-fn and friends. I had an explanation written here but now I see "async Javascript (ie. nearly all Javascript) makes the binding macro effectively useless without bound-fn". Indeed.

Comment by Herwig Hochleitner [ 28/Oct/12 7:34 PM ]

bound-fn could also be implemented by setting / resetting bound vars before / after the wrapped fn, similar to binding. We would just need to add tracking of the currently rebound dynamic vars.
That should also be CPS - friendly, no?

Comment by Tom Jack [ 28/Oct/12 9:03 PM ]

I've considered that and would love to see it happen.

But what about:

(def ^:dynamic *foo* 3)
(set! *foo* 4)
(def f (bound-fn [] *foo*))
(set! *foo* 5)
(f)




[CLJS-177] js->clj should convert JavaScript null to Clojure nil Created: 10/Apr/12  Updated: 27/Jul/13  Resolved: 10/Apr/12

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

Type: Defect Priority: Minor
Reporter: Jason Rudolph Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch,

Attachments: File js-null-to-clj-nil.diff    
Patch: Code and Test

 Description   

Prior to commit ebf45f5 [A], js->clj converted JavaScript null to Clojure nil without any errors.

As of commit ebf45f5 (related to CLJS-175), js->clj throws a TypeError when it encounters JavaScript null:

TypeError: Cannot read property 'constructor' of null
b===c}function q(b,c){return gb.call(e,b,c)}function pb(b){return b.construct
^
TypeError: Cannot read property 'constructor' of null

The attached patch [B] adds a test to demonstrate the issue and provides a suggested fix.

Note: CLJS-175 intentionally limited the types of objects it would attempt to convert to Clojure code. For example, CLJS-175 notes that conversion "doesn't make sense for something like Date or RegExp." I agree for those types of objects, but "null" seems straightforward enough that it should be converted.

[A] https://github.com/clojure/clojurescript/commit/ebf45f5
[B] Also viewable here: https://github.com/jasonrudolph/clojurescript/compare/master...js-null-to-clj-nil



 Comments   
Comment by David Nolen [ 10/Apr/12 7:38 PM ]

Fixed, http://github.com/clojure/clojurescript/commit/9319579acfc4fc9dbcf6e79f611afad707f97579





[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

 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.



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

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.





[CLJS-169] Forward slashes should not be escaped in javascript strings Created: 25/Mar/12  Updated: 27/Jul/13  Resolved: 30/Mar/12

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

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

Attachments: File no-escape-slash.diff    
Patch: Code

 Description   

cljs.compiler/escape-char incorrectly escapes forward slashes.

Generally, this isn't a problem because most Javascript engines treat unknown escape sequences as non-escape sequences. However, I ran into a JSON parser that barfs on it while working on source-maps. I tried to use escape-string for producing the JSON-based source-map by reusing the existing compiler functions for emitting valid Javascript.



 Comments   
Comment by David Nolen [ 30/Mar/12 3:21 PM ]

fixed, https://github.com/clojure/clojurescript/commit/29d60c7eb2c79768e2e2a2c572a4e4eff132946d





[CLJS-164] Add missing core printing functions Created: 18/Mar/12  Updated: 27/Jul/13  Resolved: 30/Mar/12

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

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

Attachments: File pr-fns.diff     File pr-fns-with-tests.diff    
Patch: Code

 Description   

I've attached a patch which adds a few missing printing functions to ClojureScript core.

New functions:

  • prn-str
  • print-str
  • println-str


 Comments   
Comment by David Nolen [ 19/Mar/12 12:14 PM ]

Mind adding the related tests?

Comment by Brandon Bloom [ 25/Mar/12 11:16 PM ]

New diff with tests.

Sorry I didn't get to these sooner. I'm set as "watching" this issue, but I didn't get emailed when you commented.

Comment by David Nolen [ 30/Mar/12 3:20 PM ]

fixed, https://github.com/clojure/clojurescript/commit/3a3d9b1e3ab8858f26711a8860f48619736eba2d





Generated at Thu Jul 31 21:09:05 CDT 2014 using JIRA 4.4#649-r158309.