<< Back to previous view

[CLJS-1400] self-host: doseq does not behave as expected Created: 07/Aug/15  Updated: 29/Aug/15  Resolved: 29/Aug/15

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

Type: Defect Priority: Major
Reporter: Chris Truter Assignee: Chris Truter
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 1400.patch     Text File infinite_loop2.patch     Text File infinite_loop.patch    
Approval: Vetted

 Description   

When a doseq appears in non-terminal position inside a do block, it "iterates" repeatedly over the first element only.

This only affects bootstrapped clojurescript, but can be supported with a vanilla cljs.js setup and the last example below.

Correctly prints 1 and then 2:

(do :before (doseq [x [1 2]] (println x)))

Correctly does not iterate, and returns :after

(do (doseq [x []] (println x)) :after)

Incorrectly prints 1 ad infinitum

(do (doseq [x [1 2]] (println x)) :after)

Example code including bootstrapping:

(def source "(do (doseq [e [1 2]] (println e)) 3)")

(cljs/eval-str
  (cljs/empty-state)
  source
  source
  {:eval       cljs/js-eval
   :source-map false
   :verbose    true}
  prn)

For convenience a repo to run this example is here:

https://github.com/crisptrutski/do-doseq-bootstrapped



 Comments   
Comment by David Nolen [ 07/Aug/15 6:38 AM ]

We do not take issues that involve 3rd party projects. This ticket needs to reproduced with ClojureScript only before proceeding any further.

Comment by David Nolen [ 07/Aug/15 7:14 AM ]

Reproduced. The patch should include a test case addition to src/test/self/self_host/test.cljs.

Comment by Joel Martin [ 07/Aug/15 9:55 AM ]

Here is a patch that adds a test which reproduces the problem.

Comment by Joel Martin [ 07/Aug/15 10:00 AM ]

Update patch with corrected success condition.

Comment by Chris Truter [ 12/Aug/15 8:18 AM ]

Added slightly more minimal test, that excludes do (still not a great test), and solve the issue by partially reverting this optimization

Perhaps

(-hash (select-keys binding [:name :shadow]))
is sufficient and more performant?

Comment by David Nolen [ 12/Aug/15 8:46 AM ]

If you're are going to mention commits, please link to them thanks.

Comment by David Nolen [ 29/Aug/15 10:00 AM ]

See also CLJS-1435

Comment by Chris Truter [ 29/Aug/15 10:33 AM ]

Mind if I close this as superceded by http://dev.clojure.org/jira/browse/CLJS-1435?

The latter makes a better test case (failure terminates...)

Comment by David Nolen [ 29/Aug/15 11:40 AM ]

fixed https://github.com/clojure/clojurescript/commit/3636a80c9b683cdc1190bd60910bf805e6ab22c0

Comment by Mike Fikes [ 29/Aug/15 2:54 PM ]

Confirmed that fix works downstream in Planck.





[CLJS-1436] self-host: Doesn't load dep ns when loading own cache Created: 29/Aug/15  Updated: 29/Aug/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: bootstrap


 Description   

Background: Both *load-fn* and *eval-fn* support the concept of passing JavaScript and analysis cache info. Such data passed to *eval-fn* should be able to be stored and subsequently passed to *load-fn* upon subsequent runs in order to effect analysis/compilation caching.

If one namespace depends on another then transitive loading occurs if no caching is involved. But if cached information it returned to *load-fn* when the top-level namespace is loaded, then transitive loading does not occur.

My speculation is that this is the case because, while the analysis cache is incorporated here

https://github.com/clojure/clojurescript/blob/r1.7.122/src/main/cljs/cljs/js.cljs#L206-L208

nothing further is done to load dependent namespaces as is done when ClojureScript source is passed in the callback.

Here is a concrete example. Let foo.core depend on bar.core:

(ns foo.core
  (:require bar.core))

(prn bar.core/x)
(ns bar.core)

(def x 2)

If you require foo.core (via an ns form passed to eval-str), then *load-fn* is called with

{:name foo.core, :macros nil, :path "foo/core"}

and it is called back upon with the loaded ClojureScript source:

{:lang :clj, :source "(ns foo.core\n  (:require bar.core))\n\n(prn bar.core/x)\n"}

and the embedded :require then causes *load-fn* to be invoked for bar.core:

{:name bar.core, :macros nil, :path "bar/core"}

with its source being passed to the callback:

