<< Back to previous view

[CLJS-799] Having a namespace end with ".cljs" produces wrong source map Created: 16/Apr/14  Updated: 16/Apr/14

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

Type: Defect Priority: Minor
Reporter: Sven Richter Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: maps, namespace, source
Environment:

Windows 7
JDK 1.7
CLJS version: 0.0-2138 and 0.0-2156 (probably hits other versions too, but I only tested these two)



 Description   

When an clojurescript namespaces ends with ".cljs" I cannot see the source file in google chrome.
Repro steps:

1. Create a new luminus project with: lein new luminus cljsbug +cljs +http-kit
2. Change the project.clj cljsbuild -> compiler setting to:
:compiler
{:output-to "resources/public/js/site.js"
:output-dir "resources/public/js/out"
:optimizations :none
:source-map true
}
3. Change cljsexample.html file to:
<script type="text/javascript" src="js/out/goog/base.js"></script>
<script type="text/javascript" src="servlet-context/js/site.js"></script>
<script type="text/javascript">goog.require("cljsbug.main");</script>

4. Now start the server with "lein run -dev" and "lein cljsbuild auto"
5. Open localhost:3000/cljsexample
6. Check for the source file in google chrome

It should be there now and correct.
Now to reproduce the problem do this:

7. Change the namespace of the main.cljs file to: ns cljsbug.main.cljs
8. Change the cljsexample.html goog.require line to: <script type="text/javascript">goog.require("cljsbug.main.cljs");</script>

9. Restart the cljsbuild with: lein do cljsbuild clean, cljsbuild auto
10. Reload the /cljsexample page in google chrome and the source mapping wont be there anymore.



 Comments   
Comment by Sven Richter [ 16/Apr/14 2:38 PM ]

Just to clear things up. Steps 1 to 6 are not needed to reproduce the problem. It is sufficient to go through steps 7 to 10 on any project that uses a similar cljsbuild setting.
It is important that optimizations are set to :none.

Short repro version.

1. Have cljs project with the following settings:
:compiler
{:output-to "resources/public/js/site.js"
:output-dir "resources/public/js/out"
:optimizations :none
:source-map true
}

2. Change any cljs file namespace and add ".cljs" to the namespace.
3. Have the new namespace required in the html file by google like this: goog.require("cljsbug.main.cljs")

4. Open a page in the browser and see that the source maps are missing.





[CLJS-798] regression: upstream dependencies are no longer honored Created: 16/Apr/14  Updated: 16/Apr/14

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File restore-upstream-deps.diff    

 Description   

The latest cljs no longer honors upstream deps. This is a regression introduced by https://github.com/clojure/clojurescript/commit/e16a3da. The js-dependency-index is now calculated once, before the upstream deps are loaded. One potential fix is to regenerate the index after loading the upstream deps, but I'm not familiar enough with the compiler to know if that's a bad idea. I'll attach a patch with that change for review.






[CLJS-797] Nested 'for' loops (3+ deep) in Android Chrome cause "Uncaught RangeError: Maximum call stack size exceeded" Created: 16/Apr/14  Updated: 16/Apr/14

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

Type: Defect Priority: Major
Reporter: John M. Newman III Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Android 4.3, Chrome 34, ClojureScript 2202



 Description   

(do (println "for loop test: 2 deep")
(for [a [[1]]]
(for [b a]
b)))
;; this compiles and runs fine in the browser

(do (println "for loop test: 3 deep")
(doall
(for [a [[[1]]]]
(for [b a]
(for [c b]
c)))))
;; this fails while the page loads, with the error: Uncaught RangeError: Maximum call stack size exceeded

The above works fine in a desktop browser. For some reason the error condition only happens on the Android Chrome browser.

Let me know if any further details are required.






[CLJS-796] (get [42] nil) => 42 Created: 11/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug
Environment:

r2156



 Comments   
Comment by Francis Avila [ 12/Apr/14 9:23 PM ]

This is a duplicate of CLJS-728 and is fixed by this commit, which is included in r2197 and above.





[CLJS-795] Enhance multimethod performance Created: 11/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Jonas Enlund Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJS-795.diff    

 Description   

Multimethods can be made more performant by implementing (the long list of) IFn methods. There is one benchmark (the last one) covering multimethods in script/benchmark and I saw a drop from ~250msecs to ~25msecs with the suggested changes.

If it seems to be too much repetition I'm sure its possible to throw some macros at the implementation to make most of the boilerplate disappear.



 Comments   
Comment by David Nolen [ 14/Apr/14 4:29 PM ]

fixed https://github.com/clojure/clojurescript/commit/8af3ae824ff8480806c6be847dba1765a8fd94f9





[CLJS-794] RegExp flags are being dropped by `string/replace` Created: 09/Apr/14  Updated: 09/Apr/14

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

Type: Defect Priority: Minor
Reporter: Peter Taoussanis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

`clojure.string/replace` accepts either a string or pattern argument to match against.

For pattern arguments, the current implementation discards the original RegExp and creates a new one:
`(.replace s (js/RegExp. (.-source match) "g") replacement)`

This is killing any flags on the original pattern (case insensitivity, for example). As a result, things like `(str/replace "Foo" #"(?i)foo" "bar")` currently fail. The result is "Foo", it should be "bar".

Can I submit a patch that'll check for and preserve other (i/m/y) flags?

Thanks






[CLJS-793] memoize doesn't correctly cache non-truthy return values Created: 08/Apr/14  Updated: 10/Apr/14

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

Type: Defect Priority: Minor
Reporter: Peter Taoussanis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-793.patch     Text File CLJS-793.patch     Text File CLJS-793.patch     Text File patch_commit_2ba4dac94918.patch    
Patch: Code

 Description   

ClojureScript's memoize fn currently uses `(get @mem args)` to check for the existence of a cache entry. This prevents falsey values from being cached correctly.

A direct copy of Clojure's `memoize` code fixes this, patch attached.

This is the first issue+patch I've submitted, so please double check for mistakes - thanks.



 Comments   
Comment by David Nolen [ 08/Apr/14 9:23 AM ]

The patch is not properly formatted - please follow these instructions http://github.com/clojure/clojurescript/wiki/Patches

Comment by Peter Taoussanis [ 08/Apr/14 9:41 AM ]

Updated patch formatting, thanks!

Comment by David Nolen [ 08/Apr/14 10:55 AM ]

Thanks, I've updated the instructions to be clearer on what the commit message should look like. We've been inconsistent in the past and I would like for people to follow a basic guideline. Thanks.

Comment by Peter Taoussanis [ 08/Apr/14 11:01 AM ]

No problem, thanks! Just updated to fit the new spec exactly.

Comment by David Nolen [ 08/Apr/14 7:37 PM ]

Looking at this more closely I would prefer that this be done with a closed over sentinel value via (js-obj), and to perform an identical? check on (get @mem args sentinel) to see if it matches the sentinel.

Comment by Peter Taoussanis [ 09/Apr/14 12:18 AM ]

Sure, updated the patch. Please note that I used the global `lookup-sentinel` instead of closing over a new one. Let me know if there was some reason you wanted a unique sentinel & I'll update!

Cheers

Comment by David Nolen [ 10/Apr/14 12:23 PM ]

Looks good thanks!





[CLJS-792] cljs.core.reducers/reduce does not support cljs.core/PersistentArrayMap Created: 07/Apr/14  Updated: 07/Apr/14

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

Type: Defect Priority: Major
Reporter: Shahar Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
((require '[clojure.core.reducers :as r])
(into [] (r/map identity {}))
;; Clojure: []
;; ClojureScript: Error: No protocol method IReduce.-reduce defined for type cljs.core/PersistentArrayMap: {}





[CLJS-791] Cross-target ClojureScript to the DartVM. Created: 06/Apr/14  Updated: 08/Apr/14  Resolved: 08/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Yesudeep Mangalapilly Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: cross-target, dart, enhancement, vm
Environment:

Any.



 Description   

ClojureScript is a better language than many others, I figure.
The DartVM is picking up speed. Can CLJS be retargetted to generate
Dart code as well?



 Comments   
Comment by David Nolen [ 08/Apr/14 9:22 AM ]

Queries like this are best directed toward the mailing list before opening tickets. Thanks.





[CLJS-790] Advanced compilation broken with latest closure libra Created: 04/Apr/14  Updated: 04/Apr/14

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

Type: Defect Priority: Critical
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

cljs >= 2197



 Description   

CLJS releases >= 2197 are depending on the most recent packaged closure lib [org.clojure/google-closure-library "0.0-20140226-71326067"].

This is bad,



 Comments   
Comment by Francis Avila [ 04/Apr/14 3:46 PM ]

ARG, submitted too soon by accident.

Rundown is this:

  1. CLJS >= 2197 depends on the latest packaged closure lib (March 17th release, "0.0-20140226-71326067").
  2. 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).
  3. 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!)
  4. Oddly, the compiler does not emit any warnings about this!
  5. Additionally, the clojurescript project's POM and project.clj specify different versions of the closure library starting at https://github.com/clojure/clojurescript/commit/523a0c5b18138c9a4d23c5104a77b65488bc28c3 The POM specifies the new lib, but the project.clj the older one.

A workaround is to declare an explicit dependency in your project to [org.clojure/google-closure-library "0.0-20130212-95c19e7f0f5f"]. If you do this everything will start working again.

Basically, cljs can't use the new closure lib until its closure compiler has an updated release. (There is some hope that this may be easier to do in the future: https://groups.google.com/d/msg/closure-compiler-discuss/tKZQ1eLUixA/3urgnli84SYJ)





[CLJS-789] Advanced compilation broken with latest closure libra Created: 04/Apr/14  Updated: 04/Apr/14

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

Type: Defect Priority: Critical
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

cljs >= 2197



 Description   

CLJS releases >= 2197 are depending on the most recent packaged closure lib [org.clojure/google-closure-library "0.0-20140226-71326067"].

This is bad,



 Comments   
Comment by Francis Avila [ 04/Apr/14 3:50 PM ]

I have no idea how this was submitted both too soon and twice! I'm very sorry for the mess. Please mark duplicate of CLJS-790 and close.





[CLJS-788] spurious namespace warnings about goog namespaces in 2197 Created: 31/Mar/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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: None


 Comments   
Comment by David Nolen [ 01/Apr/14 1:35 PM ]

fixed https://github.com/clojure/clojurescript/commit/7f93172bcfc8299493724c72e77ef5438932c1a2





[CLJS-787] cljs.reader does not read blank string as nil Created: 20/Mar/14  Updated: 20/Mar/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-786] cljs.core/into doesn't preserve metadata Created: 17/Mar/14  Updated: 19/Mar/14

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

Type: Defect Priority: Minor
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

cljs.core/into doesn't preserve metadata:

(meta (into ^{:foo :bar} [] [1 2 3])) ;; => nil


 Comments   
Comment by Chas Emerick [ 19/Mar/14 9:15 AM ]

Confirmed. Both TransientVector and TransientHashMap need to have meta fields added that are passed on when instances are made persistent!.

Comment by Kevin Marolt [ 19/Mar/14 9:33 AM ]

Not sure what's the preferred way to handle this. The transients in Clojure don't support metadata either. Instead, Clojure simply uses

(defn into
  "Returns a new coll consisting of to-coll with all of the items of
   from-coll conjoined."
  {:added "1.0"
   :static true}
  [to from]
  (if (instance? clojure.lang.IEditableCollection to)
    (with-meta (persistent! (reduce conj! (transient to) from)) (meta to))
    (reduce conj to from)))

