<< Back to previous view

[CLJS-1636] Mark some symbols in core macros ns as private Created: 27/Apr/16  Updated: 27/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

There are some symbols in the core macros namespace that are not meant for external consumption. Some of these are marked private and some aren't. This ticket asks that the others be marked private as well.

An example of one symbol marked private is defcurried.
An example of one symbol not marked private is caching-hash.



 Comments   
Comment by Mike Fikes [ 27/Apr/16 8:21 AM ]

In CLJS-1636.patch, I checked and it appears nothing in the compiler codebase is explicitly using these symbols outside of the cljs.core namespace. But, it is still worth scanning through these to check if they make sense. For example js-debugger and js-comment are a couple that might actually be meant for public use, but it is difficult to tell.

Comment by Mike Fikes [ 27/Apr/16 2:43 PM ]

Note, that in #cljs-dev slack, there appears to be interest in caching-hash being public.

(I don't mind revising the patch to suit whatever is needed. At the same time, I'm certainly not in a position to take decisions on what is public API or not.)

Comment by Mike Fikes [ 27/Apr/16 2:43 PM ]

Note, that in #cljs-dev slack, there appears to be interest in caching-hash being public.

(I don't mind revising the patch to suit whatever is needed. At the same time, I'm certainly not in a position to take decisions on what is public API or not.)





[CLJS-1635] Var type implements IEquiv but not IHash Created: 26/Apr/16  Updated: 27/Apr/16

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

Type: Defect Priority: Minor
Reporter: Chris Vermilion Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: deftype
Environment:

Tested on OS X 10.11, Chrome.


Attachments: Text File CLJS-1635.patch    

 Description   

The Var type implements IEquiv based on the var's symbol, but not IHash. That means that two vars with the same symbol compare equal but don't hash equal, which will cause strange results if you put them in a hash-{map,set}:

