<< Back to previous view

[CLJS-994] print a warning when :externs file paths can't be found. Created: 30/Jan/15  Updated: 01/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Crispin Wellington Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, enhancement, errormsgs, patch,
Environment:

Linux 64bit

java version "1.7.0_65"
OpenJDK Runtime Environment (IcedTea 2.5.3) (7u71-2.5.3-0ubuntu0.14.04.1)
OpenJDK 64-Bit Server VM (build 24.65-b04, mixed mode)


Attachments: Text File clojurescript-extern-missing-warning.patch    
Patch: Code

 Description   

clojurescript silently ignores missing externs files possibly leading a developer to chase their tail.

Presently it can be very confusing using advanced compilation if you have made a mistake in the path name of one of your :externs files. This patch makes the compiler print a warning on stderr so you can quickly determine the cause of the broken advanced compilation output.

As a side effect, when doing a basic lein-cljsbuild a warning is always printed:

```
WARNING: js resource path closure-js/externs does not exist
```

This is because lein-cljsbuild quietly adds this extra path to your :externs listing without you knowing.



 Comments   
Comment by David Nolen [ 31/Jan/15 1:59 PM ]

You need to bind *out* to *err*, or just print to it directly a la cljs.util/debug-prn.

Comment by Crispin Wellington [ 31/Jan/15 7:30 PM ]

I did bind out to err. Check the patch.

Comment by David Nolen [ 01/Feb/15 12:30 PM ]

Crispin, oops sorry you are correct. Thanks.





[CLJS-993] binding macro returns non-nil with empty body Created: 30/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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

Type: Defect Priority: Minor
Reporter: Stefano Pugnetti Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: binding, test
Environment:

Ubuntu Linux 12.04
Clojurescript 0.0-2740



 Description   

In Clojure the binding macro returns nil when called with an empty body. In Clojurescript it returns a non empty object.

In this repository
https://github.com/stepugnetti/binding-issue
I've put a simple project in which the two behaviors are compared. The same code

(defn foo []
  (if (= nil (binding [*test-var* 1]))
    (.log js/console "Ok!")
    (.log js/console "Too bad!")))

can be run with
1) Clojure: lein run
2) Clojurescript: lein cljsbuild once main && node target/binding-issue.js

I was expecting the same result (nil)...

See discussion at https://groups.google.com/forum/#!topic/clojurescript/anbDq9pjvEs



 Comments   
Comment by David Nolen [ 30/Jan/15 5:47 PM ]

fixed https://github.com/clojure/clojurescript/commit/996f33e5250712072eaefb5eff13bb9372d5e1b6





[CLJS-992] satisfies? does not work with locally bound symbols Created: 28/Jan/15  Updated: 30/Jan/15

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

Type: Defect Priority: Minor
Reporter: Richard Davies Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

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



 Description   
(ns sample)
(defprotocol ICustomer (doit [_]))
(defrecord Customer [fname])
(extend-type Customer
  ICustomer
    (doit [_] nil))
(let [t ICustomer]
  (js/alert (satisfies? t (Customer. nil))))

;satisfies? returns false but similar code in returns true.





[CLJS-991] Wrong inference - inconsistent behavior? Created: 28/Jan/15  Updated: 31/Jan/15  Resolved: 31/Jan/15

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

Type: Defect Priority: Minor
Reporter: Christian Fortin Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

The following function will cause a warning to appear when compiling:

(defn test-fn [a]
  (let [b (when a (aget js/document "documentElement" "clientHeight"))]
    (- 10 b 2)))