{:lang :clj, :source "(ns bar.core)\n\n(def x 2)\n"}

Then the evaluation phase begins with two evaluations (done properly in reverse dependency order):

{:lang :clj, :name bar.core, :path "bar/core", :source "goog.provide(\"bar.core\");\nbar.core.x = 2;\n", :cache {:use-macros nil, :excludes #{}, :name bar.core, :imports nil, :requires nil, :uses nil, :defs {x {:name bar.core/x, :file nil, :line 3, :column 1, :end-line 3, :end-column 7, :meta {:file bar.core, :line 3, :column 6, :end-line 3, :end-column 7}}}, :require-macros nil, :doc nil}}

and

{:lang :clj, :name foo.core, :path "foo/core", :source "goog.provide(\"foo.core\");\ncljs.core.prn.call(null,bar.core.x);\n", :cache {:name foo.core, :doc nil, :excludes #{}, :use-macros nil, :require-macros nil, :uses nil, :requires {bar.core bar.core}, :imports nil}}

which causes 2 to be printed.

Then if you start fresh, but go through the same sequence, consuming cached information, you start off the same with *load-fn* being called the same way:

{:name foo.core, :macros nil, :path "foo/core"}

and with the callback now being passed cached information back in return:

{:lang :js, :source "goog.provide(\"foo.core\");\ncljs.core.prn.call(null,bar.core.x);\n", :cache {:name foo.core, :doc nil, :excludes #{}, :use-macros nil, :require-macros nil, :uses nil, :requires {bar.core bar.core}, :imports nil}}

This JavaScript is evaluated by cljs.js here

https://github.com/clojure/clojurescript/blob/r1.7.122/src/main/cljs/cljs/js.cljs#L205

and this causes this to be printed

Can't find variable: bar

Perhaps this isn't a defect if instead it is expected that clients of cljs.js handle the loading of dependent namespaces when caching is involved.






[CLJS-1435] self-host: lexical scope is broken Created: 29/Aug/15  Updated: 29/Aug/15  Resolved: 29/Aug/15

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

Type: Defect Priority: Major
Reporter: Chris Truter Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

This has the cause as http://dev.clojure.org/jira/browse/CLJS-1400, but demonstrates a more provocative compilation error.

Sample code using cljs.js to demonstrate the failure:

(cljs/eval
  (cljs/empty-state)
  '(let [x 1]
     (let [x (inc x)]
       (prn x))
     (prn x))
  {:eval (juxt (comp prn :source) cljs/js-eval)}
  (fn [& _]))

Which displays both the code (line breaks unescaped here for convenience) and resulting error:

"var x_22 = 1;
 var x_23__$1 = (x_23 + 1);
 cljs.core.prn.call(null,x_23__$1);
 cljs.core.prn.call(null,x_22);"
VM1654:2 Uncaught ReferenceError: x_23 is not defined

This issue is more noticeable than in the case encountered via "doseq", as the naming error occurs in a read position rather than write, so we get an error instead of unexpected behaviour.

The patch attached fixies this and 1400



 Comments   
Comment by Chris Truter [ 29/Aug/15 10:32 AM ]

Attached patch with a (hopefully) faster alternate

Comment by David Nolen [ 29/Aug/15 11:00 AM ]

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





[CLJS-1432] 1.7.48 assigns same value for '$ and '. symbols under advanced Created: 24/Aug/15  Updated: 29/Aug/15  Resolved: 27/Aug/15

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

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

Attachments: Text File cljs-1432-symbols-collision-2.patch     Text File cljs-1432-symbols-collision.patch    

 Description   
(println :+ :a :$ :. :a$b :a.b :z)
      => :+ :a :. :. :a$b :a$b :z

(println '+ 'a '$ '. 'a$b 'a.b 'z)
      =>  +  a  .  .  a$b  a$b  z

expected:
(not= :$ :.)
(not= '$ '.)

Related:
CLJS-1105
https://github.com/clojure/clojurescript/commit/d92b3004046d1f83ab76a2b94f4129cb16e47848
https://github.com/tonsky/datascript/issues/108



 Comments   
Comment by Nikita Prokopov [ 26/Aug/15 12:09 AM ]

Just wanted to add that this issue breaks DataScript to the point it’s impossible to use (because it relies on both '$ and '.). DataScript cannot be used at all with 1.7.48, 1.7.58, 1.7.107

Comment by David Nolen [ 26/Aug/15 10:38 PM ]

Why not just special case '. here?

Comment by Nikita Prokopov [ 27/Aug/15 3:34 AM ]

David, I’m not sure what is the purpose of this code, e.g. why it transforms symbol/keyword name at all. If this is going to constants table as a key, it can be any string, right?

My solution just eliminates other possible collisions (e.g. :a$b vs :a.b) because I think it is not fun to encounter stuff like this. It takes really huge effort to debug, because it comes from the most unexpected place: on the one hand, language guarantees keywords are only equal to themselves, on the other, it silently treats different keywords/symbols as the same.

If you think my solution is an overkill, or if there are technical reasons it can’t be applied, as I’m not aware of all the nuances, I leave this decision to you.

Other possible solution might be, if encountered kw/sym that might conflict, do not apply this optimization to them at all. We would probably lose some performance, but keep the guarantees.

Comment by David Nolen [ 27/Aug/15 6:22 AM ]

Emitting strings for the constants table defeats minification and dead code elimination.

The bug is a simple source code generation symbol clash that can be avoided with special casing '.. If you can come up with other problematic cases then I'll consider an alternative. Otherwise this is the only patch we will take.

Comment by Nikita Prokopov [ 27/Aug/15 1:10 PM ]

Attached second patch, as you suggested

Comment by David Nolen [ 27/Aug/15 3:31 PM ]

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

Comment by Nikita Prokopov [ 27/Aug/15 3:40 PM ]

Thanks David!

Comment by David Nolen [ 29/Aug/15 7:32 AM ]

I just cut a pre-release 1.7.122 with this fix.





[CLJS-1434] clojure.walk no longer preserves meta on vectors Created: 29/Aug/15  Updated: 29/Aug/15  Resolved: 29/Aug/15

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

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


 Description   
(require '[clojure.walk :as walk])
(-> (walk/prewalk 
      identity [1 (with-meta [1 2] {:foo 1})])
  (nth 1)
  meta) ;; nil instead of {:foo 1}


 Comments   
Comment by David Nolen [ 29/Aug/15 7:15 AM ]

The problem is that there is an IMapEntry case which will capture vectors and not preserve their meta. But the IMapEntry case is redundant anyway as as map entries are just vectors and these will be handled by the coll? branch of the switch.

Comment by David Nolen [ 29/Aug/15 7:18 AM ]

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





[CLJS-1407] Exposing output file dependency graph in API Created: 09/Aug/15  Updated: 28/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Juho Teperi Assignee: Juho Teperi
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Use case for boot-cljs and boot-reload:

After compilation boot-reload reloads the changed JS files. So that the files can be reloaded in correct order, boot-cljs uses dependency graph to sort the files. Currently boot-cljs accesses compiler state directly and uses data from :js-dependency-index to build the graph: https://github.com/adzerk-oss/boot-cljs/blob/0.0-3308/src/adzerk/boot_cljs/impl.clj#L17-L36

Simple solution:

If dependencies (requires) of namespace are exposed through API it is easy to build graph of cljs namespace dependencies: https://github.com/adzerk-oss/boot-cljs/blob/d479f10935be321232e2363e2ae3e9cc515a81af/src/adzerk/boot_cljs/impl.clj#L12-L32

Problem with this solution is that all-ns, ns-dependencies or target-file-for-cljs-ns do not work with foreign-deps. While foreign-dep files don't usually change and thus aren't reloaded, it's possible that user has local JS files in the project using foreign-deps and those can change.

Questions, notes and issues

  • Should cljs-dependency-graph be exposed in the API or is it enough to provide ns-dependencies and such which user can use to create dependency graph?
  • cljs.build.api/parse-js-ns can also be used to read provides and requires from compiled JS files, but this doesn't work with foreign-deps either
  • Perhaps there is some way in Closure library to reload files in correct order?
  • Supporting foreign-deps is not perhaps necessary, but if there is good way it would be nice to have.


 Comments   
Comment by Juho Teperi [ 11/Aug/15 3:18 AM ]

I would add the call to cljs.compiler.api and it could be called output-dependency-graph.

Creating the graph requires list of all the nodes and dependencies for each node. For Cljs namespaces
these are accessible through all-ns and ns analysis map :requires. Data about foreign-deps
and closure libs is available in the compiler state under :js-dependency-index key. To create the
graph we need to:

1. Get list of all nodes
2. Get dependencies for given node
3. Get output file for given node

Because steps 2 and 3 depend on the type of node, it would probably be easiest to collect those
values in step 1. So step 1 would do something like this:

{{(get-nodes ...) => {:provides "goog.net" :file "out/goog/net.js" :dependencies #{"goog.foo"}} {:provides "frontend.core" :file "out/frontend/core.js" :dependencies #{"cljs.core"}}}}

That could be implemented by concatenating data from cljs namespaces retrieved from all-ns etc. with
data from :js-dependency-index. The next and last step would be to construct the graph using reduce.

Using this implementation there would be just one new API call: output-dependency-graph.

I was thinking alternative approach with all-ns, find-ns etc. versions which would work also with foreign-deps and closure libs, but I don't think it's very easy (or efficient) e.g. to retrieve data for foreign-dep with just a name as they are indexed by file paths.





[CLJS-1304] Behavior of clojure.string/replace differs from Clojure Created: 09/Jun/15  Updated: 27/Aug/15  Resolved: 27/Aug/15

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

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Andrew Rosa
Resolution: Completed Votes: 0
Labels: None

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

 Description   

When using clojure.string/replace with a function as the replacement value, and using a regular expression with capturing groups, the behavior differs between Clojure and ClojureScript. In Clojure, the replacement fn is passed a tuple of the match and capturing group. In ClojureScript, it is only passed the match value.

In Clojure:

=> (println (str/replace "foobar" #"f(o)o" #(do (println %) "X")))
[foo o]
"Xbar"

In ClojureScript:

=> (println (str/replace "foobar" #"f(o)o" #(do (println %) "X")))
foo
Xbar


 Comments   
Comment by Daniel Woelfel [ 11/Jun/15 3:31 AM ]

If you're looking for a workaround, you can still get the match result from the 2nd arg:

=> (println (clojure.string/replace "foobar" #"f(o)o" (fn [& args] (do (println args) "X"))))
(foo o 0 foobar)
Xbar
Comment by Andrew Rosa [ 13/Jun/15 12:40 PM ]

Implementation and tests for the requested behaviour. Despite of the `vec` not being strictly necessary, I stick with it so the behaviour will be identical on both implementations.

Comment by David Nolen [ 13/Jun/15 5:07 PM ]

Is there any reason here to not just copy the Clojure implementation?

Comment by Andrew Rosa [ 13/Jun/15 5:25 PM ]

Hi David,

Clojure's impl was the first place that I look for, but this feature in special is written against the underlaying Java libraries. You can check it here.

Studying the Clojure implementation I found that there are some of re-* family of functions missing on ClojureScript side, and unfortunately they are heavily inspired on Java's Regex structure (return matchers, find and extract groups). MAYBE we can something like them to ClojureScript, but I don't know the real value there. Anyway, I think that port them or not is subject for another ticket - if you want that I investigate more, I could create it.

Comment by Francis Avila [ 13/Jun/15 5:47 PM ]

I made a proposal some time ago to bring clj and cljs re handling closer together: CLJS-776

Comment by David Nolen [ 13/Jun/15 8:51 PM ]

Andrew, was just looking for rationale. What you've done looks OK to me. People should try the patch and give feedback. Thanks!

Comment by Andrew Rosa [ 13/Jun/15 10:41 PM ]

Hi David, I understand and appreciate your concerns I just want to make clear what I've search and done, sorry if I sound rude - maybe I stepped on my language limitations.

Francis, really this re-* thing is tricky to deal with. I will take a closer look onto your ticket and see if I can add any idea.

Thanks everyone!

Comment by David Nolen [ 27/Aug/15 5:54 AM ]

This patch needs to be rebased to master.

Comment by Andrew Rosa [ 27/Aug/15 9:50 AM ]

Rebased patch against master.

Comment by David Nolen [ 27/Aug/15 11:14 AM ]

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





[CLJS-1430] self-host: .toString is emitted as .toString$ (munged) Created: 18/Aug/15  Updated: 27/Aug/15  Resolved: 27/Aug/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1430-2.patch     Text File CLJS-1430-3.patch     Text File CLJS-1430-4.patch     Text File CLJS-1430.patch    
Patch: Code and Test

 Description   

If you evaluate a form like (.toString "a") in bootstrapped mode, the emitted JavaScript will end up with a dollar-sign: "a".toString$().

This can be reproduced via a compile-str unit test involving the form.



 Comments   
Comment by Mike Fikes [ 18/Aug/15 11:29 PM ]

This is because, even though the compiler indicates that there are no reserved words (the empty set for second argument here):

https://github.com/clojure/clojurescript/blob/v1.7/src/main/clojure/cljs/compiler.cljc#L1006

cljs.core/munge is called here

https://github.com/clojure/clojurescript/blob/v1.7/src/main/clojure/cljs/compiler.cljc#L108

to imitate Clojure's munge. But cljs.core/munge introduces the dollar-sign for reserved JavaScript keywords:

https://github.com/clojure/clojurescript/blob/v1.7/src/main/cljs/cljs/core.cljs#L9898

The direct ClojureScript imitation of Clojure's munge is munge-str here

https://github.com/clojure/clojurescript/blob/v1.7/src/main/cljs/cljs/core.cljs#L9882

Calling that (private) method instead fixes the problem.

Comment by Mike Fikes [ 18/Aug/15 11:37 PM ]

The attached file fixes the issue.

On problem with it, though it it has the compiler calling a private function in cljs.core namespace.

Comment by Mike Fikes [ 18/Aug/15 11:44 PM ]

Attaching a 2nd patch which properly updates the latch count in the unit test.

Comment by David Nolen [ 26/Aug/15 10:37 PM ]

This ticket does not explain why toString would get munged - it's not on the reserved list.

Comment by Mike Fikes [ 26/Aug/15 11:04 PM ]

You are right, David. The root cause actually has to do with (js-reserved? "toString") returning true when it should not do so. I suspect this has to do with the use of gobject/containsKey giving a false positive for things in the Object prototype, viz:

cljs.user=> (require '[goog.object :as gobject])
nil
cljs.user=> (gobject/containsKey #js {} "toString")
true

So, I retract my patch which completely glosses over the aspect that js-reserved? is probably the culprit.

Comment by Mike Fikes [ 26/Aug/15 11:53 PM ]

David, based on your feedback, attached a CLJS-1430-3.patch which might be closer to a correct approach in that it fundamentally revises js-reserved? to not return true for things like "toString".

My JavaScript is not strong enough for a feel as to whether the actual implementation in the patch is up to muster, nor whether munging is in the critical path for perf.

Comment by David Nolen [ 27/Aug/15 6:27 AM ]

Ah ok, revise the patch to remove the two tests, instead just use (.hasOwnProperty js-reserved foo)

Comment by Mike Fikes [ 27/Aug/15 7:56 AM ]

CLJS-1430-4.patch updated to use .hasOwnProperty, and ran all tests except Nashorn. Also ran self-host tests.

Comment by David Nolen [ 27/Aug/15 8:23 AM ]

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





[CLJS-1422] cljs.js/eval-str fails for ns form on node.js with simple optimizations Created: 14/Aug/15  Updated: 27/Aug/15

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

Type: Defect Priority: Major
Reporter: Michael Ballantyne Assignee: Michael Ballantyne
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS_1422.patch     Text File CLJS_1422-v2.patch    
Patch: Code and Test
Approval: Accepted

 Description   

find-ns-obj has a special case for node.js under simple optimizations that uses eval to find the namespace object. If the object does not already exist this produces an exception; on other platforms find-ns-obj returns nil for a non-existant namespace.

As a result eval-str produces an error when evaluating a ns form for a new namespace.

There's a commented out test for this already in the source at https://github.com/clojure/clojurescript/blob/master/src/test/self/self_host/test.cljs#L121



 Comments   
Comment by Michael Ballantyne [ 14/Aug/15 1:17 PM ]

Added a patch with a fix and uncomment the existing test.

The special case of find-ns-obj for node.js under optimizations now checks each path segment with an eval of typeof to see if it exists, returning nil if the namespace object is absent.

Comment by David Nolen [ 26/Aug/15 10:47 PM ]

The overall idea is sound but the patch needs work. Just use goog.isObject instead of the needless js/eval call.

Comment by Michael Ballantyne [ 27/Aug/15 12:36 AM ]

I believe we need at least one `js/eval` of `typeof` in order to establish whether the object for the initial namespace segment exists, as there is no parent object to examine and referencing an undefined variable throws `ReferenceError`. `goog.isObject(thing)` where `thing` is a reference to an undefined variable throws `ReferenceError`; `typeof thing` returns `"undefined"`.

However, after obtaining the initial namespace segment we can use the normal `find-ns-obj*`. I've attached an updated patch using this strategy.

Comment by David Nolen [ 27/Aug/15 6:33 AM ]

Just a small code review thing to clarify the intent, why not just remove the multiple js/evals and lean on try/catch? The only possible error is attempting to use an undefined var which in that case we just want to bail anyway.





[CLJS-1353] range inconsistent with Clojure if step is 0 Created: 20/Jul/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1353-14-Aug-15.patch     Text File CLJS-1353-15-Aug-15.patch    
Patch: Code and Test

 Description   
cljs.user=> (range 3 9 0)
()

With CLJ-1018 Clojure behaves this way and docstring for Clojure updated:

user=> (take 10 (range 3 9 0))
(3 3 3 3 3 3 3 3 3 3)


 Comments   
Comment by Samuel Miller [ 14/Aug/15 9:57 PM ]

Uploaded potential patch with updated tests.

Clojures implementation can be seen here...

https://github.com/clojure/clojure/blob/41af6b24dd5be8bd62dc2b463bc53b55e18cd1e5/src/jvm/clojure/lang/Range.java#L89

Basically what is boils down to is as long as the start and end are not equal give back the start number if the step is 0 (effectively repeat). Tried to keep the previous code for range the same and added a case for zero that checks for the equality of start and end.

Ran tests on Linux against SpiderMonkey, V8, and Nashorn they passed. Played around in the repl and compared against Clojure and seems to be equivalent.

Comment by Mike Fikes [ 14/Aug/15 11:51 PM ]

With CLJS-1353-14-Aug-15.patch, tests pass for me on OS X for V8, SpiderMonkey, and JavaScriptCore.

I also tried it in bootstrapped ClojureScript in JavaScriptCore on OS X, manually running the tests successfully.

Thinking about perf, it looks like it doesn't really affect the (probably common) code path for positive steps, and only adds one more necessary check to distinguish between negative and zero (which wasn't being done before). In the zero-step branch (which is probably not common at all), there is perhaps a tiny benefit in using == over = since we can assume numeric arguments. (You end up with a direct start === end as opposed to cljs.core._EQ_ being applied.)

There is one other bit of code that employs that pattern: https://github.com/clojure/clojurescript/blob/r1.7.48/src/main/cljs/cljs/core.cljs#L5058

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

Yes the test should be changed to ==. Thanks.

Comment by Samuel Miller [ 15/Aug/15 11:19 PM ]

Thanks for the reviews. I updated the patch to use == instead of =

Comment by David Nolen [ 26/Aug/15 10:41 PM ]

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





[CLJS-1431] load-file doc output missing arglists Created: 21/Aug/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

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

noderepljs


Attachments: Text File CLJS-1431-8-21-15.patch    
Patch: Code

 Description   

The output for `(doc load-file)` is missing the arglists:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 52087
To quit, type: :cljs/quit
cljs.user=> (doc load-file)
-------------------------
load-file
REPL Special Function
  Sequentially read and evaluate the set of forms contained in the file.

nil

A line reading ([name]) should appear between load-file and REPL Special Function



 Comments   
Comment by Mike Fikes [ 21/Aug/15 7:50 PM ]

Root cause is the use of :arglist instead of :arglists here:
https://github.com/clojure/clojurescript/blob/v1.7/src/main/clojure/cljs/repl.cljc#L1113

Comment by Chris Pickard [ 21/Aug/15 8:44 PM ]

I've attached a patch (CLJS-1431-8-21-15.patch) that address the issue Mike reported

Comment by Mike Fikes [ 21/Aug/15 9:00 PM ]

For me, Chris Pickard's CLJS-1431-8-21-15.patch applies cleanly to master, produces the correct behavior in script/noderepljs, and unit tests pass for V8, SpiderMonkey, and JavaScriptCore (Nashorn skipped).

Comment by David Nolen [ 26/Aug/15 10:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/19bcf10ccedf63f0e3bf0abfb7481f9c347895fc





[CLJS-1433] self-host: cljs.js/*eval-fn* passed nil :cache Created: 24/Aug/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

If you load some ClojureScript source that implements a namespace, the cljs.js/*eval-fn* will always be passed a :cache value of nil



 Comments   
Comment by Mike Fikes [ 24/Aug/15 12:24 PM ]

Simple defect—simply need to deref the atom.

Comment by David Nolen [ 26/Aug/15 10:28 PM ]

fixed https://github.com/clojure/clojurescript/commit/5f3c494ce4274177e8e9cb71adcb87b5f9394423





[CLJS-1403] Add script/bootstrap capability for Windows Created: 08/Aug/15  Updated: 26/Aug/15  Resolved: 26/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Peter Stephens Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None
Environment:

All versions of Windows


Attachments: Text File cljs-1403-v2.patch     Text File cljs-1403-v3.patch    

 Description   

Implement a Windows shell script to download and unpack the correct versions of various ClojureScript dependencies.

Proposal: Implement a PowerShell script to do this work. PowerShell has been pre-installed on Windows since version 7 and is available (in various flavors) on Windows down to XP.

1. Downloading of files from the web is a proper command (Invoke-RestMethod) in PowerShell since version 3. Earlier versions of PowerShell could be supported by using Dotnet's WebClient.DownloadFile method.

2. Unzipping files is a little more difficult. But the GUI shell's capability is exposed through COM and this appears to work well from PowerShell.

3. The current dependency versions can be extracted from the /bin/sh script using regex.

See https://gist.github.com/pstephens/b260b1ad8b166489b562 for a rough draft of this script.



 Comments   
Comment by Peter Stephens [ 08/Aug/15 1:15 PM ]

Related issues: CLJS-24 and CLJS-66, both closed.

Comment by Peter Stephens [ 08/Aug/15 9:05 PM ]

Almost have a patch ready to submit... but lot's of the Windows shell wrappers are also broken. Working through the problems so I can hopefully get through the test suite on Windows.

Comment by David Nolen [ 09/Aug/15 10:25 AM ]

Thanks for working on this!

Comment by Peter Stephens [ 11/Aug/15 9:28 PM ]

This patch includes some basic Windows support scripts.
1. A port of script/bootstrap to PowerShell
2. A port of script/test to PowerShell
3. Updated bin/cljsc.bat to match bin/cljsc
4. And a minor tweak to bin/cljsc.clj to write exceptions to stderr and describe the nature of the exception.

Comment by Peter Stephens [ 11/Aug/15 9:46 PM ]

Tested on Windows 10 w/fresh clojurescript clone.

Bootstrap pulled down required dependencies.

Ran the test suite through Nashorn with two (apparently known) failing tests:
FAIL in (test-919-generic-cas) (at <anonymous> (builds/out-adv/core-advanced-test.js:NaN:963)
testing CLJS-919, CAS should on custom atom types
expected: (== (clojure.core/deref a0) 10)
actual: (not (== nil 10))

FAIL in (test-919-generic-cas) (at <anonymous> (builds/out-adv/core-advanced-test.js:NaN:963)
testing CLJS-919, CAS should on custom atom types
expected: (== (clojure.core/deref a1) 20)
actual: (not (== nil 20))

I couldn't find a Jira issue describing these failures but some work was done by Sebastian Bensusan on this: http://www.raynes.me/logs/irc.freenode.net/clojurescript/2015-04-20.txt and https://gist.github.com/bensu/c6302a33e56d5d7eae9e

Comment by David Nolen [ 12/Aug/15 6:49 AM ]

Patch looks good but I could not apply with git am. Did you following the patch instructions here https://github.com/clojure/clojurescript/wiki/Patches?

Also please submit only squashed patches. Thanks!

Comment by Peter Stephens [ 12/Aug/15 9:18 PM ]

Apparently there are encoding and and line ending issues from Windows generated patches using the linked patching instructions. I'll squash the commits and work out the encoding issues. Then I'll test-apply the patch to a fresh clone of master before resubmitting.

Comment by Peter Stephens [ 16/Aug/15 9:59 PM ]

1. Squished the patch. There is now only one commit.
2. Created the patch with Windows git bash shell. So the encoding is in proper ASCII without extra fluff.

NOTE: The batch and PowerShell files are encoded with CRLF... so special care needs to be taken when applying this patch. Here is the command I used:

git am --keep-cr --whitespace=nowarn < cljs-1403-v3.patch

The patch will not apply without keep-cr because the CR characters will be stripped out. The whitespace=nowarn option suppresses warnings but the patch will still apply without this option.

Comment by David Nolen [ 26/Aug/15 10:24 PM ]

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





Generated at Sun Aug 30 03:05:51 CDT 2015 using JIRA 4.4#649-r158309.