The function generated by defprotocol does not work in IE8. I haven't tried any other versions of IE. The fix is pretty simple, however I don't know the edge cases so consider the patch of a proof of concept.

Inside the function the reference to _equiv is unqualified. In IE8, this evaluates to undefined. Using the fully qualified name fixes the issue. So, _equiv[goog.typeOf.call(null, a)] becomes cljs.core._equiv[goog.typeOf.call(null, a)], and equiv. becomes cljs.core.equiv.

The issue is that, if you have a namespace called 'foo.core' and a function called 'foo', foo clashes with the namespace inside the generated JavaScript code and results in odd and generally indecipherable errors. Here is some example code that fails:

This patch uses `io/reader` in conjunction with `with-open` to prevent
"too many open files" errors. This happens in a browser REPL when
evaluating a file that references a couple of other files multiple
times.

with my current REPL setup I get some really annoying "Too many
files open" errors. I'm not sure if it's a problem with
ClojureScript itself, Austin the browser REPL, nREPL or my own
miss-configuration of something.

When I connect to the browser REPL via AUSTIN and eval a whole
ClojureScript file the first time a lot of Ajax requests are sent
over the wire and my main namespace is getting compiled and
shipped to the browser. So far so good, my Java process is at
around 18676 open files. I don't care yet.

Compiling the same file again and again increases the open files

not sure if this is a problem with my setup, but I

18676, 19266, 22750, 21352, 33097, 62913, 64398, 64398, 64398,
64398, 64398 up to 171977, where some ulimit is reached and I get
an exception like this:

java.io.FileNotFoundException: .repl/5614/request/routes.js (Too many open files)

and my ClojureScript hangs up and I have to do a
cider-restart. Ok maybe I shouldn't eval whole files too often
over the XHR connection, but this seems not right.

I used the command "lsof -n | grep java | wc -l" to watch the
above numbers while evaluating the file again and again.

Does someone had a similar problem, knows how to solve that, or
has any ideas how to track this one down?

I am going to split out the (deftype T ... Object ...) capability into its own ticket as it's scope is greater than the accessing of this. It's likely that the new ticket will eliminate the need for extend-object!.

It looks like the issue is that emit-fn-method in compiler.clj just emits :name from the fn AST, but this :name lack the shadow information present that we pass along via locals in parse fn* star case in analyzer.clj

Note the case with not-found doesn't bother to check if it's dealing with an ObjMap. This is because an ObjMap is capable of holding nil or false as a value, so we would still need to call contains? which on an ObjMap does exactly the same work as -lookup, except it returns true / false thus forcing us to pick the correct return value.

This patch introduces a new function – {cljs.core/hash-imap} – which calculates a hash for a map using the algorithm of {clojure.lang.APersistentMap} and wires it into the {-hash} implementations of the various map types. The benefit is that it is order-independent and thus guarantees a stable hash value congruent with {=} without the need for keyset sorting.

This should also be used in sets once {PersistentTreeSet} lands (along with the tree map).

The divergence between the Clojure and ClojureScript way is due to first-class functions. That is, in JavaScript a property can be a field or a function or a method. There is currently no way to distinguish between the desire to call a method and retrieve a function as a property except through syntax. This ticket would shorten the gap between Clojure and ClojureScript in the following way (ClojureScript proposal below):

Back in the dark ages before Clojure 1.0, you always had to write this;

(. o (m)) ; method call
(. o f) ; field access

Later, but before the introduction of (.m o) syntax, this was shortened to (. o x) for both methods and fields. What if we went back to the original dot operator syntax, but assume that (.m o) always means a method call:

Raw field access is rare in Java, perhaps less so in JavaScript. But this doesn't require adding any new syntax and is consistent with what Clojure programmers are used to.

The common case for field access in ClojureScript is, I believe, interop which uses JavaScript objects as map-like data structures. My preferred solution for that case would be better conversion between JS objects and real ClojureScript maps.