WARNING: cljs.core/-, all arguments must be numbers, got [number #{nil clj-nil}] instead. at line 12 src/hello_world/core.cljs

This one will compile without any problem:

(defn test-fn2 []
  (let [b (rand-nth [nil (aget js/document "documentElement" "clientHeight")])]

(- 10 b 2)))



 Comments   
Comment by David Nolen [ 31/Jan/15 2:04 PM ]

fixed https://github.com/clojure/clojurescript/commit/1d53aebbe60f41abb11e6c872184e95ce457c4dc





[CLJS-990] Clojurescript records do not have same equality semantics as Clojure records Created: 27/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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

Type: Defect Priority: Major
Reporter: Richard Davies Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

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



 Description   

(defrecord Pot [a])

(= (Pot. 1) (Pot. 1)) ; returns false

This arose when I was trying to get some code to cross-compile between Clojure and ClojureScript and the Clojure code was using records as map keys.

Workaround:

(extend-type Pot
IEquiv
(-equiv [this that] (and (instance? Pot that) (= (into {} this) (into {} that)))))

Can this behaviour be baked into ClojureScript records by default?



 Comments   
Comment by Richard Davies [ 27/Jan/15 11:27 PM ]

I tried with the latest version of ClojureScript and this works (in isolation). However, when I compile it along with the rest of my code, the equality test still fails without extend-type. I will try to isolate the root case.

Comment by David Nolen [ 30/Jan/15 3:07 PM ]

Cannot reproduce. Please do not reopen this ticket until a minimal case can be supplied thanks.





[CLJS-989] ClojureScript REPL loops on EOF signal Created: 27/Jan/15  Updated: 27/Jan/15

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

Type: Defect Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

ClojureScript 2740


Attachments: Text File CLJS_989.patch    

 Description   

If you run the cljs.repl outside of clojure.main and you CTRL+D, the REPL will loop infinitely



 Comments   
Comment by Bruce Hauman [ 27/Jan/15 3:45 PM ]

Here's a simple patch that fixes it.





[CLJS-988] Support async testing in cljs.test Created: 27/Jan/15  Updated: 28/Jan/15

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


 Description   

Async tests should take the form of:

(deftest foo
  (cljs.test/async done
    ...))

This should desugar into:

(deftest foo
  (reify
    cljs.test/IAsyncTest
    IFn
    (-invoke [_ done]
      ...)))

If test running code encounters an async test it should invoke the async test with the next step as the done parameter - it's simply a thunk for the continuation of testing.



 Comments   
Comment by Thomas Heller [ 28/Jan/15 4:40 AM ]

What are the benefits of using an empty marker protocol combined with IFn?

(defprotocol IAsyncTest
  (start-test [_ done]))

Seems like it would do the trick just fine since we will probably never have another arity?

Comment by David Nolen [ 28/Jan/15 6:08 AM ]

It really doesn't matter that much but it does mean the test runner code can be written in the usual functional continuation passing style.

(let [ret (test ...)]
  (if (async? ret)
    (ret k)
    ...))

vs.

(let [ret (test ...)]
  (if (async? ret)
    (start-test k)
    ...))

The former permits simple further functional combinations. The later does not. In this light it may be better to use function metadata to detect async test functions instead of a protocol.

(deftest foo
  (with-meta
    (fn [done]
      ...)
    {:async-test true}))




[CLJS-987] Introduce Special :main marker value to help with unit testing. Created: 26/Jan/15  Updated: 31/Jan/15  Resolved: 31/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Mike Thompson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

This is a follow on enhancement building on http://dev.clojure.org/jira/browse/CLJS-851 (simplify :none script inclusion if :main supplied)

I'd like to improve the unit test story for :optimizations :none

Explanation: https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L69-L93

I propose that if ":main" has a value of "*" then instead of single require, this loop would be put in:
https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L110-L111



 Comments   
Comment by David Nolen [ 27/Jan/15 12:20 PM ]

We're not going to do this. I would be up for allowing a sequence of namespaces.

Comment by Mike Thompson [ 27/Jan/15 3:23 PM ]

Instead of a sequence, how about namespace wildcarding? Eg: :main "some-ns.test.*"

Why? Well, I really, really don't want to manually maintain a sequence. It will be a guaranteed pit of failure. If I have 3 developers working on a project, each adding and deleting unit-test "cljs" files in a certain directory, manual maintenance of that sequence will be error prone, verbose and horrible. tests will be accidentally omitted, some might, wastefully be included twice, etc. Uggh.

I guess I'm saying that manual maintenance of a sequence does not scale up, in a unittest context.

All the good unit test frameworks I've used over the years (eg Nose under python) have a "a runner" ... you point it at a directory, and it recursively "finds" all the unittests in that directory. You never have to list the tests. Instead there is a set of rules around identifying tests:

  • must be in a (sub) directory called "test"
  • must start with "test_" ;; equivalent to wildcarding etc

These "rules" are pretty much just wildcards (which is what I'm proposing) ... discovery by pattern.

Comment by David Nolen [ 28/Jan/15 8:13 AM ]

Mike, we're just not going to do it sorry. Sugar like this is best handled by third party tools like lein-cljsbuild, figwheel, etc.

Also perhaps you're not aware of cljs.test/run-all-tests? It takes an optional regex.

Comment by Mike Thompson [ 28/Jan/15 7:36 PM ]

David, that you are suggesting "run-all-tests" as a solution means I've failed to explain the problem.

RUNNING all the tests is not hard in any of the unittest runners. That is not the issue.

The problem is that of LOADING (goog.require) the namespaces of the unittests WHEN you use `optimisations :none`. In that particular case, there is no root namespace you can `goog.require` which triggers all the unittests cljs into the browser (or Nodejs).

Note: if you use `:optimisation :simple` the problem disappears because all the code comes in one big blob. All the unittests are concatenated into the one file. Easy then to run them, when they are all loaded.

It is `:optimisations :none` which causes the problem, and only when combined with an unknown, flat set of unit-test namespaces (a directory of unittests in cljs files)

I can't do better than this explanation: https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L69-L93

And lein-cljsbuild can't help. Anyway, I've got a feeling you've given your answer on this. Maybe you'll revisit this subject when you get around to getting cljs.test to work with `:optimisations :none`. In the meantime, we'll continue to use the hack.

Comment by David Nolen [ 30/Jan/15 10:10 AM ]

Mike how would you accomplish the same thing in Clojure?

cljsbuild or third party testing tools can easily help. It's simple for them to find all ClojureScript files that match and generate a test runner namespace require the matched namespaces and emit the needed run-all-tests expression.

Comment by Mike Thompson [ 30/Jan/15 9:44 PM ]

I can achieve anything in clojure, or 8-bit assembler, given sufficient time and motivation

My comment about lein-cljsbuild not being able to help was more to do with this: it is not high enough in the tool chain to provide a general solution.

Instead, Boot will have to do as you suggest, and shadow-build will do it, and then cljsbuild could do the same as well. The fact that each of them has to repeat essentially the same thing, lead me to think that it should be factored "up the tool chain" to a level which is already generating requires. And this does appear to be a generalization of the new ":main" feature to me.

But, anyway, like I said above, we should close this ticket. Your time is too important to cljs and I don't want to waste another drop of it.

I'll continue to use the hack. cemeric has asked for it to be put ported into cemerick/clojurescript.test so there is a an `:optimisations :none` story. I guess I wondered if there was a chance of a more elegant fix. Thank you for considering it.

Comment by David Nolen [ 31/Jan/15 1:57 PM ]

No doing this for now. In future we may revisit permitting multiple entry points if people find the need for cases outside of just testing.





[CLJS-986] Add :target to the list of build options that should trigger recompilation Created: 21/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

Status: Resolved
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 [ 30/Jan/15 3:11 PM ]

fixed https://github.com/clojure/clojurescript/commit/1611bdc7fa7ef21ed2e8543afaefd81b516bacec





[CLJS-985] ex-info loses stack information Created: 20/Jan/15  Updated: 23/Jan/15  Resolved: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File cljs-985-ex-info-stack.patch    

 Description   

Native js Error keeps stacktrace:

(js/console.log (.-stack (js/Error. "message")))

Error: message
    at eval (/Users/prokopov/Dropbox/ws/datascript/test/test/datascript.cljs[eval5]:10:14)
    at eval (native)
    at SocketNamespace.<anonymous> (http://localhost:65000/socket.io/lighttable/ws.js:118:26)
    at SocketNamespace.EventEmitter.emit [as $emit] (http://localhost:65000/socket.io/socket.io.js:633:15)
    at SocketNamespace.onPacket (http://localhost:65000/socket.io/socket.io.js:2248:20)
    at Socket.onPacket (http://localhost:65000/socket.io/socket.io.js:1930:30)
    at Transport.onPacket (http://localhost:65000/socket.io/socket.io.js:1332:17)
    at Transport.onData (http://localhost:65000/socket.io/socket.io.js:1303:16)
    at WebSocket.websocket.onmessage (http://localhost:65000/socket.io/socket.io.js:2378:12)

But ex-info does not:

(js/console.log (.-stack (ex-info "message")))

Error
    at file:///Users/prokopov/Dropbox/ws/datascript/web/target-cljs/cljs/core.js:32066:38

Problem is that ex-info inherits stack property from prototype which is instantiated at script load time here:

(deftype ExceptionInfo [message data cause])

(set! (.-prototype ExceptionInfo) (js/Error.))
(set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)

The possible solution is to create new instance of js/Error at (ex-info) and manually copy stack property to ExceptionInfo object. Related SO: http://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript

Problem is that Chrome has setter on stack property, and it only allows for this property to be set inside a constructor functions.

Proposed fix creates new Error each time ex-info is called and sets ExceptionInfo.prototype to newly created error. This way new ExceptionInfo instance will inherit stack from newly created Error with correct stack.

This patch has been tested in Chrome 39 Mac, Safari 8 Mac, Firefox 35 Mac and IE 10 Win. Here's test code I used:

(defn -ex-info
  ([msg data]
    (set! (.-prototype ExceptionInfo) (js/Error msg))
    (set! (.. ExceptionInfo -prototype -name) "ExceptionInfo")
    (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
    (ExceptionInfo. msg data nil))
  ([msg data cause]
    (set! (.-prototype ExceptionInfo) (js/Error msg))
    (set! (.. ExceptionInfo -prototype -name) "ExceptionInfo")
    (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
    (ExceptionInfo. msg data cause)))

(try
  (throw (ex-info "[ -- Current ex-info message -- ]" 123))
  (catch ExceptionInfo e
    (js/console.log "Current ex-info::" (.-stack e))))

(try
  (throw (js/Error "[ -- Native message -- ]"))
  (catch js/Error e
    (js/console.log "Native error::" (.-stack e))))

(try
  (throw (-ex-info "[ -- Patched ex-info message -- ]" 123))
  (catch ExceptionInfo e
    (js/console.log "Patched ex-info::" (.-stack e))))

Test results:

Chrome, Firefox, IE, Safari

Note that current implementation reports line number and overall stacktrace from cljs.core file where Error prototype is created in current implementation.
Note that patched version reports correct line number (it should be close to native error stack), stack, message and exception name.
Also note that IE is fine even without patch — that's because in IE stack is capturead at throw place, not at new Error() call site.



 Comments   
Comment by Nikita Prokopov [ 20/Jan/15 2:48 PM ]

Ok, this is crazy, but this seems to solve the issue:

(defn ex-info [msg data cause]
  (set! (.-prototype ExceptionInfo) (js/Error msg))
  (set! (.. ExceptionInfo -prototype -name) "cljs.core.ExceptionInfo")
  (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
  (ExceptionInfo. msg data cause))

Basically we change prototype before creating each object.

(taken from http://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript#answer-12030032)

I guess high performance is not needed from ex-info, so this solution is somewhat okay-ish? Should I make a patch from it?

Comment by David Nolen [ 20/Jan/15 2:55 PM ]

It would be nice to get confirmation from others that this works under the major browser - Safari, Firefox, Chrome, and modern IE.

Comment by Nikita Prokopov [ 20/Jan/15 3:12 PM ]

I can confirm Firefox 34, Firefox 35, Safari 8.0.2 and Chrome 39 (all Mac) for now

Comment by Nikita Prokopov [ 22/Jan/15 3:50 AM ]

David, I updated issue, added patch, test code and test results (including IE). There’s no unit test on this because stack traces are very engine-specific. Please take a look

Comment by David Nolen [ 22/Jan/15 2:24 PM ]

Nikita, thanks for the update will check it out.

Comment by David Nolen [ 23/Jan/15 6:13 PM ]

fixed https://github.com/clojure/clojurescript/commit/93dce672e1af8f698cfc2a61e293cb48aeeddc2c





[CLJS-984] Update Node.js REPL support to use public API Created: 20/Jan/15  Updated: 20/Jan/15  Resolved: 20/Jan/15

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   

https://github.com/google/closure-library/commit/d909e5aa4b71923d72c15305f70d01a976c9947f



 Comments   
Comment by David Nolen [ 20/Jan/15 2:56 PM ]

fixed https://github.com/clojure/clojurescript/commit/62d898ae30eb58397628b45b3c0c95d3e899a274





[CLJS-983] Make ExceptionInfo printable Created: 20/Jan/15  Updated: 24/Jan/15  Resolved: 24/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-983-printable-ex-info-2.patch    

 Description   

Pretty simple enhancement — so we can see what's inside



 Comments   
Comment by David Nolen [ 23/Jan/15 6:21 PM ]

This patch needs a rebased to master.

Comment by Nikita Prokopov [ 24/Jan/15 2:56 AM ]

Things got a little messier after CLJS-985 and prototype manipulations

Comment by Nikita Prokopov [ 24/Jan/15 2:57 AM ]

Uploaded updated patch, rebased to master

Comment by David Nolen [ 24/Jan/15 10:29 AM ]

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





[CLJS-982] Var derefing should respect Clojure semantics Created: 18/Jan/15  Updated: 18/Jan/15  Resolved: 18/Jan/15

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   

In every compilation mode other than advanced possible to preserve via a thunk



 Comments   
Comment by David Nolen [ 18/Jan/15 10:00 PM ]

fixed https://github.com/clojure/clojurescript/commit/522fbdc04e5e35171e6818ce80b3db4811cc3284





[CLJS-981] Better benchmarking infrastructure Created: 17/Jan/15  Updated: 17/Jan/15

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 use ProcessBuilder to run the various benchmark scripts and control which benchmarks we test and which engines we run. Benchmarks should produce EDN data that can be written to a file, loaded into Incanter, etc.






[CLJS-980] ClojureScript REPL stacktraces overrun prompt in many cases Created: 17/Jan/15  Updated: 17/Jan/15  Resolved: 17/Jan/15

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

Type: Defect Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript 2687


Attachments: Text File CLJS-980.patch    

 Description   

ClojureScript repl stacktraces overrun prompt in many cases

If you do
(doc 'clojure.string/join)
The failure stacktrace hides the prompt, creating the illusion the REPL has hung.



 Comments   
Comment by Bruce Hauman [ 17/Jan/15 11:10 AM ]

Provided patch that fixes this.

Comment by David Nolen [ 17/Jan/15 11:21 AM ]

fixed https://github.com/clojure/clojurescript/commit/359ea4eec7aad5902abbaeee319df44d78183469





[CLJS-979] ClojureScript REPL needs error handling for the special functions Created: 16/Jan/15  Updated: 16/Jan/15  Resolved: 16/Jan/15

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

Type: Defect Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript 2688


Attachments: Text File CLJS-979.patch    

 Description   

Currently the the special functions are not run int a try catch block. This creates a brittle repl that will fail is someone trys to require a namespace that doesn't exist or execute any of the special functions with error causing arguments.

The following fail and terminate the repl loop.
(require 'ex) where ex doesn't exist
(in-ns '5) causes an infinite loop.



 Comments   
Comment by Bruce Hauman [ 16/Jan/15 4:32 PM ]

Here is a patch that fixes this. A general handler ensures that injected special-fns are handled as well.

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

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





[CLJS-978] Analysis caching doesn't account for constants table Created: 15/Jan/15  Updated: 16/Jan/15

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   

This means advanced optimizations cannot leverage analysis caching.



 Comments   
Comment by Thomas Heller [ 16/Jan/15 4:48 AM ]

Currently the constants use a generated id, we could instead use the constant itself as the key.

:test/keyword creates cljs.core.constant$keyword$123 where 123 is the sequential id which makes it unusable between compiles. We could instead turn it into cljs.core.constant$keyword$_COLON_test_SLASH_keyword (to reuse current munge). We would then just need to keep track of which namespace uses which constant and emit this as well in the analysis cache.

Might be missing something, will investigate a bit later.

Comment by David Nolen [ 16/Jan/15 5:59 AM ]

The constants table needs to work for all compile time constants - vectors, sets, maps, etc.





[CLJS-977] Subvec missing IKVReduce implementation Created: 15/Jan/15  Updated: 15/Jan/15

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-976] Node REPL breaks from uncaught exceptions Created: 14/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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

Type: Defect Priority: Major
Reporter: Joel Holdbrooks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-976.patch    

 Description   

Certain errors raised by Node will cause a ClojureScript Node REPL to fail.

Steps to reproduce:

cljs.user> (require '[cljs.nodejs :as node])
cljs.user> (def fs (node/require "fs"))
cljs.user> (.createReadStream fs "fail")
;; => #<[object Object]>
cljs.user> (+ 1 1)
;; => java.lang.IllegalArgumentException: Value out of range for char: -1

A vanilla Node REPL will also fail under the same condition and terminate the session.

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: ENOENT, open 'fail'

The CLJS REPL should probably be a bit more durable and, instead of terminating, catch and report the error.

A gist of the above example can be found here: https://gist.github.com/noprompt/3d0e5cbfca7883727fcf.



 Comments   
Comment by Mike Fikes [ 29/Jan/15 9:59 PM ]

Hey Joel, this appears to be rooted in the Node process itself dying.

I reproduced using your initial steps and confirmed that my Node process disappears. You can produce the same java.lang.IllegalArgumentException: Value out of range for char: -1 if you simply kill the node process out from underneath the REPL, or do something else that will cause it to give up the ghost (I ran into it having it evaluate (set (range 20000000)))

If you try to issue a subsequent command you will then get a java.net.SocketException: Broken pipe when the attempt is made to write the JavaScript out to the Node process on the (defunct) socket.

The IllegalArgumentException associated with the failure to handle -1 being returned from the socket read could be converted to an IOException, perhaps giving the user more of a clue:

diff --git a/src/clj/cljs/repl/node.clj b/src/clj/cljs/repl/node.clj
index 563d63a..9ea81db 100644
--- a/src/clj/cljs/repl/node.clj
+++ b/src/clj/cljs/repl/node.clj
@@ -16,7 +16,7 @@
             [clojure.data.json :as json])
   (:import java.net.Socket
            java.lang.StringBuilder
-           [java.io File BufferedReader BufferedWriter]
+           [java.io File BufferedReader BufferedWriter EOFException]
            [java.lang ProcessBuilder ProcessBuilder$Redirect]))
 
 (defn socket [host port]
@@ -39,6 +39,7 @@
   (let [sb (StringBuilder.)]
     (loop [sb sb c (.read in)]
       (cond
+       (= c -1) (throw (EOFException.))
        (= c 1) (let [ret (str sb)]
                  (print ret)
                  (recur (StringBuilder.) (.read in)))

But perhaps an even better solution would be to, in addition, add a try-catch in node-eval to handle all IOException s encountered when attempting to communicate with the Node process and to simply print a suitable meaningful error to the user stating such. (I suppose you can't necessarily conclude that the Node process is actually gone; it may be a timeout that leads to an IOException). But this would arguably be more robust than derailing trying to cast -1 to a char.

Comment by Mike Fikes [ 30/Jan/15 8:42 AM ]

The attached patch adds robustness by handling errors reading and writing to the underlying Node process and by additionally detecting if Node has died.

For Joel's example, it doesn't fail fast, but when you attempt a subsequent evaluation, it does provide diagnostics:

ClojureScript:cljs.user> (+ 1 1)
IOException communicating with Node process: Connection closed
Node process has exited with value: 1
nil

and with the example I encountered (in a fresh REPL):

ClojureScript:cljs.user> (set (range 20000000))
FATAL ERROR: CALL_AND_RETRY_2 Allocation failed - process out of memory
IOException communicating with Node process: Connection closed
Node process has exited with value: 134
nil

(This last example is on OS X. Note that 134 - 128 = 6, which is SIGABRT.)

Comment by David Nolen [ 30/Jan/15 3:01 PM ]

The correct solution is to prevent the Node.js process from ever dying from uncaught exceptions.

Fixed in master https://github.com/clojure/clojurescript/commit/2d81bde0a7e1fbcf1f4883cb144bf2b10cea393a

Comment by Mike Fikes [ 30/Jan/15 3:17 PM ]

Confirmed fixed for Joel's example, and for mine, the outcome is also still reasonable given that the out-of-memory condition is displayed:

ClojureScript:cljs.user> (set (range 20000000))
FATAL ERROR: CALL_AND_RETRY_2 Allocation failed - process out of memory
java.lang.IllegalArgumentException: Value out of range for char: -1
	at clojure.lang.RT.charCast(RT.java:962)
	at cljs.repl.node$read_response.invoke(node.clj:47)
	at cljs.repl.node$node_eval.invoke(node.clj:57)
	at cljs.repl.node.NodeEnv._evaluate(node.clj:164)
	at cljs.repl$evaluate_form.invoke(repl.clj:206)
	at cljs.repl$evaluate_form.invoke(repl.clj:168)
	at cljs.repl$eval_and_print.invoke(repl.clj:263)
	at cljs.repl$repl_STAR_.invoke(repl.clj:427)
	at user$eval3562.invoke(NO_SOURCE_FILE:3)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.eval(Compiler.java:6666)
	at clojure.core$eval.invoke(core.clj:2927)
	at clojure.main$eval_opt.invoke(main.clj:288)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
java.lang.IllegalArgumentException: Value out of range for char: -1
nil




[CLJS-975] preserve :reload & :reload-all in ns macro sugar Created: 13/Jan/15  Updated: 13/Jan/15

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   

:include-macros true and :refer-macros [...] are both sugar forms supported in :use and :require, they expand into :use-macros or :require-macros specs. We should make sure to preserve :reload and :reload-all when desugaring.



 Comments   
Comment by Thomas Heller [ 13/Jan/15 10:48 AM ]

Oops, didn't see there was an issue for this.

Sorry for the rant but please note my comment on:
https://github.com/clojure/clojurescript/commit/e408305344ef7b3658118377e530c78c2eb93cf3

I do not think REPL related "options" have anything to do in the parse function of the namespace. I outlined a different approach which I hope you'll consider. Of course all of this is motivated by shadow-build, so YMMV.

Comment by David Nolen [ 13/Jan/15 11:16 AM ]

While shadow-build is an interesting piece of technology it's not relevant for tickets like this. Please keep threads clear of non-issues.





[CLJS-974] Clean compile/build fails to resolve namespaces for cljs.test Created: 12/Jan/15  Updated: 13/Jan/15  Resolved: 13/Jan/15

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

Type: Defect Priority: Major
Reporter: Denis Johnson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: cljs, test
Environment:

Linux Ubuntu 14.04
Java 1.7.0_17 Java HotSpot(TM) 64-Bit Server VM
leiningen 2.5.1
Intellij 13.1.6 with latest cursive 0.1.43 plugin

:dependencies
[[org.clojure/clojure "1.6.0"]
[org.clojure/clojurescript "0.0-2665"]
[reagent "0.5.0-alpha"]
[alandipert/storage-atom "1.2.3"]
[com.andrewmcveigh/cljs-time "0.3.0"]]
:plugins [[lein-cljsbuild "1.0.3"]]


Attachments: File test_suite.cljs    

 Description   

Clean compile gives the following error for a valid namespace for runner/suite (see attachment).
Same namespace works fine using cemerick/clojurescript.test
NOTE 1: if compiled using "cljsbuild auto test" after giving failure, if you delete the test/test-suite.cljs the compile completes successfully, if then you revert the file it also recompiles successfully and tests run as expected.
NOTE 2: Happens all the time on my machine but other devs on Windows platform don't seem to get this issue. The only difference I can see is I use leiningen checkouts feature to point at our other local repos and these are project dependencies. I have tested with removing the checkouts folder with change in behaviour so I doubt it is related.

Deleting files generated by lein-cljsbuild.
Compiling ClojureScript.
Compiling "compiled/test.js" from ["src" "test"]...
Compiling "compiled/test.js" failed.
clojure.lang.ExceptionInfo: failed compiling file:test/test_suite.cljs
core.clj:4403 clojure.core/ex-info
compiler.clj:1039 cljs.compiler/compile-file
compiler.clj:1069 cljs.compiler/compile-root
closure.clj:358 cljs.closure/compile-dir
closure.clj:398 cljs.closure/eval3160[fn]
closure.clj:306 cljs.closure/eval3096[fn]
closure.clj:412 cljs.closure/eval3147[fn]
closure.clj:306 cljs.closure/eval3096[fn]
compiler.clj:44 cljsbuild.compiler.SourcePaths/fn
core.clj:2557 clojure.core/map[fn]
LazySeq.java:40 clojure.lang.LazySeq.sval
LazySeq.java:49 clojure.lang.LazySeq.seq
RT.java:484 clojure.lang.RT.seq
core.clj:133 clojure.core/seq
core.clj:624 clojure.core/apply
core.clj:2586 clojure.core/mapcat
RestFn.java:423 clojure.lang.RestFn.invoke
compiler.clj:44 cljsbuild.compiler/cljsbuild.compiler.SourcePaths
closure.clj:1018 cljs.closure/build
closure.clj:972 cljs.closure/build
compiler.clj:58 cljsbuild.compiler/compile-cljs[fn]
compiler.clj:57 cljsbuild.compiler/compile-cljs
compiler.clj:159 cljsbuild.compiler/run-compiler
form-init1706317005734214457.clj:1 user/eval3512[fn]
form-init1706317005734214457.clj:1 user/eval3512[fn]
LazySeq.java:40 clojure.lang.LazySeq.sval
LazySeq.java:49 clojure.lang.LazySeq.seq
RT.java:484 clojure.lang.RT.seq
core.clj:133 clojure.core/seq
core.clj:2855 clojure.core/dorun
core.clj:2871 clojure.core/doall
form-init1706317005734214457.clj:1 user/eval3512
Compiler.java:6703 clojure.lang.Compiler.eval
Compiler.java:6693 clojure.lang.Compiler.eval
Compiler.java:7130 clojure.lang.Compiler.load
Compiler.java:7086 clojure.lang.Compiler.loadFile
main.clj:274 clojure.main/load-script
main.clj:279 clojure.main/init-opt
main.clj:307 clojure.main/initialize
main.clj:342 clojure.main/null-opt
main.clj:420 clojure.main/main
RestFn.java:421 clojure.lang.RestFn.invoke
Var.java:383 clojure.lang.Var.invoke
AFn.java:156 clojure.lang.AFn.applyToHelper
Var.java:700 clojure.lang.Var.applyTo
main.java:37 clojure.main.main
Caused by: clojure.lang.ExceptionInfo: No such namespace: test.model.dimension-glossary-test at line 1 test/test_suite.cljs
core.clj:4403 clojure.core/ex-info
analyzer.clj:297 cljs.analyzer/error
analyzer.clj:294 cljs.analyzer/error
analyzer.clj:1095 cljs.analyzer/analyze-deps
analyzer.clj:1280 cljs.analyzer/eval1561[fn]
MultiFn.java:249 clojure.lang.MultiFn.invoke
analyzer.clj:1609 cljs.analyzer/analyze-seq
analyzer.clj:1696 cljs.analyzer/analyze[fn]
analyzer.clj:1689 cljs.analyzer/analyze
compiler.clj:948 cljs.compiler/compile-file*[fn]
compiler.clj:906 cljs.compiler/with-core-cljs
compiler.clj:927 cljs.compiler/compile-file*
compiler.clj:1033 cljs.compiler/compile-file



 Comments   
Comment by David Nolen [ 13/Jan/15 7:31 AM ]

Please report cljsbuild bugs with the cljsbuild project thanks.





[CLJS-973] cljs.test ignores some deftest names Created: 12/Jan/15  Updated: 15/Jan/15  Resolved: 15/Jan/15

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

Type: Defect Priority: Minor
Reporter: Denis Johnson Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: cljs, test
Environment:

Linux Ubuntu 14.04
Java 1.7.0_17 Java HotSpot(TM) 64-Bit Server VM
leiningen 2.5.1

:dependencies
[[org.clojure/clojure "1.6.0"]
[org.clojure/clojurescript "0.0-2665"]
[reagent "0.5.0-alpha"]
[alandipert/storage-atom "1.2.3"]
[com.andrewmcveigh/cljs-time "0.3.0"]]
:plugins [[lein-cljsbuild "1.0.3"]]



 Description   

examples:

  • ignores: (deftest test:change-step-contiguous-forward> ....)
  • works: (deftest test:change-current-step ...) ignores: (deftest test-change-current-step ...)


 Comments   
Comment by David Nolen [ 15/Jan/15 1:26 AM ]
(deftest test:change-current-step: ...)
(deftest test:change-current-step@ ...)

Removed these two from ticket description these are not valid Clojure symbols see http://clojure.org/reader

Comment by David Nolen [ 15/Jan/15 1:37 AM ]

I tested the remaining names could not reproduce. Please only reopen this ticket if a full minimal case is provided.





[CLJS-972] Node.js REPL eats errors in required ns when using require Created: 12/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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 [ 30/Jan/15 4:15 PM ]

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





[CLJS-971] at REPL :reload should work for require-macros special fn Created: 12/Jan/15  Updated: 13/Jan/15  Resolved: 13/Jan/15

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 [ 13/Jan/15 8:03 AM ]

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





[CLJS-970] assert - generate Error string when compiling Created: 10/Jan/15  Updated: 12/Jan/15

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

Type: Defect Priority: Trivial
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File cljs-generate-assert-message.diff    
Patch: Code

 Description   

Currently when using assert in CLJS the assert macro will generate the assert error message through CLJS. We can do that in CLJ which reduces the amount of code generated.

(let [bar 1]
  (assert (string? bar)))

Currently generates

if(typeof bar_11332 === 'string'){
} else {
throw (new Error([cljs.core.str("Assert failed: "),cljs.core.str(cljs.core.pr_str.call(null,cljs.core.list(new cljs.core.Symbol(null,"string?","string?",-1129175764,null),new cljs.core.Symbol(null,"bar","bar",254284943,null))))].join('')));
}

But could generate

if(typeof bar_23183 === 'string'){
} else {
throw (new Error("Assert failed: (string? bar)"));
}

Attached patch does that.



 Comments   
Comment by Thomas Heller [ 12/Jan/15 9:16 AM ]

Forgot to account for the fact that

(assert (string? something) "this might not be a string"))

New patch only generates the string of the form when compiling.





[CLJS-969] Node REPL fails with TypeError under Windows Created: 08/Jan/15  Updated: 27/Jan/15  Resolved: 27/Jan/15

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

Type: Defect Priority: Major
Reporter: Adrian Sampaleanu Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Windows 8.1, Node.js 0.10.35



 Description   

With latest source and following the steps outlined in The Node.js REPL of My Dreams, the following error is emitted when noderepljs is invoked:

$ ./script/noderepljs
To quit, type: :cljs/quit
ClojureScript Node.js REPL server listening on 58343
"Error evaluating:" (do (.require js/goog "cljs.core") (set! *print-fn* (.-print (js/require "util")))) :as "goog.require(\"cljs.core\");\r\n\r\ncljs.core._STAR_print_fn_STAR_ = require(\"
util\").print;\r\n"
TypeError: Cannot read property 'nameToPath' of undefined
    at Object.goog.require (repl:2:49)
    at repl:1:6
    at Socket.<anonymous> ([stdin]:30:80)
    at Socket.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:765:14)
    at Socket.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:427:10)
    at emitReadable (_stream_readable.js:423:5)
    at readableAddChunk (_stream_readable.js:166:9)
    at Socket.Readable.push (_stream_readable.js:128:10)
ClojureScript:cljs.user>

Though the correct prompt appears, the REPL is non functional.



 Comments   
Comment by David Nolen [ 08/Jan/15 7:19 AM ]

Thanks for the report. Sadly there are no cycles to work on Windows issues ourselves. Patches from Windows users are alway welcome.

Comment by David Nolen [ 27/Jan/15 12:23 PM ]

Should be fixed in master





[CLJS-968] Metadata on function literal inside of a let produces invalid Javascript Created: 07/Jan/15  Updated: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Bobby Eickhoff Assignee: Nicola Mometto
Resolution: Unresolved Votes: 1
Labels: bug
Environment:

Originally found with [org.clojure/clojurescript "0.0-2496"]
Still reproducible with the latest cljsc (b5e9a5116259fc9f201bee4b9c6564f35306f9a5)



 Description   

Here is a minimal test case that produces the invalid Javascript:

(defn f []
  (let [a 0]
    ^{"meta" "data"}
    (fn [] true)))

The compiled Javascript includes the invalid token sequence "return return". (Per Chrome: Uncaught SyntaxError: Unexpected token return)

The problem does not occur if the metadata applies to a map literal instead of a function literal.
The problem only occurs when the function and metadata are inside of a let.



 Comments   
Comment by Bobby Eickhoff [ 07/Jan/15 9:45 PM ]

I forgot to try with-meta. Using with-meta does not produce this syntax error, so it's only a problem with the reader macro for metadata.

Comment by David Nolen [ 08/Jan/15 7:41 AM ]

Any quick thoughts about this one Nicola? Quite possibly a compiler issue on the CLJS side.

Comment by Nicola Mometto [ 08/Jan/15 8:07 AM ]

David, I understand why this happens but I don't know enough about how cljs's js emission to propose a fix.
The issue is that with this commit: https://github.com/clojure/clojurescript/commit/d54defd32d6c5ffcf6b0698072184fe8ccecc93a the following scenario is possible:

{:op :meta
 :env {:context :return}
 :expr {:op :fn
        :env {:context :expr}
        :methods [{:op :fn-method 
                   :env {:context :return} ..}]
        ..}
 ..}

i.e. analyze-wrap-meta changes the context of the :fn node to :expr but keeps the context of the :fn-methods to :return.

This causes both
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L575-L576
and
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L488 (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L233)

to be true and emit a "return".





[CLJS-967] "java.net.ConnectException: Connection refused" when running node repl Created: 07/Jan/15  Updated: 08/Jan/15

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

Type: Defect Priority: Major
Reporter: Brian Muhia Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, errormsgs
Environment:

Processor: 1.33GHz Intel Atom, Ubuntu 12.04, OpenJDK 1.7, Clojure 1.6.0, ClojureScript 0.0-2665, nodejs v0.10.26


Attachments: Text File backtrace.txt    

 Description   

I used trampoline like so:

rlwrap lein trampoline run -m clojure.main scripts/repl.clj

run the script repl.clj with contents:

(require
  '[cljs.repl :as repl]
  '[cljs.repl.node :as node])

(repl/repl* (node/repl-env)
  {:output-dir "out"
   :optimizations :none
   :cache-analysis true                
   :source-map true})

Instead of getting a repl, I got the exception: Exception in thread "main" java.net.ConnectException: Connection refused, compiling/home/brian/projects/essence-cljs-redux/scripts/repl.clj:3:30)

The full stack trace is attached in backtrace.txt.



 Comments   
Comment by David Nolen [ 08/Jan/15 7:21 AM ]

The issue is that we use a 300ms timeout to connect to the Node.js process, we need to instead redirect the process out and wait for the Node.js REPL server start string.





[CLJS-966] :foreign-libs should not go through Closure Compiler Created: 07/Jan/15  Updated: 23/Jan/15  Resolved: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently the files listed in foreign libs will go through Closure Compiler when using :advanced, they should instead be in the preamble.

https://github.com/thheller/cljs-foreign-bug

lein cljsbuild once release

will create a foreign.min.js [1] which includes the advanced optimized source of js/sample.js (grep for I WONT SURVIVE). In this case the source actually keeps working since the source is quite simple. But we don't even need to include the js/sample.js in the HTML file since the source won't access Foreign.sayHi since it got renamed and uses the optimized code.

[1] https://github.com/thheller/cljs-foreign-bug/blob/master/foreign.min.js



 Comments   
Comment by David Nolen [ 23/Jan/15 6:21 PM ]

fixed in master





[CLJS-965] Make :foreign-libs more useful Created: 07/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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


 Description   

:foreign-libs almost does what we need to make working with JavaScript dependencies painless. They need to additionally specify :requires. :foreign-libs should automatically be preambled in advanced compilation, dependency order can be solved since they provide :requires. Coupled with the deps.clj support using JS deps from JARs would be as simple and as painless as CLJS JARs.

The following conditions must be met:

A) do not break existing usage of deps.clj - means we need to check :foreign-lib true in deps.clj
B) Support optimizations :none, means we must emit goog.addDependency in deps.js
C) Support optimizations :advanced, means we should sort in dependency order and place foreign between :preamble and Closure advanced build
D) users should be able to specify a JS file to use under :none to support debugging



 Comments   
Comment by Yehonathan Sharvit [ 08/Jan/15 1:46 PM ]

Is it currently possible to include a js file inside a cljs lib, without requiring from the library clients to download manually the js file?

Comment by David Nolen [ 08/Jan/15 1:51 PM ]

It is not possible and such a feature is out of scope.

Comment by Francis Avila [ 08/Jan/15 2:45 PM ]

Isn't this done all the time, even with foreign-libs? Or do I misunderstand the question? Yehonathan, what specific scenario are you thinking of? Serving a foreign-lib from a cdn?

Comment by David Nolen [ 30/Jan/15 3:13 PM ]

fixed in 0.0-2719





[CLJS-964] Redefining exists? does not emit a warning like redefining array? does. Created: 06/Jan/15  Updated: 07/Jan/15

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

Type: Defect Priority: Minor
Reporter: Uday Verma Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

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



 Description   

ClojureScript:cljs.user> (def array? 2)
WARNING: array? already refers to: /array? being replaced by: cljs.user/array? at line 1 <cljs repl>
2
ClojureScript:cljs.user> (def exists? 2)
2
ClojureScript:cljs.user>



 Comments   
Comment by Mike Fikes [ 06/Jan/15 11:41 PM ]

Additionally, if you define exists? as as a function in your namespace, as in

(defn exists? [] 1)

You need to call it indirectly via its var:

ClojureScript:cljs.user> (@#'exists?)
1

If you instead call it directly, you will get this (this is in the Node.js REPL; similar occurs in JavaScriptCore environment):

ClojureScript:cljs.user> (exists?)
clojure.lang.ExceptionInfo: Wrong number of args (2) passed to: core/exists? at line 1 <cljs repl> {:tag :cljs/analysis-error, :file "<cljs repl>", :line 1, :column 1}
	at clojure.core$ex_info.invoke(core.clj:4403)
	at cljs.analyzer$error.invoke(analyzer.clj:297)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1562)
	at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1605)
	at cljs.analyzer$analyze$fn__1673.invoke(analyzer.clj:1696)
	at cljs.analyzer$analyze.invoke(analyzer.clj:1689)
	at cljs.analyzer$analyze.invoke(analyzer.clj:1684)
	at cljs.repl$evaluate_form.invoke(repl.clj:100)
	at cljs.repl$evaluate_form.invoke(repl.clj:96)
	at cljs.repl$eval_and_print.invoke(repl.clj:188)
	at cljs.repl$repl_STAR_.invoke(repl.clj:322)
	at user$eval3528.invoke(NO_SOURCE_FILE:3)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.eval(Compiler.java:6666)
	at clojure.core$eval.invoke(core.clj:2927)
	at clojure.main$eval_opt.invoke(main.clj:288)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.ArityException: Wrong number of args (2) passed to: core/exists?
	at clojure.lang.AFn.throwArity(AFn.java:429)
	at clojure.lang.AFn.invoke(AFn.java:36)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.AFunction$1.doInvoke(AFunction.java:29)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1568)
	... 21 more

None of this occurs for the other predicates defined near exists? in cljs.core, like array?, true?, etc.

Comment by Thomas Heller [ 07/Jan/15 4:58 AM ]

I suspect that is due to exists? only being a macro with no function in cljs.core. array? and other predicates exist as an inlining macro and a function.

(defn ^boolean array? [x]
  (cljs.core/array? x))

Should just be a matter of creating a similar function for exists?.

Comment by Thomas Heller [ 07/Jan/15 5:00 AM ]

Oops no. Won't be that simple.

;; TODO: x must be a symbol, not an arbitrary expression
(defmacro exists? [x]
  (bool-expr
    (core/list 'js* "typeof ~{} !== 'undefined'"
      (vary-meta x assoc :cljs.analyzer/no-resolve true))))




[CLJS-963] goog dep.js is not recomputed when sources are recompiled Created: 06/Jan/15  Updated: 06/Jan/15  Resolved: 06/Jan/15

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   

Encountered this in mies when switching from Node.js REPL to Browser REPL



 Comments   
Comment by David Nolen [ 06/Jan/15 7:51 AM ]

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





[CLJS-962] Inconsistent hashing of empty collections Created: 05/Jan/15  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

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

clojurescript 0.0-2496"
clojure 1.6.0


Attachments: Text File 0001-CLJS-962-fix-inconsistent-hashing-of-empty-collectio.patch    

 Description   

I get different results when hashing a literal empty vector than when hashing one from read-string, while all the other collections produce the same hash code:

``` clojure
homepage.core> [(hash [2]) (hash (cljs.reader/read-string "[2]"))]
[-1917711765 -1917711765]
homepage.core> [(hash [0]) (hash (cljs.reader/read-string "[0]"))]
[965192274 965192274]
homepage.core> [(hash []) (hash (cljs.reader/read-string "[]"))]
[0 -2017569654] ;; <--- this one looks suspicious.
homepage.core> [(hash {}) (hash (cljs.reader/read-string "{}"))]
[-15128758 -15128758]
homepage.core> (hash #{}) (hash (cljs.reader/read-string "#{}"))
[0 0]
homepage.core> clojurescript-version
"0.0-2496"
```



 Comments   
Comment by David Nolen [ 05/Jan/15 11:24 AM ]

Seems to be a bug that effects all empty collections, the hash values should match Clojure's.

Comment by Michał Marczyk [ 05/Jan/15 6:34 PM ]

Patch with test.

Comment by David Nolen [ 06/Jan/15 7:57 AM ]

Michal, wow thanks for the quick fix!

Comment by David Nolen [ 06/Jan/15 8:00 AM ]

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

Comment by Michał Marczyk [ 06/Jan/15 2:55 PM ]

Thanks for the quick merge!





[CLJS-961] REPL process should always be killable Created: 04/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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


 Description   

In the browser REPL case if user never connects browser cannot kill REPL with Ctrl-C or Ctrl-D.



 Comments   
Comment by David Nolen [ 30/Jan/15 3:12 PM ]

Duplicate CLJS-989





[CLJS-960] On carriage return REPLs should always show new REPL prompt Created: 04/Jan/15  Updated: 04/Jan/15

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-959] Protocol methods specifying an empty docstring fail to compile Created: 03/Jan/15  Updated: 05/Jan/15  Resolved: 05/Jan/15

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

Type: Defect Priority: Minor
Reporter: Julien Eluard Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File protocol-empty-docstring.diff    

 Description   

Starting ClojureScript 2411 protocols with method specifying an empty docstring fail to compile. Non-empty docstrings are fine.

Following code compiles fine:

(defprotocol Protocol
  (method [_]))
(defprotocol Protocol2
  (method [_] "some doc"))

While following code throws an exception:

(defprotocol Protocol
  (method [_] ""))
clojure.lang.ExceptionInfo: failed compiling file:src/mies_hello_world/core.cljs {:file #<File src/mies_hello_world/core.cljs>}
	at clojure.core$ex_info.invoke(core.clj:4403)
	at cljs.compiler$compile_file.invoke(compiler.clj:999)
	at cljs.compiler$compile_root.invoke(compiler.clj:1029)
	at cljs.closure$compile_dir.invoke(closure.clj:358)
	at cljs.closure$fn__2534.invoke(closure.clj:398)
	at cljs.closure$fn__2484$G__2479__2491.invoke(closure.clj:306)
	at cljs.closure$fn__2527.invoke(closure.clj:412)
	at cljs.closure$fn__2484$G__2479__2491.invoke(closure.clj:306)
	at cljsbuild.compiler.SourcePaths$fn__134.invoke(compiler.clj:67)
	at clojure.core$map$fn__4245.invoke(core.clj:2557)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$apply.invoke(core.clj:624)
	at clojure.core$mapcat.doInvoke(core.clj:2586)
	at clojure.lang.RestFn.invoke(RestFn.java:423)
	at cljsbuild.compiler.SourcePaths._compile(compiler.clj:67)
	at cljs.closure$build.invoke(closure.clj:1018)
	at cljs.closure$build.invoke(closure.clj:972)
	at cljsbuild.compiler$compile_cljs$fn__145.invoke(compiler.clj:81)
	at cljsbuild.compiler$compile_cljs.invoke(compiler.clj:80)
	at cljsbuild.compiler$run_compiler.invoke(compiler.clj:179)
	at user$eval277$iter__295__299$fn__300$fn__312.invoke(form-init1238899578077056082.clj:1)
	at user$eval277$iter__295__299$fn__300.invoke(form-init1238899578077056082.clj:1)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$dorun.invoke(core.clj:2855)
	at clojure.core$doall.invoke(core.clj:2871)
	at user$eval277.invoke(form-init6397079933373263622.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.eval(Compiler.java:6693)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.ExceptionInfo: Invalid protocol, Protocol defines method method with arity 0 at line 8 src/mies_hello_world/core.cljs {:tag :cljs/analysis-error, :file "src/mies_hello_world/core.cljs", :line 8, :column 1}
	at clojure.core$ex_info.invoke(core.clj:4403)
	at cljs.analyzer$error.invoke(analyzer.clj:297)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1520)
	at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1563)
	at cljs.analyzer$analyze$fn__1360.invoke(analyzer.clj:1654)
	at cljs.analyzer$analyze.invoke(analyzer.clj:1647)
	at cljs.compiler$compile_file_STAR_$fn__2213.invoke(compiler.clj:909)
	at cljs.compiler$with_core_cljs.invoke(compiler.clj:867)
	at cljs.compiler$compile_file_STAR_.invoke(compiler.clj:888)
	at cljs.compiler$compile_file.invoke(compiler.clj:993)
	... 44 more
Caused by: java.lang.Exception: Invalid protocol, Protocol defines method method with arity 0
	at cljs.core$defprotocol$fn__3323.invoke(core.clj:1017)
	at cljs.core$defprotocol.doInvoke(core.clj:1015)
	at clojure.lang.RestFn.applyTo(RestFn.java:146)
	at clojure.lang.AFunction$1.doInvoke(AFunction.java:29)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1526)
	... 51 more

I figured it was worth reporting giving the error message is a little unexpected.



 Comments   
Comment by David Nolen [ 05/Jan/15 11:44 AM ]

fixed https://github.com/clojure/clojurescript/commit/7561c37231100fbdbe620b5dda7368e7bd2da632





[CLJS-958] Node.js REPL: Upon error, last successfully item printed Created: 03/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None
Environment:

0.0-2657
Node.js REPL set up as per http://swannodette.github.io/2014/12/29/nodejs-of-my-dreams/

uname -a
Linux ubuntu 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15 03:51:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

java -version
java version "1.7.0_65"
OpenJDK Runtime Environment (IcedTea 2.5.3) (7u71-2.5.3-0ubuntu0.14.04.1)
OpenJDK 64-Bit Server VM (build 24.65-b04, mixed mode)

node --version
v0.10.25



 Description   

If at the REPL, you evaluate a form that results in an error, the last successfully printed item prints again.

Here is an example:

ClojureScript:cljs.user> (time (reduce + (range 1000000)))
"Elapsed time: 427 msecs"
499999500000
ClojureScript:cljs.user> (first (js/Date.))
Error: Sat Jan 03 2015 19:48:17 GMT-0500 (EST) is not ISeqable
    at seq (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:672:20)
    at first (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:681:16)
    at repl:1:81
    at repl:9:3
    at Socket.<anonymous> ([stdin]:27:80)
    at Socket.EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:746:14)
    at Socket.EventEmitter.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:408:10)
    at emitReadable (_stream_readable.js:404:5)
499999500000

Note, specifically the `499999500000` re-printed after the error.



 Comments   
Comment by David Nolen [ 03/Jan/15 7:28 PM ]

The problem is we're allowing Node.js to print the error instead of letting the REPL infrastructure handle it.

Comment by David Nolen [ 03/Jan/15 7:48 PM ]

fixed https://github.com/clojure/clojurescript/commit/9072b4bfdaf5cf9c66c4770c545f066fbec6256e

Comment by Mike Fikes [ 03/Jan/15 7:54 PM ]

Confirmed fixed in my environment with https://github.com/clojure/clojurescript/commit/9072b4bfdaf5cf9c66c4770c545f066fbec6256e

Comment by David Nolen [ 03/Jan/15 8:04 PM ]

Thanks for the confirm.





[CLJS-957] Parallel compilation Created: 03/Jan/15  Updated: 06/Jan/15

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


 Description   

For non-interacting branches of the dependency graph we could compile in parallel.



 Comments   
Comment by Thomas Heller [ 03/Jan/15 3:06 PM ]

Just a heads-up: I attempted this in shadow-build.

The problem with this is macros, specifically their require. Granted you said non-interacting but suppose two namespaces require cljs.core.async.macros, they might trigger the clojure.core/require at the same time (parallel after all).

This resulted in a very weird exception: https://www.refheap.com/91731

I'm not sure how thread-safe clojure.core/require is supposed to be but it was as close as I was able to get tracking this weird error down. Removing reducers fixed everything. FWIW the gain was minimal to begin with.

Comment by David Nolen [ 03/Jan/15 3:09 PM ]

Making this really work I think largely depends on how much time you are willing to spend on the problem. Fork/join work stealing seems ideal for this not sure if you tried that. RE: macros, seems like you could use information collected from ns analysis to serialize calls to require.

Comment by Thomas Heller [ 03/Jan/15 3:28 PM ]

Lets throw some reducers at it was about as much thought I put into it.

Just beware of macros when you attempt to do something is all I wanted to say. Treating analysis seperate from cljs.compiler/emit might be a good idea though.

Comment by Dusan Maliarik [ 06/Jan/15 7:20 AM ]

I always thought parallel compilation belongs to the build tool (process), and should be parallelized per source file. Would there be any gain in using pmap in here ? It's used by lein-cljsbuild I suppose. Or parallelize this part?

Comment by David Nolen [ 06/Jan/15 7:54 AM ]

pmap won't work because of dependencies. A work queue approach is probably going to be most fruitful. Let's keep this thread clean unless you are actually going to work on this ticket and have a clear picture how to do it. Thanks.





[CLJS-956] No *print-fn* fn set for evaluation environment new REPL Created: 03/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

0.0-2644
Node.js REPL
Ubuntu



 Description   

Some evaluations print, like

ClojureScript:cljs.user> (first [1 2 3])
1

But others result in an error regarding print-fn, and interestingly re-print the latest successful print

ClojureScript:cljs.user> (require '[clojure.string :as string])

ClojureScript:cljs.user> (string/join ", " [1 2 3])
"1, 2, 3"
ClojureScript:cljs.user> (time (reduce + (range 1000000)))
Error: No *print-fn* fn set for evaluation environment
    at _STAR_print_fn_STAR_ (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:26:12)
    at string_print (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8009:4)
    at pr_with_opts (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8139:4)
    at cljs.core.prn.prn__delegate (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8191:4)
    at cljs.core.prn.prn (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8190:6)
    at repl:3:15
    at repl:6:3
    at repl:14:3
    at Socket.<anonymous> ([stdin]:27:80)
    at Socket.EventEmitter.emit (events.js:95:17)
"1, 2, 3"


 Comments   
Comment by David Nolen [ 03/Jan/15 10:56 AM ]

This is actually a dupe of http://dev.clojure.org/jira/browse/CLJS-954.

Comment by Mike Fikes [ 03/Jan/15 1:48 PM ]

Confirmed fixed in 0.0-2655.

Comment by David Nolen [ 03/Jan/15 1:53 PM ]

Thanks for the confirmation.





[CLJS-955] Move incorrect *analyze-deps* check Created: 03/Jan/15  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File move-incorrect-check.diff    
Patch: Code

 Description   

A new analyze-deps check was added recently [1] which is at the wrong location. When running with analzye-deps false (which shadow-build always does) it will also disable all macro loading. The attached patch moves this check to the "correct" position.

[1] https://github.com/clojure/clojurescript/commit/08805fd519a46ca80aa95b9a50af8c8293df0d3f



 Comments   
Comment by Thomas Heller [ 03/Jan/15 6:20 AM ]

Wait ... this new behavior completely disables all warnings for shadow-build since it runs with analyze-deps false.

I don't think this flag should affect the warnings at all. Either a new flag should be introduced to suppress the warnings or the warning code should be moved to a analyzer pass which can be toggled on demand. Moving more side effects out of parsing is probably a good idea anyways (see CLJS-948).

Willing to provide a patch for either case.

Comment by David Nolen [ 03/Jan/15 10:56 AM ]

I need wrap up some things around REPL support before this can be addressed. Happy to look at it when things have settled down.

Comment by Thomas Heller [ 03/Jan/15 11:15 AM ]

Ok, I'll keep an eye of HEAD and provide a new patch if required.

The issue is pretty straightforward though. If cljs.analyzer/parse-ns should not load macros it should bind cljs.analyzer/load-macros to false, currently analzyze-deps sort of hijacks that flag because of (and analyze-deps load-macros).

Comment by David Nolen [ 03/Jan/15 3:24 PM ]

I don't see a problem for your use case other than the API defaults have changed. Why can't you just set analyze-deps to true and load-macros to false?

Comment by Thomas Heller [ 03/Jan/15 3:31 PM ]

because (and true false) equals false.

Comment by David Nolen [ 03/Jan/15 3:36 PM ]

Sorry not understanding your point. You say you want to analyze deps but you don't want to analyze macros. Then false is the right thing in your case. Not wanting to analyze deps but wanting to load macros doesn't make sense to me.

Comment by Thomas Heller [ 03/Jan/15 3:53 PM ]

I use a completely different approach to compiling in dependency order than CLJS does. Basically I do one discovery pass (extracts goog.provide/require from JS files, parse NS of CLJS file) then sort everything and compile top-down (dependency order). analyze with analyze-deps is recursive and compiles bottom-up. Since we were talking about parallel compile, bottom-up doesn't work parallel.

Also since I control the loop that does the file compilation I can compile in memory without touching files (analyze-deps ALWAYS uses anaylze-file).

Given that I can obviously run with analyze-deps on the second pass through since I compile in dep order the deps are already compiled. I just don't want it to ever enter analyze-deps because it doesn't need to.

Also your change didn't change API defaults it broke the API.

load-macros was meant to to be able to skip the require of macros (since I don't need them on the first pass)
analyze-deps used to control if dependencies should be compiled, now it also controls warnings?

Comment by David Nolen [ 03/Jan/15 5:15 PM ]

Well there's not a true API broken here since none of this stuff is officially anything we support.

analyze-deps never controlled whether dependencies should be compiled, it only performs analysis in dependency order so that warning & optimization information can be properly propagated. It falls out of not necessarily compiling files in dependency order. And I believe the current bottom-up behavior still has utility for analysis tools that aren't involved in building.

Still I see the value of keeping these two bits separate in your case now.

Comment by Thomas Heller [ 03/Jan/15 5:26 PM ]

Sorry mixed up compile with analyze. Of cource analyze-file doesn't compile, still it touches files and does the whole caching bits.

Still analyze-deps didn't use to toggle the warnings, but CLJS-948 will probably address all this anyways.

Side Note: I used the two pass approach in shadow-build since I wanted to discover closure-compliant JS files that aren't mentioned in deps.js. I have a few plain JS dependencies in my app which I converted to be closure-aware when I rewrote my project from JS->CLJS. cljs.closure (or lein-cljsbuild) still don't recognize these unless you configure it very explicitly. I think there is an open issue for this as well.

Comment by David Nolen [ 03/Jan/15 5:37 PM ]

I've committed the following to master https://github.com/clojure/clojurescript/commit/7b683ed43c9316d8163c2f764cc6e9b2710c3f46

Hopefully this clears up how parse-ns is actually used by the compiler, which might have been
a source of confusion. I split apart the two flags, however cljs.analyzer/parse-ns has defaults which differ
from the root bindings of *analyze-deps* and *load-macros* which must be
explicitly over-ridden. It's intention was always as a helper for build tools not for running any actual
analysis or loading any macros.

If this is satisfactory then I'll close this ticket.

Comment by David Nolen [ 09/Jan/15 7:12 AM ]

Fixed in master

Comment by Thomas Heller [ 09/Jan/15 10:27 AM ]

CLJS-948 sort of reintroduces the problem.

It is because we combine *analyze-deps* to mean analyze deps and warn about deps. I don't want shadow-build to analyze deps but I want the warnings. Would you be ok with introducing a *check-deps* or should I find another way to address this in shadow-build?

We should resolve CLJS-948 first, I'll come back to this once we have.





[CLJS-954] require needs to follow Clojure semantics more closely, support :reload, :reload-all Created: 02/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/15

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   

Currently :reload-all is implicit, should be explicit.



 Comments   
Comment by David Nolen [ 02/Jan/15 8:33 PM ]

We need :reload-all and :reload support in the ns form. goog.require only supports a single arity, so we can trivially monkey-patch a two arity function. We need to discard this information in any situation other than :none, otherwise will get in the way of static dependency loading. We can and should follow the logic that is already present in Clojure here around loaded-libs.

Comment by David Nolen [ 02/Jan/15 8:51 PM ]

Plan, we can add a loaded-libs for use by REPLs that monkey-patch the behavior of goog.require. For :reload case we can just emit goog.require("foo.core", true); for :reload-all we can save loaded-libs reload everything and merge the new loaded-libs with the old value same as clojure.core/load-all.

Comment by David Nolen [ 03/Jan/15 1:34 PM ]

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





[CLJS-953] require REPL special fn can only take one argument Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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   

Not variadic like Clojure



 Comments   
Comment by David Nolen [ 02/Jan/15 4:46 PM ]

fixed https://github.com/clojure/clojurescript/commit/02524d15db5e25dff4c1934ae4b28a2010ea2fb9





[CLJS-952] Bad type hinting on bit-test Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

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

Attachments: Text File cljs-952.patch    
Patch: Code

 Description   

bit-test returns a boolean, but is incorrectly type-hinted as returning a number.



 Comments   
Comment by David Nolen [ 02/Jan/15 4:47 PM ]

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





[CLJS-951] goog.require emitted multiple times under Node.js REPL Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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 causes a namespace to be required multiple times.



 Comments   
Comment by David Nolen [ 02/Jan/15 3:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/251b76be8d7c072e272ca8d90edec64d6d18d5b6





[CLJS-950] In 0.0-2629, Can not run compiled source with node targert Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Defect Priority: Major
Reporter: tunde ashafa Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

// Compiled by ClojureScript 0.0-2629 {}
#!/usr/bin/env node

Script hashbang on the second line seems to be an issue when running script with node via command line.



 Comments   
Comment by David Nolen [ 02/Jan/15 11:24 AM ]

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

Comment by tunde ashafa [ 02/Jan/15 11:53 AM ]

Thank you





[CLJS-949] In 0.0-2629, Can not run compiled source with node targert Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Defect Priority: Major
Reporter: tunde ashafa Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

// Compiled by ClojureScript 0.0-2629 {} |;; This buffer is for notes you don't want to save, and for Lisp evaluation.
#!/usr/bin/env node



 Comments   
Comment by tunde ashafa [ 02/Jan/15 10:51 AM ]

Please delete this issue. Hit a shortcut to create this ticket prematurely. Updated ticket is #CLJS-950





[CLJS-948] Simplify macro usage Created: 02/Jan/15  Updated: 09/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File cljs-948.diff     File cljs-948-no-passes.diff    

 Description   

The usage of macros from CLJS is very explicit and users of any given namespace must remember whether it comes with macros and which vars are macros. This leads to rather verbose :require statements which are confusing and lead to unexpected results if incomplete (eg. missing :refer-macros).

(ns my-ns
  (:require [my-app.other-ns :as other :refer (something) :refer-macros (something) :include-macros true]))

(something) ;; macro (when :refer-macros, otherwise no macro)
(other/thing) ;; maybe macro (only when :include-macros, otherwise no macro)

I think the user should not need to know whether something is a macro or not, they should just be able to use it.

I implemented a proof-of-concept that

(ns my-ns
  (:require [my-app.other-ns :refer (something)])

will work just find and do the correct thing with regards to macros (assuming the namespace has a corresponding clj namespace, :require-macros is unaffected). No changes to any code are necessary as the implementation uses ana/passes, it is also fully backwards compatible.

Implementation points which should be discussed

  • The CLJS namespace with macros currently has to opt-in through a bit of metadata (eg. (ns my-ns-with-macros {:load-macros true})), that might not be necessary and maybe should default to true.
  • The implementation assumes compilation happens in dependency order. shadow-build always does this, I'm not too sure about CLJS though. Given ana/analyze-deps equals true that is guaranteed but I'm not sure that is always the case.

If there is any interest I can provide a patch, until then refer to the proof-of-concept [1].

[1] https://github.com/thheller/shadow-build/blob/f37cfa598f1e90dd66e333d1e45580ea25650025/src/clj/shadow/cljs/passes.clj#L30-L82



 Comments   
Comment by David Nolen [ 02/Jan/15 11:25 AM ]

This is interesting. Will think about it.

Comment by Thomas Heller [ 02/Jan/15 1:15 PM ]

Is it ok if I leave implementation notes here?

Things that would need addressing should this be implemented:

  • cljs.analyzer/check-uses runs when parsing the ns form, since the macro passes run AFTER the parsing finished the check-uses will warn incorrectly if a :refer is not defined in CLJS but is a macro. Solution: check-uses should probably just become a pass itself which runs after the macro ones. Moving a side-effect out of the parse function is probably well worth it.
  • cljs.analyzer/load-macros would obsolete and the entire functionality it was toggling can be moved to the load-macros pass.
Comment by David Nolen [ 03/Jan/15 3:33 PM ]

I looked a bit at your code. It looks interesting. Definitely not going to do the namespace metadata bit. It seems to me this should work only if the required namespace loaded macros from a Clojure namespace of the same name.

Comment by Thomas Heller [ 03/Jan/15 3:36 PM ]

Yeah, probably. Also played with the idea of just looking for (io/resource (str (ns->path) ".clj")). If that is nil the namespace doesn't have macros, if its not null we require it.

But I like your approach of just using (ns my-ns (:require-macros [my-ns])) more.

Comment by David Nolen [ 03/Jan/15 3:40 PM ]

OK, let's try a patch for this. This is an interesting simplification that unifies Clojure & ClojureScript macro usage.

Comment by Thomas Heller [ 03/Jan/15 4:10 PM ]

Yay! Coming right up.

One issue though: I thought it would be a good idea to put the passes into a separate file (eg. cljs.analyzer.passes), that is currently not possible due to cljs.analyzer/analyze (it controls the passes defaults) [1]. cljs.analyzer cannot depend on cljs.analyzer.passes since it most likely depends on cljs.analyzer (circular dependency). Technically the passes currently only require the analyzer because of ::ana/namespaces but faking out a circular dependency is not a good idea.

If we remove the (or passes [infer-type]) and instead force opts to contain :passes (or bind passes beforehand) we should be fine.

Although I'm perfectly fine with putting the new passes into cljs.analyzer, just thought it would be cleaner not to.

Also, how should I address the invalid refer warning issue? Since the check-uses runs before the pass it can never find the macro. I was going to make check-uses a pass in itself if thats alright with you.

[1] https://github.com/clojure/clojurescript/blob/2d1e2167f5ab8bd76ac5c8bafd198990cc88d34a/src/clj/cljs/analyzer.clj#L1711

Comment by Thomas Heller [ 03/Jan/15 5:01 PM ]

I cleaned up the reference in shadow-build [1]. This is how a cljs.analyer.passes could look, the default passes would then be

[cljs.analyzer.passes/load-macros
 cljs.analyzer.passes/infer-macro-require
 cljs.analyzer.passes/infer-macro-use
 cljs.analyzer.passes/check-uses
 cljs.analyzer/infer-type]

This would basically make ana/load-macros obsolete since this can now be controlled via passes. Also addresses CLJS-955.

[1] https://github.com/thheller/shadow-build/blob/220d3bb0bef2cdb5696487b639ca5aaa169c56f2/src/clj/shadow/cljs/passes.clj

Comment by David Nolen [ 03/Jan/15 5:40 PM ]

This looks OK to me at the moment.

Comment by Thomas Heller [ 03/Jan/15 5:40 PM ]

Should stop refering to specific hashes, please see [1].

There is a slight issue with cljs.core itself. There is a bit of code that makes cljs.core macros special. I'm not quite sure why this is, since it would not be required if it did.

(ns cljs.core
  (:require-macros [cljs.core]))

Getting an error [2] on the recently changed def to (defonce print-fn). Haven't exactly figured out why this is yet.

[1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj
[2] https://www.refheap.com/61bc05e4b4a74a3b573797721

Comment by Thomas Heller [ 03/Jan/15 5:44 PM ]

Wait, are other namespaces allowed to use their own macros unqualified?

(ns my-ns
  (:require-macros [my-ns]))

(this-is-a-macro)
;; vs
(my-ns/this-is-a-macro)

I don't think so? The recent defonce seems to be an exception?

Comment by David Nolen [ 03/Jan/15 5:50 PM ]

yes cljs.core and core macros are an exception

Comment by Thomas Heller [ 03/Jan/15 5:54 PM ]

Yeah, but cljs.core calls all macros directly.

(defn ^boolean identical?
  "Tests if 2 arguments are the same object"
  [x y]
  (cljs.core/identical? x y))

The recently changed defonce vars are the exception here. Although even if I change them to cljs.core/defonce I still can't explain the error.

Comment by David Nolen [ 03/Jan/15 5:56 PM ]

The inlining macros are an exception, it's done that way to break circularity which would otherwise stack overflow the compiler.

Comment by Thomas Heller [ 03/Jan/15 7:09 PM ]

Alright, resolved that macro issue.

I updated [1] so the analyzer passes are initialized correctly and outside cljs.analyzer. The patch would also change the cljs.analyzer/parse 'def since that functionality is moved to cljs.analyzer.passes.

(when (and *analyze-deps* (seq @deps))
      (analyze-deps name @deps env opts))
    #_ (when (and *analyze-deps* (seq uses))
         (check-uses uses env))
    (set! *cljs-ns* name)
    #_ (when *load-macros*
         (load-core)
         (doseq [nsym (concat (vals require-macros) (vals use-macros))]
           (clojure.core/require nsym))
         (when (seq use-macros)
           (check-use-macros use-macros env)))

The question is where do we require cljs.analyzer.passes (must be required somewhere so the default is configured correctly)?

cljs.closure is probably the best bet.

Not a big fan of this alter-var-root but I'm not sure how we could do without. I doubt anyone but me currently uses cljs.analyzer/passes and therefore the default behavior of (or passes [infer-type]) always triggers. Since that would no longer work correctly we need to initialize to another default.

cljs.analyzer/parse-ns would also need to change again given the removal of load-macros. Same functionality can be achieved within the same binding though.

[1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj

Comment by Thomas Heller [ 03/Jan/15 8:16 PM ]

My head hurts, I need to get some sleep.

I cannot figure out why the attached patch does not work, but ./script/test produces errors in parse-ns.

Will get back to it tommorrow.

Exception in thread "main" java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol, compiling:(/Users/zilence/code/oss/clojurescript/bin/../bin/cljsc.clj:18:68)
	at clojure.lang.Compiler.load(Compiler.java:7142)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$script_opt.invoke(main.clj:336)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clojure.lang.Var.invoke(Var.java:388)
	at clojure.lang.AFn.applyToHelper(AFn.java:160)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol
	at clojure.lang.RT.seqFrom(RT.java:505)
	at clojure.lang.RT.seq(RT.java:486)
	at clojure.core$seq.invoke(core.clj:133)
	at cljs.analyzer$parse_ns$fn__1660.invoke(analyzer.clj:1757)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1747)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1741)
	at cljs.compiler$compile_root.invoke(compiler.clj:1069)
	at cljs.closure$compile_dir.invoke(closure.clj:358)
	at cljs.closure$eval3084$fn__3085.invoke(closure.clj:398)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$eval3071$fn__3072.invoke(closure.clj:412)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$build.invoke(closure.clj:1018)
	at cljs.closure$build.invoke(closure.clj:972)
	at user$eval3279.invoke(cljsc.clj:21)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	... 9 more
Comment by Thomas Heller [ 04/Jan/15 5:40 AM ]

That is why you should probably not write code tired at 3am. Was using infer-tag as a pass instead of infer-type.

I decided to drop the cljs.analyzer.passes namespace and instead moved the passes to cljs.analyzer. The extra namespace introduced a weird circular dependency which cljs.analyzer/parse-ns couldn't quite handle.

The patch now applies cleanly, would probably be nice to have a test that actually tests that it does what its supposed to.

Comment by David Nolen [ 07/Jan/15 8:21 AM ]

The patch has not been updated with these changes.

Comment by Thomas Heller [ 07/Jan/15 8:37 AM ]

Sorry, my mistake. Fixed.

Comment by Thomas Heller [ 08/Jan/15 7:19 AM ]

One issue I found is related to caching. Depends on how/where cljs.analyzer/parse-ns is called.

If the new passes aren't executed and the result of that analysis are used to write out the cache it won't contain the new macros information. I believe cljs.analyzer/parse-ns does this.

On the next run if it was decided to use the cache, we are missing some information AND won't require the macros. This was true before the changes but since the dependant namespaces would require the macros on their own the issue was somewhat hidden. Now they assume that macros were already loaded, therefore the must require macros when reconstructing from cache.

How to do that best in CLJS I don't know since shadow-build does its own caching. The passes could be refactored so they can be called with the cached information as well.

Note that this issue was basically introduced by load-macros (CLJS-940), but since that flag is already gone it is now moved to the :load-macros option of parse-ns.

The simplest way to address this would be to always run the new passes, basically how it was before CLJS-940.

Comment by David Nolen [ 08/Jan/15 7:39 AM ]

Thomas to me this illustrates the pitfalls of trying to bring a multipass approach to an existing compiler that isn't already fundamentally multipass. It seems to me that an alternative approach is possible without bothering at all about passes. In the analyzer parse 'ns case for each required CLJS lib you could leverage parse-ns to see if it requires a macro file of the same name - if it does add an implicit :require-macros.

While I like the passes approach in theory it will just make it harder for contributors to understand the structure of the compiler, they have to look at two different approaches to determine where things happen.

Comment by Thomas Heller [ 08/Jan/15 7:56 AM ]

Well, yeah we could to away with the passes and do it all in (defmethod parse 'ns ...) but that doesn't change the issue as long as there is a way to toggle this at all (ie. :load-macros or load-macros). It is not really about whether it is in a pass or not.

I just liked using passes since it sort of allows cherry-picking what you want and don't want to run. infer-types doesn't need to run in parse-ns for example, but it always did before this.

But I'm not committed to passes, happy to move everything so it runs in the parse method.

Let me know, if I should rewrite the patch.

Comment by Thomas Heller [ 08/Jan/15 8:35 AM ]

Hmm, you are right. Will move it out of the passes.

The only reason it was in there in the first place is due to shadow-build. It was the only place I could put it to make it work without patching CLJS. But since we agree that this should be in core the passes option really doesn't make sense any more.

Going to deliver a new patch soon.

Comment by David Nolen [ 08/Jan/15 8:37 AM ]

The difference for inter-types is that is something that needs to be run for a large number of nodes - it's a real global pass.

Let's go with moving the logic in to parse 'ns. I still don't understand the *load-macros* issue you are describing. We absolutely need to be able to disable this and dependency analysis for legitimate uses of parse-ns.

Comment by Thomas Heller [ 08/Jan/15 9:42 AM ]

Attached the no-passes version. The does get a bit simpler since it doesn't have to manually patch the compiler env.

I'm not totally sure how the @reload logic works, maybe some adjustments are required.

Comment by Thomas Heller [ 08/Jan/15 9:53 AM ]

The *load-macros* issue manifests as follows:

(ns demo.ns1
  (:require-macros [demo.ns1 :as m]))

(ns demo.ns2
  (:require [demo.ns1 :as x :refer (a-macro)]))

Compile 1: Everything is fine, cache is written.
Compile 2:

  • ns1 not modified, parse-ns decides to use cached version
  • ns2 modified, compiler attempts to compile, fails with NullPointerException
Caused by: java.lang.NullPointerException
	at cljs.analyzer$get_expander.invoke(analyzer.clj:1532)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1543)
	... 80 more

This is because the load from cache does not require the macro namespace. Before these changes demo.ns2 would have had a :require-macros or equivalent in the ns form which would then have require'd the macro ns. Now only demo.ns1 would trigger the require, but since it loads from cache it doesn't UNLESS *load-macros* is true. Since parse-ns does revive the cache (I think) it is not.

The require must be triggered somewhere before actual compilation happens. In shadow-build this is pretty simple but I'm not sure where to put this for CLJS.

Comment by Thomas Heller [ 09/Jan/15 10:25 AM ]

As per IRC the *load-macros* issues does not present in CLJS since it is true when it needs to be.





[CLJS-947] REPL require of goog namespaces does not work Created: 01/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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 [ 02/Jan/15 4:30 PM ]

fixed https://github.com/clojure/clojurescript/commit/935b0e466c6bd327799ca0a04f062b8aa23dcebe





[CLJS-946] goog.require in REPLs will not reload recompiled libs Created: 01/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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   

This makes for a bad REPL experience. (require 'foo.core) will recompile, but the emitted goog.require does not correctly reload the already seen namespace. We should monkey patch goog.require for all REPLs so that this works correctly.



 Comments   
Comment by David Nolen [ 01/Jan/15 4:53 PM ]

This is actually like a problem with Node.js integration, we need to invalidate the cache http://nodejs.org/docs/latest/api/globals.html#globals_require_cache

Comment by David Nolen [ 01/Jan/15 10:23 PM ]

fixed https://github.com/clojure/clojurescript/commit/4665f9a787c2bcb9d76066ba9ddccb89cf378dc3

Comment by David Nolen [ 01/Jan/15 10:25 PM ]

Not quite fixed, the paths are correct, but it appears goog.require has some logic that prevents from even getting to Node.js require

Comment by David Nolen [ 02/Jan/15 1:04 PM ]

fixed https://github.com/clojure/clojurescript/commit/187a45ff599eafd9e8b4f95bb31778a84ac9322f





[CLJS-945] Compile core with :static-fns true by default Created: 01/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/15

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


 Comments   
Comment by David Nolen [ 03/Jan/15 7:06 PM ]

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





[CLJS-944] deps file produced by :none needs compilation comment Created: 01/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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   

Without this a different version of the compiler believes that recompilation is not necessary.



 Comments   
Comment by David Nolen [ 01/Jan/15 2:31 PM ]

Fixed in master but is not the root reason project is not recompiled. Needs more investigation.

Comment by David Nolen [ 02/Jan/15 11:28 AM ]

This is a bug in cljsbuild. cljsbuild does it own file change detection to trigger rebuilds - this is at odds with the compiler checking compiled artifacts for staleness.





[CLJS-943] REPL require special fn is brittle Created: 31/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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   

Spec merging needs to be more robust. Additionally all errors during ns form parsing should throw not just print a warning. Otherwise the information about the ns due to REPL interaction can enter a corrupted state.



 Comments   
Comment by David Nolen [ 01/Jan/15 12:41 PM ]

Fixed https://github.com/clojure/clojurescript/commit/1a7546ecba81683b67278a47dec490761e13718e





[CLJS-942] Randomized port for Node.js REPL Created: 31/Dec/14  Updated: 31/Dec/14  Resolved: 31/Dec/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   

We need to randomize the port so the user can start multiple repls without bother to reason about which port to use.



 Comments   
Comment by David Nolen [ 31/Dec/14 4:21 PM ]

fixed https://github.com/clojure/clojurescript/commit/133721a20a4879253b5ce5f913bd2f6555b8e3c6





[CLJS-941] Warn when a symbol is defined multiple times in a file Created: 31/Dec/14  Updated: 03/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 941-fix2.patch     Text File 941-fix.patch     Text File 941.patch    

 Description   

The only reason Clojure allows vars to be re-def-ed is so you can fix bugs in running programs.
- Rich Hickey

(ns example.core)
(defn foo [] "hi")
(defn foo [] "hello")

;; WARNING: foo at line 2 is being replaced at line 3 src/example/core.cljs

;; The warning only applies to files, not REPLs.
;; Also ignoring the "cljs" namespace since many symbols are redefined there.



 Comments   
Comment by Shaun LeBron [ 31/Dec/14 11:53 AM ]

Included a short patch. Is ignoring the entire `cljs` namespace a good idea? I noticed a lot of redefs in there.

Comment by David Nolen [ 31/Dec/14 12:36 PM ]

What is the intent of the cljs-file check? Also the exclusion of core is not desirable. I looked at the warnings and most of them are legitimate problems in cljs.core with the exception of ITER_SYMBOL and imul. These two are problematic cases for this patch as they represent a real useful pattern I expect to appear in many ClojureScript codebases.

I think the warning should only be emitted if the defs are not embedded in a conditional expression.

Comment by Shaun LeBron [ 31/Dec/14 2:05 PM ]

updated patch. The cljs-file check was an attempt to prevent the warning when redefining symbols from a REPL. But I just changed it to check if it's "<cljs repl>".

I'll look into preventing the warning for defs in conditional expressions.

Should we correct the duplicated function definitions in cljs.core in a separate patch?

Comment by Shaun LeBron [ 31/Dec/14 2:09 PM ]

the updated patch also prevents this warning if a `declare` comes after a `def`. No harm there.

Comment by David Nolen [ 31/Dec/14 2:15 PM ]

I'd rather just have a single squashed patch. Agree that declare after def is harmless and this is in line with the behavior of Clojure.

Comment by Shaun LeBron [ 31/Dec/14 4:14 PM ]

updated patch:

  • removed the redefs for cljs.core [sequence rand rand-int] that triggered redef warnings
  • allow redefs inside an if-block for conditional def'ing (using a dynamic var allow-redef)
Comment by David Nolen [ 31/Dec/14 4:27 PM ]

fixed https://github.com/clojure/clojurescript/commit/378a75e4b812ebc5a3596bc94d1afd3f6fb1506f

Comment by David Nolen [ 31/Dec/14 4:42 PM ]

Shaun, reopening as I had to disable this because it still interacts badly with REPLs. To see the problem reenable the warning and follow the instructions here: http://swannodette.github.io/2014/12/31/the-old-way-the-new-way/

Comment by Shaun LeBron [ 31/Dec/14 6:18 PM ]

updated patch.

I saw the warnings for every symbol after starting the rhino REPL. It was compiling the same files from local maven and then ".repl/" for some reason, triggering the redef warnings.

To fix, I just added a check to trigger the warning only if we're overriding a symbol from the same file path, which is the only case we want to consider anyway.

Comment by David Nolen [ 31/Dec/14 6:48 PM ]

Thanks for the extra information, will look into why an extra compile is being triggered.

Comment by David Nolen [ 01/Jan/15 12:46 PM ]

fixed https://github.com/clojure/clojurescript/commit/9a5454c68c6202a732a5b0220120bde63da9bd94

Comment by David Nolen [ 01/Jan/15 1:21 PM ]

I've had to disable this again in master. Running the Rhino REPL from a checkout still spills warnings. Before turning this back on it's going to need a lot more testing under the various REPLs and incremental build scenarios.

Comment by Shaun LeBron [ 01/Jan/15 6:25 PM ]

Was able to reproduce. repl-rhino is now binding relative file paths to cljs-file for some reason, causing the filename inequality check to fail.

A general solution is for the analyzer to uniquely identify a file "session", so redefs emit warnings within a session, but not across them to allow redefs from the REPL or file-reloads.

This patch binds a new cljs-file-session var to a timestamp alongside cljs-file. Seems to have worked for the following cases:

  • tested in repl.rhino
  • tested in repl.node
  • tested in figwheel
  • tested in weasel
Comment by Andy Fingerhut [ 02/Jan/15 10:46 AM ]

Eastwood only works for Clojure on the JVM right now, and includes a warning like this because they seemed unwilling to include such a thing in Clojure itself, particularly due to interactive reloading of files during many people's typical development workflows. ClojureScript might be enough different that it makes more sense to include it in the compiler, but the number of times it has been undone makes me wonder.

Comment by Shaun LeBron [ 02/Jan/15 12:05 PM ]

I think since the analyzer functions work in sessions (i.e. one file at a time, or form at time in REPL), we can get the benefit of this warning in the compiler itself by preventing redefs on a session basis. Just took me a few tries to realize that.

Would be nice as a default capability in the compiler since its expected behavior for most compilers to detect this. Thanks for insight on Clojure and Eastwood!

Comment by David Nolen [ 03/Jan/15 2:04 PM ]

Shaun the patch is interesting but I'm concerned about using System/currentTimeMillis for the identifier. One idea that's been rolling around my head for some time is parallel compilation. This is not going to work for that. Is there any reason not to use a proper sentinel?

Comment by Shaun LeBron [ 03/Jan/15 6:09 PM ]

good point, updated fix2 patch:

  • use a uuid instead of timestamp for the file session
  • for extra caution, don't warn redefs if file session is unbound (nil)




[CLJS-940] Make macro loading of the 'ns form optional Created: 31/Dec/14  Updated: 31/Dec/14  Resolved: 31/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File optional-macro-loading.diff    

 Description   

When parsing the (ns ...) form it has the costly side effect of immediately require'ing all macro namespaces it finds. While this is nescessary for parsing the rest of the file it is not required when just parsing the ns form.

Tools should be able to disable the loading of the macros to allow faster inspection of the ns form without this side-effect.

In shadow-build I have a function which basically does what cljs.build.api/parse-js-ns does just for ClojureScript files, this function is slow initially and I'd like be able to delay the require of macro namespaces as they are not required when just looking at the ns form.



 Comments   
Comment by Thomas Heller [ 31/Dec/14 11:16 AM ]

Whoops, the first patch didn't come out right. Fixed.

Comment by David Nolen [ 31/Dec/14 12:24 PM ]

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





[CLJS-939] Quit noderepljs with EOF Created: 31/Dec/14  Updated: 31/Dec/14

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

Type: Defect Priority: Minor
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

As is common in every other REPL out there, Ctrl+D on a new line should quit the REPL






[CLJS-938] actual configurable port for Node.js REPL Created: 30/Dec/14  Updated: 31/Dec/14  Resolved: 31/Dec/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   

Currently hardcoded on the Node.js side to 5001



 Comments   
Comment by David Nolen [ 31/Dec/14 4:27 PM ]

fixed by CLJS-942 https://github.com/clojure/clojurescript/commit/133721a20a4879253b5ce5f913bd2f6555b8e3c6





[CLJS-937] local fn name should be namespace munged Created: 30/Dec/14  Updated: 30/Dec/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


 Description   
(ns foo.bar)

(fn console []
  ...)

The local name should be foo$bar$console



 Comments   
Comment by David Nolen [ 30/Dec/14 3:12 PM ]

See CLJS-833





[CLJS-936] Multi arity bitwise operators Created: 30/Dec/14  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

Type: Defect Priority: Trivial
Reporter: Justin Tirrell Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 936.patch     Text File 936.patch    
Patch: Code

 Description   

Unlike regular Clojure, bitwise operators only support two arguments

(bit-or 0 1)

not

(bit-or 0 0 1)



 Comments   
Comment by David Nolen [ 30/Dec/14 1:14 PM ]

Looks good but can we get a new patch that includes tests? Also have you sent in your CA? Thanks!

Comment by Justin Tirrell [ 30/Dec/14 2:13 PM ]

Just attached a new patch that includes tests. I submitted the CA yesterday, should I attach it here?

Comment by David Nolen [ 06/Jan/15 1:39 PM ]

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





[CLJS-935] script/noderepljs leaves node running after exit Created: 30/Dec/14  Updated: 30/Dec/14  Resolved: 30/Dec/14

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

Type: Defect Priority: Major
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

After starting noderepljs, then exiting it (via :cljs/quit), the node process is left running and listening on port 5001. This causes all subsequent executions of noderepljs to report "Error: listen EADDRINUSE"

clojurescript master mn › ./script/noderepljs
To quit, type: :cljs/quit
ClojureScript Node.js REPL server listening on 5001
ClojureScript:cljs.user> :cljs/quit

clojurescript master mn › ps
PID TTY TIME CMD
17400 ttys000 0:00.10 -bash
20391 ttys000 0:00.17 node
17634 ttys001 0:00.06 -bash

clojurescript master mn › ./script/noderepljs
To quit, type: :cljs/quit
ClojureScript Node.js REPL server listening on 5001
Error: listen EADDRINUSE
at errnoException (net.js:904:11)
at Server._listen2 (net.js:1042:14)
at listen (net.js:1064:10)
at Server.listen (net.js:1138:5)
at [stdin]:42:4
at Object.<anonymous> ([stdin]-wrapper:6:22)
at Module._compile (module.js:456:26)
at evalScript (node.js:536:25)
at ReadStream.<anonymous> (node.js:154:11)
at ReadStream.emit (events.js:117:20)
goog.require could not find: cljs.core
Error: goog.require could not find: cljs.core
at Error (<anonymous>)
at Object.goog.require (/Users/mtnygard/work/clojurescript/.cljs_node_repl/goog/base.js:470:13)
at repl:1:6
at Socket.<anonymous> ([stdin]:26:80)
at Socket.emit (events.js:95:17)
at Socket.<anonymous> (_stream_readable.js:764:14)
at Socket.emit (events.js:92:17)
at emitReadable_ (_stream_readable.js:426:10)
at emitReadable (_stream_readable.js:422:5)
at readableAddChunk (_stream_readable.js:165:9)
ClojureScript:cljs.user>



 Comments   
Comment by David Nolen [ 30/Dec/14 11:22 AM ]

fixed https://github.com/clojure/clojurescript/commit/86fa7da377c94aa9bab5b7f53374d16aa9eb2298





[CLJS-934] In the REPL return vars after defs Created: 30/Dec/14  Updated: 14/Jan/15

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   

We don't want to emit these in during normal compilation. However it's nice to unify the REPL experience with Clojure's. Currently we just display the value of the def. REPLs could set a :repl build flag which is checked by the emit* :def case. For this to work the analyzer should compile the var AST and include it in the def AST so the compiler can optionally use it.



 Comments   
Comment by Mike Fikes [ 14/Jan/15 4:03 PM ]

A change was recently made to introduce new behavior if a :repl-verbose flag is set.

Perhaps all flags that control REPL behavior could be prefixed with :repl-, with the flag controlling this ticket controlled by :repl-def-vars or somesuch.





[CLJS-933] Redef warning omiting namespace of original symbol Created: 29/Dec/14  Updated: 30/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 933.patch    

 Description   

;; The redef warning is slightly broken:

(ns example.core
(:require [example.foo :refer [a]]))

(def a 1)

;; actual
;; WARNING: a already refers to: /a being replaced by: example.core/a at line 4 src/example/core.cljs

;; expected
;; WARNING: a already refers to: example.foo/a being replaced by: example.core/a at line 4 src/example/core.cljs



 Comments   
Comment by Shaun LeBron [ 29/Dec/14 4:41 PM ]

attached a one-line patch. An `:ns` value wasn't being passed to the warning (see link below), so I'm just using `:ev` value to get the fully qualified name.

https://github.com/clojure/clojurescript/blob/0da30f5aa3b937d1a1c01891cb4601be8a3ea210/src/clj/cljs/analyzer.clj#L654

Comment by David Nolen [ 30/Dec/14 8:04 AM ]

Looks good but this is not a properly formatted patch: https://github.com/clojure/clojurescript/wiki/Patches

Could we get a new one? Thanks!

Comment by Shaun LeBron [ 30/Dec/14 11:23 AM ]

oops, attached proper patch. thanks

Comment by David Nolen [ 30/Dec/14 11:23 AM ]

Looks good have you sent in your CA? Thanks again.

Comment by Shaun LeBron [ 30/Dec/14 1:33 PM ]

Just completed the CA submission.





[CLJS-932] Node REPL errors on exit Created: 29/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Defect Priority: Minor
Reporter: Peter Schuck Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File node_repl_close.patch    
Patch: Code

 Description   

When -tear-down is called on NodeEnv the socket atom isn't dereferenced so the close method is called on the socket atom rather then the sockets.



 Comments   
Comment by David Nolen [ 01/Jan/15 3:32 PM ]

fixed https://github.com/clojure/clojurescript/commit/6296384e536212c8997aa6fa05d2f6f29ffbb077





[CLJS-931] cljs.compiler/requires-compilation? ignores changes to build options Created: 27/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

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

 Description   

static-fns, optimize-constants, and elide-asserts change the emitted code. If these build settings change we need to recompile. Compiled files should includes in a JS comment the EDN set literal of the code affecting compiler build options used.



 Comments   
Comment by Thomas Heller [ 29/Dec/14 7:54 AM ]

Not sure that requires-compilation? is the correct entry point but I believe it is vital that files that produced warnings are always recompiled. It is very easy to miss warnings and until you wipe all files you won't see them again.

Comment by David Nolen [ 30/Dec/14 2:00 PM ]

Thomas, I think we want to avoid addressing the symptom over the cause. It may also very well be the case that CLJS-927 is the solution to this particular problem.

Comment by Romain Primet [ 30/Dec/14 5:03 PM ]

I submitted a patch for this issue following an IRC chat earlier today.

Comment by Romain Primet [ 30/Dec/14 5:18 PM ]

Version number issue in the first patch.

Comment by David Nolen [ 30/Dec/14 6:38 PM ]

This looks pretty good, will apply when your CA appears. Thanks very much!

Comment by Romain Primet [ 30/Dec/14 6:44 PM ]

Great, thanks for the guidance!

Comment by David Nolen [ 31/Dec/14 4:34 PM ]

Just tried to apply this to master. 2 tests fail for me when running lein test.

Comment by Romain Primet [ 31/Dec/14 5:00 PM ]

I had this issue, but that was due to there being no cljs version number defined by the build script. Once corrected it passed. Could you try this?

Comment by David Nolen [ 31/Dec/14 5:10 PM ]

The patch should be modified so that the test can pass. A simple way to do this is with with-redefs so you can provide a dummy ClojureScript version.

Comment by Romain Primet [ 31/Dec/14 5:25 PM ]

Fixed the test to pass without requiring a cljs version number

Comment by David Nolen [ 01/Jan/15 12:49 PM ]

fixed https://github.com/clojure/clojurescript/commit/79157c2fbc7aece8c3c13f9d78cc6bbb4cc3be37





[CLJS-930] Spelling mistakes in function documentation Created: 26/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Defect Priority: Minor
Reporter: Benjamin Meyer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fix-spelling-mistakes-in-function-documentation.patch    
Patch: Code

 Description   

The clojurescript function documentation contained a few spelling mistakes.

I have signed the Contributor Agreement and created a patch with the fixes



 Comments   
Comment by David Nolen [ 01/Jan/15 3:37 PM ]

fixed https://github.com/clojure/clojurescript/commit/077ccaad863e0fdb0533e13a82e46963022cb018





[CLJS-929] Minor fixes to test script Created: 26/Dec/14  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Alex Dowad Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Linux Mint 17.1, Linux 3.13.0-24-generic kernel, x86_64 CPU


Attachments: Text File 929.patch     Text File 929.patch     Text File 929.patch    
Patch: Code

 Description   

$[], used in ./script/test, doesn't work in ash or dash. This is a problem for users of Ubuntu and other Debian-based Linux distros, since they ship with dash as the default /bin/sh. (Some other UNIX-like OSs, including various flavors of BSD, as well as Minix, also use ash.)

$(()) works in ash, dash, bash, and zsh.



 Comments   
Comment by Alex Dowad [ 26/Dec/14 1:33 AM ]

Grrr, there was a typo in the first patch. Here is a corrected version.

Comment by David Nolen [ 01/Jan/15 3:35 PM ]

This patch does not apply cleanly to master anymore. Can we get a rebased patch? Thanks.

Comment by Alex Dowad [ 02/Jan/15 3:13 AM ]

Rebased patch attached.

Comment by David Nolen [ 02/Jan/15 8:46 AM ]

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





[CLJS-928] Add cljs.test/are macro Created: 24/Dec/14  Updated: 26/Dec/14  Resolved: 24/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

cljs.test is lacking "are" macro. It can be copied as-is from clojure.test



 Comments   
Comment by Alex Dowad [ 26/Dec/14 3:42 AM ]

What is this ticket a duplicate of?

Comment by Nikita Prokopov [ 26/Dec/14 4:20 AM ]

Alex, https://github.com/clojure/clojurescript/commit/6023cd14bd642e31b9c3ea5ace14e258ab0950f2





[CLJS-927] :recompile-dependents build option Created: 24/Dec/14  Updated: 01/Feb/15  Resolved: 01/Feb/15

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


 Description   

It should be possible to optionally recompile dependent namespaces. This would make disk based caching less prone to strange errors because the user didn't do this manually.



 Comments   
Comment by David Nolen [ 25/Dec/14 12:55 AM ]

cljs.closure/add-dependencies looks like the right place to do this. For this to work we would need to always sort files into dependency order before compilation. While this is done for cljs.closure/add-dependencies it is not done for files encountered in the source directory as a result of the implementation of cljs.compiler/compile-root.

If files were always guaranteed to be compiled in dependency order we could then easily pass the dependency information along and use this in cljs.compiler/requires-compilation? - a file would then additionally require compilation if any of the namespaces it depends on was recompiled.

Comment by David Nolen [ 01/Feb/15 2:02 PM ]

fixed https://github.com/clojure/clojurescript/commit/05d0209c388bb1955af7170d573afae4d26695e1





[CLJS-926] Caching support for all REPLs Created: 24/Dec/14  Updated: 29/Dec/14  Resolved: 29/Dec/14

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

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


 Description   

All ClojureScript REPLs would benefit wrt. to load time from the same caching support provided to builds.



 Comments   
Comment by David Nolen [ 29/Dec/14 9:00 AM ]

Just required threading build options through the repl support fns https://github.com/clojure/clojurescript/commit/caaf970a503eaeae5acdd269e41f4f6bbb1c215c





[CLJS-925] Node.js REPL Created: 24/Dec/14  Updated: 29/Dec/14  Resolved: 29/Dec/14

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

Type: Task Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None


 Description   

JVM based ClojureScript REPLs suffer from loading compiled cljs/core.cljs. Supporting a modern JS runtime like Node.js would make plain ClojureScript REPL interactions considerably faster.



 Comments   
Comment by David Nolen [ 24/Dec/14 11:35 AM ]

Some basic benchmarking information, for Node.js, JavaScriptCore, V8, SpiderMonkey loading the compiled cljs/core.js (including goog deps) takes 100-200ms on a 2010 Macbook Pro 2.26ghz. Under Rhino ~800ms. Under Nashorn ~8500ms.

Comment by David Nolen [ 29/Dec/14 9:00 AM ]

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





[CLJS-924] Better error message for mistaken use of 'def' Created: 24/Dec/14  Updated: 24/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Dowad Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Better-error-message-if-def-is-mistakenly-substitute.patch    
Patch: Code

 Description   

ClojureScript shares what is IMHO one of the biggest weaknesses of the current JVM Clojure implementation: those (in)famous stack traces going down into the innards of the compiler if you get your syntax wrong. An easy mistake for noobs is to use 'def' in place of 'defn'; this patch makes that mistake a lot less painful to debug.

Feedback please!






[CLJS-923] Inconsistent warning defaults Created: 24/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File inconsitent-warning-defaults.diff    
Patch: Code

 Description   

The cljs compiler option :warnings is initialized with inconsistent defaults [1]

(def ^:dynamic *cljs-warnings*
  {:preamble-missing true
   :unprovided true
   :undeclared-var false
   :undeclared-ns false
   :undeclared-ns-form true
   :redef true
   :dynamic true
   :fn-var true
   :fn-arity true
   :fn-deprecated true
   :protocol-deprecated true
   :undeclared-protocol-symbol true
   :invalid-protocol-symbol true
   :multiple-variadic-overloads true
   :variadic-max-arity true
   :overload-arity true
   :extending-base-js-type true
   :invoke-ctor true
   :invalid-arithmetic true
   :protocol-invalid-method true
   :protocol-duped-method true
   :protocol-multiple-impls true})

If :warnings is not set

(opts :warnings true)
will enable
[:unprovided :undeclared-var :undeclared-ns :undeclared-ns-form]
, if :warnings is false it will disable them. [2]

If warnings is given a map like {:invalid-arithmetic false} it will also disable (or leave disabled) the :undeclared-var and :undeclared-ns which would otherwise be enabled by the previous logic. Having the effect of disabled 3 warnings instead of 1.

The patch only changes the defaults, although at some point the warning logic in cljs.closure/build should probably be cleaned up a little.

Why does {:warnings false} only disable 4 warnings? [2]

[1] https://github.com/clojure/clojurescript/blob/d3da2349c175a9066f4d9e4476302ff497f51937/src/clj/cljs/analyzer.clj#L40-L62
[2] https://github.com/clojure/clojurescript/blob/d3da2349c175a9066f4d9e4476302ff497f51937/src/clj/cljs/closure.clj#L981-L991



 Comments   
Comment by Thomas Heller [ 24/Dec/14 7:25 AM ]

Shouldn't paste from the patched version.

Comment by David Nolen [ 24/Dec/14 7:51 AM ]

thanks! fixed https://github.com/clojure/clojurescript/commit/b9e475698426b8bb8862ef32706b925bd6d691cd





[CLJS-922] Warning message for non-dynamic vars is incomplete Created: 23/Dec/14  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

Type: Defect Priority: Trivial
Reporter: Michael Griffiths Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: analyzer, cljs

Attachments: Text File CLJS-922-001.patch     Text File CLJS-922-002.patch     PDF File Michael Griffiths Clojure CA.pdf    

 Description   

e.g.

user> (do 
        (cljs.analyzer/analyze nil '((ns dynamic-warning-test-ns)
                                     (def a nil)
                                     (binding [a nil] nil)))
        nil)
WARNING:  not declared ^:dynamic at line 4 
;; => nil

The name of the var is missing at the start of the warning message.



 Comments   
Comment by Michael Griffiths [ 23/Dec/14 3:58 PM ]

Patch attached. Evaluating the above form with patch applied now results in:

WARNING: dynamic-warning-test-ns/a not declared ^:dynamic at line 4

Maybe it would be fine to just use the name of the var rather than the namespace-qualified name - let me know if you want the patch updated.

Comment by David Nolen [ 24/Dec/14 7:52 AM ]

Patch looks good, have you submitted a CA? Thanks.

Comment by Mike Fikes [ 24/Dec/14 8:09 AM ]

I wonder if the fix for CLJS-921 will affect this patch. (I'm thinking that the patch relied on the :name meta containing the fully qualified name, but that with CLJS-921 the :ns would also need to be consulted—as is done in the doc printing impl.)

Comment by David Nolen [ 24/Dec/14 8:16 AM ]

The patch is at the level of the compiler not the runtime so it should be unaffected by CLJS-921.

Comment by Michael Griffiths [ 24/Dec/14 11:29 AM ]

Yeah, I signed an electronic CA the same day I submitted the patch - I'm not sure how often the list of contributors is updated on clojure.org but should be on there soon. Find attached regardless.

Thanks!

Comment by Michael Griffiths [ 05/Jan/15 5:17 AM ]

I'm now listed on http://clojure.org/contributing.

Comment by David Nolen [ 05/Jan/15 9:32 AM ]

The patch needs to be rebased to master.

Comment by Michael Griffiths [ 06/Jan/15 9:54 AM ]

Of course. Rebased patch attached.

Comment by David Nolen [ 06/Jan/15 1:41 PM ]

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





[CLJS-921] cljs.repl/doc output includes namespace twice Created: 23/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

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

r2511



 Description   

The first line of cljs.repl/doc output is the fully-qualified name of the var, and in this output the namespace is repeated.

Here is an example. Note that cljs.core is repeated:

(cljs.repl/doc str)
-------------------------
cljs.core/cljs.core/str
([] [x] [x & ys])
  With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args.

This appears to be rooted in the fact that the :name meta contains the namespace (in contrast with Clojure, which this is not the case).

(meta (var str))
=> {:arglists ([] [x] [x & ys]), :test nil, :column 1, :line 2134, :file ".repl/cljs/core.cljs", :doc "With no args, returns the empty string. With one arg x, returns\n  x.toString().  (str nil) returns the empty string. With more than\n  one arg, returns the concatenation of the str values of the args.", :name cljs.core/str, :ns cljs.core}


 Comments   
Comment by David Nolen [ 24/Dec/14 8:02 AM ]

fixed https://github.com/clojure/clojurescript/commit/88b3d1e54ba43c595e11aeab226cfd071ca6f657





[CLJS-920] add-watch/remove-watch should return reference, as in Clojure Created: 23/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-920-add-remove-watch-return-iref.patch    

 Description   

For now, Atom/-add-watch returns atom, Atom/-remove-watch returns (.-watchers atom). Implementers of IWatchable should be careful to return this, otherwise they may incidentally broke this convention.

Attached patch changes wrapper functions add-watch/remove-watch to always return iref passed in, ignoring what -add-watch/-remove-watch implementations return.



 Comments   
Comment by David Nolen [ 24/Dec/14 8:09 AM ]

Thanks! In the future it would be nice to get the accompanying test with a patch like this.

Comment by David Nolen [ 24/Dec/14 8:20 AM ]

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





[CLJS-919] compare-and-set! relies on Atom record structure instead of protocols Created: 23/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: atom, protocols

Attachments: Text File cljs-919-generic-cas.patch    

 Description   

Here CAS uses .-state property, thus binding itself to Atoms only:

(defn compare-and-set! [a oldval newval]
  (if (= (.-state a) oldval)
    (do (reset! a newval) true)
    false))

We can replace it with -deref call instead, opening it up for alternative implementations:

(defn compare-and-set! [a oldval newval]
  (if (= (-deref a) oldval)
    (do (reset! a newval) true)
    false))


 Comments   
Comment by David Nolen [ 24/Dec/14 8:39 AM ]

thanks fixed https://github.com/clojure/clojurescript/commit/4ee89df427644c096d32117194ae1d76f8dec836





[CLJS-918] Analysis caching discard metadata Created: 22/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/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 [ 24/Dec/14 10:53 AM ]

We only want to preserve :arglists metadata. Fixed https://github.com/clojure/clojurescript/commit/fdcf333aadd8728e93e1044040b90d67e17b7f57





[CLJS-917] invalid "all arguments must be numbers" warning Created: 22/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

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


 Description   

With CLJS build 2498, I get this:

WARNING: cljs.core//, all arguments must be numbers, got #{nil clj-nil} number instead. at line 51 resources/public/cljs/out/cljs_time/coerce.cljs

For this code:

(defn to-epoch
  "Convert `obj` to Unix epoch."
  [obj]
  (let [millis (to-long obj)]
    (and millis (/ millis 1000))))        ; this is line 51

From cljs-time v0.2.4: https://github.com/andrewmcveigh/cljs-time



 Comments   
Comment by David Nolen [ 24/Dec/14 9:01 AM ]

Should be fixed by CLJS-907, the problem was to-long had a method call which was not type inferred prior to CLJS-907 patch.





[CLJS-916] Optimize use of js-arguments in array and variadic functions Created: 22/Dec/14  Updated: 03/Jan/15  Resolved: 03/Jan/15

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-fastcall.txt     Text File bench-master.txt     Text File cljs-916.patch    
Patch: Code

 Description   

There are currently three places where the magic arguments variable is used by the clojurescript codebase directly: the array function, in the outer arity dispatch for variadic functions, and in the max-fixed-arity method for variadic functions and protocols. In each case the goal is to cast the function arguments (or some subset) to a normal array, and in each case arguments is used in a way that is difficult for many vms to optimize. (V8 in particular cannot ever jit-optimize any function body where arguments is passed as a parameter to another function.) Microbenchmark js-perf of argument-to-array approaches

Attached is a patch which handles arguments better by looping over arguments inline and copying to a pre-allocated array. This gives me a 10x speedup in V8 for calls like (vector ...) and (array ...), and less significant speedups elsewhere.

Note this optimization really only matters for higher-order use of variadic functions and for compilation modes where :static-fns is off. Many functions (including array)are shadowed by macros which eliminate the arity dispatches and the :static-fns optimization will remove most other dispatches.

I have attached two advanced-compiled benchmarks (using script/benchmark) in v8, spidermonkey, and javascriptcore comparing master vs this patch on my machine. Note that this adds a few benchmark cases specifically to exercise the patched code. Look for ";; higher-order variadic function calls" near the end.



 Comments   
Comment by David Nolen [ 03/Jan/15 6:00 PM ]

a nice performance enhancement, thanks! fixed https://github.com/clojure/clojurescript/commit/dccc5d0feab9809cb56bd4d7257599a62232ac0d





[CLJS-915] On empty call, List and PersistentQueue do not retain meta, sorted-set/sorted map do not retain comparator Created: 19/Dec/14  Updated: 20/Dec/14  Resolved: 20/Dec/14

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File cljs-915-fix-empty-on-collections.patch    

 Description   

-empty method is implemented incorrectly at List, PersistentQueue, PersistentSortedSet, PersistentSortedMap

(-> (sorted-set-by (comp - compare))
    empty
    (into [2 3 1])) => #{1 2 3} (should be #{3 2 1})
(-> (sorted-map-by (comp - compare))
    empty
    (into [[2 :b] [3 :c] [1 :a]])) => {1 :a, 2 :b, 3 :c} (should be backwards)
(meta (empty '^{:a 1} (1 2 3))) => nil (should be {:a 1})
(meta (empty (with-meta (.-EMPTY PersistentQueue) {:b :c}))) => nil (should be {:b :c})

Patch with tests attached: cljs-915-fix-empty-on-collections.patch



 Comments   
Comment by David Nolen [ 20/Dec/14 3:27 PM ]

fixed https://github.com/clojure/clojurescript/commit/2d8b0ec44f214e059bdbeabbd6492e013347a093





[CLJS-914] thrown-with-msg? is unable to get message of exception Created: 19/Dec/14  Updated: 20/Dec/14  Resolved: 20/Dec/14

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

Type: Defect Priority: Major
Reporter: Matthew Boston Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 914.patch    

 Description   

In JavaScript, the message of an Error is retrieved with the Error.prototype.message property but the implementation of cljs.test/thrown-with-msg? uses the method .getMessage. This is causing the following error when writing tests with thrown-with-msg?.

Test console: expected: (thrown-with-msg? ArithmeticException #"Divide by zero" (\ 1 0))
Test console: actual:
Test console: #<TypeError: 'undefined' is not a function (evaluating 'e_5184auto_.getMessage()')>



 Comments   
Comment by Matthew Boston [ 19/Dec/14 5:31 PM ]

patch file for CLJS-914

Comment by David Nolen [ 20/Dec/14 3:35 PM ]

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





[CLJS-913] Error when trying to convert javascript object to clojure (js->clj obj) under :advanced compilation Created: 18/Dec/14  Updated: 31/Jan/15  Resolved: 31/Jan/15

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

Type: Defect Priority: Major
Reporter: Erik Wickstrom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript 0.0-2496

Tested on OSX 10.10 with Chrome 39



 Description   

I'm trying to convert a javascript object (parsed JSON from a web service) into a clojure data structure. It works fine if I use :simple optimization with the closure compiler, however when I switch to :advanced optimization, I get the following error:

(let my-data #js {} ; see below for JSON
(.info js/console "converted to clojure" (str (js->clj my-data))))

Uncaught Error: No protocol method IEmptyableCollection.-empty defined for type object: [object Object]

Note that this also only seems to happen with this chunk of JSON and a few other samples (though according to all the JSON linters I've tried, it is valid). I've passed other input through without issue. So it is somewhat intermittent but deepish object hierarchies seem to be a commonality.

Here is the JSON (also posted here http://pastebin.com/PLffFrFf )

[{"address_components":[{"long_name":"11810","short_name":"11810","types":["postal_code"]},{"long_name":"16 de Septiembre","short_name":"16 de Septiembre","types":["neighborhood","political"]},{"long_name":"Miguel Hidalgo","short_name":"Miguel Hidalgo","types":["sublocality_level_1","sublocality","political"]},{"long_name":"Ciudad de México","short_name":"México D.F.","types":["locality","political"]},{"long_name":"Distrito Federal","short_name":"D.F.","types":["administrative_area_level_1","political"]},{"long_name":"Mexico","short_name":"MX","types":["country","political"]}],"formatted_address":"16 de Septiembre, Miguel Hidalgo, 11810 Ciudad de México, D.F., Mexico","geometry":{"bounds":{"Ea":{"j":19.4043293,"k":19.3997335},"wa":{"j":-99.21262619999999,"k":-99.2045263}},"location":{"D":-99.20755880000002,"k":19.402037},"location_type":"APPROXIMATE","viewport":{"Ea":{"j":19.4043293,"k":19.3997335},"wa":{"j":-99.21262619999999,"k":-99.2045263}}},"types":["postal_code"]},{"address_components":[{"long_name":"11810","short_name":"11810","types":["postal_code"]},{"long_name":"West Jakarta","short_name":"West Jakarta","types":["locality","political"]},{"long_name":"Kamal","short_name":"Kamal","types":["administrative_area_level_4","political"]},{"long_name":"Kalideres","short_name":"Kalideres","types":["administrative_area_level_3","political"]},{"long_name":"West Jakarta City","short_name":"West Jakarta City","types":["administrative_area_level_2","political"]},{"long_name":"Jakarta","short_name":"Jakarta","types":["administrative_area_level_1","political"]},{"long_name":"Indonesia","short_name":"ID","types":["country","political"]}],"formatted_address":"Kamal, Kalideres, West Jakarta City, Jakarta 11810, Indonesia","geometry":{"bounds":{"Ea":{"j":-6.095065399999999,"k":-6.110835},"wa":{"j":106.68747699999994,"k":106.71448510000005}},"location":{"D":106.70282500000008,"k":-6.101219},"location_type":"APPROXIMATE","viewport":{"Ea":{"j":-6.095065399999999,"k":-6.110835},"wa":{"j":106.68747699999994,"k":106.71448510000005}}},"types":["postal_code"]},{"address_components":[{"long_name":"11810","short_name":"11810","types":["route"]},{"long_name":"Příbram District","short_name":"Příbram District","types":["administrative_area_level_2","political"]},{"long_name":"Central Bohemian Region","short_name":"Central Bohemian Region","types":["administrative_area_level_1","political"]},{"long_name":"Czech Republic","short_name":"CZ","types":["country","political"]},{"long_name":"261 01","short_name":"261 01","types":["postal_code"]}],"formatted_address":"11810, 261 01, Czech Republic","geometry":{"bounds":{"Ea":{"j":49.7328257,"k":49.7102303},"wa":{"j":13.979755599999976,"k":13.986990699999978}},"location":{"D":13.982032200000049,"k":49.7225575},"location_type":"GEOMETRIC_CENTER","viewport":{"Ea":{"j":49.7328257,"k":49.7102303},"wa":{"j":13.979755599999976,"k":13.986990699999978}}},"types":["route"]}]

Here is the original post to the mailing list: https://groups.google.com/forum/#!topic/clojurescript/MZ9esXbn2tg



 Comments   
Comment by David Nolen [ 31/Jan/15 2:23 PM ]

I cannot reproduce this. The only way this is possible is if you are loading some third party ClojureScript library which is unwisely extending JavaScript native types to ICollection. I recommend purging that dependency.





[CLJS-912] Minor enhancements to bootstrap script Created: 18/Dec/14  Updated: 20/Dec/14  Resolved: 20/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Dowad Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Check-dependencies-in-bootstrap-script.patch     Text File 0001-Enhancements-to-bootstrap-script.patch     Text File 0002-Display-status-message-if-download-fails-while-boots.patch     Text File 0003-Retry-failed-downloads-in-bootstrap-script-before-gi.patch    
Patch: Code

 Description   

Just some small tweaks to make things a bit friendlier for first-time users. See the attached *.patch files.

I would like to remove the -s flag from invocations of curl as well, at least for the larger downloads. I like feedback about what a script is doing, while it's doing it. What do you think? Silence is golden?



 Comments   
Comment by Alex Dowad [ 18/Dec/14 1:29 PM ]

Just filled in and signed the Clojure CA.

Comment by David Nolen [ 18/Dec/14 1:39 PM ]

Great! Can we please get a single squashed patch? Thanks!

Comment by Alex Dowad [ 18/Dec/14 2:21 PM ]

OK, here is one squashed commit. I would have thought that you would like "semantic commits" – one commit for each conceptual change. I guess this stuff is too trivial to litter the history with a lot of fine-grained commits.

Comment by David Nolen [ 20/Dec/14 3:57 PM ]

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





[CLJS-911] Cljs's clojure.string.replace replacement fn takes different args to Clj's clojure.string.replace Created: 17/Dec/14  Updated: 17/Dec/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
Environment:

ClojureScript 0.0-2411



 Description   

Clojure's `clojure.string.replace` takes a replacement fn which receives args `[[group1 group2 ...]]`.
ClojureScript's `clojure.string.replace` takes a replacement fn which receives args `[group1 group2 ...]` (i.e. & args).

It's my understanding that something like the `clojure.string` ns is intended partly to help pave over superficial API differences like this.

Modding ClojureScript's `string.replace` to match Clojure's behaviour would be trivial, but this would be a breaking change for anyone that's come to rely on the faulty[?] behaviour.

Would you like a patch for this? Can submit for http://dev.clojure.org/jira/browse/CLJS-794 while I'm at it (both involve a change to `clojure.string/replace`).

Thanks!






[CLJS-910] JavaScriptCore 0xbbadbeef EXC_BAD_ACCESS when evaluating (list 0 1 ... 18) Created: 16/Dec/14  Updated: 06/Jan/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Embedded JavaScriptCore on iOS simulator
Connected via Weasel / simple-brepl
:whitespace optimization


Attachments: PNG File memory.png     Text File stacktrace.txt    

 Description   

If I evaluate

(list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)

JavaScriptCore exhibits a 0xbbadbeef EXC_BAD_ACCESS, with a fairly deep stacktrace:

(lldb) bt

  • thread #1: tid = 0x3f7e, 0x0111e583 JavaScriptCore`WTFCrash + 67, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x0111e583 JavaScriptCore`WTFCrash + 67
    frame #1: 0x011395a9 JavaScriptCore`WTF::fastMalloc(unsigned long) + 1929
    frame #2: 0x00c9cb56 JavaScriptCore`WTF::Vector<JSC::UnlinkedInstruction, 0ul, WTF::UnsafeVectorOverflow>::expandCapacity(unsigned long) + 86
    frame #3: 0x00c90f27 JavaScriptCore`JSC::BytecodeGenerator::emitGetById(JSC::RegisterID*, JSC::RegisterID*, JSC::Identifier const&) + 311
    frame #4: 0x00fd4617 JavaScriptCore`JSC::DotAccessorNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 551
    ...

(Full stack trace attached as stacktrace.txt)

This only occurs with :whitespace optimization and does not under :advanced.

If you evaluate (list 0), it works, and so does (list 0 1), all the way up to 17. Interestingly, it gets progressively slower as you evaluate longer lists.



 Comments   
Comment by Mike Fikes [ 16/Dec/14 12:35 PM ]

While the EXC_BAD_ACCESS is arguably a bug in JavaScriptCore, it is likely provoked by excessive memory usage of the (list 0 1 ...) form. The attached memory.png shows what appears to be 2^n memory usage for evaluating a list of size n. This graph was produced while REPLd into an iOS device, monitoring memory use from Xcode.

Comment by Mike Fikes [ 18/Dec/14 11:28 AM ]

The construct

(list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)

works in r2014 (released Nov 6, 2013), but fails in r2024 (released Nov 8, 2013).

In r2014, the emitted JavaScript is:

cljs.core.list.call(null, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18)

while in r2024 the emitted JavaScript is:

cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null,
cljs.core.List.EMPTY, 18), 17), 16), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6), 5), 4), 3), 2), 1), 0)

This particular commit, between r2014 and r2014 on Nov 7 2013, is likely when the emitted JavaScript changed (I haven't pulled down that specific revision):

https://github.com/clojure/clojurescript/commit/5bcbc4745f599e352c51e01b210755a88aa4bc5f#diff-b64165608bed8fb21a132890b4e2fca2R1279

Comment by Mike Fikes [ 18/Dec/14 12:07 PM ]

Knowing this, it is trivial to reproduce this in desktop Safari (and also see that it works fine in Chrome and Firefox).

If you go to clojurescript.net, or himera.herokuapp.com, and define a function that returns a list constructed with (list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18), you can see that those websites are built using ClojureScript r2014 or earlier, as cljs.core.list.call(null, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18) appears in the reader literal for the returned function.

But, with either of those sites, if you evaluate the following, you will cause a severe performance problem in Safari:

(conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj '() 21) 20) 19) 18) 17) 16) 15) 14) 13) 12) 11) 10) 9) 8) 7) 6) 5) 4) 3) 2) 1) 0)

My hope is that knowing this will make it easier to profile (using desktop tools) what is giving Safari grief executing the resulting JavaScript.

Comment by Mike Fikes [ 18/Dec/14 2:38 PM ]

I don't understand JavaScriptCore's evaluation strategy. I found that if you manually revise the deeply nested composition of cljs.core._conj.call(...) invocations to extract a temporary var or two, as below, then the "doubling" effect is cut short, and the code executes quickly.

This revised code essentially builds a list of length 13 first, and then conses on 3 more elements, and 3 more.

var list0 = cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null,cljs.core.List.EMPTY, 18), 17), 16), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6);

var list1 = cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, list0, 5), 4), 3);

cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, list1, 2), 1), 0)

I found that I can cause something similar to occur from the ClojureScript side by adding a do form, as in:

(conj (conj (conj (conj (conj (do nil
(conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj '() 21) 20) 19) 18) 17) 16) 15) 14) 13) 12) 11) 10) 9) 8) 7) 6) 5)) 4) 3) 2) 1) 0)

Comment by Mike Fikes [ 18/Dec/14 7:00 PM ]

This JavaScriptCore perf problem is easily reproduced purely with JavaScript:

inc=function( x ) { return x + 1; }

alert(
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, 1))))))))))))))))))))))

Try this, at, say jsfiddle.net and you will see it max a core and use several GB in Safari, but not FireFox or Chrome.

Comment by Mike Fikes [ 19/Dec/14 9:00 AM ]

As indicated in this ticket's description, this problem doesn't occur with :advanced mode optimizations. Just to confirm, I produced the JavaScript with :pseudo-names set to true for (list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18), and you can see that it doesn't use the problematic "call" construct:

$cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$List$EMPTY$$, 18), 17), 16), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6), 5), 4), 3), 2), 1), 0);

Correction 20-DEC-2014: Even with :advanced mode optimization, this occurs. The setting that is needed to avoid the "call" construct is {:static-fns true}, as was set for the above output. With {:static-fnc false}, the emitted coded under :advanced mode is:

$cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$List$EMPTY$$, 18), 17), 17), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6), 5), 4), 3), 2), 1);
Comment by Mike Fikes [ 19/Dec/14 9:09 AM ]

Since the fundamental problem is easily reproducible with pure JavaScript, I've opened a StackOverflow regarding it: http://stackoverflow.com/questions/27568249/javascriptcore-deeply-composed-call-performance

Comment by David Nolen [ 19/Dec/14 12:08 PM ]

That was the first thing I was going to suggest trying, repo in plain JavaScript. Thanks for doing this - I believe this may explain some oddities we've encountered elsewhere.

Comment by Mike Fikes [ 19/Dec/14 1:41 PM ]

I’ve filed rdar://19310764 with Apple, and copied it here for reference: http://openradar.appspot.com/radar?id=5864025753649152

Comment by Francis Avila [ 19/Dec/14 5:38 PM ]

Excellent debugging work. Shouldn't this be filed against webkit instead? http://www.webkit.org/quality/reporting.html. (I already searched for this issue in their bug tracker and found nothing.)

This still leaves open the question of whether CLJS should do anything to mitigate. Many of the cljs macros that shadow functions expand recursively--maybe they should expand to loops or reduces instead?

For example:

(defmacro list
  ([] '(.-EMPTY cljs.core/List))
  ([& xs]
    `(let [a# (array ~@(reverse xs))]
       (areduce a# i# l# (list)
         (. l# cljs$core$ICollection$_conj$arity$2 (aget a# i#))))))

Or maybe cheat and emit a ChunkedCons instead?

(defmacro cclist
  ([] '(.-EMPTY cljs.core/LIST))
  ([& xs]
    `(cljs.core/ChunkedCons.
       (cljs.core/ArrayChunk.
         (array ~@(reverse xs)) 0 ~(count xs))
       nil nil nil)))
Comment by Mike Fikes [ 19/Dec/14 6:54 PM ]

Thanks Francis. I've confirmed that it occurs in the WebKit Nightly r177573, and I've moved the ticket to https://bugs.webkit.org/show_bug.cgi?id=139847

Comment by Thomas Heller [ 20/Dec/14 4:19 AM ]

FWIW to toggle between fn.call(null, ...) and fn(...) you can use the compiler option {:static-fns true}. This works with all optimization levels (also :none) but I'm told it causes issues with the REPL. But if you don't use a REPL it is safe to use that option always, I have been for years. Maybe the best short term option.

Comment by Mike Fikes [ 20/Dec/14 8:43 AM ]

Given Thomas's comment, I now realize that my comments above regarding :whitespace and :advanced are incorrect. (My project.clj has a dev and release build, and in addition to a change in the optimization directive, I had a change in the :static-fns directive.)

The correction: This problem occurs for both :whitespace and :advanced, iff :static-fns is set to false.

Comment by David Nolen [ 20/Dec/14 3:22 PM ]

One thing I'm curious about is whether the issue manifests if you're not nesting calls to the same function? This should be easy to confirm with plain JavaScript.

Comment by Mike Fikes [ 20/Dec/14 4:26 PM ]

Hey David, it appears to still cause a problem even with distinct functions. (This fiddle exhibits the problem with distinct functions, so don't follow the link on mobile Safari: http://jsfiddle.net/mfikes/Lwr78cmk/1/ )

Comment by Mike Fikes [ 21/Dec/14 4:16 PM ]

Activity is occurring with the WebKit ticket:

1) It has been confirmed as reproducible
2) It appears to be imported into Apple's system (InRadar keyword added).