cljs.user=> (def foo "bar")
#'cljs.user/foo
cljs.user=> (= #'foo #'foo)
true
cljs.user=> (= (hash #'foo) (hash #'foo))
false

Patch forthcoming.



 Comments   
Comment by Chris Vermilion [ 26/Apr/16 10:41 PM ]

Patch note: The patch fixes the issue but I haven't added a test. It didn't seem like the hash behavior of basic types was tested in general, but moreover while I think this behavior is desirable I'm not sure it should be guaranteed. Happy to write a test if that would be useful.

Comment by Chris Vermilion [ 26/Apr/16 10:48 PM ]

Aside for the curious on how this came up at all: the Schema library uses Vars to specify recursive schemas, and does internal caching by with a map keyed by schemas themselves. If you defined the same recursive schema multiple times, the results would be unpredictable, since two equivalent recursive schemas would compare equal but wouldn't necessarily be interchangeable as map keys.





[CLJS-1629] Fix warning about duplicate test-pr-str Created: 20/Apr/16  Updated: 26/Apr/16

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

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

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

 Description   

There are 2 definitions of test-pr-str in core_test.cljs. The 2nd definition shadows the 1st which results in some tests not beeing run.



 Comments   
Comment by Roman Scherer [ 20/Apr/16 7:59 AM ]

The attached patch moves all pr-str tests into a single test-pr-str definition.

Comment by Mike Fikes [ 26/Apr/16 2:02 PM ]

Hey Roman, now these execute, they fail in bootstrap. Try running script/test-self-parity.

It looks like the root cause is that some of the test rely on ordering in hash-based collections. (A similar fix for this kind of stuff was done in CLJS-1592.)

Comment by Roman Scherer [ 26/Apr/16 3:10 PM ]

Mike, I updated the patch and the tests now also run in bootstrap. I also changed the test name to test-printing and added testing sections for each kind of print.

Comment by Mike Fikes [ 26/Apr/16 3:20 PM ]

LGTM





[CLJS-1611] Function arity dispatch returns arity Created: 27/Mar/16  Updated: 26/Apr/16

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

Type: Defect Priority: Minor
Reporter: Prince John Wesley Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, compiler

Attachments: PNG File arity-0.png     PNG File arity-1.png     PNG File arity-2.png    

 Description   

JS code generated from cljs.js/eval-str* and cljs.js/compile-str* functions are out of order(see the attachment).

Used the below compilation option to generate js code.
{ :load-macros true
:analyze-deps true
:static-fns false
:def-emits-var true
:context :expr }



 Comments   
Comment by Mike Fikes [ 30/Mar/16 10:55 PM ]

This is not a self-host issue, per se, is it? I think it is a general limitation of the work we did for :def-emits-var (in that it doesn't handle the multi-arity case). For example, in a non-self-host REPL:

$ java -jar cljs.jar -m cljs.repl.nashorn
To quit, type: :cljs/quit
cljs.user=> (defn fun-arity
  ([] "zero")
  ([_] "one"))
1
cljs.user=> (def a 3)
#'cljs.user/a

(The issue could still be fixed... just wanted to clarify whether this is a self-host or a general issue.)

Comment by Prince John Wesley [ 30/Mar/16 11:04 PM ]

Yes. its a general issue.

Comment by Mike Fikes [ 26/Apr/16 2:37 PM ]

Removed self-host from title and bootstrap from label.





[CLJS-1634] Track bound dynamic variables to support binding in async mechanisms. Created: 26/Apr/16  Updated: 26/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Christian Weilbach Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, enhancement
Environment:

Any cljs version.



 Description   

The issue has been raised before:

While the reasoning behind the proposal is still valid, the original approach has made no progress due to the performance penalty. I have implemented a simplified approach with mutable JavaScript datastructures to minimize the performance impact. Because we are single-threaded we can use js assignment and don't need to port Clojure's binding frame. A small penalty is paid by the user of binding (see benchmark8) and a higher one by async mechanisms capturing and restoring the bindings (benchmark1-7):

https://gist.github.com/whilo/a8ef2cd3f0e033d3973880a2001be32a

I would provide patches to ClojureScript, if this looks like a worthwhile approach.






[CLJS-1633] Improve error associated with invalid foreign-libs :file path Created: 26/Apr/16  Updated: 26/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The current error reported when :advanced compiling with an invalid :foreign-libs :file path is effectively (slurp nil):

Caused by: java.lang.IllegalArgumentException: Cannot open <nil> as a Reader.

With a small patch this could be improved to provide a specific message and relevant context, something like:

Caused by: clojure.lang.ExceptionInfo: Unable to resolve url for foreign-deps source {:foreign true, :url nil, :source-url nil, :provides ["cljsjs.example-thing"], :requires (), :lines nil, :source-map nil, :file "broken-path-to-example-thing.js"}

I've created a simple repo based on the mies template to demonstrate the problem. Note that core.cljs requires the foriegn-lib which is defined in deps.clj (but with an invalid :file path). scripts/release.clj shows current behaviour. scripts/release-with-patch.clj shows proposed behaviour.

https://github.com/condense/apr26-foreign-libs-path-error.core.git

Below shows an isolated fix to cljs.closure/foreign-deps-str which checks for a nil url. An alternative approach might be to check at the point where the source maps are prepared (something like (assert (or url url-min) "xxx")) but this would be more likely to have side effects.

(defn foreign-deps-str [opts sources]
  (letfn [(to-js-str [ijs]
            (if-let [url (or (and (#{:advanced :simple} (:optimizations opts))
                                  (:url-min ijs))
                             (:url ijs))]
              (slurp url)
              (throw (ex-info "Unable to resolve url for foreign-deps source" ijs))))]
    (str (string/join "\n" (map to-js-str sources)) "\n")))

I'd be happy to prepare a patch if this seems like the right approach. Have signed contributor agreement.






[CLJS-1588] Self-host: satisfies? on defrecord instance Created: 24/Feb/16  Updated: 25/Apr/16  Resolved: 25/Apr/16

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

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

 Description   

If you define a record satisfying a protocol, satisfies? doesn't return true:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 51714
To quit, type: :cljs/quit
cljs.user=> (require 'cljs.js)
nil
cljs.user=> (def st (cljs.js/empty-state))
#'cljs.user/st
cljs.user=> (cljs.js/eval-str st
"(defrecord Foo [x] IComparable (-compare [_ o] (compare x (.-x o))))" 
nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value cljs.user/Foo}
cljs.user=> (cljs.js/eval-str st
"(satisfies? IComparable (Foo. 1))"
nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value false}


 Comments   
Comment by Mike Fikes [ 24/Feb/16 3:34 PM ]

High-level analysis:

Using :repl-verbose, you can see this as part of being emitted.

this.cljs$lang$protocol_mask$partition0$ = 2229667594;
this.cljs$lang$protocol_mask$partition1$ = 10240;

If you compare the same for bootstrap, and do a diff, ignoring gensym diffs, you see

this.cljs$lang$protocol_mask$partition0$ = -2065299702;
this.cljs$lang$protocol_mask$partition1$ = 0;

Both of first integers is the same hex value 0x84E6070A, but evidently in the bootstrapped implementation it is being interpreted and emitted as as 2's complement 32-bit value. My hunch is that this is at the root of the issue.

Comment by Mike Fikes [ 24/Feb/16 5:36 PM ]

The root cause is that, in the definition of fast-path-protocols, bit-shift-left is being used until 2147483648 (which is 0x80000000) is reached. In Clojure (bit-shift-left 1 31) yields this integer, but in ClojureScript, since the underlying host arithmetic is 32-bit 2's complement, this yields -2147483648. The specific number being produced is important because == is used in that definition. The fix is to simply use multiplication by 2 instead.

This results in the second line in the protocol mask numbers to match the one produced in JVM ClojureScript (10240 instead of 0, prior to the fix):

this.cljs$lang$protocol_mask$partition0$ = -2065299702;
this.cljs$lang$protocol_mask$partition1$ = 10240;

Presumably this is the important aspect, with the first number being OK as it is equivalent with respect to bit mask operations.

The production code change is a single switch from (core/bit-shift-left b 1) to the equivalent (for the purposes of setting up fast-path-protocols), (core/* 2 b). Also, a good thing is that this definition is not in a critical perf path (it is a def).

Comment by David Nolen [ 26/Feb/16 1:43 PM ]

This one gives me pause, I think we should conditionalize this one for now. I think for ClojureScript we also want to clamp to 32bits by bit-or'ing with 0.

Comment by Mike Fikes [ 26/Feb/16 3:38 PM ]

CLJS-1588-2.patch conditionalizes.

Here is it working in JVM ClojureScript:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 55658
To quit, type: :cljs/quit
cljs.user=> (defrecord Foo [x] IComparable (-compare [_ o] (compare x (.-x o))))
cljs.user/Foo
cljs.user=> (satisfies? IComparable (Foo. 1))
true

I don't think we can clamp (bit-or with zero), as this causes it to go back to a negative number (here using int to do the bit-or-with-zero op):

cljs.user=> (* 2 (Math/pow 2 30))
2147483648
cljs.user=> (int (* 2 (Math/pow 2 30)))
-2147483648

With clamping with int the self host unit test fails, but without it it passes.

Here is the relevant new expression reference:

[p #?(:clj  (core/bit-shift-left b 1)
      :cljs (core/* 2 b))]))
Comment by Francis Avila [ 26/Feb/16 4:39 PM ]

I think David may be suggesting that the bitfield should be coerced to signed-int on both jvm and js?

For example, the following code:

(iterate (fn [[p b]]
             (if (== (unchecked-int 0x80000000) b)
               [(inc p) 1]
               [p (unchecked-int (bit-shift-left b 1))]))
    [0 1])

However this does not work in clojurescript because unchecked-int is broken there (and the macro version is curiously missing, too).

Another option is to use only 31 bits.

Comment by Mike Fikes [ 24/Apr/16 10:35 AM ]

The previously attached patch no longer applies to master. Attaching an update CLJS-1588-3.patch which has the same production code changes, but instead puts the unit test in the regular test suite.

This new test (which is much easier to read!) will fail without the production change via script/test-self-parity in this way:

FAIL in (test-records) (at cljs.test.js:414:14)
Testing records
expected: (satisfies? IComparable (->FooComparable 1))
  actual: false
Comment by David Nolen [ 25/Apr/16 6:19 AM ]

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





[CLJS-1632] Typos, c/e, and consistency of docs/params Created: 24/Apr/16  Updated: 25/Apr/16  Resolved: 25/Apr/16

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

Type: Enhancement Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Fixes typos, consistency, making params match docs, etc.

Covers what was previously CLJS-1510, CLJS-1522, CLJS-1526, CLJS-1534, CLJS-1535, and other similar changes.



 Comments   
Comment by David Nolen [ 25/Apr/16 6:15 AM ]

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





[CLJS-1510] Invalid refer message mentions var twice Created: 11/Dec/15  Updated: 24/Apr/16  Resolved: 24/Apr/16

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

Type: Defect Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: None

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

 Description   
cljs.user=> (require '[clojure.string :refer [foo]])
clojure.lang.ExceptionInfo: Invalid :refer, var var clojure.string/foo does not exist in file <cljs repl> {:tag :cljs/analysis-error}

Note "var var" above.



 Comments   
Comment by Mike Fikes [ 24/Apr/16 12:08 PM ]

Will coalesce into a larger patch with similar changes.





[CLJS-1522] Self-host: Update parameters on root *load-fn* / *eval-fn* Created: 23/Dec/15  Updated: 24/Apr/16  Resolved: 24/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: bootstrap

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

 Description   

There are root functions set up for the dynamic vars *load-fn* and *eval-fn*, but the named parameters on those dynamic vars reflect earlier versions of the code, prior to the values being updated to being maps. This ticket asks the names to be updated in order to avoid confusing any developers that may derive meaning from the parameter names.



 Comments   
Comment by Mike Fikes [ 24/Apr/16 12:06 PM ]

Will coalesce into a larger patch with similar changes.





[CLJS-1526] Doc for *print-err-fn* refers to *print-fn* Created: 30/Dec/15  Updated: 24/Apr/16  Resolved: 24/Apr/16

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

Type: Defect Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: docstring

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

 Description   

The reference to *print-fn* in the doc output (copied below) is a cut-n-paste error.

cljs.user=> (doc *print-err-fn*)
-------------------------
cljs.core/*print-err-fn*
  Each runtime environment provides a different way to print error output.
  Whatever function *print-fn* is bound to will be passed any
  Strings which should be printed.
nil


 Comments   
Comment by Mike Fikes [ 24/Apr/16 12:05 PM ]

Will coalesce into a larger patch with similar changes.





[CLJS-1534] Docstring for bit-and-not Created: 01/Jan/16  Updated: 24/Apr/16  Resolved: 24/Apr/16

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

Type: Defect Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: docstring

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

 Description   

Docstring for bit-and-not is inadvertently a copy of the docstring for bit-and.



 Comments   
Comment by Mike Fikes [ 24/Apr/16 12:02 PM ]

Will coalesce into a larger patch with similar changes.





[CLJS-1535] Make param for neg? consistent with pos? and zero? Created: 01/Jan/16  Updated: 24/Apr/16  Resolved: 24/Apr/16

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

Type: Enhancement Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: None

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

 Description   

The param for neg? is x while for pos? and zero? it is n.

Simple enhancement for consistency.



 Comments   
Comment by Mike Fikes [ 24/Apr/16 11:57 AM ]

Will coalesce into a larger patch with similar changes.





[CLJS-1595] Update Closure Compiler to v20160208 Created: 29/Feb/16  Updated: 23/Apr/16  Resolved: 23/Apr/16

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

Type: Task Priority: Trivial
Reporter: Michael Zhou Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: clojurescript

Attachments: Text File update-closure-compiler-v20160208.patch    

 Description   

Now that Closure Cmopiler v20160208 has been pushed to Maven Central, update to that version.



 Comments   
Comment by Linus Ericsson [ 29/Feb/16 12:06 PM ]

Success report:

I applied update-closure-compiler-v20160208.patch to f58dcdf4 (22 feb) and successfully built clojurescript.

I added the resulting [org.clojure/clojurescript "1.8.28"] to a non-trivial reagent/figwheel based project.clj and updated its closure dependency as well.

I managed to start the repl and everything seem to work fine, these warnings are shown after upgrading clojurescript:

WARNING: Unknown option ':compiler-env'.
WARNING: Unknown option ':special-fns'.
WARNING: Unknown option ':warn-on-undeclared'.

I have not read all the changes to ClojureScript and therefore assume this is just some regression for changed compiler options.

$lein figwheel
Figwheel: Starting server at http://localhost:3449
Figwheel: Watching build - dev
Compiling "resources/public/js/compiled/frontlyft.js" from ["src" "../delat_src"]...
Successfully compiled "resources/public/js/compiled/frontlyft.js" in 1.693 seconds.
Figwheel: Starting CSS Watcher for paths ["resources/public/css"]
Launching ClojureScript REPL for build: dev
Figwheel Controls:
(stop-autobuild) ;; stops Figwheel autobuilder
(start-autobuild [id ...]) ;; starts autobuilder focused on optional ids
(switch-to-build id ...) ;; switches autobuilder to different build
(reset-autobuild) ;; stops, cleans, and starts autobuilder
(reload-config) ;; reloads build config and resets autobuild
(build-once [id ...]) ;; builds source one time
(clean-builds [id ..]) ;; deletes compiled cljs target files
(print-config [id ...]) ;; prints out build configurations
(fig-status) ;; displays current state of system
Switch REPL build focus:
:cljs/quit ;; allows you to switch REPL to another build
Docs: (doc function-name-here)
Exit: Control+C or :cljs/quit
Results: Stored in vars *1, *2, *3, *e holds last exception object
Prompt will show when Figwheel connects to your application
WARNING: Unknown option ':compiler-env'.
WARNING: Unknown option ':special-fns'.
WARNING: Unknown option ':warn-on-undeclared'.
To quit, type: :cljs/quit

These are the dependencies

:dependencies [[org.clojure/clojure "1.8.0"]
[org.clojure/clojurescript "1.8.28"]
[org.clojure/core.async "0.2.374"]
[reagent "0.5.1"]
[timothypratley/reanimated "0.1.4"]
[cljsjs/react "0.14.3-0"]
[com.andrewmcveigh/cljs-time "0.4.0"]
[com.google.javascript/closure-compiler "v20160208"]
[datascript "0.15.0"]
[cljs-http "0.1.39"]
[secretary "1.2.3" :exclusion [org.clojure/clojurescript]]]

:plugins [[lein-cljsbuild "1.1.2" :exclusion [org.clojure/clojure]]
[lein-figwheel "0.5.0-6"]]

Comment by Michael Zhou [ 23/Mar/16 4:12 PM ]

Is there an update on this?

Comment by David Nolen [ 23/Apr/16 3:51 PM ]

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

Comment by Michael Zhou [ 23/Apr/16 4:19 PM ]

Thanks!





[CLJS-1612] Resolve ns aliases in syntax quote Created: 31/Mar/16  Updated: 23/Apr/16  Resolved: 23/Apr/16

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

Type: Defect Priority: Minor
Reporter: Tom Jack Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: patch

Attachments: Text File CLJS-1612-aux.patch     Text File CLJS-1612.patch     Text File resolve-ns-aliases-in-syntax-quote.patch     Text File self-host-resolve-ns-aliases-in-syntax-quote.patch    
Patch: Code and Test

 Description   

This test (cljs/syntax_quote_test.cljs) fails:

(ns cljs.syntax-quote-test
  (:require [cljs.test :as test :refer-macros [deftest is]]))

(deftest test-syntax-quote
  (is (= `foo 'cljs.syntax-quote-test/foo))
  (is (= `test/test-vars 'cljs.test/test-vars))
  (is (= `test/deftest 'cljs.test/deftest))
  (is (= `test/foo 'cljs.test/foo)))

The first assertion passes (since f240826034), but the remaining assertions fail: test is not resolved to cljs.test, so e.g. `test/foo is just 'test/foo.

Of course, the analogous clojure/syntax_quote_test.clj passes:

(ns clojure.syntax-quote-test
  (:require [clojure.test :as test :refer [deftest is]]))

(deftest test-syntax-quote
  (is (= `foo 'clojure.syntax-quote-test/foo))
  (is (= `test/test-vars 'clojure.test/test-vars))
  (is (= `test/deftest 'clojure.test/deftest))
  (is (= `test/foo 'clojure.test/foo)))

Attached patch resolve-ns-aliases-in-syntax-quote.patch contains the failing test and an attempted fix.



 Comments   
Comment by David Nolen [ 08/Apr/16 2:51 PM ]

This looks good, have you submitted a CA? Thanks.

Comment by Tom Jack [ 08/Apr/16 5:15 PM ]

Yeah, I'm "Thomas Jack" on the contributors list.

Comment by Mike Fikes [ 09/Apr/16 11:44 PM ]

The attached patch causes an issue for self-hosted ClojureScript for symbols in macros namespaces. One specific example is the aclone symbol referenced in the amap macro: With self-host, instead of resolving to cljs.core/aclone, it ends up with the macro-pseudonamespace: cljs.core$macros/aclone. Here is how to reproduce this:

Apply the patch and then start up script/noderepljs:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 51454
To quit, type: :cljs/quit
cljs.user=> (require 'cljs.js)
nil
cljs.user=> (def st (cljs.js/empty-state))
#'cljs.user/st
cljs.user=> (cljs.js/eval-str st "(def an-array (int-array 5 0))" nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value #js [0 0 0 0 0]}
cljs.user=>  (cljs.js/eval-str st "(amap an-array idx ret (+ 1 (aget an-array idx)))" nil {:eval cljs.js/js-eval :context :expr} identity)
WARNING: No such namespace: cljs.core$macros, could not locate cljs/core$macros.cljs, cljs/core$macros.cljc, or Closure namespace "" at line 1 
WARNING: Use of undeclared Var cljs.core$macros/aclone at line 1 
{:error #error {:message "ERROR", :data {:tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot read property 'call' of undefined]}}

This affects the aclone symbol as well as several references to get, as far as I can tell.

Comment by Mike Fikes [ 09/Apr/16 11:48 PM ]

I forgot to add, apart from the regression issue described in the previous comment, the patch does fix the intended use case described in the ticket under self-hosted ClojureScript (causing aliases to expand properly).

Comment by Tom Jack [ 10/Apr/16 12:23 PM ]

D'oh, thanks!

Patch self-host-resolve-ns-aliases-in-syntax-quote.patch adds your example as a failing self-host/test, and fixes it in the stupidest way: just trim off "$macros" in resolve-symbol.

Not sure if this stupid fix is acceptable..

Comment by Mike Fikes [ 10/Apr/16 4:05 PM ]

Yeah, Tom. I suspect that's a little heavy-handed. Here is a specific case it breaks in bootstrap:

Let's say you have this macros namespace:

(ns foo.macros)

(defn add*
  [a b]
  (+ a b))

(defmacro add
  [a b]
  `(add* ~a ~b))

The add* symbol needs to resolve properly and the $macros suffix is key. Without this latest patch, it does, but with it, it fails to do so. Here is the way to reproduce the issue with your patch:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 57728
To quit, type: :cljs/quit
cljs.user=> (require 'cljs.js)
nil
cljs.user=> (def st (cljs.js/empty-state))
#'cljs.user/st
cljs.user=> (cljs.js/eval st
'(ns foo.core (:require-macros foo.macros))
{:context :expr
 :eval cljs.js/js-eval
 :load         (fn [_ cb]
                 (cb {:lang   :clj
                          :source "(ns foo.macros) (defn add* [a b] (+ a b)) (defmacro add [a b] `(add* ~a ~b))"}))}
identity)
{:value nil}
cljs.user=> (cljs.js/eval st
'(foo.macros/add 1 2)
{:context :expr
 :eval cljs.js/js-eval}
identity)
WARNING: No such namespace: foo.macros, could not locate foo/macros.cljs, foo/macros.cljc, or Closure namespace ""
WARNING: Use of undeclared Var foo.macros/add*
TypeError: Cannot read property 'add_STAR_' of undefined
    at eval (eval at cljs$js$js_eval (/Users/mfikes/Projects/planck/planck-cljs/clojurescript/.cljs_node_repl/cljs/js.js:131:13), <anonymous>:1:11)
    at cljs$js$js_eval (/Users/mfikes/Projects/planck/planck-cljs/clojurescript/.cljs_node_repl/cljs/js.js:131:8)
    at cljs$js$eval_STAR_ (/Users/mfikes/Projects/planck/planck-cljs/clojurescript/.cljs_node_repl/cljs/js.js:1168:146)
    at Function.cljs.js.eval.cljs$core$IFn$_invoke$arity$4 (/Users/mfikes/Projects/planck/planck-cljs/clojurescript/.cljs_node_repl/cljs/js.js:1247:27)
    at cljs$js$eval (/Users/mfikes/Projects/planck/planck-cljs/clojurescript/.cljs_node_repl/cljs/js.js:1233:21)
    at repl:1:102
    at repl:9:3
    at repl:14:4
    at Object.exports.runInThisContext (vm.js:54:17)
    at Domain.<anonymous> ([stdin]:41:34)
cljs.user=>

Without your patch, the last bit will return.

{:value 3}
Comment by Tom Jack [ 10/Apr/16 5:16 PM ]

OK, the issue is beyond my comprehension, then. Your example does not make sense to me – I was surprised that it works as of master.

I will probably let someone who understands self-hosted cljs resolve the issue before I understand it enough to resolve the issue myself.

Comment by Mike Fikes [ 10/Apr/16 7:01 PM ]

Tom, part of me thinks your first patch is correct, and that it is aclone that needs to be qualified as cljs.core/aclone (and similarly for calls to get). I'll try an experiment along these lines and report back.

Comment by Mike Fikes [ 11/Apr/16 10:25 PM ]

The attached {{CLJS-1612-aux.patch}}, when applied with resolve-ns-aliases-in-syntax-quote.patch, results in the compiler core unit tests passing under bootstrap ClojureScript.

At the heart of what needs to be considered here is illustrated in the use of aclone in the amap macro. In particular, under bootstrap ClojureScript, syntax-quoting function symbol like aclone, when used in a macro, result in it being qualified with the current macro pseudo-namespace. (While this does not occur today for aclone, it does occur for this use case in bootstrap outside of cljs.core, so there is a tangential issue regarding cljs.core perhaps getting special treatment. But, regardless, with resolve-ns-aliases-in-syntax-quote.patch, the pseudo-namespace comes into play.)

So, if qualified as cljs.core$macros/aclone, this makes it so that aclone could be a regular function in the macros namespace, called by the macro. This is like the add* function in the example in the Bootstrapped ClojureScript portion at the bottom of http://blog.fikesfarm.com/posts/2016-01-05-clojurescript-macros-calling-functions.html

But, aclone is not a function in the macro namespace. It is a function in the runtime namespace. So, it needs to be qualified as such, as, cljs.core/aclone and the attached patch does just that. The attached patch does this for other function symbols that need the same treatment.

Now, the above analysis is based simply on the current behavior of the compiler, and not based on any notion of how the compiler should behave. An alternate view could be that the $macros suffix should never appear in qualified names, and resolution should just find the correct symbol. (So, a dual argument here is the fact that or can be called as cljs.core/or or cljs.core$macros/or, and both just work somehow, with the ideal publicized behavior being that the $macros suffix is an internal implementation detail, and users should always use forms like cljs.core/or. With this view, we'd instead strive to make self-host-resolve-ns-aliases-in-syntax-quote.patch work.

If we do want to go with the patch attached to this ticket, there are a few things we'd need to do:

1. Combine the patches into a single one. (Presuming a single patch is the only way it would be accepted.) If so, I'd have no problem if Tom wanted to combine this work into his and submit it under his name. (I've signed the CA, so this is OK.)
2. The only reason I currently know this patch works is because I built ClojureScript with it and ran the compiler test suite in bootstrap ClojureScript, but with a downstream bootstrap environment (Planck). I'd feel more comfortable if we added specific tests to the self-host test suite to provoke the failures that are needed to warrant the changes in this patch. (In other words, bootstrap is kind-of unacceptably fragile if we need to rely on something like Planck to ensure it is working correctly—the needed tests need to be with the ClojureScript compiler—ideally one day we'd get to the point where we can run the entire compiler's unit tests in bootstrap mode, but we aren't there now.)

Comment by Mike Fikes [ 17/Apr/16 7:52 AM ]

I'll take a stab at putting the above together.

Comment by Mike Fikes [ 17/Apr/16 10:24 AM ]

CLJS-1612.patch includes Tom Jack's original change plus additional revisions for bootstrap.

Comment by David Nolen [ 23/Apr/16 1:59 PM ]

The patch no longer applies to master, can we rebase?

Comment by Tom Jack [ 23/Apr/16 2:28 PM ]

Hmm, Mike's latest patch CLJS-1612.patch seems to apply to master for me.

$ git fetch -v --all
Fetching origin
From https://github.com/clojure/clojurescript
 = [up to date]      master     -> origin/master
 ...
$ git reset --hard origin/master
HEAD is now at 28e040d CLJS-1626: cljs.test for bootstrap
$ git am --keep-cr -s --ignore-whitespace < CLJS-1612.patch
Applying: CLJS-1612: Resolve ns aliases in syntax quote

Thanks, Mike, by the way, for the patch and explanation!

Comment by Mike Fikes [ 23/Apr/16 2:41 PM ]

Yeah, I can confirm what Tom's seeing. Here's my alternate verification:

$ cd /tmp
$ git clone https://github.com/clojure/clojurescript
Cloning into 'clojurescript'...
remote: Counting objects: 27250, done.
remote: Total 27250 (delta 0), reused 0 (delta 0), pack-reused 27250
Receiving objects: 100% (27250/27250), 10.25 MiB | 3.67 MiB/s, done.
Resolving deltas: 100% (12667/12667), done.
Checking connectivity... done.
$ cd clojurescript
$ curl -L http://dev.clojure.org/jira/secure/attachment/15609/CLJS-1612.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8472  100  8472    0     0  13148      0 --:--:-- --:--:-- --:--:-- 13155

After this, script/test, script/test-self-host, and script/test-self-parity all pass as well.

Comment by David Nolen [ 23/Apr/16 3:18 PM ]

fixed https://github.com/clojure/clojurescript/commit/1e794f0a9d6c03b12644f00cd8aa4f8a3e86ab83. Not sure why the patch wasn't applying for me before.





[CLJS-1627] jsdoc parsing fails to recognize union types, breaking resolution Created: 18/Apr/16  Updated: 23/Apr/16

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

Type: Defect Priority: Minor
Reporter: Patrick Killean Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: patch

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

 Description   

The Closure Spec For Union Types states that parentheses are necessary for union type expressions. Trying this ...

(defn foo
  "@param {(IBar|IMap)} x"
  [x] 
  ...)

Raises a Closure Error :

...ERROR - Bad type annotation. expected closing }
* @param {user.(IBar|user.IMap)}

This is because comp/resolve-types treats the parentheses as a part of the type tokens and incorrect var resolution occurs as a result. In addition, the compiler emits multiple resolved types separated by "|" characters but does not enclose them in parentheses to create a valid union type.



 Comments   
Comment by Patrick Killean [ 18/Apr/16 4:36 PM ]

This patch includes:

  • comp/resolve-types now removes parentheses when present and emits them when >1 type is detected. This makes parenthesis use optional and existing code remains unbroken (with the added benefit that it may work now)
  • changes to comp/resolve-type
    1. checks for js globals like document or window which are recognized by closure
    2. allows dot.delimited.forms to pass through so we can use types defined in externs and avoid unnecessary resolution
    3. uses ana/resolve-existing-var with a "unresolved jsdoc type" warning
    4. checks if a resolved var is a protocol and warns otherwise. This is more informative than Closure's standard unrecognized type error
  • a test for comp/resolve-types
Comment by David Nolen [ 21/Apr/16 12:45 PM ]

Thanks will try to look more closely at this tomorrow.

Comment by David Nolen [ 23/Apr/16 2:03 PM ]

The patch is getting there, please remove the `js-doc-type` meta stuff. Just extend the signature of resolve-existing-var to take an additional parameter - the confirm-var-exists handler.





[CLJS-1623] using `with-redefs` to add meta to a function via `with-meta` fails when using advanced compilation Created: 12/Apr/16  Updated: 23/Apr/16

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

Type: Defect Priority: Minor
Reporter: Justin Cowperthwaite Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Given the following:

(ns test.cljs)

(enable-console-print!)

(defn print-foobar [] (println "foobar"))

(defn test-redefs-meta []
  (print-foobar)
  (with-redefs [print-foobar (with-meta print-foobar {:foo "bar"})]
    (print-foobar)))

(test-reefs-meta)

running with `:optimizations :none`, it correctly prints:

foobar
footer

However, running with `:optimizations :advanced`, it prints:

foobar
main.js:232 Uncaught TypeError: te is not a function(anonymous function) @ main.js:232

Reproduced on r1.8.40 and current master (29eb8e0).



 Comments   
Comment by Justin Cowperthwaite [ 14/Apr/16 5:42 PM ]

it seems that the actual issue is with having :static-fns true (as is default under :optimizations :advanced)





[CLJS-1630] Add unit test for static dispatch Created: 21/Apr/16  Updated: 23/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Yehonathan Sharvit Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1630.patch    

 Description   

This unit test is an edge case that illustrates why in the code of `emit :invoke` we must stay with `call` for the high order case where static information is missing .






[CLJS-1628] Make instances of js/Symbol printable Created: 20/Apr/16  Updated: 21/Apr/16

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

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

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

 Description   

At the moment instances of js/Symbol are not printable. Trying to
print them or enter them in the REPL causes an exception.

cljs.user> (.for js/Symbol "react.element")
#object[TypeError TypeError: Cannot convert a Symbol value to a string]

Symbols are supported in all major browsers, except Internet Explorer and Nashorn.
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol



 Comments   
Comment by Roman Scherer [ 20/Apr/16 10:23 AM ]

The attached patch adds an additional clause to `pr-writer-impl` and
implements the printing of Symbol instances, in the same way as it is
done for other JavaScript objects. Here's an example of a printed
js/Symbol:

(.for js/Symbol "react.element")
;;=> #object[Symbol "react.element"]

@david: Regarding your point about shims, do you think the
implementation of `js-symbol-defined?`, which was used for the ES6
iterator support, is enough for this patch? I'm not too familiar with
JavaScript and not sure if this already addressed the "shim" issue.

Another thing I stumbled upon is, that my test currently generates a
compiler warning when using the default compiler options. The warning
is generated when compiling the following ClojureScript form:

(.for js/Symbol "react.element")

The following snippet shows the warning and the generated code from my
test:

WARNING - Keywords and reserved words are not allowed as unquoted
property names in older versions of JavaScript. If you are targeting
newer versions of JavaScript, set the appropriate language_in option.

try{var values__13328__auto__ = (function (){var x__6628__auto__ = cljs.core.pr_str.cljs$core$IFn$_invoke$arity$variadic(cljs.core.array_seq([Symbol.for("react.element")], 0));

I think this has nothing to do with this patch, but with the emitted
code not being legal Ecmascript 3, since "for" is a reserved word.

The warning goes away when changing the :language-in option to
something newer than Ecmascript 3, or doing something like this:

((gobj/get js/Symbol "for") "react.element")

So, the questions is: Should the ClojureScript compiler handle those
reserved words when they appear in a function call or property lookup?

If that's the case I would leave the warning in that patch, and open
another issue for this new problem.

What do you think?

Roman

Comment by Roman Scherer [ 20/Apr/16 11:33 AM ]

Ok, given advanced mode and renaming of symbols I think there's not much the ClojureScript compiler can do about this.
I think I'll change the test to use ((gobj/get js/Symbol "for") "react.element") to remove the warning.
https://github.com/google/closure-compiler/wiki/FAQ#i-get-invalid-property-id-errors-but-it-works-on-firefox

Comment by David Nolen [ 20/Apr/16 2:21 PM ]

ECMAScript 3 is not an issue for people who are ok with outputting ECMAScript 5. The goog.typeOf of check on "symbol" does not seem like it would work with Symbol shims, have you confirmed?

Comment by Roman Scherer [ 21/Apr/16 10:57 AM ]

David, yes you are right about goog.typeOf, it returns "object" when
js/Symbol is implemented via shims. I updated the patch and tested it
against the following shim implementation in Nashorn so far.

Start a Nashorn REPL.

./script/nashornrepljs

js/Symbol does not exist in Nashorn yet.

(exists? js/Symbol)
;;=> false

Load core-js shim into Nashorn.

(js/load "https://raw.githubusercontent.com/zloirock/core-js/master/client/shim.min.js")
(print (.for js/Symbol "x"))
;;=> #object[Symbol "x"]

I gave up on this shim implementation because I got an error when
trying to load the minified js into Nashorn.

https://github.com/medikoo/es6-symbol

This is another shim implementation, but it only enhances js/Symbol
when it already exists, so not useful in the context of Nashorn.

https://github.com/paulmillr/es6-shim

Comment by Roman Scherer [ 21/Apr/16 2:07 PM ]

Which implementations do support js/Symbol?

JavaScriptCore, NodeJS, SpiderMonkey, Chrome and Firefox support
js/Symbol. Evaluating (instance? js/Symbol (.for js/Symbol "x")) on
those engines returns false. On all those implementations symbols
generated via (.for js/Symbol "x") are primitive values and can be
identified via typeof. See the "Using the typeof operator with
symbols" section in [1].

Nashorn and Rhino do not support js/Symbol.

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Comment by Roman Scherer [ 21/Apr/16 2:27 PM ]

Looking around I found that the exception caused by printing symbols
happens because the str [1] function uses implicit string conversion
to convert it's argument into a string. This is explained in the
"Coercing a symbol to string" section of [2]. One way to solve the
problem is to use (.toString x) instead.

(.join #js [(.for js/Symbol "x")] "")
;;=> TypeError: Symbol is not a constructor

(.toString (.for js/Symbol "x"))
;;=> "Symbol(x)"

[2] https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/core.cljs#L2695
[2] http://www.2ality.com/2014/12/es6-symbols.html

Comment by Roman Scherer [ 21/Apr/16 2:38 PM ]

(= (goog/typeOf x) "symbol") vs (instance? js/Symbol x)

Symbols are primitive types and must be constructed via
Symbol("x"). They can't be constructed via new Symbol("x"), trying
to do so raises a TypeError. This is explained in more detail in the
"Safety checks" section of [1]. The "Using the typeof operator with
symbols" section of [2] explains how to identify symbols. It has to be
done via typeof, because symbols are primitive types and not
instances of Symbol.

[1] http://www.2ality.com/2014/12/es6-symbols.html
[2] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Comment by Roman Scherer [ 21/Apr/16 3:02 PM ]

I think we have 2 options now to fix exceptions when printing symbols:

  • Change str to handle symbols as well.
  • Implement custom printing for symbols.

I think I would go for the first option. I think calling str on a
symbol can't be seen as implicitly coercing a symbol to a string, I
would say this is as explicit as it can get and we don't have to raise
an exception in this case. I think str is basically the Clojure
equivalent to toString in JavaScript, at least this is what the out
of date doc string of str says.

The file CLJS-1628-fix-str has an implementation of the first option.

Comment by Francis Avila [ 21/Apr/16 3:35 PM ]

Changing str to use .toString is not possible because of CLJS-847: there are unconfirmable reports that .toString() causes TypeError on Safari 6.0.5.

This problem has spilled over into the unresolved CLJS-890. This is a long thread, so see my recap comment, my failed efforts to reproduce the bug and my proposed final patch, which can be easily extended for the symbol case with a new "symbol" clause in the case statement.

Comment by Roman Scherer [ 21/Apr/16 4:20 PM ]

Thanks Francis, I'll take a look at this discussion.

Comment by Roman Scherer [ 21/Apr/16 5:12 PM ]

Since one can still make so called Symbol wrapper objects via the
Object() function [1], I added one more test case for this:

(js/Object (.for js/Symbol "x"))

Also the updated patch defines the test case only if js/Symbol exists?
in the JavaScript engine.

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol





[CLJS-1631] The str function should handle JavaScript symbols Created: 21/Apr/16  Updated: 21/Apr/16

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

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


 Description   

The str function should handle primitive JavaScript symbols produced by Symbol.for. At the moment the str function raises an exception, because it runs into some JavaScript safety checks by using implicit string coersion via (.join #js [x] "").
More info on the safety check here: http://www.2ality.com/2014/12/es6-symbols.html
This ticket is also related to:

(def x (.for js/Symbol "x"))
(str x)
TypeError: Cannot convert a Symbol value to a string
    at Array.join (native)
    at Function.cljs.core.str.cljs$core$IFn$_invoke$arity$1 (/home/roman/workspace/clojurescript/.cljs_node_repl/cljs/core.js:9561:12)
    at Object.cljs$core$str [as str] (/home/roman/workspace/clojurescript/.cljs_node_repl/cljs/core.js:9543:22)
    at repl:1:100
    at repl:9:3
    at repl:14:4
    at Object.exports.runInThisContext (vm.js:54:17)
    at Domain.<anonymous> ([stdin]:41:34)
    at Domain.run (domain.js:228:14)
    at Socket.<anonymous> ([stdin]:40:25)

Calling the toString method on a symbol directly works

(.toString x)
;;=> "Symbol(x)"





[CLJS-1626] cljs.test for bootstrap Created: 18/Apr/16  Updated: 21/Apr/16  Resolved: 21/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1626-1.patch    
Patch: Code

 Description   

Make it so that the cljs.test namespace can work with bootstrap ClojureScript.

Currently the known obstacles that prevent this largely surround macros, but they are known to be solvable. (It has been done successfully downstream in Planck.)

Primary goals:

  • Preserve existing functionality and performance for regular JVM ClojureScript users of cljs.test
  • Make it so that cljs.test namespace can be loaded in any bootstrap environment that makes use of cljs.js and implements load functions supporting macros.
  • Add scripted facilities so that the ClojureScript compiler's unit tests can be executed (perhaps in Node if installed) as an auxiliarry test capability (like script/test but for self-host)

Secondary goals:

  • Ensure that it is possible for downstream ClojureScript projects that target bootstrap can make use cljs.test to run their test suites in bootstrap mode. (The ClojureScript compiler wouldn't provide any scripts for this—it would just strive to ensure that this is possible.)
  • Support custom asserts assert-expr in bootstrap mode which are written in ClojureScript (as compared to Clojure for the JVM-based approach). (This has been shown to be possible with downstream Planck.)

Rationale:

While bootstrap is of lower priority than regular JVM-based ClojureScript, it is still a burden to support it: It is easy to make a change to the compiler, but forget a conditional branch for bootstrap, or some such, thus causing a regression for bootstrap. With this capability, we'd have the ability to run the very same compiler unit tests in bootstrap mode, thus drastically reducing the support burden for bootstrap's existence.

Secondarily, this would help downstream libraries confidently target bootstrap as an environment by facilitating those libraries in running their own unit tests using cljs.test.



 Comments   
Comment by Mike Fikes [ 20/Apr/16 2:10 PM ]

Attaching CLJS-1616-1.patch for feedback. (It might be good to go.)

It makes some very minor changes to the production code so that cljs.test can be loaded in bootstrap environments.

The renames

src/main/cljs/cljs/test.clj → src/main/cljs/cljs/test.cljc
src/main/clojure/cljs/analyzer/api.clj → src/main/clojure/cljs/analyzer/api.cljc

actually involve very few changes after renames.

In addition to running the regular ClojureScript tests, I've confirmed that you can build an uberjar and still properly {{(require '[cljs.test :refer-macros [deftest is]])}} in a REPL. I've also confirmed that tools.reader, which runs CLJS tests can still do so with these changes.

In addition it adds a new script/test-self-parity which will run (most of) the compiler unit tests in bootstrap mode in Node. This is done by bringing up an :optimizations :none environment in Node and using the cljs.js capability to load the compiler unit tests along with cljs.test. Of interest is that this script pulls out the clojure.template namespace so it can be loaded. (Downstream projects that might end using script/test-self-parity as an example of how to run their own unit tests in bootstrap mode would need to do something similar.)

(Note: If we can come up with a better name than "parity", willing to revise it. script/test-self-host was taken.)

This patch does not address writing custom assert assert-expr in ClojureScript by treating the defaulti as being in the cljs.test namespace (as is illustrated in http://blog.fikesfarm.com/posts/2016-02-25-custom-test-asserts-in-planck.html), but I did confirm that you can easily achieve the same by simply working with cljs.test$macros/assert-expr.

Comment by Mike Fikes [ 21/Apr/16 10:14 AM ]

I've tested this patch using a downstream example repository that illustrates testing a library targeting bootstrap. It also makes use of a custom test assert. This shows that all of the secondary goals in the ticket description are met.

(The downstream repo, for those interested: https://github.com/mfikes/titrate)

Comment by David Nolen [ 21/Apr/16 12:43 PM ]

fixed https://github.com/clojure/clojurescript/commit/28e040d41f3a83901b6391898f51c4bfc2622452





[CLJS-1519] Collection invoke errors off by 1 Created: 22/Dec/15  Updated: 19/Apr/16

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Mike Jackson
Resolution: Unresolved Votes: 0
Labels: newbie


 Description   

Runtime collection invokes will report arity that is off by one. This is because we use the generic function arity dispatching logic which doesn't account for the 1st self argument.



 Comments   
Comment by Mike Jackson [ 15/Apr/16 1:02 AM ]

Hey David,

Can I pick this one up? I'm a first time contributor and I wouldn't mind using this to get a lay of the land. I've already signed the Contributor Agreement.

Cheers

Comment by David Nolen [ 19/Apr/16 2:12 PM ]

Mike, I've updated your permissions. Please assign the ticket to yourself. Thanks!

Comment by Mike Jackson [ 19/Apr/16 2:42 PM ]

Awesome, thanks. Looking forward to it.





[CLJS-1621] Foreign libs modules of different type doesn't compile together Created: 09/Apr/16  Updated: 19/Apr/16  Resolved: 19/Apr/16

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

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

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

 Description   

Given the following config

{:file "my-modular-lib/logger.js"
  :provides ["logger"]
  :module-type :commonjs}
 {:file "my-modular-lib/index.js"
  :provides ["msglogger"]
  :module-type :es6}

and these JavaScript modules

logger.js

exports.default = function log(msg) {
  return console.log(msg);
};

index.js

import log from './logger';

export function logMsg(msg) {
  return log(msg);
};

compiler throws an error

ERROR: JSC_ES6_MODULE_LOAD_ERROR. Failed to load module "./logger" at my-modular-lib/index.js line 1 : 10

Closure Compiler is not able to find logger.js because it is not included into compilation process of index.js file.
This happens because foreign libs are being filtered out by module type at closure.clj#L1501
Changing filtering to something like this: (filter #(or (= (:module-type %) :es6) (= (:module-type %) :commonjs))) makes build pass successfully.



 Comments   
Comment by Roman Liutikov [ 11/Apr/16 2:55 AM ]

This patch allows compiling foreign libs with dependencies of different module types (AMD, CommonJS, and ES6).

Comment by Mike Fikes [ 13/Apr/16 11:44 AM ]

I successfully tested this in 2 ways:

1) It doesn't break downstream bootstrap Planck (theoretically it can't, but tested anyway).
2) I tested to ensure that this works dynamically in a REPL (per CLJS-1313). There is one probably innocuous warning, but things appear to work properly:

$ java -jar cljs.jar 
Clojure 1.8.0
user=> (require
  '[cljs.repl :as repl]
  '[cljs.repl.node :as node])
nil
user=> (def foreign-libs [{:file "my-modular-lib/logger.js"
  :provides ["logger"]
  :module-type :commonjs}
 {:file "my-modular-lib/index.js"
  :provides ["msglogger"]
  :module-type :es6}])
#'user/foreign-libs
user=> (cljs.repl/repl (node/repl-env) :foreign-libs foreign-libs)
ClojureScript Node.js REPL server listening on 49586
WARNING: JSC_INVALID_ES3_PROP_NAME. Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option. at my-modular-lib/logger.js line 1 : 8
To quit, type: :cljs/quit
cljs.user=> (require '[msglogger :refer [logMsg]])
nil
cljs.user=> (logMsg "Hi")
Hi
nil
Comment by Roman Liutikov [ 13/Apr/16 2:02 PM ]

I have also tested it with Leiningen. Having JS modules which depends on modules of different type compilation is successfull without optimizations and with advanced level.

Comment by Maria Geller [ 13/Apr/16 5:16 PM ]

Also tested it with my personal projects which use JS modules and it looks good for me.

Comment by David Nolen [ 19/Apr/16 2:15 PM ]

fixed https://github.com/clojure/clojurescript/commit/56611b5f653e5b8cbeaff9d975cf9f73b755f9c3





[CLJS-1624] Avoid the use of JSC_HOME for tests Created: 15/Apr/16  Updated: 19/Apr/16  Resolved: 19/Apr/16

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

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

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

 Description   

For script/test, we have devs set JSC_HOME and make use of this to decide when to run tests with jsc. (See https://github.com/clojure/clojurescript/wiki/Running-the-tests)

There is a recent change in WebKit (http://trac.webkit.org/changeset/194606) which causes a warning to appear in command-line apps that make use of JavaScriptCore (Planck is an example), essentially indicating that JCS_HOME=/path... is not a valid option.

It turns out that JSC_HOME is not needed in order to run jsc. If we simply avoid using JSC_HOME altogether, and instead check if jsc is on the path (which occurs when the PATH is set up per the Running the Tests wiki page, then all is good.



 Comments   
Comment by Mike Fikes [ 15/Apr/16 9:26 AM ]

Successfully tested the changes in CLJS-1624.patch on both OS X and Linux. For Linux, tested on Ubuntu 14.04 with the libjavascriptcoregtk-3.0-bin package installed, and it finds jsc on the path and the compiler unit tests successfully run.

Comment by David Nolen [ 19/Apr/16 2:11 PM ]

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





[CLJS-1625] Clojurescript macros used in named function are expanded two times because the analyzer performs a two pass analysis when analyzing named functions Created: 16/Apr/16  Updated: 19/Apr/16

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

Type: Defect Priority: Minor
Reporter: Ewen Grosjean Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1625.patch    

 Comments   
Comment by Ewen Grosjean [ 16/Apr/16 9:41 AM ]

During the first analysis of named functions, only the function definition is analyzed in order to know its function-ness/arity. Its body is only analyzed during the second pass.

Comment by Kevin Downey [ 17/Apr/16 12:09 AM ]

http://dev.clojure.org/jira/browse/CLJS-1617 seems to add a similar issue





[CLJS-1617] Evaluation order of arguments to 'list' is right-to-left Created: 07/Apr/16  Updated: 17/Apr/16  Resolved: 08/Apr/16

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

Type: Defect Priority: Major
Reporter: Sam Roberton Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   
(do
    (.log js/console "Evaluation order test")
    (let [result (list
                  (or
                    (.log js/console "1")
                    1)
                  (or
                    (.log js/console "2")
                    2))]
      (.log js/console "Result: " (pr-str result))))

prints

Evaluation order test
2
1
Result:  (1 2)


 Comments   
Comment by Kevin Downey [ 07/Apr/16 1:46 PM ]

`list` is sometimes a function and sometimes a macro in clojurecript, I am not sure how the compiler chooses between them, but this is behavior is the result of the macro version.

replacing the macro with something like

(core/defmacro list
  ([] '(.-EMPTY cljs.core/List))
  ([x & xs]
     `(let [x# ~x]
        (-conj (list ~@xs) x#))))

should fix it

Comment by Kevin Downey [ 07/Apr/16 1:49 PM ]

why the macro version exists, I have no idea

Comment by David Nolen [ 08/Apr/16 2:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/29eb8e014f60d57c974fef46f8609ca0be0fd247

Comment by Kevin Downey [ 08/Apr/16 7:28 PM ]

this fix seems like it would cause macros as arguments to list to be expanded twice, which I know causes problems in clojure (a number of the macros in clojure.core have side effects at expansion time), maybe it is a bad idea in clojurescript as well

Comment by Mike Fikes [ 09/Apr/16 7:57 PM ]

Hey Kevin, the ~x appears only once on both branches of the if so evaluation should occur once. This test pans out:

(let [x (atom 0)]
  [(list (swap! x inc) (swap! x inc)) @x])

properly yielding [(1 2) 2]. (FWIW, an example of double evaluation in a macro is in CLJS-1518, which is solvable there by letting once as is done in this ticket's fix.)

Comment by Kevin Downey [ 10/Apr/16 1:27 AM ]

I am not talking about double evaluation, I am talking about double macro expansion.

The analyzer does macro expansion so the list macro that calls the analayzer is sort of like

(defmacro x [y]
  (macroexpand y)
  y)

y will then be macroexpanded again as compilation continues, so macroexpansion happens twice.

I suspect this is more of an issue in clojure than it is in clojurescript, because the compilation environment is isolated from the runtime environment in clojurescript. In clojure while hacking on the compiler, it would be very nice to analyze a form and then make decisions based on that analysis, and then re-analyze the form differently, but the double macro expansion breaks things (a long with other stateful bits in the compiler). Regardless it seems like a bad idea.

Comment by Mike Fikes [ 10/Apr/16 7:38 AM ]

Ahh, sorry Kevin. I completely missed what you were saying earlier.

So, perhaps your concern does come in to play with bootstrap ClojureScript where there is no separation (macros are expanded in the runtime environment, so side effects could have a consequence).

FWIW, I ran the ClojureScript compiler's unit tests in bootstrap ClojureScript with this change and they pass.

I suppose one corollary example of what you are saying is that it is no longer possible to evaluate

(macroexpand '(list (when-let) 1))
Comment by Kevin Downey [ 17/Apr/16 12:08 AM ]

http://dev.clojure.org/jira/browse/CLJS-1625 is a similar issue





[CLJS-1604] Self-host: cljs.js/compile-str causes a javascript error Created: 19/Mar/16  Updated: 14/Apr/16

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

Type: Defect Priority: Minor
Reporter: Yehonathan Sharvit Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bootstrap, bug


 Description   

When requiring `cljs.js` and calling `cljs.js/compile-str` with `:optimizations :advanced`
I get the following error in the browser:
"Uncaught TypeError: Cannot set property 'rd' of undefined"

Steps to reproduce:

1. Make a directory
2. Copy shipping cljs.jar into the directory
3. Make an index.html, src/hello_world/core.cljs, and build.clj file with contents as below.
4. java -cp cljs.jar:src clojure.main build.clj
5. Open index.html with Chrome and view the JavaScriptConsole in Chrome.

index.html:

<html>
<body>
<script type="text/javascript" src="out/main.js"></script>
</body>
</html>
src/hello_world/core.cljs:
(ns hello-world.core
(:require [cljs.js :as cljs]))

(set! (.. js/window -cljs -user) #js {})

(cljs/compile-str (cljs/empty-state) "" indentity)

build.clj:

(require 'cljs.build.api)

(cljs.build.api/build "src"
{:output-to "out/main.js"
:optimizations :whitespace})

(System/exit 0)



 Comments   
Comment by Yehonathan Sharvit [ 19/Mar/16 5:31 PM ]

I need to fix the title of the issue: "Self-host: in advanced compilation - cljs.js/compile-str causes a javascript error"

Comment by Mike Fikes [ 30/Mar/16 11:14 PM ]

You can only use up to :optimizations :simple with self-host. See https://github.com/clojure/clojurescript/wiki/Optional-Self-hosting#production-builds

Discussion: One rationale for this is that the emitted code, in order to be executable, needs access to non-Closure-munged/DCEd symbols from the standard ClojureScript lib. Perhaps this limitation need only exist for eval-str, (while not for compile-str, analyze-str, etc.)

Comment by Mike Fikes [ 14/Apr/16 7:02 AM ]

I'd recommend closing this as declined (no plans exist to support self-host with :advanced).





[CLJS-1609] Self-host: Expressions like `(+ 42)` and `(let [x 42] x)` are evaluated to nil. Created: 26/Mar/16  Updated: 14/Apr/16  Resolved: 14/Apr/16

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

Type: Defect Priority: Minor
Reporter: Yehonathan Sharvit Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: bootstrap


 Description   

Expressions like `(+ 42)` and `(let [x 42] x)` are evaluated to nil.

(cljs/eval-str (cljs/empty-state) "(+ 42)" 'test {:eval cljs/js-eval} identity); => {:ns cljs.user, :value nil}

while

(cljs/eval-str (cljs/empty-state) "(+ 0 42)" 'test {:eval cljs/js-eval} identity); => {:ns cljs.user, :value 42}

(cljs/eval-str (cljs/empty-state) "(let [x 42] x)" 'test {:eval cljs/js-eval} identity); => {:ns cljs.user, :value nil}

while
(cljs/eval-str (cljs/empty-state) "[(let [x 42] x)]" 'test {:eval cljs/js-eval} identity); => {:ns cljs.user, :value [42]}



 Comments   
Comment by Mike Fikes [ 30/Mar/16 10:39 PM ]

If you are evaluating ClojureScript source that reduces expressions to values, then you should add :context :expr to the opts map. (See CLJS-1357.) Otherwise, I believe the default will be :statement. (See https://github.com/clojure/clojurescript/blob/c72e9c52156b3b348aa66857830c2ed1f0179e8c/src/main/clojure/cljs/analyzer.cljc#L512)

Comment by Yehonathan Sharvit [ 04/Apr/16 12:35 PM ]

I tried the following, but it didn't work

(def st (cljs/empty-state (fn [state] (assoc-in state [:options :context] :expr))))
(cljs/eval-str st "(let [x 2])" "bla" {:eval cljs/js-eval} identity); =>{:ns cljs.user, :value nil}

Comment by Mike Fikes [ 10/Apr/16 12:14 AM ]

Sorry, by opts, I was referring to the 4th argument to cljs.js/eval-str. Here's an example:

cljs.user=> (require '[cljs.js :as cljs])
nil
cljs.user=> (def st (cljs/empty-state))
#'cljs.user/st
cljs.user=> (cljs/eval-str st "(let [x 2] x)" "bla" {:eval cljs/js-eval :context :expr} identity)
{:ns cljs.user, :value 2}

On the other hand, if you explicitly set the context to be :statement, you will get the nil you reported:

cljs.user=> (cljs/eval-str st "(let [x 2] x)" "bla" {:eval cljs/js-eval :context :statement} identity)
{:ns cljs.user, :value nil}

I don't think this behavior is a bug for :statement context.

Comment by Yehonathan Sharvit [ 10/Apr/16 1:44 AM ]

Thanks Mike!
It works fine.

Comment by Mike Fikes [ 14/Apr/16 7:00 AM ]

Since it works, closing.





[CLJS-910] JavaScriptCore 0xbbadbeef EXC_BAD_ACCESS when evaluating (list 0 1 ... 18) Created: 16/Dec/14  Updated: 14/Apr/16  Resolved: 10/Nov/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: Declined 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

Comment by Timothy Pratley [ 24/Jul/15 8:38 AM ]

Just for reference: https://github.com/google/closure-compiler/issues/1049

Comment by David Nolen [ 10/Nov/15 9:57 AM ]

this is an upstream bug that we are not going to fix that has known solutions, closing for now.

Comment by Mike Fikes [ 13/Nov/15 6:41 AM ]

Collected summary:

For certain forms like (list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18), the generated JavaScript under :none involves nested call constructs provoking a known upstream O(2^n) perf issue in JavaScriptCore:
https://bugs.webkit.org/show_bug.cgi?id=139847

This can be worked around by passing the compiler option :static-fns true

Comment by Yehonathan Sharvit [ 14/Apr/16 1:23 AM ]

Why not making :static-fns true by default?





[CLJS-1619] cljs.test/is has trouble identifying functions Created: 07/Apr/16  Updated: 12/Apr/16

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

Type: Defect Priority: Minor
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In clojurescript, the `is` macro doesn't pretty print a predicate test defined with "def".

In the example below, I would expect "(is (amap? []))" to report "actual: (not (amap? []))".

;; using clojure.test
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

user=> (is (map? []))

FAIL in () (form-init8298503787610781742.clj:1)
expected: (map? [])
actual: (not (map? []))

user=> (def amap? map?)
user=> (is (amap? []))

FAIL in () (form-init8298503787610781742.clj:1)
expected: (amap? [])
actual: (not (amap? []))

;; using cljs.test
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

user=> (is (map? []))

FAIL in () (at eval (eval at figwheel$client$utils$eval_helper (http:12:4)
expected: (map? [])
actual: (not (map? []))

user=> (def amap? map?)
user=> (is (amap? []))

FAIL in () (at eval (eval at figwheel$client$utils$eval_helper (http:11:4)
expected: (amap? [])
actual: false



 Comments   
Comment by David Nolen [ 08/Apr/16 1:12 PM ]

Please do not report problems that involve third party tooling. Can you replicate this problem without Figwheel?

Comment by Oliver George [ 08/Apr/16 7:32 PM ]

Hi David

Yes, repeatable with what I believe is a minimal setup. I used the mies template and scripts/build.clj to test with this script.

(ns testerr.core
  (:require [cljs.test :refer-macros [deftest is testing run-tests]]))

(enable-console-print!)

(is (map? []) "Expected a map")

(def amap? map?)

(is (amap? []) "Expected a map")

Output is as before:

FAIL in () (:)
Expected a map
expected: (map? [])
  actual: (not (map? []))

FAIL in () (:)
Expected a map
expected: (amap? [])
  actual: false
Comment by Mike Fikes [ 12/Apr/16 9:27 PM ]

This might pose a challenge—without considering a radical restructuring—in JVM ClojureScript because the not decoration is computed at macro expansion time, where only static analysis metadata is available.

Clojure, of course, has an easier time with this, accessing the runtime. FWIW, I confirmed that bootstrapped ClojureScript can analogously just as easily pull this off via an eval and a fn? check.





[CLJS-1622] `with-redefs` can cause `&` argument to be assigned incorrectly under advanced compilation Created: 12/Apr/16  Updated: 12/Apr/16

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

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


 Description   

Given the following:

(ns with-redefs-bug.core)

(enable-console-print!)

(defn a-function [arg-1 arg-2 & args]
  nil)

(with-redefs [a-function (fn [& args] args)]
  (prn (a-function :arg-1))
  (prn (a-function :arg-1 :arg-2))
  (prn (a-function :arg-1 :arg-2 :arg-3)))

Under :optimizations :none, this code correctly prints:

(:arg-1)
(:arg-1 :arg-2)
(:arg-1 :arg-2 :arg-3)

However, under :optimizations :advanced, this code prints:

(:arg-1)
(:arg-1 :arg-2)
:arg-1

That is, as long as the function is called with exactly or less than the number of non-variadic arguments in the original function bound to a-function, args is (correctly) bound to a seq of all the arguments, but if any more arguments are given, args is bound to the first argument.

Also, under either compilation, the following warning is generated:

WARNING: Wrong number of args (1) passed to with-redefs-bug.core/a-function at line 9

That surprises me, but since it happens under both methods, perhaps it's intentional.



 Comments   
Comment by Peter Jaros [ 12/Apr/16 4:21 PM ]

Reproduced on r1.8.40 and current master (29eb8e0).

Comment by Mike Fikes [ 12/Apr/16 8:24 PM ]

This is actually not specific to :optimizations :advanced, but to :static-fns true.





[CLJS-1618] `extend-type string` doesnt work without wrapping the string object into `(str)` Created: 07/Apr/16  Updated: 10/Apr/16

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

Type: Defect Priority: Minor
Reporter: Yehonathan Sharvit Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

(ns my.car)

(defprotocol Car
(drive [this]))

(extend-type js/String
Car
(drive [this] (map #({"a" "A"} %) [this (str this)])))

(drive "a"); (nil "A") expected ("A" "A")

See the reproduction of the bug in a live environment with KLISPE here: http://app.klipse.tech/?sourcce=jira&cljs_in=(ns%20my.car)%0A%0A(defprotocol%20Car%0A%20%20(drive%20%5Bthis%5D))%0A%0A(extend-type%20js%2FString%0A%20%20Car%0A%20%20(drive%20%5Bthis%5D%20(map%20%23(%7B%22a%22%20%22A%22%7D%20%25)%20%5Bthis%20(str%20this)%5D)))%0A%0A%0A(drive%20%22a%22)%0A



 Comments   
Comment by Francis Avila [ 07/Apr/16 6:27 PM ]

This is because of boxing and the implementation of cljs.core._EQ_

By extending type js/String (the String object/class in javascript) instead of "string", your "this" will be the boxed string rather than the primitive string (in non-strict js mode--in strict mode it will be the primitive also). The (str this) is coercing the boxed string back to a primitive string.

The core issue is really:

(= (js/String. "a") "a") ;=> false
;; thus
({"a" "A"} (js/String. "a")) ;=> nil

You should really use

(extend-type string ...)
Comment by Francis Avila [ 07/Apr/16 6:41 PM ]

BTW this appears to be different from Clojure where primitives and boxed-primitives appear equal:

;; Clojure code
(= (String. "a") "a")
=> true
(= (Long. 1) 1)
=> true
(= (Long. 1) (long 1))
=> true

Not sure if clojurescript should try to replicate this more closely or not.

Clojurescript bottoms out with triple-equals in most cases, which is why primitives and boxes do not compare equal. To get them to compare equal would require adding special (instance? js/BOXED x) checks and some modifications to existing -equiv implementations which extend primitive types. (e.g. (extend-type number IEquiv ...) uses identical? without checking if the right-hand side is boxed or not.)

Comment by Mike Fikes [ 10/Apr/16 12:24 AM ]

As Francis alludes to, this is not a bug. If you do (doc extend-type), it indicates the type-sym can be string to cover the base type js/String, and it elaborates with

Note that, for example, string should be used instead of js/String.
and if a user does try to use js/String as the type-sym argument, a diagnostic is issued:

WARNING: Extending an existing JavaScript type - use a different symbol name instead of js/String e.g string at line 1




[CLJS-1620] In JavaScript ES2015 modules default export name is munged to default$ Created: 08/Apr/16  Updated: 08/Apr/16

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

Type: Defect Priority: Minor
Reporter: Roman Liutikov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When using a foreign lib which is ES2015 module with default export, the value which is being exported is assigned to a property default on a namespace object. In ClojureScript code this means one should call to default var of that namespace. However in complied output of ClojureScript code the name default is getting munged to default$.

export default function inc(v) {
  return v + 1;
}
(ns cljs-example.core
  (:require [lib.inc :as lib]))

(lib/default 0)
goog.provide("module$lib$inc");
function inc$$module$lib$inc(v){return v+1}
module$lib$inc.default=inc$$module$lib$inc
// Compiled by ClojureScript 1.8.40 {}
goog.provide('cljs_example.core');
goog.require('cljs.core');
goog.require('module$lib$inc');
module$lib$inc.default$.call(null,(0));

//# sourceMappingURL=core.js.map


 Comments   
Comment by David Nolen [ 08/Apr/16 2:42 PM ]

One possible path around this is to respect the Closure Compiler language setting when munging instead of blindly munging ECMA-262 keywords. A patch that adopts this approach would be welcome.





[CLJS-1598] Honor printing of function values via IPrintWithWriter Created: 03/Mar/16  Updated: 08/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

If a user wishes to define how function values are printed, allow that to be controlled via IPrintWithWriter with code like

(extend-type function
  IPrintWithWriter
  (-pr-writer [obj writer opts]
    ,,,))


 Comments   
Comment by Mike Fikes [ 03/Mar/16 10:28 AM ]

Can be tested manually:

$ script/nashornrepljs 
To quit, type: :cljs/quit
cljs.user=> inc
#object[cljs$core$inc "function cljs$core$inc(x){
return (x + (1));
}"]
cljs.user=> (extend-type function
  IPrintWithWriter
  (-pr-writer [obj writer _]
    (let [name (.-name obj)
          name (if (empty? name)
                 "Function"
                 name)]
      (write-all writer "#object[" name "]"))))
#object[Function]
cljs.user=> inc
#object[cljs$core$inc]
Comment by David Nolen [ 11/Mar/16 1:04 PM ]

The problem is this makes printing slower. For people using EDN as interchange format this may be a problem. Would need to see some numbers.

Comment by Antonin Hildebrand [ 08/Apr/16 2:11 PM ]

I'm not sure what is the difference between implements? and satisfies?. But by reading the code I would assume that it should be printed by this line:
https://github.com/clojure/clojurescript/blob/9a2be8bc665385be1ef866e2fd76b476c417d2bf/src/main/cljs/cljs/core.cljs#L9056-L9057

Don't we want to change implements? to satisfies? there? Not sure about (perf) implications.





[CLJS-1614] Race condition in parse-ns can break compilation under parallel build Created: 03/Apr/16  Updated: 08/Apr/16  Resolved: 08/Apr/16

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

Type: Defect Priority: Major
Reporter: Nicolás Berger Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

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

 Description   

I found this issue by trying to compile reagent with parallel-build. Once parallel-build is enabled the compilation fails most of the times, creating warnings about different undeclared vars. The issue is not deterministic: the vars are usually different, and sometimes it even works fine, without warnings.

By running with :verbose true I found that the warnings always occur after a line about Reading analysis cache for file: <file name for a namespace with macros> (in the case of reagent, that's the reagent.interop namespace). For example:

[snip]
Compiling /home/nicolas/projects/reagent/src/reagent/dom.cljs
Reading analysis cache for file:/home/nicolas/projects/reagent/src/reagent/interop.cljs
WARNING: Use of undeclared Var reagent.dom/load-error at line 11 /home/nicolas/projects/reagent/src/reagent/dom.cljs
[snip]

After a lot of debugging I arrived at the following technical explanation and fix proposal (taken from the commit message in the attached patch):

parse-ns allows for running the parse in two ways: updating the global compiler env or not updating it. This is controlled via the :restore
option. To implement this, it isolates the parsing process by binding the compiler env to a snapshot of the compiler env when it starts. When the parsing is done, it checks the :restore option and if it's false it merges back parts of the resulting compiler env (the ::namespaces and ::constants keys) into the global compiler env.

The problem is that during the parsing, other compiler processes could have legitimately updated those same keys in the global compiler env, so this merge can occasionally result in compiler data loss.

The race condition can be avoided by using a different approach: only when :restore is true the compiler runs isolated from the global compiler env, by taking a snapshot of the env at the beginning. When :restore is false, no snapshot is taken: env/compiler binding is not modified, so the global compiler env is mutated in-place

I couldn't create a minimal repro, but I created a branch of the reagent project with parallel-build enabled. The branch can be found in https://github.com/nberger/reagent/tree/parallel-build. To run the build from a clean slate, run lein do clean, cljsbuild once client from the commmand line.

Versions affected: 1.8.40 and older (couldn't find 1.8.40 from the "Affects version/s" select)



 Comments   
Comment by Nicolás Berger [ 08/Apr/16 1:47 PM ]

Added new patch with the correct logic on whether to wrap with an isolated atom depending on the :restore option

Comment by David Nolen [ 08/Apr/16 2:02 PM ]

fixed https://github.com/clojure/clojurescript/commit/4e64079741e5d586c16c5ef2746aad3886d6d7d6





[CLJS-1615] Inlining truth checks can lead to better optimisation results Created: 04/Apr/16  Updated: 08/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

I had a situation when DCE was (naively) expected but didn't happen (in :advanced compilation mode). I did some exploration and discovered that inlined truth check code is better understood by Closure Compiler and leads to expected optimisation (for some reasons).

I believe understanding this behaviour and exploiting it where desirable could lead to more predictable code generation without resorting to using cljs type hints.



 Comments   
Comment by Antonin Hildebrand [ 04/Apr/16 2:54 PM ]

The backstory as posted to #cljs-dev in Slack:

When @domkm requested proper dead code elimination in cljs-devtools, I got burnt by the need to explicitly specify `(if ^boolean js/goog.DEBUG …)` hint to get `:closure-defines` working under :advanced build[1]. It was unexpected to me that closure compiler cannot see that optimization and does not inline truth test for js/goog.DEBUG “constant”. So I started poking around and found a way how to aggressively inline checked truth checks in a compatible way (I believe). I also believe this could potentially open optimizations in other places. I think we should explore `@nosideeffects` annotation[2] and tag core functions where appropriate.

[1] https://github.com/binaryage/cljs-devtools/releases/tag/v0.5.3
[2] https://developers.google.com/closure/compiler/docs/js-for-compiler?hl=en#tag-nosideeffects

Comment by Francis Avila [ 05/Apr/16 3:11 PM ]

`@nosideeffects` should only be relevant for extern files where the compiler cannot see the implementation and know if the function is pure. Normally the compiler just analyzes the function to see if it side-effects.

This may be a performance win as well: it looks like advanced compile will unwrap the function expression entirely in some cases (both expression and statement contexts), so no more megamorphic calls to truth_ or even function object allocations.

However, I don't think there's a guarantee that the closure compiler will always understand enough to remove the need for the ^boolean hint for defines in all cases.





[CLJS-1613] Preserve Structural Sharing for clj->js Created: 03/Apr/16  Updated: 08/Apr/16  Resolved: 08/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Tom Raithel Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: clj->js
Environment:

OSX
java version "1.8.0_74"
Java(TM) SE Runtime Environment (build 1.8.0_74-b02)
Java HotSpot(TM) 64-Bit Server VM (build 25.74-b02, mixed mode)



 Description   

Hi, is there any possibility to keep structural sharing intact after calling clj->js on a collection?

It seems like clj->js just makes a deep copy from the collection on every call. I´m new to ClojureScript but it is really confusing that the equality-check against the clojure set is true and the check against the js object equals false.

 
(def set1 {:a [1 2 3]})
(def set2 (assoc set1 :b "foo"))
(.log js/console (== (get set1 :a) (get set2 :a)))
; -> true

(def set1js (clj->js2 set1))
(def set2js (clj->js2 set2))
(.log js/console (== (.-a set1js) (.-a set2js)))
; -> false


 Comments   
Comment by Thomas Heller [ 04/Apr/16 3:25 AM ]

This is just JS being JS. clj->js converts the entire structure, JS doesn't have a set type so we get a [1,2,3] javascript array.

[1,2,3] == [1,2,3]

is false in JS, nothing we can do about that. Generally you want to stay away from JS native collections as much as possible.

Comment by Tom Raithel [ 04/Apr/16 3:46 AM ]

Thanks for your response.

Yes, the two arrays are not the same because clj->js copies all the values over. I was just wondering if there may be a solution to pick up the real reference to the underlying JS object instead of just copying them over.

For example in this case, if I access the [1 2 3] array from the maps directly via js (not calling clj->js), the actual JS objects are equal:

(def a1 (.-tail (nth (.-arr set1) 1)))
(def a2 (.-tail (nth (.-arr set2) 1)))
(.log js/console a1)
(.log js/console a2)
(.log js/console (== a1 a2))
; -> true

I know that example above is hacky, but I just wanted to demonstrate that the structural sharing is already in place, it´s just not preserved when using clj->js.

This would be very helpful for applications, where only part of the code - for example the business layer - are written in ClojureScript and data is shared via plain JS objects.

Comment by Thomas Heller [ 04/Apr/16 4:22 AM ]

That this works is just pure coincidence. If you put a few more elements into the initial vector (eg. (def set1 {:a (into [] (range 500))})) the (def a1 (.-tail (nth (.-arr set1) 1))) will only contain a small part of the values. If you want all values in JS you would need to create a new array and lose the "shareable" reference.

Comment by Tom Raithel [ 04/Apr/16 4:34 AM ]

Ah I see, didn´t know that they are lazy evaluated. I will probably have to come up with another solution to that specific problem. Thx for your help Thomas.

Comment by Francis Avila [ 05/Apr/16 2:09 PM ]

Tom, the issue is not lazy evaluation, it's that a vector internally divides its values into multiple js arrays whose values never change after initialization.

I don't see how this idea is in any way feasible. clj->js cannot ever safely reuse the same underlying arrays from clojure collection objects because any mutation to the structure returned by clj->js would mutate the original clj collections.

At most what could be done is ensuring that clj objects of equal value produce identical js objects. However, even this I don't think is desirable in the general case because it would completely frustrate expectations from javascript consumers of your object. It also has other problems.

Suppose this did exist and you had a clj->js with this behavior:

(def a (clj->js [#{1 2 3} [1 2 3]]))
(def b (clj->js [#{1 2 3} [1 2 3]]))
(def c (clj->js #{1 2 3}))
(assert (identical? a b))
(assert (identical? (aget a 0) c)
(assert (not (identical? (aget a 0) (aget a 1))) ; I assume?

Now how would you avoid this scenario if you mutate something?

(aset a 0 3 "new")
; a is now #js[#js[1 2 3 "new"] #js[1 2 3]]
; b is now the same
; c is now #js[#js[1 2 3 "new"]]

; and subsequent clj->js calls are broken unless you go to extraordinary lengths to invalidate cached entries
(clj->js #{1 2 3}) ; => #js[1 2 3 "new"]

;; But suppose you don't like the above answer. Your options are either:
;; 1. Create a new cached copy with the correct mapping, breaking the identity with already-created values
(def d (clj->js #{1 2 3}))
; #js[1 2 3] again
(assert (not (identical? c d)) ; no more sharing

;; or 2. mutate the cached value back to the original value!
(def d (clj->js #{1 2 3}))
; a, b, c, d are now what they were before the (aset a 0 3 "new")

The core problem is that js data structures are all mutable so the only safe thing to do is clone everything. If you have some special case where you know that the js consumer of the clj->js call will not mutate anything and you don't mind the memory overhead of memoization to get the reference-equality you want, you can always implement a clj->js variant with the properties you desire.

Comment by Tom Raithel [ 07/Apr/16 2:19 PM ]

Hey Francis,

your first code block is not quite what I meant. I was proposing something that behaves exactly like the identical? check of clj objects. That means - regarding your example - I would expect to (assert (identical? (aget a 0) c) equals false because c has no relation to the a object.

I hacked together an extended version of clj->js which is basically storing all the already converted JS objects on to the clj object, so that the second clj->js call on the identical data returns the same object that was already transformed.

It may be a little bit hacky and probably agains all laws of cljs (I´m still new to it), but it works for me. I assume it would also be a bit faster then clj->js because if the value is already cached, there is no need for transformation.

(defn clj->cached-js
  "Recursively transforms ClojureScript values to JavaScript and cache its values.
sets/vectors/lists become Arrays, Keywords and Symbol become Strings,
Maps become Objects. Arbitrary keys are encoded to by key->js."
  [state]
  (when-not (nil? state)
    (if (satisfies? IEncodeJS state)
      (-clj->js state)
      (if (aget state "__cachedJs")
        (aget state "__cachedJs")
        (let [obj (cond
                    (keyword? state) (name state)
                    (symbol? state) (str state)
                    (map? state) (let [m (js-obj)]
                               (doseq [[k v] state]
                                 (aset m (key->js k) (clj->cached-js v)))
                               m)
                    (coll? state) (let [arr (array)]
                                (doseq [state (map clj->cached-js state)]
                                  (.push arr state))
                                arr)
                    :else state)]
          (aset state "__cachedJs" obj)
          obj)))))

Here are some tests that I wrote:

(def some-symbol "my symbol")
(def state {
  :symbol some-symbol
  :key :some-key
  :id 1234
  :user {
    :name "Tom",
    :age 33,
    :city "Wiesbaden"
  }
  :hobbies [
    {:name "code"}
    {:name "sleep"}
  ]
  :tags '("foo" "bar" "baz")
})

(t/deftest test-matches-original-behaviour
  (let [obj (core/clj->cached-js state)
        original-obj (clj->js state)]
    (t/is (= (js->clj original-obj) (js->clj obj)))))

(t/deftest test-change-hash-value
  (let [obj (core/clj->cached-js state)
        new-state (assoc-in state [:user :name] "Bert")
        new-obj (core/clj->cached-js new-state)]
    (t/is (not= new-obj obj))
    (t/is (not= (.-user new-obj) (.-user obj)))
    (t/is (= (.-user.name new-obj) "Bert"))
    (t/is (= (.-hobbies new-obj) (.-hobbies obj)))
    (t/is (= (.-tags new-obj) (.-tags obj)))
))

(t/deftest test-change-vector-value
  (let [obj (core/clj->cached-js state)
        new-state (assoc-in state [:hobbies 1 :name] "drink")
        new-obj (core/clj->cached-js new-state)]
    (t/is (not= new-obj obj))
    (t/is (= (.-user new-obj) (.-user obj)))
    (t/is (not= (.-hobbies new-obj) (.-hobbies obj)))
    (t/is (= (aget new-obj "hobbies" 0) (aget obj "hobbies" 0)))
    (t/is (not= (aget new-obj "hobbies" 1) (aget obj "hobbies" 1)))
    (t/is (= (aget new-obj "hobbies" 1 "name") "drink"))
    (t/is (= (.-tags new-obj) (.-tags obj)))
))

(t/deftest test-add-value-to-vector
  (let [obj (core/clj->cached-js state)
        new-state (assoc-in state
                            [:hobbies]
                            (let [hobbies (get state :hobbies)
                                 [before after] (split-at 1 hobbies)]
                              (vec (concat before [{:name "kid"}] after))))
        new-obj (core/clj->cached-js new-state)]
    (t/is (not= (.-hobbies new-obj) (.-hobbies obj)))
    (t/is (= (aget new-obj "hobbies" 0) (aget obj "hobbies" 0)))
    (t/is (not= (aget new-obj "hobbies" 1) (aget obj "hobbies" 1)))
    (t/is (= (aget new-obj "hobbies" 2) (aget obj "hobbies" 1)))
))

Of course it does not prevent you from manipulating the converted JS objects which subsequently would change other JS references, but that is - like Thomas Heller mentioned - just JS beeing JS. It is just not immutable, but you should treat it as such.

Don´t get me wrong, I believe that is not something that should be implemented into the ClojureScript itself. But this ticket helped me a lot in understanding the principle of clj->js. For now I will go with this solution. If you see some problems with this approach, please let me know.

Thanks for your help!

Comment by Francis Avila [ 08/Apr/16 1:48 PM ]

Consider using (def clj->cached-js (memoize clj->js)).





[CLJS-1616] Self-host: improve documentation for compile-str Created: 06/Apr/16  Updated: 06/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Yehonathan Sharvit Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It's not clear at all how to use the `opts` arguments for compile-str.

In the code - https://github.com/clojure/clojurescript/blob/c3899acf797eb6779c53b313f5606c5018360b83/src/main/cljs/cljs/js.cljs#L660 - we
only have
:load - library resolution function, see load-fn
:source-map - set to true to generate inline source map information

In fact, there is also :verbose and ::def-emits-var

They are not documented.

Are there more options?






[CLJS-1547] Wrong output encoding when compile with goog.LOCALE Created: 17/Jan/16  Updated: 04/Apr/16  Resolved: 19/Jan/16

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

Type: Defect Priority: Minor
Reporter: Sergey Zharinov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1547.patch    

 Description   

How to reproduce:

1. Compiler options:

{:closure-defines {"goog.LOCALE" "ru"}

2. In REPL:

(import '[goog.i18n DateTimeFormat])
(let [fmt (DateTimeFormat. "dd MMMM")]
  (.format fmt (js/Date.)))
;=> "17 ÑнварÑ"

Closure Compiler source code contains note about this behaviour:
https://github.com/google/closure-compiler/blob/62ca536d0a184a8852646e2bc2db178f700f5cfc/src/com/google/javascript/jscomp/AbstractCommandLineRunner.java#L163



 Comments   
Comment by Sergey Zharinov [ 17/Jan/16 1:12 PM ]

Possible solution:
https://github.com/zharinov/clojurescript/commit/72486836d5a1fdb2799f3c21194349403a7828d2

Comment by David Nolen [ 18/Jan/16 2:42 PM ]

This probably needs a compiler flag with a default to "UTF-8". Patch welcome.

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

fixed https://github.com/clojure/clojurescript/commit/404d6444cb6419658a7dacb343a5fed5b9451e0c

Comment by Antonin Hildebrand [ 04/Apr/16 1:23 PM ]

I probably hit one of those legacy problems with this patch while developing a chrome extension with :optimizations :whitespace (which is needed for some other reasons in how so called "content-scripts" work in Chrome).

Google Closure library contains this string literal:
https://github.com/google/closure-library/blob/d66b94513df131c9776cdf70ac476bbb1a62e5d0/closure/goog/i18n/bidi.js#L202

Google Closure Compiler (when set to output UTF-8) blindly spits it as a raw UTF-8, which is not correct UTF-8 according to the specs (even their comment mentions it).

Google Chrome Extension script loader is pretty strict and picky about script validity. They use IsStringUTF8[1] check which boils down to IsValidCharacter[2] checks which fails for that invalid raw goog.i18n.bidi.rtlChars_ string from bidi namespace.

I don't think we should take any action here. My case is rare and I can easily work around this problem by setting :closure-output-charset to "US-ASCII" again. I'm posting it here to maybe save some time to other people who run into similar issues. Eventually we should report this upstream so they resolve this either in Closure Library or somehow in Closure Compiler (a compiler-time warning would be nice at least).

[1] https://chromium.googlesource.com/chromium/src.git/+/9f6c8e18f1233b4ca4cb84b38a1df2e5c1462dcf/base/strings/string_util.cc#519
[2] https://chromium.googlesource.com/chromium/src.git/+/9f6c8e18f1233b4ca4cb84b38a1df2e5c1462dcf/base/strings/utf_string_conversion_utils.h#29

Comment by Antonin Hildebrand [ 04/Apr/16 2:20 PM ]

I have just raised the issue upstream:
https://github.com/google/closure-compiler/issues/1704





[CLJS-1601] Optimize cljs.core dump Created: 12/Mar/16  Updated: 01/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Nikita Beloglazov Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1601.patch     Text File CLJS-1601.patch    

 Description   

When building cljs source that uses cljs.js namespace the final js file is quite huge: 6.4M. As described in wiki: https://github.com/clojure/clojurescript/wiki/Optional-Self-hosting it mostly consists of analysis cache of the cljs.core namespace. As a workaround, the wiki article suggests dumping cache to a separate file and load it at runtime instead of bundling in js binary. I think it is possible to have something in between that doesn't require additional efforts from a user and also optimizes the size of the js file. The idea that instead of dumping cache as raw clojure data-structure it is serialized to string. This way compiler won't compile cache into js (which adds a lot of code) and leave it a string. At runtime, this string will be parsed back to clojure using tools.reader.

Here is the proposal: https://gist.github.com/nbeloglazov/0bf163fb62fa4b61d446

Checking locally it reduces the size of js file from 6.4M to 2.7M which I think quite good. The downside is that now js has to do more work on runtime (parse huge string) when today it simply read js code and evaluates it. But I don't think if it's a big concern. If it is desired to keep all behavior a new option can be added for :dump-core compiler setting, something like :dump-core :string that enables string serialization of the cache.

Does it sound reasonable?



 Comments   
Comment by Nikita Beloglazov [ 27/Mar/16 8:54 PM ]

Attaching suggested fix. Analysis cache is serialized to string and read back to clojure datastructure when cljs.js is initialized.

Comment by David Nolen [ 28/Mar/16 6:39 AM ]

Please change the patch so this optional as you've suggested.

Comment by David Nolen [ 28/Mar/16 6:40 AM ]

Also have you submitted your Clojure CA yet?

Comment by Nikita Beloglazov [ 28/Mar/16 1:35 PM ]

Will do. Yes, I've submitted CA. I used my official name, Mikita Belahlazau there.

Comment by Nikita Beloglazov [ 29/Mar/16 12:16 AM ]

Updated patch that adds option to serialize core analysis cache as string. Possible values of :dump-core are :raw, :string, :none. Old true/false values supported for backward compatibility.

As for default, current patch uses :raw, but I think it makes more sense to use :string. Saving extra few mb of final js is quite good. I think most devs won't go deep into figuring out why js is big and just leave it as it is. Additional one-time parsing performance hit :string introduces acceptable: when :string is used, page loads in 1s while with :raw the time is ~800ms.





[CLJS-1450] Arithmetic warning thrown for impossible condition Created: 15/Sep/15  Updated: 30/Mar/16

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

Type: Defect Priority: Trivial
Reporter: Quest Yarbrough Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, compiler


 Description   

The following code triggers an arithmetic warning even though the condition it's warning about is impossible to reach. I tested this code in Clojure and it did not generate a warning. I would guess that the CLJS compiler doesn't take note of the (js/Error) in the same way that the Clojure compiler treats (Error.)

The exact warning triggered is below, followed by the code.

WARNING: cljs.core/+, all arguments must be numbers, got [number clj-nil] instead. at line 22 src\spurious_arithmetic\core.cljs
(def x [0 1 2 3 4 nil])
(def index (atom -1))
(defn take-value []
  (->> (swap! index inc)
       (nth x)))

(-> (loop [result (take-value)
           prev nil]
      (if (= nil result prev) (throw (js/Error. "This condition prevents nil arithmetic.")))
      (if (some? result)
        (recur (take-value) result)
        (+ 1 prev)))                                        ; this triggers the [number cljs-nil] warning
    (print)) ; 5


 Comments   
Comment by Mike Fikes [ 30/Mar/16 11:33 PM ]

The type inference logic in the compiler expects that the type of a loop local is static. In fact, somewhat the opposite of this ticket is being done in CLJS-1561.





[CLJS-1507] Implicit macro loading: macro var inference in :refer Created: 09/Dec/15  Updated: 30/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-1507-partial.patch    

 Description   

Background: When employing ns, if a namespace is required or used, and that namespace itself requires or uses macros from its own namespace, then the macros will be implicitly required or used using the same specifications.

This ticket asks for a mild extension to the above, automatically inferring when referred symbols are macro vars in two cases:

  1. :require / :refer
  2. :use / :only

Here is a concrete example illustrating the cases:

Assume src/foo/core.clj contains a single macro:

(ns foo.core)

(defmacro unless [pred a b]
  `(if (not ~pred) ~a ~b))

and src/foo/core.cljs requires that macro namespace:

(ns foo.core
  (:require-macros foo.core))

Without loss of generality, assume we do the following in a REPL.

$ java -cp cljs.jar:src clojure.main -m cljs.repl.node
ClojureScript Node.js REPL server listening on 49522
To quit, type: :cljs/quit
cljs.user=> (require '[foo.core :refer [unless]])
;; Currently get an error

Currently, the code assumes that unless is a non-macro var and a "Referred var foo.core/unless does not exist" diagnostic is emitted. To satisfy this ticket, the code should consult the foo.core macro namespace for the existence of an unless macro, and refer it as if

(require-macros '[foo.core :refer [unless]])
had been issued.

Likewise, this should work for the :use / :only dual form: When

(ns bar.core (:use [foo.core :only [unless]]))
is issued.

Additionally, the same behavior should be ensured to work in bootstrapped mode.



 Comments   
Comment by Thomas Heller [ 10/Dec/15 4:26 AM ]

Great to see you pick this up.

My patch for CLJS-948 included an implementation for this if you need a reference, although some things have changed since then and the patch won't apply cleanly. Still the basic idea is still the same.

Comment by Thomas Heller [ 10/Dec/15 4:33 AM ]

Again, just for reference:
https://github.com/thheller/shadow-build/blob/7b2564e9aa33a93c1af90826c837e3f5d307a116/src/clj/shadow/cljs/util.clj#L214-L279

Is the full implementation in use by shadow-build ever since the :refer part was rejected from CLJS-948, that is also where my patch initially came from.

Comment by Mike Fikes [ 11/Dec/15 8:33 PM ]

Attaching a partial patch in case it leads to interesting commentary.

This patch was written prior to my knowledge of Thomas' prior work. I don't have an opinion yet on the values of either approach: This one differs simply by accident of being written independently.

This patch works by transforming things in the sugared domain, thus taking something like

(:require [foo.core :refer [bar unless]])

into

(:require [foo.core :refer [bar] :refer-macros [unless]])

This patch is incomplete because it determines whether a symbol is a macro by attempting to find its macroexpander, and thus encounters the need for the foo.core namespace to have been analyzed, which is not guaranteed.

But, this patch will work if you artificially (say, in a REPL), load foo.core by itself first. Evidently Thomas' approach encountered a similar issue, but he resolved it (presumably by forcing the needed analysis to occur).

Comment by Thomas Heller [ 12/Dec/15 4:09 AM ]

Mike: I had two separate goals with my implementation. In shadow-build I have a discovery phase that inspects all files and parses the ns form in case of .cljs files. I wanted this phase to be completely free of side-effects in regards to the compiler state. I did not want to touch the analyzer env or load macros since I do not know whether the discovered namespace is going to be used later. I only extract the basic requires/provides to build the dependency graphs via :main (forced by shadow-build). Actual compilation always happens in full dependency order and macros are loaded when needed with all checks. The discovery skips some basic checks it cannot know about yet, ie. it checks for self-require but doesn't load macros yet.

The approach chosen by parse-ns in cljs.analyzer is to capture the compiler env and reset it after parsing the ns, your approach moves side-effects into the parsing again which may become a problem later since it cannot reset {{require}}ing a Clojure ns.

Whether or not this is a problem I cannot say, it just didn't agree with my goals. YMMV.

Comment by Mike Fikes [ 12/Dec/15 7:35 AM ]

Thanks @thheller, I'll see if I can come up with a patch that implements the inference in ns-side-effects.

Comment by Mike Fikes [ 30/Mar/16 11:18 PM ]

It's been a while since I've dedicated any time to this one. Unassigned from me in case anyone else wants to take it up.





[CLJS-1610] Refs api Created: 26/Mar/16  Updated: 28/Mar/16

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

Type: Enhancement Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It would be nice to support the refs API in clojurescript to ease porting of libraries which require it. Obviously usage of it would not bring concurrency benefits, but it would at least allow some existing clojure code to run.

I've satisfied myself it can be done reasonably easily (see https://github.com/jjl/clojurescript/tree/ref-support ), and I can't really see any downsides.

For the minute, this has been put into the cljs.stm namespace. I don't know whether it should be a feature you are automatically opted into, so I didn't put it into core. Thoughts?



 Comments   
Comment by David Nolen [ 28/Mar/16 6:44 AM ]

It's definitely worth considering. If you've done the work, attach an actual patch for review. In general we try to avoid links outside.





[CLJS-1608] Self-host: Alias-scoped keywords works only for first segment Created: 23/Mar/16  Updated: 23/Mar/16  Resolved: 23/Mar/16

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

Type: Defect Priority: Minor
Reporter: Andrea Richiardi Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bootstrap
Environment:

ClojureScript 1.8.34



 Description   

An aliased keyword works only if the alias corresponds to the first segment, example:

(ns dummy.core (:require [clojure.string :as s])
cljs.user=> ::s/key
Invalid token: ::s/key

BUT

dummy.core=> (ns dummy.core (:require [clojure.string :as clojure]))
nil
dummy.core=> ::clojure/key
:clojure.string/key


 Comments   
Comment by Andrea Richiardi [ 23/Mar/16 5:42 PM ]

By adding this test, I was actually able to prove that this works here and I must made a mistake:

(deftest test-CLJS-1608
  (let [st (cljs/empty-state)]
    (cljs/eval-str st
      "(ns alias-load.core (:require [clojure.string :as s]))"
      nil
      {:ns      'cljs.user
       :context :expr
       :eval    cljs.js/js-eval
       :load    (fn [_ cb]
                  (cb {:lang :clj :source "(ns clojure.string)"}))}
      (fn [{:keys [error value]}]
        (is (nil? error))
        (cljs.js/eval-str st
          "::s/bar"
          nil
          {:ns      'alias-load.core
           :context :expr
           :eval    cljs.js/js-eval}
          (fn [{:keys [error value]}]
            (is (nil? error))
            (is (= :clojure.string/bar value))))))))
Comment by Andrea Richiardi [ 23/Mar/16 5:42 PM ]

Sorry for the noise





[CLJS-1607] Advanced compilation bug with `specify!` in JS prototypes Created: 23/Mar/16  Updated: 23/Mar/16

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

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None
Environment:

affects 1.8.34



 Description   

compiling this code with advanced optimizations

(ns bug.core)

(defprotocol IBug
  (bug [this other] "A sample protocol"))

(defn MyBug [])
(specify! (.-prototype MyBug)
  IBug
  (bug [this other]
    "bug")
  Object
  (foo [this]
    (bug this 3))) ;; line 13

causes the following warning:

WARNING: Use of undeclared Var bug.core/x14072 at line 13


 Comments   
Comment by António Nuno Monteiro [ 23/Mar/16 1:42 PM ]

narrowed it down to this line (https://github.com/clojure/clojurescript/blob/f0ac4c92006ac618516c11e9ca3527904d35d4af/src/main/clojure/cljs/compiler.cljc#L936) being called in `:advanced` because it passes the check of cljs-static-fns in that case





[CLJS-1605] Some repl options missing from known option set Created: 21/Mar/16  Updated: 21/Mar/16  Resolved: 21/Mar/16

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

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

Affects 1.8.34


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

 Description   

Related to http://dev.clojure.org/jira/browse/CLJS-1603

Not all options used by cljs.repl/repl* are currently included in known-repl-options. CLJS-1603 fixed most of unnecessary warnings as a warning is not shown if suggestion is not found, but some valid options still cause unnecessary warnings:

WARNING: Unknown option ':init'. Did you mean ':main'?



 Comments   
Comment by David Nolen [ 21/Mar/16 6:34 PM ]

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





[CLJS-1603] Only warn for misspelled comp/REPL opts Created: 18/Mar/16  Updated: 21/Mar/16  Resolved: 21/Mar/16

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

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

Affects 1.8.34


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

 Description   

REPLs may have REPL-specific options which are added to the compiler/REPL option map and this triggers "unknown compiler option" warnings. For one downstream example, with Figwheel, you get

WARNING: Unknown option ':compiler-env'.
WARNING: Unknown option ':special-fns'.
WARNING: Unknown option ':warn-on-undeclared'.

Instead, only issue warnings when there are known suggestions within the Levenshtein distance threshold. This effectively limits the feature to its original use case of detecting minor misspellings per CLJS-1492.



 Comments   
Comment by Juho Teperi [ 20/Mar/16 5:04 PM ]

I don't know about REPL-specific options, but looks like currently warning is given for many options used directly on repl*. I think it would make sense to include these in known options?

At least the following are used on repl* but not included in known opts:

  • init
  • quit-prompt
  • eval
  • source-map-inline
  • wrap
  • repl-requires
  • compiler-env
  • bind-err
Comment by Mike Fikes [ 20/Mar/16 5:51 PM ]

Juho: Yep. Perhaps they would be added to this set (perhaps as a separate defect ticket?): https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl.cljc#L40-L43

Comment by David Nolen [ 21/Mar/16 1:43 PM ]

fixed https://github.com/clojure/clojurescript/commit/177b05fc5935c659e4829b7b1798e216b6797bec





[CLJS-1575] Combination of - and _ params causes JSC_DUPLICATE_PARAM Created: 17/Feb/16  Updated: 18/Mar/16

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

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


 Description   

The following function definitions each cause ERROR: JSC_DUPLICATE_PARAM. Parse error. Duplicate parameter name "_" at compile time with :advanced compilation:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defn causes-duplicate-param [{_ :foo}]
  (reify
    Object
    (a-function [-]
      (fn [] "arbitrary function inside a-function body"))))

(defn causes-duplicate-param [{- :foo}]
  (reify
    Object
    (a-function [_]
      (fn [] "arbitrary function inside a-function body"))))

They generate, respectively:

duplicate_param_name_demo.core.t_duplicate_param_name_demo$core1601.prototype.a_function = ((function (map__1599,map__1599__$1,_){
return (function (){
var self__ = this;
var _ = this;
return ((function (_,map__1599,map__1599__$1,_){
return (function (){
return "arbitrary function inside a-function body";
});
;})(_,map__1599,map__1599__$1,_))
});})(map__1599,map__1599__$1,_))
;

duplicate_param_name_demo.core.t_duplicate_param_name_demo$core1612.prototype.a_function = ((function (map__1610,map__1610__$1,_){
return (function (){
var self__ = this;
var _ = this;
return ((function (_,map__1610,map__1610__$1,_){
return (function (){
return "arbitrary function inside a-function body";
});
;})(_,map__1610,map__1610__$1,_))
});})(map__1610,map__1610__$1,_))
;

(Notice the duplicate _ param on the 5th line of each.)

The following do not:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defn causes-duplicate-param [{- :foo}]
  (reify
    Object
    (a-function [-]
      (fn [] "arbitrary function inside a-function body"))))

(defn causes-duplicate-param [{_ :foo}]
  (reify
    Object
    (a-function [_]
      (fn [] "arbitrary function inside a-function body"))))

They generate:

duplicate_param_name_demo.core.t_duplicate_param_name_demo$core1601.prototype.a_function = ((function (map__1599,map__1599__$1,_){
return (function (){
var self__ = this;
var ___$1 = this;
return ((function (___$1,map__1599,map__1599__$1,_){
return (function (){
return "arbitrary function inside a-function body";
});
;})(___$1,map__1599,map__1599__$1,_))
});})(map__1599,map__1599__$1,_))
;

duplicate_param_name_demo.core.t_duplicate_param_name_demo$core1612.prototype.a_function = ((function (map__1610,map__1610__$1,_){
return (function (){
var self__ = this;
var ___$1 = this;
return ((function (___$1,map__1610,map__1610__$1,_){
return (function (){
return "arbitrary function inside a-function body";
});
;})(___$1,map__1610,map__1610__$1,_))
});})(map__1610,map__1610__$1,_))
;

(Notice that one of the {} params has become __$1.)

My guess, though I haven't looked into the compiler code, is that the compiler escapes {} to _$1 when it would conflict with another {}, and also it translates - to {}, but it doesn't notice the conflict when the _ClojureScript symbols are different.



 Comments   
Comment by Peter Jaros [ 17/Feb/16 11:44 AM ]

Forgive the weird formatting errors. I couldn't find a preview function and there doesn't appear to be a way to edit the issue now that it's posted.

Comment by David Nolen [ 18/Mar/16 1:46 PM ]

This is because of munging they will become the same thing. Patch welcome.





[CLJS-1576] cljs.js sourcemap support throws on non-latin1 characters Created: 17/Feb/16  Updated: 18/Mar/16

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

Type: Defect Priority: Minor
Reporter: Matt Huebert Assignee: Matt Huebert
Resolution: Unresolved Votes: 0
Labels: bootstrap

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

 Description   

In cljs.js/append-source-map we encode the source-map string in base64 without escaping non-latin1 characters. In Chrome, this throws the error: "DOMException: Failed to execute 'btoa' on 'Window': The string to be encoded contains characters outside of the Latin1 range."

Source: https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/js.cljs#L152

The problem & a couple of solutions are explained here: https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding#The_Unicode_Problem



 Comments   
Comment by David Nolen [ 18/Feb/16 8:21 AM ]

Can bootstrapped users apply this and verify it works for them? Thanks.

Comment by Mike Fikes [ 18/Feb/16 10:03 AM ]

I tried this with Planck and I can confirmed that, with a function name in Japanese, sourceMappingURL does indeed change and then includes base-64 data that covers my entire set of functions (whereas previously it did not), but the Japanese function name appears to have been munged into some Latin-1 characters (which I suppose is the point of the patch).

With Planck, I can't confirm the overall functionality as Planck doesn't make use of this information with JavaScriptCore (it instead uses equivalent info stored in map files).

So, as far as I can tell, this patch is good in that it appears to be doing the right thing when run with the bootstrap compiler.

Comment by David Nolen [ 18/Mar/16 1:45 PM ]

OK, patch looks ok to me but it needs to be rebased to master.





[CLJS-1592] Self-host: Robustness for core tests Created: 25/Feb/16  Updated: 18/Mar/16  Resolved: 18/Mar/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

There are a couple minor things that can be improved in the core tests which facilitates running them in bootstrapped environments:

1. Restore *print-newline*
2. Add a little more order robustness for hash-based collections

Will attach patch for consideration.



 Comments   
Comment by David Nolen [ 18/Mar/16 1:38 PM ]

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





[CLJS-1602] The `extend-type` raises unexpected exception on extending a js type with IFn. Created: 16/Mar/16  Updated: 18/Mar/16  Resolved: 18/Mar/16

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

Type: Defect Priority: Major
Reporter: Andrey Antukh Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Linux, OpenJDK8



 Description   

When trying use `extend-type` (and `extend-protocol` that uses the first under the hood) raises the following exception:

Caused by: java.lang.UnsupportedOperationException: nth not supported on this type: Symbol
        at clojure.lang.RT.nthFrom(RT.java:947)
        at clojure.lang.RT.nth(RT.java:897)
        at cljs.core$adapt_ifn_invoke_params.invokeStatic(core.cljc:1330)
        at cljs.core$adapt_ifn_invoke_params.invoke(core.cljc:1330)
        at cljs.core$ifn_invoke_methods$fn__4744.invoke(core.cljc:1355)
        at clojure.core$map$fn__4785.invoke(core.clj:2646)
        at clojure.lang.LazySeq.sval(LazySeq.java:40)
        at clojure.lang.LazySeq.seq(LazySeq.java:49)
        at clojure.lang.RT.seq(RT.java:521)
        at clojure.core$seq__4357.invokeStatic(core.clj:137)
        at clojure.core$map$fn__4785.invoke(core.clj:2637)
        at clojure.lang.LazySeq.sval(LazySeq.java:40)
        at clojure.lang.LazySeq.seq(LazySeq.java:56)
        at clojure.lang.ChunkedCons.chunkedNext(ChunkedCons.java:59)
        at clojure.lang.ChunkedCons.next(ChunkedCons.java:43)
        at clojure.lang.RT.next(RT.java:688)
        at clojure.core$next__4341.invokeStatic(core.clj:64)
        at clojure.core$butlast__4385.invokeStatic(core.clj:279)
        at clojure.core$butlast__4385.invoke(core.clj:277)
        at cljs.analyzer$analyze_do_statements_STAR_.invokeStatic(analyzer.cljc:1380)
        at cljs.analyzer$analyze_do_statements_STAR_.invoke(analyzer.cljc:1379)
        at cljs.analyzer$analyze_do_statements.invokeStatic(analyzer.cljc:1383)
        at cljs.analyzer$analyze_do_statements.invoke(analyzer.cljc:1382)
        at cljs.analyzer$eval1448$fn__1450.invoke(analyzer.cljc:1387)
        at clojure.lang.MultiFn.invoke(MultiFn.java:251)
        at cljs.analyzer$analyze_seq_STAR_.invokeStatic(analyzer.cljc:2368)
        at cljs.analyzer$analyze_seq_STAR_.invoke(analyzer.cljc:2366)
        at cljs.analyzer$analyze_seq_STAR__wrap.invokeStatic(analyzer.cljc:2373)
        ... 102 more

The minimal reproducible code (also happens in node repl):

(extend-type js/Date
  IFn
  (-invoke [this]
    (.valueOf this)))


 Comments   
Comment by David Nolen [ 18/Mar/16 12:37 PM ]

This is not a bug. The IFn implementations are malformed.

(extend-type js/Date
  IFn
  (-invoke
    ([this]
      (.valueOf this))))




[CLJS-1562] WARN on hinted fn call type mismatch Created: 06/Feb/16  Updated: 18/Mar/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

If a call is made to a function that has hinted arguments (especially {^boolean} and {^number}), with an expression that is known to not be of that type, emit a diagnostic type mismatch warning.

An example that should emit a warning is:

(defn f [^boolean b])
(f 0)





[CLJS-1582] Type-hint extend-type first arg for primitives Created: 20/Feb/16  Updated: 18/Mar/16  Resolved: 18/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1582-1.patch     Text File CLJS-1582-2.patch     Text File CLJS-1582-3.patch    
Patch: Code

 Description   

The docstring for extend-type implies that the type will be propagated into the first argument of all fns.

But, if you evaluate

(extend-type boolean ICounted (-count [b] (if b 1 0)))

You will see, for example that the boolean type hint is not propagated to the b param.

(This is evidently done for non-primitives, so this ticket covers primitives.)



 Comments   
Comment by Mike Fikes [ 20/Feb/16 11:21 AM ]

With the attached patch (really just attached for feedback / discussion), you the boolean type hint is propagated:

cljs.user=> (extend-type boolean ICounted (-count [b] (if b 1 0)))
#object[Function "function (b){
if(b){
return (1);
} else {
return (0);
}
}"]

One thing I don't like about the patch, though, is it involves code walking, which could be fragile, lacking robustness if invalid code is passed to the macro.

Comment by Mike Fikes [ 20/Feb/16 11:58 AM ]

CLJS-1582-2.patch fixes things up with respect to bootstrap considerations.

Comment by Mike Fikes [ 24/Feb/16 2:01 PM ]

CLJS-1582-3.patch fixes a bug. (It was using a map instead of set to test if type-sym is boolean or number.)

Comment by David Nolen [ 18/Mar/16 12:29 PM ]

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





[CLJS-1591] Compilation time go up significantly when nesting multimethods Created: 25/Feb/16  Updated: 14/Mar/16

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

Type: Defect Priority: Minor
Reporter: Marian Schubert Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: cljs, compiler

Attachments: Text File CLJS-1591.patch    

 Description   

Code like this takes 140 seconds to compile on my machine. Regular functions don't seem to trigger this behaviour.

(ns slow.core)

(defmulti my-multimethod (fn [x] :whatever))

(defn this-is-sloow-to-compile []
  (my-multimethod
   (my-multimethod
    (my-multimethod
     (my-multimethod
      (my-multimethod
       (my-multimethod
        (my-multimethod
         (my-multimethod
          (my-multimethod
           (my-multimethod
            (my-multimethod
             (my-multimethod
              (my-multimethod
               (my-multimethod
                (my-multimethod
                 (my-multimethod
                  (my-multimethod
                   (my-multimethod
                    (my-multimethod
                     (my-multimethod {})))))))))))))))))))))

$ rm -rf target/ && ./scripts/release 
Building ...
Analyzing jar:file:/Users/maio/.m2/repository/org/clojure/clojurescript/1.7.228/clojurescript-1.7.228.jar!/cljs/core.cljs
Compiling src/slow/core.cljs        <-- here it spends most of the time
Applying optimizations :advanced to 11 sources
... done. Elapsed 141.85386191 seconds

Whole project is here https://github.com/maio/slow-cljs-build



 Comments   
Comment by Mike Fikes [ 04/Mar/16 4:32 PM ]

Hrm. This fix is evidently in the Closure compiler used by 1.7.228: https://github.com/google/closure-compiler/issues/1049

Comment by Thomas Heller [ 06/Mar/16 6:25 AM ]

@mfikes the slowdown is not related to the Closure Compiler since it happens when compiling cljs->js not when optimizing.

The reason for the slowdown is due to the arguments of a multimethod call being analyzed twice (or more in case of deep nesting).

See [1] for the problematic code.

multimethods are not fn-var? so the or does not short circuit and (all-values? argexprs) is reached. This forces the argexprs lazy-seq (thereby analyzing the args). Since the args are not all-values? the else-branch of the if is taken, which then later causes the args to be analyzed again. My math is weak but I'm not mistaken this is O(n!), explaining the dramatic slowdown.

Every var that is not fn-var? is affected by this:

(defn test [& args] :whatever)
(def my-multimethod test)
;; or
(def my-multimethod
  (reify
    IFn
    (-invoke [a] :whatever)))

One solution would be to fix all-values? that instead of running through analyze it could just check whether all args are fixed literals (ie. not list? but all of number? string? symbol? keyword? etc.).

I'm not really sure why the else-branch in [1] exists at all but I assume it is to work around some JS quirks. I will hold off on writing a patch until I figure out why the extra let introduced in the else-branch is needed.

[1] https://github.com/clojure/clojurescript/blob/f58dcdf4dc37ef52d4eb1e2b7c994282bf6351f5/src/main/clojure/cljs/analyzer.cljc#L2257-L2263

PS: forgot to add that this does not happen with :static-fns false since it also prevents the else from being reached.

Comment by Thomas Heller [ 06/Mar/16 6:55 AM ]

The else was introduced in CLJS-855 and is sort of required for invokes without arity information and :static-fns true.

Changing all-values? to just check literals instead of analyzing should be a valid solution.

Comment by Thomas Heller [ 14/Mar/16 8:31 AM ]

The patch removes the extra analyze and instead just checks the few cases that can actually be used without assignment first.

This removes the slowdown while keeping all the functionality.





[CLJS-1590] split, split-lines differs from Clojure on empty string Created: 25/Feb/16  Updated: 14/Mar/16  Resolved: 14/Mar/16

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

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

 Description   

In Clojure (clojure.string/split "" #"\n") yields [""], but in ClojureScript [].

In Clojure (clojure.string/split-lines "") yields [""], but in ClojureScript [].



 Comments   
Comment by Peter Schuck [ 03/Mar/16 6:47 PM ]

Works in V8, SpiderMonkey, and Nashorn. The fix was only discarding "" from the split when the split had more then one items.

Comment by Mike Fikes [ 13/Mar/16 11:41 AM ]

Hey Peter, the attached patch no longer applies cleanly to master.

Comment by Peter Schuck [ 14/Mar/16 11:22 AM ]

Added patch rebased on the latest master. All the tests pass as before.

Comment by Mike Fikes [ 14/Mar/16 1:08 PM ]

FWIW, the v2 patch has been incorporated downstream into Planck master and will be "soaking". It passes tests and the specific items in this ticket's description.

Comment by David Nolen [ 14/Mar/16 1:18 PM ]

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





[CLJS-1444] Node.js shim requires `node` invocation to be in the same directory as shim script Created: 10/Sep/15  Updated: 14/Mar/16

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

Type: Defect Priority: Minor
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Don't have time to provide a proper repro now but the basic issue can be illustrated by this:

~/c/boot-cljs-example (master=) node target/main.js
module.js:338
    throw err;
          ^
Error: Cannot find module '/Users/martin/code/boot-cljs-example/out/goog/bootstrap/nodejs.js'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/martin/code/boot-cljs-example/target/main.js:6:1)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
~/c/boot-cljs-example (master=) cd target/
~/c/b/target (master=) node main.js
Starting...

This is compiled with boot because that was what I had at hand right now. The compiled shim looks like this:

var path = require("path");
try {
    require("source-map-support").install();
} catch(err) {
}
require(path.join(path.resolve("."),"out","goog","bootstrap","nodejs.js"));
require(path.join(path.resolve("."),"out","cljs_deps.js"));
goog.global.CLOSURE_UNCOMPILED_DEFINES = {"cljs.core._STAR_target_STAR_":"nodejs"};
goog.require("boot.cljs.main");
goog.require("cljs.nodejscli");

The problem here is that path.resolve(".") will return the directory the node command was invoked in and not the directory of the shim. (See the "Cannot find module..." error above)

A solution could be to use __dirname which always resolves to the directory of the current file. This might result in some breakage for existing setups.



 Comments   
Comment by Sebastian Bensusan [ 14/Oct/15 11:31 AM ]

I have a proposed solution but I fear that supporting "run from anywhere" adds essential complexity to the resulting code. My thought process:

1. Relative paths are desirable to produce "context free code." If the user chooses absolute paths, then that behavior is respected and there's nothing to handle (no "path algebra") .

2. When dealing with relative paths, the whole system needs to establish a "frame of reference", a root path. The ClojureScript compiler assumes the path from which it is compiling to be that frame of reference, which usually coincides with the top root of the project. Though arbitrary, it is the only choice that makes sense.

3. The frame of reference is not explicit anywhere in the code, since it is defined as ".". If it were explicit, it would reveal context, as in "/home/some-user/their-folder/this-project/".

4. When we approach the code from another reference point (executing the script from another directory), we first need to find the original compiler path (reference point,) and then resolve all paths from there. The compiler uses `cljs.closure/path-relative-to` for this purpose.

Path algebra:
compiler-path = __dirname - output-to

Node.js

var compiler-path = __dirname.replace(output-to, "")
path.resolve (compiler-path, output-dir, "goog", "base.js")
path.resolve (compiler-path, output-dir, "cljs_deps.js")

which assumes that if output-to was given relatively, then output-dir is also relative. If they are not in sync, more work needs to be done to keep them that way.

It's not up to me to decide if the extra complexity is worth the use-case. I actually hope there is a simpler solution to solve this that I'm not seeing.

Comment by Karol Majta [ 14/Mar/16 10:43 AM ]

I find this behavior really weird and would opt for switching to __dirname. I am also not sure i fully understand consequences of such switch (I have little cljs experience, speaking more from the perspective of nodejs user). My point is: current behavior renders clojurescript hard to use for commandline and desktop applications (namely electron).

For command line and desktop applications assumptions about CWD cannot be made. For now i run my applications through a bootstrap script JS script:

process.chdir(__dirname);
require('./target/out');

I am lucky that my code does not have to use the real CWD, but it's a hack more than a real solution.

Speaking from nodejs perspective:

1. Using absolute paths is considered a bad practice anyway.
2. Nodejs programs that don't use external packages (don't depend on node_modules) can be run from any CWD
3. Nodejs programs that do depend on node_modules will break if run from a different directory than the one containing node_modules, but this is expected behavior.





[CLJS-1594] NaN and both infinities cannot be elements of a set Created: 27/Feb/16  Updated: 11/Mar/16  Resolved: 11/Mar/16

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

Type: Defect Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1594-case-check.patch     Text File CLJS-1594.patch    
Patch: Code and Test

 Description   
(hash (.-POSITIVE_INFINITY js/Number)) ;; => NaN

which somehow leads to:

(hash-set (.-POSITIVE_INFINITY js/Number) 0 1 2 3 4 5 6 7 8) ;; => RangeError: Maximum call stack size exceeded

The same seems to be true for negative infinity and NaN itself, and also applies to keys of sufficiently large maps.



 Comments   
Comment by Mike Fikes [ 01/Mar/16 5:02 PM ]

The attached patch fundamentally works.

NaN is interesting in that it can't equal itself. So even in Clojure

user=> (disj #{Double/NaN} Double/NaN)
#{NaN}

The hash values in this patch match those from Clojure.

I suppose the most interesting aspect of the patch is how fast we can make it.

I haven't timed js/isFinite to see if it is any faster than a few == checks, but I figure worth attaching for discussion.

Comment by Mike Fikes [ 01/Mar/16 5:34 PM ]

Using this as a comparison

(time (run! hash (range 0 1e8)))

which takes about 20 seconds (not sure if it is a valid perf test), the variant in the patch and all of these run in the same speed.

(These are my best effort at trying to do the right thing wrt perf---certainly open to someone pointing out a better way.)

One == at the expense of a let:

(let [h (js-mod (Math/floor o) 2147483647)]
  (if (== NaN h)
    (if (== -Infinity o)
      -1048576
      (if (== Infinity o)
        2146435072
        2146959360))
    h))

Simply do the tests for the non-finite ones first:

(if (== -Infinity o)
  -1048576
  (if (== Infinity o)
    2146435072
    (if (== NaN o)
      2146959360
      (js-mod (Math/floor o) 2147483647))))

Try an or for a single expression:

(if (or (== -Infinity o)
        (== Infinity o)
        (== NaN o))
  (if (== -Infinity o)
    -1048576
    (if (== Infinity o)
      2146435072
      2146959360))
  (js-mod (Math/floor o) 2147483647))
Comment by Mike Fikes [ 01/Mar/16 8:42 PM ]

The change in CLJS-1594.patch would introduce the only call to isFinite in the ClojureScript standard library, and, while isFinite was introduced with JavaScript 1.3 (1998), I could see an argument for going with another patch that avoids it.

I retract the above statement: goog.math.isFiniteNumber calls isFinite:

cljs.user=> (require 'goog.math)
nil
cljs.user=> goog.math.isFiniteNumber
#object[Function "function (num) {
  return isFinite(num) && !isNaN(num);
}"]
Comment by Peter Schuck [ 02/Mar/16 12:24 PM ]

Hey Mike have you tried using case instead of if for speed comparisons?

(if (js/isFinite)
  (js-mod (Math/floor o) 2147483647)
  (case o
    Infinity
    2146435072
    -Infinity
    -1048576
    2146959360))
Comment by Mike Fikes [ 02/Mar/16 4:56 PM ]

Hi Peter, I tried the above, revising your code slightly to add the o argument. And it is the same speed. My tests focus on the speed of regular numbers. Perhaps case is faster for the other branch.

I even tried

(time (loop [n 1e8] (hash n) (when (pos? n) (recur (dec n))))))

as to try to eliminate overhead, and it still takes 20 seconds.

Also, a lot of my other variants posted above involve == with NaN, so we can chuck them out.

IMHO, your case variant is more readable. (I'd put the numbers on the same lines with the Infinities, but that's minor.)

Comment by Peter Schuck [ 03/Mar/16 9:35 AM ]

Cool thanks for testing the perf results. I've put my patch up. The patch also fixes the tests since

(not= NaN (hash x)) ;; always true

will always be true since NaN is not equal to anything including itself.

(not (js/isNaN x)) ;; only false when x is NaN

should be used instead

Comment by Mike Fikes [ 03/Mar/16 9:43 AM ]

Cool. Peter's revisions pass for me on ChakraCore, SpiderMonkey, V8, JavaScriptCore, and Nashorn.

Comment by David Nolen [ 11/Mar/16 1:15 PM ]

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

Comment by Gary Fredericks [ 11/Mar/16 6:51 PM ]

Thanks!





[CLJS-1589] Self-host: case fail with nil test Created: 25/Feb/16  Updated: 11/Mar/16  Resolved: 11/Mar/16

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

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

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

 Description   

(case 1 nil nil :x) will fail in bootstrap:

(require 'cljs.js)
(cljs.js/eval-str (cljs.js/empty-state)
"(case 1 nil nil :x)" nil {:eval cljs.js/js-eval :context :expr} identity)

produces

{:error #error {:message "Could not eval ", :data {:tag :cljs/analysis-error}, :cause #error {:message "Cannot read property 'length' of null at line 1 ", :data {:file nil, :line 1, :column 1, :tag :cljs/analysis-error}, :cause #object[TypeError TypeError: Cannot read property 'length' of null]}}}

The root cause is that, in ClojureScript char? throws if passed nil, while this is not the case in Clojure, and the case macro ultimately does this here:

https://github.com/clojure/clojurescript/blob/e531c34e04adc815a6c25c8d2499465296ca290d/src/main/clojure/cljs/core.cljc#L2143

So, an aspect to ponder is whether to change char? or just conditionally fnil the bit of code for :cljs.



 Comments   
Comment by Mike Fikes [ 27/Feb/16 4:24 PM ]

While you could perhaps argue that char? should return false if passed any non-char value, the attached CLJS-1589.patch is perhaps the most conservative way to address the issue, by conditioanlly nil-patching the affected call site.

Note: This patch adds tests for bootstrap and there already exists coverage for non-bootstrap: https://github.com/clojure/clojurescript/blob/f58dcdf4dc37ef52d4eb1e2b7c994282bf6351f5/src/test/cljs/cljs/core_test.cljs#L1979-L1990

Comment by David Nolen [ 11/Mar/16 2:52 PM ]

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





[CLJS-1596] Self-host: :load-macros and :analyze-deps don't work in cljs.js Created: 01/Mar/16  Updated: 11/Mar/16  Resolved: 11/Mar/16

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

Type: Defect Priority: Minor
Reporter: Nikita Beloglazov Assignee: Mike Fikes
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

Playing with bootstrapped cljs I noticed that :analyze-deps doesn't work correctly. Setting it to false doesn't make any effect and deps are still being analyzed. I checked source code and it seems like :analyze-deps and :load-macros are always true regardless what you provide:

For example:
https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/js.cljs#L225

{
:*load-macros*  (or (:load-macros opts) true)
:*analyze-deps* (or (:analyze-deps opts) true)
}

Both these variables are always truthy. I think correct version is (:load-macros opts true) instead of using or in this case unless it is intentionally written this way to "disable" :load-macros/:analyze-deps options.



 Comments   
Comment by Mike Fikes [ 04/Mar/16 3:01 PM ]

Revised binding forms and added tests.

Comment by David Nolen [ 11/Mar/16 1:59 PM ]

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





[CLJS-1597] Redundant IPrintWithWriter test in pr-writer-impl Created: 03/Mar/16  Updated: 11/Mar/16  Resolved: 11/Mar/16

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

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

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

 Description   

This cond branch cannot be followed

https://github.com/clojure/clojurescript/blob/f58dcdf4dc37ef52d4eb1e2b7c994282bf6351f5/src/main/cljs/cljs/core.cljs#L9096-L9097

owing to this test

https://github.com/clojure/clojurescript/blob/f58dcdf4dc37ef52d4eb1e2b7c994282bf6351f5/src/main/cljs/cljs/core.cljs#L9049

and can be removed as effectively dead code.



 Comments   
Comment by David Nolen [ 11/Mar/16 1:08 PM ]

fixed https://github.com/clojure/clojurescript/commit/032077b52fd7c4359f18004f14faadab0d1a0782





[CLJS-1599] UUIDs are not equal for upper/lower case strings Created: 07/Mar/16  Updated: 11/Mar/16

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

Type: Defect Priority: Minor
Reporter: Nikolay Durygin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Windows 7x64



 Description   

UUIDs generated for strings in different case (one is upper and one is lower) are equal.

For example (= (uuid "071C600F-B72B-44AE-8A15-9366EA1BB9D9") (uuid "071c600f-b72b-44ae-8a15-9366ea1bb9d9")) returns false.

Spec http://www.itu.int/rec/T-REC-X.667/en says following:

6.5.4 Software generating the hexadecimal representation of a UUID shall not use upper case letters.
NOTE - It is recommended that the hexadecimal representation used in all human-readable formats be restricted to lower-case
letters. Software processing this representation is, however, required to accept both upper and lower case letters as specified
in 6.5.2.



 Comments   
Comment by Gary Fredericks [ 07/Mar/16 8:17 AM ]

Would this be a good time to change the internal representation from a string to either a pair of goog.math.Longs or a quartet of "32-bit" integer doubles?

Comment by Nikolay Durygin [ 09/Mar/16 2:22 AM ]

Is there any special need? Issue described above can be solved by lower casing all strings inside uuid. Another problem - the fact that uuid doesn't complain if non uuid format string is passed can be solved with regex.





[CLJS-1600] Destructuring defprotocol fn args causes defrecord impls to silently fail Created: 11/Mar/16  Updated: 11/Mar/16

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

Type: Defect Priority: Minor
Reporter: James Henderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
(defprotocol IFoo
  (foo-fn [_ {:keys [a b] :as args}]))

(defrecord Foo []
  IFoo
  (foo-fn [_ {:keys [a b] :as args}]
    args))

(foo-fn (->Foo) {:a 1, :b 2})

returns

{:keys [1 2], :as {:a 1, :b 2}}

in CLJS, rather than

{:a 1, :b 2}

as Clojure does.

Redefining IFoo to

(defprotocol IFoo
  (foo-fn [_ args]))

causes the issue to go away.

While it's quite a minor bug, it was quite a hard one to track down, in practice - I didn't think to look at the protocol definition when the record was breaking, even after having used ClojureScript for a good few years!

Cheers,

James






[CLJS-1553] browser repl "EOF while reading string" when evaluating symbol "enable-console-print!" Created: 28/Jan/16  Updated: 05/Mar/16  Resolved: 05/Mar/16

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

Type: Defect Priority: Minor
Reporter: Scott Bale Assignee: Scott Bale
Resolution: Declined Votes: 0
Labels: repl
Environment:

On client side: Chrome browser (47.0.2526.111) On Mac OS 10.10.5 Yosemite

On server side:

  • Amazon EC2 instance running amazon linux

java -version
openjdk version "1.8.0_65"
OpenJDK Runtime Environment (build 1.8.0_65-b17)
OpenJDK 64-Bit Server VM (build 25.65-b01, mixed mode)


Attachments: GZip Archive cljs-hello-world.tar.gz    

 Description   

Just playing around in the browser repl, running through the CLJS quickstart, I happened upon this. I crashed the repl by attempting to evaluate enable-console-print!.

Below is output of my repl. As you can see, I can doc, source, or even invoke the function, but if I try to simply evaluate the symbol, the repl crashes with a Java runtime exception. (I was expecting the repl to output a toString representation of the enable-console-print! function object, as it would for any other symbol that resolves to a function.)

I've attached a .tar.gz file of my project, but I excluded the cljs.jar file from the root. (The IP address in core.cljs is the public IP address of the ec2 instance I was using.)

The command I used to start the repl is:

java -cp cljs.jar:src clojure.main repl.clj

REPL output:

cljs.user=> (doc enable-console-print!)
-------------------------
cljs.core/enable-console-print!
([])
  Set *print-fn* to console.log
nil
cljs.user=> (source enable-console-print!)
(defn enable-console-print!
  "Set *print-fn* to console.log"
  []
  (set! *print-newline* false)
  (set! *print-fn*
    (fn [& args]
      (.apply (.-log js/console) js/console (into-array args))))
  (set! *print-err-fn*
    (fn [& args]
      (.apply (.-error js/console) js/console (into-array args))))
  nil)
nil
cljs.user=> (enable-console-print!)
nil
cljs.user=> enable-console-print!
Exception in thread "Thread-138" java.lang.RuntimeException: EOF while reading string
        at clojure.lang.Util.runtimeException(Util.java:221)
        at clojure.lang.LispReader$StringReader.invoke(LispReader.java:525)
        at clojure.lang.LispReader.read(LispReader.java:263)
        at clojure.lang.LispReader.readDelimitedList(LispReader.java:1200)
        at clojure.lang.LispReader$MapReader.invoke(LispReader.java:1158)
        at clojure.lang.LispReader.read(LispReader.java:263)
        at clojure.lang.LispReader.read(LispReader.java:196)
        at clojure.lang.LispReader.read(LispReader.java:185)
        at clojure.lang.RT.readString(RT.java:1821)
        at clojure.lang.RT.readString(RT.java:1816)
        at clojure.core$read_string.invoke(core.clj:3663)
        at cljs.repl.server$dispatch_request.invoke(server.clj:148)
        at cljs.repl.server$handle_connection.invoke(server.clj:157)
        at cljs.repl.server$server_loop$fn__5056.invoke(server.clj:167)
        at clojure.core$binding_conveyor_fn$fn__4444.invoke(core.clj:1916)
        at clojure.lang.AFn.run(AFn.java:22)
        at java.lang.Thread.run(Thread.java:745)


 Comments   
Comment by Mike Fikes [ 31/Jan/16 8:34 PM ]

I tried reproducing with the attached cljs-hello-world.tar.gz, revising the IP address to be localhost, and running everything on my local Mac with Chrome 48.0.2564.97, and I was unable to repro.

Perhaps, since this involved a network connection between AWS and your local Mac, the connection was closed during the phase when it was reading the response? In other words, I wonder if this is reproducible in general or was just a connectivity outage.

Comment by Scott Bale [ 08/Feb/16 8:33 PM ]

Thanks for checking on that Mike. I'm also unable to reproduce the crash if I run my example locally on a macbook pro (Chrome 48.0.2564.97 also; Mac's Java 8 build 1.8.0_65-b17).

However, the original behavior I'm seeing is not sporadic; it is reproducible 100% of the time. I was able to reproduce it again just now. So I guess the next step would be for me to build a debug version of cljs.jar which provides some insight into what is tripping up the LispReader$StringReader.

Comment by David Nolen [ 12/Feb/16 2:29 PM ]

Patch welcome for this one, not really use case we ever had in mind for Browser REPL.

Comment by Scott Bale [ 05/Mar/16 1:16 PM ]

I've tinkered around with this enough to satisfy myself that it is nothing to do with cljs. Sorry for the distraction.

Fwiw: I have a small clj test script that reads the body of a POST (similar to cljs browser repl env). If I run that within an ec2 instance, and send a POST from outside of ec2, then about half the time a large chunk of the post body is truncated. Length is reported correctly in header, but a bunch of characters are just missing. Openjdk bug? >shrug<





[CLJS-1587] Duplicate keys via quoting Created: 24/Feb/16  Updated: 03/Mar/16

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

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


 Description   

For

#{1 '1}

you get

#{1 1}


 Comments   
Comment by Peter Schuck [ 03/Mar/16 10:01 PM ]

This happens for the has-set macro and the hash-set literal. Here's what I get from the repl

cljs.user=> (hash-set 1 '1 2 '2 3 '3 4 '4 5)
#{1 2 3 4 5}
cljs.user=> (hash-set 1 '1 2 '2 3 '3 4 '4)
#{1 1 2 2 3 3 4 4}
cljs.user=> #{ 1 '1 2 '2 3 '3 4 '4}
#{2 1 4 4 3 2 1 3}
cljs.user=> #{ 1 '1 2 '2 3 '3 4 '4 5}
#{2 1 4 4 3 2 5 1 3}
cljs.user=> #{ 1 '1 2 '2 3 '3 4 '4 5 '5}
#{2 5 1 4 4 3 2 5 1 3}
cljs.user=> (apply hash-set [1 '1 2 '2 3 '3 4 '4])
#{1 2 3 4}

Calling hash-set as a function gives the correct results. The hash-set macro gives the incorrect results until we have more then 8 elements and uses the fromArray method on PersistentHashSet to build the set instead of creating a literal PersistentArrayMap for the set. The literal notation is incorrect no matter how many elements there are.





[CLJS-1074] Externs inference Created: 02/Mar/15  Updated: 01/Mar/16

Status: In Progress
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3211
Fix Version/s: GSoC

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Maria Geller
Resolution: Unresolved Votes: 3
Labels: None


 Description   

Given all externs generally need to be supplied for js/foo we could probably automatically compute externs based on js/foo usage in user code. For this to work correctly we need to account for property access through js/foo i.e. (def Bar js/foo.Bar). This should infer that Bar is also a foreign object. Things gets more complicated for higher order cases, we probably want to support a ^js type hint.

Finally externs inference needs to account for externs likely already supplied by the user - i.e. don't emit dupes, Google Closure will complain.



 Comments   
Comment by Leon Grapenthin [ 27/Feb/16 3:17 PM ]

Is this still being worked on?

Here is an approach: https://gist.github.com/Chouser/5796967

A very lean first approach would be to generate a `var foo = {}` for every interop expression.

I. e. by experimentation I could observe that no nested statements or var foo = function() statements are required to prevent minification.

js/foo 
js/foo.Bar 
(js/foo.Bar) 
(.-Bar js/foo) 
(.-Bar x) 
;; etc... would all not be minified with 
var foo = {}; 
var Bar = {};

To prevent dupes a cheap way to go would be a CLJS compiler mode in which no extern files are loaded. We can disable Closures externs via the exclude_default_externs compiler flag.

IDK if the minification quality is in any way different if the externs are type annotated or declared nested of with =function() --?

At least it looks like doing this would automate the most common use case of externs in CLJS: Preventing minification.

Comment by David Nolen [ 29/Feb/16 9:05 PM ]

Not actively being worked on at the moment but Maria Geller has a pretty solid proof of concept in a branch that somebody else can pick up. It takes the basic idea from that gist much further.

Comment by Leon Grapenthin [ 01/Mar/16 12:41 AM ]

Branch for reference: https://github.com/mneise/clojurescript/commits/CLJS-1074

Thanks David. Will have a closer look asap.





[CLJS-1586] Self-host: No duplicate key error Created: 23/Feb/16  Updated: 26/Feb/16  Resolved: 26/Feb/16

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: Not Reproducible Votes: 0
Labels: bootstrap


 Description   

If you evaluate a set or map with duplicate keys, no error is caused as is normally done:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 54226
To quit, type: :cljs/quit
cljs.user=> (require 'cljs.js)
nil
cljs.user=> (cljs.js/eval-str (cljs.js/empty-state)
  "#{1 1}" nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value #{1}}
cljs.user=> (cljs.js/eval-str (cljs.js/empty-state)
  "{1 1 1 2}" nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value {1 2}}


 Comments   
Comment by Mike Fikes [ 23/Feb/16 10:46 AM ]

Appears to be upstream: http://dev.clojure.org/jira/browse/TRDR-35





[CLJS-1583] (hash (symbol "/")) does not match (hash '/) Created: 22/Feb/16  Updated: 26/Feb/16  Resolved: 26/Feb/16

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

Type: Defect Priority: Minor
Reporter: Nick Vrvilo Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: bug, cljs

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

 Description   

(== (hash '/) (hash (symbol "/"))) ; returns false
Obviously we would expect the result to be true.

I believe that (symbol "/") is incorrectly treating the slash as the namespace/name separator.
I think that updating the condition at src/main/cljs/cljs/core.cljs:981 as follows might fix the bug:

(let [idx (.indexOf name "/")]

  • (if (== idx -1)
    + (if (> idx 0)
    (symbol nil name)

Alternatively, since this issue seems to be related to CLJ-179 (resolved) in the Clojure compiler, a similar patch would probably also solve this issue:

(let [idx (.indexOf name "/")]

  • (if (== idx -1)
    + (if (or (== idx -1) (= name "/"))
    (symbol nil name)


 Comments   
Comment by David Nolen [ 22/Feb/16 2:31 PM ]

Patch welcome.

Comment by Nick Vrvilo [ 22/Feb/16 5:16 PM ]

symbol function fix + non-regression tests

Comment by Mike Fikes [ 25/Feb/16 12:49 PM ]

I can confirm that this patch works for me. Specifically, in bootstrap, if you try to look in the compiler analysis cache for /, this patch fixes things for this case making this work:

(get-in @st [:cljs.analyzer/namespaces 'cljs.core :defs '/])

and lots of things surrounding cljs.core// start to work, like doc, etc.

Comment by Mike Fikes [ 25/Feb/16 12:54 PM ]

Hey Nick, have you signed the Clojure CA? (You don't appear to be listed http://clojure.org/community/contributors)

Comment by Nick Vrvilo [ 25/Feb/16 1:12 PM ]

Hi Mike. I've received the confirmation email that my CA was successfully filed. My name should pop up on the list whenever it does a refresh. Thanks for confirming the patch!

Comment by David Nolen [ 26/Feb/16 1:39 PM ]

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





[CLJS-1593] Self-host: Munged minus macro Created: 25/Feb/16  Updated: 25/Feb/16

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

Type: Defect Priority: Trivial
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bootstrap


 Description   

In bootstrap, the macro form of cljs.core/- is evidently available as _ so, for example

(_ 7 3)
works.

Repro:

cljs.user=> (require 'cljs.js)
nil
cljs.user=> (cljs.js/eval-str (cljs.js/empty-state)
  "(_ 7 3)" nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value 4}





[CLJS-1420] get-in behavior differs from Clojure by always deferring to the 3 arity fn Created: 12/Aug/15  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

Type: Defect Priority: Minor
Reporter: Kevin Webster Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs_1420.patch     Text File CLJS-1420.patch    

 Description   

CLJS-464 should be reverted since the commit changes how get-in is handled by types that implement ILookup.

Since get-in always defers to the 3 arity function, the 2 arity ILookup -lookup fn never gets called. This behavior is inconsistent with how Clojure behaves.

clj:

(get-in
  (reify
    clojure.lang.ILookup
    (valAt [o k] :2-arity)
    (valAt [o k not-found] :3-arity))
  [:foo])
=> :2-arity

cljs:

(get-in
  (reify
    ILookup
    (-lookup [o k] :2-arity)
    (-lookup [o k not-found] :3-arity))
  [:foo])
=> :3-arity


 Comments   
Comment by Kevin Webster [ 12/Aug/15 3:30 PM ]

Tested patch in both V8 & Nashorn

Comment by David Nolen [ 14/Aug/15 7:51 PM ]

Please modify the patch to include a test case. The patch should be squashed into a single commit.

Comment by Nate Wildermuth [ 23/Feb/16 3:07 PM ]

Added test to original patch. Test passes on V8, SpiderMonkey, JavaScriptCore, Nashorn.

Comment by David Nolen [ 23/Feb/16 4:10 PM ]

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





[CLJS-1585] Self-host: Alias-scoped keywords Created: 22/Feb/16  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

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

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

 Description   

If you use an alias-scoped keyword, it is not expanded properly in bootstrapped ClojureScript.

A concrete working example in a REPL of this concept:

cljs.user=> (require '[clojure.string :as string])
nil
cljs.user=> ::string/bar
:clojure.string/bar

Here is a minimal repo showing that expansion doesn't properly occur with self-host:

Evaluate these forms in script/noderepljs:

(require 'cljs.js)

(def st (cljs.js/empty-state))

(cljs.js/eval-str st
  "(ns alpha.core (:require [foo.core :as foo]))"
  nil
  {:ns      'cljs.user
   :context :expr
   :eval    cljs.js/js-eval
   :load    (fn [_ cb]
              (cb {:lang :clj :source "(ns foo.core)"}))}
  identity)

(cljs.js/eval-str st
  "::foo/bar"
  nil
  {:ns      'alpha.core
   :context :expr
   :eval    cljs.js/js-eval}
  identity)

That last form produces:

{:ns alpha.core, :value :foo/bar}

It is expected to instead produce:

{:ns alpha.core, :value :foo.core/bar}


 Comments   
Comment by Mike Fikes [ 22/Feb/16 9:31 PM ]

Bind cljs.tools.reader/*alias-map* so that alias-scoped keywords expand properly. Simply pass in the :requires map from the compiler state for the current namespace.

Comment by Mike Fikes [ 23/Feb/16 9:25 AM ]

Attached CLJS-1585-2.patch to cleanly apply to master.

Note, if the patch in CLJS-1577 has already been applied, then, owing to some nearby edits a three-way patch will apply cleanly:

git am -3 CLJS-1585-2.patch

Comment by Mike Fikes [ 23/Feb/16 3:05 PM ]

Attached CLJS-1585-3.patch, revised to apply cleanly.

Comment by David Nolen [ 23/Feb/16 3:15 PM ]

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





[CLJS-1577] Self-host: syntax-quote resolves on dot forms Created: 18/Feb/16  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

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

 Description   

In Clojure or ClojureScript, if you evaluate a form involving syntax-quote of a dot form, the instance member / field, is not resolved. For example, in ClojureScript,

`(.x o)

yields

(.x cljs.user/o)

With bootstrap ClojureScript, the .x symbol is resolved, resulting in /x:

$ script/noderepljs
ClojureScript Node.js REPL server listening on 55080
To quit, type: :cljs/quit
cljs.user=> (require 'cljs.js)
nil
cljs.user=> (cljs.js/eval-str (cljs.js/empty-state)
"`(.x o)" nil {:eval cljs.js/js-eval :context :expr} identity)
{:ns cljs.user, :value (/x cljs.user/o)}

Note: See TRDR-34 . I've also confirmed that

(defn- resolve-symbol
  [sym]
  (if (string/starts-with? (str sym) ".")
    sym
    (ana/resolve-symbol sym)))

if used in cljs.js in lieu of binding ana/resolve-symbol actually fixes this particular ticket (but it isn't clear if this is a hack that doesn't cover all bases or is misplaced in some way).



 Comments   
Comment by Mike Fikes [ 23/Feb/16 8:47 AM ]

Attached revised CLJS-1577-2.patch to apply cleanly to master.

Comment by David Nolen [ 23/Feb/16 2:53 PM ]

fixed https://github.com/clojure/clojurescript/commit/22a2692c9fcbae7a002ddedc088e50c7c2cbccfe





[CLJS-1564] Self-host: cached macro *loaded* update Created: 07/Feb/16  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

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

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

 Description   

When using cljs.js/require, the cljs.js/*loaded* atom is updated reflecting loaded namespaces. When loading a cached macro namespace, the non-macros namespace name is added to the *loaded* set.

Minimal repo, with script/noderepljs:

$ script/noderepljs 
ClojureScript Node.js REPL server listening on 52507
To quit, type: :cljs/quit
cljs.user=> (require '[cljs.js :as cljs])
nil
cljs.user=> ;; Load a macros namespace
cljs.user=> (cljs/require
  {}
  'foo.core
  :reload-all
  {:macros-ns true
   :load      (fn [_ cb] (cb {:lang   :clj
                              :source "(ns foo.core)"}))
   :eval      (constantly nil)}
  (fn [_]
    (prn @cljs/*loaded*)))
#{foo.core$macros}
nil
cljs.user=> ;; Do the same again, but return cached js
cljs.user=> (cljs/require
  {}
  'foo.core
  :reload-all
  {:macros-ns true
   :load      (fn [_ cb] (cb {:lang   :js
                              :source ""}))
   :eval      (constantly nil)}
  (fn [_]
    (prn @cljs/*loaded*)))
#{foo.core}
nil
cljs.user=> ;; Note that *loaded* now has #{foo.core}


 Comments   
Comment by Mike Fikes [ 07/Feb/16 10:44 PM ]

This was a simple oversight with the changes made for CLJS-1504: One spot was missed in the places where name needed to be converted to aname.

With this change, the repo in the description goes away and the included additional unit tests covering *loaded* pass.

Additionally in a downstream cljs.js client, where a regular namespace refers several symbols from a cached macros namespace, it can be seen that loading of the cached namespace occurs once and only once, as opposed to an additional time for each referred symbol as :use-macros is being processed.

Comment by David Nolen [ 12/Feb/16 2:35 PM ]

Bizarrely when I apply this one to master I get reader errors. Can anyone else reproduce this?

Comment by Mike Fikes [ 12/Feb/16 10:32 PM ]

I can't repo any reader errors. I applied it against master, and ran through the stuff in the ticket description, ran the self-host test, and the regular tests, and everything appeared normal for me. (This was against 1.8.2.)

Comment by Mike Fikes [ 23/Feb/16 8:38 AM ]

Attached revised CLJS-1564-2.patch which fixes new test interference issue on master (over-use of foo.core namespace in the tests).

Comment by David Nolen [ 23/Feb/16 2:50 PM ]

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





[CLJS-1573] Self-host: Invalid UTF escaping in cljs-in-cljs Created: 15/Feb/16  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

Type: Defect Priority: Minor
Reporter: Nikita Beloglazov Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

Evaluating following "clojurescript" string produces invalid "javascript" string:
"90°" => "90\ub0"
Correct string is "90\u00b0"

Also the character escaped correctly when compiling using regular jvm-based compiler.

Repro in script/noderepljs:

cljs.user=> (require 'cljs.js)
nil
cljs.user=> (cljs.js/eval-str (cljs.js/empty-state) 
  "\"90°\"" nil {:eval cljs.js/js-eval :context :expr} identity)
{:error #error {:message "ERROR", :data {:tag :cljs/analysis-error}, :cause #object[SyntaxError SyntaxError: Unexpected token ILLEGAL]}}
cljs.user=> (cljs.js/compile-str (cljs.js/empty-state)
"\"90°\"" nil {:context :expr} identity)
{:value "\"90\\ub0\""}


 Comments   
Comment by Mike Fikes [ 20/Feb/16 3:31 PM ]

The underlying problem is that the self-hosted compiler was not zero-padding \u characters to be 4 nybbles long.

Comment by David Nolen [ 23/Feb/16 7:53 AM ]

fixed https://github.com/clojure/clojurescript/commit/16666f37cc13ead5a66330046db82a2976b6f1f0

Comment by Mike Fikes [ 23/Feb/16 9:48 AM ]

Confirmed fixed downstream with Planck:

cljs.user=> "90°"
"90°"





[CLJS-1298] source-on-disk conditional should include :source-url Created: 05/Jun/15  Updated: 23/Feb/16  Resolved: 05/Jan/16

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

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

All



 Description   

cljs.closure/source-on-disk is conditional on having the IJavaScript having a :source-url. Currently this conditional is exercised after the call to util/ext. But util/ext has a precondition that will fail on a nil :source-url.

Solution: protect util/ext by checking for :source-url before calling util/ext.

(defn source-on-disk
  "Ensure that the given IJavaScript exists on disk in the output directory.
  Return updated IJavaScript with the new location if necessary."
  [opts js]
  (if (write-js? js)
    (write-javascript opts js)
    ;; always copy original ClojureScript sources to the output directory
    ;; when source maps enabled
    (let [out-file (when-let [ns (and (:source-map opts)
                                      (:source-url js) ;;<--- ADD THIS HERE
                                      (first (:provides js)))]
                     (io/file (io/file (util/output-directory opts))
                       (util/ns->relpath ns (util/ext (:source-url js)))))
          source-url (:source-url js)]
      (when (and out-file source-url
                 (or (not (.exists ^File out-file))
                     (> (.lastModified (io/file source-url))
                        (.lastModified out-file))))
        (spit out-file (slurp source-url)))
      js)))

Or some refactoring of the above.



 Comments   
Comment by David Nolen [ 05/Jun/15 2:20 PM ]

In what cases will an IJavaScript not have a :source-url? Just trying to get some context.

Comment by Bruce Hauman [ 05/Jun/15 2:42 PM ]

Good question. When the sources are already output javascript and they have an output dir relative :url but not a :source-url, this doesn't seem like it should happen.

I'm getting a situation like this

:url #object[java.net.URL 0x5f921081 "file:/Users/brucehauman/workspace/temp/hello-world/resources/goog/uri/utils.js"]
:source-url nil

Comment by David Nolen [ 05/Jun/15 2:58 PM ]

Ok that seems like the real underlying issue. Do you have a small reproducer by chance?

Comment by Bruce Hauman [ 05/Jun/15 3:08 PM ]

It seems like its when the :asset-path is "". And :output-dir is at the root of a resource path like :output-dir "resources".

Comment by Sebastian Bensusan [ 14/Oct/15 4:14 PM ]

I have been able to reproduce the issue here with the following build script:

(cljs.build.api/build "src"
  {:main 'cljs-1298.core
   :output-to "resources/cljs_1298.js"
   :output-dir "resources"
   :asset-path "resources"  ;; <- Note that is not "" as described earlier
   :verbose true})

As described in the ticket, the error is an AssertionError[1] from a call to (cljs.util/ext nil) during compilation.

[1] AssertionError Assert failed: (or (file? x) (url? x) (string? x)) cljs.util/ext (util.cljc:124)

Comment by David Nolen [ 14/Oct/15 4:34 PM ]

Ok so we have a counter example, but still no clue about how the failure comes to be?

Comment by Sebastian Bensusan [ 15/Oct/15 8:25 AM ]

I think it's a classpath clash of resources. If for some reason a IJavaScript has an :url that is already on the :output-dir then the error happens. This happens when the :output-dir is exactly loaded under resources because (io/resources "goog/base.js") is used.

Longer explanation:

After some investigation, I can reproduce it only when both the source folder and the :output-dir/:asset-path are on the classpath. If "src" is not in the classpath it doesn't happen.

The first file to trigger the error is "goog/base.js" but only in the second run of cljs.closure/build. In the first run, "goog/base.js" has the following IJavaScript value:

#cljs.closure.JavaScriptFile{:foreign nil,
:url #object[java.net.URL 0x212b731a jar:file:/home/carlos/.m2/repository/org/clojure/google-closure-library/0.0-20150805-acd8b553/google-closure-library-0.0-20150805-acd8b553.jar!/goog/base.js],
:source-url nil, 
:provides (goog), :requires (), :lines nil, :source-map nil}

which has :url in a jar and triggers cljs.closure/write-javascript.

In the second run, goog/base.js is searched under resources:

(defn add-dependencies [opts & inputs]
  ...
  (javascript-file nil (io/resource "goog/base.js") ["goog"] nil)
  ...)

and io/resources finds it on the :output-dir which is also in resources, which results in this IJavaScript value:

#cljs.closure.JavaScriptFile{:foreign nil, 
:url #object[java.net.URL 0x7879862 file:/home/carlos/OpenSource/clojurescript/resources/goog/base.js],
:source-url nil,
:provides (goog), :requires (),
:lines nil, :source-map nil}

which results in the error since it doesn't trigger write-js? and then has no :source-map.

Comment by Sebastian Bensusan [ 15/Oct/15 8:29 AM ]

Also, Bruce's proposed fix properly sweeps the issue under the rug.

Comment by David Nolen [ 05/Jan/16 4:30 PM ]

fixed https://github.com/clojure/clojurescript/commit/17e167effe2c62f988e659f812dfecb8b4b8f26f





[CLJS-1584] Self-host: core/str error with condp Created: 22/Feb/16  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

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

 Description   
cljs.user=> (cljs.js/eval-str (cljs.js/empty-state)
"(condp = 1)" nil {:eval cljs.js/js-eval :context :expr} identity)
WARNING: No such namespace: core, could not locate core.cljs, core.cljc, or Closure namespace "" at line 1 
WARNING: Use of undeclared Var core/str at line 1 
{:error #error {:message "ERROR", :data {:tag :cljs/analysis-error}, :cause #object[ReferenceError ReferenceError: core is not defined]}}

Analysis:
The condp macro uses core/str in a syntax-quote context, while every other place it appears that cljs.core/str is used in that context.



 Comments   
Comment by Andrea Richiardi [ 22/Feb/16 10:57 PM ]

I confirm it works.

Testing with Node

Testing self-host.test

Ran 7 tests containing 45 assertions.
0 failures, 0 errors.
$ g ln | head -1
* bf5e075 (HEAD -> master) CLJS-1584: Self-host: core/str error with condp
Comment by Mike Fikes [ 23/Feb/16 8:11 AM ]

Attached CLJS-1584-2.patch with revisions to apply cleanly to master.

Comment by David Nolen [ 23/Feb/16 8:15 AM ]

fixed https://github.com/clojure/clojurescript/commit/2023353d635578a0666a50c3305bf92347185646





[CLJS-1521] Self-host: Macro namespaces cannot be aliased Created: 23/Dec/15  Updated: 23/Feb/16  Resolved: 23/Feb/16

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

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

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

 Description   

If, for example, you employ code like

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

you cannot successfully invoke macros in the foo.core namespace using the alias foo.

Will supply a battery of unit tests that exercise various ways in which you can make use of macros and in those tests, the ones involving alias will fail without the patch.



 Comments   
Comment by Mike Fikes [ 23/Dec/15 9:25 PM ]

Attached patch fixes things so you can use namespace aliases when invoking macros in bootstrapped ClojureScript.

Comment by Andrea Richiardi [ 23/Dec/15 9:53 PM ]

I will try it in replumb for issue 108.

Comment by Mike Fikes [ 24/Dec/15 8:26 AM ]

CLJS-1521-2.patch contains the same content as CLJS-1521.patch but moves a form so it can be applied with the patch in CLJS-1515.

Comment by Andrea Richiardi [ 24/Dec/15 7:18 PM ]

I applied the patch locally with replumb and I reciprocate the LGTM (while before it was failing).

...:~/git/replumb (cljs-1.7.202 $=)$ rlwrap lein node-repl
...
cljs.user=> (ns my.namespace (:require-macros [foo.bar.baz :as f]))
nil
my.namespace=> (f/mul-baz 10 20)
200
Comment by Nikita Beloglazov [ 20/Feb/16 3:21 PM ]

This patch also fixes similar problem with (:require [foo.core :as foo :include-macros true]) described in https://groups.google.com/forum/#!topic/clojurescript/ya0-38K4WVU

Comment by David Nolen [ 23/Feb/16 7:57 AM ]

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





[CLJS-1561] WARN if recur passes non-inferred type Created: 06/Feb/16  Updated: 23/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Take this code as an example:

(defn f [^boolean b]
  (loop [x b]
    (if x
      (recur 0)
      :done)))

The type of x is inferred to be Boolean, but there is a recur form that can be statically deduced to be passing a non-Boolean.

This ticket asks that a WARN be issued for this case, and perhaps others (where maybe x itself is directly type hinted).



 Comments   
Comment by Mike Fikes [ 06/Feb/16 2:59 PM ]

Attached a patch which warns on for the case of boolean and number, since those two types have special handling.

Some example usage:

cljs.user=> (defn f [^boolean b]
       #_=>   (loop [x b]
       #_=>     (if x
       #_=>       (recur 0)
       #_=>       :done)))
WARNING: recur target parameter x has inferred type boolean, but being passed type number at line 4 
#'cljs.user/f
cljs.user=> (loop [x 1 y true z :hi]
       #_=>   (when false (recur 'a "hi" nil)))
WARNING: recur target parameter x has inferred type number, but being passed type cljs.core/Symbol at line 2 
WARNING: recur target parameter y has inferred type boolean, but being passed type string at line 2 
nil
cljs.user=> (loop [x 1 y true]
       #_=>  (when false (recur nil nil)))
WARNING: recur target parameter x has inferred type number, but being passed type clj-nil at line 2 
WARNING: recur target parameter y has inferred type boolean, but being passed type clj-nil at line 2 
nil
cljs.user=> (loop [x 1]
       #_=>   (let [y (inc x)]
       #_=>     (when false (recur (inc y)))))
nil
cljs.user=> (loop [b true]
       #_=>   (when false (recur (inc 1))))
WARNING: recur target parameter b has inferred type boolean, but being passed type number at line 2 
cljs.user=> (loop [x 1] 
       #_=>   (inc x) 
       #_=>     (when false (recur :hi)))
WARNING: recur target parameter x has inferred type number, but being passed type cljs.core/Keyword at line 3 
nil
cljs.user=> (loop [x :hello] 
       #_=>   (inc x) 
       #_=>     (when false (recur :hi)))
WARNING: cljs.core$macros/+, all arguments must be numbers, got [cljs.core/Keyword number] instead. at line 2 
nil




[CLJS-1572] REPL doesn't give error for expressions with too many right parentheses. Created: 15/Feb/16  Updated: 22/Feb/16

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

Type: Defect Priority: Minor
Reporter: J David Eisenberg Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: repl
Environment:

Fedora 23, java version "1.8.0_40", javac 1.8.0_40, clojure 1.7.0


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

 Description   

I was expecting an error message from this; using [org.clojure/clojurescript "1.7.228"]; the Clojure REPL does produce an error.

To quit, type: :cljs/quit
cljs.user=> (+ 3 5)))))
8


 Comments   
Comment by Mike Fikes [ 16/Feb/16 12:49 PM ]

A suggestion on a strategy to fix this: Make the ClojureScript REPL sequentially process all of the forms it can read on a line, just like the Clojure REPL does:

user=> 3 (+ 3 5) 7
3
8
7

If this is done, then the fix for this ticket will fall out “for free” and the ClojureScript REPL will error when it hits a form that appears to start with ).

Comment by Mike Fikes [ 21/Feb/16 4:01 PM ]

The REPL code is very close to working the way mentioned in the previous comment. It currently does not only because this line

https://github.com/clojure/clojurescript/blob/c59e957f6230c07e7a228070dd8eb393d5b8ce40/src/main/clojure/cljs/repl.cljc#L100

invokes code that causes a new PushbackReader to wrap things (discarding things):

https://github.com/clojure/clojurescript/blob/c59e957f6230c07e7a228070dd8eb393d5b8ce40/src/main/clojure/cljs/repl.cljc#L773-L775

If you either let the PushbackReader once and let that reader fn close over it, or otherwise comment out things so that a new PushbackReader is not created for each loop / recur, you will see that the code behaves as suggested in the previous comment, having the desired effect.

The only thing I see that would need to be additionally sorted out with such a patch is being a little more clever about when need-prompt evaluates to true, etc. (otherwise polishing thing so there are no missed corner cases).

Comment by Mike Fikes [ 21/Feb/16 11:02 PM ]

Attached a patch that, in essence makes the ClojureScript REPL behave like the Clojure REPL with respect to multiple items on a line and with respect to detecting malformed input. The patch is fairly straightforward, but could use some testing. I've tried things like

cljs.user=> 3_    ; where _ here is a space

cljs.user=> 3 4 5

cljs.user=> 3)

cljs.user=> 3))

cljs.user=> 3 [4
5]

cljs.user=> (let [x 1]
(+ 1 "a"))         ;; testing to make sure line numbers are right

All the above is looking good to me.

Here is the commit comment:

Take extra care to preserve the state of in so that anything beyond
the first form remains for reading. This fundamentally makes the
ClojureScript REPL behave like the Clojure REPL. In particular, it
allows entering multiple forms on a single line (which will be evaluated
serially). It also means that if malformed input lies beyond the initial
form, it will be read and will cause an exception (just like in the
Clojure REPL).

The bulk of the complexity in this commit has to do with the case where
a new line-numbering reader is established, so that errors in forms
can be associated with line numbers, starting with line 1 being the
first line of the form. This requires a little extra handling because
the source-logging-push-back-reader introduces an extra 1-character
buffer which must be transferred back to the original (pre-bound) in,
otherwise things like an unmatched extra paren right after a well-formed
form won't be detected (as the paren would be in the 1-char buffer and
discarded.)

Also, a Java PushbackReader needs to be eliminated, as it causes things
to fail to behave like the Clojure REPL.

Comment by Mike Fikes [ 21/Feb/16 11:14 PM ]

Note that one extremely useful thing this patch enables is pasting of multiple forms into a ClojureScript REPL!

This fails if pasted using the current cljs.jar, but works with the patch applied:

(def a 1)

(def b 2)

(def c (+ a b))

c




[CLJS-1569] IndexedSeq doesn't implement IWithMeta / IMeta Created: 14/Feb/16  Updated: 22/Feb/16  Resolved: 15/Feb/16

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

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bug
Environment:

affects master


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

 Description   

this is possible in clojure:

user=> (meta (with-meta (seq [1 2 3]) {:a 1}))
{:a 1}

while it throws in CLJS:

cljs.user=> (meta (with-meta (seq [1 2 3]) {:a 1}))
No protocol method IWithMeta.-with-meta defined for type cljs.core/IndexedSeq: (1 2 3)
     cljs$core$missing_protocol (cljs/core.cljs:264:4)
     cljs$core$_with_meta (cljs/core.cljs:570:1)
     cljs$core$with_meta (cljs/core.cljs:1845:8)


 Comments   
Comment by António Nuno Monteiro [ 14/Feb/16 6:40 PM ]

submitted patch with fix and test

Comment by David Nolen [ 15/Feb/16 4:10 PM ]

This needs to be rebased on master it.

Comment by António Nuno Monteiro [ 15/Feb/16 4:36 PM ]

attached updated patch based on current master

Comment by David Nolen [ 15/Feb/16 4:42 PM ]

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

Comment by Mike Fikes [ 15/Feb/16 11:26 PM ]

Confirmed fixed using downstream client of bootstrapped ClojureScript.

Comment by Matt Huebert [ 22/Feb/16 11:56 AM ]

Just ran into this issue - can also confirm the fix in master works for me





[CLJS-1494] turn cljs.core/*assert* into a goog-define Created: 25/Nov/15  Updated: 22/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File goog-define-assert.patch    
Patch: Code

 Description   

This patch turns the cljs.core/*assert* boolean into a goog.define and also checks *assert* at runtime (instead of only at compile-time).

The closure define option allows the closure compiler to eliminate asserts in :advanced, while :none builds can keep the asserts. This is one of the few remaining issues that prevent :advanced builds to re-use :none compiled (cached) files.

:elide-asserts is unaffected to keep this as simple as possible, but could be built on top of the goog.define instead of actually affecting the compiled output.



 Comments   
Comment by Mike Fikes [ 20/Feb/16 8:02 AM ]

Patch no longer applies, probably owing to CLJS-970.

Comment by Thomas Heller [ 22/Feb/16 5:08 AM ]

There was one more issue I discovered with my approach. My goal was to enable the Closure Compiler to eliminate the asserts when using :advanced compilation. This works perfectly fine with using a goog.define for *assert* but the compiler will complain if you try to adjust the define later since goog.define vars are not allowed to be adjusted at runtime.

(binding [*assert* false]
  (something-that-asserts))

This works in CLJ but not in CLJS since *assert* is only checked at compile time. If compiled with :elide-asserts true you can't bind assert to true either since the code no longer exists.

So some compromise must be made either way, the best solution IMHO would be to have a goog.define which lets the compiler decide whether to eliminate the asserts or not, independent from the *assert* and then moving the assert check itself into js instead of the compiler.

Happy to write the patch if interested.





[CLJS-1574] CLJS string equivalence is very slow in Chrome Created: 16/Feb/16  Updated: 22/Feb/16

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

Type: Defect Priority: Minor
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

Google Chrome 48.0.2564.109 on Mac OS X 10.11.3
Safari 9.0.3 (11601.4.4) on Mac OS X 10.11.3



 Description   

Clojurescript's equivalence for strings in Google Chrome is ~1000 times slower than equivalent javascript functionality, and ~1000 times slower than the same function in Safari.

Google Chrome
js equiv:  0.005 seconds
cljs equiv:  1.898 seconds
Safari
js equiv:  0.005 seconds
cljs equiv:  0.006 seconds
(def size (* 128 1024))

(defn now []
  (.getTime (js/Date.)))

(defn delta [b a]
  (str (/ (- b a) 1000) " seconds"))

(deftest test-js-eq-perf
  (let [str1  (apply str (repeat size "a"))
        str2  (apply str (repeat size "a"))
        start (now)
        _     (is (js* "~{} == ~{}" str1 str2)
                  "js equivalence")
        end   (now)
        ]
    (println "js equiv: " (delta end start))))

(deftest test-cljs-eq-perf
  (let [str1  (apply str (repeat size "a"))
        str2  (apply str (repeat size "a"))
        start (now)
        _     (is (= str1 str2)
                  "cljs equivalence")
        end   (now)
        ]
    (println "cljs equiv: " (delta end start))))


 Comments   
Comment by Stephen Nelson [ 16/Feb/16 6:06 PM ]

This bug only occurs when cljs.pprint has been required.

Comment by Stephen Nelson [ 16/Feb/16 6:38 PM ]

After a whole lot of binary search, here's a minimal reproduction. When cljs.pprint is loaded it constructs write-option-table. It seems that constructing a hash map with the keys :added and :ns causes a call to (= :added :ns), which is sufficient to cause string equality to become extremely slow.

(ns hello-world.core)

(enable-console-print!)

(def size (* 128 1024))

(defn now []
  (.getTime (js/Date.)))

(defn delta [b a]
  (str (/ (- b a) 1000) " seconds"))

(defn test [] 
  (let [str1  (apply str (repeat size "a"))
        str2  (apply str (repeat size "a"))
        start (now)
        _     (= str1 str2)
        end   (now)
        ]
      (println "cljs equiv: " (delta end start))))

(test)

(= :added :ns)

(test)
Comment by Peter Schuck [ 17/Feb/16 4:50 PM ]

Is the ClojureScript compiled with options :optimizations :advanced or :static-fns true? Compiling ClojureScript without those options results in call indirection for all function calls which might explain the slowdown. See http://swannodette.github.io/2015/03/16/optimizing-clojurescript-function-invocation/ for more information.

Comment by Stephen Nelson [ 17/Feb/16 9:06 PM ]

This happens with :advanced, :simple, and without optimisations. Stepping through the generated javascript seems to indicated that the slow down comes from the VM's internal function dispatch. Regardless, I don't think that the extra function calls related to dynamic dispatch in clojurescript could add minutes of overhead per call. Note that the test case above only uses 128k of string data, the case where I encountered this issue first used ~512K and took about 5 minutes to complete a single function call.

Comment by Francis Avila [ 17/Feb/16 9:14 PM ]

I have reproduced this in Chrome for Linux, on :none and :advanced optimization levels using different test code. I verified the result of the compare so the JIT won't optimize it away and I used performance.mark() and performance.measure() for timing, although none of this should have mattered.

Every subsequent string compare after the first -equiv-invoking use of equal is significantly slower for no reason I can see. There are no intermediate GCs or anything to suggest that it should be slower--it just takes longer! The only thing I can think of is maybe the keyword-equals triggers a deopt because it makes the equal-function megamorphic, but the code is run so few times that there should not be jit optimizations kicking it at all. Also, the keyword-compare itself remains fast.

I suspect a Chrome/v8 bug. Possibly a different internal string representation kicks in for some reason which has a slower compare? This is only an issue for largish, non-constant strings, and the slowdown is proportional to string size. I'm going to try and reproduce this with pure JS.

Comment by Francis Avila [ 18/Feb/16 12:33 AM ]

All you need to reproduce this is to use the strict equality operator in a function body non-monomorphically. Subsequent executions of the function with strings (at least) which have not been compared before the polymorphic call will be very slow.

If you replace strict equality (triple-equal) with normal equality (double-equal), this issue goes away.

This is clearly a Chrome/v8 bug, but I'm not sure where to report it.

Minimal pure-javascript reproduction:

function str(size) {
  var s = "";
  for (var i = 0; i < size; i++) s += "a";
  return s;
}

function eq(x, y) {
  performance.mark("start");
  x === y; // No slowdown if use == instead
  performance.mark("end");
}

function print_measures() {
  performance.getEntriesByType("measure")
  .forEach(entry => console.log(entry.name, entry.duration));
}

var s1 = str(64 * 1024);
var s2 = str(64 * 1024);
var s3 = str(64 * 1024);

eq(s1, s2);
performance.measure("eq(s1, s2)", "start", "end");

eq(0, 0);
performance.measure("eq(0, 0)", "start", "end");

eq(s1, s3);
performance.measure("eq(s1, s3)", "start", "end");

eq(s1, s2);
performance.measure("eq(s1, s2)", "start", "end");

eq(s1, s3);
performance.measure("eq(s1, s3)", "start", "end");

print_measures();

Results with Chrome 48.0.2564.109 (64-bit) on a slow iMac with OS X 10.11.3

eq(s1, s2)   4.465000000000003     // fast string compare
eq(0, 0)     0.009999999999990905  // break monomorphism of eq()
eq(s1, s3) 259.665                 // Now string compare is slow
eq(s1, s2)   0.019999999999924967  // Repeated call still fast
eq(s1, s3) 232.52499999999998      // ... but not from after the polymorphic invoke
Comment by Francis Avila [ 22/Feb/16 3:14 AM ]

Issue added to v8: https://bugs.chromium.org/p/v8/issues/detail?id=4773





[CLJS-1581] `(vector :blah)` and `[:blah]` crashing with `inst_XXXX.call not a function` within core.async go routine Created: 19/Feb/16  Updated: 21/Feb/16  Resolved: 20/Feb/16

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

Type: Defect Priority: Minor
Reporter: Jonathan Leonard Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Mac OS X.Node 4.1.1.



 Description   

Summary says it all.

[Worked around it by using a list rather than a vector (which compiled fine)].

I don't have time to create a minimal repro at the moment but if no one can repro this and it's necessary, I can devote time to that later.



 Comments   
Comment by David Nolen [ 20/Feb/16 10:08 AM ]

This sounds like a core.async issue not a ClojureScript one.

Comment by Jonathan Leonard [ 21/Feb/16 11:13 PM ]

Actually now that I've distilled it, it is looking more like a CLJS issue. See this repro:

(def a (atom {:a {:b '() :c '()}}))
(defn- repro-async-160 []
  (go
    (let [_ (<! (to-chan [1 2 3 4]))
          new (for [order (range 100)] nil)]
      (swap! a update-in [:a] assoc :b '() :c '()))))

Note that changing `new` to `z` avoids the issue. I suspect that identifier shadowing in let binding is not playing nicely with all the other macros in play (i.e., for & go).

Comment by Jonathan Leonard [ 21/Feb/16 11:14 PM ]

Or that JavaScript's `new` is being replaced.





[CLJS-1164] quot and rem are inefficient Created: 24/Mar/15  Updated: 20/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: math

Attachments: Text File CLJS-1164-1.patch     Text File cljs-1164.patch    
Patch: Code and Test

 Description   

The implementation of the quot and rem functions are needlessly complicated. Currently they are:

(defn quot [n d] (fix (/ (- n (js-mod n d)) d)))
(defn rem [n d] (- n (* d (quot n d))))

However all numbers in js are doubles already, so all this is unnecessary:

(defn quot [n d] (fix (/ n d)))
(defn rem [n d] (js-mod n d)))

Notice that "rem" is simply js-mod, and I'm not sure why no one noticed this before. I keep js-mod for now since a lot of code uses it, and if cljs ever grows a number tower the distinction may be important.

Patch attached, which also:

  • Creates a macro version of quot and rem.
  • Updates documentation for quot, rem, js-mod and mod for clarity.
  • Implement fix (private function to round to zero) with ES6 Math.trunc() if available.

Existing quot and rem tests pass, although there could be some better tests of edge cases (negative decimal num or div, NaN and +-Infinity args).



 Comments   
Comment by Francis Avila [ 24/Mar/15 12:27 PM ]

Better tests found rounding errors in my updated rem, which should stay as-is. (Not simply js-mod after all! Seems to round args first? Not obvious from the spec.) Changed quot however is correct and introduces less error than the current one. Will update patch and tests when I get a chance.

Comment by Francis Avila [ 29/Mar/15 12:39 AM ]

Working patch with tests attached. Tests expanded to cover floating-point cases. rem is now fundamentally the same as master (was more accurate than js-mod!!), but returns results consistent with js-mod for non-finite args or zero divisor.

Comment by Mike Fikes [ 31/Jan/16 3:23 PM ]

cljs-1164.patch no longer applies on master

Comment by Andrea Richiardi [ 14/Feb/16 9:02 PM ]

Patch now applies. I only tested with Nashorn:

V8_HOME not set, skipping V8 tests
SPIDERMONKEY_HOME not set, skipping SpiderMonkey tests
JSC_HOME not set, skipping JavaScriptCore tests
Testing with Nashorn

...

Ran 185 tests containing 17195 assertions.
0 failures, 0 errors.
Tested with 1 out of 4 possible js targets
Comment by Andrea Richiardi [ 14/Feb/16 9:02 PM ]

Patch cleaned up

Comment by Mike Fikes [ 20/Feb/16 10:11 PM ]

Successfully ran Andrea's update to Francis's patch through V8, SpiderMonkey, JavaScriptCore, and Nashorn unit tests.

I also manually ran some of the unit tests in bootstrapped ClojureScript built with the patch.

LGTM.

Comment by Mike Fikes [ 20/Feb/16 10:23 PM ]

Since this is a low-level numerics update, also ran the unit tests through ChackraCore (successfully).





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

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

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

Attachments: Text File 0001-Better-error-message-if-def-is-mistakenly-substitute.patch     Text File CLJS-924.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!



 Comments   
Comment by Mike Fikes [ 05/Feb/16 8:19 PM ]

Patch no longer applies.

Comment by Mike Fikes [ 20/Feb/16 9:49 PM ]

Revised patch CLJS-924.patch applies to master.

(Based on Alex Dowad's original patch. Both Alex and I have signed the CA.)





[CLJS-1238] Setting *main-cli-fn* when using :target :nodejs shouldn't be manditory Created: 01/May/15  Updated: 20/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Jeremy Shoemaker Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File nodejs-main-cli-fn.patch    
Patch: Code

 Description   

Currently, when you use :target :nodejs in the build options for ClojureScript, the resulting code requires you to set *main-cli-fn* to a function.

This prevents someone from writing a library that can be used by JavaScript developers because it forces code execution on require. It also makes writing a CLI tool that can be distributed using NPM less straightforward. I ran into this issue trying to create a Leiningen template for writing CLI tools that could be installed using npm install or npm link. I had a wrapper script to take care of the CLI use-case, and intended to write the ClojureScript module in a more library oriented way, but ran into issues. I could work around this by not using the wrapper script, but it got me thinking about the more general library issue.

I don't see any reason why you should be forced to set *main-cli-fn* and so I'm suggesting making it optional.

Attached is a patch that makes it optional but retains the check for whether the value it is set to is a function in the case where it is set.

This is my first time submitting a change to a project using a git patch and not a pull request, so let me know if I've made the patch wrong.



 Comments   
Comment by Jeremy Shoemaker [ 01/May/15 7:27 PM ]

I just noticed the priority defaulted to "Major". I don't know if I'd say it's major, so feel free to bump it down if that doesn't seem appropriate.

Comment by Ning Sun [ 18/Feb/16 4:08 AM ]

+1.

I was working on a clojurescript library and going to build it as a node library. Currently blocked by this.

Comment by Mike Fikes [ 20/Feb/16 8:07 AM ]

Patch no longer applies.





[CLJS-1570] :parallel-build causes invalid truth check in cljs.reader/read-number Created: 14/Feb/16  Updated: 19/Feb/16  Resolved: 19/Feb/16

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

Type: Defect Priority: Major
Reporter: Andrew Childs Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None


 Description   

When compiling a large project with :parallel-build true I see intermitted failures in a test that parses "0" with cljs.reader/read-string.

Inspecting the difference in generated code between a successful build and failed build, I see the following difference in cljs.reader/read-number:

cljs.reader.unread(reader,ch);

 var s = buffer.toString();
 var or__6156__auto__ = cljs.reader.match_number(s);
-if(or__6156__auto__){
+if(cljs.core.truth_(or__6156__auto__)){
 return or__6156__auto__;
 } else {
 return cljs.reader.reader_error.cljs$core$IFn$_invoke$arity$variadic(reader,cljs.core.array_seq(["Invalid number format [",s,"]"], 0));
 }
 } else {


 Comments   
Comment by David Nolen [ 15/Feb/16 4:04 PM ]

This is a good catch, we should be binding cljs.analyzer/*unchecked-if* before analyzing or compiling a namespace to avoid other threads seeing a state change. It should also just get a simple false root binding and then we can avoid reader conditionals.

Comment by David Nolen [ 19/Feb/16 2:03 PM ]

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





[CLJS-1568] LazyTransformer doesn't implement IMeta Created: 14/Feb/16  Updated: 19/Feb/16  Resolved: 19/Feb/16

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

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bug

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

 Description   

While it implements `IWithMeta` and it's possible to add meta to a LazyTransformer, it's impossible to retrieve it because it doesn't implement `IMeta`

Example:

cljs.user=> (def x (with-meta (sequence (map inc) [1 2 3]) {:a 1}))
#'cljs.user/x
cljs.user=> x
(2 3 4)
cljs.user=> (meta x)
nil


 Comments   
Comment by António Nuno Monteiro [ 14/Feb/16 6:36 AM ]

submitted patch with `IMeta` implementation and a test that verifies the expected behavior

Comment by David Nolen [ 19/Feb/16 1:16 PM ]

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





[CLJS-1578] Corrupted Analysis Files Break Compilation Created: 18/Feb/16  Updated: 19/Feb/16  Resolved: 19/Feb/16

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

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


 Comments   
Comment by Mike Fikes [ 18/Feb/16 3:24 PM ]

Perhaps related to CLJS-1125

Comment by David Nolen [ 19/Feb/16 1:10 PM ]

fixed https://github.com/clojure/clojurescript/commit/68524a89eea8a8ae6cb69d1329acb3e3025c0fb6





[CLJS-1579] cljs.source-map/invert-reverse-map discards gcol Created: 18/Feb/16  Updated: 19/Feb/16  Resolved: 19/Feb/16

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

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

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

 Description   

If you try the commented example at the bottom of the file, you get:

cljs.source-map=> (invert-reverse-map
             #_=>     {1
             #_=>      {1 [{:gcol 0, :gline 0, :name "cljs.core/map"}],
             #_=>       5 [{:gcol 24, :gline 0, :name "cljs.core/inc"}]}})
{0 {1 [{:line 1, :col 1, :name "cljs.core/map"}], 5 [{:line 1, :col 5, :name "cljs.core/inc"}]}}

Instead you should get the gcol 24 value mapped:

cljs.source-map=> (invert-reverse-map
             #_=>     {1
             #_=>      {1 [{:gcol 0, :gline 0, :name "cljs.core/map"}],
             #_=>       5 [{:gcol 24, :gline 0, :name "cljs.core/inc"}]}})
{0 {0 [{:line 1, :col 1, :name "cljs.core/map"}], 24 [{:line 1, :col 5, :name "cljs.core/inc"}]}}


 Comments   
Comment by David Nolen [ 19/Feb/16 12:37 PM ]

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





[CLJS-1580] Self-host: goog.provide offsets source-maps Created: 18/Feb/16  Updated: 19/Feb/16  Resolved: 19/Feb/16

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

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

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

 Description   

If you use cljs.js to load code with source maps enabled, a goog.provide is emitted without accounting for its effects on source map lines.

To reproduce using source/noderepljs evaluate these forms:

(require 'cljs.js)

(def st (cljs.js/empty-state))

(cljs.js/eval-str st
  "(ns foo.core (:require bar.core))"
  nil
  {:eval       (fn [{:keys [source]}]
                 (println source))
   :context    :expr
   :load       (fn [_ cb] (cb {:lang   :clj
                               :source "(ns bar.core)\n(def a 3)"}))
   :source-map true}
  identity)

(get-in @st [:source-maps 'bar.core])

In addition to seeing that for bar.core code is generated starting with these lines:

goog.provide("bar.core");
bar.core.a = (3);

you will see that (via the get-in call) the source map info for bar.core/a is off by one (being listed as being on JavaScript source line 0 instead of line 1):

{0 {0 [{:line 1, :col 0, :name nil} {:line 1, :col 0, :name nil}], 5 [{:line 1, :col 5, :name "bar.core/a"}]}}

(Note the 0 at the beginning of this map; it should be 1.)



 Comments   
Comment by Mike Fikes [ 18/Feb/16 7:39 PM ]

With the attached patch, the correct source map info is generated:

{1 {0 [{:line 1, :col 0, :name nil} {:line 1, :col 0, :name nil}], 5 [{:line 1, :col 5, :name "bar.core/a"}]}}

(Note the 1 as the key for this map.)

Comment by David Nolen [ 19/Feb/16 12:34 PM ]

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





[CLJS-1492] Warn when using :optimisations instead of :optimizations Created: 24/Nov/15  Updated: 15/Feb/16  Resolved: 15/Feb/16

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

Type: Defect Priority: Major
Reporter: Daniel Compton Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: usability

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

 Description   

Coming from British English, it is easy to accidentally type the Anglicised :optimisations, instead of the American english :optimizations. Would it be ok to add a warning when users pass in :optimisations to use :optimizations instead?



 Comments   
Comment by Mike Fikes [ 29/Jan/16 8:26 PM ]

Perhaps if we maintained a collection of all known compiler and REPL option keys somewhere in the code, we could warn (and even suggest a correction) if an unknown key is passed with a small Levenshtein distance from a known key.

Comment by David Nolen [ 12/Feb/16 4:39 PM ]

Happy to take a patch for this one.

Comment by Mike Fikes [ 13/Feb/16 7:55 PM ]

CLJS-1492-1.patch attached for consideration.

It employs the Levenshtein distance technique to emit warnings like:

WARNING: Unknown compiler option ':optimisations'. Did you mean ':optimizations'?
WARNING: Unknown compiler option ':bogus'.

In this example, one suggestion is found, and once bogus option doesn't have a close-enough correction.

The approach is instrumented at the build API entry point (I checked that it works for down lein cljsbuild), and also at the point of setting up REPLs.

When setting up a REPL, an affordance is made for REPL options to be in the opts map as well, and warnings like this will be emitted:

$ script/noderepljs 
WARNING: Unknown option ':repo-verbose'. Did you mean ':repl-verbose'?
ClojureScript Node.js REPL server listening on 52123
To quit, type: :cljs/quit
cljs.user=>
Comment by David Nolen [ 15/Feb/16 4:18 PM ]

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





[CLJS-1566] Undeclared Var apply in make-array Created: 12/Feb/16  Updated: 15/Feb/16  Resolved: 15/Feb/16

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

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

Affects master 1.8.2


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

 Description   

After script/bootstrap,

$ script/noderepljs 
WARNING: Use of undeclared Var cljs.core/apply at line 369 /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/core.cljs
ClojureScript Node.js REPL server listening on 50938
To quit, type: :cljs/quit
cljs.user=>

(Note the new WARNING above.)

Regressed with https://github.com/clojure/clojurescript/commit/74164b47e0254fbae319596ab4b517e0ce44ea68



 Comments   
Comment by David Nolen [ 15/Feb/16 4:14 PM ]

fixed in master already





[CLJS-1567] make-array macro missing > 2 arg arity Created: 13/Feb/16  Updated: 15/Feb/16  Resolved: 15/Feb/16

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

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

Affects master


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

 Description   

You can do

(apply make-array [nil 3 2])

but not

(make-array nil 3 2)


 Comments   
Comment by Mike Fikes [ 13/Feb/16 12:50 AM ]

The attached patch addresses the issue and passes newly added tests.

Note: I couldn't find a clean way to eliminate the bulk of the existing function and have it delegate to the macro (which is largely a copy of the function). (The challenge being with respect to unrolling the rest arguments in the function when delegating to the macro.)

Comment by David Nolen [ 15/Feb/16 4:13 PM ]

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





[CLJS-1571] Make special-symbol? true for 'var Created: 14/Feb/16  Updated: 15/Feb/16  Resolved: 15/Feb/16

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

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

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

 Description   

Note: Analogous to CLJS-1557.

In Clojure,

(special-symbol? 'var)

evaluates to true.

Do this in ClojureScript

1. for consistency
2. because cljs.tools.reader (tools.reader-1.0.0-alpha3) makes use of special-symbol? and without this change syntax quoting doesn't work properly, making it difficult to write bootstrapped macros

With respect to the second item above

`(var x)

evaluates to

(cljs.user/var cljs.user/x)

if using cljs.tools.reader tools.reader-1.0.0-alpha3 with bootstrap ClojureScript.



 Comments   
Comment by David Nolen [ 15/Feb/16 3:56 PM ]

fixed https://github.com/clojure/clojurescript/commit/431b6e86d5d0515baed3b8dc4a82c8fecc41f529





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

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

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

Attachments: Text File CLJS-744-1.patch     Text File CLJS-744-2.patch     Text File CLJS-744-3.patch     Text File CLJS-744.patch     Text File CLJS-744.patch    
Patch: Code and Test

 Description   

added cljs-744-3.patch which cleans whitespace



 Comments   
Comment by António Nuno Monteiro [ 14/Feb/16 10:47 AM ]

I'm working on a patch for this issue. Assigned to myself.

Comment by António Nuno Monteiro [ 14/Feb/16 6:09 PM ]

submitted patch with code & tests; the most recent one should be considered, includes updated commit message

Comment by Andrea Richiardi [ 14/Feb/16 9:37 PM ]

Fix whitespace error so that the new patch applies cleanly.

Comment by António Nuno Monteiro [ 15/Feb/16 6:00 AM ]

Found an edge case, working on an update to the patch. Not ready to apply yet.

Comment by António Nuno Monteiro [ 15/Feb/16 6:39 AM ]

attached updated patch `CLJS-744-2.patch`

Comment by David Nolen [ 15/Feb/16 3:55 PM ]

fixed https://github.com/clojure/clojurescript/commit/70a43138844e19f771ef20bc6e847829ce0a5711





[CLJS-1515] Self-host: Allow :file key in cljs.js/*load-fn* callback Created: 17/Dec/15  Updated: 14/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1515-1.patch     Text File CLJS-1515-2.patch     Text File CLJS-1515-3.patch     Text File CLJS-1515-4.patch     Text File CLJS-1515-5.patch     Text File CLJS-1515-6.patch    
Patch: Code and Test

 Description   

Bootstrapped ClojureScript is abstracted away from direct I/O by use of a *load-fn* callback. A result is that when a namespace is loaded, the :file attribute associated with def s in [:cljs.analyzer/namespaces 'foo.ns :defs] in the AST is nil, because cljs.analyzer/*cljs-file* cannot be set to a meaningful value.

This ticket asks for an extension to *load-fn*, allowing a :file key to be optionally included by cljs.js clients, and for cljs.analyzer/*cljs-file* to be bound to that value in appropriate places in cljs.js so that the :file info appears in the AST.

One rationale for this :file attribute is that it makes it easier for clients of cljs.js to look up the file for a def, say, for use when implementing a source REPL special, for example.



 Comments   
Comment by Andrea Richiardi [ 17/Dec/15 4:31 PM ]

Initial patch, adding a :file key to load-fn and a :file-env key inside opts and then assigning it to cljs.analyzer/cljs-file in eval-str. This approach can be discussed and we can create an ad-hoc function for binding. It felt right there.
Moreover, cljs.analyzer/cljs-file gets overridden every time with the payload coming from load-fn.
All this was very quickly done in order to have a feedback from who's more expert than me about the consequences. This is also my very first ClojureScript patch

Comment by Mike Fikes [ 17/Dec/15 5:33 PM ]

I tried this patch. It is working fine for me when loading namespaces, but if I use cljs.js/analyze-str where the string is an ns form referring other namespaces loaded via *load-fn*, along with a def, things are off. (I have that ns referring macros from a clj file and a symbol from a cljs file, and the clj file gets associated with the top-level def and the macro, and the def in the referred file ends up with nil.

As a minor aside, the patch has a spurious whitespace change at the end.

Comment by Mike Fikes [ 17/Dec/15 5:56 PM ]

With respect to the last comment: The patch employs the pattern of conveying the :file passed in the cb via a :file-env opt to the consuming fn. It is consumed in eval-str* but not in analyze-str*. If the same logic is added to analyze-str* then the problem mentioned in the last comment goes away.

Comment by David Miller [ 17/Dec/15 6:48 PM ]

I'm hopeful someone will assign this to a responsible party. I am not that person.

Comment by Andrea Richiardi [ 17/Dec/15 7:21 PM ]

sorry David (Miller) and thanks Mike, I will rework it, adding some tests as well

Comment by Andrea Richiardi [ 17/Dec/15 7:23 PM ]

By the way this makes me think that maybe a better choice is to consider this a side effect and directly modify cljs.analyzer/*cljs-file* returning from *load-fn*, who knows how many other spots I am not covering...

Comment by Mike Fikes [ 18/Dec/15 5:36 AM ]

Two more comments:

1) Broadening the scope of the binding doesn't appear to work properly for me. But things do work if the bindings are done as in the patch now (next to where the other bindings are done).

2) Perhaps :file should be only set if the :lang being called back with is :clj. Maybe this could at least be documented. (It is not clear to me if it is useful for :js, as the patch is setting ana/*cljs-file*.)

Comment by Andrea Richiardi [ 18/Dec/15 10:27 AM ]

About 2), is any AST generated for .js files at all? If yes maybe then we should add it too...I need to explore that code path as well.

Comment by Andrea Richiardi [ 18/Dec/15 3:33 PM ]

So basically with ana/*cljs-file* binding the :file in :meta is not changed at all (I fixed following Mike's advice) but :file is, are we ok with this? In replumb (from planck) we check both so no problem, nonetheless it would be great to know why..

:defs {foo {:protocol-inline nil, :meta {:file bootstrap-test.core, :line 3, :column 7, :end-line 3, :end-column 10, :arglists (quote ([a b]))}, :name bootstrap-test.core/foo, :variadic false, :file /.../clojurescript/src/test/self/bootstrap_test/core.cljs, :end-column 10, :method-params ([a b]), :protocol-impl nil, :arglists-meta (nil nil), :column 1, :line 3, :end-line 3, :max-fixed-arity 2, :fn-var true, :arglists (quote ([a b]))}}, :require-macros nil, :doc nil

Comment by Andrea Richiardi [ 18/Dec/15 3:44 PM ]

It looks like the information in :meta comes directly from the multimethod parse which I dont' think we can change easily. So either we override :file in :meta or we leave as it is with a note in the documentation for :file in *load-fn*

Comment by Andrea Richiardi [ 18/Dec/15 4:10 PM ]

About :js files at least to me it looks like the only trace of importing, say, goog.sting in the AST is in the :imports of the parent namespace. No :file key anywhere, but please correct me if I am wrong as the AST is difficult to untangle

Comment by Andrea Richiardi [ 18/Dec/15 5:29 PM ]

Patch and test

Comment by Mike Fikes [ 18/Dec/15 7:43 PM ]

Comments on {{CLJS-1515-2.patch}} (mostly just opinion):

  1. (Opinion): Introduces new public API, especially with respect to AST exposure. Perhaps fn could instead be added to the test namespace.
  2. (Opinion): I wouldn't try anything complicated to try to patch up the :file that is in the :meta map. (Maybe we'll ultimately figure out why setting cljs.analyzer/*cljs-file* is insufficient for that bit.)
  3. (Opinion): For the :file docstring, I'd avoid mentioning AST. (Even though that was the true motivation for this ticket.) I'd only indicate that it represents the location where :source was obtained. (Which I guess would leave open it being perfectly fine for clients to provide it in the case that :lang is :js.)
  4. script/test-self-host passes for me.
  5. Inadvertent whitespace changes in append-source-map.
Comment by Andrea Richiardi [ 18/Dec/15 7:49 PM ]

1. Sorry Mike I don't understand when you say fn...what do you mean? Can you expand?
2. Yes and it would change a lot of code, that's why I didn't even try
3. Ok can change that, but where should be mentioned that we are modifying :file but not inside :meta?
4. Great!
5. You know I really tried hard not to have that, I will try again to disable all the auto indent my emacs has.

Comment by Mike Fikes [ 18/Dec/15 8:30 PM ]

1. The three new public functions in cljs.js: (var-ast, ns-ast, file->lang) could perhaps be moved to be utility functions in the self-host test namespace.
3. Dunno about the :meta question. But on the :lang :js question, perhaps the patch should only bind :cljs.analyzer/*cljs-file* if :lang :clj?

Comment by Andrea Richiardi [ 18/Dec/15 8:38 PM ]

1. I know it looks like they are used in test only, but I put them there as public because both replumb and planck use them and I was kind of "proposing" this kind of AST utils to be part of the official API (so that the poor dev does not have to go through cljs.analyzer in order to query the AST. I understand if no though.
3. This I don't really know, and seek guidance. I have not noticed any significant change in the AST for .js file, maybe *cljs-file* is never queried in that code path. I could not even find a way to test it. But I could, of course, be very wrong.

Comment by Andrea Richiardi [ 21/Dec/15 2:13 PM ]

This puts the utils functions in the test namespace for now, maybe thinking about exposing some API in the future.

Comment by Andrea Richiardi [ 21/Dec/15 8:19 PM ]

About :js:

  • it looks like the analyze-str code path simply recurs to fetch the next dep. So I guess that branch does not touch the AST.
  • for the require code path it looks like it -> is -> similar.

Therefore I don't see the point in adding :file for :js and I will not bind *cljs-file* if this is the case, as you suggested.

Comment by Andrea Richiardi [ 21/Dec/15 9:48 PM ]

Patch #4 changes the conveying key to :cljs-file, after Mike's good suggestion, and moves the assoc to the (condp ... :clj) branch only. I also added a test to check that *cljs-file* does not match the file path when in the :js branch.

Comment by Andrea Richiardi [ 21/Dec/15 11:56 PM ]

Another note, the *cljs-file* test works because the binding form does not actually restore the old value when it exits...In Clojure it would not probably work.

^ This is plain wrong, I was not considering the "when" my tests are executed, please disregard.

Comment by Mike Fikes [ 23/Dec/15 5:13 PM ]

CLJS-1515-4.patch LGTM.

Details: I tested against current ClojureScript master, using downstream Planck to load regular and macro namespaces and the :file portion of the AST gets properly updated. This also occurs if I instead use cljs.js/analyze-str passing in an ns form that causes code to be loaded. Additionally unit tests (regular and bootstrap) pass for me. I think this patch is functionally good to go.

Comment by David Nolen [ 26/Dec/15 6:54 AM ]

Copying goog.string into the source tree is not desirable. Please fix the tests to remove this. If you must, copy it to a temporary a location from the Google Closure Library JAR and remove it after the test has completed, thanks.

Comment by Andrea Richiardi [ 26/Dec/15 2:20 PM ]

Patch 5 avoid copying string.js and re-uses self_host/test.js.

Comment by Andrea Richiardi [ 26/Dec/15 2:22 PM ]

Done what you asked

Comment by Mike Fikes [ 05/Feb/16 8:05 PM ]

CLJS-1515-5.patch no longer applies

Comment by Andrea Richiardi [ 14/Feb/16 9:17 PM ]

Reapplied and re-tested. Works

Testing with Node

Testing self-host.test

Ran 8 tests containing 47 assertions.
0 failures, 0 errors.




[CLJS-1411] make-array signature differs from clojure Created: 10/Aug/15  Updated: 14/Feb/16  Resolved: 12/Feb/16

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

Type: Defect Priority: Minor
Reporter: Lars Andersen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: newbie

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

 Description   

When I was porting clj code using a two-dimensional array I noticed the cljs version only accepts size, instead of dim & more-dims.

Signature in cljs: ([size] [type size])
signature int clj: ([type len] [type dim & more-dims])



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

Unlike on the JVM there's no efficient way to allocate multi-dimensional arrays. An enhancement patch that includes a modification to the docstring that warns about the computational cost is welcome.

Comment by Lars Andersen [ 10/Aug/15 8:00 AM ]

What I want is for the signatures to be the same, so I don't have to use reader conditionals to get the same behavior on the two platforms.

When I'm asking for a two-dimensional array, I'm presumably going to do work that is O(m*n), so even though the pre-allocation is O(m*n) too it won't affect the runtime of the program significantly.

Comment by David Nolen [ 10/Aug/15 8:13 AM ]

Lars, yes I understood As I said patch welcome. This is a very ClojureScript newbie friendly ticket.

Comment by António Nuno Monteiro [ 19/Oct/15 12:30 PM ]

Submitted patch with suggested changes

Comment by Erik Assum [ 26/Nov/15 11:27 AM ]

@Antonio: Out of curiosity, isn't the "will run in polynomial time" a bit unprecise, since I guess all array-allocations run in polynomial time?

Comment by António Nuno Monteiro [ 03/Dec/15 5:46 AM ]

You're right. I misunderstood what David mentioned regarding the JVM performance. should it just say "Note that there is no efficient way to allocate multi-dimensional arrays in JavaScript" and stop there?

Comment by Mike Fikes [ 31/Jan/16 9:51 AM ]

This patch fails for me if I try using it with make-array in operator position. In fact, there's an underlying issue CLJS-1555 that probably needs to be fixed first, and then a patch could be made that takes care of the extra arity in the macro version of make-array as well.

Comment by David Nolen [ 12/Feb/16 3:33 PM ]

fixed https://github.com/clojure/clojurescript/commit/74164b47e0254fbae319596ab4b517e0ce44ea68

Comment by Andrea Richiardi [ 14/Feb/16 8:47 PM ]

I launched ./script/test today for another issue and got:

{{Analyzing file:/home/kapitan/git/clojurescript/src/main/cljs/cljs/core.cljs
WARNING: Use of undeclared Var cljs.core/apply at line 369 /home/kapitan/git/clojurescript/src/main/cljs/cljs/core.cljs}}





[CLJS-1364] cljs.js: Update default *load-fn* args to reflect docstring Created: 23/Jul/15  Updated: 14/Feb/16

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

Type: Enhancement Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1364-1.patch    

 Description   

The default *load-fn* :

(fn [name cb]
    (throw (js/Error. "No *load-fn* set")))

But the name arg reflects an older impl, with the new arg actually being a map.

To avoid confusion for anyone reading this code, perhaps

(fn [_ _]
    (throw (js/Error. "No *load-fn* set")))

or maybe name the first argument something meaningful?



 Comments   
Comment by Andrea Richiardi [ 18/Dec/15 6:52 PM ]

I decided to give it a try





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

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


 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-1186] add :postamble option to compiler Created: 02/Apr/15  Updated: 12/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Michael Bradley Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs

Attachments: Text File cljs_1186.patch    

 Description   

Similar to CLJS-723:

1) :postamble's value will be a vector of paths
2) the compiled output is appended with the contents of the files at those paths
3) the generated source map points to the correct/adjusted line numbers



 Comments   
Comment by David Nolen [ 12/Feb/16 4:42 PM ]

Would like to hear more use cases for this one.





[CLJS-1474] Warn if reserved symbol is defined Created: 21/Oct/15  Updated: 12/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie


 Description   

Currently a definition like

(defn set! [] ...)

will not cause any warning. Any usage of it (without :as namespace aliasing) however will not use the defined var but the set! special form.

A warning seems appropriate.






[CLJS-1448] lib-rel-path fails on Windows because of File/separator being \\ Created: 14/Sep/15  Updated: 12/Feb/16

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

Type: Defect Priority: Minor
Reporter: Orlando William Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, newbie
Environment:

Windows



 Description   

https://github.com/clojure/clojurescript/blob/cc953d4be7b4a256fd5eae783f9106a2929a4126/src/main/clojure/cljs/closure.clj#L1210

That code calls replace with \ on windows, who ends up calling (.replaceAll "foo.js" "." "\") and fails with
IllegalArgumentException character to be escaped is missing java.util.regex.Matcher.appendReplacement (:-1)



 Comments   
Comment by Orlando William [ 14/Sep/15 1:20 PM ]

I can't find a way to edit the description, I meant \ followed by \ but somehow it got a line break

Comment by David Nolen [ 12/Feb/16 4:12 PM ]

A patch for this is welcome.





[CLJS-1294] Macroexpand only accept quoted lists Created: 01/Jun/15  Updated: 12/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Julien Eluard Assignee: Julien Eluard
Resolution: Unresolved Votes: 0
Labels: newbie

Attachments: Text File CLJS-1294.patch    

 Description   

In Clojure macroexpand and macroexpand-1 accept any quoted argument while in ClojureScript anything but quoted seq will throw an exception.



 Comments   
Comment by Julien Eluard [ 01/Jun/15 2:16 PM ]

In Clojure some special forms are handled specifically i.e. (macroexpand '(Boolean true)) => (new Boolean true).

I am not sure if/how it applies to ClojureScript.

Comment by David Nolen [ 14/Jul/15 5:58 AM ]

This patch needs to be rebased to master. Thanks!





[CLJS-1406] macroexpand munging goog.string.StringBuffer Created: 09/Aug/15  Updated: 12/Feb/16  Resolved: 12/Feb/16

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

Type: Defect Priority: Minor
Reporter: Matthew Molloy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bootstrap
Environment:

Bootstrapped Planck REPL


Attachments: Text File cljs_1406.patch     Text File cljs_1406_test.patch    

 Description   

(macroexpand '(with-out-str)) on bootstrapped ClojureScript gives (let* [sb__13582__auto__ (string.StringBuffer.)]...
on JVM compiled ClojureScript it is (let* [sb__1155__auto__ (goog.string.StringBuffer.)]...

goog.string.StringBuffer has been incorrectly munged to string.StringBuffer when the cljs.js compiler is used.



 Comments   
Comment by Joel Martin [ 13/Aug/15 7:28 PM ]

I ran into this today. Here is a test case for the self host tests that reproduces the problem.

Comment by Joel Martin [ 13/Aug/15 7:50 PM ]

Fix the test. Propose a fix that just puts "js/" in front to prevent the broken name resolution. Probably not the correct solution, but it does work.

Comment by David Nolen [ 14/Aug/15 7:50 PM ]

Not going to take the current patches. The underlying resolution issue needs to be fixed.

Comment by David Nolen [ 12/Feb/16 3:27 PM ]

no longer an issue as far as I know.





[CLJS-1410] Support source maps in deps.cljs Created: 09/Aug/15  Updated: 12/Feb/16

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

Type: Enhancement Priority: Major
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

There should be support to package source maps with a foreign-lib using deps.cljs



 Comments   
Comment by David Nolen [ 12/Feb/16 3:00 PM ]

Patch welcome for this one!





[CLJS-1479] Race condition in browser REPL Created: 03/Nov/15  Updated: 12/Feb/16

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

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

Attachments: File heavy-load.sh     File race-condition.clj     File race-condition.jstack    

 Description   

Evaluation in browser REPL occasionally hangs. It seems that repl environment and browser sometimes miss each other and their "randezvous" fails. Browser is waiting for POST reply and repl is trying to send a command, but they do not meet each other.

I found the issue when we switched our tests from nodejs to browser environment. Luckily I was able to find very small example which hangs during execution. It seems that (simulated) heavy load increases the chance of "hanging".

Minimal setup:

(ns race.condition
  (:require [cljs.repl.browser :as browser]
            [cljs.repl :as repl]
            [cljs.env :as env]
            [cljs.build.api :as api]))


(api/build '[(ns race.repl
               (:require [clojure.browser.repl]))
             (clojure.browser.repl/connect "http://localhost:9000/repl")]
           {:output-to  "target/cljs-race/main.js"
            :output-dir "target/cljs-race"
            :main       'race.repl})

(spit "target/cljs-race/index.html"
      (str "<html>" "<body>"
           "<script type=\"text/javascript\" src=\"main.js\">"
           "</script>" "</body>" "</html>"))

Now start the environment:

(def env (browser/repl-env :static-dir ["target/cljs-race" "."] :port 9000 :src nil))

(env/with-compiler-env (env/default-compiler-env)
  (repl/-setup env {}))

cross your fingers and start this endless loop:

(loop [i 0]
  (println (java.util.Date.) i)
  (dotimes [j 100]
    (let [result (repl/-evaluate env "<exec>" "1"  "true")]
      (when-not (= :success (:status result))
        (println i j result))))
  (recur (inc i)))

To simulate heavy load run heavy-load.sh from attachment.

After some iterations (eg 55 big loop i) execution stops. If you investigate stacks (see race-condition.jstack), you can see in one thread:

at clojure.core$promise$reify__6779.deref(core.clj:6816)
	at clojure.core$deref.invoke(core.clj:2206)
	at cljs.repl.browser$send_for_eval.invoke(browser.clj:65)
	at cljs.repl.browser$browser_eval.invoke(browser.clj:193)
	at cljs.repl.browser.BrowserEnv._evaluate(browser.clj:262)

The code is waiting for a promise with a connection (which already did arive).

My guess is suspicious code in cljs.repl.server functions connection and set-connection. Both functions access an atom in non-standard way. They deref a valua and make a swap! in two steps.

Can somebody with better understanding of REPL internals investigate? Thank you.



 Comments   
Comment by David Nolen [ 12/Feb/16 2:57 PM ]

A patch is welcome for this one.





[CLJS-1555] make-array macro missing 2 arg arity Created: 31/Jan/16  Updated: 12/Feb/16  Resolved: 12/Feb/16

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

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

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

 Description   

You can do

(apply make-array [nil 2])
but not
(make-array nil 2)
.



 Comments   
Comment by David Nolen [ 12/Feb/16 2:56 PM ]

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





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

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: Text File cljs-970.patch    
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.

Comment by Mike Fikes [ 02/Feb/16 5:47 PM ]

Hey Thomas, the attached patch no longer applies.

I can confirm that if you make the same changes in master, they work there, and also in bootstrapped ClojureScript.

Comment by Thomas Heller [ 06/Feb/16 6:38 AM ]

Updated patch.

Comment by Mike Fikes [ 06/Feb/16 9:24 AM ]

I tested this patch, trying it with downstream `cljs.js` client, and it is working for me.

Comment by David Nolen [ 12/Feb/16 2:40 PM ]

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





[CLJS-1563] :source-map option to cljs.build.api/build should take nil Created: 07/Feb/16  Updated: 12/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Isaac Cambron Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It should be possible to specify nil or false when providing the :source-map option to cljs.build.api/build, for example, like this:

(build {...
        :optimizations :whitespace
        :source-map (when debug? "somepath.js.map")})

Currently that causes:

Exception in thread "main" java.lang.AssertionError: Assert failed: :source-map nil must specify a file in the same directory as :output-to "target/js/zs-background.js" if optimization setting applied
(or (nil? (:output-to opts)) (:modules opts) (string? source-map)), compiling:(/Users/isaac/code/zensight/client/cljs/build.clj:66:1)

Using false has the same behavior. The alternative of conditionally assoc ing the key in works just fine, but is a tad awkward. It seems reasonably straightforward to fix - need to change that assert to check the value in the map and double-check that it's checked properly downstream. Happy to submit a patch if you'll take it.



 Comments   
Comment by Isaac Cambron [ 07/Feb/16 10:18 AM ]

Apologies for the formatting; forgot that backtick stuff doesn't work in Jira.

Comment by Mike Fikes [ 08/Feb/16 5:05 PM ]

Reformatted description.

Comment by David Nolen [ 12/Feb/16 2:36 PM ]

Patch welcome.





[CLJS-1565] Self-host: whitespace optimization is broken Created: 08/Feb/16  Updated: 12/Feb/16  Resolved: 12/Feb/16

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

Type: Defect Priority: Minor
Reporter: Yehonathan Sharvit Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap, bug

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

 Description   

When requiring `cljs.js` and compiling in `:optimizations :whitespace`
I get the following error in the browser:
"goog.require could not find: cljs.core$macros"

And the page is broken.


Steps to reproduce:

1. Make a directory, say CLJS-1565
2. Copy shipping cljs.jar into the directory
3. Make an index.html, src/hello_world/core.cljs, and build.clj file with contents as below.
4. java -cp cljs.jar:src clojure.main build.clj
5. Open i