I would also prefer Stuart's solution due to the reasons he mentioned and because the currently implemented concatenation of sugar characters (.-somefield obj) didn't really make the matter taste more sweet it feels rather clumsy to me.
By the way, I would also propose to implement 'memfn' for staying consistent with Clojure (if it hasn't already been done so).

Patch fixes typo and causes existing reducer tests to pass (tests run with whitespace optimization). Doesn't make sense to write any more tests, although the test runner should probably not use advanced compilation as the default....

This closure lib version is incompatible with the closure compiler closurescript depends on at least because of changes to how goog.base works. See commit 5bdb54d221dbf7c6177ba5ba6901c012981501ec on the closure compiler library (and many more after this one with a similar commit message).

When you advanced-compile a cljs project, goog classes which internally use the goog.base(this, 'superMethod') form will all be munged incorrectly and throw exceptions. Minimal test case: (ns myproj (:require goog.net.XhrIo)) (goog.net.XhrIo/send "http://example.org" #(js/console.log %)). This will fail at a "disposeInternal" call. (Notice that the output js still has the "disposeInternal" string!)

The workaround suggested above (using the old closure library) works, but versions of clojurescript newer than 2197 cannot be used in production without the workaround. If this bug is going to remain open in new releases, please consider documenting the workaround in the release notes.

Bumped version. Note that this patch is unnecessary in the 1.6.0 branch, as its versions are up to date and its pom and project.clj agree. This commit is likely conflict when 1.6.0 is merged into master (the master side should be ignored if that happens).

The attached patch solves only a part of the issue. After applying the patch I was still not able to compile twitterbuzz. Here's the exception I got:

c:\development\cvstree\clojurescript\samples\twitterbuzz>..\..\bin\cljsc src > twitterbuzz.js
Exception in thread "main" java.lang.RuntimeException: java.io.FileNotFoundException: The file C:\development\cvstree\clojurescript\src\cljs\cljs%5ccore.cljs does not exist.
at clojure.lang.Util.runtimeException(Util.java:153)
at clojure.lang.Compiler.eval(Compiler.java:6417)
at clojure.lang.Compiler.load(Compiler.java:6843)
at clojure.lang.Compiler.loadFile(Compiler.java:6804)
at clojure.main$load_script.invoke(main.clj:282)
at clojure.main$script_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:426)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:405)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:518)
at clojure.main.main(main.java:37)
Caused by: java.io.FileNotFoundException: The file C:\development\cvstree\clojurescript\src\cljs\cljs%5ccore.cljs does not exist.
at cljs.compiler$compile_file.invoke(compiler.clj:1135)
at cljs.closure$compile_file.invoke(closure.clj:224)
at cljs.closure$eval1132$fn__1133.invoke(closure.clj:266)
at cljs.closure$eval1068$fn_1069$G1059_1076.invoke(closure.clj:187)
at cljs.closure$eval1119$fn__1120.invoke(closure.clj:280)
at cljs.closure$eval1068$fn_1069$G1059_1076.invoke(closure.clj:187)
at cljs.closure$eval1127$fn__1128.invoke(closure.clj:271)
at cljs.closure$eval1068$fn_1069$G1059_1076.invoke(closure.clj:187)
at cljs.closure$get_compiled_cljs.invoke(closure.clj:421)
at cljs.closure$cljs_dependencies.invoke(closure.clj:451)
at cljs.closure$add_dependencies.doInvoke(closure.clj:473)
at clojure.lang.RestFn.applyTo(RestFn.java:139)
at clojure.core$apply.invoke(core.clj:602)
at cljs.closure$build.invoke(closure.clj:712)
at user$eval1266.invoke(cljsc.clj:21)
at clojure.lang.Compiler.eval(Compiler.java:6406)
... 10 more

One bit I'm not sure of is whether :use-only-custom-externs on nodejs should include "cljs/nodejs_externs.js". In the current patch, it doesn't. You would have to specify it manually.

:use-only-custom-externs is also untested.

I've attached a small test.external.js defines external.fn(str) (which is just toUpperCase).test.cljs defines test.main which calls external.fn.test.html sources external.js and test.js (the advanced-compiled test.cljs) and calls external.fn directly and then via test.main.externs.js gives an empty definition of external.fn.

Please test your code with those changes to ensure that this works for you or to discover if more work needs to be done. If you run into problems, comment on that issue or on the Clojure dev mailing list.

Note that extend-protocol in Clojure expects the foo.bar/IQuux form (or IQuux in same namespace / after :use) and refuses to work with foo.bar.Quux. That's because in contrast to types and records, protocols actually expose a Var. The dotted name refers to the interface backing the protocol and seems to be an implementation detail.

So, I think the current behaviour is in line with how things are in Clojure.

Seqing on obj-maps moves in key insertion order, which can lead to different hash values for maps with equal key/value pairs.
This issue is related to CLJS-118, and attached patch fixes it in the same way (i.e., seq in sorted key order).

ClojureScript is currently using 20120710-r2029, which is several versions out of date. The most recent zip version available was released in December 2012 and it seems there won't be any more of that form as Google Closure has migrated to git and is pushing versions to maven central using a new version scheme (https://code.google.com/p/closure-compiler/wiki/Maven). The latest release is v20130603.

I think ClojureScript needs to switch to using the maven central releases, and it would be good to have a process/plan in place for getting more regular library updates.

I think I've misunderstood the difference between the Closure compiler and the Closure libraries. The compiler is being regularly release to central (as you are obviously aware - it's used in the build process) but the libraries don't seem to be being released at all since the switch to git. I can't find any references to a release plan for the closure libraries: this seems like something ClosureScript is going to need to deal with. There are several features that have been added to the libraries in the last year that would be 'nice to have'.