Additionally, I succeeded in reproducing the issue on an older PowerPC Mac OS X 10.4.11 Safari 4.1.3 (from 2010), so it is not a recent regression, FWIW.

Comment by Mike Fikes [ 03/Jan/15 7:43 PM ]

Note: CLJS-945 sets :static-fns true as the default.

Comment by David Nolen [ 03/Jan/15 7:49 PM ]

Mike this is true only for cljs.core.

Comment by Mike Fikes [ 06/Jan/15 1:42 PM ]

FWIW, the original ticket I filed with Apple (rdar://19310764) has subsequently been closed by Apple as a duplicate of rdar://19321122, which is the Apple ticket the WebKit team opened in association with https://bugs.webkit.org/show_bug.cgi?id=139847





[CLJS-909] Add stable api for consumers of compiler data. Created: 15/Dec/14  Updated: 16/Dec/14  Resolved: 16/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS_909.patch    

 Description   

The ClojureScript compiler is under very active development. It would be nice for consumers of internal compiler data to have a stable api.



 Comments   
Comment by Bruce Hauman [ 15/Dec/14 2:50 PM ]

Here's the patch

Comment by David Nolen [ 16/Dec/14 12:10 PM ]

fixed https://github.com/clojure/clojurescript/commit/163f079407d1310755d326884b6965d9d74ed3c5





[CLJS-908] Duplicate goog.require emit when using :refer Created: 14/Dec/14  Updated: 16/Dec/14  Resolved: 16/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File duplicate-require-emit.diff    
Patch: Code

 Description   

In a CLJS (ns ...) a (:require [some-ns :refer (something)]) will end up with two goog.require("some_ns") statements in the generated .js. This also used to happen with (:require [clojure.string :as str]) but was fixed some time ago. The attached page should fix it for all cases.



 Comments   
Comment by David Nolen [ 16/Dec/14 12:22 PM ]

Thanks!

Fixed
https://github.com/clojure/clojurescript/commit/ec0023bd45a7f956b1e58aab84cb207a94e35965





[CLJS-907] False positives from arithmetic checks Created: 12/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/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   
WARNING: cljs.core/>=, all arguments must be numbers, got [#{nil clj-nil} number] instead. at line 59 file:/Users/kimmoko/.m2/repository/prismatic/dommy/1.0.0/dommy-1.0.0.jar!/dommy/core.cljs

Line 59 from core.cljs is the body of when-let:

        (when-let [i (utils/class-index class-name c)]
          (>= i 0))))))