See also [#CLJ-916] into loses metadata.





[CLJS-785] :refer-macros in conjunction with :refer not working Created: 17/Mar/14  Updated: 17/Mar/14

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

Type: Defect Priority: Minor
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The :refer-macros directive isn't working when used in conjunction with the :refer directive. Compiling a ns-form like

(ns foo
  (:require
    [bar :refer [baz] :refer-macros [quux]]))

produces the compiler error

Each of :as and :refer options may only be specified once in :require / :require-macros; offending spec: (bar :refer [baz] :refer [quux])

The problem seems to be with analzyer.cljs/desugar-ns-specs. Invoking

(desugar-ns-specs '((:require [bar :refer [baz] :refer-macros [quux]])))

returns

'((:require-macros (bar :refer [baz] :refer [quux])) (:require (bar :refer [baz])))

instead of

'((:require-macros (bar :refer [quux])) (:require (bar :refer [baz])))

Furthermore, there seems to be a typo in the local remove-sugar function on line 1094 [1]: It should probably be

(let [[l r] (split-with ...)] ...) ;; no '&' before 'r'

instead of

(let [[l & r] (split-with ...)] ...)

Otherwise, something like

(desugar-ns-specs '((:require [bar :include-macros true :as b])))

becomes

((:require-macros (bar :as b)) (:require (bar)))

instead of

((:require-macros (bar :as b)) (:require (bar :as b)))

[1] https://github.com/clojure/clojurescript/blob/78d20eebbbad17d476fdce04f2afd7489a507df7/src/clj/cljs/analyzer.clj#L1094






[CLJS-784] PersistHashMap's -conj implementation recurses infinitely if element to be conjed is not a vector. Created: 15/Mar/14  Updated: 15/Mar/14

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

Type: Defect Priority: Minor
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Happens on cljsfiddle, among other environments.



 Description   

In commit b8681e8 the implementation is

ICollection
(-conj [coll entry]
(if (vector? entry)
(-assoc coll (-nth entry 0) (-nth entry 1))
(reduce -conj coll entry)))

Thus, e.g., (-conj {} "foo") results in an infinite recursion, and a stack overflow. This causes things like (merge {} "foo") to fail for the same reason.

Not sure what the purpose of the not-vector branch could be. I can't think of a situation where it would give a useful result. Maybe it could throw a more helpful error message.






[CLJS-783] Confusing error messages when ns compilation fails due to a missing dependency Created: 11/Mar/14  Updated: 12/Mar/14

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

Type: Defect Priority: Minor
Reporter: Michael Klishin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, errormsgs, usability


 Description   

I have a namespace proj.a which requires proj.b. proj.b, in turn, relies on core.async. I did not have
core.async listed in project.clj by accident, and the resulting error message was

goog.require could not find: proj.b.

This is not incredibly helpful. I've wasted over an hour trying to understand why one ns in my project
cannot reference another one.

Expected outcome: compilation must fail instead of swallowing exceptions. If it matters, I use lein-cljsbuild 1.0.2.



 Comments   
Comment by David Nolen [ 12/Mar/14 8:36 AM ]

And which version of ClojureScript are you using?

Comment by Michael Klishin [ 12/Mar/14 8:52 AM ]

0.0-2138

Comment by Michael Klishin [ 12/Mar/14 8:53 AM ]

I strongly disagree with the severity change. Anything that can waste beginners hours of time is not a minor priority.

Comment by David Nolen [ 12/Mar/14 10:18 AM ]

That is a fairly old release of ClojureScript, can you replicate the issue with 0.0-2173? When you change your dependency please make sure to run "lein cljsbuild clean" first.





[CLJS-782] cljs.core.UUID should have a toString method Created: 10/Mar/14  Updated: 12/Mar/14  Resolved: 12/Mar/14

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

Type: Enhancement Priority: Trivial
Reporter: James Reeves Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Sometimes it's useful to turn a UUID into a string, particularly when interoperating with Javascript. Currently the toString implementation for UUIDs is quite opaque:

=> (str #uuid "f47ac10b-58cc-4372-a567-0e02b2c3d479")
"[object Object]"

Mimicking the behavior in Clojure/Java would be preferable:

=> (str #uuid "f47ac10b-58cc-4372-a567-0e02b2c3d479")
"f47ac10b-58cc-4372-a567-0e02b2c3d479"


 Comments   
Comment by David Nolen [ 12/Mar/14 8:23 AM ]

fixed https://github.com/clojure/clojurescript/commit/2870101b1a4ad4ef70ff06df3c04cda5d621b2cc





[CLJS-781] ClojureScript Browser REPL analysis regression Created: 07/Mar/14  Updated: 07/Mar/14  Resolved: 07/Mar/14

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: Declined Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 07/Mar/14 3:29 PM ]

Latest 2173 has an analysis regression that results in spurious warnings about cljs.core symbols that did not exist in 2156. I strongly suspect this is related to http://dev.clojure.org/jira/browse/CLJS-615. When that patch landed I tested only the behavior of the browser REPL from within the repo and did not test the behavior when used with CLJS dependencies in a project.

Comment by David Nolen [ 07/Mar/14 3:37 PM ]

After closer inspection this does appear to be a bug in ClojureScript itself, rather lein-cljsbuild's browser REPL support.





[CLJS-780] Incorrect behaviour of apply-to for (>= argc 6) Created: 03/Mar/14  Updated: 05/Mar/14  Resolved: 05/Mar/14

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

Type: Defect Priority: Major
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None


 Description   

The cljs.core/apply-to function exhibits incorrect behaviour when called with an "argument count" argc (the second argument to apply-to) greater than or equal to 6.

This is because the cljs.core/gen-apply-to macro produces (essentially) to following function:

(defn apply-to
  [f argc args]
  (let [args (seq args)]
    (if (== argc 0)
      (f)
      (let [[a & args] args]
        (if (== argc 1)
          (f a)
          (...
            (if (== argc 6)
              (let [[f % args] args] ;; f is being overshadowed
                (f a b c d e f)
                (if (== argc 7)
                  (let [[g & args] args]
                    (f a b c d e f g)
                    (...)))))))))))

For (>= argc 6) the f in

(defn apply-to [f argc args] ...)

is being overshadowed by the one in

(let [[f % args] args] ...)

The obvious solution would be to output something like

(defn apply-to
  [func argc args]
  (let [args (seq args)]
    (if (== argc 0)
      (func)
      (let [[a & args] args]
        (if (== argc 1)
          (func a)
          (...
            (if (== argc 6)
              (let [[f % args] args]
                (func a b c d e f)
                (if (== argc 7)
                  (let [[g & args] args]
                    (func a b c d e f g)
                    (...)))))))))))

The following changes to gen-apply-to-helper and gen-apply-to should do the trick:

(defn gen-apply-to-helper
  ([] (gen-apply-to-helper 1))
  ([n]
     (let [prop (symbol (core/str "-cljs$core$IFn$_invoke$arity$" n))
           func (symbol (core/str "cljs$core$IFn$_invoke$arity$" n))]
       (if (core/<= n 20)
         `(let [~(cs (core/dec n)) (-first ~'args)
                ~'args (-rest ~'args)]
            (if (core/== ~'argc ~n)
              (if (. ~'func ~prop)
                (. ~'func (~func ~@(take n cs)))
                (~'func ~@(take n cs)))
              ~(gen-apply-to-helper (core/inc n))))
         `(throw (js/Error. "Only up to 20 arguments supported on ifntions"))))))
         
(defmacro gen-apply-to []
  `(do
     (set! ~'*unchecked-if* true)
     (defn ~'apply-to [~'func ~'argc ~'args]
       (let [~'args (seq ~'args)]
         (if (zero? ~'argc)
           (~'func)
           ~(gen-apply-to-helper))))
     (set! ~'*unchecked-if* false)))


 Comments   
Comment by David Nolen [ 04/Mar/14 11:33 AM ]

apply-to is an internal helper do you have an example where this is a problem in practice? Thanks.

Comment by Kevin Marolt [ 04/Mar/14 5:26 PM ]

Sure, try this with :optimizations :none (I haven't tested it with other optimization settings):

(def a (atom {:foo (with-meta [] {:bar '(1 2 3)})}))
(swap! a update-in [:foo] vary-meta update-in [:bar] vector)
(prn (= @a {:foo (with-meta [] {:bar [1 2 3]})})) ;; prints "false"
(prn (= @a [{:foo (with-meta [] {:bar '(1 2 3)})}
            [:foo]
            vary-meta
            update-in
            [:bar
            vector])) ;; prints "true"

The goal was to vectorize the list '(1 2 3), but swap! is applying update-in to the arguments

{:foo (with-meta [] {:bar '(1 2 3)})} ;; a
[:foo]                                ;; b
vary-meta                             ;; c
update-in                             ;; d
[:bar]                                ;; e
vector                                ;; f

and thus, because vector is in sixth position (and therefore the f-argument), what's actually happening is that vector is incorrectly called with those same arguments instead.

Comment by David Nolen [ 05/Mar/14 8:51 AM ]

fixed https://github.com/clojure/clojurescript/commit/13cacc68b4c9816bb09ce048ca3eb6dcf44e3144





[CLJS-779] Permit omitting hashbang with nodes target. Created: 03/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-779.patch    

 Description   

In some environments (such as parse.com's Code Cloud CLJS-722), the node.js hashbang should be omitted. Currently if the target is "nodejs", a hashbang will always be applied at the top of the file.



 Comments   
Comment by Michael Glaesemann [ 03/Mar/14 5:48 PM ]

Following brief discussion on CLJS-771 and <https://groups.google.com/d/msg/clojurescript/CuInr2L5yFo/562AIcRsrksJ>, I've worked up a patch that omits the hashbang if :hashbang false is added as a compiler option.

Another option would be to go back to the original behavior that if the :preamble option is provided, it assumes the necessary hashbang is provided in the preamble. I find this a bit unintuitive (which is why I provided a patch to allow preambles with the node.js target anyway), and think that this is a better, more explicit solution.

Comment by David Nolen [ 04/Mar/14 11:44 AM ]

fixed https://github.com/clojure/clojurescript/commit/00a9b7bde0c8823175560b453a00e7be09ddd250





[CLJS-778] Unable to call `set` on `RSeq`s Created: 03/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Defect Priority: Minor
Reporter: Omri Bernstein Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript release 1798+
(Probably not relevant: Leiningen 2.3.4, Java 1.7.0_51, Ubuntu 12.04, Toshiba Satallite L755)



 Description   

Error when I run, at a REPL:

user> (set (reverse [0]))
"Error evaluating:" (set (reverse [0])) :as "cljs.core.set.call(null,cljs.core.reverse.call(null,new cljs.core.PersistentVector(null, 1, 5, cljs.core.PersistentVector.EMPTY_NODE, [0], null)))"
org.mozilla.javascript.JavaScriptException: Error: No protocol method INext.-next defined for type cljs.core/RSeq: (0) (cljs/core.cljs#266)
at cljs/core.cljs:266 (anonymous)
at cljs/core.cljs:260 (_next)
at cljs/core.cljs:6368 (set)
at <cljs repl>:1 (anonymous)
at <cljs repl>:1
nil

(Specifically, this is in a SublimeREPL Rhino REPL.)

The problem seems to happen for any `cljs.core/RSeq` types. I've tried loading different ClojureScript release versions. The problem seems to exist for releases 1798 and higher but not for 1586 and lower. If someone could explain how to load non-released ClojureScript commits, I would be happy to narrow the window even further.



 Comments   
Comment by David Nolen [ 04/Mar/14 11:41 AM ]

fixed https://github.com/clojure/clojurescript/commit/87deb8b080d0c930fe92a7a58c51f22a559033e5





[CLJS-777] multimethods should be ifn? Created: 01/Mar/14  Updated: 01/Mar/14  Resolved: 01/Mar/14

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

Type: Defect Priority: Major
Reporter: Jacob Maine Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

In regular Clojure, this passes

(defmulti mm :type)
(assert (ifn? mm))

But it fails in ClojureScript.

Note that (fn? mm) is false for Clojure, so this can't be fixed by adding the Fn marker protocol to MultiFn.



 Comments   
Comment by David Nolen [ 01/Mar/14 3:26 PM ]

fixed https://github.com/clojure/clojurescript/commit/525154f2a4874cf3b88ac3d5755794de425a94cb





[CLJS-776] re-matches is incorrect Created: 28/Feb/14  Updated: 05/Mar/14

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

Type: Defect Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The re-matches function does not have the correct semantics: it performs a search (not match) against the string and returns nil if the string and matched-string are unequal. This is not the same as true matching, which is like inserting "^" and "$" at the beginning and end of the pattern.

Example in Clojure:

user=> (re-find #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
"0"
user=> (re-matches #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
"0x1"

Compare Clojurescript:

ClojureScript:cljs.user> (re-find  #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
"0"
ClojureScript:cljs.user> (re-matches #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
nil

This bug is (one of the) reasons why CLJS-775.

I'm not completely sure what to do here. My first thought is to have re-matches inspect the -source property of its regex input, wrap the string with "^$", then carefully copy all flags over to a new regexp.

Questions:

  1. Are there any valid patterns where this is not safe? E.g., where we could not put ^ first? Is "^^abc$$" ok?
  2. Can we avoid cloning if ^ and $ are already the first and last chars of the pattern?
  3. How does multiline mode play in to this, if at all?
  4. regexinstance.lastIndex is a piece of mutability on regex instances (or the RegExp global on older browsers) which is used as a string offset for multiple invocations of exec() on the same string. I have no idea what to do if re-* gets a regex with the global flag set. (BTW, this is a very good reason to reject CLJS-150: allowing clojure to accept the global flag makes regular expression objects stateful, and would completely screw up re-seq for example.)





[CLJS-775] cljs.reader parses radix form of int literals (e.g. 2r101) incorrectly Created: 27/Feb/14  Updated: 28/Feb/14

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

Type: Defect Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader

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

 Description   
ClojureScript:cljs.user> (cljs.reader/read-string "2r10")
"Error evaluating:" (cljs.reader/read-string "2r10") :as "cljs.reader.read_string.call(null,\"2r10\")"
org.mozilla.javascript.JavaScriptException: Error: Invalid number format [2r10] (file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js#107)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:107 (anonymous)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:112 (anonymous)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:374 (read_number)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:650 (read)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:677 (read_string)
	at <cljs repl>:1 (anonymous)
	at <cljs repl>:1


 Comments   
Comment by Francis Avila [ 28/Feb/14 1:42 AM ]

Turns out other integer literals were broken besides radix due to a re-match problem (CLJS-776). Patch includes fix and tests for all the different integer literal forms.

Floats, ratios, and symbols/keywords might also have parsed incorrectly in certain cases, but I did not produce failing tests to confirm.





[CLJS-774] cljs.reader should js js/parseInt with radix specified Created: 27/Feb/14  Updated: 12/Mar/14  Resolved: 12/Mar/14

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

Type: Defect Priority: Major
Reporter: Tatu Tarvainen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-774.patch    

 Description   

In cljs.reader, the parse-int calls js/parseInt without a radix.

The radix is implementation defined if not specified.
This causes problems for example in older Android browsers which parse "08" as 0 instead of 8.

This makes reading timestamps fail. For example:
Trying to read: #inst "2013-07-08T21:00:00.000-00:00"
causes:
Error: timestamp day field must be in range 1..last day in month Failed: 1<=0<=31

in js console:
parseInt("08") => 0
parseInt("08", 10) => 8



 Comments   
Comment by Francis Avila [ 27/Feb/14 10:38 PM ]

Discovered integer-with-radix parsing problem but split it into CLJS-775 since it's not a simple parseInt issue.

This patch should fix ratio and inst parsing, but I don't have access to a browser which infers octals in parseInt.

Existing tests are sufficient to catch this problem for #inst.

There are no ratio tests for reader at all currently, but since ratios are shaky ground in cljs anyway I didn't add any.

Comment by Dave Della Costa [ 12/Mar/14 1:14 AM ]

Just chiming in to mention that I have experienced this issue in Internet Explorer 8 and was going to submit a patch myself. Would be interested in getting this into the next version as for now we have a customized reader that we use to get past this.

Comment by David Nolen [ 12/Mar/14 8:26 AM ]

fixed https://github.com/clojure/clojurescript/commit/004224511b4351acbfe051c7dea913fb0c183c04





[CLJS-773] Use unchecked-*-int functions for real 32-bit math Created: 26/Feb/14  Updated: 26/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: numerics
Environment:

r2173



 Description   

Currently the unchecked-* functions and macros simply alias the primitive js operators. It would be nice if the unchecked-*-int family of functions and macros implemented C/Java-like signed int operations with silent overflows (just like in Clojure) using asm.js coersion idioms. This should also allow us to share such code between clojure and clojurescript without worrying about their different numerics.

A use case is that porting hash algorithms from java to clojurescript is trickier and more verbose than it needs to be.






[CLJS-772] Enhance Node.js Support Created: 24/Feb/14  Updated: 24/Feb/14  Resolved: 24/Feb/14

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Google Closure now comes with a Node.js bootstrap script that makes require and provide work https://code.google.com/p/closure-library/wiki/NodeJS. This enhancement requires a newer version of Closure Library.



 Comments   
Comment by David Nolen [ 24/Feb/14 7:48 AM ]

fixed https://github.com/clojure/clojurescript/commit/0c7b31ada01237de33cef77b817ccef3f2b3576d





[CLJS-771] Allow nodejs target preambles Created: 21/Feb/14  Updated: 26/Feb/14  Resolved: 21/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-771.2.patch     Text File CLJS-771.patch    

 Description   

Currently targeting nodejs precludes the use of a preamble. There are cases where you might want to add arbitrary code (besides the node hashbang) at the top of the compiled file. (see brief discussion at https://groups.google.com/forum/#!topic/clojurescript/CuInr2L5yFo)



 Comments   
Comment by David Nolen [ 21/Feb/14 1:16 PM ]

Thanks but it appears your patch is dirty - it includes compiler version information. Will apply if we get a cleaned up version. Thanks!

Comment by Michael Glaesemann [ 21/Feb/14 1:23 PM ]

Updated (git -a is not always helpful).

Comment by David Nolen [ 21/Feb/14 1:40 PM ]

fixed http://github.com/clojure/clojurescript/commit/4b7cdd4fb3cfae1c2e325a78eae54de0cb164d78

Comment by Travis Vachon [ 26/Feb/14 12:59 PM ]

Arg, sorry I missed this - the problem is that not all node-like environments need a hashbang. As this code used to be it was up to the creator of the preamble to include the hashbang if they wanted one - the idea was that the hashbang was just the default preamble in a node environment.

I've replied to the list addressing the original motivation for this patch, but I'd very much like to see it reconsidered (except for the test, which is great! though of course I'd want it updated for the behavior I think is correct ;-D )

happy to submit a followup patch if needed

Comment by Michael Glaesemann [ 26/Feb/14 1:29 PM ]

That'd be an option, though it's also possible to set the value of the hashbang. Does :hashbang "" do what you want?

Comment by Michael Glaesemann [ 26/Feb/14 1:31 PM ]

Scratch that, it doesn't, does it. The code prepends #! to the path to node.





[CLJS-770] Include :js-dependency-index in result of `(env/default-compiler-env)` Created: 18/Feb/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

Type: Defect Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: File CLJS-770.diff    
Patch: Code

 Description   

It looks like the change from CLJS-615 requires the addition of something like this prior to using any of the dependency machinery:

(swap! env/*compiler* assoc :js-dependency-index (cljsc/js-dependency-index opts))

That the dependency index is now in the compiler environment isn't a problem (on the contrary), but cljs.env/default-compiler-env is there for exactly this sort of thing: the environment that fn returns should have :js-dependency-index added already. This would necessitate pulling all of the JS dependency resolution machinery up into cljs.env (or perhaps another namespace, cljs.js-deps?), but would eliminate the user-facing complication of getting a useful compiler environment with a side benefit of getting all the JavaScript dependency stuffs out of cljs.closure and into a home of their own.

I'm mucking with this in conjunction w/ CLJS-656; shouldn't be difficult to do afterwards.

Interested?



 Comments   
Comment by David Nolen [ 18/Feb/14 8:11 AM ]

I'm ok with pulling this stuff into it's own namespace.

Comment by Chas Emerick [ 23/Feb/14 9:35 AM ]

Attached patch refactors goog-style JS dependency stuff into cljs.js-deps, and changes cljs.env/default-compiler-env to populate :js-dependency-index based on the provided set of options.

I didn't move the externs stuff, seemed like it should stay in cljs.closure.

Note that this doesn't change the fact that CLJS compiler API consumers will need to change: cljs.env/default-compiler-env now takes a map argument of options, which needs to be provided if such consumers need to have their e.g. :libs, :foreign-libs options picked up. The advantage is default-compiler-env again does the job it's intended to do. The refactoring / slimming of cljs.closure is a nice side effect, tho.

Comment by David Nolen [ 23/Feb/14 4:24 PM ]

fixed https://github.com/clojure/clojurescript/commit/90cf724d8f5d4fa5029aad204a9e2bcfe36174d4





[CLJS-769] clojure.core/unsigned-bit-shift-right not excluded in cljs.core Created: 17/Feb/14  Updated: 17/Feb/14

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

Type: Defect Priority: Trivial
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: File CLJS-769.diff    
Patch: Code

 Description   

EOM






[CLJS-768] (persistent! (assoc! (transient [0]) nil 1)) => [1] Created: 17/Feb/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

r2156


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

 Description   

assoc!-ing a non-numeric index into a transient vector is the same as assoc!-ing index 0. This should throw an exception instead. Patch and tests attached.



 Comments   
Comment by David Nolen [ 20/Feb/14 11:41 AM ]

This patch no does not cleanly apply on master. Can we get a new patch? Thanks.

Comment by Francis Avila [ 21/Feb/14 9:31 AM ]

Rebased patch.

Comment by David Nolen [ 23/Feb/14 4:30 PM ]

fixed https://github.com/clojure/clojurescript/commit/8419d8aeb6d2e975029fc693f96137775d2442c6





[CLJS-767] vector and subvector implement assoc-n with assoc; (assoc [0] nil 1) -> [1] Created: 17/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

r2156


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

 Description   

PersistentVector and Subvec implement their IVector protocol (-assoc-n) with IAssociative (-assoc); it should be the other way around:

  1. Because IVector has an additional numeric index contract that IAssociative does not. The neglected number check causes (assoc [0] nil 1) to return [1] instead of throwing an exception.
  2. Because Java Clojure does not do this (APersistentVector.assoc does a numeric check then calls PersistentVector.assocN).

See comments in CLJS-728 for more detailed rationales.

Patch attached which reverses this, and also fixes the error when a non-number is used as a key/index to assoc on PersistentVector and Subvec. Note: (assoc! (transient [0]) nil) is still broken.



 Comments   
Comment by David Nolen [ 20/Feb/14 11:40 AM ]

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





[CLJS-766] Fix broken NodeJS samples Created: 16/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Minor
Reporter: Jeff Dik Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-766.patch    
Patch: Code

 Description   

Broken by CLJS-722



 Comments   
Comment by David Nolen [ 20/Feb/14 11:47 AM ]

fixed https://github.com/clojure/clojurescript/commit/f6d743b835df5d1b2a4991193a4d7740b1d67aff





[CLJS-765] Subvec should implement IReversible Created: 16/Feb/14  Updated: 16/Feb/14  Resolved: 16/Feb/14

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

Type: Defect Priority: Major
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-765-implement-IReversible-for-Subvec.patch    

 Description   

Reported by Sunil Nandihalli in this thread:

https://groups.google.com/d/msg/clojure/1XkYDB-FDMA/AAWHMukms8MJ

rseq doesn't work on Subvec, because no implementation of IReversible is provided.

Patch forthcoming.



 Comments   
Comment by Michał Marczyk [ 16/Feb/14 1:54 PM ]

Oops, didn't include tests in the last patch. Same fix + tests this time.

Comment by David Nolen [ 16/Feb/14 2:46 PM ]

fixed https://github.com/clojure/clojurescript/commit/6e5f063c0256521756aa4f06aa14b9e7753f208e





[CLJS-764] repl/evaluate-form should pass along file-information Created: 16/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Major
Reporter: Gary Trakhman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

I was getting missing file info for core defs in a repl context, where normally (a full compile) would show the correct information.

This is required for repl-level file-metadata, which cider needs for jump-to-definition.
https://github.com/clojure-emacs/cider/issues/462

With the current patch, cider can jump through cljs files and jar resources correctly.



 Comments   
Comment by Gary Trakhman [ 16/Feb/14 9:44 AM ]

fix and repl-based test

Comment by Gary Trakhman [ 16/Feb/14 9:00 PM ]

In piggieback, load-stream seems to miss file/line for transitive requires, but it looks fine when I try to replicate it in a cljs test. Will work on it.

Comment by Gary Trakhman [ 19/Feb/14 12:40 PM ]

Further testing shows that file+line is indeed making it through, I think the current patch is enough.

Comment by David Nolen [ 20/Feb/14 11:43 AM ]

Because of whitespace changes it's impossible to see what changed. Can you explain the changes? Thanks!

Comment by Gary Trakhman [ 20/Feb/14 12:09 PM ]

The relevant line is '(binding [ana/*cljs-file* filename]'.

It sets the var so code down the line can pick it up.

Comment by David Nolen [ 20/Feb/14 12:20 PM ]

fixed https://github.com/clojure/clojurescript/commit/953819ff7e4250e593489445ae650856cd0ce1b7





[CLJS-763] Docstring does not mention argument names in description. Created: 13/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Ryan Mulligan Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

http://clojuredocs.org/clojure_core/clojure.core/rem

The arguments to rem are named num and div and the docstring refers to numerator and denominator.



 Comments   
Comment by Ryan Mulligan [ 14/Feb/14 9:30 AM ]

Sorry, I meant to report this against the Clojure project, not the Clojurescript one. I recommend closing this! Thanks.





[CLJS-762] "goog.require could not find: cljs_gwt.css" since compiler generates wrong code Created: 06/Feb/14  Updated: 08/Feb/14  Resolved: 08/Feb/14

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

Type: Defect Priority: Major
Reporter: Sergey Stupin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

[org.clojure/clojure "1.5.1"]
[org.clojure/clojurescript "0.0-2156"]

OSX 10.9.1
Chrome Version 32.0.1700.102



 Description   

The project for which the problem occurred is here https://github.com/hsestupin/cljs-gwt.
Steps to reproduce the problem:
1) from one console: lein ring server
2) from other console: lein cljsbuild clean & lein cljsbuild auto dev
3) http://localhost:3000/ in Google Chrome with dev-tools console. There will be an error: "goog.require could not find: cljs_gwt.css"

It was happened because of cljs/cljs_gwt/core.cljs is beginning with lines

(ns cljs-gwt.core
  (:require [cljs-gwt.css :as css]))

And then ClojureScript compiler emits the following from previous 2 lines resources/public/js/cljs_gwt.js

goog.require("cljs_gwt.css");
goog.require("cljs_gwt.css");

instead of

goog.provide("cljs_gwt.css");
goog.require("cljs_gwt.css");

therefore an error occurred. That's it.



 Comments   
Comment by Francis Avila [ 06/Feb/14 4:15 PM ]

There is no goog.provide("cljs_gwt.css") emitted because there is no file that provides that namespace. Your css.clj file is misnamed--it should be css.cljs, otherwise clojurescript won't see it. (Also it has unbalanced parens and an illegal set!, so it won't compile anyway.)

There are two minor issues that you did hit upon, though:

  • You should have gotten a compiler warning for the missing namespace. I'm not sure why you didn't.
  • The cljs compiler may emit the same goog.require multiple times. This doesn't harm anything and the google closure compiler will remove them all.
Comment by Sergey Stupin [ 06/Feb/14 5:00 PM ]

Thanks for explanation, it helps a lot.





[CLJS-761] no method may be called on number literals Created: 02/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Minor
Reporter: Ryan Mulligan Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

http://himera.herokuapp.com/index.html
Chromium 32.0.1700.77 (244343)
Linux hostname 3.12.7-2-ARCH #1 SMP PREEMPT Sun Jan 12 13:09:09 CET 2014 x86_64 GNU/Linux



 Description   

Himera REPL v0.1.5
cljs.user> (.toString 1)
Compilation error: SyntaxError: Unexpected token ILLEGAL

The problem is that it compiles down to

1.toString()

and the parser interprets "1." as a literal float, followed by junk. This can be fixed with parenthesis around the literal:

(1).toString()



 Comments   
Comment by Ryan Mulligan [ 02/Feb/14 1:11 PM ]

At dc4ba2e this happens at the REPL started by following the instructions at https://github.com/clojure/clojurescript/wiki/Quick-Start

ClojureScript:cljs.user> (.toString 1)
"Error evaluating:" (.toString 1) :as "1.toString()"
org.mozilla.javascript.EvaluatorException: missing ; before statement (<cl
js repl>#1)

Comment by Francis Avila [ 03/Feb/14 10:36 AM ]

Duplicate of CLJS-715





[CLJS-760] Add an IAtom protocol Created: 01/Feb/14  Updated: 17/Feb/14  Resolved: 17/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-760.patch     Text File CLJS-760.patch     File patch    
Patch: Code

 Description   

Add an IAtom protocol with a -reset! method and a fast path for Atom in cljs.core/reset!.

See jsperf here - http://jsperf.com/iatom-adv

Latest chrome and firefox versions suffer ~20-30% slowdown. Older firefox versions suffer up to 60-70%.



 Comments   
Comment by David Nolen [ 02/Feb/14 11:40 AM ]

This is not a properly formatted patch - http://github.com/clojure/clojurescript/wiki/Patches.

Comment by Jamie Brandon [ 02/Feb/14 11:51 AM ]

Fixed

Comment by David Nolen [ 02/Feb/14 6:33 PM ]

fixed https://github.com/clojure/clojurescript/commit/33692b79a114faf4bedc6d9ab38d25ce6ea4b295

Comment by Jozef Wagner [ 03/Feb/14 2:40 AM ]

What is the rationale behind this? Do you plan to have multiple Atom types?

Comment by David Nolen [ 03/Feb/14 8:13 AM ]

There's no good reason to lock down atom behavior to the one provided by ClojureScript.

Comment by Jozef Wagner [ 03/Feb/14 10:25 AM ]

I agree that such behavior should be polymorphic, I was just surprised that it is a pressing issue in a single threaded CLJS. Clojure itself does not provide such abstraction. Anyway, if you want to get it right, I suggest also to include swap! and compare-and-set! in the protocol (in order to not lock down the sharing and change policy), and to name it something like IMutableReference, as these methods are generic enough to be used outside atom, (as atom promises additional features besides those of a simple mutable reference). See https://gist.github.com/wagjo/8786305 for inspiration.

Comment by David Nolen [ 03/Feb/14 11:21 AM ]

I question the utility of compare-and-set! given most JS hosts are single threaded (but who knows? things are changing quickly in the JS world). It's something to consider in the future. IMutableReference is just more to type and I'm not really convinced it offers anything semantically over IAtom. Atoms do support more functionality but this is covered in other protocols.

Comment by David Nolen [ 03/Feb/14 11:21 AM ]

We need to include -swap! in the IAtom protocol.

Comment by Jozef Wagner [ 03/Feb/14 12:02 PM ]

The usefulness of the atom and protocols behind it should 'transcendent' the notion of single/multiple threads. Prime example is the Avout and its zk-atom and zk-ref. If e.g. implementing Avout in CLJS is a reasonable and useful thing to do, we should prepare for it by designing protocols to handle such use cases.

Comment by David Nolen [ 03/Feb/14 12:30 PM ]

Avout is an third party library which has no relation to the direction of ClojureScript. This ticket is a conservative change that opens up the door for extending atom-like capabilities to user types, that's it. Further enhancements may or may not come in the future.

Comment by David Nolen [ 17/Feb/14 1:49 PM ]

fixed https://github.com/clojure/clojurescript/commit/3e6564a72dc5e175fc65c9767364d05af34e4968





[CLJS-759] Exceptions are reported at weird locations in the presence of bad quasiquoting Created: 31/Jan/14  Updated: 31/Jan/14

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

Type: Defect Priority: Minor
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

[org.clojure/clojurescript "0.0-2138"]



 Description   

(defn foo [x]
`(f ~@(for [i (range 10)] ~i)))

(foo 1)

This code has an extra unquote next to the i causing:

TypeError: Cannot call method 'call' of undefined
at aurora.compiler.foo.cljs.core.with_meta.call.cljs.core.seq.call.cljs.core.concat.call.iter_7602auto_ (/home/jamie/aurora/src/aurora/compiler.cljs[eval362]:229:100)

Compare this to:

(defn foo [x]
(bar x))

(foo 1)

Which causes:

TypeError: Cannot call method 'call' of undefined
at foo (/home/jamie/aurora/src/aurora/compiler.cljs[eval376]:212:67)

In the first case, the symbol is mangled and the line number and column are incorrect.






[CLJS-758] Browser REPL doesn't properly catch errors Created: 31/Jan/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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: None


 Description   

Brandon Bloom has a patch here http://gist.github.com/brandonbloom/8744556/raw/2efb894b11bb25e9724e3514c7bccc872e30b28d/open+0001-Use-catch-default-in-browser-repl-and-reflect.patch



 Comments   
Comment by David Nolen [ 20/Feb/14 11:45 AM ]

fixed https://github.com/clojure/clojurescript/commit/203d853f1e9c5673cfd987feb7cd07a4762e1479





[CLJS-757] Redundant bounds checking in indexed types Created: 29/Jan/14  Updated: 24/Feb/14  Resolved: 24/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

r2156


Attachments: Text File cljs-757.patch     Text File cljs-757.patch    

 Description   

PersistentVector and TransientVector contain code paths where a bounds check is performed more than once unnecessarily.



 Comments   
Comment by Francis Avila [ 29/Jan/14 11:44 PM ]

Patch gives a minor across-the-board speedup on vector operations.

I also discovered and fixed an off-by-one check in PersistentVector's -seq, which causes an unnecessary ChunkedSeq to be created when vector length is exactly 32.

Comment by David Nolen [ 23/Feb/14 4:34 PM ]

Can we get a rebased version of this patch? Thanks.

Comment by Francis Avila [ 24/Feb/14 12:22 AM ]

Rebased cljs-757 with a change to unchecked-array-for-longvec (now first-array-for-longvec) and extra comments documenting invariants.

Comment by David Nolen [ 24/Feb/14 5:59 AM ]

fixed https://github.com/clojure/clojurescript/commit/0f943b616e36c65e0a8921fb838b71368fe9674c





[CLJS-756] undeclared ns warning needs to check for existing required namespaces Created: 29/Jan/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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: None


 Description   

with-out-str currently produces a warning even though goog.string is loaded by core.clj.



 Comments   
Comment by David Nolen [ 29/Jan/14 11:46 AM ]

This means we need to always know the entire set of namespaces that are currently required. This will eliminate a long outstanding bug with respect to resolving symbols generated by macros that are in namespaces not explicitly required by users as they are implicit when loading the main library namespace. This is of course known for ClojureScript libraries, but we need special casing for Google Closure libs, Closure compatible libs, and foreign libs that the user provides a namespace for.

Comment by David Nolen [ 14/Feb/14 9:04 PM ]

See http://dev.clojure.org/jira/browse/CLJS-615

Comment by David Nolen [ 20/Feb/14 11:46 AM ]

Resolved by CLJS-615





[CLJS-755] Expose Clojure's hash calculation helper API Created: 29/Jan/14  Updated: 29/Jan/14

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

To aid in collection portability between Clojure implementations.






[CLJS-754] Assess Murmur3 for Collections Created: 29/Jan/14  Updated: 26/Feb/14

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

Type: Task Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

We should assess the performance implications of adopting Murmur3 for hashing - http://github.com/clojure/clojure/commit/dff9600387b962f16fc78e6477e10e34651fd366



 Comments   
Comment by Francis Avila [ 19/Feb/14 5:19 AM ]

I have a (lightly tested, probably untrustworthy) gist of clj.lang.murmur3 ported to javascript. The two other murmur3 js libs I saw both had flaws which I attempt to address.

Found a jsperf comparing murmur3 libs and added a goog.string.hashCode case as a baseline. This jsperf test isn't very good because:

  1. it only tests a tiny string
  2. the murmur libraries are both flawed:
    1. one of them assumes strings are 8-bit only (consumes 4 chars at a time for hashing)
    2. the other one treats strings as utf-16, but does not account for integer overflows.

However, the results are still informative--string hashing with murmur3 in js will probably be half as fast as goog.string.hashCode, except on firefox where it is about the same.

Not nearly enough data for a true assessment, but I hope it helps.

Comment by Francis Avila [ 19/Feb/14 10:20 AM ]

It occurs to me that the slowdown is probably entirely due to integer multiplication, and that spidermonkey's jit is detecting and optimizing that pattern of code to Math.imul while the others are not. Math.imul is still experimental but supported on many browsers already--it might be better to polyfill it instead of unconditionally inlining the bit-shifting. Will try later.

Comment by David Nolen [ 19/Feb/14 2:23 PM ]

There's very little need to write this logic in JavaScript - it should just be done in ClojureScript. I think modern JS engines can probably handle Murmur3, here are some perf numbers for simulating multiplication of 2 unsigned 32bit ints that I found on StackOverflow http://jsperf.com/bit-multiply

Comment by Francis Avila [ 19/Feb/14 3:14 PM ]

I agree re: implementing in clojurescript. It looked hairier when I started.

You're also right about the bit multiply. I extended that case with larger input values and a comparison against Math.imul and it seems to be compiling to the same code. Maybe inlining the bit-multiply is confusing the jits, or maybe it's something else entirely. I'll give it another shot when I get a chance.

Any advice on setting up tests for this that would work in the clojurescript project's environment? Probably what we want is a generative test which ensures the hash value in cljs is the same as in clj?

Comment by David Nolen [ 19/Feb/14 4:34 PM ]

Generative tests that guarantee the hashes are the same in both CLJS & CLJ is a nice to have and not really a priority. Murmur3 is pretty straightforward looking, I'm sure we can get it right

Comment by David Nolen [ 20/Feb/14 9:57 AM ]

One of the V8 engineer did a more accurate version of the benchmark. It looks like Math.imul is the way to go and we should shim in something for older browsers.

http://jsperf.com/bit-multiply/5

Comment by Francis Avila [ 22/Feb/14 2:20 AM ]

I thought those results looked too good. 32-bit maths in js is always tricky, hence desire for tests, especially if one of the use cases is comparing compile-time and run-time hashes.

Cleaned up js implementation of murmur3 just to get something up quickly to assess murmur3 performance. It uses Math.imul or a shim for integer multiplication. Then created a pair of jsperfs:

In both tests, setup code is the same: pretty-printed closure-advanced-compiled code from my js-murmur3 implementation, and copy-pasted code from cljs's current number and string hashing.

Results are...interesting. So interesting that I am suspicious that something is wrong with my benchmarks or code. Perhaps you can have Mr. Egorov take a look? In summary:

  • murmur3 int hash is about an 8% performance regression in firefox and safari.
  • murmur3 string hash is more than double the speed of goog.string.hashCode on ff and safari for small strings, and even better for large ones.
  • Except in chrome, where murmur3 is abysmal--about an order of magnitude regression on both ints and strings!

All these browsers have a native Math.imul, and Chrome's imul is definitely working. There must be something else making chrome's jit cranky.

I did not expect murmur3 to perform so well or so badly!

Comment by Francis Avila [ 26/Feb/14 3:37 AM ]

Maintaining a fork with murmur3 hashing (see murmur3 branch). So far numbers, strings, and collections (ordered and unordered) hash identically to clojure 1.6-beta1, but symbols and keywords do not. I suspect integer-math differences in cljs's hash-combine vs clojure.lang.Util/hashCombine, but I have not isolated the problem yet.

Comment by Francis Avila [ 26/Feb/14 3:06 PM ]

Nevermind, cljs and clj reverse the order of ns+name hashing and I just didn't notice.

This branch appears to produce hashes identical to clojure 1.6 for the following types:

  • strings
  • keywords and symbols
  • vectors, lists, persistentqueue, seqs
  • maps, sets
  • integers representable with doubles in js (i.e., about 53 bits of signed integer)

Types which do not hash the same (likely incomplete list):

  • uuids
  • instances
  • floating-point types. Clojure 1.6 still uses Java's native hashCode for these, and checking for a non-int is a bit involved in js, so I just hash all js numbers as if they were longs. This might be a bad idea if we have a collection of "real" doubles.




[CLJS-753] Too many open files exception Created: 28/Jan/14  Updated: 28/Jan/14  Resolved: 28/Jan/14

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

Type: Defect Priority: Blocker
Reporter: Roman Scherer Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: File too-many-open-files.diff    
Patch: Code

 Description   

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.

From the Google Group discussion at: https://groups.google.com/forum/#!topic/clojurescript/r2iGPh2Lv0U

----------------------------

Hello Clojure Scripters,

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?

Thanks for your help, Roman.



 Comments   
Comment by Roman Scherer [ 28/Jan/14 5:56 PM ]

Hi David, can you review this. Thanks.

Comment by David Nolen [ 28/Jan/14 6:19 PM ]

Looks good to me will apply soon and test.

Comment by David Nolen [ 28/Jan/14 10:51 PM ]

fixed, https://github.com/clojure/clojurescript/commit/905c64445faa1c60e21b66fb226982759b0d4001





[CLJS-752] Implement specify! to enable extending instances that don't implement ICloneable Created: 28/Jan/14  Updated: 28/Jan/14  Resolved: 28/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Implementing specify! would enable instances of built-in objects (DOM objects, etc) to be extended with protocols without polluting the environment.



 Comments   
Comment by David Nolen [ 28/Jan/14 10:59 PM ]

fixed, https://github.com/clojure/clojurescript/commit/6e5896cb6cd3549d069473e86e424104c107223d





[CLJS-751] requiring clojure.core.reducers throws error Created: 15/Jan/14  Updated: 16/Jan/14  Resolved: 16/Jan/14

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

Type: Defect Priority: Critical
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reducers
Environment:

r2138


Attachments: Text File cljs-751.patch    

 Description   

Requiring the reducers library in any file throws a TypeError at runtime. The generated javascript in reducers.js is this:

cljs.core.IPersistentVector.prototype.clojure$core$reducers$CollFold$ = true;

Error messages is "Uncaught TypeError: Cannot read property 'prototype' of undefined reducers.js:733
(anonymous function)". This stops all execution of JS and basically makes it impossible to use any

IPersistentVector in reducers.cljs should probably be PersistentVector. The specify machinery in extend-type probably exposed this bug.



 Comments   
Comment by Francis Avila [ 15/Jan/14 4:54 PM ]

Note that because of advanced-compilation dead-code elimination, you won't catch this if you advance-compile. Run script/test with whitespace optimizations to see the reducer tests fail.

Comment by Francis Avila [ 15/Jan/14 4:57 PM ]

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

Comment by David Nolen [ 16/Jan/14 5:19 PM ]

fixed, https://github.com/clojure/clojurescript/commit/6e10f3d2ca99c58c441de1a1031be2649dae4072





[CLJS-750] js->clj not turning keys into keywords Created: 14/Jan/14  Updated: 16/Jan/14  Resolved: 16/Jan/14

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

Type: Defect Priority: Major
Reporter: Matthew Phillips Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I created a fiddle for this: http://cljsfiddle.net/fiddle/matthewp.myfiddle

Click run and then inspect the console. You'll notice that the keys are just strings, not clojure keywords as expected.



 Comments   
Comment by David Nolen [ 16/Jan/14 5:15 PM ]

The fiddle is incorrect.

(js->clj x :keywordize-keys true)

Is what you want.





[CLJS-749] Changing ClojureScript versions may break browser REPL Created: 14/Jan/14  Updated: 14/Jan/14

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

Type: Defect Priority: Minor
Reporter: Osbert Feng Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File change-browser-repl-compile-directory.patch    
Patch: Code

 Description   

By default in a project using ClojureScript, the .repl/ directory is used to store compiled CLJS for the browser REPL. This compilation is skipped if the directory already exists (src/clj/cljs/repl/browser.clj:205:create-client-js-file). However, it really should be re-compiled if the version of ClojureScript is upgraded/changed or else the browser REPL may fail with some very difficult to interpret error messages. At the moment, it seems people simply know to delete .repl/ when changing ClojureScript versions (https://groups.google.com/forum/#!topic/clojure/C-H4gSWIUwc) but this can be extremely tough on new users when they upgrade ClojureScript for the first time.

We could append clojurescript-version to the directory name. Unfortunate that it generates a new directory each time a new version of CLJS/BrowserREPL combo is used, but shoudl not occur too often and makes it very explicit that :working-dir should be tied to CLJS version. Also developers utilizing ClojureScript though lein checkouts will still have to delete .repl/ since clojurescript-verion is only set by script/build.

See attached patch.

NOTE: I do not have a CA agreement on file, but one is in the mail.

NOTE: Sorry if this is bad form, but as a preceding commit, in cases where clojurescript-version is unbound I changed (clojurescript-version) to return "" instead of ". This is so that when clojurescript-version is unbound .repl/ will be used instead of .repl-./ Let me know if this should be considered as a separate issue.

Alternatively, we could remove the exists check entirely, and instead recompile client.js every time (repl-env) is called at the cost of slowing down browser REPL startup.






[CLJS-748] Dump analyzer state during a compilation. Created: 13/Jan/14  Updated: 16/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Trakhman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler

Attachments: Text File CLJS_748.patch    
Patch: Code

 Description   

CLJS doesn't have the luxury of a fully-reified runtime environment, therefore effective tooling needs a view of the internal compiler state. This issue addresses this need by enabling a dump of the compiler state based on additional cljs compiler options.

The compiler state is a map contained within an atom, either passed in as the 'compiler-env' arg to cljs.closure/build or contained globally within cljs.env/compiler.

The prototype is implemented as such:

:dump-analysis-to
either a string or :print, when :print, output will be written to out, when a string, the argument will be passed to clojure.java.io/writer.

:dump-analysis-full
checked for truthiness, default is false
When this is switched on, the full structure of the compiler state will be printed, which is impractically large for most use cases. In normal operation, :env keys and extended :method-params will be removed from the output, making the analysis represent simply the globally reachable environment. Additionally, only the contents under :cljs.analyzer/namespaces and :cljs.analyzer/constants will be printed.



 Comments   
Comment by Gary Trakhman [ 13/Jan/14 6:46 PM ]

Added implementing patch

Comment by David Nolen [ 16/Jan/14 5:21 PM ]

I question the utility of :dump-analysis-full for now. Lets remove it.

Comment by Gary Trakhman [ 16/Jan/14 5:32 PM ]

I was thinking it might be good along with :print followed by a pipe, but... I don't have an explicit use-case. I'll create a new patch.





[CLJS-747] Numerical inconsistency between Clojure and ClojureScript (running in Rhino) Created: 11/Jan/14  Updated: 16/Jan/14  Resolved: 16/Jan/14

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

Type: Defect Priority: Minor
Reporter: Jim Pivarski Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: integer-overflow
Environment:

Clojure (1.5.1) on JVM vs. ClojureScript (cloned from GitHub today) in Rhino on JVM



 Description   

In Clojure, the maximum long value throws an ArithmeticException when incremented with + and rolls over to a BigInt when incremented with +'

user=> (+ 9223372036854775807 1)
ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1388)
user=> (+' 9223372036854775807 1)
9223372036854775808N

But ClojureScript rolls over to BigInt with + and does not have a +' function

ClojureScript:cljs.user> (+ 9223372036854775807 1)
9223372036854776000N
ClojureScript:cljs.user> (+' 9223372036854775807 1)
WARNING: Use of undeclared Var cljs.user/+' at line 1 
"Error evaluating:" (+' 9223372036854775807 1) :as "cljs.user._PLUS__SINGLEQUOTE_.call(null,9223372036854775807,1)"
org.mozilla.javascript.EcmaError: TypeError: Cannot call method "call" of undefined (<cljs repl>#1)
        at <cljs repl>:1 (anonymous)
        at <cljs repl>:1

In fact, the inexactness of 9223372036854776000N suggests the reason: it's actually a double, presented as though it were a BigInt. (Perhaps that's a limitation of JavaScript? If so, at least it should be presented as a double, as very large numbers are: e.g. (* 9223372036854775807 9223372036854775807) --> 8.507059173023462E37.) From http://stackoverflow.com/questions/8664093/not-getting-integer-overflow-in-clojure , it is my understanding that this behavior was defined in Clojure version 1.3--- is ClojureScript a few versions behind in that it attempts roll-over and does not have a +' function?



 Comments   
Comment by Jim Pivarski [ 11/Jan/14 3:53 AM ]

That is, ClojureScript doesn't implement http://dev.clojure.org/display/design/Documentation+for+1.3+Numerics

Comment by Jozef Wagner [ 11/Jan/14 5:54 AM ]

If I understand your description correctly, the issue is that Clojurescript does not have BigInts and precise/unchecked variants of arithmetic functions. Please read this bigint discussion and design page for some background information about this.

Comment by Jim Pivarski [ 11/Jan/14 1:43 PM ]

You're right--- I misunderstood ClojureScript's interoperability policy. I'm considering Clojure/ClojureCLR/ClojureScript/ClojureC/Clojure-Py for cross-platform numeric processing, so I've started testing them for numerical compatibility at the edge cases. At first, this looked like a serious bug (no exception, inexact result) until I figured out that it's just double precision. For my purposes, I could say that the largest integer with cross-platform support is 9223372036854775000, and above that, Clojure* may throw an exception, roll over to BigInt, or inexactly represent it as a double, depending on platform.

I'd like to convert this into a minor feature request for BigInt and +' functions or close this ticket and open a new one, but it doesn't look like I have permission to change its resolution state.

I researched the problem before submitting this ticket, but I didn't find the references that you sent (very helpful, thanks!). Should I continue to report edge cases of incompatibility between Clojure and ClojureScript? For my own purposes, I need to know the ranges within which to expect compatibility, so I'm building up a test suite with an emphasis on numerical processing. (And I'm only checking Clojure, ClojureScript, and Clojure-py for now.) If it's helpful, I'll submit bug reports here; if it's annoying, I won't.

Comment by Jozef Wagner [ 12/Jan/14 4:12 AM ]

Numbers are one of few things which are not consistent in Clojure across hosts. This is a design decision in order to provide fast numerics for hosts which have one. A host independent numeric API would preclude many optimizations. Another outcome of this design decision is that the number types are not user extensible. Even basic arithmetic functions may behave differently in edge cases in different hosts. ClojureScript may get BigInt if someone finds time to implement them, but I doubt they will behave exactly the same as in JVM. You'd better accept that numbers will always be host dependent and code according to that.

Comment by David Nolen [ 16/Jan/14 5:23 PM ]

This will require a considerable amount of design work. Until that happens there's not much point opening tickets.





[CLJS-746] clojure.string/replace pattern/function of match API difference with clojure version Created: 10/Jan/14  Updated: 10/Jan/14

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

Type: Defect Priority: Minor
Reporter: Curtis Gagliardi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

[org.clojure/clojurescript "0.0-2138"]



 Description   

When calling clojure.core/replace with a pattern and a function, the Clojurescript version delegates to Javascript's s.replace method, which calls that function with a variable number of arguments, depending on how many match groups are in your pattern. The Clojure version always calls it with a single argument, which may be a vector if you have match groups in your pattern.

I'm not sure if this was intentional. If it wasn't, I think this difference could be fixed through some use of re-find, which appears to return the same string or vector that you'd get in Clojure. If this is intentional for performance reasons, perhaps the doc string should be updated to note this, as there's no warning that the function is being called with too many arguments.



 Comments   
Comment by Curtis Gagliardi [ 10/Jan/14 1:32 AM ]

Afraid I don't see how to edit, but I wanted to include a simple example:

CLJS:
(clojure.string/replace "hello world" #"(hello) world" (fn [m] (.log js/console (str "Match: " m)) m))

will log: "Match: hello world"

CLJ
user=> (clojure.string/replace "hello world" #"(hello) world" (fn [m] (println (str "Match: " m) m)))
Match: ["hello world" "hello"] [hello world hello]

NullPointerException java.util.regex.Matcher.quoteReplacement (Matcher.java:655)





[CLJS-745] Support keywords and namespaced keywords in destructuring Created: 06/Jan/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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: None


 Description   

http://dev.clojure.org/jira/browse/CLJ-1318, slated for 1.6



 Comments   
Comment by David Nolen [ 23/Feb/14 4:52 PM ]

fixed https://github.com/clojure/clojurescript/commit/95d60acf610b255dbb4079ccf60739c81a611922





[CLJS-744] ISequential types should implement JS indexOf, lastIndexOf Created: 05/Jan/14  Updated: 05/Jan/14

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-743] Allow language_in and language_out options to be passed to the Google Closure compiler Created: 04/Jan/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Ivan Willig Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-743.patch    

 Description   

While I was messing around with ClojureScript and nodejs, I
noticed that the Google Closure compiler complains when JavaScript
reserved words are used. Newer versions of JavaScript engines allow
reserved words (like "static", "class") to be used as object
properties. The Closure Compiler knows this and does not complain when
you set the language_in option to the correct version of JavaScript.



 Comments   
Comment by David Nolen [ 07/Jan/14 7:07 AM ]

Excellent thanks.

Comment by David Nolen [ 09/Jan/14 5:17 PM ]

Ivan have you signed and sent in your CA?

Comment by Chas Emerick [ 20/Feb/14 9:23 AM ]

@iwillig Looking forward to seeing this, def send in that CA.

Comment by Ivan Willig [ 20/Feb/14 10:06 PM ]

I apologize for letting this slip, work got busy. CA is in the mail.

Comment by David Nolen [ 23/Feb/14 4:33 PM ]

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





[CLJS-742] Compilation with :output-file option set fails Created: 03/Jan/14  Updated: 03/Jan/14

Status: In Progress
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Timothy Pratley Assignee: Timothy Pratley
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File CLJS-742.diff    

 Description   

./bin/cljsc hello.cljs '{:output-file "foo.js"}'

Expected:
a file called foo.js

Actual:
Compiler throws an internal error

Notes:
The correct option is :output-to
:output-file might be removable (it is broken, nobody is using it?)
Further analysis required to determine if it is useful to keep.



 Comments   
Comment by Timothy Pratley [ 03/Jan/14 12:14 PM ]

Fixes handling of compiling one IJavaScript or many





[CLJS-741] seq error message readability is not optimal Created: 02/Jan/14  Updated: 02/Jan/14

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

Type: Enhancement Priority: Trivial
Reporter: Julien Eluard Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File cljs_741.patch    

 Description   

When calling seq on an invalid type an errr with following message is thrown: :keywordis not ISeqable.

Adding a space between the argument and the message improves the readability.



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

Excellent thanks!





[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-739] fn/recur bug Created: 01/Jan/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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: None


 Description   
(defn create-functions [arr names]
  (let [name (first names)]
    (if name
      (recur (conj arr (fn [] (println name)))
             (rest names))
      arr)))
 
(doseq [fn (create-functions [] [:a :b :c :d])] (fn))

Generates the following incorrect code, the local name is not closed over:

cljs_play.let_bug.core.create_functions = function create_functions(arr, names) {
  while (true) {
    var name = cljs.core.first.call(null, names);
    if (cljs.core.truth_(name)) {
      var G__4744 = cljs.core.conj.call(null, arr, function(arr, names) {
        return function() {
          return cljs.core.println.call(null, name);
        };
      }(arr, names));
      var G__4745 = cljs.core.rest.call(null, names);
      arr = G__4744;
      names = G__4745;
      continue;
    } else {
      return arr;
    }
    break;
  }
};


 Comments   
Comment by David Nolen [ 01/Jan/14 9:43 PM ]

This is because we don't do two passes to determine if a function uses recur. loop/recur works because we know up front.

Comment by David Nolen [ 23/Feb/14 6:04 PM ]

I actually can't reproduce this in master - I've added tests to confirm https://github.com/clojure/clojurescript/commit/aaae2688aa2646bb252a9a4dd321e9fa8dbedb6a





[CLJS-738] port clojure.data Created: 31/Dec/13  Updated: 01/Jan/14  Resolved: 01/Jan/14

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

Type: Task Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Comments   
Comment by Francis Avila [ 31/Dec/13 9:46 PM ]

Isn't this already done? CLJS-378 data.cljs

Comment by David Nolen [ 01/Jan/14 9:55 AM ]

Oh wow, was looking for the wrong file thanks!

Comment by David Nolen [ 01/Jan/14 9:55 AM ]

Already done.





[CLJS-737] tool.reader can't handle white-space Created: 30/Dec/13  Updated: 09/Jan/14  Resolved: 09/Jan/14

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

Type: Defect Priority: Minor
Reporter: Andrew Zures Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug
Environment:

MBP running lion 10.7.5


Attachments: Text File 0001-Bump-tools.reader-version-to-0.8.3.patch     PNG File Screen Shot 2013-12-30 at 4.40.47 PM.png     PNG File Screen Shot 2013-12-30 at 4.41.14 PM.png     PNG File Screen Shot 2013-12-30 at 4.44.42 PM.png    

 Description   

I believe the tool.reader (may some other part of ClojureScript) cannot handle a file with a large amount of white space. I have a cljx generated file with nothing but a namespace and about 300-400 lines of white-space. The entire file is simply a namespace and whitespace (file attached shows white-space in red). I get a stackoverflow error arising mainly from clojure.tools.reader$read.invoke(reader.clj:727). I have screenshots of the beginning and end of the stackoverflow attached. If I remove the whitespace, ClojureScript will compile, which leads me to believe it is the white space that is the cause of the issue.



 Comments   
Comment by Nicola Mometto [ 30/Dec/13 5:13 PM ]

Updating to tools.reader >=0.8.1 should fix this issue.

Comment by Nicola Mometto [ 30/Dec/13 5:21 PM ]

Attached a patch that bumps tools.reader to the lastest version (0.8.3)

Can you apply this and verify that it solves the issue?

Remeber you'll have to clean the lib/ folder form old tools.reader jars.

Comment by David Nolen [ 09/Jan/14 5:17 PM ]

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





[CLJS-736] Functions folder and reducer broken for types nil and array + fix for typo Created: 29/Dec/13  Updated: 18/Jan/14

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

Type: Defect Priority: Major
Reporter: Jonas De Vuyst Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-736-alt.patch     Text File CLJS-736.patch     Text File CLJS-736-patch-1-redux.patch     Text File CLJS-alt-satisfies.patch    
Patch: Code and Test

 Description   

1. This currently doesn't work:

(->> nil
(r/map identity)
(r/reduce + 0))
; org.mozilla.javascript.JavaScriptException: Error: No protocol method IReduce.-reduce defined for type null

The reason for this is that reducers created by r/reducer or r/folder, invoke -reduce (of IReduce) directly. They thereby bypass the special case for nil in the function r/reduce.

2. An entirely analogous problem exists for collections of type array.

3. The patch CLJS-700 mistakenly defined coll-fold for the type cljs.core/IPersistentVector. This should have been cljs.core/PersistentVector. (There exists no protocol IPersistentVector in ClojureScript.)

I will shortly attach a patch that addresses all of the above problems by implementing IReduce for nil and array. The patch also includes unit tests.



 Comments   
Comment by Jonas De Vuyst [ 29/Dec/13 2:22 PM ]

Alternative patch in which r/reduce and r/fold treat arrays and nil as special cases – as opposed to having arrays and nil implement IReduce and CollFold.

The functions r/reducer, r/folder, and the protocol methods of r/Cat now call r/reduce and r/fold instead of calling -reduce and coll-fold directly.

This patch also fixes a bug in the coll-fold implementation for Cat, which previously used (reducef) as the initial value rather than (combinef). The new code is copied and pasted from the Clojure implementation and uses the fork-join stubs.

Comment by David Nolen [ 30/Dec/13 8:23 AM ]

The implements? should probably be a satisfies? in the second patch. Have you run any benchmarks of before/after the patch?

Comment by Jonas De Vuyst [ 30/Dec/13 11:24 AM ]

If I understand correctly then (satisfies? x y) is roughly equivalent to (or (implements? x y) (natively-satisfies? x y)).

If native types (nil, array, object currently) are treated as special cases then implements? seems more appropriate.

satisfies? works also, however, so I have attached a new 'alt' patch.

Comment by Jonas De Vuyst [ 30/Dec/13 11:26 AM ]

The first patch is in fact faster when running the following code:

(time (->> (repeat 1000 (vec (range 1000)))
vec
(r/mapcat identity)
(r/map inc)
(r/filter even?)
(r/fold +)))

This takes about 700 msecs. Using the first patch this terminates 100-300 msecs faster. This is after repeated (but informal) testing.

I guess the worry is that the first patch would slow down other random code since it involves extending the types nil, array, and object. I'm not sure what exactly I should test for though.

(Note that the 2nd and 3rd patch also contain a fix for Cat and include more unit tests. The first patch should preferably not be applied as-is.)

Comment by David Nolen [ 30/Dec/13 11:35 AM ]

Yeah you're timing too many things, including vec, range, lazy sequences. Also testing a small N. Take a look at the reducers example on the Mori README - https://github.com/swannodette/mori. Thanks.

Comment by Jonas De Vuyst [ 30/Dec/13 12:52 PM ]

I tried running the following code:

(let [coll (vec (repeat 1000 (vec (range 10))))]
  (time (doseq [n (range 1000)]
               (->> coll
                    (r/mapcat identity)
                    (r/map inc)
                    (r/filter even?)
                    (r/fold +)))))

Some of the last results I got were:

1st patch: 75680 msecs
2nd patch: 76585 msecs

Truth be told, although the first patch seemed to win most of the times, sometimes the second patch was faster.

One other thing I tried was removing the implements?/satisfies? check from the second patch and overriding the protocol method coll-fold for the type object instead (as in the first patch). This 'hybrid' approach generally (but not always) seemed to result in a slowdown.

I'm not sure how I should proceed. Should I perhaps just run both patches simultaneously for several minutes?

Comment by David Nolen [ 30/Dec/13 1:21 PM ]

This is still a bad way to do timing, you're recording the cost of range and seq'ing. Use dotimes.

Comment by Jonas De Vuyst [ 30/Dec/13 4:33 PM ]

Hm. I guess the lazy sequence does lead to a lot of allocations.

Alright, I rewrote my test and ran it a few more times. I now also tested on both vectors and arrays.

Patch 1 needed a slight tweak. When coll-fold is invoked, patch 1 only specifies a fallback for type object (i.e. r/reduce is called). I had to add the same fallback for type array. (This is weird!)

So here are the results.

For vectors:

(let [coll (vec (repeat 100 (vec (range 100))))]
  (time (dotimes [n 3000]
          (->> coll
              (r/mapcat identity)
              (r/map inc)
              (r/filter even?)
              (r/fold +)))))

Patch 1: 205872 msecs
Patch 2: 210756 msecs

For arrays:

(let [coll (into-array (repeat 100 (into-array (range 100))))]
  (time (dotimes [n 3000]
          (->> coll
              (r/mapcat identity)
              (r/map inc)
              (r/filter even?)
              (r/fold +)))))

Patch 1: 123567 msecs
Patch 2: 119704 msecs

I ran my tests a few times and the results were pretty consistent. Patch 1 is faster for vectors and patch 2 is faster for arrays.

This makes sense.

In patch 1 reducer will call -reduce directly. In patch 2, reducer first calls r/reduce, which calls -reduce if the collection is a vector and array-reduce if it's an array. Hence patch 2 contains an extra function call in the case of vectors, but avoids invoking a protocol method on a native type in the case of arrays.

Using macros (or copy and paste) the extra function call can be avoided. Would that be worth trying or is it more important to keep the code clean?

I just realized that patch 2 is semantically slightly different from what Clojure does, although perhaps this is a bug in Clojure: <https://groups.google.com/forum/#!searchin/clojure-dev/kv-reduce/clojure-dev/bEqECvbExGo/iW4B2vEUh8sJ>. My suggestion to use a macro (or copy and paste) to avoid the extra function call in patch 2, could also fix this discrepancy.

Comment by David Nolen [ 30/Dec/13 4:42 PM ]

How are you benchmarking this? With V8? JavaScriptCore? SpiderMonkey? In the browser? What optimization settings, etc.

Comment by Jonas De Vuyst [ 30/Dec/13 4:48 PM ]

I used repljs (Rhino?). I'll test again in a more realistic setting tomorrow.

Comment by David Nolen [ 30/Dec/13 4:54 PM ]

Yeah, benchmarking with Rhino isn't informative.

Comment by Jonas De Vuyst [ 31/Dec/13 1:40 AM ]

I compiled the same code (with n=3000) using cljs with "{:optimizations :advanced}".

I then tested it in the latest stable releases of Firefox, Chrome, and Safari. I closed all my browsers. For each browser I then followed the following procedure:

  • Open the browser
  • Open the developer console
  • Run the benchmark for patch 1
  • Run the benchmark for patch 2
  • Run the benchmark for patch 1 and write down the result
  • Run the benchmark for patch 2 and write down the result
  • Close the browser

Firefox:

  • Patch 1. Vectors: 26057 msecs
  • Patch 1. Arrays: 25026 msecs
  • Patch 2. Vectors: 26258 msecs
  • Patch 2. Arrays: 36653 msecs
  • Summary: Patch 1 is faster for vectors and arrays

Chrome:

  • Patch 1. Vectors: 7804 msecs
  • Patch 1. Arrays: 7092 msecs
  • Patch 2. Vectors: 7754 msecs
  • Patch 2. Arrays: 6768 msecs
  • Summary: Patch 2 is faster for vectors and arrays

Safari:

  • Patch 1. Vectors: 167230 msecs
  • Patch 1. Arrays: 108780 msecs
  • Patch 2. Vectors: 173940 msecs
  • Patch 2. Arrays: 110012 msecs
  • Summary: Patch 1 is faster for vectors and arrays

I'm not sure what to make of this.

Comment by Jonas De Vuyst [ 31/Dec/13 2:47 AM ]

I have attached a new version of the first patch.

This patch fixes an issue with r/Cat. (This issue was also addressed in the second and third patch. A unit test is included.).

This patch also fixes r/fold for arrays.

To summarize, a choice needs to be made between the following patches.

  • CLJS-736-patch-1-redux.patch
  • CLJS-736-alt.patch (uses implements?) / CLJS-alt-satisfies.patch (uses satisfies?)

The implementation details are patch-1-redux is more similar in spirit to the Clojure source code. The alt patches are more similar in spirit to the ClojureScript source code.

As explained above, the alt patches are semantically a bit different from the original Clojure source—but it's not clear which behavior is 'right'.

Comment by David Nolen [ 16/Jan/14 5:27 PM ]

The benchmarks would be more informative if they explained the performance before and after that patch.

Comment by Jonas De Vuyst [ 18/Jan/14 11:55 AM ]

r/reduce previously didn't work for nil or JavaScript arrays.

One reason why I have trouble recommending a patch is that I don't know what use case you would like to optimize for.

Comment by David Nolen [ 18/Jan/14 12:30 PM ]

Yes but now that we have new logic we can at least test the lack of regression on the other types.

Comment by David Nolen [ 18/Jan/14 12:40 PM ]

Ok I tried to apply this patch and run ./script/benchmarks in the repo but the patch will no longer apply. Can we rebase the patch on master. Thanks. If you also want to give the benchmarks a shot follow these instructions to install the JS engines - http://github.com/clojure/clojurescript/wiki/Running-the-tests. Then you can also run the benchmarks at the command line. I see there aren't any reducers benchmarks, I will add some.





[CLJS-735] Set literal and hash-set cleanup Created: 28/Dec/13  Updated: 30/Dec/13  Resolved: 30/Dec/13

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

Type: Defect Priority: Minor
Reporter: Logan Campbell Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Clojurescript 0.0-2127


Attachments: Text File cljs-735-dead-code-cleanup.patch    

 Description   

Some sets drop values, causing loss of data.

The smallest example I've been able to find is:

#{[1 4] [2 4] [3 4] [0 3] [1 3] [2 3] [3 3] [3 2] [4 2]}
Drops: [1 4] [3 4] [1 3] [3 3]

hash-set also drops values. Though they are different.

(hash-set [1 4] [2 4] [3 4] [0 3] [1 3] [2 3] [3 3] [3 2] [4 2])
Drops: [2 4] [0 3] [2 3] [3 2]

Re-ordering the values appears to make no difference.

I have found no instance where sorted-set drops values.



 Comments   
Comment by Francis Avila [ 29/Dec/13 6:04 PM ]

This is a bug in PersistentHashSet.fromArray that causes every other item to be skipped when the input is longer than the PersistentArrayMap.HASHMAP_THRESHOLD (which is 8). Demonstration:

ClojureScript:cljs.user> (hash-set [0] [1] [2] [3] [4] [5] [6] [7] [8])
#{[2] [4] [6] [8] [0]}
ClojureScript:cljs.user> (hash-set [0] [1] [2] [3] [4] [5] [6] [7])   
#{[1] [2] [3] [4] [5] [6] [7] [0]}

Working on a patch and test.

Comment by Francis Avila [ 29/Dec/13 6:11 PM ]

Nevermind, looks like David fixed this already.

Comment by Francis Avila [ 29/Dec/13 8:02 PM ]

Some dead code was left in PersistentHashSet.fromArray (no-clone arg and arr let-binding). Attached a patch that cleans this up.

Comment by David Nolen [ 30/Dec/13 8:51 AM ]

Thanks for the patch but remember we need CAs to apply them.

The current hash-set construction implementation misses an optimization opportunity. We could build hash-sets much faster if we know that we have only unique constants at compile time.

Comment by David Nolen [ 30/Dec/13 8:52 AM ]

Need to implement the hash-set construction optimization, the old removed approach is faster when we know we have a set literal containing only constants.





[CLJS-734] Add multi-arg versions of conj! disj! assoc! and dissoc! Created: 26/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File bench-cljs-734.txt     File benchmark_runner.cljs     Text File bench-master.txt     Text File bench-variadic.txt     Text File cljs-734-2.patch    
Patch: Code

 Description   

The transient collection manipulation functions conj! disj! assoc! and dissoc! don't have an "& rest" argument form, so it is not possible to write for example (conj! t-vec :a :b).

Besides being inconvenient, this is also different from Clojure.

Patch attached.



 Comments   
Comment by David Nolen [ 26/Dec/13 12:47 PM ]

Please include a properly formatted squashed patch that includes tests -https://github.com/clojure/clojurescript/wiki/Patches. Thanks!

Comment by Francis Avila [ 26/Dec/13 4:29 PM ]

Patch amended, tests added.

Comment by Jozef Wagner [ 28/Dec/13 1:36 PM ]

Let's step back a little. Transient variants of conj, dissoc, etc. were created just for performance, not convenience. Clojure version of conj! DOES NOT support multi arg version and IMO it should not, as the transient variants should do one thing and do it fast, and not fiddle with seqs and deconstruct... I would go as far as to propose to remove multi arg versions from Clojure, rather than adding them in CLJS.

Comment by Francis Avila [ 28/Dec/13 4:39 PM ]

I didn't realize conj! wasn't variadic in Clojure--assoc! dissoc! and disj! are though. I would rather patch Clojure's conj! to add a variadic form.

I think we should support variadic forms of these functions because their non-transient counterparts do. Code typically starts out non-transient and then becomes transient later. If conj/conj! assoc/assoc! etc support the same function signatures then getting a working transient version is trivial--wrap the initial structure with transient, the result with persistent!, and add bangs to your conj/disj/assoc/dissoc names. It's an unnecessary annoyance to not have a variadic form in this case. This is precisely the annoying scenario that I've encountered enough times to drive me to write this patch.

We lose nothing by having a variadic form--if you want to avoid messing with seqs in your transient code you still can by calling the non-variadic form. But if you're using transients directly you're almost certainly have a seq somewhere in your code path anyway, so what's the harm in letting conj!/disj! etc consume it from the inside instead of forcing the user to do it on the outside.

Comment by David Nolen [ 30/Dec/13 8:25 AM ]

It would be nice to see actual performance numbers. If the performance difference is minor under V8 and JavaScriptCore I'm inclined to merge in this simple enhancement and consistency patch.

Comment by Francis Avila [ 04/Jan/14 2:02 AM ]
  • Patch updated:
    • Made variadic assoc! faster
    • rebased to latest master.
  • Wrote a benchmark, benchmark_runner.cljs, which I run using script/benchmark. Results on my machine attached: linux with x64 v8 and spidermonkey 17 (no JSCore benches).
    • On V8, there is no difference between master and my patch for non-variadic calls.
    • On spidermonkey, non-variadic vec assoc!, set disj!, and map dissoc! are slower with the patch; the rest are the same. I can't explain why!
    • The variadic calls (using apply) are in some cases faster than loop/recur with non-variadic calls, which I also can't understand.
Comment by David Nolen [ 07/Jan/14 7:08 AM ]

Can we remove the old patches thanks.

Comment by Francis Avila [ 07/Jan/14 8:17 AM ]

Actually I don't think I can, or at least I see no way to remove them in JIRA. The latest patch is cljs-734-2.patch

Comment by David Nolen [ 07/Jan/14 8:19 AM ]

K thanks, I've cleaned up the stale ones.

Comment by David Nolen [ 23/Jan/14 8:08 AM ]

fixed, https://github.com/clojure/clojurescript/commit/8b2ed1d57b8904341727d4b5b251c8815e618a10





[CLJS-733] Implement printing & equality for the JSValue type Created: 25/Dec/13  Updated: 26/Dec/13

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

Type: Enhancement Priority: Trivial
Reporter: Roman Scherer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File js-value-print.diff     File js-value-print-only.diff    
Patch: Code and Test

 Description   

Using the JSValue type in Clojure tests is a little bit cumbersome at
the moment. The following test for example is not passing at the
moment, because equality is not defined on JSValue.

(is (= '(js/React.DOM.div #js {})
'(js/React.DOM.div #js {})))

It would be nice if the JSValue type implements at least equality and
tagged literal printing on the Clojure side as well. The attached
patch implements this functionality.



 Comments   
Comment by David Nolen [ 25/Dec/13 11:15 AM ]

The equality test doesn't match equality semantics in JavaScript. So while this is convenient, this is going to need a really strong argument. I'm inclined to just say no.

Printing for JSValue is OK with me.

Comment by Roman Scherer [ 25/Dec/13 12:40 PM ]

Ok, this solves half of my problem. My strong argument for the
equality test would be "but JSValue lives in Clojure land, and
consumers (analyzer, compiler, tests) of JSValue are better served
with the same equality semantics that Clojure already provides". My
use case for this is over here:

https://github.com/r0man/sablono/blob/js-literal/test/sablono/compiler_test.clj#L18

The sablono compiler generates forms that can contain JSValues. Those
forms I need to check for equality in my tests. Ok, I can define my
own equality operator that walks the forms and replaces JSValue with
something else, but that feels really strange. Any other idea?

Can you make an example why implementing equality this way would be
problematic? I think I didn't get your point.

If you are still against it, I'm happy to provide a patch for the
print functionality. That's the one I really need. Unfortunately this
one I could have provided myself, the equality thing not.

Comment by David Nolen [ 26/Dec/13 8:43 AM ]

Consider if we bootstrapped and JSValue disappeared and we could use the JS Array type directly instead. But arrays are not equal in Clojure(Script) because they are not values.

Comment by Roman Scherer [ 26/Dec/13 8:50 AM ]

Ok. Thanks for the explanation.





[CLJS-732] Compilation a lot slower in 0.0-2127 compared to 0.0-1978. Created: 21/Dec/13  Updated: 26/Dec/13  Resolved: 26/Dec/13

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

Type: Defect Priority: Minor
Reporter: William Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Linux, org.clojure/clojurescript "0.0-2127"


Attachments: File project.clj    

 Description   

Using cljsbuild auto: the compilation times I had with 0.0-1978:
21.705943966 seconds. (first file modification)
6.565231432 seconds. (stays stable for future modifications)

With 0.0-2127:
31.177024278 seconds. (first file modification)
10.492762657 seconds. (stays stable for future modifications)

I don't know if at some point a feature justifies the slow down, or if this is unexplained and hence should be treated as a bug.

The project I'm compiling is around 1138 lines of code (wc -l) spread over 15 files.



 Comments   
Comment by David Nolen [ 23/Dec/13 1:39 PM ]

Is this with or without source maps enabled?

Comment by William [ 24/Dec/13 4:21 AM ]

With source maps, targeting node.

Comment by William [ 24/Dec/13 8:31 AM ]

project.clj used for the project, might be useful.

Comment by David Nolen [ 25/Dec/13 11:18 AM ]

Source maps slow things down. There have been a variety of changes that make source maps more accurate and more compatible with incremental compilation which no doubt slows things down.

So no I would not consider this a bug. Certainly there are further enhancements we could make that would improve compile times significantly, like caching analysis results to disk, etc.

Comment by David Nolen [ 26/Dec/13 12:22 PM ]

I've improved the performance of master a bit by avoiding redundant analysis passes, https://github.com/clojure/clojurescript/commit/2a2e5df90b51093bc5476cf797f9cfd489e0c83e https://github.com/clojure/clojurescript/commit/2267322cd9c14c7bbe3480051638ca023c4864c8





[CLJS-731] clojure.walk/walk does not descend into js-arrays or js-objects Created: 21/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

r2127



 Description   

The clojure.walk/walk function does not descend into plain javascript objects and arrays--it'd be nice if it did using the same semantics as other collections.

I needed this functionality and ended up creating my own walk function. With a small patch I could add this to the native clojure.walk/walk, but I wanted to test interest here first.

Possibly the right thing to do instead is to make js-obj and array seqable?



 Comments   
Comment by David Nolen [ 23/Dec/13 1:40 PM ]

I'd rather see ClojureScript's walk be protocol-ized so that people can always solve this problem themselves.

Comment by David Nolen [ 23/Jan/14 8:08 AM ]

Closing.





[CLJS-730] (object? nil) throws TypeError exception Created: 21/Dec/13  Updated: 23/Dec/13  Resolved: 23/Dec/13

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

r2127


Attachments: Text File fix-object-nil-exception.patch    
Patch: Code and Test

 Description   

Some primitive types and host objects throw an exception on property access. I know null does this, but I see reports that window.location on Firefox does this too, possibly other objects.

This is a problem because the implementation of object? is

o.constructor === Object{/code}, which causes
(object? nil)

to throw a TypeError exception. Suggested patch attached.

The patch guards the constructor test with a try-except clause. I don't know what the performance implications of this are, but object? is only used once in cljs core (in pr-writer, in a giant cond, where it could be moved further down), so it probably doesn't matter much.

(Note: I don't have a CA yet--it's in the mail.)



 Comments   
Comment by David Nolen [ 23/Dec/13 1:51 PM ]

fixed, https://github.com/clojure/clojurescript/commit/78801e5d5f6e6e1f39b7f3a50fdaeb104eda3296





[CLJS-729] strange extend protocol to to native case Created: 17/Dec/13  Updated: 17/Dec/13  Resolved: 17/Dec/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Copied from gist: http://gist.github.com/cemerick/7998162

; using 2120

(defprotocol P (m [x]))

(extend-protocol P
  number
  (m [x] (- x)))

(set! (.-foo js/Number.prototype)
      #(this-as this (m this)))

cljs.user> (def j 5)
5
cljs.user> (m j)
-5
cljs.user> (.foo j)
"Error evaluating:" (.foo (inc 5)) :as "(5 + 1).foo();\n"
#<Error: No protocol method P.m defined for type object: 6>
Error: No protocol method P.m defined for type object: 6
    at :17
    at m (:20)
    at :1
    at :1
    at :5
nil
;; no idea why, but producing the same number via parsing causes the dispatch to succeed

(set! (.-foo js/Number.prototype)
      #(this-as this
         (m (js/parseFloat (str this)))))

cljs.user> (.foo j)
-5
(ns prototype-dispatch)

(defprotocol P (m [x]))
 
(extend-protocol P
number
(m [x] (- x)))
 
(set! (.-foo js/Number.prototype)
      #(this-as this (m this)))

(def j 5)

(defn ^:export -main [& args]
  (.log js/console (m j))
  (.log js/console (.foo j)))
;(set! *main-cli-fn* -main)

(try
  (-main)
  ((aget js/phantom "exit") 0)
  (catch js/Error e
    (.log js/console e)
    ((aget js/phantom "exit") 1)))


 Comments   
Comment by David Nolen [ 17/Dec/13 7:53 AM ]

not going to fix this. Inside of the prototype fn {this} will be the prototype not the Number instance.





[CLJS-728] (get [1 2 3] nil) -> 1 Created: 16/Dec/13  Updated: 24/Feb/14  Resolved: 24/Feb/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: File cljs-728-minimal.diff     Text File cljs-728-more-minimal.patch     Text File cljs-728-nth-number.patch     Text File cljs-728-only-nth-lookup.patch     Text File cljs-728-only-nth-lookup.patch    

 Comments   
Comment by Francis Avila [ 26/Dec/13 4:49 PM ]

This is due to javascript coercing null to 0 in the various places where -assoc and -nth check (<= 0 n) or (bit-and n MAGIC-NUMBER). E.g. in PersistentVector:

IIndexed
  (-nth [coll n]
    (aget (array-for coll n) (bit-and n 0x01f)))
  (-nth [coll n not-found]
    (if (and (<= 0 n) (< n cnt))
      (-nth coll n)
      not-found))

I can track all these down and patch them by changing the guards to (and (not (nil? n)) (<= 0 n) (< n cnt)) (or maybe even (or (zero? n) (< 0 n cnt))) and adding (when-not (nil? n) ...) guards where appropriate.

However I wasn't sure if there's any intention to make the comparison operators <= < > >= null-safe. I.e. instead of adding these extra nil checks, should we instead make sure comparing against nil always returns false or throws an exception? (We would still need nil checks to make sure (get [1] nil) returns nil like Clojure.)

Comment by Francis Avila [ 26/Dec/13 11:26 PM ]

Currently failing test cases.

(assert (nil? (get [1 2] nil)))
(assert (= :fail (try (nth [1 2] nil) (catch js/Error e :fail))))
(assert (= 4 (get [1 2] nil 4)))
(assert (= 4 (nth [1 2] nil 4)))
(assert (= :fail (try (assoc [1 2] nil 4) (catch js/Error e :fail))))

(assert (nil? (get (transient [1 2]) nil)))
(assert (= :fail (try (nth (transient [1 2]) nil) (catch js/Error e :fail))))
(assert (= 4 (get (transient [1 2]) nil 4)))
(assert (= 4 (nth (transient [1 2]) nil 4)))
(assert (= :fail (try (assoc! (transient [1 2]) nil 4)
                   (catch js/Error e :fail))))

(assert (nil? (get (subvec [1 2] 1) nil)))
(assert (= :fail (try (nth (subvec [1 2] 1) nil) (catch js/Error e :fail))))
(assert (= 4 (get (subvec [1 2] 1) nil 4)))
(assert (= 4 (nth (subvec [1 2] 1) nil 4)))
(assert (= :fail (try (assoc (subvec [1 2] 1) nil 4)
                   (catch js/Error e :fail))))
Comment by Francis Avila [ 27/Dec/13 2:11 AM ]

Actually this fails for other values that may coerce to zero as well (e.g. false, numeric strings, etc). I solved this with a number? type check in appropriate places. Patch attached with comprehensive tests.

Comment by David Nolen [ 30/Dec/13 8:54 AM ]

Putting number? checks on critical paths is unacceptable for performance reasons. Ideally we only add one number? check. And even then we'd want to know the performance implications.

Comment by Francis Avila [ 30/Dec/13 1:04 PM ]

Is number? really that slow? At least on v8 it seems to be twice as fast as nil?. Some repl checks in v8 don't seem to show a significant performance regression, but without a proper profiling harness and test cases I don't feel comfortable saying that for sure.

I think we can give up some type safety but still catch silent-coerce-to-zero cases by expressing the conditional as (and (< i (.-cnt pv)) (or (< 0 i) (zero? i))). I'll try that later today and try to compare the performance.

Your concern about checking bounds only once can be resolved by moving the guts of array-for into unchecked-array-for and editable-array-for into unchecked-editable-array-for. This is probably a good idea even if we do nothing about (get [1] nil) --at least -pop and -kv-reduce would benefit along with the not-found arities of -nth and -lookup. Perhaps a new ticket for this?

Comment by David Nolen [ 30/Dec/13 1:09 PM ]

No coercions. Any check should be higher up the chain, like at nth and nowhere else.

Comment by Francis Avila [ 30/Dec/13 1:31 PM ]

What do you mean by "no coercions"? Do you mean edge cases like {{(get [1 2] "1") => 2}} are unacceptable even if it means we can avoid a typeof check?

Comment by David Nolen [ 30/Dec/13 1:40 PM ]

No coercions means no coercions. The only valid key to an IVector instance for lookup is a JavaScript number.

Comment by Francis Avila [ 30/Dec/13 1:53 PM ]

OK, then in that case a number? check somewhere is absolutely required, but I can move code around a bit to ensure that no type or bounds checking is done more than once for persistent and transient vectors (there is already a fair amount of redundant bounds checking). I'll work on this tonight.

(I sent a CA about 10 days ago but I see my name isn't on the CA page yet. It's probably just delayed by holiday stuff.)

Comment by Francis Avila [ 31/Dec/13 12:02 AM ]

Updated patch.

  • I've removed all redundant bounds checking that I could easily eliminate using unchecked-array-for in place of array-for.
  • IVector and IIndexed protocol implementations don't have number? checks in them. nth does the check.
  • But the number? check is in the IAssociative and ILookup implementations for these types.
  • The logic here is that for protocols which semantically require a numeric index, we assume the user of the protocol (nth or ci-reduce for example) has already done the necessary type checking on the outside. But for protocols which don't have this semantic (e.g. ILookup), we need to do the check in the protocol implementation.
  • I ran script/benchmark and compared master and this patch on the v8 and spidermonkey engines. I didn't see any performance regressions.
Comment by David Nolen [ 07/Jan/14 7:09 AM ]

Can we remove old patches, thanks.

Comment by Francis Avila [ 07/Jan/14 8:24 AM ]


JIRA won't let me delete them. The latest patch is cljs-728-nth-number.patch

Comment by David Nolen [ 16/Jan/14 5:31 PM ]

This patch does too much. Why are we not just checking in nth?

Comment by Francis Avila [ 16/Jan/14 7:04 PM ]

Remember the problem isn't just (get [1] nil), it's also (get [1] :a) and also on Range, subvectors, and transientvector types (any non-numeric lookup on an indexed type). So a nil? check in nth is nowhere near enough.

For indexed types, get calls -lookup calls -nth (nth is not called), and you didn't want to do a nil? or number? check inside -nth (for good reason, as we can assume callers of -nth know they need to supply a number). This patch puts the number check on indexed types in -lookup (rationale given in my previous comment) and makes sure that other core code is calling -nth safely without checks.

The patch also removes some bounds and type checking that was redundant (the unchecked-array-for and -assoc-n stuff) which I noticed because I had to examine all those calling paths.

I suppose an alternative that would change less code is to have get use IIndexed if present and do the number check there, but then it's impossible for public code to use ILookup if there were a type where there was a difference. (e.g., Suppose an xml-element type wanted to provide access attributes with get and child elements with nth.)

Comment by David Nolen [ 16/Jan/14 10:39 PM ]

The only code that needs to change it seems to me is nth and implementations of ILookup for IIndexed types.

Comment by Francis Avila [ 29/Jan/14 6:49 PM ]

-assoc and -assoc! need fixing too on various types because (assoc [:a] nil :b) => [:b].

I have attached a smaller patch which does not include the hunks related to redundant bounds checking and will move those to CLJS-757. This is the smallest patch which will pass all the tests related to using non-numeric indexes with indexed types.

Comment by David Nolen [ 29/Jan/14 8:26 PM ]

-assoc and -assoc! do not need fixing. Only -assoc-n and -assoc-n! do. The patch needs to be far more minimal if it hopes to get accepted.

Comment by Francis Avila [ 30/Jan/14 11:46 AM ]

It doesn't make sense for indexed types to have -lookup call -nth but -assoc-n call -assoc (as PersistentVector and Subvec do, but not TransientVector). This means if we know we are using a numeric key we call -nth on read, but call -assoc on writes instead of -assoc-n. -assoc-n is what has the required "key is a number" semantic, not -assoc, and this leads to an unnecessary number? check being done in the implementation of Subvec -conj. (There are also other places, e.g. Subvec -assoc, where -assoc-n could be called instead of assoc or -assoc, but I don't address those.)

In other words, it would be best if -assoc-n/-assoc-n! consistently meant "numeric key" 1) so that callers could avoid a number? check if they know their key is numeric, 2) so there is a sane analogy -lookup:-nth :: -assoc:-assoc-n :: -assoc!:-assoc-n!

Comment by Francis Avila [ 30/Jan/14 11:49 AM ]

New patch cljs-728-more-minimal.patch

Hunk-by-hunk justification of changes:

  1. core_test.cljs
    1. test (get x n) (get x n nf) (nth x n) (nth x n nf) and (assoc x n v) with non-numeric n against PersistentVector, Subvec, TransientVector, and Range types.
  2. core.cljs
    1. number? check in nth
    2. number? check in PersistentVector -lookup
    3. number? check in PersistentVector -assoc
    4. number? check in Subvec -lookup
    5. number? check in Subvec -assoc
    6. number? check in TransientVector -assoc!
    7. number? check in TransientVector -lookup

Although it does not swap -assoc-n and -assoc on Subvec and PersistentVector, I would hardly describe this patch as
"far more minimal" than the previous one--I hope that does not mean it is unacceptable. The only way I can see to
make this patch smaller is to file a different ticket for each hunk and corresponding test, which seems crazy.

If the patch is still not acceptable, could you please be much more specific about what exactly is wrong with it so we can close this bug.

Comment by David Nolen [ 30/Jan/14 12:03 PM ]

That the ClojureScript vector types implement assoc in terms of assoc-n and not the other way around should be considered a flaw. Please refer to Clojure's PersistentVector.java, RT.java for a guide about how the logic should be implemented. I'll take a patch that aligns ClojureScript's logic closer to Clojure. Thanks.

Comment by Francis Avila [ 17/Feb/14 11:54 AM ]

Patch that aligns ClojureScript vector type's -assoc-n/-assoc with Clojure's is in CLJS-767, and patch that fixes assoc! on TransientVectors is in CLJS-768.

I will cut conflicting hunks out of this CLJS-728 patch (hunks 2.3, 2.5 and 2.6 as outlined above) so all can apply cleanly with one another. (I'll have to do it this evening though.)

Full list of related tickets and patches (stuff that was factored out of the initial patches):

  1. CLJS-757: remove redundant bounds checking in indexed types (will need a rebase once the others are in).
  2. CLJS-767: Fix assoc on PersistentVector and Subvec
  3. CLJS-768: Fix assoc! on TransientVector
Comment by David Nolen [ 17/Feb/14 12:00 PM ]

Francis Avila, many thanks will look over these.

Comment by Francis Avila [ 17/Feb/14 10:49 PM ]

Patch rebased and trimmed: cljs-728-only-nth-lookup.patch.

Comment by Francis Avila [ 24/Feb/14 12:41 AM ]

Rebased cljs-728-only-nth-lookup.patch again. (core_test.cljs changes would have "conflicted".)

Comment by David Nolen [ 24/Feb/14 6:04 AM ]

fixed https://github.com/clojure/clojurescript/commit/71f781c75bf6e9d19be8d0e529ed0275fa523942





[CLJS-727] no warning on namespace aliased vars that don't exist Created: 16/Dec/13  Updated: 07/Jan/14  Resolved: 07/Jan/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Duplicate Votes: 0
Labels: None


 Comments   
Comment by Timothy Pratley [ 06/Jan/14 12:51 PM ]

I believe this is resolved by the patch attached to CLJS-615
Please provide an example if it is not and I'll look into it.





[CLJS-726] Cljs reader cannot handle comments (eg in project.clj) Created: 13/Dec/13  Updated: 14/Dec/13  Resolved: 14/Dec/13

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

Type: Defect Priority: Major
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File patch    
Patch: Code

 Comments   
Comment by David Nolen [ 14/Dec/13 2:40 PM ]

fixed, https://github.com/clojure/clojurescript/commit/454bcb70fbbd9e9feb20ada7b4043eab70dafea2





[CLJS-725] `(apply vector ...` does not work as expected on the product of `(drop-while ...` Created: 11/Dec/13  Updated: 11/Dec/13  Resolved: 11/Dec/13

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

Type: Defect Priority: Major
Reporter: Tim Visher Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X 10.9 (13A603)
de6ee41 CLJS-721: support :include-macros and :refer-macros


Attachments: Text File fix-725.patch    

 Description   
$ script/repljs
To quit, type: :cljs/quit
ClojureScript:cljs.user> (drop-while (partial = 1) [1 2 3])
(2 3)
ClojureScript:cljs.user> (apply vector (drop-while (partial = 1) [1 2 3]))
[1 2 3]
ClojureScript:cljs.user> (into [] (drop-while (partial = 1) [1 2 3]))
[2 3]


 Comments   
Comment by Tim Visher [ 11/Dec/13 9:49 AM ]

Failing test.

Comment by David Nolen [ 11/Dec/13 10:32 AM ]

fixed, https://github.com/clojure/clojurescript/commit/8943faf94856ea706252fa3276ad8ded234595f2





[CLJS-724] "range" values are sometimes behaving unexpectedly Created: 10/Dec/13  Updated: 11/Dec/13  Resolved: 11/Dec/13

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

Type: Defect Priority: Major
Reporter: Michael Bradley, Jr. Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

browsers, rhino



 Description   

The lazy sequences returned by "range" are, in some situations, producing unexpected results as elements of larger computations.

Example:

(defn range-print [rng]
  (let [fst (first rng)
        rst (rest rng)]
    (when fst
      (.log js/console "first: " (clj->js fst))
      (.log js/console "rest: " (clj->js rst))
      (range-print rst))))

(range-print (range 3))

(range-print (map identity (range 3)))

(comment

  (range-print (range 3))

  ;; In my browser's console, the output of the above expression looks as
  ;; follows. Note the UNexpected "first" at the end.

  first:  0
  rest:  [1, 2]
  first:  1
  rest:  [2]
  first:  2
  rest:  []
  first:  3
  rest:  []

  ;; --------------------------------------------------------------------------

  (range-print (map identity (range 3)))

  ;; The above expression generates the output I expect.

  first:  0
  rest:  [1, 2]
  first:  1
  rest:  [2]
  first:  2
  rest:  []

  )


 Comments   
Comment by David Nolen [ 11/Dec/13 10:43 AM ]

fixed, https://github.com/clojure/clojurescript/commit/69f3a390df4a954d49077bfd67200aac18d8cd99





[CLJS-723] add :preamble option to compiler Created: 10/Dec/13  Updated: 17/Dec/13  Resolved: 17/Dec/13

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

Type: Enhancement Priority: Minor
Reporter: Travis Vachon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs

Attachments: Text File cljs_723.patch    

 Description   

Per this thread:

https://groups.google.com/forum/#!searchin/clojurescript/preamble/clojurescript/rUIlLfcYZvE/Yskfh4znL_0J

1) :preamble 's value will be a vector of paths
2) the compiled output is prepended with the contents of the files at those paths
3) the generated source map points to the correct/adjusted line numbers

Additionally, when compiling for the :nodejs target the preamble contents will default to the hashbang we currently write in that situation.



 Comments   
Comment by Travis Vachon [ 11/Dec/13 4:59 PM ]

patch to add :preamble

I'm not confident this is the best way to do the source map stuff - any thoughts/suggestions would be very welcome

Comment by Michael Bradley, Jr. [ 13/Dec/13 5:55 PM ]

Could the issue (and patch) be expanded to also support a :postamble option?

With both :preamble and :postamble options available, it would be easy to implement more sophisticated compiler-output wrappers, such as the one used by David Nolen's mori library (which I helped adapt from the Q javascript library).

See: https://github.com/swannodette/mori/blob/master/support/wrapper.js

Comment by David Nolen [ 14/Dec/13 2:37 PM ]

I'm fine with extending this to {:postamble}.

Comment by David Nolen [ 14/Dec/13 2:39 PM ]

fixed https://github.com/clojure/clojurescript/commit/454bcb70fbbd9e9feb20ada7b4043eab70dafea2

Comment by David Nolen [ 14/Dec/13 2:40 PM ]

Oops resolved wrong ticket

Comment by David Nolen [ 17/Dec/13 4:26 PM ]

fixed, https://github.com/clojure/clojurescript/commit/136bf46c656265a93dd15c40925f11edb34bd127.

If someone wants to open a postamble ticket and attach a patch go for it.





[CLJS-722] Support parse.com's Cloud Code environment in :nodejs builds Created: 09/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Travis Vachon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs

Attachments: Text File cljs_722.patch     Text File cljs_722v2.patch     Text File cljs_722v3.patch     Text File parse.patch    
Patch: Code

 Description   

parse.com's Cloud Code is very similar to node.js but expects no hashbang and doesn't provide util.js, so provide a compiler argument to elide the header and wrap the util.js require in a try...catch



 Comments   
Comment by David Nolen [ 10/Dec/13 9:21 AM ]

Please format the patch so it can be applied with git am. See http://github.com/clojure/clojurescript/wiki/Patches

Comment by Travis Vachon [ 10/Dec/13 9:47 AM ]

git am-able patch

Comment by Travis Vachon [ 10/Dec/13 9:47 AM ]

sorry about that! will update all the tickets I've filed recently as well

Comment by Travis Vachon [ 10/Dec/13 1:22 PM ]

so actually, I think a better solution to this will be to add support for :preamble as proposed here:

https://groups.google.com/forum/#!searchin/clojurescript/preamble/clojurescript/rUIlLfcYZvE/Yskfh4znL_0J

I'm imagining that the default preamble for the :nodejs target will be the hashbang

that will let us a) redefine the require function in our compiled output to squelch the util import and b) avoid writing the hashbang

Comment by David Nolen [ 11/Dec/13 10:52 AM ]

so is this ticket superseded by CLJS-723?

Comment by Travis Vachon [ 11/Dec/13 12:02 PM ]

yeah - if I can write arbitrary js at the beginning of my compiled file I don't need this patch. Can close.

Comment by Travis Vachon [ 12/Dec/13 8:22 AM ]

arg, it looks like I spoke too soon - redefining require in the cloud code env appears to be impossible. I spent a couple hours trying every scoping trick I could find - it looks like the only way to redefine require is by defining a function (assignment just doesn't seem to take) and that function is always hoisted to the very top, so there's no way to get a handle on the original require function. Parse's sandbox doesn't seem to have another way to reference require, either.

How would you feel about moving {{ (set! cljs.core/string-print (.-print (require "util"))) }} into a function node.js people can call rather than doing it when the module is loaded?

Comment by David Nolen [ 12/Dec/13 8:38 AM ]

Honestly I'd be happy to remove it altogether and let people just handle this themselves. Or allow people to disable it if they are using the nodejs target option.

Comment by Travis Vachon [ 12/Dec/13 10:07 AM ]

ok great. I'm inclined to leave it in a function because it feels sort of non-obvious to me - would be nice to help people avoid reinventing in every codebase. I'll attach a new patch.

Comment by Travis Vachon [ 12/Dec/13 10:53 AM ]

add new patch that just moves the problematic require into a function. the hashbang issue the original patch addressed is better fixed with the preamble work in http://dev.clojure.org/jira/browse/CLJS-723

Comment by David Nolen [ 16/Jan/14 5:33 PM ]

Isn't this a breaking change for existing Node.js users?

Comment by Travis Vachon [ 22/Jan/14 2:00 PM ]

it is yeah, I assumed that was ok because you said you'd be happy to remove it altogether - I'm ok with removing it if you think that's a preferable breaking change.

Comment by David Nolen [ 22/Jan/14 2:11 PM ]

Lets rename it to enable-util-print! to match enable-console-print!.

Comment by Travis Vachon [ 22/Jan/14 2:13 PM ]

cool! will do. I'll update the patch today.

Comment by Travis Vachon [ 22/Jan/14 7:22 PM ]

rename function to enable-util-print!

Comment by David Nolen [ 23/Jan/14 7:52 AM ]

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





[CLJS-721] support :include-macros true modifier in :require Created: 09/Dec/13  Updated: 10/Dec/13  Resolved: 10/Dec/13

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None


 Description   

This modifier will additionally trigger a load of a Clojure file containing macros that matches the namespace. This would allow libraries to adopt cljs.core's inlining macro style without needing both a :require-macros and :require.

The following should be supported:

(ns foo.bar
  (:require [baz.woz :as woz :include-macros true))
(ns foo.bar
  (:require [baz.woz :as woz :refer [one] :refer-macros [two]))

I think the following is probably a bridge too far:

(ns foo.bar
  (:use [baz.woz :only [one] :include-macros [two]))


 Comments   
Comment by Jozef Wagner [ 10/Dec/13 4:15 AM ]

Same goal as CLJS-563

Comment by David Nolen [ 10/Dec/13 9:20 AM ]

Thanks I've closed CLJS-563. The required explicit modifier here avoids unpleasant surprises.

Comment by Jozef Wagner [ 10/Dec/13 11:32 AM ]

What will be the semantics if only the .clj file is present on classpath?

Comment by David Nolen [ 10/Dec/13 11:36 AM ]

An error.

Comment by Thomas Heller [ 10/Dec/13 12:35 PM ]

I assume you plan something like this (if not ignore me):

(ns wants-to-use-macros
(:require [has-macros :as m :include-macros true])

Could we instead do something like

(ns has-macros
(include-macros))

or

(ns ^:include-macros has-macros)

Seems to me it would be more helpful to write the include-macros once instead of every time the ns is used?

Comment by David Nolen [ 10/Dec/13 1:08 PM ]

Further complicating ns form parsing via metadata and custom forms is not on the table. I'm not particularly interested in convenience beyond removing two requires for two files that are logically related.

Comment by Thomas Heller [ 10/Dec/13 3:38 PM ]

Sorry to be blunt but I don't see where its more complicated to parse one extra form in the ns vs. parsing a far more complicated argument list in (:require) as you put in your example. Also trading one inconvenience (:require-macros) for another (:refer-macros, :include-macros) is only a small improvement.

IMHO its totally inconvenient that I a) have to remember which namespaces provide macros b) which defs were actually macros (assuming :refer). Assuming someone writes a library which provides some macros, shouldn't the library author worry about including the correct macros and let me just use it, just as in Clojure?

But I assume my proposal is more complex since you can't analyze a namespace on its own anymore (since any referred var may be a macro), which is a totally valid argument not to do it.

Comment by David Nolen [ 10/Dec/13 4:01 PM ]

Yes your suggestion has other implications. What I'm suggesting will desugar into existing supported functionality.

Comment by David Nolen [ 10/Dec/13 5:30 PM ]

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





[CLJS-720] #queue literal behavior is incorrect Created: 07/Dec/13  Updated: 07/Dec/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In order for queue to work we need to adopt an approach similar to the one for #js data literals - i.e. needs special casing in the analyzer since queues are not "atomic" values.






[CLJS-719] this-as behaves incorrectly in "scoping function" Created: 07/Dec/13  Updated: 07/Dec/13

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

Type: Defect Priority: Minor
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When a this-as expression gets put in a "scoping function", e.g. in a let-binding, the value bound via this-as refers to the scoping function, and not to the outer scope.

Example:

(def foo
  (js-obj
    "bar" "baz"
    "getBarRight" (fn [] (this-as self (.-bar self)))
    "getBarWrong" (fn []
                    (let [bar (this-as self (.-bar self))]
                      bar))))
     
(.log js/console (.getBarRight foo)) ;; => "baz"
(.log js/console (.getBarWrong foo)) ;; => undefined

Whereas foo.getBarRight expands to something like

function() {
  var self = this; // this refers to foo
  return self.bar; // returns "bar"
}

foo.getBarWrong on the other hand expands to

function() {
  var bar = function() {
    var self = this; // this refers to enclosing function
    return self.bar; // returns undefined
  }();
  return bar; // returns undefined
}





[CLJS-718] quoted tagged literal don't work Created: 06/Dec/13  Updated: 07/Dec/13  Resolved: 07/Dec/13

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   
'#inst "2013-12-06"


 Comments   
Comment by David Nolen [ 07/Dec/13 9:19 PM ]

fixed for all but #queue https://github.com/clojure/clojurescript/commit/3424908269d427fc5ad054f7f4e8a69d27cffddc





[CLJS-717] #js tagged literal Created: 06/Dec/13  Updated: 06/Dec/13  Resolved: 06/Dec/13

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   
#js {:foo "bar"} ;; (js-obj "foo" "bar")
#js {"foo" "bar"} ;; (js-obj "foo" "bar")
#js [1 2 3] ;; (array 1 2 3)

#js is shallow. Namespaced keywords not allowed.



 Comments   
Comment by David Nolen [ 06/Dec/13 5:49 PM ]

fixed, https://github.com/clojure/clojurescript/commit/91f6a3223122e3ae147cca0e9838f84290292789





[CLJS-716] Lookup for Date keys does not work in PersistentMaps and PersistentSets Created: 06/Dec/13  Updated: 12/Mar/14

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

Type: Defect Priority: Minor
Reporter: Sunil Gunisetty Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug
Environment:

Clojurescript version "0.0-2080"
Browser : Firefox 25.0.1
: Chrome 31.0.1650.63



 Description   

Lookup of js/Date objects fails, if there are more than 8 elements in the map. Works correctly if the map contains 8 elements or less.

Examples :

1. Map with more than 8 elements

cljs.user> (def test-map
{
:a 1
:b 2
#inst "2013-12-19T05:00:00.000-00:00" 3
:d 4
:e 5
#inst "2013-12-06T05:00:00.000-00:00" 6
:g 7
:h 8
:i 9
:j 10
})

cljs.user> (test-map #inst "2013-12-19T05:00:00.000-00:00")
nil

2. Map with 8 elements

cljs.user> (def test-map
{
:a 1
:b 2
#inst "2013-12-19T05:00:00.000-00:00" 3
:d 4
:e 5
#inst "2013-12-06T05:00:00.000-00:00" 6
:g 7
:h 8
})

cljs.user> (test-map #inst "2013-12-19T05:00:00.000-00:00")
3



 Comments   
Comment by David Nolen [ 06/Dec/13 5:07 PM ]

This is because JS Dates don't hash consistently, http://dev.clojure.org/jira/browse/CLJS-523





[CLJS-715] Numbers are always emitted as literals Created: 06/Dec/13  Updated: 06/Dec/13

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

Type: Defect Priority: Minor
Reporter: George Fraser Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

ClojureScript 2080



 Description   

At the REPL:

> (.toString 1)
"Error evaluating:" (.toString 1) :as "1.toString()"
org.mozilla.javascript.EvaluatorException: missing ; before statement (<cljs repl>#3)

The emitted code `1.toString()` is a parse error, it should be `(1).toString()`






[CLJS-714] cljs.closure/source-on-disk inconsistency Created: 05/Dec/13  Updated: 05/Dec/13  Resolved: 05/Dec/13

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

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

clojurescript with source maps


Attachments: Text File cljs-fix-source-url-reference.patch    
Patch: Code

 Description   

cljs.closure/source-on-disk will ensure that all source files are available when source maps are enabled. Sources from jar files are copied and reference the target location afterwards, sources on the filesystem are copied BUT the reference to that location is lost.

The attached patch ensures that the correct url is used in cljs.closure/output-unoptimized since otherwise the source maps will contain links to invalid files, assuming the source files are not on :source-map-path.



 Comments   
Comment by Thomas Heller [ 05/Dec/13 5:57 AM ]

Oops this breaks some other cases, working on a fix.

Comment by Thomas Heller [ 05/Dec/13 6:15 AM ]

Ok the issue is a lot more involved than I had hoped but I uncovered a bug while looking into it. Basically CLJS expects javascript files in jars never to change, it at least wont try to copy it again so the source map might be incorrect if a javascript file inside a jar changes.





[CLJS-713] optimized case Created: 04/Dec/13  Updated: 22/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Michał Marczyk
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-713-first-cut-at-compiling-case-to-switch.patch    

 Description   

With the advent of asm.js many engines will like compile switch statements over integers into jump tables. We should provide a real `case*` ast node that compiles to JS `switch` when possible - i.e. numbers, strings, keywords etc.



 Comments   
Comment by Michał Marczyk [ 18/Feb/14 5:56 PM ]

First cut impl also available here:

https://github.com/michalmarczyk/clojurescript/tree/713-compile-case-to-switch

With this patch applied, case expressions are compiled to switch + some extra bits when all tests are numbers or strings, otherwise old logic is used.

For example, {{(fn [] (let [x 1] (case x 1 :foo (2 3) :bar :quux)))}} gets compiled to

function () {
    var x = 1;
    var G__6469 = x;
    var caseval__6470;
    switch (G__6469) {
      case 1:
        caseval__6470 = new cljs.core.Keyword(null, "foo", "foo", 1014005816);
        break;
      case 2:
      case 3:
        caseval__6470 = new cljs.core.Keyword(null, "bar", "bar", 1014001541);
        break;
      default:
        caseval__6470 = new cljs.core.Keyword(null, "quux", "quux", 1017386809);
    }
    return caseval__6470;
}

The existing test suite passes, but I suppose it wouldn't hurt to add some tests with case in all possible contexts.

Comment by Michał Marczyk [ 18/Feb/14 6:05 PM ]

As a next step, I was planning to arrange things so that numbers/strings are fished out from among the tests and compiled to switch always, with any leftover tests put in an old-style nested-ifs-based case under default:. Does this sound good?

It seems to me that to deal with symbols and keywords in a similar manner we'd have to do one of two things:

1. check symbol? and keyword? at the top, then compile separate switches (the one for keywords would extract the name from the given keyword and use strings in the switch);

2. use hashes for dispatch.

Which one sounds better? Or is there a third way?

Comment by Michał Marczyk [ 18/Feb/14 6:11 PM ]

Of course we'd need to compute hashes statically to go with 2. I'd kind of like it if it were impossible (randomized seed / universal hashing), but currently it isn't.

Comment by Francis Avila [ 19/Feb/14 12:22 AM ]

At least on v8, there are surprisingly few cases where a switch statement will be optimized to a jump table. Basically the type of the switched-over value must always (across calls) match the type of every case, and there must be fewer than 128 cases, and integer cases must be 31-bit ints (v8's smi type). So mixing string and number cases in the same switch guarantees the statement will never be compiled. In many cases an equivalent if-else will end up being significantly faster on v8 just because the optimizing jit recognizes them better. There's an oldish bug filed against v8 switch performance. Looking at the many jsperfs of switch statements, it doesn't seem that v8 has improved. Relevant jsperf

Firefox is much better at optimizing switch statements (maybe because of their asm.js/emscripten work) but I don't know what conditions trigger (de)optimization.

I suspect the best approach is probably going to be your option one: if-else dispatch on type if any case is not a number, and then a switch statement covering the values for each of the keyword/string/symbol types present (no nested switch statements, and outlining the nested switches might be necessary). Even with a good hash, to guarantee v8 optimizing-compilation you would need to truncate the hashes into an smi (signed-left-shift once?) inside the case*.

Comment by David Nolen [ 19/Feb/14 12:50 AM ]

There's no need for invention here. We should follow the strategy that Clojure adopts - compile time hash calculation.

Comment by Francis Avila [ 19/Feb/14 3:09 PM ]

The problem, as Michal alluded to, is that the hash functions in cljs's runtime environment are not available at compile-time (unlike in Clojure). This might be a good opportunity to clean up that situation or even use identical hash values across Clojure and Clojurescript (i.e. CLJS-754), but that's a much bigger project. Especially considering it will probably not bring much of a speedup over an if-else-if implementation except in very narrow circumstances.

Comment by David Nolen [ 19/Feb/14 4:38 PM ]

Francis Avila I would make no such assumptions about performance without benchmarks. One of the critical uses for case is over keywords. Keyword hashes are computed at compile time, so that's one function call and a jump on some JavaScript engines. This is particularly useful for the performance of records where you want to lookup a field via keyword before checking the extension map.

This ticket should probably wait for CLJS-754 before proceeding.

Comment by Francis Avila [ 22/Feb/14 4:44 AM ]

Record field lookup is a good narrow use case to test. I put together a jsperf to compare if-else (current) vs switch with string cases vs switch with int cases (i.e., hash-compares, assuming perfect hashing).





[CLJS-712] resolve-var for symbol with dot still wrong Created: 03/Dec/13  Updated: 03/Dec/13

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

We need to recur on the first segment passing an new additional argument to resolve-var indicating that we should not try to resolve in the current namespace and instead warn.






[CLJS-711] Sequences should support ES6 Generator interface Created: 03/Dec/13  Updated: 20/Dec/13  Resolved: 20/Dec/13

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 20/Dec/13 8:41 AM ]

Not something we need to solve in CLJS itself.





[CLJS-710] port clojure.pprint Created: 02/Dec/13  Updated: 28/Jan/14

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

Type: Task Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-709] clj->js for array is slow Created: 02/Dec/13  Updated: 02/Dec/13  Resolved: 02/Dec/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: None


 Description   

This is because we use apply, should probably just do a loop/recur here.



 Comments   
Comment by David Nolen [ 02/Dec/13 8:31 PM ]

fixed, https://github.com/clojure/clojurescript/commit/1a8faf57dde2a0096478a5e7b8e7d086921ceaf2





[CLJS-708] Use //# for brepl source maps Created: 30/Nov/13  Updated: 01/Dec/13  Resolved: 01/Dec/13

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

Type: Enhancement Priority: Minor
Reporter: Nelson Morris Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 708.patch    

 Description   

Use //# for brepl source maps.

I missed when the change happened for compiler.clj, but this will bring repl.clj up to date for //# vs //@.



 Comments   
Comment by David Nolen [ 01/Dec/13 2:16 PM ]

fixed, https://github.com/clojure/clojurescript/commit/569c3c22b7b4191050ed968def2a5eae47d6c7ad





[CLJS-707] script/bootstrap downloads unspecified version of closure-compiler Created: 29/Nov/13  Updated: 30/Nov/13

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

Type: Defect Priority: Minor
Reporter: Ryan Berdeen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

script/bootstrap downloads http://dl.google.com/closure-compiler/compiler-latest.zip (https://github.com/clojure/clojurescript/blob/r2080/script/bootstrap#L64), while the project.clj specifies a specific version.

As a result, you'll get a different version of the compiler when using following the Quick Start guide. The latest version of the Closure Compiler was compiled for Java 7, causing cljsc to fail with a java.lang.UnsupportedClassVersionError under Java 6.



 Comments   
Comment by David Nolen [ 29/Nov/13 4:02 PM ]

Does Closure Compiler no longer support Java 6?

Comment by Ryan Berdeen [ 30/Nov/13 1:16 PM ]

The release notes for v20131118 say "Move compiler to Java 7": https://groups.google.com/d/topic/closure-compiler-discuss/_T5Aai2sg68/discussion.

This release can still be built for Java 6 using ant jar -Dant.build.javac.source=1.6 -Dant.build.javac.target=1.6, but the current master makes use of Java 7 features in one class.

The Java 6/Java 7 support seems like it should be it's own issue. It's just how I discovered the problem, which is that running the bootstrap script doesn't give a consistent result. All of the other dependencies apart from closure-compiler have explicitly set versions, so I'm not sure I understand the reasoning. Shouldn't it download from central.maven.org with a version that matches project.clj?

Comment by David Nolen [ 30/Nov/13 1:44 PM ]

project.clj is just a convenience for people actually hacking on the compiler for now.





[CLJS-706] Source map column mapping is off by one Created: 28/Nov/13  Updated: 29/Nov/13  Resolved: 29/Nov/13

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

Type: Defect Priority: Minor
Reporter: Nelson Morris Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File dec_column.patch    
Patch: Code

 Description   

tools.reader uses 1 for the starting index of the line and columns.

`(meta (reader/read (readers/source-logging-push-back-reader (java.io.PushbackReader. (java.io.StringReader. "+")))))
=> {:end-column 1, :end-line 1, :column 1, :line 1, :source "+"}`

Sourcemaps use 0 for the starting index. So we need to decrement the :column as well as the :line when making the sourcemap map. Also, the comment regarding line numbers and 0- vs 1- indexing is reversed.



 Comments   
Comment by David Nolen [ 29/Nov/13 10:34 AM ]

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





[CLJS-705] locals clearing Created: 27/Nov/13  Updated: 29/Nov/13

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Without locals clearing ClojureScript is likely to suffer from the same cases as Clojure did for common uses of lazy sequences.



 Comments   
Comment by David Nolen [ 29/Nov/13 3:03 PM ]

For this we'll need to introduce some special private way to set a local to nil, i.e. (_clear_local sym)





[CLJS-704] warn if type extended to same protocol multiple times Created: 27/Nov/13  Updated: 27/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-703] warn if protocol signature vector is empty Created: 27/Nov/13  Updated: 27/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

currently get a mysterious exception






[CLJS-702] warn if protocol signature doesn't matched declared Created: 27/Nov/13  Updated: 27/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-701] warn if protocol used multiple times in a deftype Created: 27/Nov/13  Updated: 27/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-700] clojure.core.reducers/fold broken Created: 27/Nov/13  Updated: 29/Nov/13  Resolved: 29/Nov/13

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

Type: Defect Priority: Minor
Reporter: Jonas De Vuyst Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs_700.patch     Text File cljs-fold-fix-2.patch     Text File cljs-fold-fix.patch    
Patch: Code and Test

 Description   

clojure.core.reducers/fold currently does not work correctly if both a reducef and combinef are passed as arguments.

It is expected that (r/fold + + [1 2 3]) would return 6 (as do (r/fold + [1 2 3]) and (r/reduce + [1 2 3])). However, this is not the case because reducers.cljs currently defines fold as follows:

(def fold reduce)

As a result, the second + is used as the initial value for the reduce operation (instead of the value returned by ).

The attached patch fixes this bug. It also adds support for the protocol CollFold in CLJS.

(I will send in the CA tomorrow.)



 Comments   
Comment by David Nolen [ 28/Nov/13 8:05 AM ]

This mostly looks good but clojure.lang.PersistentHashMap should be cljs.core/PersistentHashMap and we should have a corresponding test.

Comment by Jonas De Vuyst [ 29/Nov/13 1:45 PM ]

I added a new patch, in which cljs.core/PersistentHashMap is substituted for clojure.lang.PersistentHashMap – although that part is actually commented out because CLJS maps do not have a method .fold.

I also added tests for reducing and folding of maps (which I also tested in Clojure).

Comment by David Nolen [ 29/Nov/13 2:45 PM ]

The patch has not been formatted so that it can be applied with git am. Please generate the patch following these instructions - http://github.com/clojure/clojurescript/wiki/Patches.

Comment by Jonas De Vuyst [ 29/Nov/13 3:58 PM ]

Oh, sorry. I have attached a new patch.

Comment by David Nolen [ 29/Nov/13 4:00 PM ]

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





[CLJS-699] local function calls not optimized Created: 25/Nov/13  Updated: 25/Nov/13  Resolved: 25/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   
(defn foo []
  (letfn [(bar [x y] (= x y))]
    (bar :foo :bar)))

generates

hugo_a_go_go.board.foo = function() {
  var a = function(a, c) {
    return cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(a, c)
  };
  return a.cljs$core$IFn$_invoke$arity$2 ? a.cljs$core$IFn$_invoke$arity$2(new cljs.core.Keyword(null, "foo", "foo", 1014005816), new cljs.core.Keyword(null, "bar", "bar", 1014001541)) : a.call(null, new cljs.core.Keyword(null, "foo", "foo", 1014005816), new cljs.core.Keyword(null, "bar", "bar", 1014001541))
};


 Comments   
Comment by David Nolen [ 25/Nov/13 9:09 AM ]

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





[CLJS-698] ^:export on a deftype/record method should goog.exportProperty Created: 25/Nov/13  Updated: 28/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

See http://developers.google.com/closure/compiler/docs/api-tutorial3



 Comments   
Comment by Eduard Bondarenko [ 27/Nov/13 7:37 AM ]
(deftype ^:export SceneMain []
  Object
  (handleShow [_]
    (display-categories)))

I used exportSymbol:

(goog/exportSymbol "SceneMain" SceneMain)
(goog/exportSymbol "SceneMain.prototype.handleShow" SceneMain.prototype.handleShow)

It works even with advanced optimizations:

ca("SceneMain",mg);
ca("SceneMain.prototype.handleShow",SceneMain.prototype.Cb);
Comment by David Nolen [ 28/Nov/13 8:03 AM ]

It would be nice if the following worked:

(deftype ^:export SceneMain []
  Object
  (^:export handleShow [_]
    (display-categories)))




[CLJS-697] top-level symbol reference doesn't get an automatically inserted ns-name Created: 23/Nov/13  Updated: 23/Nov/13  Resolved: 23/Nov/13

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

Type: Defect Priority: Major
Reporter: Limbo Peng Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler, bug, namespace
Environment:

org.clojure/clojurescript "0.0-2030"



 Description   

I'm trying to use a Node.js module (with nested namespaces) in ClojureScript - the code goes like this:

(ns myapp)
(def evernote (js/require "evernote"))
(def token "TOKEN")
(defn do-sth []
  (let [client (evernote.Evernote.Client. (js-obj "token" token))]
    (.log js/console client)))
(do-sth)

which gets compiled (with :simple optimization) to:

var myapp = {}
myapp.evernote = require("evernote")
myapp.token = "TOKEN"
myapp.do_sth = function() {
  var a = new evernote.Evernote.Client({token:myapp.token})
  return console.log(a)
}
myapp.do_sth()

which will obviously fail with error "Uncaught ReferenceError: evernote is not defined".



 Comments   
Comment by David Nolen [ 23/Nov/13 11:55 PM ]

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





[CLJS-696] remove arguments usage from defrecord constructor Created: 23/Nov/13  Updated: 23/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There is no need for the arguments usage in the defrecord constructor and it's a perf hit for construction. We should always construct defrecords by passing in the extra three arguments: __extmap, __meta, and hash automatically.






[CLJS-695] arithmetic type checking is too permissive Created: 22/Nov/13  Updated: 23/Nov/13  Resolved: 23/Nov/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: Declined Votes: 0
Labels: None


 Description   

Current we allow any. We should only allow numbers and expressions whose type we cannot infer.



 Comments   
Comment by David Nolen [ 22/Nov/13 12:06 PM ]

Actually if we really get any that should be ok. It should be possible to put a union on the meta of any so we can verify that it's truly any and not some union of non-numeric types.

Comment by David Nolen [ 23/Nov/13 11:46 PM ]

numeric type checking is improved, it will not advance much further until we have more sophisticated inference support





[CLJS-694] Dependency on Java 7 introduced by patch for CLJS-674 Created: 21/Nov/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Major
Reporter: Tim Visher Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix-694.patch    

 Description   

I introduced a call to .toPath, which is defined in Java 7. Need to refactor to avoid this so we can support down to Java 6.



 Comments   
Comment by Tim Visher [ 21/Nov/13 10:47 AM ]

fix-694.patch @ 21/NOV/13

I'm using URI's for relativizing and basic string comparison for directory comparison. URIs have a bug where they can't produce ../../ style relative paths, but since we require that your :output-dir be in the same directory as :source-map, this shouldn't bite us.

Can someone (David Powell?) who has easy access to Java 6 test this?

All tests pass.

Comment by Tim Visher [ 21/Nov/13 3:31 PM ]

I was able to get 1.6 installed:

java version "1.6.0_65"
Java(TM) SE Runtime Environment (build 1.6.0_65-b14-462-11M4609)
Java HotSpot(TM) 64-Bit Server VM (build 20.65-b04-462, mixed mode)

And run the test prior to my patch:

$ JAVA_HOME=/Library/Java/Home V8_HOME=/usr/local/bin SPIDERMONKEY_HOME=/usr/local/bin JSC_HOME=/usr/local/bin ./script/test
java version "1.6.0_65"
Java(TM) SE Runtime Environment (build 1.6.0_65-b14-462-11M4609)
Java HotSpot(TM) 64-Bit Server VM (build 20.65-b04-462, mixed mode)
Reflection warning, cljs/source_map.clj:166:41 - reference to field toPath can't be resolved.
Reflection warning, cljs/source_map.clj:166:41 - reference to field toAbsolutePath can't be resolved.
Reflection warning, cljs/source_map.clj:170:41 - reference to field toPath can't be resolved.
Reflection warning, cljs/source_map.clj:170:41 - reference to field toAbsolutePath can't be resolved.
Reflection warning, cljs/source_map.clj:170:41 - reference to field getParent can't be resolved.
Reflection warning, cljs/source_map.clj:175:18 - call to relativize can't be resolved.
Testing with V8
Tests completed without exception


Testing with SpiderMonkey
out/core-advanced-test.js:463: Error: Assert failed: (not (integer? 1e+308))
Testing with JavaScriptCore
Tests completed without exception


Tested with 3 out of 3 possible js targets

and after the patch:

$ JAVA_HOME=/Library/Java/Home V8_HOME=/usr/local/bin SPIDERMONKEY_HOME=/usr/local/bin JSC_HOME=/usr/local/bin ./script/test
Testing with V8
Tests completed without exception


Testing with SpiderMonkey
out/core-advanced-test.js:463: Error: Assert failed: (not (integer? 1e+308))
Testing with JavaScriptCore
Tests completed without exception


Tested with 3 out of 3 possible js targets
Comment by David Nolen [ 21/Nov/13 3:38 PM ]

Excellent

Comment by David Powell [ 22/Nov/13 12:58 AM ]

Prior to this patch, I have seen cljsbuild emit sourcemaps that refer to files from the /target directory, which if you set :output-dir to "out", is going to require an implementation of relativize() that is able to emit paths starting with ../target

So I'm not sure if something wrong is happening there? I'll have to check that again once the rest of my windows issues are sorted.

I'm still looking at CLJS-681 - the main issue is it omitting files entirely from the sourcemap - I'm getting closer...

Comment by Tim Visher [ 22/Nov/13 6:09 AM ]

@David Powell: After CLJS-674 though, we've restricted what :output-dir can be when using :source-map to ensure that :output-dir will be relative to :source-map's parent so that URI relativization should work fine.

Comment by David Nolen [ 22/Nov/13 7:39 AM ]

fixed, https://github.com/clojure/clojurescript/commit/023b49280a347979548cecc9b555f717b65b884f





[CLJS-693] new and dot form work on locals Created: 20/Nov/13  Updated: 02/Dec/13

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

Type: Defect Priority: Minor
Reporter: Joshua Headapohl Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Given some JavaScript library included in the same page as CLJS output:

var Lib = {};

Lib.Thing = function (val) {
    this.val = val;
};

Lib.Thing.prototype.log = function () {
    console.log(this.val);
};

I can bind the Thing property to a local and construct it using new or the dot form. This code compiles and runs without errors, and logs three lines to the console.

(ns cljs-construct-locals-bug.core)

; Legit
(let [thing (new js/Lib.Thing "hello")]
  (.log thing))

; Questionable
(let [Thing (.-Thing js/Lib)
      thing1 (new Thing "maybe")
      thing2 (Thing. "no way")]
  (.log thing1)
  (.log thing2))

I talked to David Nolen and he said this behavior is not correct.



 Comments   
Comment by David Nolen [ 02/Dec/13 8:58 PM ]

Upon further consideration this probably requires some feedback from the community. I'd forgotten that (Thing. foo bar ...) is just sugar for (new Thing foo bar ...) ...





[CLJS-692] cannot access properties on imported Google Closure constructors Created: 20/Nov/13  Updated: 23/Nov/13  Resolved: 23/Nov/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: None


 Description   
(ns foo
  (:import [goog.math Integer]))

(Integer.fromString "10")

Does not work, this will emit "Integer" directly instead of "goog.math.Integer".



 Comments   
Comment by David Nolen [ 23/Nov/13 11:43 PM ]

fixed, https://github.com/clojure/clojurescript/commit/8f7a19983b449155175a5e77d6bd8a86d36d2f68





[CLJS-691] Add IComparable implementions to Symbol and Keyword Created: 20/Nov/13  Updated: 20/Nov/13  Resolved: 20/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: File 691.diff    
Patch: Code and Test

 Description   

None exist now, which means that compare yields different values comparing namespaced and non-namespaced keywords and symbols in ClojureScript than in Clojure.



 Comments   
Comment by Chas Emerick [ 20/Nov/13 3:37 PM ]

Patch attached. Funny, the tests were already in core_test.cljs, but commented out.

BTW, this was another simple-check win.

Comment by David Nolen [ 20/Nov/13 5:23 PM ]

fixed,https://github.com/clojure/clojurescript/commit/6fc70acb04210fbd8513deb68b4d95694e1a0a60





[CLJS-690] add cljs.core/sequence + cljs.core/sorted? Created: 20/Nov/13  Updated: 02/Dec/13  Resolved: 02/Dec/13

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None


 Description   

Remember to update devnotes/corelib.org when complete



 Comments   
Comment by David Nolen [ 02/Dec/13 8:41 PM ]

fixed, https://github.com/clojure/clojurescript/commit/2a8c2a95fb94feca50413d20fdaf95e7d280a181





[CLJS-689] js/-Infinity munges to _Infinity Created: 20/Nov/13  Updated: 02/Dec/13

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

Type: Defect Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A dumb special-casing of js/ munging should fix.






[CLJS-688] Unhelpful error message when compiling with blank .cljs file Created: 20/Nov/13  Updated: 11/Dec/13

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

Type: Defect Priority: Minor
Reporter: John Mumm Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When compiling, if you have a blank .cljs file in your source path, you get the following unhelpful message:

java.lang.NullPointerException: 
          closure.clj:370 cljs.closure/compiled-file
            core.clj:2485 clojure.core/map[fn]
          LazySeq.java:42 clojure.lang.LazySeq.sval
          LazySeq.java:67 clojure.lang.LazySeq.seq
              RT.java:484 clojure.lang.RT.seq
             core.clj:133 clojure.core/seq
             core.clj:678 clojure.core/concat[fn]
          LazySeq.java:42 clojure.lang.LazySeq.sval
          LazySeq.java:60 clojure.lang.LazySeq.seq
             Cons.java:39 clojure.lang.Cons.next
             RT.java:1654 clojure.lang.RT.boundedLength
          RestFn.java:130 clojure.lang.RestFn.applyTo
             core.clj:619 clojure.core/apply
         closure.clj:1018 cljs.closure/build
          closure.clj:981 cljs.closure/build
          compiler.clj:58 cljsbuild.compiler/compile-cljs[fn]
          compiler.clj:57 cljsbuild.compiler/compile-cljs
         compiler.clj:158 cljsbuild.compiler/run-compiler
form-init6897480153702409709.clj:1 user/eval2998[fn]
form-init6897480153702409709.clj:1 user/eval2998[fn]
          LazySeq.java:42 clojure.lang.LazySeq.sval
          LazySeq.java:60 clojure.lang.LazySeq.seq
              RT.java:484 clojure.lang.RT.seq
             core.clj:133 clojure.core/seq
            core.clj:2780 clojure.core/dorun
            core.clj:2796 clojure.core/doall
form-init6897480153702409709.clj:1 user/eval2998
       Compiler.java:6619 clojure.lang.Compiler.eval
       Compiler.java:6609 clojure.lang.Compiler.eval
       Compiler.java:7064 clojure.lang.Compiler.load
       Compiler.java:7020 clojure.lang.Compiler.loadFile
             main.clj:294 clojure.main/load-script
             main.clj:299 clojure.main/init-opt
             main.clj:327 clojure.main/initialize
             main.clj:362 clojure.main/null-opt
             main.clj:440 clojure.main/main
          RestFn.java:421 clojure.lang.RestFn.invoke
             Var.java:419 clojure.lang.Var.invoke
             AFn.java:163 clojure.lang.AFn.applyToHelper
             Var.java:532 clojure.lang.Var.applyTo
             main.java:37 clojure.main.main


 Comments   
Comment by Travis Thieman [ 06/Dec/13 11:30 AM ]

I tried this using both cljsbuild (from a mies template) and bin/cljsc on a directory. Both of these methods give the following error message, which seems reasonable:

java.lang.AssertionError: Assert failed: file:/repos/empty/src/empty/empty.cljs does not provide a namespace

How are you compiling to get what you're seeing?





[CLJS-687] error/warning when deftype/record constructor used as a function Created: 19/Nov/13  Updated: 19/Nov/13  Resolved: 19/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 19/Nov/13 5:11 PM ]

fixed,https://github.com/clojure/clojurescript/commit/792b2b87c9982a76093fcc3278dd246e8672627b





[CLJS-686] Auto-generate externs for foreign libs Created: 19/Nov/13  Updated: 19/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Chouser has something simple and nice that should work for most use cases - https://gist.github.com/Chouser/5796967






[CLJS-685] Cannot call method 'fromArray' of undefined -- Clojurescript 0.0-2030 Created: 17/Nov/13  Updated: 26/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Minor
Reporter: John Chijioke Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler, bug, errormsgs
Environment:

Linux 3.2.0-52-generic x86_64 GNU/Linux, java 1.7, clojure 1.5.1



 Description   

Clojurescript 0.0-2030

This line from compile cljs.core is causing problems:

cljs.core.PersistentQueue.EMPTY = (new cljs.core.PersistentQueue(null, 0, null, cljs.core.with_meta(cljs.core.PersistentVector.EMPTY, cljs.core.PersistentArrayMap.fromArray([new cljs.core.Keyword(null, "end-line", "end-line", 2693041432), 3820, new cljs.core.Keyword(null, "end-column", "end-column", 3799845882), 69], true)), 0));

error message: Uncaught TypeError: Cannot call method 'fromArray' of undefined.

That's the first mention of fromArray in that file. I don't know if it's an ordering problem.



 Comments   
Comment by John Chijioke [ 17/Nov/13 11:10 PM ]

I solved it by replacing [] with cljs.core.PersistentVector.EMPTY. I think this must be a reader problem.

Comment by David Nolen [ 17/Nov/13 11:32 PM ]

This ticket needs more details, how can this error be reproduced?

Comment by Peter Taoussanis [ 22/Nov/13 3:03 AM ]

Hi, I'm seeing the same problem with tools.reader 0.8.0.

Any Clojurescript file (even an empty file) will produce the error.

Clojure: 1.6.0-alpha2
Clojurescript: 0.0-2030
Cljsbuild: 1.0.0
tools.reader: 0.8.0

Tried `lein cljsbuild clean`.

Problem is resolved by dropping back to tools.reader 0.7.10.

Update: have created an issue on the tools.reader GitHub page: https://github.com/clojure/tools.reader/issues/7

Update 2: this isn't something specific to Cljs 0.0-2030 btw, tools.reader 0.8.0 seems to produce the same error against at least Cljs 0.0-2060, 0.0-2027, 0.0-2024.

Comment by Nicola Mometto [ 22/Nov/13 6:49 AM ]

tools.reader 0.8.0 introduces end-column/end-line metadata, this needs to be elided as per line/column to avoid this bootstrapping issue.

Comment by David Nolen [ 22/Nov/13 8:02 AM ]

fixed, http://github.com/clojure/clojurescript/commit/36d401797f85c99794eef8a71239641930c36871

Comment by Peter Taoussanis [ 22/Nov/13 10:30 AM ]

Thanks a lot David, Nicola - much appreciated! Cheers

Comment by John Chijioke [ 26/Nov/13 6:32 AM ]

Thanks David. Cheers!





[CLJS-684] throw on circular dependency in ns form Created: 17/Nov/13  Updated: 17/Nov/13  Resolved: 17/Nov/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: None


 Description   

Otherwise the user will get a cryptic stack overflow error.



 Comments   
Comment by David Nolen [ 17/Nov/13 2:08 PM ]

fixed, https://github.com/clojure/clojurescript/commit/8f0eafadebce199bf92c788e5db1a7924f12606b





[CLJS-683] :source-map-path so that the path to source files can easily be made arbitrarily relative for web workflows Created: 17/Nov/13  Updated: 17/Nov/13  Resolved: 17/Nov/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: None


 Description   

See also CLJS-674



 Comments   
Comment by David Nolen [ 17/Nov/13 12:29 PM ]

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





[CLJS-682] Inline PersistentVector construction Created: 16/Nov/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently the compiler calls cljs.core.PersistentVector.fromArray, we should just inline an instantiation.



 Comments   
Comment by David Nolen [ 22/Nov/13 9:21 AM ]

already fixed in master





[CLJS-681] Source maps in advanced mode don't work when compiled on Windows Created: 15/Nov/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Major
Reporter: David Powell Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File fix-681.patch    

 Description   

See the difference between:

https://github.com/djpowell/cstest2/blob/linux/out/main-map.js
and
https://github.com/djpowell/cstest2/blob/windows/out/main-map.js

Note that windows has an entry for "out/" in sources and is missing all of the cljs entries.

Might be something to do with path munging and backslashes on Windows?



 Comments   
Comment by David Powell [ 16/Nov/13 11:06 AM ]

One problem is that file URLs converted using url.getPath() are of the form:
/C:/Temp/some/file.js

whereas the keys in @env/::compiled-cljs/* are absolute paths, such as:
C:\Temp\some\file.js

so these files get dropped by cljs.closure/optimize because they can't be found in @env.

A second problem, that shows up when :output-dir isn't specified, is that the cljs.source-maps/relativize-path returns things like:
/C:/Temp/cstest2/target/cljsbuild-compiler-0//goog/base.js
from the browser, this doesn't resolve properly. We need a better implementation of relativize here - perhaps the one in cljs.closure/path-relative-to could be used?

It is a bit unclear in the code what is supposed to refer to a file, and what is supposed to be a relative uri. Windows paths are particularly sensitive to the difference.

Comment by David Nolen [ 18/Nov/13 8:06 AM ]

It probably makes sense for the the lookup to be based on URL and not file paths as the source map needs relative paths anyway. I think if we switch the keys in ::compiled-clj to URLs we should be able to avoid host path issues?

Comment by David Nolen [ 20/Nov/13 1:29 PM ]

Can we verify that this is resolved by CLJS-674? You can install ClojureScript master locally by running ./script/build in a git checkout of the project. Thanks!

Comment by David Powell [ 20/Nov/13 7:36 PM ]

No, this still doesn't fix it on windows.
I'm now getting content like this at the start of my sourcemaps when I specify :output-dir "out" -

{"version":3,
"file":"main-map.js",
"sources":
["out", "out\\goog
base.js", "out\\goog\\string
string.js",
"out\\goog\\debug
error.js", "out\\goog\\asserts
asserts.js",
"out\\goog\\array
array.js", "out\\goog\\object
object.js",
"out\\goog\\string
stringbuffer.js", "out
constants_table.cljs"],
"lineCount":3609,
...

  • backslashes instead of forward slashes
  • "out" instead of the expected cljs files

It looks similar to before, but now with backslashes instead of forward slashes.
I haven't checked, but I assume that the format of paths in ::compiled-clj isn't matching what is being looked up again.

Comment by Tim Visher [ 21/Nov/13 9:44 PM ]

I don't have any more time tonight but I believe the patch for CLJS-694 corrects at least the backslash issue. Probably won't correct more than that, but the source maps being relativized as URIs rather than files appears to produce the right output if the URI is in the same directory as the one being compared to, which should cover our cases here.

BTW, you can use modern.ie VMs to test this stuff out! Who knew, right? (Windows 8 is… Different.)

Comment by Tim Visher [ 22/Nov/13 12:06 PM ]

I just tested this on Windows 8 and the backslashes at least are gone. Maybe I'll have some time this weekend to get in there with a little trace action and see what else can be done to get the non-jar file dependencies correctly output.

Comment by Tim Visher [ 22/Nov/13 2:32 PM ]

fix-681.patch @ 22/NOV/13 fixes this for me on Windows and continues to pass on OS X.

Should be more thoroughly tested and impact analyzed, I'm sure, but I wanted to keep the discussion going.

Comment by David Powell [ 22/Nov/13 3:55 PM ]

fix-681 is working for me with either :output-dir as "out" or unset.

Comment by David Nolen [ 22/Nov/13 4:20 PM ]

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





[CLJS-680] shadowing of js/foo Created: 15/Nov/13  Updated: 16/Nov/13  Resolved: 16/Nov/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: None


 Description   
(defn generic-report [f]
  (fn [msgs]
    (let [msgs (str msgs)]
      (f msgs))))

(defn alert [& msgs]
  ((generic-report js/alert) msgs))

This will result in an infinite loop, this is because the code generated by alert will create a local called alert, and js/alert also becomes alert in the generated code.



 Comments   
Comment by David Nolen [ 16/Nov/13 7:59 PM ]

fixed, https://github.com/clojure/clojurescript/commit/08b310f24b184d39082eac91b1d8a2dba7ae38af





[CLJS-679] Unexpected variable con3$cljs$demo$geometry Created: 15/Nov/13  Updated: 16/Nov/13  Resolved: 16/Nov/13

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

Type: Defect Priority: Major
Reporter: Rostislav Svoboda Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

3.11.0-13-generic #20-Ubuntu SMP Wed Oct 23 07:38:26 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux



 Description   

I was playing with three.js and obtained the error below. A snapshot of my project & env is here: https://github.com/Bost/con3-cljs/commit/ebc6e2fc49935f57b4ee642bb345824d76c6b2ec

Compiling "resources/public/js/main.js" from ["src-cljs"]...
WARNING: Use of undeclared Var con3.cljs.demo/geometry at line 53 src-cljs/demo.cljs
Compiling "resources/public/js/main.js" failed.
java.lang.RuntimeException: java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.
Unexpected variable con3$cljs$demo$geometry
Node(NAME con3$cljs$demo$geometry): /home/bost/dev/con3/target/cljsbuild-compiler-2/con3/cljs/demo.js:32:7
return con3.cljs.demo.geometry;
Parent(NEW): /home/bost/dev/con3/target/cljsbuild-compiler-2/con3/cljs/demo.js:35:23
con3.cljs.demo.mesh = (new THREE.Mesh(con3.cljs.demo.geometry,con3.cljs.demo.material));

Compiler.java:715 com.google.javascript.jscomp.Compiler.runInCompilerThread
Compiler.java:647 com.google.javascript.jscomp.Compiler.compile
Compiler.java:603 com.google.javascript.jscomp.Compiler.compile
(Unknown Source) sun.reflect.GeneratedMethodAccessor16.invoke
DelegatingMethodAccessorImpl.java:43 sun.reflect.DelegatingMethodAccessorImpl.invoke
Method.java:606 java.lang.reflect.Method.invoke
Reflector.java:93 clojure.lang.Reflector.invokeMatchingMethod
Reflector.java:28 clojure.lang.Reflector.invokeInstanceMethod
closure.clj:718 cljs.closure/optimize
RestFn.java:139 clojure.lang.RestFn.applyTo
core.clj:619 clojure.core/apply
closure.clj:1030 cljs.closure/build
closure.clj:981 cljs.closure/build
compiler.clj:58 cljsbuild.compiler/compile-cljs[fn]
compiler.clj:57 cljsbuild.compiler/compile-cljs
compiler.clj:158 cljsbuild.compiler/run-compiler
form-init7759117007336734092.clj:1 user/eval2998[fn]
form-init7759117007336734092.clj:1 user/eval2998[fn]
LazySeq.java:42 clojure.lang.LazySeq.sval
LazySeq.java:60 clojure.lang.LazySeq.seq
Cons.java:39 clojure.lang.Cons.next
RT.java:598 clojure.lang.RT.next
core.clj:64 clojure.core/next
core.clj:2781 clojure.core/dorun
core.clj:2796 clojure.core/doall
form-init7759117007336734092.clj:1 user/eval2998
Compiler.java:6619 clojure.lang.Compiler.eval
Compiler.java:6609 clojure.lang.Compiler.eval
Compiler.java:7064 clojure.lang.Compiler.load
Compiler.java:7020 clojure.lang.Compiler.loadFile
main.clj:294 clojure.main/load-script
main.clj:299 clojure.main/init-opt
main.clj:327 clojure.main/initialize
main.clj:362 clojure.main/null-opt
main.clj:440 clojure.main/main
RestFn.java:421 clojure.lang.RestFn.invoke
Var.java:419 clojure.lang.Var.invoke
AFn.java:163 clojure.lang.AFn.applyToHelper
Var.java:532 clojure.lang.Var.applyTo
main.java:37 clojure.main.main
Caused by: java.lang.RuntimeException: INTERNAL COMPILER ERROR.
Please report this problem.
Unexpected variable con3$cljs$demo$geometry
Node(NAME con3$cljs$demo$geometry): /home/bost/dev/con3/target/cljsbuild-compiler-2/con3/cljs/demo.js:32:7
return con3.cljs.demo.geometry;
Parent(NEW): /home/bost/dev/con3/target/cljsbuild-compiler-2/con3/cljs/demo.js:35:23
con3.cljs.demo.mesh = (new THREE.Mesh(con3.cljs.demo.geometry,con3.cljs.demo.material));

VarCheck.java:159 com.google.javascript.jscomp.VarCheck.visit
NodeTraversal.java:540 com.google.javascript.jscomp.NodeTraversal.traverseBranch
NodeTraversal.java:534 com.google.javascript.jscomp.NodeTraversal.traverseBranch
NodeTraversal.java:534 com.google.javascript.jscomp.NodeTraversal.traverseBranch
NodeTraversal.java:534 com.google.javascript.jscomp.NodeTraversal.traverseBranch
NodeTraversal.java:534 com.google.javascript.jscomp.NodeTraversal.traverseBranch
NodeTraversal.java:534 com.google.javascript.jscomp.NodeTraversal.traverseBranch
NodeTraversal.java:314 com.google.javascript.jscomp.NodeTraversal.traverseRoots
NodeTraversal.java:503 com.google.javascript.jscomp.NodeTraversal.traverseRoots
VarCheck.java:102 com.google.javascript.jscomp.VarCheck.process
PhaseOptimizer.java:293 com.google.javascript.jscomp.PhaseOptimizer$NamedPass.process
PhaseOptimizer.java:237 com.google.javascript.jscomp.PhaseOptimizer.process
Compiler.java:1916 com.google.javascript.jscomp.Compiler.optimize
Compiler.java:749 com.google.javascript.jscomp.Compiler.compileInternal
Compiler.java:83 com.google.javascript.jscomp.Compiler.access$000
Compiler.java:650 com.google.javascript.jscomp.Compiler$2.call
Compiler.java:647 com.google.javascript.jscomp.Compiler$2.call
Compiler.java:677 com.google.javascript.jscomp.Compiler$3.call
FutureTask.java:334 java.util.concurrent.FutureTask$Sync.innerRun
FutureTask.java:166 java.util.concurrent.FutureTask.run
ThreadPoolExecutor.java:1145 java.util.concurrent.ThreadPoolExecutor.runWorker
ThreadPoolExecutor.java:615 java.util.concurrent.ThreadPoolExecutor$Worker.run
Thread.java:724 java.lang.Thread.run
Caused by: java.lang.IllegalStateException: Unexpected variable con3$cljs$demo$geometry



 Comments   
Comment by David Nolen [ 16/Nov/13 10:08 AM ]

you have a syntax error starting at line 53, the compiler warning is a clue, geometry is outside the let form.

Comment by Rostislav Svoboda [ 16/Nov/13 12:52 PM ]

> you have a syntax error starting at line 53, the compiler warning is a clue, geometry is outside the let form.

I didn't open this issue in order to be told how to fix my code.
I opened it because (1) the clojurescript compiler crashed and (2) I was instructed: "Please report this problem."!

Comment by David Nolen [ 16/Nov/13 6:38 PM ]

Sorry for the confusion and the short response - was at Clojure/conj the past two days.

The "Report this error" is from Google Closure - not ClojureScript.





[CLJS-678] warn on keyword literals used in identical? test Created: 13/Nov/13  Updated: 13/Nov/13

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-677] cljs.reader doesn't support keywords starting with a digit Created: 12/Nov/13  Updated: 02/Dec/13

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

Type: Defect Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
ClojureScript:foo> (r/read-string ":0")
"Error evaluating:" (r/read-string ":0") :as "cljs.reader.read_string.call(null,\":0\")"
org.mozilla.javascript.EcmaError: TypeError: Cannot read property "0.0" from null (file:/home/chas/dev/clojure/cljs/.repl/cljs/reader.js#451)

The topic of leading digits in keywords came up separately, as they've been supported in Clojure for some time, but can now be considered part of the spec, as it were. See CLJ-1286.

BTW, this is another simple-check win...



 Comments   
Comment by Chas Emerick [ 21/Nov/13 9:38 AM ]

This is not a simple regex change, as I had hoped given the recent flurry in Clojure. The symbol pattern in cljs.reader is faithful to Clojure HEAD, but the processing of matches isn't. I think it may be a wash as to whether it'd be easier to fix what's there vs. porting clojure.tools.reader.impl.commons/parse-symbol (which incidentally doesn't use a regex)…either way, leaving it for another day (or someone else, if they're up for it).





[CLJS-676] source maps get out of sync if not using :optimizations :none Created: 10/Nov/13  Updated: 18/Nov/13  Resolved: 18/Nov/13

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None


 Description   

This is probably because of compiled-cljs in closure.clj - it memoizes without being aware of recompilation. A problem for auto builds.



 Comments   
Comment by David Nolen [ 18/Nov/13 7:59 AM ]

fixed, https://github.com/clojure/clojurescript/commit/44c932a1b852818e0b364d0639083c394101f48f





[CLJS-675] QuickStart example not working properly Created: 10/Nov/13  Updated: 10/Nov/13

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

Type: Defect Priority: Minor
Reporter: Anton Logvinenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

To reproduce only have to follow simple steps as described in https://github.com/clojure/clojurescript/wiki/Quick-Start
git clone git://github.com/clojure/clojurescript.git
cd clojurescript
./script/bootstrap



 Description   

Section "Using ClojureScript on a Web Page"
Executing:

./bin/cljsc hello.cljs '{:optimizations :advanced}' > hello.js

produces error message "java.io.FileNotFoundException: out/constants_table.js (No such file or directory)"

Just creating "out" directory seems to be fixing the problem. The directory is then deleted after the compilation.

mkdir out
./bin/cljsc hello.cljs '{:optimizations :advanced}' > hello.js

Also, "out" directory is mentioned in .gitignore.



 Comments   
Comment by Anton Logvinenko [ 10/Nov/13 5:28 AM ]

UPD: The "out" directory is not deleted after the compilation. It's simply ignored by git.





[CLJS-674] Relative paths are incorrect when source map isn't in same directory as project.clj Created: 09/Nov/13  Updated: 21/Nov/13  Resolved: 20/Nov/13

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

Type: Defect Priority: Major
Reporter: George Fraser Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

CLJS 2030


Attachments: Text File fix-674.patch    
Patch: Code

 Description   

For example:

(defproject sandbox "0.1.0"
  :dependencies [[org.clojure/clojure "1.5.1"]
                 [org.clojure/clojurescript "0.0-2030"]]

  :plugins [[lein-cljsbuild "1.0.0-alpha2"]]

  :source-paths ["src"]

  :cljsbuild { :builds [{:id "sandbox"
                         :source-paths ["src"]
                         :compiler { :output-to "dist/main.js"
                                     :output-dir "dist/out"
                                     :optimizations :advanced
                                     :source-map "dist/main.js.map"}}]})

Chrome looks for dist/dist/out/foo.cljs instead of dist/out/foo.cljs
You can manually fix it by opening dist/main.map.js and changing all the dist/out to out.
I think the general solution is that all the paths must be relative to the parent of :source-map

See: https://github.com/fivetran/cljs-source-maps-incorrect-paths

Potentially related to http://dev.clojure.org/jira/browse/CLJS-591



 Comments   
Comment by David Nolen [ 10/Nov/13 2:05 PM ]

:source-map should probably only allow a file name, not a path - it should always be placed in the same directory as the single generated JS file. Before attempting to address we should probably consider limiting what is supported - I'm inclined to say that all paths must be relative to :output-to for simplicity.

Comment by Tim Visher [ 15/Nov/13 5:23 PM ]

This patch (fix-674.patch on 15/NOV/13) allows users to specify output-dir or not (defaulting to "out", as usual), and relativizes paths consistently between both the intermediary compilation phase and the final source-map output.

Comment by David Nolen [ 16/Nov/13 10:02 AM ]

Could we get a squashed patch please? Thanks!

Comment by Tim Visher [ 16/Nov/13 1:42 PM ]

fix-674.patch @ 16/Nov/13 1:41 PM

Sorry about that. Used to pull requests. (_)b

Comment by David Nolen [ 16/Nov/13 8:37 PM ]

So reviewed the patch - I'm inclined to just pick the simplest approach, I think :output-dir :source-map can only specify a name, nothing more.

Comment by Tim Visher [ 17/Nov/13 11:56 AM ]

I would think that the more sensible approach would be that `:output-to` and `:source-map` would be names, and `:output-dir` would be a required entry in the options map. That way all output goes to the `:output-dir` (which make the name make sense to me), and the file resulting from compilation would reside at its top level, as well as the source map, if specified.

In either case, I wonder if we should issue some sort of warning when using source maps that they are alpha and subject to change, even though that maybe should already be obvious.

As a more general thought, most of why I did it the way I did so far was to avoid breaking people's extant code. Do we feel like this is an alpha enough feature that we're ok with breaking it for people? I suppose that thread you (David) started was silent enough that people don't seem to have much of an opinion.

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

Tim, I thought about it some more. I think my idea of only supporting names is probably unwise because of the resulting breakage for lein-cljsbuild etc. I think your approach is ok, however I note that some combinations result in confusing exceptions for example:

:output-to "resources/js/main.js"
:output-dir "out"

If we can address cases like this I think your patch is fine.

Comment by David Nolen [ 17/Nov/13 12:16 PM ]

Related, CLJS-683

Comment by David Nolen [ 18/Nov/13 11:49 AM ]

Can we delete old patches please?

In the latest patch the how directories and files are detected is unlikely to work across platforms. For the next release I would like to resolve outstanding issues for Windows users that want source maps.

Comment by Tim Visher [ 18/Nov/13 11:52 AM ]

I put together a patch that would follow some of the intransigents we're defining here (output-dir must be a relative directory, output-to and source-map can only specify a single file). I like this approach as the other option, I think, basically requires special configuration on the server.

I also have it output warnings if it needed to muck with your configuration at all so that people aren't silently confused.

Also, because we sanitize input before hand, it allows the code further in to be a bit simpler.

@David: With your comments I tried out your example with the current patch and it throws, so I'll need to dig in further. I'll keep this up for historical benefit but it doesn't look ready yet.

fix-674-follow-assumptions-about-nature-of-output-dir-output-to-source-map.patch @ 18/Nov/13

Comment by David Nolen [ 18/Nov/13 12:04 PM ]

Tim, I prefer the earlier approach + validation. Anything else will likely just be a source of confusion. CLJS-683 solves the server configuration issue already.

For validation purposes, I think we should just enforce that :output-dir and :source-map be in the same directory as :output-to and call it a day.

Comment by Tim Visher [ 18/Nov/13 2:08 PM ]

fix-674.patch @ 18/NOV/13

Adds in assertions for the following conditions:

1. output-to must be a relative file
2. output-dir must be a relative directory
3. output-to's .getParent must be or contain output-dir
4. if you have named a source-map, its .getParent must be output-to's .getParent
5. if you have named a source-map, it must be a relative file

Comment by Tim Visher [ 18/Nov/13 2:16 PM ]

Looks like I stomped on source-map-path from CLJS-683. I'll need to refactor. :\

What's the semantics of the options at this point?

A full config might look like:

:output-to "a/relative/dir/app.js"
:output-dir "a/relative/dir/out"
:source-map "a/relative/dir/app.js.map"
:source-map-path "a/relative/dir/maps"

I don't follow what the `:source-map-path` option is getting us over and above `:output-dir`. Anyway, fix-674.patch @ 18/NOV/13 works in all the contexts we've listed so far without this, but I'm just not following if we're trying to use `:source-map-path` in other parts of the code that I'm not thinking of.

Comment by David Nolen [ 18/Nov/13 2:30 PM ]

:source-map-path allows arbitrarily specifying the prefix for paths that appear in the source map file itself. It simplifies web server configuration.

Comment by Tim Visher [ 18/Nov/13 3:32 PM ]

I don't have time to do it right now, but I'm getting a little concerned by the number of knobs we're introducing to support source maps. I think a simpler design is possible. I'd like to whip together a listing of a couple of example configurations and their resulting output before developing this much further as I think the design is changing faster than I (at least) am implementing it.

I need to be offline for the next few hours but I'll try do this before the end of the night so that when I or whoever else finishes this out, we can start from solid ground.

@David: Specifically, what I was asking about was what the prefix attached to the paths in the source-map file would be? In that case would it be `maps/`?

Comment by David Nolen [ 18/Nov/13 3:48 PM ]

:source-map-path does something not supported by any of the other source map flags, nor does your patch address the problem it's intended to solve. Currently if you specify some relative path, the entire relative path will prefixed to the path of every compiled file. So instead of having to deal with "js/out/foo.js" you will have to configure your web server to understand "public/resources/js/out/foo.js". The :source-map-path option gives you a way to avoid this.

Comment by Chas Emerick [ 18/Nov/13 3:51 PM ]

This may be a non sequitur, but I'm seeing some concern re: lein-cljsbuild configuration breakage wagging the dog of how the compiler should structure its options. I'm largely a spectator around source maps, but my suggestion (/me puts on his cljsbuild maintainer hat) is to make the CLJS compiler how it should be, and let the tooling work itself out. Taking on debt around configuration / API changes to satisfy tools written to match a known-insufficient existing "schema" doesn't make sense. If downstream coordinating changes are necessary, we can make that happen.

That said, source map configuration is a compiler option, which lein-cljsbuild passes through unmodified AFAIK. Unless I'm wrong there, then we're just talking about breakage of existing user config...for which I'd take the same perspective as above. If there's a right way, make it so now; later will hurt more.

Comment by David Nolen [ 18/Nov/13 4:20 PM ]

I think making :output-dir and :source-map relative to :output-to is more, not less confusing. People already understand making the path relative to where the JVM started up. What I think is correct path forward for this ticket - a) fix the broken cases, b) validate inputs so the user understands what is required.

Comment by Tim Visher [ 18/Nov/13 9:08 PM ]

I'll attempt to outline the design I think makes the most sense so people can agree or disagree with that specifically.

I believe we need 3 keys: :output-to, :output-dir (not crazy about that name but can't come up with anything better…), and :source-map.

I want to require :output-to and :output-dir. Requiring :output-dir is not strictly necessary, but I think it's sufficiently useful in a wide variety of workflows that it should be called out as required just to teach people about it. Alternatively, it could simple emit a warning if the default value, out, is being used.

:output-to should name a file. It can be a relative path, in the case of a typical ring served app, or absolute path, in the case of a file that should be served out of some pre-existing server.

:output-dir should name a directory. It can be a relative or absolute path, depending on the goal. It will hold the compiled intermediary files as well as all duplicated source files.

:source-map is not required, but if it is specified, it should denote a relative or absolute file. It will contain relativized references to source files in :output-dir.

:output-dir and :output-to must be independent of one another to support the widest variety of workflows. Specifically, you must be able to :output-to a separate directory from :output-dir in order to support using build to prepare a directory for directoly syncing to a server so that it contains nothing but the desired output.

Because :output-dir and :output-to must already be independent of one another, I believe it makes sense to specify :source-map as indepedent of both of them, again to support the widest variety of workflows. Some of those workflows might not make sense, but at this level it probably pays to be permissive rather than prescriptive.

The pro I see of making all three of these configuration inputs independent of one another is that it supports a wide variety of workflows not possible without further scripting when you tie any of them to each other.

For example, if :output-dir was the forced parent of all build output, then a workflow which intended to run a build and then sync the output to a server would have to specify an exclude option or do some further munging.

The biggest con is that there is a lot of repetition. I think the repetition can be forgiven given the explicitness of the configuration (explicit first, later by convention?) and the wide variety of workflows that it supports.

I'll now list out examples of configurations and their build output to illustrate why I named the intransigents I listed above.

;; The hello example
{:output-to  "hello.js"
 :output-dir "out"}
=>
./hello.js
./out/goog/base.js
./out/goog/…
;; The hello example with the alternative idea of not requiring :output-dir
{:output-to  "hello.js"}
=>
*WARNING* Using "out" as :output-dir
*WARNING* Consider setting :output-dir in your compiler options to use a consisent location for build output.
./hello.js
./out/goog/base.js
./out/goog/…
;; A 'typical' cljsbuild setup, with ring doing the serving during development.
{:output-to  "resources/public/js/app.js"
 :output-dir "resources/public/js"}
=>
./resources/public/js/app.js
./resources/public/js/goog/base.js
./resources/public/js/goog/…
;; A 'typical' Mac workflow, with the built in apache server serving the documents
{:output-to  "/Library/WebServer/Documents/my-great-app/js/app.js"
 :output-dir "/Library/WebServer/Documents/my-great-app/js"}
=>
/Library/WebServer/Documents/my-great-app/js/app.js
/Library/WebServer/Documents/my-great-app/js/goog/base.js
/Library/WebServer/Documents/my-great-app/js/goog/…
;; A production build intended to support bare syncing of a directory to a server
{:output-to  "sync/app.js"
 :output-dir "target/advanced"}
=>
./sync/app.js
./target/advanced/goog/base.js
./target/advanced/goog/…
;; The hello example with source maps
{:output-to  "hello.js"
 :output-dir "out"
 :source-map "hello.js.map"}
=>
./hello.js
    //# sourceMappingURL=hello.js.map
./out/goog/base.js
./out/goog/…
./hello.js.map
    "sources": ["out/goog/base.js" "out/goog/…"]
;; A 'typical' cljsbuild setup, with ring doing the serving during development.
{:output-to  "resources/public/js/app.js"
 :output-dir "resources/public/js"
 :source-map "resources/public/js/app.js.map"}
=>
./resources/public/js/app.js
    //# sourceMappingURL=app.js.map
./resources/public/js/goog/base.js
./resources/public/js/goog/…
./resources/public/js/app.js.map
    "sources": ["goog/base.js" "goog/…"]
;; A 'typical' Mac workflow, with the built in apache server serving the documents
{:output-to  "/Library/WebServer/Documents/my-great-app/js/app.js"
 :output-dir "/Library/WebServer/Documents/my-great-app/js"
 :source-map "/Library/WebServer/Documents/my-great-app/js/app.js.map"}
=>
/Library/WebServer/Documents/my-great-app/js/app.js
    //# sourceMappingURL=app.js.map
/Library/WebServer/Documents/my-great-app/js/goog/base.js
/Library/WebServer/Documents/my-great-app/js/goog/…
/Library/WebServer/Documents/my-great-app/js/app.js.map
    "sources": ["goog/base.js" "goog/…"]
;; A 'typical' Windows workflow, with the an apache server serving the documents
{:output-to  "c:/htdocs/my-great-app/js/app.js"
 :output-dir "c:/htdocs/my-great-app/js"
 :source-map "c:/htdocs/my-great-app/js/app.js.map"}
=>
c:/htdocs/my-great-app/js/app.js
    //# sourceMappingURL=app.js.map
c:/htdocs/my-great-app/js/goog/base.js
c:/htdocs/my-great-app/js/goog/…
c:/htdocs/my-great-app/js/app.js.map
    "sources": ["goog/base.js" "goog/…"]
;; A production build intended to support bare syncing of a directory to a server, potentially non-sensical
{:output-to  "sync/app.js"
 :output-dir "target/advanced"
 :source-map "sync/app.js.map"}
=>
./sync/app.js
    //# sourceMappingURL=app.js.map
./target/advanced/goog/base.js
./target/advanced/goog/…
./sync/app.js.map
    "sources": ["../target/advanced/goog/base.js" "../target/advanced/goog/…"]
;; serving source-map from parent dir for some insane reason
{:output-to  "resources/public/js/app.js"
 :output-dir "resources/public"
 :source-map "resources/public/source-map.js.map"}
=>
./resources/public/js/app.js
    //# sourceMappingURL=../source-map.js.map
./resources/public/goog/base.js
./resources/public/goog/…
./resources/public/source-map.js.map
    "sources": ["goog/base.js" "goog/…"]
;; serving sources from one dir, app from another, and soure-map from a third, because crazy
{:output-to  "resources/public/js/app.js"
 :output-dir "target/advanced"
 :source-map "resources/public/source-map.js.map"}
=>
./resources/public/js/app.js
    //# sourceMappingURL=../source-map.js.map
./target/advanced/goog/base.js
./target/advanced/goog/…
./resources/public/source-map.js.map
    "sources": ["../../target/advanced/goog/base.js" "../../target/advanced/goog/…"]
Comment by Tim Visher [ 18/Nov/13 9:17 PM ]

@David: In http://dev.clojure.org/jira/browse/CLJS-674?focusedCommentId=32700&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-32700 you said that my patch doesn't address the concern listed there. I believe it does.

I just ran

(build "samples/hello/src" 
       {:optimizations :whitespace
        :output-dir "samples/hello"
        :output-to "samples/hello/hello.js" 
        :source-map "samples/hello/hello.js.map"})

And it produced the right output:

./samples/hello/hello.js
    //# sourceMappingURL=hello.js.map
./samples/hello/goog/base.js
./samples/hello/goog/…
./samples/hello/foo/bar.cljs
./samples/hello/hello.js.map
    "sources": ["goog/base.js" "goog/…" "hello/foo/bar.cljs"]

This is as I expected. I no longer think the patch is The Right Thing To Do™, but I think it addresses all the concerns we listed before my little design session, and it doesn't need source-map-path.

Am I missing something?

FWIW, the name source-map-path doesn't indicate to me that it's the path that will be prepended, in alternation to output-dir, to the relative source listings inside the source map. Others may feel differently, but that's my opinion. Maybe a better name would be source-file-request-path-parent (which is pretty bad, I know) to specifically denote that it's to specify something for the requests against the server.

Comment by Tim Visher [ 18/Nov/13 9:30 PM ]

@David: Good catch regarding cross platform detection of directories and files. I read up a little bit on java.io.File and there's some goodness I can use from there pretty easily. I don't want to do too much more development till the design is locked down, but it shouldn't be any trouble to make it work cross-platform.

Comment by David Nolen [ 19/Nov/13 10:11 AM ]

The problem with your new proposal - the directory contains both transient compiler artifacts and the final output source. This is undesirable, unless I hear more compelling arguments we should keep :source-map-path and move on.

Comment by Tim Visher [ 19/Nov/13 3:17 PM ]

fix-674.patch @ 19/NOV/13 should address all the concerns listed above, sans the functionality I was hoping for.

Specifically, it checks the following intransigents in a Host independent fashion.

Key-spec:

:output-to
:output-dir
[:source-map
 :source-map-path]

Relationship Rules:

1. :output-to names an absolute or relative file
2. :output-dir names an absolute or relative directory, which must be or be contained by the directory containing :output-to
3. :source-map and :source-map-path are optional but must be specified together if at all.
4. If specified, :source-map must name a file that is a sibling to :output-to.
5. If :source-map is specified, :source-map-path must also be specified. Because of the behavior of it, I can't figure out a good way to verify that it's 'correct'. It essentially has to be the unique portion of output-dir against output-to.

I have no new ways to articulate why I think this is a bad direction and so I'll drop the issue and submit the patch that I believe fulfills all of David's requirements. One last time, specifically:

1. I can't figure out a way to make this support creating a one-stop sync dir
2. Because we require :output-dir to be within the same directory as :output-to, build output will be mixed with the source code as a rule.
3. Much more repetition is required amongst all the configuration options (you must specify the same directory prefix 3 times for a source-map)
4. the :source-map-path option, confusingly, requires you to leave off the common directory and only specify the unique portion of the :output-dir option, which is something we could easily have derived.

Anyway, I'm sure I'm just missing something, which would make the patch perhaps not correct. I just want something that support relative source-maps to make it in!

Comment by David Nolen [ 19/Nov/13 3:33 PM ]

I don't see any use for :source-map-dir, it should always be exactly the same as :output-dir.

Comment by Tim Visher [ 19/Nov/13 3:46 PM ]

@David: Whoops! Forgot to replace that. I had remembered the name wrong when brainstorming about it. The patch doesn't reference that var.

Comment by David Nolen [ 19/Nov/13 3:48 PM ]

I don't see why :source-map-path needs to provided, it should be completely optional.

Comment by Tim Visher [ 19/Nov/13 3:59 PM ]

Because otherwise we'll just prepend the relative paths with the value of :output-dir which is almost certainly the wrong behavior (it's where we were before this patch, prepending things like "a/relative/out/dir/" to the relative path). Unless we also want to dedup the resulting relative source file with the value of source-map or something, which wouldn't be that hard to add in to the patch.

Comment by David Nolen [ 19/Nov/13 4:08 PM ]

This doesn't follow given the restrictions. Since :output-dir must always be located in the same directory as :source-map, it will simply be the last component of the :output-dir path. :source-map-path is just about overriding that behavior.

Comment by Tim Visher [ 19/Nov/13 4:25 PM ]

Good point. All that's left on this then is fixing the intransigents (specifically removing the requirement of specifying a :source-map-path if a :source-map is specified) and then also finishing the relativizing of the paths is source_map.clj. I'll be offline for a few hours but I should be able to finish this out tonight.

I promise I'm not trying to be obtuse! Thanks for your patience.

Comment by Tim Visher [ 19/Nov/13 5:07 PM ]

As a matter of fact, if you're not specifying :source-map then we don't need the constraint of :output-dir being in the same directory as :output-to, which would make setting up a prod sync very easy.

So

{:output-to "resources/public/js/hello.js"
 :output-dir "target/dont/sync/me"}

Would be legal but

{:output-to "resources/public/js/hello.js"
 :output-dir "target/dont/sync/me"
 :source-map "resources/public/js/hello.js.map"}

would not.

How do we feel about that?

Comment by David Nolen [ 19/Nov/13 5:09 PM ]

I'm ok with that as long as the second case throws an intelligible error.

Comment by Tim Visher [ 20/Nov/13 7:49 AM ]

fix-674.patch @ 20/NOV/13 I think is the final version!

Specifically, it checks the following intransigents in a Host independent fashion.

Key-spec:

:output-to
:output-dir
[:source-map]
[:source-map-path]

Relationship Rules:

1. :output-to names an absolute or relative file
2. :output-dir names an absolute or relative directory, which must be or be contained by the directory containing :output-to if :source-map is also specified.
3. :source-map is optional. If specified, it must name a file in the same directory as :output-to
4. :source-map-path is optional. If specified, it must name a directory.

This relaxes the requirement that :output-dir name a relative directory only so that you can set up a prod sync system easily while keeping it (the requirement) if you also want source maps.

IMO, the error messages are quite informative.

/me crosses fingers.

Comment by David Nolen [ 20/Nov/13 10:06 AM ]

The enforcement that :output-to provide a file is not correct, leaving it out means that the output will be sent to standard out. Otherwise looks promising, fingers crossed it addresses our Windows issues too

Comment by Tim Visher [ 20/Nov/13 10:43 AM ]

Interesting. Hadn't considered that use case.

So, at this point is the following more accurate?

Key-spec:

[:output-to
 :output-dir
 [:source-map]
 [:source-map-path]]

Relationship Rules:

1. :output-to is optional. If specified, it names an absolute or relative file
2. :output-dir is required if :output-to is specified. If specified, it names an absolute or relative directory, which must be or be contained by the directory containing :output-to if :source-map is also specified.
3. :source-map is optional. If specified, it must name a file in the same directory as :output-to, and :output-to and :output-dir must also be specified.
4. :source-map-path is optional. If specified, it must name a directory, and :source-map, :output-to, and :output-dir must also be specified.

I'll move ahead with this understanding but I wanted to give everyone the opportunity to correct me.

Comment by David Nolen [ 20/Nov/13 10:54 AM ]

Even :output-dir is optional, if not provided it defaults to "out".

Comment by Tim Visher [ 20/Nov/13 11:22 AM ]

LOL. That's good because that's what I coded. (I'm in the wrong business...)

Anyway, fix-674.patch @ 20/NOV/13 should address all the issues. :output-to is no longer required. The cascade goes:

If :output-to is specified, check that it's a file (using string?).

If :output-dir is specified, check that it's a directory (using string?) and that :output-to has been specified.

If :source-map is specified, check that it's a sibling file to :output-to and that :output-dir is specified and names a directory within the parent of :output-to. :output-to and :output-dir must have also been specified.

If :source-map-path is specified, check that it's a file (using string?) and ensure that :output-to, :output-dir, and :source-map have also been specified.

Comment by David Nolen [ 20/Nov/13 11:34 AM ]

For checking whether something is a file/directory shouldn't we be using the existing Java facilities?

Comment by Tim Visher [ 20/Nov/13 11:57 AM ]

I was trying to figure out a way to do that. The problem is that .isFile and .isDirectory only return true if the path exists and is of that type. See http://stackoverflow.com/questions/8648793/java-isfile-isdirectory-without-checking-for-existence

I had toyed around with the idea of creating a tmp file to verify that it can indeed be a file, but that seemed more complicated than it was worth.

Comment by David Nolen [ 20/Nov/13 12:01 PM ]

Ah good point, those approaches for validation work for me then.

Comment by David Nolen [ 20/Nov/13 12:03 PM ]

The patch does not work for me, I get an assertion about :output-to needing to be a file still. Please make sure to run the tests ./script/test and verify that the REPL works ./script/repljs.

Comment by Tim Visher [ 20/Nov/13 12:17 PM ]

Interesting. It works for me at repl running (build "samples/hello/cljs" {:optimizations :whitespace}).

I'm trying to get up and running with ./script/test now (need to install spidermonkey, v8, and jsc).

Are you running that or did you run something directly that you could paste in?

Comment by David Nolen [ 20/Nov/13 12:26 PM ]

I always run ./script/test and ./script/repljs before pushing a commit. Note you don't need to setup every single JS engine - v8 is enough for this.

Comment by Tim Visher [ 20/Nov/13 12:36 PM ]

Thanks for the tip.

So it's been a long time since I was manually messing with the CLASSPATH at the command line… I'm running into issues with tools.reader not being present as well as clojure.instant.

Reading the scripts, it looks like it expects everything to be in lib but the version of Clojure there is quite old and doesn't have clojure.instant, and tools.reader isn't even there.

Also, testing this on master doesn't produce a different result.

Anyone able to help bootstrap me here? :\

Comment by David Nolen [ 20/Nov/13 12:39 PM ]

You need to run ./script/bootstrap first.

Comment by Tim Visher [ 20/Nov/13 12:41 PM ]

Yep. Thanks!

Comment by Tim Visher [ 20/Nov/13 12:45 PM ]

Ah, I had forgotten about the `:print` use case. Shouldn't be too hard to fix.

Comment by Tim Visher [ 20/Nov/13 12:57 PM ]

fix-674.patch @ 20/NOV/13. Go!

This fixes the {:output-to :print} case.

I get a couple failures (ethel count, integer?, etc.) but they don't seem to be at all related to anything I've done and fail on master as well.

Comment by David Nolen [ 20/Nov/13 1:28 PM ]

fixed, https://github.com/clojure/clojurescript/commit/047dbb3d2bd7c3a2e00805ec2f2480e449451521

Comment by David Powell [ 20/Nov/13 7:40 PM ]

This patch introduces the use of java.io.File.toPath() - which isn't available prior to Java 7.
Is that intended?

Comment by Tim Visher [ 21/Nov/13 9:50 AM ]

Not completely, no.

What version of Java do we support? I'm sure I can rework this in terms of older versions if that's necessary. Can we settle on Java 6 since that's what Clojure supports at this point?

Comment by David Nolen [ 21/Nov/13 10:04 AM ]

Yes to 1.6. Put lets open a new ticket for this patch please. I'll cut another release fixing this issue when I get a patch.

Comment by Tim Visher [ 21/Nov/13 10:19 AM ]

http://dev.clojure.org/jira/browse/CLJS-694





[CLJS-673] add support for *print-level* Created: 09/Nov/13  Updated: 03/Dec/13  Resolved: 03/Dec/13

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 03/Dec/13 10:56 AM ]

fixed, https://github.com/clojure/clojurescript/commit/13d49ec0180dc5199b580a3c9e46c0eba171cfe4





[CLJS-672] Source maps break user-defined javascript namespaces Created: 09/Nov/13  Updated: 18/Nov/13  Resolved: 18/Nov/13

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

Type: Defect Priority: Major
Reporter: George Fraser Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None
Environment:

CLJS 2030



 Description   

With source maps on, user-defined javascript modules are broken:

project.clj:

(defproject sandbox "0.1.0"
  :dependencies [[org.clojure/clojure "1.5.1"]
                 [org.clojure/clojurescript "0.0-2030"]]

  :plugins [[lein-cljsbuild "1.0.0-alpha2"]]

  :source-paths ["src"]

  :cljsbuild { :builds [{:id "sandbox"
                         :source-paths ["src"]
                         :compiler { :libs ["src/bar.js"]
                                     :output-to "main.js"
                                     :output-dir "out"
                                     :optimizations :advanced
                                     :source-map "main.js.map"}}]})

foo.cljs:

(ns foo
  (:require bar))

(defn ^:export fooFunction []
  (bar/barFunction))

bar.js:

goog.provide("bar");

var bar = {};

bar.barFunction = function() {
  return "barResult";
};

goog.exportSymbol("bar.barFunction", bar.barFunction);

Compilation throws the error:

No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil

Deleting one line from project.clj fixes it:

:source-map "main.js.map"

See https://github.com/fivetran/cljs-source-map-breaks-js



 Comments   
Comment by David Nolen [ 09/Nov/13 7:36 PM ]

Thanks for the report and minimal case will look into it.

Comment by David Nolen [ 17/Nov/13 12:47 PM ]

Can we get the full stack trace? Thanks.

Comment by David Nolen [ 18/Nov/13 8:34 AM ]

fixed, https://github.com/clojure/clojurescript/commit/34821a2595c194a9acee9b122cd331fbf495a7a8





[CLJS-671] Inconsistencies when calling 'name' on keywords from strings vs literal keywords Created: 08/Nov/13  Updated: 09/Nov/13  Resolved: 08/Nov/13

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

Type: Defect Priority: Major
Reporter: Joseph Smith Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: keywords
Environment:

Clojurescript r2024; Java 1.7.0_25; OSX 10.9



 Description   

Clojurescript:
1. (keyword "some.name.spaced/keyword")
:some.name.spaced/keyword

2. (name (keyword "some.name.spaced/keyword"))
"some.name.spaced/keyword"

3. (name :some.name.space/keyword)
"keyword"

I'd expect 2 and 3 to return the same string, given 1 returns what looks like the keyword I give to 3.

In contrast to Clojure:
(keyword "some.name.spaced/keyword")
:some.name.spaced/keyword

(name (keyword "some.name.spaced/keyword"))
"keyword"

(name :some.name.space/thing)
"thing"



 Comments   
Comment by David Nolen [ 08/Nov/13 4:52 PM ]

CLJS-660





[CLJS-670] Fix/enhance ^:export to work with properties that will be modified externally Created: 08/Nov/13  Updated: 11/Nov/13  Resolved: 11/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File CLJS-670.patch    
Patch: Code

 Description   

Thin line between bug/enhancement here, but whatever...

Given this code:

(ns foo.bar)

(def ^:export baz 42)

(defn ^:export quux [] baz)

Advanced compilation yields code where baz is inlined, and even the property itself disappears. Whatever a downstream user/app/lib does to foo.bar.baz, quux will always return 42. Here's that code:

var d = this;
function f(g, e) {
  var b = g.split("."), a = d;
  b[0] in a || !a.execScript || a.execScript("var " + b[0]);
  for (var c;b.length && (c = b.shift());) {
    b.length || void 0 === e ? a = a[c] ? a[c] : a[c] = {} : a[c] = e;
  }
}
;f("foo.bar.baz", 42);
f("foo.bar.quux", function() {
  return 42;
});

Initially, I thought this was only because baz was a scalar, but the problem remains the same if an exported fn is returned by another fn; if the former is set to a different value downstream, the latter will continue returning the original that it closed over.

Based on this exchange with a GClosure compiler contributor, we need to use the @expose annotation on each exported property, and export the symbol for each intervening "namespace" segment in order to ensure that functions that refer to exported properties will return the same value that downstream users (might) mutate.



 Comments   
Comment by Chas Emerick [ 08/Nov/13 2:50 PM ]

Attached patch that changes the effect of ^:export as described above. With this patch, this input:

(ns foo.bar)

(def ^:export baz 42)

(defn ^:export quux [] baz)

(defn ^:export a-fn [] 88)

(defn ^:export returns-a-fn [] a-fn)

yields this under advanced compilation:

var a = this;
function b(h, f) {
  var d = h.split("."), c = a;
  d[0] in c || !c.execScript || c.execScript("var " + d[0]);
  for (var e;d.length && (e = d.shift());) {
    d.length || void 0 === f ? c = c[e] ? c[e] : c[e] = {} : c[e] = f;
  }
}
;var g = {a:{}};
g.a.baz = 42;
b("foo", g);
b("foo.bar", g.a);
g.a.quux = function() {
  return g.a.baz;
};
b("foo", g);
b("foo.bar", g.a);
g.a.a_fn = function() {
  return 88;
};
b("foo", g);
b("foo.bar", g.a);
g.a.returns_a_fn = function() {
  return g.a.a_fn;
};
b("foo", g);
b("foo.bar", g.a);

No exported properties are inlined, and external modifications to baz or a-fn affect functions that refer to them as expected.

Comment by Chas Emerick [ 08/Nov/13 4:24 PM ]

BTW, please consider that patch a draft. I'd like to work out some way to test this...

Comment by David Nolen [ 09/Nov/13 7:38 PM ]

Approach seems reasonable.

Comment by Chas Emerick [ 09/Nov/13 8:52 PM ]

It's subtly broken, discovered only after using it to build/test some real code. Working on it.

Comment by Chas Emerick [ 11/Nov/13 10:57 AM ]

There are various problems with the attached patch / approach, all rooted in GClosure's @expose:

1. More than preventing the munging/renaming/etc of a property, @expose disables a variety of advanced optimizations and analysis. When applied to functions, this will result in potentially a significant perf hit compared to what CLJS emits now.
2. More irritating than problematic is that a side effect of @expose is that the way we are transliterating CLJS namespaces into JavaScript "namespaces" yields spurious "incomplete alias created for namespace XXX" warnings when @expose is used within the affected namespace (even though projects that use ^:export continue to test well with the proposed patch). This is apparently a known issue.
3. @expose doesn't affect only the annotated property; its don't-rename and don't-optimize policies carry over to any same-named property anywhere in the codebase. e.g. (defn ^:export foo [] ...) in one namespace will cause any other foo vars in all other namespaces to not be renamed, inlined, optimized, etc.

Much of the above is reiterated elsewhere on the web, e.g. https://zooskdev.wordpress.com/2013/11/05/integrating-angularjs-with-a-large-google-closure-codebase/

Stepping back a bit, the objective of ^:export is solely to ensure that the affected var/property retains the name we give it, nothing else. GClosure provides an escape hatch for preventing renaming already — string-based property access — which, when combined with exporting of the "namespace"'s property, would address the problems with the existing ^:export impl.

The big hurdle is that the compiler will need to use string-based property access when touching any ^:export ed var/property. Lifting this information into the compiler environment so it can be looked up is easy; the delicate part will be changing any part of the compiler that emit var/property names to use a helper that distinguishes between locals and vars and uses string-based property access for the latter that are ^:export ed. This seems quite doable, but would be a more significant compiler change than simply twiddling goog.export* calls and such, so I thought I'd solicit feedback before moving forward.

Comment by Chas Emerick [ 11/Nov/13 11:18 AM ]

Per feedback from @dnolen in #clojure, ^:export is not intended to support mutable properties, and the changes necessary to add that support are undesirable.

Workarounds:

  • provide "setters" for any configuration/parameterization users need to do
  • in a testing context, fixtures can look up parameters set on e.g. window ahead of time and either set! or use binding to configure the tests being run, etc.




[CLJS-669] namespaced keyword that uses a :require-macros alias broken Created: 08/Nov/13  Updated: 10/Nov/13  Resolved: 10/Nov/13

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJS-669.diff    
Patch: Code and Test

 Description   
ClojureScript:cljs.user> (ns foo.bar (:require-macros [clojure.core :as cc]))

ClojureScript:foo.bar> ::cc/blah
Invalid token: ::cc/b