There's no process for staying up to date with closure-library beyond having the periodically bundle it and release them ourselves. More than happy to bump provided that someone tests that the latest versions of closure-libary don't break anything.

The use of coercive-not=, and coercive-not is getting a bit ugly. We could avoid this by checking for not invokes. If the argument is a bool-expr we can emit "!" directly on the argument. This would make core.cljs considerably more idiomatic.

I'm trying to update a port of hiccup [1] to the latest
ClojureScript version. During HTML compilation I call at some
point eval (in a Clojure macro) on a datastructure and get a
java.lang.ClassCastException. This used to work in ClojureScript
versions before 0.0-1211.

I traced it down to the following example. This works in
ClojureScript 0.0-1011:

java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to clojure.lang.Namespace, compiling:(NO_SOURCE_PATH:1)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6462)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.eval(Compiler.java:6508)
at clojure.lang.Compiler.eval(Compiler.java:6477)
at clojure.core$eval.invoke(core.clj:2797)
at hiccup.core$eval_test.invoke(core.clj:20)
at clojure.lang.AFn.applyToHelper(AFn.java:167)
at clojure.lang.AFn.applyTo(AFn.java:151)
at clojure.core$apply.invoke(core.clj:605)
at cljs.compiler$macroexpand_1.invoke(compiler.clj:1351)
at cljs.compiler$analyze_seq.invoke(compiler.clj:1368)
at cljs.compiler$analyze.invoke(compiler.clj:1425)
at cljs.compiler$analyze.invoke(compiler.clj:1418)
at cljs.repl$evaluate_form.invoke(repl.clj:64)
at cljs.repl$eval_and_print.invoke(repl.clj:124)
at cljs.repl$repl.doInvoke(repl.clj:173)
at clojure.lang.RestFn.invoke(RestFn.java:410)
at cljsbuild.repl.listen$run_repl_listen.invoke(listen.clj:10)
at cljsbuild.repl.listen$run_repl_launch.invoke(listen.clj:31)
at user$eval2490.invoke(NO_SOURCE_FILE:1)
at clojure.lang.Compiler.eval(Compiler.java:6511)
at clojure.lang.Compiler.eval(Compiler.java:6500)
at clojure.lang.Compiler.eval(Compiler.java:6501)
at clojure.lang.Compiler.eval(Compiler.java:6477)
at clojure.core$eval.invoke(core.clj:2797)
at clojure.main$eval_opt.invoke(main.clj:297)
at clojure.main$initialize.invoke(main.clj:316)
at clojure.main$null_opt.invoke(main.clj:349)
at clojure.main$main.doInvoke(main.clj:427)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:419)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.main.main(main.java:37)
Caused by: java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to clojure.lang.Namespace
at clojure.lang.Compiler.currentNS(Compiler.java:6864)
at clojure.lang.Compiler.lookupVar(Compiler.java:6826)
at clojure.lang.Compiler.lookupVar(Compiler.java:6847)
at clojure.lang.Compiler.isInline(Compiler.java:6323)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6448)
... 33 more
java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to clojure.lang.Namespace, compiling:(NO_SOURCE_PATH:1)

Does anyone have an idea what has changed between those versions?
Could this be a bug?

The lookup function in PersistentTreeSet behaves differently in clojurescript than in clojure when a custom comparison function is used. If two elements are evaluated as equal, one of then is in the set and we do a lookup on the other, clojure returns the element in the set, whereas clojurescript returns the lookup element.

I went looking for exactly this feature two days ago, as part of investigating ClojureScript as part of the "component model" for our single page application. It uses the "Asynchronous Module Definition" pattern to isolate code, as defined here: https://github.com/amdjs/amdjs-api/wiki/AMD

While it probably isn't the only headache, being able to easily produce an appropriate function definition around the ClojureScript code would make it much easier to get started here.

Unfortunately I can't get the patch won't apply, even when I update to the revision you mention (ba94ed7). I'm rusty with git, so maybe I'm just doing something wrong. However, when I manually made the changes, my problem went away.

I think the patch needs further work. doseq maps to loop/recur which is an efficient looping construct. By just wrapping the body of the loop in a function to preserve bindings is inefficient for many use cases. Better would be to analyze the body of loop to see if the bindings are involved in closures and only then wrap the body of the loop in a function.

This function works properly when passed a keyword or symbol. The current implementation is incorrect when passing something other than a keyword or symbol. It is weird that the (if) function allows three arguments without complaining.

In Clojure an exception is thrown if (namespace) is passed something other than a keyword or symbol. I think we should do the same here. I'm not sure why the thrown exception was commented out and nil is returned.