(defn class-index
  "Finds the index of class in a space-delimited class-name
    only will be used when Element::classList doesn't exist"
  [class-name class]
  (loop [start-from 0]
    (let [i (.indexOf class-name class start-from)]
      (when (>= i 0)
        (if (class-match? class-name class i)
          i
          (recur (+ i (.-length class))))))))


 Comments   
Comment by David Nolen [ 12/Dec/14 10:06 AM ]

The problem is that the inferred type of class-index doesn't include number.

Comment by David Nolen [ 24/Dec/14 9:00 AM ]

fixed https://github.com/clojure/clojurescript/commit/576b85d83bf0eb0e232db3b2b8bddee0405f29d9





[CLJS-906] Naming conventions: use clojure.pprint instead of cljs.pprint Created: 12/Dec/14  Updated: 12/Dec/14  Resolved: 12/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I noticed a recent addition of cljs.pprint namespace to CLJS repo (which is great, thanks!).

I would like to point out minor naming issue. Could we avoid using cljs prefix when there is no significant difference between Clojure and ClojureScript version?

I am very sorry for my nitpicking. But these minor differences make unnecessary obstacles for maintaining code which targets both CLJ and CLJS.

Thank you, Dan



 Comments   
Comment by David Nolen [ 12/Dec/14 10:02 AM ]

Not possible due to the presence of an existing Clojure source file with the same namespace that provide macros.





[CLJS-905] Dependency tree fail Created: 11/Dec/14  Updated: 14/Dec/14  Resolved: 12/Dec/14

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

Type: Defect Priority: Major
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, compiler


 Description   

In the following repo

https://github.com/skrat/deporder-bug-cljs

Looking at the compiled source, I would expect the `bug.b` module to be initialized before `bug.a`. It is not, and it leads to a runtime error about `bug.b/registry` being undefined. Now, when I switch things around (`around` branch in the repo), rename `bug.a` to `bug.b` and vice versa, I will get a compiler warning about `bug.a/registry` being undeclared, but at runtime everything works as expected.

This example was extracted from a large project that uses similar macro that expands to `swap!`, where with `none` optimizations everything works, but with `simple` and `advanced` it throws during runtime.



 Comments   
Comment by Thomas Heller [ 11/Dec/14 6:38 AM ]

Technically not a "bug" but a weakness due to the nature ClojureScript handles macros.

'bug.macros requires 'bug.b cause it emits code calling it. 'bug.a requires 'bug.macro and therefore technically requires 'bug.b but since it cannot figure this out you technically have to tell it by also doing a (:require [bug.b]) in 'bug.a.

I had this problem a while back [1] and implemented a workarround by letting the macro declare what namespaces it requires, which in your case would look like

