<< Back to previous view

[CLJS-1501] Add :parallel-build support to REPLs Created: 05/Dec/15  Updated: 27/Jun/17

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

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


 Description   

The :parallel-build option does not currently work in REPLs due to the implementation of cljs.repl/load-namespace






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

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

Type: Defect Priority: Major
Reporter: Bobby Eickhoff Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: bug
Environment:

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



 Description   

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

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

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

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



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

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

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

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

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

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

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

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

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

to be true and emit a "return".

Comment by David Nolen [ 06/May/15 7:15 PM ]

Hrm, it appears analyze-wrap-meta may need to defer to a helper to change the :context of the given AST node.

Comment by Herwig Hochleitner [ 11/Dec/15 10:52 AM ]

I just randomly ran into this, when upgrading an old project. There is also a duplicate already: http://dev.clojure.org/jira/browse/CLJS-1482

Comment by Jonathan Chu [ 28/Jan/16 6:19 PM ]

This issue occurs for me even without a let.

(fn []
  ^{"meta" "data"}
  (fn [] true))

gives me

#object[SyntaxError SyntaxError: Unexpected token return]




[CLJS-719] this-as behaves incorrectly in "scoping function" Created: 07/Dec/13  Updated: 27/Jun/17

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

Type: Defect Priority: Major
Reporter: Kevin Marolt Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None


 Description   

When a this-as expression gets put in a "scoping function", e.g. in a let-binding, the value bound via this-as refers to the scoping function, and not to the outer scope.

Example:

(def foo
  (js-obj
    "bar" "baz"
    "getBarRight" (fn [] (this-as self (.-bar self)))
    "getBarWrong" (fn []
                    (let [bar (this-as self (.-bar self))]
                      bar))))
     
(.log js/console (.getBarRight foo)) ;; => "baz"
(.log js/console (.getBarWrong foo)) ;; => undefined

Whereas foo.getBarRight expands to something like

function() {
  var self = this; // this refers to foo
  return self.bar; // returns "bar"
}

foo.getBarWrong on the other hand expands to

function() {
  var bar = function() {
    var self = this; // this refers to enclosing function
    return self.bar; // returns undefined
  }();
  return bar; // returns undefined
}





[CLJS-1300] REPLs do no write out updated deps.js when compiling files Created: 05/Jun/15  Updated: 27/Jun/17

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

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

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

 Description   

For example a user may edit a file including a new dependency. This will work at the REPL but if a browser refresh is made the emitted goog.require will fail due to the initial deps.js file being stale.



 Comments   
Comment by ewen grosjean [ 05/Dec/15 4:15 PM ]

load-file is broken into 4 sub-functions:
repl-compile-cljs: compile the cljs file beeing loaded
repl-cljs-on-disk: ensures all dependencies are on disk
refresh-cljs-deps: refreshes the cljs_deps.js file
repl-eval-compiled: eval the compiled file

Comment by David Nolen [ 05/Dec/15 9:02 PM ]

Thanks will review.

Comment by Mike Fikes [ 31/Jan/16 3:25 PM ]

cljs-1300.patch no longer applies on master





[CLJS-2541] binding not made in parallel Created: 21/Feb/18  Updated: 22/Feb/18

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

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

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

 Description   

The docstring for binding indicates "The new bindings are made in parallel (unlike let); all init-exprs are evaluated before the vars are bound to their new values."

You can observe this in Clojure:

user=> (def ^:dynamic *a* 1)
#'user/*a*
user=> (def ^:dynamic *b* nil)
#'user/*b*
user=> (binding [*a* 10 *b* (inc *a*)] *b*)
2

but in ClojureScript bindings are made in serial as if with let:

cljs.user=> (def ^:dynamic *a* 1)
#'cljs.user/*a*
cljs.user=> (def ^:dynamic *b* nil)
#'cljs.user/*b*
cljs.user=> (binding [*a* 10 *b* (inc *a*)] *b*)
11


 Comments   
Comment by Mike Fikes [ 22/Feb/18 9:15 AM ]

The defect also extends to with-redefs.

The attached patch ensures the correct evaluation order by placing values into temporary vectors.

Currently, if you macroexpand the example given in the ticket description, you get:

(let* [*a*28 *a*
       *b*29 *b*]
  (set! *a* 10)
  (set! *b* (inc *a*))
  (try
   *b*
   (finally
    (set! *b* *b*29)
    (set! *a* *a*28))))

The patch revises the expansion so that you instead get:

(let* [orig-vals__30 [*a* *b*]
       temp-vals__31 [10 (inc *a*)]]
  (set! *a* (temp-vals__31 0))
  (set! *b* (temp-vals__31 1))
  (try
   *b*
   (finally
    (set! *b* (orig-vals__30 1))
    (set! *a* (orig-vals__30 0)))))

The patch also fixes a place in cljs.pprint which has an invalid binding form. (It attempts to bind a var but provides no value.)