All supported protocols have been implemented. The main piece that is missing is defaulting __meta and __extmap to nil when the record ctor is called without those params (currently you get a warning when calling without those params). This breaks assoc and conj, and causes meta to return nothing, as opposed to nil.

I think I've been working on this in a vacuum too long, and could really use a review of the work I've done. I've included a patch to make it easier for whomever takes a look. I had a few questions about my approach:

Is creating a new emit method for :defrecord* in the compiler kosher, or should I have worked this into the :deftype* method?

The quoting of the impls for extend-type in the emit-defrecord fn seem awkward to me. Feels like I'm doing something wrong.

I used gclosure @param jsdocs to avoid warnings from the 2 different ctor "arities", is that okay?

I don't know if this is happening on Linux systems, but the reports appear to be from Windows and OS X users. I'm running OS X 10.6.8 myself. Probably not relevant, but I figured I'd throw that out there anyway.

Internet Explorer treats \uFDD0 up to \uFDEF as equal. e.g. '(= \uFDD0 \uFDD1)' returns true.
Therefore e.g. '(symbol? :whatever)', '(symbol? (keyword "whatever"))' and so on return true which obviously shouldn't happen.
Further on read-string does not correctly unmarshal keywords because of that issue.
Using pr-str on that unicode range returns ":" for all codes.
All other non IE Browsers I've tested (Firefox, Chrome/Chromium, Opera, iOS BRowser, etc...) do not expose this problem and work as expected.

My proposed solution, replacing \uFDD0 and \uFDD1 with \uFDF0 and \uFDF1 respectively works on all tested Browsers (Firefox9, Chromium17, iPAD/iOS-5.0.1, Epiphany AND IE7 + IE8).
I used this bash command line inside the "src" folder to do the replacement (after checking that the replacements will be appropriate):
for src in $(grep -R -i -l 'fdd' .); do sed -i -e 's/FDD/FDF/g' $src; done

I just did another test where I compared the two values using Javascript directly and one using compiled Clojurescript with only an alert outputting the result of the same test. These two tests produced the correct results (as David Powell observed) even in the IEs. So it seems that there happens something in the context of my application, maybe it's caused by one of the included js libs (I'm also using jquery, jquery.mobile and ckeditor... though I've already included these in the testing environment, uhmmm...). I will have to investigate that further. I'll post another comment when I've found out something.

Intermediate result of my investigations:
It only happens when I turn Clojurescript optimizations OFF!!!
As soon as I activate any compiler optimizations even if it's only set to "simple", the problem will not manifest itself on IE.
It also seems not to depend on any of my included third party libs.