(def macro
  ^{:js-requires '[bug.b]}
  zwap [v]
  `(swap! bug.b/registry conj ~v))

But I was turned down so the feature never made it.

The recommended way is to have macros in namespaces that have a matching CLJS namespace and using :include-macros/:refer-macros when requiring them. Has its own issues cause a) everyone needs to know that bug.b uses macros and b) everyone needs to know what actually is a macro.

Anyways, in your case move bug/macros.clj to bug/b.clj and rewrite bug/a.cljs to

(ns bug.a
  (:require [bug.b :refer-macros (zwap)]))

[1] https://groups.google.com/forum/?fromgroups#!searchin/clojurescript/macro$20require/clojurescript/sLRGCIa8E1c/N9sFcTP_i9wJ

Comment by Thomas Heller [ 11/Dec/14 6:49 AM ]

Shameless plug: shadow-build [1] supports the macro meta feature. Still sort of hack-ish since it only works when macros are referred directly but my use of macros is very limited, basically only whats in [2].

[1] https://github.com/thheller/shadow-build
[2] https://github.com/thheller/shadow/blob/master/src/clj/shadow/macros.clj

Comment by Dusan Maliarik [ 11/Dec/14 8:33 AM ]

I wonder how this works in Clojure land, but I would still say it's a bug in ClojureScript compiler, because, afaik, it first expands macros, which results in a symbol from another namespace ('bug.b), and at that point, 'bug.b should be added as a dependency, resulting in goog.require('bug.b') in the compiled 'bug.a, isn't that right?

Comment by Francis Avila [ 11/Dec/14 9:18 AM ]

In java Clojure you still need to ensure that the symbols you expand in your macros are resolvable by the time the expanded form is executed. For example, if you create a macro that includes a clojure.data/diff call, you need to require clojure.data somewhere in your program, ideally in the namespace that defines the macro. Symbols which refer to other namespaces do not automatically require those namespaces. Most likely you will get a {{ClassNotFoundException [namespace-part]}} when you try to execute the form.

However Clojurescript introduces the extra problem that the macro expansion and execution environments are different. (In Clojure they are the same.) So there are additional constraints not present in Clojure:

1. A separate, completely different namespace dependency tree exists at compile time vs runtime.
2. Because of the Google Closure compiler, the entire runtime dependency tree must be known statically at compile time. (Otherwise some code you need at runtime might not be compiled into the final js.)
3. Because of javascript's dynamism, some symbols may not be known even to the Google Closure compiler. For example, if you have a macro which expands to calls to jQuery code, neither clojure, clojurescript, nor google closure, nor even your browser have a mechanism to "require" jQuery. The code must simply trust that you have done whatever is necessary to ensure jQuery/whatever is resolvable when it finally executes.

Additionally, automatically requiring namespaces from macro expansion is impossible (not a bug in the ClojureScript compiler):
1. The namespaces might not exist (i.e. the symbol is being used as a symbol, rather than as a reference or var).
2. The namespace/symbol might not be resolveble right now. This is clearer in the clojurescript case: is that symbol a reference to a clojure namespace that I should require right now, or is it a reference to a clojurescript namespace which I need to expand code to require, but I can't actually expect it to exist until runtime?
3. The macro might create symbols dynamically, in which case there is no way to know what namespaces the macro requires without executing the macro.

Comment by Dusan Maliarik [ 11/Dec/14 9:36 AM ]

Yes that's clear to me. Let's factor the non-ClojureScript deps like jQuery out, that's of course impossible to get right for everyone. What I propose is:

  1. run the macros
  2. walk through the expanded code
  3. when a symbol from another namespace is found, if this namespace isn't already required in that package, do either
    • add the require for that namespace
    • print out a warning

Currently it doesn't do anything, it just breaks during the runtime.

Comment by Thomas Heller [ 11/Dec/14 9:46 AM ]

I get a Warning?

WARNING: Use of undeclared Var bug.b/registry at line 8 src/bug/a.cljs

Comes down to a tooling problem again (hint: use shadow-build) that the warning is only printed once with lein-cljsbuild.

Comment by Dusan Maliarik [ 12/Dec/14 4:58 AM ]

Still, why does the different naming produce different results?

Comment by David Nolen [ 12/Dec/14 6:22 AM ]

This is not a bug.

Comment by Dusan Maliarik [ 12/Dec/14 7:14 AM ]

The behavior we talked about earlier might not be a bug indeed, but the fact that merely changing namespace names breaks things, sure is a bug. David, I advise to checkout the repo, try for yourself.

Comment by David Nolen [ 12/Dec/14 10:23 AM ]

I looked at the repo there is no compiler bug that I can discern. You just have two different invalid ClojureScript programs. All the salient points have been covered by other commenters.

Comment by Dusan Maliarik [ 12/Dec/14 11:31 AM ]

I won't insist anymore, nuff was said. Still the point about naming choice affecting the compiler output, wasn't addressed (compare 'master' and 'around' branches). "invalid ClojureScript programs" made me laugh though thanks

Comment by Thomas Heller [ 14/Dec/14 5:32 AM ]

Naming has very little to do with it. If you declare no dependency when using the macro (either bug.b or bug.a depending on branch) the analyzer cannot establish its "correct" position in the dependency graph. Therefore it is basically "luck" whether it will end up in the correct position or not. Declare the correct position and it will always be in the correct position.

Pretty sure the around branch will behave exaclty like master when you switch the order of the :require, but again: DO NOT RELY on such undefined behavior. Declare the dependencies!

(ns bug.core
  (:require [bug.b :refer [woot]]
            [bug.a :refer [registry]]
            ))




[CLJS-904] timeout in start-evaluator is too short Created: 10/Dec/14  Updated: 10/Dec/14

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

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


 Description   

The timeout specified in cljs.browser.repl/start-evaluator is too short.
https://github.com/clojure/clojurescript/blob/master/src/cljs/clojure/browser/repl.cljs#L87

Usually I have to update it from 50 to 500 to get my repl working any time I update to a new version. Would be nice to have this factored in as I don't suspect it has any noticeable effect on any other functionality.






[CLJS-903] Add volatile! (vswap! and vreset!) Created: 09/Dec/14  Updated: 12/Dec/14  Resolved: 12/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File ds--001--volatile.diff     File ds--002--volatile-transducers.diff     Text File transducer_atom_to_volatile.patch    
Patch: Code

 Description   

From JS perspective implementing volatile! in CLJS does not make much sense. It duplicates same functionality present in atom (and in fact CLJS statefull transducers use atom in place of volatile!).

However if you want to share statefull transducer function between CLJ and CLJS, you run into troubles and have to fall back to cljx workarounds.



 Comments   
Comment by David Nolen [ 09/Dec/14 12:53 PM ]

A patch is most welcome for this.

Comment by Peter Schuck [ 10/Dec/14 11:40 AM ]

The new volatile functions (volatile!, vswap!, and vreset!) are just aliases of their atom counterparts

Comment by Daniel Skarda [ 12/Dec/14 7:25 AM ]

Implementation of Volatile without alias to Atom.

Should be a little bit faster than Atoms (no watches, no validations, no meta handling etc).

Second patch updates transducer function to use volatile!

Comment by David Nolen [ 12/Dec/14 10:09 AM ]

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





[CLJS-902] Give defined type constructors names for debugging's sake Created: 03/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

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

Attachments: Text File 0001-CLJS-902-Give-ctors-names-for-debugging-purposes.patch    
Patch: Code

 Description   

I've wished more than once for the ability to use (.. x -constructor -name) during debugging to be able to tell what type an object has. The attached patch makes that work.



 Comments   
Comment by David Nolen [ 01/Jan/15 3:36 PM ]

pr-str already does the right thing for ClojureScript types





[CLJS-901] In memory based builds Created: 03/Dec/14  Updated: 08/Dec/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   

Currently builds are based one files on disk. It is desirable to be able to pass arbitrary ClojureScript programs as string or seq of forms and get the fully compiled source including dependencies as a string result.



 Comments   
Comment by Thomas Heller [ 03/Dec/14 9:31 AM ]

This and some other issues opened recently (CLJS-900, CLJS-851, CLJS-899, ...) have some overlap with what I have done in shadow-build [1]. Memory based builds are actually already possible cause it will only touch the disk when asked to, although the API could use some cleanup.

Anyways, might be worthwhile to coordinate these efforts to make CLJS more accessible for everyone.

[1] https://github.com/thheller/shadow-build





[CLJS-900] Parameterize caching strategy Created: 03/Dec/14  Updated: 03/Dec/14

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

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


 Description   

Currently the caching strategy is hard coded to a disk based one. It would be desirable in many situations for the caching to be in memory. We should decouple the caching strategy and support disk / memory out of the box.






[CLJS-899] AOT cache core.cljs analysis Created: 02/Dec/14  Updated: 24/Dec/14

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





[CLJS-898] Cache analysis results to disk Created: 02/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/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


 Comments   
Comment by David Nolen [ 24/Dec/14 8:41 AM ]

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





[CLJS-897] AOT core.cljs Created: 02/Dec/14  Updated: 24/Dec/14

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


 Comments   
Comment by David Nolen [ 08/Dec/14 4:22 PM ]

As pointed out by Colin Fleming it will be important to cache both optimized and unoptimized ClojureScript source - specifically the static-fns build option toggled.





[CLJS-896] Investigate AOTed ClojureScript JAR Created: 02/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

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


 Comments   
Comment by David Nolen [ 24/Dec/14 8:43 AM ]

Probably best if this is handled at the level of tooling.





[CLJS-895] Hello-js sample is broken when following README instructions Created: 02/Dec/14  Updated: 02/Dec/14

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

Type: Enhancement Priority: Trivial
Reporter: Andrew Rosa Assignee: Andrew Rosa
Resolution: Unresolved Votes: 0
Labels: documentation

Attachments: Text File 0001-Create-sample-to-demonstrate-only-extern-usage.patch    

 Description   

Currently we have on hello-js the usage of externes libraries. But it not only mix different aspects of cljsc as also have some breakage if we follow the README instructions

So, instead of adding more complexity into it, here I suggest us to extract the extern functionality into it's own sample project, which is much more approachable for newcomers that want to grok cljsc usage. This new sample project is included in my first patch, so anyone could see the end result.

If this patch is desirable, I could move around the other samples and split them into separate, focused aspect (simple hello, calling cljs from js, fixing the dom sample). Only the twitter buzz that I'm not sure how to approach it - I guess it has issues with API limitations of the last Twitter updates.






[CLJS-894] Hot reloading compiled CLJS code into the browser breaks source maps. Created: 02/Dec/14  Updated: 02/Dec/14  Resolved: 02/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Chrome, FF


Attachments: Text File fix_source_map_reloading.patch    
Patch: Code

 Description   

When one dynamically reloads code into a running application in the browser (Chrome). The browser doesn't reload the source map.

This can be fixed by appending a unique id to the source map epilogue which can be found at the bottom of a compiled file.

This forces the browser to reload the source map, however ... it doesn't fetch the new cljs source identified in the source map.

So a fix for that is to add yet another unique id to the paths in the source map.

The result is that hot loaded code has correct source and mapping in the browser (Chrome, FF).

This is what started the issue:
https://github.com/bhauman/lein-figwheel/issues/51

Here is a twitter conversation about it.
https://twitter.com/jlongster/status/539868749739094016

This is a fairly non invasive solution. It really has very little impact on devs who are not hot reloading.
Well actually it prevents the browser from accidentally caching source.

While I would prefer the browser vendors not cache in this situation, this patch significantly improves the coding experience.



 Comments   
Comment by David Nolen [ 02/Dec/14 3:44 PM ]

fixed https://github.com/clojure/clojurescript/commit/254e54876dfeae8c50d885010e730cdeaef26c99

Comment by Glen Mailer [ 02/Dec/14 3:53 PM ]

I'm not very familiar with these code paths, will this lead to non-deterministic builds?





[CLJS-893] Nodejs target shouldn't insert shebang by default Created: 02/Dec/14  Updated: 02/Dec/14  Resolved: 02/Dec/14

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

Type: Defect Priority: Major
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug, compiler


 Description   

If people want, they will prepend the shebang themselves, or run the file using node, where-ever it is on PATH. This is not good default behavior, and breaks things when the intention is to compile a node.js module.



 Comments   
Comment by David Nolen [ 02/Dec/14 12:52 PM ]

Whether it was a good or bad default is water under the bridge at this point. Do you have any suggested solutions? :node-module comes to mind for a new target that doesn't append the shebang. Another option would be something to explicitly suppress it.

Comment by Dusan Maliarik [ 02/Dec/14 1:20 PM ]

Sorry, my fault
https://github.com/clojure/clojurescript/commit/00a9b7bde0c8823175560b453a00e7be09ddd250

I just have to :hashbang false. Unfortunately I was unable to find a complete list of supported options to the compiler. Is it documented somewhere?

Comment by David Nolen [ 02/Dec/14 1:29 PM ]

It is not but the wiki is publicly editable - nothing is stopping anyone from creating a page that documents all the knobs and their relationships.





[CLJS-892] Improve performance of compare-symbols/compare-keywords Created: 29/Nov/14  Updated: 12/Dec/14  Resolved: 12/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs_892.patch    
Patch: Code

 Description   

It appears to me that compare-symbols function is in a good position. It knows a and b are not nil (nils are checked in compare), it knows a and b are both symbols (again, checked in compare) and it knows that .-ns and .-name fields are either nil or Strings.

This is how this function is implemented in core.cljs now:

(defn- compare-symbols [a b]
  (cond
   (= a b) 0
   (and (not (.-ns a)) (.-ns b)) -1
   (.-ns a) (if-not (.-ns b)
              1
              (let [nsc (compare (.-ns a) (.-ns b))]
                (if (zero? nsc)
                  (compare (.-name a) (.-name b))
                  nsc)))
   :default (compare (.-name a) (.-name b))))

Given prerequisites above, we can replace quite expensive (= a b) call (one nil check, identical? check, -equiv call, protocol dispatch, (instance? Symbol) check) with (identical? (.-str a) (.-str b)).

Also all calls to compare here happen in a controlled environment. Their arguments are not nil, and they are Strings. Call to compare is quite expensive too (identical? check, two nil checks, two type calls, comparison of types). All three calls to compare can be replaced with direct garray/defaultCompare call.

From my measurments, each change gives ×2 performance boost in comparison speed, total speedup ~4 times.

I also created compare-keywords function. They used to share implementation with compare-symbols before, but they differ in property .-fqn/.-str which is now used.

Patch attached: cljs_892.patch



 Comments   
Comment by David Nolen [ 02/Dec/14 4:52 AM ]

Looks useful. Mind adding a case to the benchmarks to the repo so I can verify behavior under the various engines on my machine? jsperf link would also be acceptable. Thanks!

Comment by Nikita Prokopov [ 11/Dec/14 4:22 PM ]

Updated version with benchmark code

Comment by Nikita Prokopov [ 11/Dec/14 4:24 PM ]

David, I uploaded new patch version with updated benchmark_runner.cljs, adding simple test for keywords comparison, will it do? Please check it out.

Comment by David Nolen [ 12/Dec/14 10:17 AM ]

Excellent performance enhancement, thanks! https://github.com/clojure/clojurescript/commit/f40a9cf49e8cd3ad016514df5d9aae9df995c801





[CLJS-891] Defs in "parent" namespaces clash with "child" namespaces with the same name? Created: 28/Nov/14  Updated: 02/Dec/14

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

Type: Defect Priority: Major
Reporter: Russell Dunphy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, namespace
Environment:

Clojurescript 0.0-2371, OSX



 Description   

This has had me totally flummoxed for a few hours, but I have finally been able to create a minimal project replicating the problem based on the mies template: https://github.com/rsslldnphy/cljs-ns-issue.

The problem seems to happen when a "parent" namespace, for example `my-project.foo`, contains a def with the same name as a "child" namespace, eg if there is a namespace `my-project.foo.bar`, and a def named `bar` in `my-project.foo`, and both those namespaces are required by `my-project.core`, then calling functions in `my-project.foo.bar` ends up with an Uncaught TypeError: Cannot read property 'call' of undefined. Sometimes, depending on which ns requires which, I've also seen an Uncaught Error: Namespace "cljs_ns_issue.foo.bar" already declared.

I don't think I'm doing a particularly good job of explaining this so it might be easier to look at the code. The crux is: comment out this line and the code works, leave it in and you get an error.



 Comments   
Comment by Francis Avila [ 28/Nov/14 2:01 PM ]

Clojurescript implements namespaces with Google Closure compiler's require/provide system. Unfortunately that system does not have a hard distinction between names and namespaces like Clojure does but instead is more like a sloppy java classname. The crux of it is that vars and namespaces occupy the same tree of js objects and thus their names may not overlap.

Compare the cljs on the left with the emitted javascript (This isn't exactly what is happening, but only in essence):

(ns my-project.foo ; goog.provide('my_project.foo') // something like window.my_project = {foo: {}}
  (:require my-project.foo.bar)) //goog.require('my_project.foo.bar')

;; the "require" evaluates the other namespace, which sets
;; // window.my_project.foo = {bar:{}};
;; // my_project.foo.bar.baz = "some var in the bar ns";
;; Now window.my_project.foo.bar = {baz: "some var in the bar ns"};

(defn bar [] 1)         ; my_project.foo.bar = (function bar(){return 1;});
;; Now the js object that was the bar namespace is gone, replaced with this function.

(my-project.foo.bar/baz)
; my_project.foo.bar.baz.call() // Uncaught TypeError: Cannot read property 'call' of undefined.

;; Alternatively, if (ns my-project.foo.bar) got evaluated *after* my-project.foo namespace was
;; evaluated, then my-project.foo/bar is defined, and the emitted goog.provide('my-project.foo.bar')
;; throws "Uncaught Error: Namespace "my_project.foo.bar" already declared".

So basically this is a leaky abstraction. In Clojurescript, you cannot define a var whose ns-qualified name matches that of another namespace: the slash between name and namespace is not real.

I think the only possible things that can be done are either:

  • Warn at compile-time if a var and a namespace object-path clash. Obviously there may still be runtime-created vars.
  • Put namespace vars behind some magic name. E.g. (ns foo.bar)(def baz 1) becomes goog.provide('foo.bar.__NS__'); foo.bar.__NS__.baz = 1; This would significantly uglify cljs names exported to js (and break existing code), and the magic name could never be used as a var name.
Comment by David Nolen [ 02/Dec/14 6:07 AM ]

We actually already have some logic for this in place, we track namespace segments for this reason. This is a different manifestation of it than previously encountered. I think any further workarounds are likely more trouble than they are worth (debugging complications) - probably the best thing to do is report a warning if we detect a clash.





[CLJS-890] Incorrect behaviour of (str obj) when obj has valueOf method Created: 24/Nov/14  Updated: 20/Jan/15

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

Type: Defect Priority: Minor
Reporter: Nikita Beloglazov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File cljs-890.patch     File cljs-core-str-perf.diff    

 Description   

Example

(str #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) ; => "42"

The problem in the fact that ClojureScript uses concatenation to convert values to strings and that doesn't work well with objects which have valueOf() method overriden.

Example in js:

var obj = {
    toString: function() { return 'hello'; },
    valueOf: function() { return 42; }
};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'

Potential solution might be to use String() function. Using toString() won't work as described in this issue: http://dev.clojure.org/jira/browse/CLJS-847



 Comments   
Comment by Kevin Neaton [ 24/Nov/14 10:34 AM ]

Is there a valid use case where toString and valueOf are not in sync? E.g.

(not= (.toString x) (js/String (.valueOf x))

If not, is it "incorrect" for the two methods to be out of sync?

Comment by Nikita Beloglazov [ 24/Nov/14 10:40 AM ]

Here is an example of such use case: https://github.com/processing-js/processing-js/blob/master/src/Objects/Char.js
That's how I found this bug.

Comment by Kevin Neaton [ 24/Nov/14 10:49 AM ]

Thanks for the link. I see what you mean.

Comment by David Nolen [ 02/Dec/14 5:08 AM ]

The problem with going with String appears to be a massive performance hit to printing http://jsperf.com/string-vs-tostring2/6.

Unless a brilliant idea is proposed this seems best solved / worked around in user code.

Comment by Nikita Beloglazov [ 02/Dec/14 6:41 AM ]

Append performs better on strings and numbers, but it performs worse on objects so it is not a clear performance hit. If I heavily work with objects and use (str) to convert them into strings then I actually lose on performance with current implementation.
Anyway current implementation of str is incorrect as it doesn't honor toString method. And this is what str function supposed to do. I believe a compiler should be correct first and then worry about performance.

Comment by David Nolen [ 02/Dec/14 7:38 AM ]

Sorry going back over this I believe the issue is really that we simply need to backout CLJS-801.

Comment by David Nolen [ 02/Dec/14 7:41 AM ]

reverted CLJS-801 in master

Comment by Francis Avila [ 02/Dec/14 10:32 AM ]

CLJS-801 only deals with the str macro. Aren't we still going to have str function problem because of CLJS-847? https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 (also Yet if we use toString there, Safari 6.0.5 blows up. Maybe we need {{[o].join('')}}? Depending on where the bug is this may be wrong in Safari 6.0.5 too.

What we need to do very specifically is somehow get the return value of the (in ECMASCRIPT-ese) ToString abstract operation on the object (or the underlying ToPrimitive abstract operation with the String hint). String concat with the add operator

Options as I see it are:

  • x.toString() : Bad because of CLJS-847
  • {{[x].join('')}} : Should work (and does right thing for null/undefined), but I think we should test in Safari 6.0.5. Also very slow.
  • String
  • String.prototype.concat
  • String.prototype.slice(x,0) String.prototype.substring(x,0) String.prototype.substr(x, 0)
  • x.toString() normally, but String if we detect that we'll trigger CLJS-847. (Can specialize on startup.)
Comment by David Nolen [ 02/Dec/14 10:35 AM ]

Is there any evidence that higher usage of str is actually problematic?

Comment by Francis Avila [ 02/Dec/14 10:44 AM ]

String concat using the addition operator uses an un-hinted ToPrimitive abstract call (which will try x.valueOf() first then x.toString(), usually) and then {{ToString}}s the result of that, so it's not an option unless we are concating primitive values.

Details:

Comment by David Nolen [ 02/Dec/14 10:50 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

Comment by Francis Avila [ 02/Dec/14 11:01 AM ]

Is there any evidence that higher usage of str is actually problematic?

Kevin Neaton, who opened CLJS-847, was using a patch in production which only addressed the higher order case and he said the patch fixed the issue for them. He was unaffected by the str macro case because it either used ''+x already (with CLJS-801 applied) or it used {{[x].join('')}} (which hasn't been tested with Safari 6.0.5 yet, but probably works).

So if we had a problem using ''+x with the str macro, we will certainly have a problem with ''+x with a string function as long as CLJS-847 is applied.

I haven't pulled down master yet, but here is a test case which I bet will fail with the CLJS-847 patch:

(def tricky-obj #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)})
(assert (= (apply str tricky-obj) "hello")) ;; will get "42"
Comment by Francis Avila [ 02/Dec/14 11:09 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

To be clear, there are two issues here:

CLJS-847: x.toString() fails on Safari 6.0.5. Workaround is ''+x (only done in str macro case).
CLJS-890: ''+x doesn't give expected results for objects which define valueOf. Expectation is that x.toString() is called, instead x.valueOf().toString(). Fix is to use array join instead of string concat in str macro, but it doesn't address the ''+x workaround from CLJS-847.

To make matters worse, it looks like the toString() error on Safari may only be triggered at certain JIT levels!

Comment by Francis Avila [ 02/Dec/14 11:10 AM ]

Workaround is ''+x (only done in str macro case).

I mean "Workaround is ''+x (only done in str function case)."

Comment by Nikita Beloglazov [ 08/Dec/14 6:14 PM ]

Can this bug be reopened meanwhile? If I understand correctly the fix should affect https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 but this code still present in HEAD.

Comment by David Nolen [ 08/Dec/14 6:37 PM ]

We've switched to goog.string.buildString in master https://github.com/clojure/clojurescript/commit/94eb8a960fef6aaca4ba44b251cefbfa04d0f6ac

Comment by Nikita Beloglazov [ 08/Dec/14 8:32 PM ]

Yes, that works. Cool, thanks!

Comment by Thomas Heller [ 01/Jan/15 7:12 AM ]

Sorry for re-opening.

I was doing some profiling of my code and noticed a warning in the profiling output about cljs.core/str.

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

goog.string.buildString = function(var_args) {
  return Array.prototype.join.call(arguments, '');
};

Given that we don't ever call it with more than one argument it is probably not best implementation choice.

Maybe skip the call and inline it ala

(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x] (if (nil? x)
         ""
         (.join #js [x] "")))
  ([x & ys]
    (loop [sb (StringBuffer. (str x)) more ys]
      (if more
        (recur (. sb  (append (str (first more)))) (next more))
        (.toString sb)))))

I didn't follow this issue but why are we not using .toString? The buildString/array approach seems kind of hackish?

I'm not too sure about the overall impact but since cljs.core/str showed up pretty high in my profile I think this should be investigated further.

Comment by Thomas Heller [ 01/Jan/15 7:28 AM ]

Before:

;;; str
[], (str "1"), 1000000 runs, 254 msecs
[], (str 1), 1000000 runs, 266 msecs
[], (str nil), 1000000 runs, 80 msecs
[], (str "1" "2" "3"), 1000000 runs, 753 msecs

After:

;;; str
[], (str "1"), 1000000 runs, 82 msecs
[], (str 1), 1000000 runs, 86 msecs
[], (str nil), 1000000 runs, 79 msecs
[], (str "1" "2" "3"), 1000000 runs, 242 msecs

But I only tested V8, probably needs some verification.

Comment by Thomas Heller [ 01/Jan/15 7:39 AM ]
(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x1]
     (.join #js [x1] ""))
  ([x1 x2]
     (.join #js [x1 x2] ""))
  ([x1 x2 x3]
     (.join #js [x1 x2 x3] ""))
  ([x1 x2 x3 x4]
     (.join #js [x1 x2 x3 x4] ""))
  ...)

Does perform even better.

;;; str
[], (str "1"), 1000000 runs, 40 msecs
[], (str 1), 1000000 runs, 43 msecs
[], (str nil), 1000000 runs, 96 msecs
[], (str "1" "2" "3"), 1000000 runs, 117 msecs

How many args should it inline?

Comment by David Nolen [ 01/Jan/15 12:43 PM ]

I'd be OK with up to 4 then variadic.

Comment by Thomas Heller [ 01/Jan/15 5:05 PM ]

There is some weird interaction between the code generated by the cljs.core/str macro and function.

The macro generates

(str "hello" 1 "world" :yo nil)

yields

[cljs.core.str("hello"),cljs.core.str((1)),cljs.core.str("world"),cljs.core.str(new cljs.core.Keyword(null,"yo","yo",1207083126)),cljs.core.str(null)].join('');

Given that str with 1 arg will basically unroll to

[["hello"].join(""), ...]

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

Comment by Francis Avila [ 02/Jan/15 11:14 AM ]

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

Chrome complains about the variadic function dispatch code in the same way, see CLJS-916 plus patch.

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

The Closure compiler is not smart enough to remove the intermediate array, which is why I filed CLJS-801 (which this ticket rolled back). I don't think JITs can do it either.

I am beginning to wonder if we should ignore the Safari 6.0.5 problem in CLJS-847 that started all this string mess. To recap:

  1. CLJS-801 is accepted, which removes [123, x].join('') in the str macro case in favor of ''+123+(cljs.core/str$arity$1 x) style code, which the closure compiler can precompute. At this time, the one-arg cljs.core/str function (not macro) calls toString on its argument.
  2. CLJS-847 is filed. On Safari 6.0.5 at higher JIT levels, calling toString on some things (possibly only unboxed numbers? definitely not strings) throws a TypeError. This is unquestionably a bug in Safari. David fixes by making one-arg cljs.core/str function call js-str instead of toString. js-str uses string-concat ''+x.
  3. However, this breaks for objects that define valueOf (issue in current ticket), because in js ''+x is the same as ''+x.valueOf().toString() not ''+x.toString().
  4. David considers using String() and variations but rejects because of performance hit.
  5. David rolls back CLJS-801 from the string-concat back to the array-join style to fix.
  6. Nikita and I point out that rolling back CLJS-801 only fixes the str macro, not the string function, which still uses js-str and hence string-concat.
  7. David fixes the str function to use goog.string.buildString, which has the behavior of array.join. Behavior is now correct even on Safari 6.0.5.
  8. Thomas points out that buildString uses arguments in a way unoptimizable by v8, and now the str function (not macro) has a performance regression. He suggests using [].join() directly.

So, there's a lot of back and forth on this issue, and it's all because of a bug in Safari 6.0.5 which no one has been able to reproduce first-hand because Safari 6.0.5 is old and rare. For some perspective, Safari 6.0.x was only available on Lion and Mountain Lion between July 25,2012 and June 11,2013. Before July 25,2012 Lion used Safari 5.1.x and there was no Mountain Lion. On June 11, 2013, both Lion and Mountain Lion switched to Safari 6.1.x which does not suffer from the toString TypeError bug (I checked--I have an iMac with Lion on it). The only machines on Safari 6.0.5 are (Mountain) Lion machines which used software updates until late 2012-early 2013 and then stopped. I can't imagine this is a large number of people.

It is theoretically possible for me to run Safari 6.0.x on my Lion machine to actually test this, but I can't find a way to downgrade from 6.1.x.

I think the options are:

  1. Use array.join() for all stringification and take the performance hit (which we should quantify). Include a comment that this is only for Safari 6.0.x (only confirmed second-hand on 6.0.4 and 6.0.5) for future generations, who are going to think this is weird.
  2. Use CLJS-801 and toString (status quo before CLJS-847), and ignore this problem for Safari 6.0.x.
  3. Use CLJS-801, but add a number? check (with comment) to cljs.core/str$arity$1 for Safari 6.0.5. The number case should use js-str, and the rest toString. I think this will work, but again we have no way to test--we really need to get our hands on a Safari 6.0.x browser.

Of course we should benchmark these approaches but my hunch is that 2 is faster than 3 is faster than 1.

Comment by David Nolen [ 02/Jan/15 11:16 AM ]

We are not going to ignore Safari 6.0.X. Any decisions made about this ticket will include supporting it.

Comment by Francis Avila [ 10/Jan/15 4:12 AM ]

Update on some research I am doing into this.

I created a jsperf of alternative str implementations that I am trying out. Right now I've only looked at one-arg str. I discovered a few things:

  • {{''+[x]}} is a faster alternative to [x].join('').
  • Advanced compilation can compute {{''+[x]}} at compile time if x is a bool, str, undefined, null, or number, even through function calls! I.e. str_test.str_arr(123) compiles to "123" without macro magic.
  • However, using an intermediate array (even if a preallocated singleton) is still slower than the old (if (nil? x) "" (.toString x))
  • Using a switch statement is as least as fast as the str-tostr baseline, usually faster.
  • I am 99% sure all these implementations (except str-tostr, the baseline, which definitely fails) work on the dreaded Safari 6.0.x. If anyone has this version, point it at the jsperf link above and run the tests. I think Browserstack has this version of Safari.

I'm still investigating the variadic case (str x y z a b c). It might be better to use reduce instead of Stringbuffer+seq. (Stringbuffer just does ''+x now instead of an array-join.)

Comment by Thomas Heller [ 10/Jan/15 6:37 AM ]

Sorry, got side-tracked for a bit.

@Francis: Thanks for the recap.

Don't have Safari 6 available either, but seems wrong that we all have to suffer because is minor percentage still has this (667 users of 190k+ on my site). Don't have a solution cause I can't test whether it works, we might try String.concat.

"".concat(obj); // "42"
"".concat(obj, ""); // "hello"
String.prototype.concat(obj, "") // "hello"
String.prototype.concat("", obj) // "hello"

But no idea if String.concat works, also it behaves odd with regards to valueOf.

http://jsperf.com/js-string-concat-variants

Perf is also inconclusive since Firefox appears to be cheating.

Comment by Francis Avila [ 10/Jan/15 2:04 PM ]

Tested that jsperf with Safari 6.0.5 using Browserstack, results are there.

Note I could not reproduce CLJS-847 because str-tostr does not fail as expected. I will try harder now that I have a browser to test.

Comment by Francis Avila [ 10/Jan/15 6:55 PM ]

Still cannot reproduce CLJS-847.

This script includes my attempt at a minimum reproducible case. My theory was that certain types at higher jit levels would fail. I could not get any case to fail. I also tried flapping back and forth between types and using one type at a time, but still no failures.

In this thread I found this "minimal" script which the OP said he could get to fail reliably. I could not get it to fail. However the original post was from feb 15, 2013, which means the Safari he was using would have to be 6.0.2 or lower.

Hypotheses:

  1. This error does not affect 6.0.5 but maybe 6.0.4 or lower.
  2. BrowserStack's system somehow mitigates the bug, meaning we need a "real" Lion Safari 6.0.x to test.
  3. These tests only fail under the correct phase of the moon.

So I can code up a patch for str using the str-switch implementation (which is at least a bit faster on some browsers), but I have no idea if it may fail on Safari 6.0.5. I only know that it works so far. CLJS-801 should also be safe to reapply because the root cause of all issues is the implementation 1-arity of the cljs.core/str function.

I have also asked for Kevin's help back in CLJS-847. (Kevin was the original reporter of the Safari 6.0.x issue.)

Comment by Francis Avila [ 19/Jan/15 12:51 AM ]

Made a jsperf of variadic cases. Chrome seems to really prefer IReduce to seq+stringbuilder for vectors (other collections not tested), but there is no difference or a small slowdown on other browsers. Not sure if it's worth it.

Also updated arity-one cases with a str using switch and never using toString. Nearly 50% slower than using switch or toString on Chrome, smaller on Safari.

In terms of safety str-switch-notostr does not use toString at all so is probably safer. I think str-switch will likely work too, though, and is significantly faster. However I haven't been able to get any TypeErrors in Safari 6.0.5 so it's anyone's guess.

I suggest something like this as a new str (which doesn't use reduce, but could):

(defn str
 ([x]
  (case (js* "typeof ~{}" x)
   "string" x
   "object" (if (identical? nil x) "" (.toString x))
   ("boolean" "number") (js-str x)
   "undefined" ""
   (js-str #js [x])))                                       ;; insurance against Safari 6.0.x TypeError bug.
 ([a b] (js* "~{}+~{}" (str a) (str b)))
 ([a b c] (js* "~{}+~{}+~{}" (str a) (str b) (str c)))
 ([a b c d] (js* "~{}+~{}+~{}+~{}" (str a) (str b) (str c) (str d)))
 ([a b c d & more]
  (loop [s (str a b c d) [e f g h & r] more]
   (let [s' (js* "~{}+~{}+~{}+~{}+~{}" s e f g h)]
    (if (nil? r)
     s'
     (recur s' r))))))
Comment by Francis Avila [ 19/Jan/15 11:24 PM ]

First cut of a possible patch that resolves this while not breaking CLJS-847. Should wait for confirmation that this does not break Safari 6.0.x.

Comment by Francis Avila [ 19/Jan/15 11:34 PM ]

Oops forgot tests.





[CLJS-889] 2 characters not supported in re-pattern: \u2028 \u2029 Created: 18/Nov/14  Updated: 26/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Dave Sann Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

[org.clojure/clojurescript "0.0-2371"]
ubuntu


Attachments: Text File 0001-re-pattern-works-on-strings-containing-u2028-or-u202.patch     Text File 889.patch    

 Description   

The following 2 patterns

(re-pattern "[\u2028]")
(re-pattern "[\u2029]")

Cause:

SyntaxError: Invalid regular expression: missing terminating ] for character class

They are probably somewhat rare characters in practice.

http://www.fileformat.info/info/unicode/char/2028/index.htm
http://www.fileformat.info/info/unicode/char/2029/index.htm



 Comments   
Comment by Alex Dowad [ 18/Dec/14 1:51 PM ]

The problem is that re-pattern uses a (.*) regexp to pick out the "pattern" portion of a regexp string (separate from the "flags" portion), and "." doesn't match \u2028 and \u2029.

Comment by Alex Dowad [ 18/Dec/14 2:15 PM ]

Sorry, JIRA's helpful formatting mangled that patch. Here it is again.

Comment by Alex Dowad [ 26/Dec/14 1:31 AM ]

Here is a fixed version of the patch, tested on V8, Nashorn, and Spidermonkey. It includes a regression test.





[CLJS-888] Make better use of newlines in emitted javascript Created: 17/Nov/14  Updated: 20/Nov/14  Resolved: 20/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-888-Better-placement-of-newlines-in-emitter.patch     Text File 0002-CLJS-888-Omit-redundant-around-emitted-recur.patch    

 Description   

Problem Statement

Javascript debuggers are pretty line-oriented and so it's often very difficult to follow control flow without source maps. Even with source maps it's often not feasible to properly work with breakpoints in emitted Javascript.

Proposal

Emit more newlines and do so in places that would make sense when hand-coding javascript. The attached patch came from experience in debugging on IE and makes the emitter break up lines pretty neatly, with the exception of deeply nested literals.



 Comments   
Comment by Herwig Hochleitner [ 17/Nov/14 4:11 PM ]

Patch 0002 is not strictly in scope of the ticket title, but fits within an emitter cleanup, so I think it could be reviewed as part of this ticket.

Comment by David Nolen [ 20/Nov/14 11:24 AM ]

fixed
https://github.com/clojure/clojurescript/commit/124998c05dea844e892cd28b40f89eecdd442e1a
https://github.com/clojure/clojurescript/commit/bf2d2413dcb46b2cec9a00e37af407006634c804





[CLJS-887] cljs.repl.browser does not serve css by default Created: 17/Nov/14  Updated: 10/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Christopher Shea Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl
Environment:

*


Attachments: File cljs-browser-repl-serve-css.diff    
Patch: Code

 Description   

A browser repl's server serves js, cljs, map, and html files by default, but not css.



 Comments   
Comment by John Chijioke [ 10/Dec/14 2:05 AM ]

What is css needed for from the repl?

Comment by Christopher Shea [ 10/Dec/14 9:13 AM ]

cljs.repl.browser/send-static can already send css with the appropriate MIME type, so this just seems like a minor oversight.

If you follow the Using the browser as an Evaluation Environment section of the ClojureScript wiki and have a css file linked from your html page, it will not be served, which can make it more difficult to work on your project (you won't see the effects of changing an element's class, e.g.).

As a workaround, I've been doing this when setting up my repl:

(server/dispatch-on :get
  (fn [{:keys [path]} _ _] (.endsWith path ".css"))
  browser/send-static)

It's not that my interactions with the repl require the css, it's the browser that's connected to the repl that does.





[CLJS-886] Add a contains-key? function to the ITransientAssociative protocol Created: 14/Nov/14  Updated: 17/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Alejandro Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I propose to add a contains-key? function to the ITransientAssociative protocol equivalent to the function with the same name in IAssociative.



 Comments   
Comment by David Nolen [ 17/Nov/14 7:49 AM ]

This is a departure from the interface design in Clojure. I recommend starting a discussion on clojure-dev.





[CLJS-885] read-string result raising a warning Created: 11/Nov/14  Updated: 05/Dec/14  Resolved: 05/Dec/14

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

Type: Defect Priority: Major
Reporter: Stefano Pugnetti Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: Compiler, bug
Environment:

Ubuntu 12.04 (64 bit)
Oracle JDK 1.7.0_72
leiningen 2.5.0
ClojureScript 0.0-2371
lein-cljsbuild 1.0.4-SNAPSHOT



 Description   

The result of read-string is not correctly recognized as a number in the example discussed here:

https://groups.google.com/forum/#!topic/clojurescript/kwJNH2MBbao

Wrapping it in a call to the identity function makes the symptom disappear.



 Comments   
Comment by David Nolen [ 05/Dec/14 1:37 PM ]

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





[CLJS-884] with-redefs don't restores previous values when used inside go block Created: 09/Nov/14  Updated: 09/Nov/14

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

Type: Defect Priority: Trivial
Reporter: Vladimir Iakovlev Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

clojure 1.6, clojurescript 0.0-2371, core.async 0.1.346.0-17112a-alpha


Attachments: Text File cljs_884.patch    

 Description   

For example I have code and test:

(defn test-fn [] :original)

(deftest ^:async test-with-redefs-async
  (go (is (= (test-fn) :original))
      (with-redefs [test-fn (fn [] :mocked)]
        (is (= (test-fn) :mocked)))
      (is (= (test-fn) :original))
      (done)))

And test failed in last `is`:

FAIL in (test-with-redefs-async) (:)
expected: (= (test-fn) :original)
  actual: (not (= :mocked :original))


 Comments   
Comment by Vladimir Iakovlev [ 09/Nov/14 1:26 PM ]

I attached little patch with fix. With it `with-redefs` forced to use previous value for restoring bindings.

But I don't know how to add test for this issue, because it requires core.async. So I tested with:

(defn test-fn [] :original)

(deftest test-with-redefs-sync
  (is (= (test-fn) :original))
  (with-redefs [test-fn (fn [] :mocked)]
    (is (= (test-fn) :mocked)))
  (is (= (test-fn) :original)))

(deftest ^:async test-with-redefs-async
  (go (is (= (test-fn) :original))
      (with-redefs [test-fn (fn [] :mocked)]
        (is (= (test-fn) :mocked)))
      (is (= (test-fn) :original))
      (done)))




[CLJS-883] Add nthrest Created: 07/Nov/14  Updated: 13/Nov/14  Resolved: 13/Nov/14

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

Type: Enhancement Priority: Trivial
Reporter: Ben Moss Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File add_nthrest.patch    
Patch: Code

 Description   

Discovered last night at the Clojurescript meetup that nthrest is missing from CLJS! Found this https://groups.google.com/forum/#!topic/clojurescript/MZQUInUGCRc, but nothing seems to have been done.



 Comments   
Comment by David Nolen [ 13/Nov/14 9:16 AM ]

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





[CLJS-882] cljs.reader/read-string throws errors when reading keywords that begin with integers Created: 05/Nov/14  Updated: 02/Dec/14  Resolved: 02/Dec/14

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

Type: Defect Priority: Minor
Reporter: Joseph Fahey Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: keywords, reader
Environment:

Using clojurescript 0.0-2371. Error seen with both phantomjs and Chrome



 Description   

(cljs.reader/read-string ":1") throws an error: TypeError: 'null' is not an object (evaluating 'a[(0)]')
at read_keyword (:474)



 Comments   
Comment by Joseph Fahey [ 05/Nov/14 4:10 PM ]

read-keyword, in reader.cljs, matches against symbol-pattern, which disallows symbol names that begin with numbers. Symbols can't begin with numeric characters, but keywords actually begin with ":", so in a keyword like :1 , "1" is actually the second character.

Comment by Francis Avila [ 05/Nov/14 8:01 PM ]

Your reasoning that in a keyword the number is the second character is precisely one of the unclear points about keyword and symbol parsing. Some implementations say yes, and some do not, and there is no "spec" unambiguous enough to decide the issue.

This issue is a duplicate of CLJS-677. The comments in there go in to much greater detail.

Comment by Joseph Fahey [ 06/Nov/14 2:52 AM ]

I'll leave it at that then.

I would like to add that the current state of affairs is rather confusing, because keywords like :1 seem to work fine in clojurescript, except when deserializing with cljs.reader/read-string, from localStorage for example, which fails without a clear explanation.

Comment by Francis Avila [ 06/Nov/14 12:19 PM ]

Agreed, the current state of affairs is not good but the proper fix would be:

  1. Produce a rigorous formal specification of the reader syntax for Clojure (and variants/subsets for edn, Clojurescript, ClojureCLR). (Including consideration of unicode chars, etc.)
  2. Unifying all reader implementations around these specifications (across multiple projects).
  3. Dealing with code breakage in upstream libraries.

Understandably the core developers would probably think this is a very large effort with a lot of disruption for very little gain. I advise just avoiding edge cases in the spec, like :1, :supposedly:ok:keyword, non-ascii characters, etc, in both code and edn.

Comment by Joseph Fahey [ 07/Nov/14 4:15 AM ]

One last thing about the confusion this causes. The problem I see is that {:1 "one"} compiles without any problem in Clojurescript, (keyword? :1} returns true, and (keyword "1") returns :1. The only time the problem comes up is when using reader/read-string. It seems to me that this should be coherent at least within Clojurescript, even if there are discrepancies with the other implementations.

And when using :keywordize :true with externally supplied data, it is hard to be sure that some of the JSON keys won't begin with a digit. (This is how I stumbled onto this.)





[CLJS-881] array-map does not check for duplicate keys Created: 01/Nov/14  Updated: 05/Nov/14  Resolved: 05/Nov/14

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

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

Attachments: Text File CLJS-881-p1.patch    

 Description   

array-map has a macro version that works correctly, but the function version will happily create a map with duplicate keys:

(keys (array-map :foo 1 :foo 2)) ;; => (:foo)
(keys (apply array-map [:foo 1 :foo 2])) ;; => (:foo :foo)


 Comments   
Comment by Gary Fredericks [ 01/Nov/14 9:09 PM ]

Attached CLJS-881-p1.patch, which contains a test and switches array-map to use the .fromArray method the same way the macro does.

Comment by David Nolen [ 05/Nov/14 6:31 AM ]

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





[CLJS-880] With empty collections, specify! extends all instances, not just this one Created: 01/Nov/14  Updated: 13/Nov/14  Resolved: 13/Nov/14

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

Type: Defect Priority: Minor
Reporter: Marcus Lewis Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

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



 Description   

Some pretty surprising behavior:

my.ns> (defprotocol IFoo)
my.ns> (satisfies IFoo {})
false
my.ns> (specify! {} IFoo)
my.ns> (satisfies IFoo {})
true

The IFoo specification leaks onto essentially all other equal-valued instances. As a result, one of my functions was overwriting this specification for all existing instances with a single specify!.

Non-empty collections behave as expected:

my.ns> (defprotocol IFoo)
my.ns> (satisfies IFoo {:a "a"})
false
my.ns> (specify! {:a "a"} IFoo)
my.ns> (satisfies IFoo {:a "a"})
false



 Comments   
Comment by David Nolen [ 05/Nov/14 6:11 AM ]

I'm not sure that it's worth making any changes for this. We optimize empty literals and specify! is a lower level operation. specify is should be preferred in nearly every case - the use of specify! seem gratuitous in the above examples. Is there a more compelling argument?

Comment by Marcus Lewis [ 05/Nov/14 11:29 AM ]

Here's the code in question. On a side note, I've since abandoned this function, and use React mixins instead. https://github.com/mrcslws/saccade/blob/45d5ac2f2dd5396b7cf27339e4b436d1bf236e15/src/cljs/saccade/components/helpers.cljs#L11

I worked around the bug via (let [faker (reify)]). If "faker" was initialized to an empty collection, very confusing things happened.

I used specify! because in this case (implementing protocols case-by-case) it was much easier to read. specify is doable, but you'd have to reduce over a vector of functions, one function per protocol, each conditionally returning sofar or (specify sofar IFoo) based on (satisfies? IFoo actual). It's a fun intellectual exercise, but I couldn't find a good non-religious reason to do it.





[CLJS-879] Add function `update` from Clojure 1.7 Created: 31/Oct/14  Updated: 31/Oct/14  Resolved: 31/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 31/Oct/14 2:48 AM ]

fixed https://github.com/clojure/clojurescript/commit/15fbbf5d63fbc860e0fe4f7d45c7f00a27ebc0ba

Comment by Daniel Skarda [ 31/Oct/14 3:03 AM ]

David, thanks for being so quick!

You are faster than my `git format-patch` Are not you supposed to be on tour with Hairy Sands?

Thank you for fast fix.





[CLJS-878] Missing preamble file causes unhelpful stack trace Created: 30/Oct/14  Updated: 02/Dec/14  Resolved: 31/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: cljs, errormsgs


 Description   

I think it would be helpful to throw an exception with the missing file path rather than the current

java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil
.

Right now you need to work your way down about 10 lines into the stacktrace to find

cljs.closure$preamble_from_paths$fn__3097.invoke(closure.clj:526)
which is the only hint as to what has happened.



 Comments   
Comment by David Nolen [ 31/Oct/14 2:42 AM ]

This is a simple enhancement, a patch is welcome. Thanks.

Comment by David Nolen [ 31/Oct/14 2:44 AM ]

Actually this already appears fixed in master https://github.com/clojure/clojurescript/commit/9fd6bf5bd55421c3d5becacc5230ed661d6fb3c3

Comment by David Nolen [ 31/Oct/14 2:45 AM ]

See CLJS-869





[CLJS-877] to-string of js/Date objects is inconsistent Created: 30/Oct/14  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Defect Priority: Minor
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

0.0-2371, Chrome



 Description   

When converting a js/Date object to a string, different things happen depending on whether the object is nested in a data structure or not.

For example:

(def date (js/Date.))
(str date) ;; => "Thu Oct 30 2014 16:49:50 GMT-0400 (EDT)"
(str [date]) ;; => "[#inst \"2014-10-30T20:49:50.999-00:00\"]"


 Comments   
Comment by David Nolen [ 02/Dec/14 5:00 AM ]

The behavior is identical to java.util.Date in Clojure. Not saying it isn't a bug but a discussion should probably be started elsewhere first.

Comment by David Nolen [ 03/Jan/15 6:30 PM ]

Closing this for now, behavior mirrors Clojure





[CLJS-876] merged sourcemap doesn't account for output-wrapper Created: 25/Oct/14  Updated: 05/Nov/14

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

Type: Defect Priority: Major
Reporter: J. David Lowe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 876.alternative.patch     Text File 876.patch    

 Description   

When merging sourcemaps, the cljs.closure/optimize code correctly accounts for "preamble" lines that get prepended to closure compiler output. However, it fails to account for the extra lines added when :output-wrapper is true. As a result, the merged sourcemap is incorrect whenever :output-wrapper is true. (And for extra action-at-a-distance: :output-wrapper is implicitly set to true by lein-cljsbuild whenever :optimizations is :advanced.)

Too tired to make a patch tonight, but I'll go through the CA process and send a patch in a day or two, if nobody gets to it before then.



 Comments   
Comment by J. David Lowe [ 27/Oct/14 2:42 PM ]

Here are two patches, which both fix the problem.

876.patch is tiny, but I think it's probably a step in the wrong direction, because all future output prefixes will need the same fix applied to them.

876.alternative.patch is a deeper change, but I think is a better solution: it makes it much more apparent that all output prefixes should be added via make-preamble. The downside (IMO) is that the output-wrapper prefix and suffix are now separated a bit in the code.

Comment by David Nolen [ 05/Nov/14 6:46 AM ]

This patch approach looks OK but something more minimal is preferred to simplify patch reviewing. Please submit a patch which makes the most minimal set of code changes (it's preferred not to move code) use declare if necessary. Thanks!





[CLJS-875] Compiler error on :" Created: 22/Oct/14  Updated: 05/Nov/14  Resolved: 05/Nov/14

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

Type: Defect Priority: Major
Reporter: Stuart Mitchell Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: compiler, errormsgs
Environment:

Ubuntu 14.04
org.clojure/clojurescript "0.0-2277"



 Description   

(str "this will break" :"the parser")
This simple form (note the misplaced ':' causes the compiler to barf with the following error
Caused by: clojure.lang.ExceptionInfo: EOF while reading string

no mention of the ':' and in larger blocks of code the unmatched brackets several lines away is flagged

in clojure the error is
RuntimeException Invalid token: : clojure.lang.Util.runtimeException (Util.java:221)
RuntimeException Unmatched delimiter: ) clojure.lang.Util.runtimeException (Util.java:221)

Note it mentions the errant ':'



 Comments   
Comment by Stuart Mitchell [ 22/Oct/14 11:04 PM ]

I would like the error reporting from the compiler improved in this case, so that it mentions that the problem is caused by the ':'.

Comment by Nicola Mometto [ 23/Oct/14 7:53 AM ]

This appears to be an issue caused by tools.reader, I'm working on a fix for it

Comment by Nicola Mometto [ 23/Oct/14 9:17 AM ]

I just pushed a 0.8.10 release of tools.reader that fixes this issue

Comment by David Nolen [ 05/Nov/14 6:33 AM ]

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





[CLJS-874] Add closure/path-relative-to support to relative paths based on directories Created: 19/Oct/14  Updated: 24/Jan/15  Resolved: 24/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Andrew Rosa Assignee: Andrew Rosa
Resolution: Declined Votes: 0
Labels: None

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

 Description   

The referred function could not handle directories as base paths, since it only calculate the relative paths via string manipulation. I propose using some checking on file system (through {File#isDirectory()}) to be able to differentiate directories from files.

The current behavior:

(path-relative-to (io/file ".") {:url (deps/to-url (io/file "cljs/core.js"))})
;; => "core.js"

After the patch we will have:

(path-relative-to (io/file ".") {:url (deps/to-url (io/file "cljs/core.js"))})
;; => "cljs/core.js"

This behavior is needed for my patch for CLJS-851. If there is some better alternative to address that, I'm open to make the appropriate changes.



 Comments   
Comment by David Nolen [ 02/Dec/14 5:10 AM ]

Can we get a rebased patch? Thanks.

Comment by Andrew Rosa [ 02/Dec/14 5:24 AM ]

Of course I can provide it @David. But the current CLJS-851 implementation this behavior is not needed anymore. Do you still want this patch or prefer to wait until CLJS-851 resolution?

Comment by David Nolen [ 24/Jan/15 12:20 PM ]

No longer relevant





[CLJS-873] Non-higher-order calls to cljs.core/array-map sometimes return PHMs, should always return PAMs Created: 14/Oct/14  Updated: 20/Nov/14  Resolved: 20/Nov/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-873-non-higher-order-calls-to-array-map-should-.patch    

 Description   

Non-higher-order calls to cljs.core/array-map are backed by a macro which emits hash maps for > 8 key-value pairs. However, array-map should always return array maps - that is the contract promised by the docstring (in both Clojure and ClojureScript) and the established behaviour in Clojure.

Example:

ClojureScript:cljs.user> (type (array-map :a true :c true :d false :b true :z false :h false :o true :p true :w false :r true :t false :g false))
cljs.core/PersistentHashMap

The map above comes from this StackOverflow question.



 Comments   
Comment by Michał Marczyk [ 14/Oct/14 12:21 PM ]

(Automatically) rebased on top of current master.

Comment by David Nolen [ 20/Nov/14 11:19 AM ]

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





[CLJS-872] Ordered collections printed as if they were unordered at the REPL Created: 13/Oct/14  Updated: 14/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is due to cljs.repl's read-then-print processing of string representations of return values that come back from the JS env. As of release 2371, the relevant code fragment lives here:

https://github.com/clojure/clojurescript/blob/r2371/src/clj/cljs/repl.clj#L156

A larger snippet to demonstrate this:

ClojureScript:cljs.user> (array-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14)
{1 2, 3 4, 5 6, 7 8, 9 10, 11 12, 13 14}
ClojureScript:cljs.user> (array-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)
{7 8, 1 2, 15 16, 13 14, 17 18, 3 4, 11 12, 9 10, 5 6}
ClojureScript:cljs.user> (seq (array-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18))
([1 2] [3 4] [5 6] [7 8] [9 10] [11 12] [13 14] [15 16] [17 18])
ClojureScript:cljs.user> (hash-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)
{7 8, 1 2, 15 16, 13 14, 17 18, 3 4, 11 12, 9 10, 5 6}

This issue seems to be the most likely cause of the problem described in this StackOverflow question. It would be nice to print ordered collections in the "expected" way to prevent user confusion.



 Comments   
Comment by David Nolen [ 14/Oct/14 5:23 AM ]

How is this handled in Clojure?

Comment by Michał Marczyk [ 14/Oct/14 7:40 AM ]

The built-in REPL simply prints string representations of values to *out* without attempting to read them back in first.

Comment by David Nolen [ 14/Oct/14 7:54 AM ]

Well the result of REPL evaluation is a string unlike Clojure, in order to get a proper print at the ClojureScript REPL it seems necessary to read back the result. I will say it's not clear to me array-map promises anything about order anyway.

Comment by Michał Marczyk [ 14/Oct/14 8:02 AM ]

It does – see http://clojure.org/data_structures, where it says

ArrayMaps
When doing code form manipulation it is often desirable to have a map which maintains key order. An array map is such a map - it is simply implemented as an array of key val key val... As such, it has linear lookup performance, and is only suitable for very small maps. It implements the full map interface. New ArrayMaps can be created with the array-map function. Note that an array map will only maintain sort order when un-'modified'. Subsequent assoc-ing will eventually cause it to 'become' a hash-map.

This snippet has been there for as long as I can remember; and this particular feature comes up in various online discussions from time to time. The docstrings for array-map (the core function) are unambiguous in their promise to construct array maps in both Clojure and ClojureScript.

As for the issue at hand, I do think it's a very minor one (hence the "Trivial" priority), but it is real – the representation printed at the REPL is out of line with the string returned from pr-str on the same value. In fact, one way to fix this would be to use pr-str representations computed on the CLJS side. (There may be some complications here, though, which we'd have to work out.)

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

Thanks for pointing out the API promise. OK, yeah I don't really have a good idea for how do this but a patch that isn't too ugly is welcome





[CLJS-871] .-default property access returns nil Created: 11/Oct/14  Updated: 14/Oct/14

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

Type: Defect Priority: Major
Reporter: Joel Holdbrooks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

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

 Description   

Types defined with deftype/defrecord which have a default field will incorrectly return nil with property access. The following example will return nil.

(deftype Foo [default])

(let [foo (Foo. "bar")]
  (.-default foo))


 Comments   
Comment by Joel Holdbrooks [ 13/Oct/14 4:19 PM ]

Patch attached. I should point out that I had to borrow js-reserved from the compiler namespace and the warning message provided hard codes the munged symbol information instead of reusing the compiler's munge fn.

Comment by Joel Holdbrooks [ 13/Oct/14 9:41 PM ]

For the sake of history, I should provide more context to this patch (I'm unable to edit the issue title for some reason). It isn't just .-default it is any field name that is also a JavaScript identifier (eg. public, private, if).

Comment by David Nolen [ 14/Oct/14 5:26 AM ]

Please lift js-reserved and any helpers like munge into the shared namespace cljs.util so that logic an be shared and hard coding avoided. Thanks.

Comment by Joel Holdbrooks [ 14/Oct/14 5:03 PM ]

Are you sure, David? That might make this patch a bit more noisy. If it's not a problem I'm happy to do it.

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

I'm sure, I'd like to avoid this kind of code duping. Cleaner in the end and better moving forward.





[CLJS-870] :preamble should place newline between files Created: 09/Oct/14  Updated: 20/Nov/14  Resolved: 20/Nov/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: 3
Labels: None

Attachments: Text File cljs-870.patch    

 Description   

To catch cases where JS libs don't end in a newline.



 Comments   
Comment by Maksim Soltan [ 18/Nov/14 9:26 AM ]

Fixed how preambles are joined, also updated test to cover this patch.

Comment by David Nolen [ 18/Nov/14 9:31 AM ]

Max this looks good, have you submitted your CA? Thanks.

Comment by Maksim Soltan [ 19/Nov/14 2:11 AM ]

Yes, also I updated my full name in JIRA to match information in CA.

Comment by David Nolen [ 20/Nov/14 11:15 AM ]

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

Comment by Maksim Soltan [ 20/Nov/14 2:10 PM ]

Thanks!





[CLJS-869] When preamble is not found in source directory, compiler does not report it Created: 03/Oct/14  Updated: 02/Dec/14  Resolved: 08/Oct/14

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

Type: Defect Priority: Minor
Reporter: Timothy Pratley Assignee: Timothy Pratley
Resolution: Completed Votes: 0
Labels: cljsbuild, compiler, patch

Attachments: Text File CLJS-869.patch    

 Description   

file target is not validated
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L525-L531
externs might need checking also.

Steps:
in project.clj add :preamble ["nosuchfile"] to the :compiler map

Expected:
"Could not find preamble file X in source path"

Actual:
Stacktrace gives no indication of the problem (without reading the compiler code)::

Compiling "resources/public/js/device_manager.js" failed.
java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil
at clojure.core$_cache_protocol_fn.invoke(core_deftype.clj:544)
at clojure.java.io$fn_8628$G8610_8635.invoke(io.clj:69)
at clojure.java.io$reader.doInvoke(io.clj:102)
at clojure.lang.RestFn.invoke(RestFn.java:410)
at clojure.lang.AFn.applyToHelper(AFn.java:154)
at clojure.lang.RestFn.applyTo(RestFn.java:132)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$slurp.doInvoke(core.clj:6390)
at clojure.lang.RestFn.invoke(RestFn.java:410)
at cljs.closure$preamble_from_paths$fn__3018.invoke(closure.clj:524)
at clojure.core$map$fn__4245.invoke(core.clj:2557)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:484)
at clojure.core$seq.invoke(core.clj:133)
at clojure.core$apply.invoke(core.clj:624)
at cljs.closure$preamble_from_paths.invoke(closure.clj:524)
at cljs.closure$make_preamble.invoke(closure.clj:529)
at cljs.closure$optimize.doInvoke(closure.clj:592)
at clojure.lang.RestFn.applyTo(RestFn.java:139)
at clojure.core$apply.invoke(core.clj:626)
at cljs.closure$build.invoke(closure.clj:980)
at cljs.closure$build.invoke(closure.clj:923)
at cljsbuild.compiler$compile_cljs$fn__3200.invoke(compiler.clj:58)
at cljsbuild.compiler$compile_cljs.invoke(compiler.clj:57)
at cljsbuild.compiler$run_compiler.invoke(compiler.clj:159)
at user$eval3326$iter_33443348$fn3349$fn_3361.invoke(form-init6680721970243583147.clj:1)
at user$eval3326$iter_33443348$fn_3349.invoke(form-init6680721970243583147.clj:1)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:484)
at clojure.core$seq.invoke(core.clj:133)
at clojure.core$dorun.invoke(core.clj:2855)
at clojure.core$doall.invoke(core.clj:2871)
at user$eval3326.invoke(form-init6680721970243583147.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6703)
at clojure.lang.Compiler.eval(Compiler.java:6693)
at clojure.lang.Compiler.load(Compiler.java:7130)
at clojure.lang.Compiler.loadFile(Compiler.java:7086)
at clojure.main$load_script.invoke(main.clj:274)
at clojure.main$init_opt.invoke(main.clj:279)
at clojure.main$initialize.invoke(main.clj:307)
at clojure.main$null_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:420)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)



 Comments   
Comment by Timothy Pratley [ 05/Oct/14 1:02 PM ]

Reports a warning

Preamble resource file not found: <file1> <file2> <file3>

Comment by David Nolen [ 08/Oct/14 4:59 PM ]

fixed https://github.com/clojure/clojurescript/commit/9fd6bf5bd55421c3d5becacc5230ed661d6fb3c3





[CLJS-868] no arity warnings on recursive calls Created: 03/Oct/14  Updated: 03/Oct/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


 Description   

If a function recursively invokes itself within its own body the invoke will not be checked for arity mismatch.






[CLJS-867] specify with Object methods requires multi-arity style definition Created: 02/Oct/14  Updated: 02/Oct/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


 Description   
Object
(foo [] ...)

Throws an `nth not supported on this type: Symbol` exception.

Object
(foo ([] ...))

Is required even though this is not true for protocol implementations.






[CLJS-866] Faulty ns macro desugaring Created: 01/Oct/14  Updated: 10/Oct/14  Resolved: 09/Oct/14

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

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

ClojureScript 0.0-2356, Clojure 1.7.2-alpha2 (but I believe this has been around for some time)


Attachments: Text File CLJS-866.patch    

 Description