Additionally, the patch fixes places in cljs.js where the binding of r/*alias-map* was established using a function that itself relies on bindings that are established earlier in the binding form list.





[CLJS-2615] Shared AOT cache: Avoid referring to cache directory in analyzer warnings Created: 04/Mar/18  Updated: 22/Mar/18

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

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

Java 1.8 (for ClojureScript 1.9.946)


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

 Description   

If an analysis warning is emitted for code in a JAR, the warning will include the AOT cache path.

For example, with

foo/core.cljs
(ns foo.core)

(inc "a")

in foo.jar, loading this code will cause a diagnostic that includes the AOT cache path in the message:

WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 3 /Users/mfikes/.cljs/.aot_cache/1.10.126/47C5358/foo/core.cljs

Perhaps in this case it would be better to indicate just a classpath-relative filename

WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 3 @/foo/core.cljs

or if possible, even using a JAR file url like this

WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 3 jar:file:///foo.jar!/foo/core.cljs

To repro, first create foo.jar using the following command, with the foo.core namespace in foo/core.cljs:

jar cvf foo.jar foo

Then use the 1.10.126 uberjar (renamed to cljs-1.10.126.jar here) to launch a Node REPL:

java -cp cljs-1.10.126.jar:foo.jar clojure.main -m cljs.repl.node

and evaluate

(require 'foo.core)

To see what you get with 1.9.946, remove the .cljs_node_repl directory and using the 1.9.946 uberjar (renamed to cljs-1.9.946.jar here):

java -cp cljs-1.9.946.jar:foo.jar clojure.main -m cljs.repl.node

In this case, (require 'foo.core) still refers to the local cache directory

WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 3 .cljs_node_repl/foo/core.cljs

but this somehow seems less problematic when compared to exposing the user to the shared AOT cache path.



 Comments   
Comment by Mike Fikes [ 05/Mar/18 1:56 PM ]

The attached patch

  1. Revises a few of the Compilable extensions (to File, URL, and String) to add :origin-file to the opts (merging opts last so that any :origin-file already in opts is maintained)
  2. Adds a new ana/*origin-file* dyn var to go along with ana/*cljs-file*, binding it to the value of (:origin-file opts)
  3. Updates the code that renders error messages to use ana/*origin-file* in preference to ana/*cljs-file*

This has the effect of pointing to the original source of the error rather than to the on-disk copy in a cache (local or shared).

For, example, with the analysis error originating from a file in a JAR, you see:

cljs.user=> (require 'foo.core)
WARNING: cljs.core/+, all arguments must be numbers, got [string number] instead. at line 3 jar:file:/private/tmp/test-origin-file/foo.jar!/foo/core.cljs

Another illustrative example: Using

deps.edn
{:deps {org.clojure/clojurescript {:mvn/version "1.10.132"}
        com.cerner/clara-rules {:mvn/version "0.17.0"}}}

master produces this (elided for brevity):

clj -Srepro -m cljs.main -re node -e "(require 'clara.rules)"

WARNING: macroexpand already refers to: cljs.core/macroexpand being replaced by: clojure.reflect/macroexpand at line 33 /Users/mfikes/.cljs/.aot_cache/1.10.132/3837055/clojure/reflect.cljs
WARNING: Use of undeclared Var schema.core/MapEntry at line 239 /Users/mfikes/.cljs/.aot_cache/1.10.132/2E9D363/schema/core.cljs
WARNING: Wrong number of args (3) passed to MapEntry at line 759 /Users/mfikes/.cljs/.aot_cache/1.10.132/2E9D363/schema/core.cljs

while the patch, built as 1.10.133 produces more informative error pointers into the actual source JARs instead of the AOT cache directory

WARNING: macroexpand already refers to: cljs.core/macroexpand being replaced by: clojure.reflect/macroexpand at line 33 jar:file:/Users/mfikes/.m2/repository/org/clojure/clojurescript/1.10.133/clojurescript-1.10.133.jar!/clojure/reflect.cljs
WARNING: Use of undeclared Var schema.core/MapEntry at line 239 jar:file:/Users/mfikes/.m2/repository/prismatic/schema/1.1.6/schema-1.1.6.jar!/schema/core.clj
WARNING: Wrong number of args (3) passed to MapEntry at line 759 jar:file:/Users/mfikes/.m2/repository/prismatic/schema/1.1.6/schema-1.1.6.jar!/schema/core.cljs
Comment by David Nolen [ 07/Mar/18 10:04 PM ]

Yeah this is a REPL specific problem. I think the right way to handle this is to fix how REPLs compile code. Currently they do an analysis pass in reverse fashion instead of just putting everything in dependency order and compiling in that order same as build. This would be a good opportunity to clean up some old code paths that only the REPLs use.





[CLJS-1896] Externs file validation Created: 23/Jan/17  Updated: 27/Jun/17

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

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


 Description   

Invalid externs file will fail silently and not provide the expected externs inference warnings. We should try to catch such issues when we parse the file and throw an exception.






[CLJS-2002] Don't throw when no *print-fn* is set Created: 07/Apr/17  Updated: 29/Mar/18

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: errors, warnings

Attachments: Text File CLJS-2002.patch    
Approval: Vetted

 Description   

Currently calling (enable-console-print!) causes a bunch of code to be retained in :advanced mode even if you never print.

While that is not ideal it doesn't cause runtime errors. Not calling it and trying to print however will throw an exception which will potentially break your app.

No *print-fn* fn set for evaluation environment

So we end up in a no-win situation for :advanced builds where a "forgotten" prn may break your app in production or "maybe" bloating your file size by retaining all the print-related things.

I think the no-print-fn condition should never throw, maybe just try to write a warning using console.log. Or just dropping the prn altogether.



 Comments   
Comment by David Nolen [ 13/Jul/17 8:29 PM ]

Let's move the old behavior to `string-print` only.

Comment by Mike Fikes [ 13/Jul/17 9:01 PM ]

The attached CLJS-2002.patch moves the throw to string-print and using nil as the init for the two Vars solves CLJS-2231. But it doesn't address the ticket as written: Leaving an inadvertent prn in your code leads to a call to string-print which throws.

Comment by David Nolen [ 13/Jul/17 10:06 PM ]

https://github.com/clojure/clojurescript/commit/797e247fbef676544060a57da995f058db061f37 partially addresses this issue. Keeping this open and moving to lower priority, we should revisit.





[CLJS-1701] cljs.spec impact on :advanced builds Created: 07/Jul/16  Updated: 07/Jul/16

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 11
Labels: None


 Description   

Investigate the impact of cljs.spec on :advanced builds.

Currently all specs are kept in the (private) cljs.spec/registry-ref atom. This atom is not understood by the Closure Compiler and cannot be eliminated as dead code. So even if specs are not used in "production" they still bloat the generated JS size. Some specs may be used at runtime and cannot not be removed, the gen parts however are probably never required in :advanced builds and should be omitted somehow.

In a test build (with 1.9.93) this adds 11kb (102kb vs 91kb) as soon as cljs.spec is :require'd somewhere and goes up with each defined spec.






[CLJS-868] no arity warnings on recursive calls Created: 03/Oct/14  Updated: 19/Nov/17

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

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

Attachments: File cljs_868_14_Nov_2015.md     Text File cljs_868_14_Nov_2015.patch    
Patch: Code

 Description   

If a function recursively invokes itself within its own body the invoke will not be checked for arity mismatch.



 Comments   
Comment by Samuel Miller [ 10/Aug/15 10:06 PM ]

Took some time to look at this issue. Originally thought "Do what loop/recur does" but that does not take into account multi-arity. It seems like maybe the best option is to somehow use the second pass of the analyze(analyze-fn-methods-pass2). The entire information about the function is present and the warning section of the code gets triggered but because of no-warn is ignored. Any other ideas for a solution to this?

Comment by Samuel Miller [ 14/Nov/15 7:47 PM ]

So I am looking for feed back on this patch and I will try to explain the reasoning for each section.

The issue is that a function only knows about it's arity after it has been parsed once.
So we need to check arity issues on the second pass

First off, added two new variables.
-activate-second-pass-warnings:Boolean Basically if you want to have second-pass warnings turned on
-second-pass-cljs-warnings:Set Right now we only have :fn-arity but I figure might as well make it generic.

So first up if the modifications to the analyze-fn-methods-pass2 function.
Instead of using no-warn marco here we have some new functionality.
The goal is to turn everything off except the second-pass warnings

So if activate-second-pass-warnings is false just use no-warn else it will use the new section of code.

The default-warning-handler was also modified. After checking if a warning is on, it checks if the warning is a second-pass warning and
if that warning can now be activated. If activate-second-pass-warnings is false AND a warning is still on that implies it is a second pass warning
in the second pass so we activate it.

Also I tried to keep all modifications in cljs.analyzer.

Originally I had the cljs-warnings :fn-arity to false and it would only be turned on in the second pass.
However the repl section just sets everything to true (and turns off select parts like ns errors).
So I decided to not touch those sections and instead keep how other files interface with the analyzer the same.

Comment by Samuel Miller [ 16/Nov/15 10:58 PM ]

Just realized that I have the patch marked as .md instead of .patch

Comment by Mike Fikes [ 19/Nov/17 8:09 PM ]

Patch no longer applies.





[CLJS-2761] Warn if fn has more than 20 fixed arguments Created: 25/May/18  Updated: 25/May/18

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

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

Approval: Accepted

 Description   

Currently we do not warn if a function declares more than 20 fixed arguments.






[CLJS-2693] Have Range implement IChunkedSeq Created: 22/Mar/18  Updated: 01/Jul/18

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 3
Labels: None

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

 Description   

When Clojure's range was converted from a lazy seq to a LongRange in CLJ-1515, LongRange was made to implement IChunkedSeq. It seems that this could also be applicable in ClojureScript, with superficial evidence being that by taking this form

(apply + (filter even? (map inc (range 1e6))))

and simply adding a call to vec

(apply + (filter even? (map inc (vec (range 1e6)))))

dramatically speeds it up. Benchmarks are included below.

Note that Alex Miller recalls that the change in Clojure was rather challenging, with difficulty:
"balancing the perf concerns of reducible, sequence, and chunked sequence traversal (all of which are possible and may even interleave) while also avoiding potential overflow/underflow cases was a surprisingly narrow path"

Benchmarking with V8
[x (range 100000)], (apply + (filter even? (map inc x))), 10 runs, 1014 msecs
Benchmarking with SpiderMonkey
[x (range 100000)], (apply + (filter even? (map inc x))), 10 runs, 849 msecs
Benchmarking with JavaScriptCore
[x (range 100000)], (apply + (filter even? (map inc x))), 10 runs, 423 msecs
Benchmarking with Nashorn
[x (range 100000)], (apply + (filter even? (map inc x))), 10 runs, 2386 msecs
Benchmarking with ChakraCore
[x (range 100000)], (apply + (filter even? (map inc x))), 10 runs, 1798 msecs
Benchmarking with V8
[x (vec (range 100000))], (apply + (filter even? (map inc x))), 10 runs, 280 msecs
Benchmarking with SpiderMonkey
[x (vec (range 100000))], (apply + (filter even? (map inc x))), 10 runs, 201 msecs
Benchmarking with JavaScriptCore
[x (vec (range 100000))], (apply + (filter even? (map inc x))), 10 runs, 230 msecs
Benchmarking with Nashorn
[x (vec (range 100000))], (apply + (filter even? (map inc x))), 10 runs, 2954 msecs
Benchmarking with ChakraCore
[x (vec (range 100000))], (apply + (filter even? (map inc x))), 10 runs, 427 msecs


 Comments   
Comment by Mike Fikes [ 22/Mar/18 7:19 PM ]

An initial spike into feasibility shows promise:

With the current build of master (1.10.217) as the baseline for the form in the ticket description, in the Node REPL:

cljs.user=> (time (apply + (filter even? (map inc (range 1e6)))))
"Elapsed time: 1247.869027 msecs"
250000500000

and with the experimental exploratory change in CLJS-2693-0.patch

cljs.user=> (time (apply + (filter even? (map inc (range 1e6)))))
"Elapsed time: 251.344390 msecs"
250000500000
Comment by David Nolen [ 15/Jun/18 4:32 PM ]

Nice! Seems like it just needs a bit more work?

Comment by Mike Fikes [ 15/Jun/18 4:42 PM ]

Yes, work towards taking the spike idea and creating a production-quality change has been in progress over here https://github.com/mfikes/clojurescript/commits/CLJS-2693

Comment by Mike Fikes [ 01/Jul/18 1:51 PM ]

The attached
CLJS-2693.patch patch depends on the patches in CLJS-2798 and CLJS-2799 having been applied.

Here are the performance speedups across the 3 major engines, covering the same metrics given in CLJ-1515 (minus the ones depending on ratio support):

Code V8 Speedup SpiderMonkey JavaScriptCore
(count (range (* 1024 1024))) 5.28 8.58 7.43
(reduce + (map inc (range (* 1024 1024)))) 4.04 6.41 2.02
(reduce + (map inc (map inc (range (* 1024 1024))))) 5.80 7.53 2.57
(count (keep odd? (range (* 1024 1024)))) 4.77 4.39 1.43
(transduce (comp (map inc) (map inc)) + (range (* 1024 1024))) 1.01 1.00 1.00
(reduce + 0 (range (* 2048 1024))) 1.01 1.00 1.00
(reduce + 0 (rest (range (* 2048 1024)))) 1.01 1.00 0.99
(doall (range 0 31)) 0.60 0.98 0.81
(doall (range 0 32)) 0.60 0.94 0.87
(doall (range 0 4096)) 0.59 0.88 0.82
(into [] (map inc (range 31))) 1.91 1.58 1.30
(into [] (map inc) (range 31)) 0.86 1.00 1.12
(into [] (range 128)) 0.93 1.04 1.02
(doall (range 0.5 1000 0.33)) 1.27 0.97 0.80
(into [] (range 0.5 1000 0.33)) 1.13 1.02 0.98
(count (filter odd? (take (* 1024 1024) (range)))) 1.00 1.15 0.52
(transduce (take (* 1024 1024)) + (range)) 1.05 1.06 0.95




[CLJS-2247] Warn when overwriting protocol method Created: 15/Jul/17  Updated: 02/Jul/18

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: errormsgs

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

 Description   

Warn when a protocol overwrites a method in another protocol.

Observe the warning in this Clojure REPL session:

user=> (defprotocol IAlpha
  #_=>   (-foo [this]))
IAlpha
user=> (defrecord Alpha []
  #_=>   IAlpha
  #_=>  (-foo [this] :alpha))
user.Alpha
user=> (defprotocol IBeta
  #_=>   (-foo [this]))
Warning: protocol #'user/IBeta is overwriting method -foo of protocol IAlpha
IBeta
user=> (defrecord Beta []
  #_=>   IBeta
  #_=>   (-foo [this] :beta))
user.Beta
user=> (-foo (->Alpha))

IllegalArgumentException No implementation of method: :-foo of protocol: #'user/IBeta found for class: user.Alpha  clojure.core/-cache-protocol-fn (core_deftype.clj:568)

Here is the same in ClojureScript:

To quit, type: :cljs/quit
cljs.user=> (defprotocol IAlpha
(-foo [this]))
nil
cljs.user=> (defrecord Alpha []
IAlpha
(-foo [this] :alpha))
cljs.user/Alpha
cljs.user=> (defprotocol IBeta
(-foo [this]))
nil
cljs.user=> (defrecord Beta []
IBeta
(-foo [this] :beta))
cljs.user/Beta
cljs.user=> (-foo (->Alpha))
repl:13
throw e__5612__auto__;
^

Error: No protocol method IBeta.-foo defined for type : [object Object]
    at cljs$core$missing_protocol (/Users/mfikes/.cljs_node_repl/.cljs_node_repl/cljs/core.js:317:9)
    at cljs$user$_foo (repl:25:34)
    at repl:1:94
    at repl:9:3
    at repl:14:4
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
    at Domain.<anonymous> ([stdin]:50:34)
    at Domain.run (domain.js:242:14)
    at Socket.<anonymous> ([stdin]:49:25)

Note that they both result in the same runtime behavior, but Clojure emits a nice diagnostic when IBeta is defined.



 Comments   
Comment by Mike Fikes [ 16/Jul/17 9:28 AM ]

This is an interesting one: While the attached patch produces nice warnings, even without it the :redef-in-file warning triggers if the forms involved happen to reside in files, and in that case you'd get two slightly different warnings being emitted:

WARNING: Protocol IDelta is overwriting method -bar of protocol IGamma in file /Users/mfikes/Desktop/src/foo/core.cljc
WARNING: -bar at line 6 is being replaced at line 7 /Users/mfikes/Desktop/src/foo/core.cljc

Each of the warnings above has their own strengths; but having both seems to be a negative. Here are my initial thoughts on variants:

  1. Leave both: Could be perceived to be noisy, albeit useful if it occurs.
  2. Have the warning disabled, but enabled for the REPL: Odd to have a warning that only happens while in the REPL (and you can still get two when loading files)
  3. Have the warning only be emitted for REPL-entered forms: This perhaps directly addresses the issue: (No double warnings, useful for people learning the language). But this seems even odder to have a warning limited to REPL-entered forms.
  4. Figure out a way to suppress the :redef-in-file warning in this case: Maybe cleaner.
  5. Same as above, but find a way to get the line numbers into the "overwrite" warning: Perhaps cleanest, but difficult to do?
  6. Do nothing, decline the ticket: Perhaps this leaves users learning the language at a loss if they encounter this while in the REPL where redefining forms is "normal", but not overwriting via protocol definitions.
Comment by Mike Fikes [ 29/Dec/17 10:19 AM ]

The attached CLJS-2247-2.patch rebaselines against master.

Comment by Mike Fikes [ 02/Jul/18 12:10 PM ]

CLJS-2247-3.patch rebaselines





[CLJS-1558] Code allowed to re-define referred var Created: 31/Jan/16  Updated: 02/Jul/18

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: David Nolen
Resolution: Unresolved Votes: 2
Labels: None

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

 Description   

If you refer a var from another namespace, then you can def a new value for that var, and the def will mutate the other namespace, and other things will go wrong as illustrated in the example below.

FWIW, Clojure disallows this, and refuses to allow you to evaluate a def involving a referred var, and emits an error diagnostic like:

CompilerException java.lang.IllegalStateException: foo already refers to: #'some.name.space/foo in namespace: user, compiling:(NO_SOURCE_PATH:2:1)

Here is a complete example illustrating the issues:

Given:

(ns foo.core)

(defn square [x]
  (* x x))

then do this in a REPL:

cljs.user=> (require '[foo.core :refer [square]])
nil
cljs.user=> (var square)
#'foo.core/square
cljs.user=> (square 3)
9
cljs.user=> (ns-interns 'cljs.user)
{}
cljs.user=> (defn square [x] (+ x x))
WARNING: square already refers to: foo.core/square being replaced by: cljs.user/square at line 1 <cljs repl>
#'foo.core/square
cljs.user=> (square 3)
6
cljs.user=> (var square)
#'foo.core/square
cljs.user=> (in-ns 'foo.core)
nil
foo.core=> (square 3)
6
foo.core=> (in-ns 'cljs.user)
nil
cljs.user=> (ns-interns 'cljs.user)
{square #'cljs.user/square}
cljs.user=> (cljs.user/square 3)
TypeError: Cannot read property 'call' of undefined
    at repl:1:105
    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:221:14)
    at Socket.<anonymous> ([stdin]:40:25)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)
cljs.user=> #'cljs.user/square
#'cljs.user/square
cljs.user=> @#'cljs.user/square
nil


 Comments   
Comment by Mike Fikes [ 27/Nov/17 2:00 PM ]

The attached patch essentially turns the warning into an error for non-core names, throwing an exception that matches Clojure's.

Example:

$ script/noderepljs
ClojureScript Node.js REPL server listening on 49203
To quit, type: :cljs/quit
cljs.user=> (def map 3)
WARNING: map already refers to: cljs.core/map being replaced by: cljs.user/map at line 1 <cljs repl>
#'cljs.user/map
cljs.user=> (require '[clojure.set :refer [intersection]])
nil
cljs.user=> (def intersection 3)
clojure.lang.ExceptionInfo: intersection already refers to: #'clojure.set/intersection in namespace cljs.user at line 1 <cljs repl> {:file "<cljs repl>", :line 1, :column 1, :root-source-info {:source-type :fragment, :source-form (def intersection 3)}, :tag :cljs/analysis-error}
	at clojure.core$ex_info.invokeStatic(core.clj:4725)
	at clojure.core$ex_info.invoke(core.clj:4725)
	at cljs.analyzer$error.invokeStatic(analyzer.cljc:697)
...
Comment by Mike Fikes [ 27/Nov/17 2:05 PM ]

Here is an example showing that it is (as was before), OK to define a Var that shadows a core Var:

$ script/noderepljs
ClojureScript Node.js REPL server listening on 57077
To quit, type: :cljs/quit
cljs.user=> (defn quot [s] (str "'" s "'"))
WARNING: quot already refers to: cljs.core/quot being replaced by: cljs.user/quot at line 1 <cljs repl>
#'cljs.user/quot
cljs.user=> (quot "hello")
"'hello'"
cljs.user=> (cljs.core/quot 6 2)
3
Comment by Mike Fikes [ 02/Jul/18 12:14 PM ]

CLJS-1558-2.patch rebaselines





[CLJS-2802] empty? is broken for transient collections Created: 03/Jul/18  Updated: 03/Jul/18

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

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

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

 Description   

Like CLJ-1872 but for ClojureScript. (Note that CLJ-1872 is vetted, targeting Clojure 1.10.)

ClojureScript 1.10.339
cljs.user=> (empty? (transient []))
...
Error: [object Object] is not ISeqable
...
cljs.user=> (empty? (transient {}))
...
Error: [object Object] is not ISeqable
...
cljs.user=> (empty? (transient #{}))
...
Error: [object Object] is not ISeqable
...
cljs.user=>


 Comments   
Comment by Mike Fikes [ 03/Jul/18 8:24 PM ]

Adds an additional check for ITransientCollection in empty? implementation and delegates to count, which should be O(1).

It may be worth waiting to see if CLJ-1872 actually ships in Clojure before applying. (This would also afford an opportunity to make the docstring match Clojure's.)





[CLJS-2806] Bump test.check to 0.10.0-alpha3 Created: 05/Jul/18  Updated: 14/Jul/18

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

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

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

 Description   

https://github.com/clojure/test.check/blob/master/CHANGELOG.markdown#0100-alpha3-2018-05-27



 Comments   
Comment by Mike Fikes [ 14/Jul/18 10:37 AM ]

The attached patch is straightforward as expected. The only twist is that, since the set of sources inside the test.check JAR have changed, we need to update the self-host testing infrastructure to reflect these new source names. Instead of updating the explicit list, this patch just does it implicitly so we don't have to deal with this for future updates.





[CLJS-2822] cljs.core.specs.alpha: Map bindings should be `:kind map?` Created: 14/Jul/18  Updated: 14/Jul/18

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

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

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

 Description   

Since the initial port of clojure.core.specs.alpha a defect has been fixed that should be ported so that map bindings are required to satisfy map?:

(s/def ::map-bindings
  (s/every (s/or :mb ::map-binding
                 :nsk ::ns-keys
                 :msb (s/tuple #{:as :or :keys :syms :strs} any?)) :kind map?))

See https://github.com/clojure/core.specs.alpha/commit/10439e14795e4b1d365b4076460adec441f9c687






[CLJS-1278] Asserts still fail while :require-ing .js file (either in :libs or in :source-paths) (same as CLJS-1196) Created: 20/May/15  Updated: 14/Jul/15

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

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

Attachments: Text File cljs_1278.patch    

 Description   

Following on CLJS-1196, I can't get it to work.

In version 0.0-3264 lein-cljsbuild crashed on weird eception `Caused by: java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil"` but the current version 0.0-3269 gives the same failed assertion as previously.

I've put up a sample project to illustrate the issue.

Steps to reproduce:

`git clone https://github.com/tillda/stackone`
`cd stackone`
`git checkout 537e5c69b844bc53c159e85cafc24310543cc918`
`lein clean && lein cljsbuild once temp`

Expected behaviour: cljs compiled successfully with src/vendor/client/closure.js and env/stackone/helpersjs.js being included.

Actual behaviour:

```
Compiling "resources/public/lein-cljsbuild-temp/dev-mode-deps.js" failed.
Exception in thread "main" java.lang.AssertionError: Assert failed: (or (file? x) (url? x) (string? x)), compiling/private/var/folders/ym/l2qxd7l97kzfzftrdpqsclm40000gn/T/form-init3642888309490821030.clj:1:125)
at clojure.lang.Compiler.load(Compiler.java:7249)
at clojure.lang.Compiler.loadFile(Compiler.java:7175)
at clojure.main$load_script.invoke(main.clj:275)
at clojure.main$init_opt.invoke(main.clj:280)
at clojure.main$initialize.invoke(main.clj:308)
at clojure.main$null_opt.invoke(main.clj:343)
at clojure.main$main.doInvoke(main.clj:421)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)
Caused by: java.lang.AssertionError: Assert failed: (or (file? x) (url? x) (string? x))
at cljs.util$ext.invoke(util.cljc:115)
at cljs.closure$source_on_disk.invoke(closure.clj:1206)
at cljs.closure$output_unoptimized$fn__3708.invoke(closure.clj:1235)
at clojure.core$map$fn__4551.invoke(core.clj:2622)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:507)
at clojure.core$seq__4126.invoke(core.clj:135)
at clojure.core$filter$fn__4578.invoke(core.clj:2677)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:507)
at clojure.core$seq__4126.invoke(core.clj:135)
at clojure.core$map$fn__4551.invoke(core.clj:2614)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.Cons.next(Cons.java:39)
at clojure.lang.RT.next(RT.java:674)
at clojure.core$next__4110.invoke(core.clj:64)
at clojure.core$str$fn__4186.invoke(core.clj:528)
at clojure.core$str.doInvoke(core.clj:526)
at clojure.lang.RestFn.applyTo(RestFn.java:139)
at clojure.core$apply.invoke(core.clj:628)
at cljs.closure$deps_file.invoke(closure.clj:1040)
at cljs.closure$output_deps_file.invoke(closure.clj:1060)
at cljs.closure$output_unoptimized.doInvoke(closure.clj:1243)
at clojure.lang.RestFn.applyTo(RestFn.java:139)
at clojure.core$apply.invoke(core.clj:630)
at cljs.closure$build.invoke(closure.clj:1514)
at cljs.closure$build.invoke(closure.clj:1426)
at cljsbuild.compiler$compile_cljs$fn__3884.invoke(compiler.clj:81)
at cljsbuild.compiler$compile_cljs.invoke(compiler.clj:80)
at cljsbuild.compiler$run_compiler.invoke(compiler.clj:187)
at user$eval4018$iter_40544058$fn4059$fn_4077.invoke(form-init3642888309490821030.clj:1)
at user$eval4018$iter_40544058$fn_4059.invoke(form-init3642888309490821030.clj:1)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:507)
at clojure.core$seq__4126.invoke(core.clj:135)
at clojure.core$dorun.invoke(core.clj:3007)
at clojure.core$doall.invoke(core.clj:3023)
at user$eval4018.invoke(form-init3642888309490821030.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6792)
at clojure.lang.Compiler.eval(Compiler.java:6782)
at clojure.lang.Compiler.load(Compiler.java:7237)
... 11 more
Subprocess failed
```



 Comments   
Comment by David Nolen [ 20/May/15 10:21 AM ]

This issue is in danger of being closed. Please supply minimal steps to reproduce that do not involve anything other than the ClojureScript compiler. We no longer have time to wade through the indirection introduced by cljsbuild or any other downstream tooling. Thanks.

Comment by Michal Till [ 20/May/15 11:14 AM ]

@David Nolen: I have created a failing minimal testcase based on the Quick Start document. Here it is: https://github.com/tillda/cljs-testcase/

Comment by David Nolen [ 20/May/15 11:27 AM ]

Michal the failing example is not correct. You are not supplying any :libs option.

Comment by Michal Till [ 20/May/15 11:45 AM ]

Ah! Thank you very much! This additional issue was therefore my error. Now it seems to work even in my "big" example.

However it would be cool if there was a meaningful error message stating that a file path can't be resolved. If one is not an expert in the cljs compiler this is almost impossible to figure out. After all the error message in the CLJS-1196 issue and in this wrongfully reported one are exactly the same.

You may close this issue.

Comment by David Nolen [ 20/May/15 11:55 AM ]

We'll leave it open for the improving the error message.

Comment by Sebastian Bensusan [ 22/May/15 7:16 AM ]

Added the check in cljs.closure/source-on-disk where there is info for the error message.

For the supplied case, the error message is:

java.lang.IllegalArgumentException: The file file:/home/carlos/Playground/cljs-testcase/src/hello_world/closure.js 
lacks an associated source file. If it is a JavaScript library please add it to :libs}}

If a different wording or location of the check is needed, I'll submit a new patch with corrections.

Notes:

  • Changed:(:provides js) to (-provides js) in order to be consistent with IJavaScript.
  • cljs.clojure/source-on-disk takes a js argument that should satisfy with IJavaScript and ISourceMap if :source-map is enabled but the implementation is hardcoded to maps because :source-map and :source-url are used instead of ISourceMap methods -source-map and -source-url. I propose to extend PersistentMap and PersistentArrayMap to ISourceMap to make source-on-disk compliant with both protocols.




[CLJS-404] Automate Browser REPL testing Created: 23/Oct/12  Updated: 05/Jan/16

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

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


 Description   

It's worth investigating Selenium, PhantomJS, etc. as solutions to sanity check the Browser REPL when we run the other tests.



 Comments   
Comment by Robert Krahn [ 22/Dec/14 1:22 PM ]

An attempt: https://github.com/clojure/clojurescript/pull/42

Comment by David Nolen [ 24/Dec/14 8:57 AM ]

This looks like an interesting patch, thanks!

Comment by Robert Krahn [ 26/Dec/14 10:57 AM ]

I'll post a patch here, first I'll investigate the load-file issue, though.





[CLJS-374] satisfies? produces strange code when the protocol is not in the fast-path list Created: 06/Sep/12  Updated: 05/Jan/16

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

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





[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-1924] The compiler cannot infer the target type of "(. RecordName -prototype)" expressions Created: 01/Feb/17  Updated: 01/Feb/17

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

Type: Defect Priority: Minor
Reporter: Viktor Magyari Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: compiler, extern


 Description   

Repro:

Place

(set! *warn-on-infer* true)

(defrecord Foo [])

anywhere in your source files, compile with :infern-externs true.

Expected:

Multiple warnings like:

  • WARNING: Cannot infer target type in expression (. Foo -prototype)
  • WARNING: Cannot infer target type in expression (. other__8838__auto__ -constructor)
  • WARNING: Cannot infer target type in expression (. user/Foo -getBasis)

There are also warnings for (. cljs.core/List -EMPTY), but this might be unrelated.






[CLJS-1164] quot and rem are inefficient Created: 24/Mar/15  Updated: 25/May/17

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

Comment by Mike Fikes [ 25/May/17 7:29 PM ]

CLJS-1164-1.patch no longer applies to master





[CLJS-2103]  clarify arg type and order constraints of keys and vals Created: 19/Jun/17  Updated: 19/Jun/17

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

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

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

 Description   

Backport CLJ-1302 to ClojureScript, while also making the argument name be map instead of hash-map.






[CLJS-1415] Handling JSDoc param name [x] optional syntax Created: 10/Aug/15  Updated: 27/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: type-check





[CLJS-2127] Add invoke* helper macro Created: 26/Jun/17  Updated: 03/Jul/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: refactoring

Attachments: Text File CLJS-2127.patch    
Patch: Code
Approval: Screened

 Description   

This is a simple refactor around {IFn} protocol around core.cljc and core.cljs. We would like to hide the details of the invocation naming convention to avoid simple errors as well as to support changes more simply.



 Comments   
Comment by David Nolen [ 29/Jun/17 7:05 AM ]

The scope of this ticket needs to be narrowed to make it simpler for me to review. For the time being the only thing I would like to see is `invoke*` which hides the naming convention for direct invokes. No other higher level macro helpers should be provided in the resolution of this sissue.

Comment by A. R [ 29/Jun/17 12:29 PM ]

Patch updated. Much fewer changes to keep it simple for now.

Comment by David Nolen [ 29/Jun/17 2:08 PM ]

Looking better but lets have one helper for constructing the name, should just take number or :variadic.





[CLJS-2196] SpiderMonkey path needs quoting in test scripts Created: 08/Jul/17  Updated: 08/Jul/17

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

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

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

 Description   

The paths to the other engines (V8, Nashorn, etc.), are quoted, allowing paths with spaces. This needs to also be done for SpiderMonkey.






[CLJS-2383] Improve perf of select-keys by using keyword-identical? Created: 17/Oct/17  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File 0001-CLJS-2383-Speed-up-select-keys.patch     Text File 0002-CLJS-2383-Speed-up-select-keys-no-transient.patch     Text File CLJS-2383.patch    
Patch: Code and Test

 Description   

select-keys uses not= to compare keywords. Instead, using keyword-identical? results in decent speedups (an average of 1.34 across benchmarks and engines). Note that using identical? and lookup-sentinel doesn't appear to improve perf.

Speedup Summary:

V8: 1.15, 1.08, 1.08
  SpiderMonkey: 1.71, 1.48, 1.67
JavaScriptCore: 1.33, 1.35, 1.25
       Nashorn: 1.16, 1.04, 0.97
    ChakraCore: 1.59, 1.66, 1.72

Before:

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 179 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 121 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 183 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 251 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 201 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 290 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 112 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 73 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 119 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1277 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 524 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 635 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 463 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 268 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 414 msecs

After

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 155 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 112 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 169 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 146 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 135 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 173 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 84 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 54 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 95 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1099 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 502 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 648 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 292 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 151 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 240 msecs


 Comments   
Comment by Erik Assum [ 17/Oct/17 2:37 PM ]

Just want to bring your attention to CLJ-1789, where reimplementing `select-keys` in terms of reduce gave a significant speedup.
Added a patch to show that way.

Comment by Mike Fikes [ 18/Oct/17 6:46 AM ]

Here are the perf numbers for Erik's patch:

V8: 0.81, 1.14, 1.30
  SpiderMonkey: 1.49, 1.31, 1.58
JavaScriptCore: 1.02, 0.99, 0.96
       Nashorn: 1.13, 1.17, 1.21
    ChakraCore: 1.27, 1.35, 1.28

After:

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 220 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 106 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 141 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 169 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 153 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 183 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 110 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 74 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 124 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1128 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 447 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 524 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 366 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 199 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 323 msecs
Comment by Erik Assum [ 18/Oct/17 6:56 AM ]

Uploaded a patch without transients as well.

Comment by Mike Fikes [ 18/Oct/17 7:40 AM ]

Since the CLJ-1789 patch works better with larger maps, here is an additional perf test covering that case using the data from that ticket, testing both the original patch I had attached and Erik's subsequent patch. You can see the CLJ-1789 approach pays off for ClojureScript as well.

Erik, I see you attached a 3rd patch. I'd recommend adding perf numbers with each such patch, so the effect of the patch under advanced optimizations can be more readily assessed.

Engine          keyword-identical?  CLJ-1789
            V8:               1.13      1.29
  SpiderMonkey:               1.89      2.39
JavaScriptCore:               1.02      0.96
       Nashorn:               1.12      1.42
    ChakraCore:               1.68      1.82

Before:

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 373 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 668 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 200 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 2236 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1074 msecs

After (keyword-identical?)

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 330 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 353 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 197 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1991 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 640 msecs

After (CLJ-1789)

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 290 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 279 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 209 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1578 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 591 msecs
Comment by Erik Assum [ 18/Oct/17 7:54 AM ]

Both patches should have benchmarks now

Comment by Erik Assum [ 18/Oct/17 7:56 AM ]

oh, and as a comment to the comment about larger maps, I believe the performance `transient` bit is dependent on the size of the selected keys,
eg the more selected keys found in the map, the more we gain from `conj!`

Comment by Mike Fikes [ 23/Oct/17 12:04 PM ]

Running these 4 benchmarks

[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs

against all 5 engines with the 3 attached patches yields the following speedups on my machine:

CLJS-2383.patch:

            V8: 1.11, 1.10, 1.64, 1.18  Avg: 1.26
  SpiderMonkey: 2.36, 1.46, 1.79, 2.02  Avg: 1.91
JavaScriptCore: 1.28, 1.34, 1.23, 1.49  Avg: 1.34
       Nashorn: 1.19, 1.17, 1.06, 1.29  Avg: 1.18
    ChakraCore: 1.61, 1.78, 1.75, 2.11  Avg: 1.81
                                Overall avg: 1.50
         Avg excluding Nashorn & ChakraCore: 1.50

0001-CLJS-2383-Speed-up-select-keys.patch:

            V8: 0.70, 0.95, 1.05, 1.23  Avg: 0.98
  SpiderMonkey: 1.20, 1.09, 1.05, 2.03  Avg: 1.34
JavaScriptCore: 0.78, 0.84, 0.83, 1.02  Avg: 0.87
       Nashorn: 1.18, 1.08, 1.02, 1.48  Avg: 1.19
    ChakraCore: 1.00, 1.12, 1.19, 1.75  Avg: 1.27
                                Overall avg: 1.13
         Avg excluding Nashorn & ChakraCore: 1.06	

0002-CLJS-2383-Speed-up-select-keys-no-transient.patch:
	
            V8: 1.28, 1.45, 1.37, 1.33  Avg: 1.36
  SpiderMonkey: 1.54, 1.44, 1.70, 2.17  Avg: 1.71
JavaScriptCore: 1.01, 0.99, 1.03, 1.13  Avg: 1.04
       Nashorn: 1.39, 1.21, 1.14, 1.26  Avg: 1.25
    ChakraCore: 1.20, 1.23, 1.19, 1.39  Avg: 1.25
                                Overall avg: 1.32
         Avg excluding Nashorn & ChakraCore: 1.37
Comment by Mike Fikes [ 19/Nov/17 7:08 PM ]

Summary: If applied, CLJS-2383.patch would be the one to apply as it provides the greatest speedup of all the patches.





[CLJS-2346] Make (:require foo/bar) work for JS modules Created: 01/Sep/17  Updated: 05/Sep/17

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

Type: Enhancement Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: js-modules

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

 Description   

the string require only really becomes necessary when there are multiple slashes. We can support this style as a symbol for dependencies that only have 1 slash because it's a valid symbol.



 Comments   
Comment by Thomas Heller [ 02/Sep/17 2:43 AM ]

Me again ... what is so bad about using string requires? Qualified symbols are illegal for normal CLJS code and using a string already solves all potential issues. Adding a case where you can use a symbol if there is one slash but not if there are two is just going to confuse new users with no additional benefit.

Comment by Juho Teperi [ 05/Sep/17 3:40 AM ]

I can see the point of only allowing slashes with strings.

However, if we do this, it might be good to check the warnings we give when trying to do this:

(ns example.core
  (:require [react-dom/server :as sdf]))

  No such namespace: react-dom/server, could not locate react_dom_SLASH_server.cljs, react_dom_SLASH_server.cljc, or JavaScript source providing "server"

Notice how it says "server" instead of react-dom/server. This is because everything allows slashes, but foreign lib code only uses name part instead of namespace. (This patch doesn't change the warning.)

Comment by Thomas Heller [ 05/Sep/17 4:50 AM ]

There are other ambiguities when it comes to JS requires which is why I'm still advocating for just using strings in all circumstances.

Technically we could make {{(:require [@scoped/thing :as thing])}} work as well but shouldn't.

Also I'm pretty sure I saw several JS packages that either used "-" or "_" in their names. For CLJS we always convert to an underscore which would add more confusion if we don't do this for JS.

Yes, the warning should be fixed but wouldn't even be an issue if a string was used.





[CLJS-2045] sort-by: Avoid re-creating comparator Created: 20/May/17  Updated: 25/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

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

 Description   

The fn->comparator call should be lifted:

(sort (fn [x y] ((fn->comparator comp) (keyfn x) (keyfn y))) coll)
(let [comparator (fn->comparator comp)]
  (sort (fn [x y] (comparator (keyfn x) (keyfn y))) coll))

Also, fn->comparator is again called on the function in sort, not sure how to avoid that unless we copy the sort code into sort-by.






[CLJS-1601] Optimize cljs.core dump Created: 12/Mar/16  Updated: 19/Nov/17

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

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

 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.

Comment by David Nolen [ 16/Jun/17 12:41 PM ]

I'm questioning whether this actual valuable? It seems to me if you're serious about code size you would just use Transit and them load the analysis asynchronously?

Comment by Nikita Beloglazov [ 17/Jun/17 2:39 AM ]

Yes, if size is critical then there are better ways to hand-tune the way of loading analysis. At the same time having 3m vs 6m file for local/simple development is a nice win. The only downside is speed, but I feel like big reduction in size is better than small speed penalty.

Comment by Mike Fikes [ 19/Nov/17 7:56 PM ]

Patch no longer applies.





[CLJS-1587] Duplicate keys via quoting Created: 24/Feb/16  Updated: 19/Nov/17

Status: Open
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: Unresolved Votes: 1
Labels: None

Attachments: File CLJS-1587.diff    
Patch: Code and Test

 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.

Comment by Rohit Aggarwal [ 15/Jun/16 8:02 AM ]

The underlying problem for both is the same in that a PersistentHashSet is being created directly using a PersistentArrayMap where the keys are the elements from the provided sequence. It manifests itself in two places though.

Comment by Rohit Aggarwal [ 15/Jun/16 9:32 AM ]

I've taken the approach that if we see a quoted constant then don't create the PersistentHashSet directly and instead go via the fromArray function.

Patch has the fix and a test.

Comment by Mike Fikes [ 13/Jun/17 9:40 PM ]

Attached patch no longer applies to master.

Comment by A. R [ 14/Jun/17 1:43 AM ]

It'd be nice if this patch/ticket also included the following case:

(hash-set "a" \a)
Comment by A. R [ 14/Jun/17 10:50 AM ]

Should we increase the scope of this ticket? The same issue exists for maps:

{'0 "a", 0 "b"}
{\a "a", "a" "b"}

I think one possible solution that solves both, quoting and the char/string, could be to to call emit-str in cljs.compiler on the keys/set-members and only then check for uniqueness. Not sure that's a good idea though.

Doesn't solve the hash-set, array-map macros.

Edit: Related ticket: CLJS-2087

Comment by David Nolen [ 14/Jun/17 1:45 PM ]

Increasing the scope of tickets is not desirable. Please move to a separate issue and cross-reference thanks.

Comment by Mike Fikes [ 19/Nov/17 7:57 PM ]

Patch no longer applies.





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

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

Type: Enhancement Priority: Minor
Reporter: Crispin Wellington Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: cljs, enhancement, errormsgs, patch,
Environment:

Linux 64bit

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


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

 Description   

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

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

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

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

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



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

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

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

I did bind out to err. Check the patch.

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

Crispin, oops sorry you are correct. Thanks.

Comment by David Nolen [ 13/Mar/15 7:33 AM ]

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

Comment by David Nolen [ 14/Mar/15 5:55 AM ]

The solution does not work for cljsbuild. It's unclear why there so much machinery in place over the approach taken for deps.clj.

Comment by David Nolen [ 15/Mar/15 10:37 AM ]

Stalled on this cljsbuild issue https://github.com/emezeske/lein-cljsbuild/issues/383

Comment by Crispin Wellington [ 23/Mar/15 2:50 AM ]

This lein-cljsbuild issue is what made me make it just a warning initially, and not a hard error like raising IllegalArgumentException does. Though I agree it should be a hard error. If we start with a warning, it enables the immediate problem for the developer to be resolved, and leaves a wart that the cljs-build project can then see that need fixing on their end. Then when that end is fixed it could be made a hard error. If cljsbuild is fixed fairly soon then all is well, but if it takes a long time, a warning might be a good first step.

Comment by Mike Fikes [ 19/Nov/17 8:06 PM ]

Patch no longer applies. (Also it doesn't work with git am—see https://clojurescript.org/community/patches.)





[CLJS-2038] self calls do not optimize - regression Created: 15/May/17  Updated: 25/Jun/17

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

Type: Defect Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is a regression of:

https://dev.clojure.org/jira/browse/CLJS-275



 Comments   
Comment by A. R [ 18/May/17 2:19 AM ]

This issue can be solve by changing the defn macro for the "simple" case and carrying the function name over to the function:

(core/list 'def (with-meta name m)
  (cons `fn (cons name fdecl)))

This isn't done in clojure because of: https://dev.clojure.org/jira/browse/CLJ-809

Though I don't think that's and issue in CLJS since we don't have real vars anyways and can't redefine {{def}}s.

Comment by A. R [ 17/Jun/17 10:07 AM ]

So the issue that the CLJ ticket has is emulated/shown below in CLJS:

(enable-console-print!)

(defn self-call-test
  [n]
  (prn "inner")
  (when (pos? n)
    (self-call-test (dec n))))

(let [orig self-call-test]
  (set! self-call-test
        (fn outer [n]
          (pr "outer")
          (orig n))))
(self-call-test 2)

(def self-call-test2
  (fn self-call-test2
       [n]
       (prn "inner")
       (when (pos? n)
         (self-call-test2 (dec n)))))

(let [orig self-call-test2]
  (set! self-call-test2
        (fn outer [n]
          (pr "outer")
          (orig n))))
(self-call-test2 2)

Output in with no optimizations:

"outer"
"inner"
"outer"
"inner"
"outer"
"inner"


"outer"
"inner"
"inner"
"inner"

So: It does seem this would also break the current behaviour, HOWEVER, the above with advance optimizations gives this:

"outer"
"inner"
"inner"
"inner"

*for both*. Given this, it seem better to not change behavior during advanced builds to avoid hard to track down production bugs for the users. Even if this is a slight deviation from CLJ behavior. Thoughts?

Comment by A. R [ 25/Jun/17 2:27 PM ]

Any thoughts on this? I can create a patch if that this change is ok. It could matter a bit (performance wise) since a few of the very low level data structure functions are recursive.





[CLJS-2479] compare should accept objects with valueOf function Created: 24/Jan/18  Updated: 25/Jan/18

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Currently compare (for any non-CLJS IComparable) types only accepts primitives (numbers, bool, string, array) and forwards them to goog.array.defaultCompare (which is just a {{ return a > b ? 1 : a < b ? -1 : 0 }} ).

Though, the standard defines anything with a valueOf can be compared:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Relational_operators

since valueOf should return one of these primitves. Ie this works:

(let [x0 #js{:x 1, :valueOf #(-> 1)}
      x1 #js{:x 2, :valueOf #(-> 2)}
      dc garr/defaultCompare] 
  (list (dc x0 x1) (dc x1 x0) (dc x1 x1)))

We should allow this, ie. instead of:

:else
(if (and (or (string? x) (array? x) (true? x) (false? x))
         (identical? (type x) (type y)))
     (garray/defaultCompare x y)
     (throw (js/Error. (str "Cannot compare " x " to " y))))

do a:

(if (and (or (string? x) (array? x) (true? x) (false? x)
             (and (js-in "valueOf" x) (js-in "valueOf" y)))
         (identical? (type x) (type y)))
      (garr/defaultCompare x y)
      (throw (js/Error. (str "Cannot compare " x " to " y))))

We should also give js-in a boolean tag.






[CLJS-2268] clojure.string/escape in ClojureScript (unlike in Clojure) assumes cmap is a map Created: 23/Jul/17  Updated: 02/Mar/18

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

Type: Defect Priority: Minor
Reporter: Max Kreminski Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: cljs

Attachments: Text File 0001-CLJS-2268-Make-clojure.string-escape-constent-with-C.patch    
Patch: Code and Test

 Description   

The ClojureScript implementation of the clojure.string/escape function assumes that the cmap parameter will always be a map. This makes it different from (and specifically less general than) the Clojure implementation of the same function, which permits cmap to be anything callable.

Here's the relevant lines of the clojure.string/escape implementations in Clojure and ClojureScript. The ClojureScript implementation calls get on cmap, while the Clojure implementation invokes cmap directly.

Here's an example that works on Clojure, but doesn't work on ClojureScript, because it passes a function to clojure.string/escape instead of a map:

(defn regex-escape
  "Escapes regex special chars in the string `s`."
  [s]
  (let [special? #{\- \[ \] \{ \} \( \) \* \+ \? \. \\ \^ \$ \|}]
    (clojure.string/escape s #(when (special? %) (str \\ %)))))

Ideally, this discrepancy would be fixed by changing the ClojureScript implementation of clojure.string/escape to follow the Clojure one. This would also match the behavior described in the function's docstring, which is the same on both platforms.






[CLJS-2537] Negative fractional index in contains? on array Created: 19/Feb/18  Updated: 14/Mar/18

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

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

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

 Description   

Clojure:

user=> (contains? (to-array [7 13 41]) -0.5)
true
user=> (get (to-array [7 13 41]) -0.5)
7

ClojureScript:

cljs.user=> (contains? (to-array [7 13 41]) -0.5)
false
cljs.user=> (get (to-array [7 13 41]) -0.5)
7


 Comments   
Comment by Mike Fikes [ 20/Feb/18 6:36 AM ]

The problem might actually be with get. See how it behaves with arrays, and strings:

Clojure:

user=> (get (to-array [7 13 41]) -0.5)
7
user=> (get (to-array [7 13 41]) -0.5 :not-found)
7
user=> (get "ab" -0.5)
\a
user=> (get "ab" -0.5 :not-found)
\a

ClojureScript:

cljs.user=> (get (to-array [7 13 41]) -0.5)
7
cljs.user=> (get (to-array [7 13 41]) -0.5 :not-found)
:not-found
cljs.user=> (get "ab" -0.5)
"a"
cljs.user=> (get "ab" -0.5 :not-found)
:not-found
Comment by Mike Fikes [ 20/Feb/18 6:55 AM ]

It is evidently possible to fix without hurting perf, and the solution ends up having a nice symmetry with respect to the index lower and upper bound.

Comment by Mike Fikes [ 14/Mar/18 8:23 PM ]

CLJS-2537-2.patch rebaselines





[CLJS-2538] nth on fractional indices near array and string bounds Created: 20/Feb/18  Updated: 14/Mar/18

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

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

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

 Description   

Clojure:

user=> (nth (to-array [1 2]) -0.5)
1
user=> (nth (to-array [1 2]) 1.5)
2
user=> (nth "ab" -0.5)
\a
user=> (nth "ab" 1.5)
\b

ClojureScript:

cljs.user=> (nth (to-array [1 2]) -0.5)
Index out of bounds
	cljs.core/nth (cljs/core.cljs:1830:7)

cljs.user=> (nth (to-array [1 2]) 1.5)
nil
cljs.user=> (nth "ab" -0.5)
Index out of bounds
	cljs.core/nth (cljs/core.cljs:1830:7)

cljs.user=> (nth "ab" 1.5)
"b"


 Comments   
Comment by Mike Fikes [ 20/Feb/18 7:35 AM ]

Much like the changes in CLJS-2537 along with applying int to indices in a few places, which corrects the code and also makes it consistent with get and with Clojure.

Comment by Mike Fikes [ 14/Mar/18 8:25 PM ]

CLJS-2538-2.patch rebaselines





[CLJS-2342] Speed up printing of js object by using forEach and regex optimizations Created: 30/Aug/17  Updated: 14/Mar/18

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File CLJS-2342-2.patch     Text File CLJS-2342-3.patch     Text File CLJS-2342-4.patch     Text File CLJS-2342.patch    
Patch: Code

 Description   

By using goog.object/forEach, along with direct interop for regex checking it is possible to speed up the printing of JavaScript objects anywhere between 1.14 and 1.77



 Comments   
Comment by Mike Fikes [ 30/Aug/17 10:50 AM ]
Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.42, 1.31, 1.19
  SpiderMonkey: 1.14, 1.48, 1.41
JavaScriptCore: 1.48, 1.58, 1.62
       Nashorn: 1.27, 1.36, 1.20
    ChakraCore: 1.49, 1.77, 1.77


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 74 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 84 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 75 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 95 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 92 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 134 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 46 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 41 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 55 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 1228 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 1048 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 620 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 55 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 76 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 129 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 52 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 64 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 63 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 83 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 62 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 95 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 31 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 26 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 34 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 970 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 769 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 518 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 37 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 43 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 73 msecs
Comment by Mike Fikes [ 01/Oct/17 9:27 AM ]

Added CLJS-2342-2.patch which rebases against current master.

Comment by Mike Fikes [ 29/Oct/17 9:03 AM ]

CLJS-2342-3.patch rebaselines against current master.

Comment by Mike Fikes [ 14/Mar/18 8:20 PM ]

CLJS-2342-4.patch rebaselines





[CLJS-1975] Perf: Compare f with nil in Delay impl Created: 11/Mar/17  Updated: 21/Nov/17

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

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

 Description   

When a Delay has been realized, f is set to nil. We can avoid truth_ and not calls and directly compare with nil for a minor perf boost.

In script/noderepljs, this leads to these

(simple-benchmark [x (delay 1)] @x 1e9)
(simple-benchmark [x (delay 1)] (realized? x) 1e9)
(simple-benchmark [x (doto (delay 1) deref)] (realized? x) 1e9)

speeding up by 6%, 11% and 9%, respectively.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 2:35 PM ]

Under :advanced across all engines, the 3 benchmarks in the ticket description produce these speedup numbers:

            V8: 1.18, 1.15, 1.09
  SpiderMonkey: 1.08, 1.04, 1.19
JavaScriptCore: 1.70, 0.86, 0.93
       Nashorn: 1.05, 1.15, 1.14
    ChakraCore: 1.21, 0.52, 0.81




[CLJS-2120] Optimize keyword function Created: 24/Jun/17  Updated: 25/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 4
Labels: performance

Attachments: Text File CLJS-2120.patch    
Patch: Code
Approval: Accepted

 Description   

keyword function can be sped up. This matters when keywords are created dynamically when doing XHR.

The patch now matches Clojure more closely (using substring). It's also optimized for passing strings.

Results:

(enable-console-print!)
(let [sims 1000000]
  (dotimes [_ 2]
    (doseq [x ["foo" "foo/bar" [nil "foo"] ["foo" "bar"] [:foo :bar] [nil :foo]]]
      (prn "Testing keyword with: " x)
      (if (vector? x)
        (do (simple-benchmark [[a0 a1] x] (set! js/FOOO (keyword a0 a1)) sims)
            (simple-benchmark [[a0 a1] x] (set! js/FOOO (keyword2 a0 a1)) sims))
        (do (simple-benchmark [] (set! js/FOOO (keyword x)) sims)
            (simple-benchmark [] (set! js/FOOO (keyword2 x)) sims))))))


"Testing keyword with: " "foo"
[], (set! js/FOOO (keyword x)), 1000000 runs, 194 msecs
[], (set! js/FOOO (keyword2 x)), 1000000 runs, 71 msecs
"Testing keyword with: " "foo/bar"
[], (set! js/FOOO (keyword x)), 1000000 runs, 260 msecs
[], (set! js/FOOO (keyword2 x)), 1000000 runs, 104 msecs
"Testing keyword with: " [nil "foo"]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 278 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 188 msecs
"Testing keyword with: " ["foo" "bar"]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 379 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 215 msecs
"Testing keyword with: " [:foo :bar]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 351 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 207 msecs
"Testing keyword with: " [nil :foo]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 376 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 37 msecs


 Comments   
Comment by A. R [ 24/Jun/17 10:56 AM ]

Changes the behavior of:

((juxt namespace name) (keyword "foo/bar/hmm"))
=> [nil "foo"]
(.-fqn (keyword "foo/bar/hmm"))
=> "foo/bar/hmm"
((juxt namespace name) (keyword2 "foo/bar/hmm"))
=> ["foo" "bar/hmm"]
(.-fqn (keyword2 "foo/bar/hmm"))
=> "foo/bar/hmm"

Clojure 1.9:

((juxt namespace name) (keyword "foo/bar/hmm"))
=> ["foo" "bar/hmm"]

So: yay





[CLJS-2301] Avoid use of deprecated goog.string/isEmptySafe in clojure.string/blank? Created: 05/Aug/17  Updated: 05/Aug/17

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

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

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

 Description   

clojure.string/blank? calls goog.string/isEmptySafe, which is marked as deprecated. Instead this can be inlined with the code that this internally calls, which is non-deprecated code. Also, it can be seen that such a change has no effect on perf, with these benchmarking tests tried out:

Before:

Benchmarking with V8
[s nil f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 28 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 26 msecs
Benchmarking with SpiderMonkey
[s nil f clojure.string/blank?], (f s), 1000000 runs, 265 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 268 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 268 msecs
Benchmarking with JavaScriptCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s nil f clojure.string/blank?], (f s), 1000000 runs, 331 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 336 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 59 msecs
Benchmarking with ChakraCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 56 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 56 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 57 msecs


After:

Benchmarking with V8
[s nil f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 26 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
Benchmarking with SpiderMonkey
[s nil f clojure.string/blank?], (f s), 1000000 runs, 262 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 262 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 261 msecs
Benchmarking with JavaScriptCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s nil f clojure.string/blank?], (f s), 1000000 runs, 328 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 324 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 60 msecs
Benchmarking with ChakraCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 62 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 63 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 62 msecs
Lindas-iMac-2:clojurescript mfikes$ # Using or nil?


 Comments   
Comment by Mike Fikes [ 05/Aug/17 11:37 AM ]

The change in the attached patch was used to produce the benchmarks in the ticket description.

I also tried an alternative which uses a Clojure or and a nil? check in lieu of the goog.string/makeSafe call, and this resulted in the same benchmark numbers.

So, the patch goes with the recommended deprecation change in the documentation for goog.string/isEmptyOrWhitespaceSafe, which indicates "Use goog.string.isEmptyOrWhitespace(goog.string.makeSafe(str)) instead.", which exactly matches the code we are indirectly calling today.





[CLJS-2543] IndexingPushbackReader error when compiling :reload-all with cljs.spec.alpha Created: 22/Feb/18  Updated: 22/Feb/18

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

Type: Defect Priority: Minor
Reporter: Andrea Richiardi Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File cljs-master-error.txt    

 Description   

There seems to be an error when the following happens:

(ns repro.a-namespace
  (:require [cljs.spec.alpha :as s] :reload-all))

There is a repro here: https://github.com/arichiardi/cljs-reload-all-repro

The stack is enormous so I attached a file but the gist of it is:

Caused by: clojure.lang.ExceptionInfo: No implementation of method: :read-char of protocol: #'clojure.tools.reader.reader-types/Reader found for class: clojure.tools.reader.reader_types.IndexingPushbackReader {:type :reader-exception}
	at clojure.core$ex_info.invokeStatic(core.clj:4739)
	at clojure.core$ex_info.invoke(core.clj:4739)
	at clojure.tools.reader$read_STAR_.invokeStatic(reader.clj:941)
	at clojure.tools.reader$read_STAR_.invoke(reader.clj:905)
	at clojure.tools.reader$read.invokeStatic(reader.clj:972)
	at clojure.tools.reader$read.invoke(reader.clj:949)
	at cljs.analyzer$forms_seq_STAR_$forms_seq___3119$fn__3120$fn__3121.invoke(analyzer.cljc:3676)
	at cljs.analyzer$forms_seq_STAR_$forms_seq___3119$fn__3120.invoke(analyzer.cljc:3669)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:528)
	at clojure.core$seq__5124.invokeStatic(core.clj:137)
	at clojure.core$seq__5124.invoke(core.clj:137)
	at cljs.compiler$emit_source.invokeStatic(compiler.cljc:1389)
	at cljs.compiler$emit_source.invoke(compiler.cljc:1370)
	at cljs.compiler$compile_file_STAR_$fn__4580.invoke(compiler.cljc:1471)
	at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:1285)
	at cljs.compiler$with_core_cljs.invoke(compiler.cljc:1274)
	at cljs.compiler$compile_file_STAR_.invokeStatic(compiler.cljc:1456)
	at cljs.compiler$compile_file_STAR_.invoke(compiler.cljc:1449)
	at cljs.compiler$compile_file$fn__4611.invoke(compiler.cljc:1553)
	... 37 more


 Comments   
Comment by Mike Fikes [ 22/Feb/18 1:43 PM ]

Minimal repro, with no links to projects:

Place the following in src/repro/a_namespace.cljs:

(ns repro.a-namespace
  (:require [cljs.spec.alpha :as s]))

and compile with

clojure -Sdeps '{:deps {org.clojurescript {:git/url "https://github.com/clojure/clojurescript" :sha "7c754fbb9ffb9da790f21776d53a3b83deef922b"}}}' -m cljs.main -O simple -t node -c repro.a-namespace

This will compile properly.

Then revise the source file to add :reload-all in the ns form:

(ns repro.a-namespace
  (:require [cljs.spec.alpha :as s] :reload-all))

and attempt the same compile:

$ clojure -Sdeps '{:deps {org.clojurescript {:git/url "https://github.com/clojure/clojurescript" :sha "7c754fbb9ffb9da790f21776d53a3b83deef922b"}}}' -m cljs.main -O simple -t node -c repro.a-namespace
Exception in thread "main" clojure.lang.ExceptionInfo: failed compiling file:/Users/mfikes/Desktop/src/repro/a_namespace.cljs {:file #object[java.io.File 0x2904bb45 "/Users/mfikes/Desktop/src/repro/a_namespace.cljs"]}
	at clojure.core$ex_info.invokeStatic(core.clj:4739)
	at clojure.core$ex_info.invoke(core.clj:4739)
	at cljs.compiler$compile_file$fn__4619.invoke(compiler.cljc:1567)
	at cljs.compiler$compile_file.invokeStatic(compiler.cljc:1528)
	at cljs.compiler$compile_file.invoke(compiler.cljc:1504)
	at cljs.closure$compile_file.invokeStatic(closure.clj:558)
	at cljs.closure$compile_file.invoke(closure.clj:549)
	at cljs.closure$eval6938$fn__6939.invoke(closure.clj:627)
	at cljs.closure$eval6874$fn__6875$G__6863__6882.invoke(closure.clj:511)
	at cljs.closure$compile_sources$iter__7062__7066$fn__7067.invoke(closure.clj:972)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.Cons.next(Cons.java:39)
	at clojure.lang.RT.next(RT.java:706)
	at clojure.core$next__5108.invokeStatic(core.clj:64)
	at clojure.core$dorun.invokeStatic(core.clj:3134)
	at clojure.core$doall.invokeStatic(core.clj:3140)
	at clojure.core$doall.invoke(core.clj:3140)
	at cljs.closure$compile_sources.invokeStatic(closure.clj:968)
	at cljs.closure$compile_sources.invoke(closure.clj:957)
	at cljs.closure$build.invokeStatic(closure.clj:2756)
	at cljs.closure$build.invoke(closure.clj:2662)
	at cljs.build.api$build.invokeStatic(api.clj:205)
	at cljs.build.api$build.invoke(api.clj:189)
	at cljs.build.api$build.invokeStatic(api.clj:192)
	at cljs.build.api$build.invoke(api.clj:189)
	at cljs.cli$default_compile.invokeStatic(cli.clj:299)
	at cljs.cli$default_compile.invoke(cli.clj:274)
	at cljs.cli$compile_opt.invokeStatic(cli.clj:305)
	at cljs.cli$compile_opt.invoke(cli.clj:303)
	at cljs.cli$main.invokeStatic(cli.clj:427)
	at cljs.cli$main.doInvoke(cli.clj:416)
	at clojure.lang.RestFn.applyTo(RestFn.java:139)
	at clojure.core$apply.invokeStatic(core.clj:659)
	at clojure.core$apply.invoke(core.clj:652)
	at cljs.main$_main.invokeStatic(main.clj:60)
	at cljs.main$_main.doInvoke(main.clj:52)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.lang.Var.applyTo(Var.java:702)
	at clojure.core$apply.invokeStatic(core.clj:657)
	at clojure.main$main_opt.invokeStatic(main.clj:317)
	at clojure.main$main_opt.invoke(main.clj:313)
	at clojure.main$main.invokeStatic(main.clj:424)
	at clojure.main$main.doInvoke(main.clj:387)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.lang.Var.applyTo(Var.java:702)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.ExceptionInfo: No implementation of method: :read-char of protocol: #'clojure.tools.reader.reader-types/Reader found for class: clojure.tools.reader.reader_types.IndexingPushbackReader {:type :reader-exception}
	at clojure.core$ex_info.invokeStatic(core.clj:4739)
	at clojure.core$ex_info.invoke(core.clj:4739)
	at clojure.tools.reader$read_STAR_.invokeStatic(reader.clj:941)
	at clojure.tools.reader$read_STAR_.invoke(reader.clj:905)
	at clojure.tools.reader$read.invokeStatic(reader.clj:972)
	at clojure.tools.reader$read.invoke(reader.clj:949)
	at cljs.analyzer$forms_seq_STAR_$forms_seq___3329$fn__3330$fn__3331.invoke(analyzer.cljc:3676)
	at cljs.analyzer$forms_seq_STAR_$forms_seq___3329$fn__3330.invoke(analyzer.cljc:3669)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:528)
	at clojure.core$seq__5124.invokeStatic(core.clj:137)
	at clojure.core$seq__5124.invoke(core.clj:137)
	at cljs.compiler$emit_source.invokeStatic(compiler.cljc:1389)
	at cljs.compiler$emit_source.invoke(compiler.cljc:1370)
	at cljs.compiler$compile_file_STAR_$fn__4588.invoke(compiler.cljc:1471)
	at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:1285)
	at cljs.compiler$with_core_cljs.invoke(compiler.cljc:1274)
	at cljs.compiler$compile_file_STAR_.invokeStatic(compiler.cljc:1456)
	at cljs.compiler$compile_file_STAR_.invoke(compiler.cljc:1449)
	at cljs.compiler$compile_file$fn__4619.invoke(compiler.cljc:1553)
	... 44 more
Caused by: java.lang.IllegalArgumentException: No implementation of method: :read-char of protocol: #'clojure.tools.reader.reader-types/Reader found for class: clojure.tools.reader.reader_types.IndexingPushbackReader
	at clojure.core$_cache_protocol_fn.invokeStatic(core_deftype.clj:583)
	at clojure.core$_cache_protocol_fn.invoke(core_deftype.clj:575)
	at clojure.tools.reader.reader_types$eval19106$fn__19118$G__19095__19123.invoke(reader_types.clj:24)
	at clojure.tools.reader$read_STAR_.invokeStatic(reader.clj:916)
	... 62 more




[CLJS-2459] Compiler should emit warning about namespace and/or non-existent var Created: 05/Jan/18  Updated: 06/Jan/18

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

Type: Enhancement Priority: Minor
Reporter: Michiel Borkent Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Approval: Vetted

 Description   

Made project as described in quickstart.

hello-world/core.cljs

(ns hello-world.core
  (:require [hello-world.foo]))

(enable-console-print!)

(println "Hello world!")
(hello-world.foo/foo)
;; no warning

hello_world/foo.cljs

(ns hello-world.foo2)
(defn foo []
  (println "o no"))

Note that `foo.cljs` has a namespace name that is not consistent with the file structure.

I expect
1) a warning about `(:require [hello-world.foo])` not being able to find the namespace, and/or
2) a warning about `(hello-world.foo/foo)` being a non-existent var.






[CLJS-1965] letfn collisions across namespaces Created: 02/Mar/17  Updated: 02/May/18

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

Type: Defect Priority: Minor
Reporter: Jeremy R Sellars Assignee: David Nolen
Resolution: Unresolved Votes: 3
Labels: None
Environment:

Affects 1.9.495 and earlier. Probably only affects browsers (not nodejs).


Attachments: Text File CLJS-1965-failing-tests.patch     Text File cljs-1965_wrap_letfn_statements.patch     Text File CLJS-1965-Wrap-top-level-letfn-to-avoid-collisions.patch    
Patch: Code

 Description   

If you `letfn` a fn with the same name in two namespaces, the wrong fn is used.

one.cljs
(ns hello-world.one)

(letfn [(answer [] "1")]
  (defn get-answer []
    (answer)))
two.cljs
(ns hello-world.two)

(letfn [(answer [] "2")]
  (defn get-answer []
    (answer)))
core.cljs
(ns hello-world.core
  (:require [hello-world.one]
            [hello-world.two]))

(println "one =>" (hello-world.one/get-answer))  ; one => 1
(println "two =>" (hello-world.two/get-answer))  ; two => 1      WHAT?!?

This issue seems to exist both on top-level `letfn`s and non-top-level `(let [] (letfn [...]))`.



 Comments   
Comment by Jeremy R Sellars [ 02/Mar/17 4:21 PM ]

This patch wraps `letfn` :expr and :statement forms in a function declaration (formerly, only :expr forms were wrapped).

I only did minimal testing. It fixes the code from the description.

Note: This is my first crack at the compiler and it is entirely possible I have not understood the entire problem.

Comment by Mike Fikes [ 16/Jun/17 10:54 AM ]

Confirmed that this fixes things downstream in self-hosted ClojureScript (Planck):

Without the patch:

cljs.user=> (require 'hello-world.core)
one => 2
two => 2
nil

With the patch:

cljs.user=> (require 'hello-world.core)
one => 1
two => 2
nil
Comment by daniel sutton [ 29/Dec/17 3:18 PM ]

This doesn't require different namespaces. The bug is that `let-fn` is putting its binding as a global variable.

And easy reproduction is
1. `lein new mies letfn-bug`,
2. update cljs version to `[org.clojure/clojurescript "1.9.946"]`
3. and then

(ns letfn-bug.core
  (:require [clojure.browser.repl :as repl]))

(enable-console-print!)

(letfn [(non-unique-name [] 4)]
  (defn f1 [] (non-unique-name)))

(letfn [(non-unique-name [] 5)]
  (defn f2 [] (non-unique-name)))

(println "should be 4-> " (f1))
(println "should be 5-> " (f2))

then `scripts/repl`.

results in:

cljs.user=> (load-file "letfn_bug/core.cljs")
should be 4->  5
should be 5->  5
nil
cljs.user=>

With the generated js:

// Compiled by ClojureScript 1.9.946 {}
goog.provide('letfn_bug.core');
goog.require('cljs.core');
goog.require('clojure.browser.repl');
cljs.core.enable_console_print_BANG_.call(null);
var non_unique_name = (function letfn_bug$core$non_unique_name(){
return (4);
});
letfn_bug.core.f1 = (function letfn_bug$core$f1(){
return non_unique_name.call(null);
});
var non_unique_name = (function letfn_bug$core$non_unique_name(){
return (5);
});
letfn_bug.core.f2 = (function letfn_bug$core$f2(){
return non_unique_name.call(null);
});
cljs.core.println.call(null,"should be 4-> ",letfn_bug.core.f1.call(null));
cljs.core.println.call(null,"should be 5-> ",letfn_bug.core.f2.call(null));
Comment by David Nolen [ 05/Jan/18 10:42 AM ]

Quick review of the patch, instead of always wrapping in a statement context it might be better if we only did it at the top-level where this is actually a problem.

Comment by George Lipov [ 01/May/18 2:27 PM ]

I'd just like to chime in and suggest that the priority on this be raised. A compiler breaking scope like this is certainly not a minor issue.

Comment by Jeremy R Sellars [ 02/May/18 10:06 AM ]

This patch is like the first, but only function-wraps statement letfn forms that are outside a fn-scope. (CLJS-1965-Wrap-top-level-letfn-to-avoid-collisions.patch)





[CLJS-2004] Minor fix for test-simple script Created: 10/Apr/17  Updated: 08/May/18

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

Type: Enhancement Priority: Minor
Reporter: Dejan Josifovic Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: clojurescript, script, test
Environment:

Lubuntu 16.10


Attachments: Text File CLJS-2004.patch     Text File CLJS-2004-rebase-08052018.patch     Text File CLJS-2004-rebase.patch    
Patch: Code

 Description   

On Ubuntu based Linux distributions $[] doesn't work.
Output is: Tested with $[ran+1] out of 4 possible js targets
and should be: Tested with 4 out of 4 possible js targets

As in CLJS-929 (for test script), $(()) will work for ash, dash, bash, and zsh.



 Comments   
Comment by Dejan Josifovic [ 10/Apr/17 2:42 PM ]

Add patch.

Comment by Mike Fikes [ 19/Nov/17 7:32 PM ]

Patch no longer applies; needs re-baseline.

Comment by Dejan Josifovic [ 12/Dec/17 5:33 PM ]

Hi,
Sorry for the late replay.
Rebased patch is uploaded.
Regards.

Comment by Mike Fikes [ 08/May/18 9:48 AM ]

CLJS-2004-rebase.patch no longer applies

Comment by Dejan Josifovic [ 08/May/18 1:03 PM ]

Done.
Cheers.





[CLJS-2132] Optimize transient vector creation Created: 27/Jun/17  Updated: 27/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 3
Labels: performance

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

 Description   

This is a very simple optimization around transient []. It avoids copying the empty array.

Performance improvements, for mapv on smallish vectors (5-32) elements anywhere from 20% up to 100% across FF & Chrome.

(defn faster-editable-root
  [node]
  (if (identical? (.-EMPTY_NODE PersistentVector) node)
    (VectorNode. (js-obj) (make-array 32))
    (VectorNode. (js-obj) (aclone (.-arr node)))))
(def orig-editabe-root tv-editable-root)
(enable-console-print!)
(dotimes [_ 2]
  (doseq [size [5 10 40]]
    (let [xs (range size)
          sims 500000]
      (set! tv-editable-root orig-editabe-root)
      (prn "Size: " size)
      (simple-benchmark [] (mapv inc xs) sims)
      (set! tv-editable-root faster-editable-root)
      (prn "NEW:")
      (simple-benchmark [] (mapv inc xs) sims))))





[CLJS-2770] Problem with namespaces starting with com in Rhino Created: 09/Jun/18  Updated: 17/Jun/18

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

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

{:deps {org.clojure/clojurescript {:mvn/version "1.10.238"}}}


Approval: Screened

 Description   
$ clj -m cljs.main -re rhino -r
ClojureScript 1.10.238
cljs.user=> (ns com.foo)

com.foo=> (def x 3)
#'com.foo/x
com.foo=> x
org.mozilla.javascript.EcmaError: TypeError: Cannot call property cljs$lang$ctorPrWriter in object [JavaPackage com.foo.x]. It is not a function, it is "object". (/var/folders/gx/nymj3l7x4zq3gxb97v2zwzb40000gn/T/out314052489996668423318817887553502/goog/../cljs/core.js#32857)
	 cljs$core$pr_writer_impl (cljs/core.cljs:9919:9)
	 cljs.core/pr-writer (cljs/core.cljs:10003:5)
	 cljs.core/pr-seq-writer (cljs/core.cljs:10006:4)
	 cljs$core$pr_sb_with_opts (cljs/core.cljs:10014:6)
	 cljs$core$pr_str_with_opts (cljs/core.cljs:10024:5)
	 (cljs/core.cljs:10052:3)
	 cljs$core$pr_str (cljs/core.cljs:10049:1)
	 (<NO_SOURCE_FILE> <cljs repl>:1:0)


 Comments   
Comment by David Nolen [ 15/Jun/18 3:48 PM ]

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

Comment by David Nolen [ 17/Jun/18 9:04 AM ]

See CLJS-2784. Reverting and lowering the priority on this one.

Comment by Thomas Heller [ 17/Jun/18 10:29 AM ]

Given that the error message contains JavaPackage com.foo.x is it possible that this affects anything that has a Java class loaded? Given that Java uses TLDs commonly how about org?





[CLJS-2466] Faster reduce for lazy-seqs Created: 12/Jan/18  Updated: 19/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 5
Labels: None

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

 Description   

Lazy seqs that are built from vectors/chunked-seqs can sometimes take a slow first/next reduce.

Observation:

(def xs (vec (range 3000000)))

(time (reduce max xs)) ;; #1: 130ms, (reference)
(time (reduce max (lazy-cat xs))) ;; #2: 130ms, just as fast
(time (reduce max 0 (lazy-cat xs))) ;; #3: 500ms, 4x slower!!

;; Double them with concat and they're not 2x slower but 10x slower:
(time (reduce max (lazy-cat xs xs))) ;; #4: 1200ms
(time (reduce max 0 (lazy-cat xs xs))) ;; #5: 1200ms

Explanation for #3: The problem is that seq-reduce when called without init will properly call reduce again and take a fast path. With init given it won't ever escape to a faster reduce but will stick to first/next.

Note: In Clojure they scale "properly" (first 3 are ~45ms, last 2 are ~110ms).

The reason is that Clojure properly escapes to a fast path:

https://github.com/clojure/clojure/blob/2b242f943b9a74e753b7ee1b951a8699966ea560/src/clj/clojure/core/protocols.clj#L131-L143

This is an RFC, since I'm not 100% certain about the implemenation. Need to be careful to not blow the stack here...

Questions:
1. Should ChunkedCons implement IReduce? I think so.



 Comments   
Comment by A. R [ 12/Jan/18 1:59 AM ]

Timings with advanced compilation:

CHROME:
"OLD"
#1: "Elapsed time: 21.870000 msecs"
#2: "Elapsed time: 25.485000 msecs"
#3: "Elapsed time: 134.900000 msecs"
#4: "Elapsed time: 317.155000 msecs"
#5: "Elapsed time: 313.225000 msecs"
"NEW"
#1: "Elapsed time: 23.290000 msecs"
#2: "Elapsed time: 19.445000 msecs"
#3: "Elapsed time: 20.075000 msecs"
#4: "Elapsed time: 102.895000 msecs"
#5: "Elapsed time: 100.430000 msecs"
"OLD"
#1: "Elapsed time: 19.585000 msecs"
#2: "Elapsed time: 19.475000 msecs"
#3: "Elapsed time: 87.805000 msecs"
#4: "Elapsed time: 303.340000 msecs"
#5: "Elapsed time: 291.680000 msecs"
"NEW"
#1: "Elapsed time: 20.760000 msecs"
#2: "Elapsed time: 19.690000 msecs"
#3: "Elapsed time: 18.960000 msecs"
#4: "Elapsed time: 101.385000 msecs"
#5: "Elapsed time: 101.290000 msecs"

FIREFOX:

"OLD"
#1: "Elapsed time: 7.880000 msecs"
#2: "Elapsed time: 7.820000 msecs"
#3: "Elapsed time: 69.460000 msecs"
#4: "Elapsed time: 197.800000 msecs"
#5: "Elapsed time: 209.300000 msecs"
"NEW"
#1: "Elapsed time: 7.380000 msecs"
#2: "Elapsed time: 7.360000 msecs"
#3: "Elapsed time: 8.100000 msecs"
#4: "Elapsed time: 110.640000 msecs"
#5: "Elapsed time: 89.960000 msecs"
"OLD"
#1: "Elapsed time: 6.520000 msecs"
#2: "Elapsed time: 7.360000 msecs"
#3: "Elapsed time: 106.920000 msecs"
#4: "Elapsed time: 252.300000 msecs"
#5: "Elapsed time: 249.800000 msecs"
"NEW"
#1: "Elapsed time: 7.360000 msecs"
#2: "Elapsed time: 6.940000 msecs"
#3: "Elapsed time: 6.880000 msecs"
#4: "Elapsed time: 99.380000 msecs"
#5: "Elapsed time: 53.220000 msecs"
Comment by A. R [ 12/Jan/18 2:14 AM ]

Impact on truly (unchunked) lazy-seqs is neglibile (hard to measure due to garbage collection causing a lot of variance).

Comment by A. R [ 12/Jan/18 10:22 AM ]

New ticket for reduce for ChunkedCons: CLJS-2468

Comment by A. R [ 13/Jan/18 7:00 AM ]

Depends on CLJS-2469 for tests to pass. Patch is attached.

Comment by Thomas Mulvaney [ 18/Jun/18 12:16 PM ]

ChunkedSeq's already implements an efficient reduce stategy. I believe the issue is that LazySeq which is just a container that may have a different type of seq inside, is always calling `seq-reduce` on that inner sequence. Sometimes, such as in the case of ChunkedSeq, the inner sequence actually knows how to reduce itself best.

So I think the solution is as simple as changing the body of LazySeq.IReduce.-reduce from: (seq-reduce f init coll) to (reduce f init (seq coll). The key here, is calling seq pulls out the inner seq if there is one, and then reduce will call the best pathway for it.

Hope this helps.

Comment by A. R [ 18/Jun/18 1:08 PM ]

I don't think that's a good idea. You can easily blow the stack with that approach. Though, I haven't tried it.

Comment by Thomas Mulvaney [ 19/Jun/18 4:43 AM ]

One thing worth considering, the patch improves a particular case, ChunkedSeqs. seqs of hash-maps, IndexedSeqs, Ranges, are not handled. Dispatching, to reduce would fix this. You may have a point about blowing the stack, I haven't experimented though.

For example, (reduce + (range 10000)) is much faster than (reduce + (lazy-cat (range 10000)), but making LazySeq's reduce call reduce on the inner collection should fix this issue.

Comment by A. R [ 19/Jun/18 1:22 PM ]

Thomas, happy to bounce ideas back and forth. Feel free to compare your ideas to my patch with perf measures. The problem with calling reduce on the inner fn is the stack and also the non-preserving reduce. And wrapping the reducing function is pretty expensive. I tried that and it's much slower. IIRC that's why I did it that way. But again, need to be very careful not to blow the stack. That'd be a huge problem for users.





[CLJS-2341] Speed up js->clj on objs using forEach and transient volatile Created: 30/Aug/17  Updated: 30/Jun/18

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 9
Labels: performance

Attachments: Text File CLJS-2341-2.patch     Text File CLJS-2341.patch    
Patch: Code

 Description   

It is possible to speed up js->clj on JavaScript objects by revising the implementation to employ goog.object/forEach, accumulating by bashing on a transient volatile map.

The speedups range from 1.5 to 2.1 over the current implementation.

Note: The current implementation uses js-keys. The optimization in CLJS-2340 could help js->clj, but it doesn't appear to provide much speedup in itself (perhaps 1.1?) compared to the change in implementation described above.



 Comments   
Comment by Mike Fikes [ 30/Aug/17 9:19 AM ]
Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.62, 1.50, 1.13
  SpiderMonkey: 1.91, 1.74, 1.59
JavaScriptCore: 1.67, 1.74, 2.10
       Nashorn: 1.54, 2.13, 1.51
    ChakraCore: 1.71, 2.10, 1.95


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 55 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 63 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 93 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 155 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 87 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 94 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 45 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 33 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 42 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 1123 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 1195 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 773 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 65 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 44 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 78 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 34 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 42 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 82 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 81 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 50 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 59 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 27 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 19 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 20 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 728 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 561 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 513 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 38 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 21 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 40 msecs
Comment by Thomas Mulvaney [ 30/Apr/18 7:38 AM ]

Hi Mike. I don't think the volatile is required, see for example: PersistentHashMap.createWithCheck - the transient can be mutated in place. Although quite often in core.cljs it does seem to be passed as an arg in recursive calls such as in most of the .-fromArray methods of collections. Removing the volatile will likely make even more of an improvement.

Comment by Francis Avila [ 30/Apr/18 10:09 AM ]

To clarify what Thomas said: It's not part of the contract of transients that they can be mutated in place--you are supposed to use the return value of assoc! etc as you have done.

If you bash in place, you will get this:

(let [t (transient {})]
  (dotimes [i 32]
    (assoc! t i i))
  (persistent! t))
=> {0 0, 1 1, 2 2, 3 3, 4 4, 5 5, 6 6, 7 7} ;; broken!

(t is initially a transient array-map; assoc! returns a transient hash-map after 8 items, which you never see because of the in-place bashing.)

So your patch is good in this respect: you should definitely not bash transients in place!

However, particular transient types may be implemented in such a way that the only thing that their edit functions (assoc! dissoc! etc) ever return is the same (identical) object passed in to them. This is true for TransientHashMap's assoc!, which is why PersistentHashMap.createWithCheck is not broken. (It's liable to be brittle though if its implementation ever changes--I hope createWithCheck has good test coverage.)

Comment by Mike Fikes [ 30/Apr/18 10:13 AM ]

So, TL;DR is that, with some risk of brittleness, we could bash in place and potentially get more perf.

Comment by Francis Avila [ 30/Apr/18 10:35 AM ]

No, wrong TLDR: in this particular patch you cannot bash (transient {}) in place, because this starts out as an array-map and returns a transient hash-map once assoc! would return a map with a count greater than 8.

PersistentHashMap.createWithCheck (the example Thomas cited) can bash because it is written very carefully and knows all the types and their implementations.

If you only use hash-maps e.g. with (transient (hash-map)) you can bash in place, but then you won't have array-map for small maps.

(Really, don't bash transients in place.)

Comment by Thomas Mulvaney [ 30/Apr/18 11:00 AM ]

Francis is right, ignore me. Bashing things in place that don't change type works but possibly might break if implementations change. Docs clearly say its a bad idea - just because I've seen it done doesn't make it ok!

Comment by Mike Fikes [ 30/Jun/18 10:12 AM ]

Since CLJS-844 has been applied, the previous patch and benchmarks are no longer applicable. Attaching re-basedlined patch. It gives mixed results relative to the code now on master. Including updated benchmarks here so that a decision can be taken on whether it is worth applying or not.

Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.04, 1.05, 0.92
  SpiderMonkey: 1.09, 1.00, 1.08
JavaScriptCore: 1.00, 1.31, 1.35
       Nashorn: 0.72, 0.93, 0.90
    ChakraCore: 1.07, 1.00, 1.09
       GraalVM: 0.98, 0.99, 0.84
	   
	   
Before:

Benchmarking with V8
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 25 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 22 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 45 msecs
Benchmarking with SpiderMonkey
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 49 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 30 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 39 msecs
Benchmarking with JavaScriptCore
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 15 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 17 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 23 msecs
Benchmarking with Nashorn
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 330 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 411 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 343 msecs
Benchmarking with ChakraCore
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 31 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 11 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 25 msecs
Benchmarking with GraalVM
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 243 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 318 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 419 msecs

After:

Benchmarking with V8
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 24 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 21 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 49 msecs
Benchmarking with SpiderMonkey
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 45 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 30 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 36 msecs
Benchmarking with JavaScriptCore
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 15 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 13 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 17 msecs
Benchmarking with Nashorn
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 461 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 440 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 381 msecs
Benchmarking with ChakraCore
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 29 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 11 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 23 msecs
Benchmarking with GraalVM
;;; js->clj
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 247 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 320 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 500 msecs




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

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-4.patch     Text File CLJS-1627-5.patch     Text File CLJS-1627-6.patch     Text File CLJS-1627-7.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.

Comment by Patrick Killean [ 09/May/16 4:58 PM ]

CLJS-1627-1.patch:
resolve-existing-var now has an additional arity that accepts a missing-var handler passed to confirm-existing-var

Comment by Patrick Killean [ 10/May/16 6:16 AM ]

This has revealed a problem where deftype + defrecord using Object protocols emit resolved names when really they shouldn't. For example : "@implements {cljs.core.async.impl.timers.Object}" --> Bad Type Annotation

Since Object is a special case simply excluding it from the comments should fix it. Another patch incoming

Comment by Patrick Killean [ 10/May/16 7:42 AM ]

CLJS-1627-2.patch:
The emit* methods for deftype and defrecord now filter out Object protocols.

This produced an interesting result! With no more bad type annotations, static analysis can now proceed... and it has alot to say. Theres all kinds of info now about arity discrepencies (particularly cljs.core.IndexedSeq), type mismatches, and more. It even includes a type coverage percentage. Lots to parse here but very cool.

Comment by Patrick Killean [ 18/May/16 4:26 PM ]

CLJS-1627-3.patch:

  • fix require extern
  • add type application support for Array & Object
  • GC likes uppercase for Object & Array, lowercase for string, number.
  • support for explicit nullable types, variable typed arg
  • function type context modifiers this + new

Missing is the GC 'record type' . It also may be useful to fill out the node externs for common types

Comment by Patrick Killean [ 20/May/16 11:42 AM ]

CLJS-1627-4.patch:

  • fix a few problems in last patch
  • add record type support. Everything here should be covered
Comment by Patrick Killean [ 02/Sep/16 8:21 AM ]

update patch

Comment by Mike Fikes [ 19/Nov/17 7:55 PM ]

CLJS-1627-5.patch no longer applies

Comment by Patrick Killean [ 15/Apr/18 6:50 AM ]

patch 6:

  • routes js* comments through comp/emit-comment which has been altered to handle inline comments.
  • Supported tags: param, return, type, implements, typedef, enum, extends, throws, lends, const, this
  • add macro core/goog-typedef. This lets you name a custom + type refer to it in annotations.

More work is needed to support multi-arity fns, but I think this pretty much unlocks basic static type checking

Comment by Mike Fikes [ 15/Apr/18 7:31 AM ]

Hey Patrick, CLJS-1627-6.patch does not apply to master.

Comment by Patrick Killean [ 15/Apr/18 7:42 AM ]

shoulda had coffee first

Comment by Mike Fikes [ 28/May/18 8:15 AM ]

CLJS-1627-7.patch doe not apply to current master





[CLJS-2469] ChunkedCons chunk-next returns empty seq instead of nil Created: 13/Jan/18  Updated: 04/Jul/18

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

Type: Defect Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

There is a bug in ChunkedCons. In Clojure ChunkedCons (correctly) always calls seq in chunked-next. In CLJS it's not done. But since ChunkedCons has to be lazy it pretty much always gets an (empty) lazy seq as the "more" part.

Bug:

(-> (map inc (vec (range 64)))
    seq
    chunk-next
    seq
    chunk-next)

Returns and empty sequence instead of nil. This hasn't surfaced yet since nothing calls chunk-next on a ChunkedCons (yet).



 Comments   
Comment by A. R [ 13/Jan/18 7:26 AM ]

Found another bug that surfaced: Current implementation calls -seq on more which could be nil and this would blow up. So the patch also make a tiny change to -next also just calling seq on more. Pretty straight forward.

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

This patch needs a test.

Comment by A. R [ 20/Jan/18 12:27 AM ]

Test's added.

Comment by Mike Fikes [ 14/Mar/18 7:00 PM ]

patch does not apply

Comment by A. R [ 04/Apr/18 2:49 AM ]

Patch rebased

Comment by Mike Fikes [ 04/Jul/18 8:02 AM ]

Patch no longer applies.





[CLJS-2424] Improve compiler munge performance Nr 2 Created: 28/Nov/17  Updated: 08/Jul/18

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 4
Labels: performance

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

 Description   

This is similar to CLJS-2065 and further improves the performance by avoiding reduce and using a key iterator instead.

Results for a large CLJS project with lots of namespaces are:

  • Initial compile (cold) Old: 11.4s New: 11.2s
  • First full recompile: Old: 6.8s New: 5.9s
  • After a few full recompiler (warmed up JVM): Old: ~6.1s New: 5.1s

lein count:

Ext Files Lines of Code Nodes
cljs 138 23388 424745


 Comments   
Comment by David Nolen [ 22/Dec/17 2:24 PM ]

No difference for compiling ClojureScript tests, I will give a try with something else.

Comment by A. R [ 26/Apr/18 5:23 AM ]

Kind of related: CLJS-2461
Better approach would be to re-implement this idea: https://github.com/clojure/clojurescript/commit/1c71ab3bff27dc4a099b06e122ec3fdf5a2a8ba8

Close this issue?

Comment by Mike Fikes [ 08/Jul/18 7:14 AM ]

FWIW, patch no longer applies.





[CLJS-2813] Make JSValue be serializable Created: 10/Jul/18  Updated: 11/Jul/18

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-2813.patch    
Patch: Code and Test

 Description   

If you have code like (let [#js [1] 3]), the JSValue is placed in the resulting compiler exception. Since JSValue is not serializable (it is just a plain deftype), this evidently causes an issue in environments expecting exceptions to be serializable (boot is evidently one).

Notes: Simply changing to defrecord causes unit tests to fail for some reason. Also, see CLJS-1898.



 Comments   
Comment by Mike Fikes [ 11/Jul/18 9:45 AM ]

The problem with using defrecord for JSValue appears to be rooted in the need for some core functions that are not yet available when bootstrapping.

The attached patch takes a less aggressive approach and simply makes JSValue be Serializable, along with testing this aspect under Clojure.





[CLJS-2817] Suppress private var warnings for specs on private vars Created: 12/Jul/18  Updated: 12/Jul/18

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

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

{:deps {org.clojure/clojurescript {:git/url "https://github.com/clojure/clojurescript" :sha "0773689ec748109a8c09ba924f90c25875eb6a9d"}}}


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

 Description   

Observe the warnings generated here:

$ clj -m cljs.main
Checking out: https://github.com/clojure/clojurescript at 0773689ec748109a8c09ba924f90c25875eb6a9d
cljs.user=> (defn- foo [x] x)
#'cljs.user/foo
cljs.user=> (ns foo.core)

foo.core=> (require '[clojure.spec.alpha :as s])

foo.core=> (s/fdef cljs.user/foo :args (s/cat :x int?))
WARNING: var: cljs.user/foo is not public at line 1 <cljs repl>
WARNING: var: cljs.user/foo is not public at line 1 <cljs repl>
cljs.user/foo

The private var feature CLJS-1702 already supports instrumentation of private vars (albeit where the specs are being constructed in that private namespace, just enabled from another). The above case is perhaps a mild extension to it and it is debatable as to whether a diagnostic should be emitted.

We have a case here, where this is done in core (it only affects self-hosted), so if this ticket is not addressed perhaps we should address that case. https://github.com/clojure/clojurescript/blob/0773689ec748109a8c09ba924f90c25875eb6a9d/src/main/cljs/cljs/core/specs/alpha.cljc#L201



 Comments   
Comment by Mike Fikes [ 12/Jul/18 8:26 AM ]

Note: Clojure is OK with this:

$ clj
Clojure 1.9.0
user=> (defn- foo [x] x)
#'user/foo
user=> (ns foo.core)
nil
foo.core=> (require '[clojure.spec.alpha :as s])
nil
foo.core=> (s/fdef user/foo :args (s/cat :x int?))
user/foo




[CLJS-2819] Warn on non-dynamic earmuffed vars Created: 12/Jul/18  Updated: 12/Jul/18

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-2819-2.patch     Text File CLJS-2819-3.patch     Text File CLJS-2819.patch    
Patch: Code and Test

 Description   

Clojure:

user=> (def *foo* 1)
Warning: *foo* not declared dynamic and thus is not dynamically rebindable, but its name suggests otherwise. Please either indicate ^:dynamic *foo* or change the name. (NO_SOURCE_PATH:1)
#'user/*foo*

While this warning message is associated with a potentially breaking change (CLJ-752), the warning is still arguably useful in ClojureScript.



 Comments   
Comment by Mike Fikes [ 12/Jul/18 7:27 PM ]

2nd patch also excludes cljs.js/*loaded*.

Comment by Mike Fikes [ 12/Jul/18 8:34 PM ]

Sorry for the churn... there is clojure.browser.net/*timeout* also triggers the warning. The third patch excludes any such var in a namespace starting with cljs. or clojure. (except for cljs.user).





[CLJS-2821] Update doto docstring to not use Java example Created: 14/Jul/18  Updated: 14/Jul/18

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

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

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

 Description   

doto's docstring employs java.util.HashMap.



 Comments   
Comment by Mike Fikes [ 14/Jul/18 2:26 PM ]

This would normally just be a simple textual docstring update, but since it the docstring comes from Clojure for JVM ClojureScript, it involves defining the macro in that case in order to define its docstring.





[CLJS-2827] Avoid var special in core macros for private var access Created: 15/Jul/18  Updated: 16/Jul/18

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-2827.patch    
Patch: Code

 Description   

There are two places where macros in cljs.core were revised to use var in order to access private vars in cljs.core without triggering the private var warning introduced with CLJS-1702.

In order to avoid the potential emitted code bloat that might occur (these var uses are in the defrecord and defaulti macros), just use JavaScript interop to directly access the var values in question.



 Comments   
Comment by Mike Fikes [ 15/Jul/18 11:34 PM ]

Instead of using js/-style interop, the attached patch uses js*. This is because if js/ is used, then if externs inference is enabled, this results in cljs.core, and the two properties being accessed in cljs.core being added to the automatically inferred externs.

Comment by Thomas Heller [ 16/Jul/18 5:59 AM ]

This looks like a hack to me. Why not just make the vars public instead?





Generated at Mon Jul 16 09:35:13 CDT 2018 using JIRA 4.4#649-r158309.