I attached a test that illustrates the described problem with IE (at least on IE7 and IE8, I still couldn't test more recent IE versions).
The result of the equations should always be "false" which isn't the case with non optimized, compiled ClojureScript within the unicode range \uFDD0 up to \uFDEF while it works e.g. for \uFDF0 and \uFDF1.
"unicodetest.html" uses the the non optimized code which exposes the bug; it was compiled using: cljsc unicodetest.cljs > unicodetest.js
"unicodetest_simpleopt.html" uses the "simple" optimized code which does not expose the bug; it was compiled using: cljsc unicodetest.cljs '{:output-dir "simpleopt_out" :optimizations :simple}' > unicodetest_simpleopt.js
Using advanced optimization also produces correct results even on IE.

The problem was observed on:
Windows XP (SP 2)
using IE7 and IE8
(it did not occur on any other non-IE browser I've tested: Firefox, Chromium, iPad Browser, Epiphany and Opera regardless of the OS they were running on)

Looking at the compiled code I see that in the version without optimizations the Unicode characters are converted from the UTF-16 \u notation to their UTF-8 byte values (3 bytes); look here: http://www.fileformat.info/info/unicode/char/fdd0/index.htm
... while in the optimized version the \u notation stays intact.
That of course does not explain why IE treats strings containing the byte sequences 0xEFB790 up to 0xEFB7AF as equal but it explains why there is a difference between the optimized and non optimized compiler output and also why it works in IE when coding the equality test directly in Javascript using the \u notation.

I've just tested your new fix which prevents the compiler from converting unicode literals to their respective UTF-8 representations but now I encounter the bug when using read-string as described here: CLJS-133
Again only on IE and only when not using any optimization during compilation.

I have attached a new patch which should fix all problems related to this issue (including CLJS-133 which is related) so far (works for me). It includes David's partial fix so only this one is required.
However, to prevent possible related future issues it should probably be considered to escape every non-ascii character in emitted strings and characters because IE obviously HAS severe problems with handling unescaped unicode.

This seems less ideal than just solving this once and for all. I'm assuming many people will want to emit unicode chars and want them to emit properly and work under development in IE. When we emit strings we should rebuild the string replacing unicode chars. This should be done efficiently as possible.

I have just attached my first stab at a general non-ascii escape patch for emit-constant. It should already be pretty efficient. I basically took the core from Stuart Sierra's clojure/data.json write-json-string function (to not having to reinvent the wheel), optimized it a bit (hopefully) and integrated it properly into compiler.cljs.

Just to nip questions regarding the emittance of regular expressions in the bud, I've just tested this and it behaves correctly without "manual" intervention. More precisely, escaped unicode in regular expressions does apparently not get converted to utf-8 (or whatever character set) when applying ".toString" or "str" to them.

Records don't currently cache their hash values, nor do they use the map-specific hashing algorithm (in contrast to Clojure's records which share their hashing algo with Clojure's maps). The patch to be attached in a moment implements hash caching and wires in hash-imap.

When using a closure compiler generated goog/deps.clj, goog-dependencies* fails to parse anything from the file. This is due to the compiler writing the dependencies using double quotes for the first argument to addDepenendency.

The attached patch modifies the regex used to match either single or double quotes.

In general, we should not complicate the compiler with additional options when functionality can be provided by external tools.

I think this feature can be provided by external tools. The compiler will only automatically pull in things that are actually dependencies of the sources that we provide to the compiler. External tools should provide ways to limit what is handed to the compiler.

I would first attempt to modify lein-cljsbuild to do what you want.

When using the compiler directly, you can provide your own implementation of Compilable which, when given a directory, will filter out sources based on some criteria you provide. In my projects I have custom implementations of Compilable that do just this. You should be able to do the same thing in lein-cljsbuild.

Thanks for the advice Brenton. I'll try to understand from the maintainer of `lein-cljsbuild` where to start from. I agree with you about keeping the compiler clean from options that can be implemented by the tools. But I'm no so sure that patching lein-cljsbuild we'll be as easy as adding `:exclude` option to the compiler.

I didn't know that cljs.closure/build was flexible enough to do this already. I always thought that it needed to be extended so that a vector of files could be passed in, or something, but it sounds like the Compilable approach should work just fine.

I will happily accept a patch for this. One thing to keep in mind, though, is that the :exclude entry should not be in the :compiler map if lein-cljsbuild is handling it. The :compiler map is passed straight through as options to cljs.closure/build. So, the :exclude entry should be a neighbor of the :compiler entry.

Hi all,
we all agree with Brenton and yes, the :exclude option has to be at the same hierarchical level of :source-path. I'll verify if we can also extend :source-path from taking a String to taking a vector of String, in such a way that the user could ask to compile more cljs src-dir.

Hi all,
just a small add on to the previous comment. I don't think we're going to update cljsc/cljsc.clj, which I consider a kind of tool, much more fragile and limited than cljsbuild plugin, to interoperate with CLJS compiler.

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.

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.

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.

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.

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!

Here's a first cut at a patch. Certain vector ops are indeed much faster than on master, but most of the other benchmarks seem to get slower (unfortunately first/rest/next on all sorts of inputs seem to suffer particularly badly).

The commit message includes a summary of the approach and some issues (with String/Number/Boolean and truth_) worked around in the patch. NB. js/String actually is extended in core.cljs.

The satisfies? part of this patch is not going to work. It emits a function call as well as a call to truth_. type inference won't help you here since cond expands into if's which return null in the false branch defeating boolean inference. As far as the function call, I suspect the emitting code is complex enough that Closure won't optimize it. satisfies? at this point is finely crafted to emit the most optimal code both for Closure and JS engines I strongly suggest against any attempts at refactoring. Simply make the most minimal change to make it work.

There is no call to truth_ that I can see and still only a single function call in the test part of an 'if (and of about the same complexity as the one generated for the 'or on master). With the patch #(satisfies? IVector []) compiles to the following JavaScript (notice the outer function is the one introduced by #(...); that just makes it convenient to print from Rhino and I'm copying and pasting it here with no change so as not to introduce any mistakes):

I tried making small modifications to the main Boolean expression (test bit mask, fall back to flag) w/o introducing any conds, but these all resulted in truth_ appearing in the compiled output. I'm not making any claims as to whether I've tried all possible small modifications, but I've attempted a fair number with and without bool-expr hints.

As for Closure's optimizations – I'll have to check that. As mentioned above though, there is still only one function generated in the test part of an 'if expression (in this version for an 'and form, since the refactoring into a 'cond gets rid of the 'or form), is about the same size as the one generated for the 'or previously and occurs in a test which resides fully inside a single curly-delimited block.

Not that I think this patch should be applied – quite the contrary, it makes things slower for the most part; my point is just that there's no truth_ to lay the blame on and I'm not sure how to arrive at the same semantics in the same amount of generated JS code while still attaching the masks to the instances for inline protocol impls.

This does give me an idea for another possible approach, though... I'll be back with a different patch if it works.

NB. fast path protocols are implemented both inline and with a standalone extend-type; however, I think we can skip the flag only in the case where the impl occurs inline in a deftype/defrecord (this includes the implicit impls provided by defrecord) and emit a flag otherwise. I'll write a patch along these lines.

When macro uses current namespace for its expansion the current namespace is bound to 'user' one rather to whatever ns points to. Even though fixing it, clojurescript compiler fails due to assertion '(assert (not (namespace sym)) "Can't def ns-qualified name")' in compiler.clj:663

In Clojure records are not considered to be equal if they have different types, but the record type is not included in the hashcode - this will lead to excessive hash collisions if a map contains heterogenous record keys that don't have distinct data.

ClojureScript is consistent with Clojure in this area, but I think it should be changed.
]

Thanks get is just extremely common and likely to appear in expression cases - where the CLJS compiler will emit a function. If these get too nested GClosure won't handle them and it's a pretty big perf hit.

The included benchmark seems to demonstrate a substantial reduction in {apply} overhead on V8 for a function with 10 positional arguments preceding the rest arg and a less substantial reduction for a function with 2 positional arguments, as one would expect. The other benchmark entries for {apply} remain in the same ballpark ({list} actually produces the same compilation output with or without the patch; {+} is changed, but only slightly).

The problem was that the metadata that tells the compiler what fields are available was being conveyed through the argument list. This meta-data was being destroyed during the expansion of fn when the argument list required destructuring.

I've pulled in and altered the Clojure fn macro so that it conveys the meta-data even if it has to destructure.

I wonder if adorning the arguments was the best idea. Why not put the fields on the (fn ...) form itself and pull that out in parse 'fn*? Then fields can be added to the environment which makes more sense to me anyhow and lot of duplicated computation can be avoided.

Bash completion shows `bin/cljsc.bat` as an executable on UNIX systems
making it very easy to accidentally execute it. However, since
Windows command scripts do not work with UNIX shells, it is not
advisable to allow executable permissions to Windows batch scripts
as accidentally executing them may cause undefined behavior on such
systems.

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.

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.

inode-find implementations are under Object and thus are not subject to various optimizations - in particular multi-arity dispatch. local functions are create and returned - this is a big performance hit on all engines.

The attached patch wires in a new PHM method, inode-lookup, which always takes a not-found argument and returns just the value found under the given key, rather than a map entry / vector of [key val].

I also experimented with removing one arity of inode-find (passing in nil for not-found instead of calling the smaller arity overload); that resulted in a much smaller speedup.

This patch leaves inode-find implementations intact, as mentioned in the commit message. They can be removed by a subsequent patch or used in what I think would be a more Clojure-like find implementation.

I should say, given the present experience with inode-lookup I think I could write a much faster impl of inode-find anyway if that seems worthwhile. What do you think?

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:

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

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?

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.

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?

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.

We ned the abilty to supply custom and default tag parsers when using cljs.reader/read-string (found in src/cljs/cljs/reader.cljs) in running ClojureScript applications.

ns prefixed tags

As per the edn specification "user tags must contain a prefix component". Currently, because (name tag) is being called, the ns prefix component is dropped when looking up the tag in *tag-table*

default reader fn

Currently, there is no support for supplying a default reader fn (akin to *default-data-reader-fn* in Clojure proper).

2 issues rolled into one?

I know this may be bad practice rolling 2 issues into one, but I do not know how to formulate the separate patches that affect the same lines of code and have the apply correctly regardless of order. If I need to correct this, please advise.

Already reported?

I know this issue was already raised in CLJS-335, but from my experience with making these modifications for a running ClojureScript app, I did not come across the fns in the files mentioned in that task. I assume this is because that had more to do with compiling cljs code, but please correct me if I'm wrong.

Along those lines, is this patch not acceptable if it doesn't fix both places at once?

Approach & Naming

I followed the approach that was already in place for tag parsers, supplying register and deregister helper fns. I was a bit torn on naming because in Clojure *default-data-reader-fn* is used, but in the existing ClojureScript code they are called tag-parser. Should the "reader" vs "parser" nomenclature be unified between Clojure and ClojureScript, or are they named differently because they are fundamentally different in some way that I am missing?

I don't think we can have a unified solution without a reader that is shared both at compile time and run time and I think the benefits today of run time support trumps the possible future convenience of compile time support. I don't think there is any good reason for the difference in naming - I'm inclined to keep things in line with Clojure as much as possible.

Happy to move forward with this one unless there are objections from others.

My problem in CLJS-335 was that you couldn't compile cljs code with embedded user-defined tagged literals.

The code in this ticket is also great. But it would be even better if there was a generic function for reading tagged literals, known or unknown, that had the exact same argument structure as the the tag reader functions.

My solution for CLJS-335 is to read the TL into code that resolves and invokes the tag reader function on the CLJS side.

However it will fail for unknown tags, and will not fall back on the default reader functionality defined in the solution for this ticket. It would be trivial to fix this, if there was a single function that my solution can use that handled both known and unknown tags.

Otherwise, my solution can be extended with more code, to first check for a specific reader, and then fall back on the default reader.

RRB trees can support chunked seqs without a problem, but they need their own implementation, as the one in cljs.core uses PersistentVector's array-for. Since chunked-seq? is currently hard-wired to check for the built-in chunked seq types as an optimization for for and doseq, it returns false for such custom chunked seqs.

This can be solved by using satisfies? with false passed in as the extra check-native argument. Happily the performance difference is negligible.

We actually have a lot of information around deftype / defrecord / extend-type. We could use this information to emit direct dispatches to protocol implementations and avoid going through protocol fns.

We needs to store all the protocols a type implements. We then should add type meta to the this argument of the protocol fn implementations. This should actually result in a reasonable performance boost across the board.

This patch passes all currently existing tests and doesn't break the reader through the use of transient vectors in cljs.core.PersistentVector/fromArray, but includes no new tests for parts of the transient functionality other than simple conj!-ing followed by persistent!; I'll write those sometime (very) soon, but since unfortunately I need to take a break for now, here's the patch for anybody interested in taking it for a spin.

It appears that this works correctly on current master. In fact, it worked correctly on r1576 – but not on r1552. Also, splitting the :let into two ("adjacent") :let's fixes this, whereas moving both the y and f bindings into a single regular let inside the doseq does not. That means it's probably an instance of CLJS-442 (fns defined in local init exprs not closing over earlier locals), fixed in https://github.com/clojure/clojurescript/commit/84e9488f49bcfea4b4037a562f8f797c7ddd79f0 (included in releases since r1576).

While writing this patch I thought it would be nice also to provide support for :refer – since I'm already working on :require... The patch thus ended up being a bit of an overhaul of :require and :use handling. Here's the commit message:

CLJS-272: support :refer and skipping :as in :require
The :as alias part of a :require spec can now be omitted.
Alternatively, a symbol naming the namespace to be required without an
alias can be specified without wrapping it in a vector.
Additionally, :require accepts a :refer option with :use-like effect.
(ns foo.core
(:require
;; bar-fn will be available unqualified
[lib.bar :as bar :refer [bar-fn]]
;; :as lib.baz is implicit
[lib.baz]
;; likewise
lib.quux))
Either, both or none of :as, :refer may be specified in each :require
spec.
:require-macros supports the same options.
:use / :use-macros is now supported by rewriting the specs as :require
/ :require-macros specs with :refer.
:require / :require-macros now produce problem-specific error
messages which include the offending spec.
Tests are provided for :refer and the new handling of :use.

I wonder if parse-require-spec should be a factored out of parse 'ns (something appropriate would need to be done about error-msg – not sure which possibility is best).

The contract of goog.string.quote is to produce a "valid JavaScript string", which can end up being quite different from what is expected by e.g. the Clojure reader and other edn-compatible readers. This has been discussed in a variety of threads/issues:

The most significant issue is that characters > 127 and < 256 are outputted with an \x.. escape sequence, which no other reader supports. Further, other Unicode characters are always escaped using the \u.... notation; this is not a functional difference, but can be a stumbling block, e.g. when viewing data that had been pr-str'd that contains high codepoint characters.

The most straightforward fix is to take control of printed string escaping to ensure the results conform first to the expectations of existing readers (primarily Clojure's and ClojureScript's), and to the formal [edn specification](https://github.com/edn-format/edn/) as that matures.

We need benchmarks for large data structures, e.g. large PVs with shifts greater than 5 (a shift of 20 would perhaps be problematic due to the necessary size of the vector, so probably 10 or 15).

My first cut at a patch (to be attached in a moment) provides benchmarks for building a large vector without using transients (to get an idea of conj perf), conjing onto a large vector in a situation where this involves a push-tail operation, associng in non-tail position in a large vector and popping a large vector where this involves a pop-tail op.

The push-tail / pop-tail benchmarks, as written here, are dependent on the exact position of the tail inside the vector; as an alternative, we could benchmark conjing / popping 32 times (max tail length is probably going to be pretty stable). Thoughts?

The problem is that new always fails if given a complex expression (= not a symbol) as the ctor argument. This used not to be the case in earlier ClojureScript releases; the reason it fails now is due to number-of-fields validation not guarding against the non-symbol-ctor-argument case.

I see two possible solutions: (1) restore the old behaviour while preserving the number-of-fields validation for symbolic ctor args; (2) mandate symbolic-ctor-only usage of new and throw an error with a useful message in parse 'new. I'm leaning towards (1), so the forthcoming patch takes this approach. Either approach is easy enough to implement. (I think the way parse 'new is written suggests that allowing complex ctor expressions was the original intention; I could be wrong about this though.)

@David Nolen – Yes. I'm quite fond of prefix lists myself, but seeing how they're not supported for other libspec types... (Whenever the time is ripe for that to change, I'll be happy to provide a patch. )

@Tom Jack – Thanks, I'll look into it!

I guess the precise form of any new patch will need to depend on the resolution to CLJS-272.

I wouldn't expect it to, since types & records are fundamentally different to GClosure classes in that they don't constitute separate "dependencies". For this reason e.g. (:require clojure.core.reducers.Cat) won't work, whereas (:require goog.string.StringBuffer) works fine.

It would be straightforward to have, say, (:import foo.bar/Quux) pull in Quux from the foo.bar ns. Of course that's already possible with :use / :require :refer, plus this kind of syntax would constitute a departure from the current state of affairs in Clojure; but if it seems worthwhile, I'll be happy to adjust the patch / provide a new one to support this.

Figuring out automagically whether a GClosure-style dependency or a native type is needed would be somewhat more involved, I think. I'll need to think about what exactly might be required.

Incidentally, I wonder if there's any chance of Clojure supporting deftype/defrecord in use/require... I'll think about this some more, see if I can find anything on the topic in clojure-dev archives and possibly bring it up and/or produce a patch.

In any event, I'll be attaching a new patch ready for application on current master. It does seem to address Tom's issue at least.

Some random thoughts re: how much of a problem treating records/types in :import otherwise than as in Clojure would be: we have {->Foo} and {map->Foo} for records; types are close to the metal, particularly perf-conscious users will already expect to be exposed to some platform details etc.

After pondering this some more. I wonder if :import should simply be restricted to the Google namespaces? I don't really see another case for it. Related - a warning from :require about specs that look like the following:

(:require [goog.foo.Bar :as Bar])

Should be pretty simple to detect. It could suggest the correct :import.

@Tom: The test is indeed flawed since StringBuffer is being required by core, thanks for pointing this out! I need to use some Closure "class" which is available in the version of Closure library used by ClojureScript, but not required elsewhere.

Not sure what the story is re: g.a.Deferred. I understand you're seeing that behaviour in a project where bringing it in with a :require does work? At any rate, I'll be preparing a new version of the patch soon; I'll make double sure it works for me on some non-trivial case.

@David: I think convenience of working with GClosure classes is the no. 1 reason :import might be useful, so thus restricting it or introducing a restricted version first and only expanding it to cover types/records later seems ok to me. It occurred to me that types/records could be goog.provide'd in the JS output unless, say, goog.provide calls must occur early in the file, which I don't know to be or not to be the case – will check that later; if this turns out to work fine, we might as well support the full Clojure-like behaviour.

Re: warning, do you consider that :require form strictly incorrect? Is any :require spec of the form [a.b.Foo :as Foo] always incorrect (as opposed to not the most elegant way to go about it etc.)? Honest questions both.

This patch should actually work. The test now uses goog.math.Long and tests both constructor and "static method" calls.

Support for "static methods" is achieved by implicitly :require'ing the namespace/class :as the final segment of its name. Resolution of the ctor symbol is still handled by a separate :imports entry in the namespace map.

I haven't looked into implementing the type/record idea I mentioned before – that's coming next (if it works). Not sure if it should be a separate commit, an ammendment to the current one or a separate ticket.

Re: warnings, I think {:import foo.Bar} should cause foo.Bar to be :require'd anyway. If :require provided an option to :import – (:require [foo.bar.Quux :as Quux :import true]), say – the situation would be entirely analogous to that with :use / :refer (no strong opinion on whether that would be a good idea at this time). Plus there's nothing broken in (:require [foo.Bar :as Bar]); what is broken is going on to expect that (Bar. ...) will then be a ctor call. So maybe the warning should occur when (Bar. ...) is encountered in a namespace with this sort of :require spec? This sounds like a separate ticket though.

Here's a patch which causes emit :deftype*/:defrecord* to emit goog.provide when compiling a file. It does indeed allow :import to bring in cljs types/records.

Some notes:

1. This patch goes out of its way to prevent duplicate goog.provides, because (1) GClosure doesn't like them, (2) the test suite actually specifies (defrecord A ...) twice. I guess the sensible thing to do may be to fix the test to use unique record names and always emit goog.provide in files.