<< Back to previous view

[CLJS-2351] Setting :arglists metadata when vararg is present Created: 08/Sep/17  Updated: 11/Dec/17

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

Type: Defect Priority: Minor
Reporter: Hlöðver Sigurðsson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: vars
Environment:

I'm running cljs 1.9.909 with figwheel and lumo 1.7, bug is present in both environments.


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

 Description   

consider function with no parameters

(defn aarg {:arglists '([fake])} [])
=> {:ns cljs.user,
 :name aarg,
 :file nil,
 :end-column 11,
 :column 1,
 :line 1,
 :end-line 1,
 :arglists ([fake]),
 :doc nil,
 :test nil}

All works as expected, but with the introduction of a vararg

(defn aarg {:arglists '([fake])} [& env])
(meta #'aarg)
=> {:ns cljs.user,
 :name aarg,
 :file nil,
 :end-column 11,
 :top-fn {:variadic true,
          :max-fixed-arity 0,
          :method-params [(env)],
          :arglists ([& env]),
          :arglists-meta (nil)},
 :column 1,
 :line 1,
 :end-line 1,
 :arglists ([& env]),
 :doc nil,
 :test nil}

:arglists does not get affected.



 Comments   
Comment by Hlöðver Sigurðsson [ 09/Dec/17 8:54 PM ]

I just submitted a patch, never used jira patch system before, that aside, the patch will make the compiler respect :arglists metadata when the user provdes one. Hope to get feedback and merge on this

Comment by David Nolen [ 11/Dec/17 8:53 AM ]

Thanks have you submitted your Clojure CA?

Comment by David Nolen [ 11/Dec/17 8:56 AM ]

Patch review, I don't really understand the purpose the changing arity of variadic-fn and adding this new flag. Just put the logic directly into variadic-fn no?

Comment by Hlöðver Sigurðsson [ 11/Dec/17 9:35 AM ]

Yes, I'll find a better solution to my patch, just that the `m` symbol in the let binding on line 3176

m (conj {:arglists (core/list 'quote (sigs fdecl))} m)

Merges the arglists and there's after this point no way to know if the :arglists came from a provided metadata or not. But I'll rename the let symbols and it's easy to avoid adding arity, I'll do another patch later today.

No haven't submitted my Clojure Contibutor Agreement, I'm filling it out now...





[CLJS-2438] Self-host: script/test-self-parity abends sometimes Created: 08/Dec/17  Updated: 08/Dec/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Mike Fikes
Resolution: Unresolved Votes: 0
Labels: bootstrap


 Description   

Sometimes the script/test-self-parity tests terminate early with

Testing with Node

Testing self-host.test
WARNING: double already refers to: cljs.core/double being replaced by: cljs.user/double at line 1

Ran 33 tests containing 206 assertions.
0 failures, 0 errors.
x

Full transcript:

$ script/test-self-host
Analyzing file:/Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/core.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/core.cljs, elapsed time: 2548.700649 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/inspect.cljs to builds/out-self/cljs/tools/reader/impl/inspect.cljs
Compiling builds/out-self/cljs/tools/reader/impl/inspect.cljs, elapsed time: 32.942144 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/clojure/string.cljs, elapsed time: 67.24495 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/utils.cljs to builds/out-self/cljs/tools/reader/impl/utils.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/env.cljc, elapsed time: 7.9361 msecs
Compiling builds/out-self/cljs/tools/reader/impl/utils.cljs, elapsed time: 52.978771 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/reader_types.cljs to builds/out-self/cljs/tools/reader/reader_types.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/clojure/set.cljs, elapsed time: 63.896603 msecs
Compiling builds/out-self/cljs/tools/reader/reader_types.cljs, elapsed time: 65.685336 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/errors.cljs to builds/out-self/cljs/tools/reader/impl/errors.cljs
Compiling builds/out-self/cljs/tools/reader/impl/errors.cljs, elapsed time: 47.949386 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/commons.cljs to builds/out-self/cljs/tools/reader/impl/commons.cljs
Compiling builds/out-self/cljs/tools/reader/impl/commons.cljs, elapsed time: 26.952248 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader.cljs to builds/out-self/cljs/tools/reader.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/source_map/base64.cljs, elapsed time: 4.063721 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/source_map/base64_vlq.cljs, elapsed time: 15.036982 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/source_map.cljs, elapsed time: 125.606578 msecs
Compiling builds/out-self/cljs/tools/reader.cljs, elapsed time: 213.744986 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/edn.cljs to builds/out-self/cljs/tools/reader/edn.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/clojure/walk.cljs, elapsed time: 15.457761 msecs
Compiling builds/out-self/cljs/tools/reader/edn.cljs, elapsed time: 87.207366 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/reader.cljs, elapsed time: 69.046241 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/tagged_literals.cljc, elapsed time: 11.240989 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/spec/gen/alpha.cljs, elapsed time: 260.765109 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/spec/alpha.cljs, elapsed time: 947.753769 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/analyzer.cljc, elapsed time: 1202.162467 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/nodejs.cljs, elapsed time: 3.422987 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/pprint.cljs, elapsed time: 1300.527383 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/compiler.cljc, elapsed time: 403.391288 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/test.cljs, elapsed time: 143.651189 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/js.cljs, elapsed time: 1554.368126 msecs
Compiling src/test/self/self_host/test.cljs, elapsed time: 1493.203925 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/core.cljc, elapsed time: 7060.124722 msecs
Compile sources, elapsed time: 14324.531439 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/base.js to builds/out-self/goog/base.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/string/string.js to builds/out-self/goog/string/string.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/debug/error.js to builds/out-self/goog/debug/error.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/dom/nodetype.js to builds/out-self/goog/dom/nodetype.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/asserts/asserts.js to builds/out-self/goog/asserts/asserts.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/array/array.js to builds/out-self/goog/array/array.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/object/object.js to builds/out-self/goog/object/object.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/structs/structs.js to builds/out-self/goog/structs/structs.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/functions/functions.js to builds/out-self/goog/functions/functions.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/math/math.js to builds/out-self/goog/math/math.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/iter/iter.js to builds/out-self/goog/iter/iter.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/structs/map.js to builds/out-self/goog/structs/map.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/uri/utils.js to builds/out-self/goog/uri/utils.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/uri/uri.js to builds/out-self/goog/uri/uri.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/math/integer.js to builds/out-self/goog/math/integer.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/string/stringbuffer.js to builds/out-self/goog/string/stringbuffer.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/reflect/reflect.js to builds/out-self/goog/reflect/reflect.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/math/long.js to builds/out-self/goog/math/long.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/util.js to builds/out-self/goog/labs/useragent/util.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/platform.js to builds/out-self/goog/labs/useragent/platform.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/engine.js to builds/out-self/goog/labs/useragent/engine.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/crypt/crypt.js to builds/out-self/goog/crypt/crypt.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/browser.js to builds/out-self/goog/labs/useragent/browser.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/useragent/useragent.js to builds/out-self/goog/useragent/useragent.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/useragent/product.js to builds/out-self/goog/useragent/product.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/crypt/base64.js to builds/out-self/goog/crypt/base64.js
Applying optimizations :simple to 55 sources
Optimizing with Google Closure Compiler, elapsed time: 17638.02089 msecs
Optimizing 55 sources, elapsed time: 18894.849293 msecs
Testing with Node

Testing self-host.test
WARNING: double already refers to: cljs.core/double being replaced by: cljs.user/double at line 1

Ran 33 tests containing 206 assertions.
0 failures, 0 errors.
x





[CLJS-2407] Add docstrings for map and positional factory functions Created: 21/Nov/17  Updated: 08/Dec/17  Resolved: 08/Dec/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File CLJS-2407.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Imitate the docstring behavior seen in Clojure for defrecord and deftype synthetically generated constructor functions:

user=> (defrecord Foo [x y])
user.Foo
user=> (doc map->Foo)
-------------------------
user/map->Foo
([m__7585__auto__])
  Factory function for class user.Foo, taking a map of keywords to field values.
nil
user=> (doc ->Foo)
-------------------------
user/->Foo
([x y])
  Positional factory function for class user.Foo.
nil
user=> (deftype Bar [x])
user.Bar
user=> (doc ->Bar)
-------------------------
user/->Bar
([x])
  Positional factory function for class user.Bar.
nil


 Comments   
Comment by Mike Fikes [ 21/Nov/17 8:46 AM ]

With the attached patch:

cljs.user=> (defrecord Foo [x y])
cljs.user/Foo
cljs.user=> (doc map->Foo)
-------------------------
cljs.user/map->Foo
([G__19108])
  Factory function for cljs.user/Foo, taking a map of keywords to field values.

nil
cljs.user=> (doc ->Foo)
-------------------------
cljs.user/->Foo
([x y])
  Positional factory function for cljs.user/Foo.

nil
cljs.user=> (deftype Bar [x])
cljs.user/Bar
cljs.user=> (doc ->Bar)
-------------------------
cljs.user/->Bar
([x])
  Positional factory function for cljs.user/Bar.

nil
Comment by Mike Fikes [ 21/Nov/17 9:31 AM ]

As a measure of the effect on code size, builds/out-adv/core-advanced-test.js increases by 5825 bytes uncompressed, 1028 bytes gzipped.

If this deemed unacceptable, perhaps we could consider a way to have this work only while in the REPL.

Comment by Mike Fikes [ 21/Nov/17 12:55 PM ]

After digging deeper into this, the increase in generated code size is exactly attributable to the added tests that are in the patch. (In particular, the use of var in the tests contributes a decent amount to the code size.)

So, in short, applying the patch would give us the desired docstrings without increasing production artifact size.

Comment by David Nolen [ 08/Dec/17 3:36 PM ]

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





[CLJS-2410] Side-effectful assignments are wrongly eliminated by GCC under :advanced Created: 22/Nov/17  Updated: 08/Dec/17

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

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

CLJS 1.9.946



 Description   

DEMO REPO: https://github.com/tonsky/closure-elimination-bug

Given this code:

(ns bug.core
  (:require
    [goog.object :as gobj]))


(defn get-obj []
  (js/document.querySelector "body"))


(defn ^:export aaa []
  (js/console.log "aaa start")
  (gobj/set (get-obj) "className" "+++aaa+++")
  (js/console.log "aaa end"))


(defn ^:export bbb []
  (js/console.log "bbb start")
  (set! (.-className (get-obj)) "+++bbb+++")
  (js/console.log "bbb end"))


(defn ^:export ccc []
  (js/console.log "ccc start")
  (aset (get-obj) "className" "+++ccc+++")
  (js/console.log "ccc end"))

gets compiled to

// Compiled by ClojureScript 1.9.946 {:static-fns true, :optimize-constants true}
goog.provide('bug.core');
goog.require('cljs.core');
goog.require('cljs.core.constants');
goog.require('goog.object');
bug.core.get_obj = (function bug$core$get_obj(){
return document.querySelector("body");
});
bug.core.aaa = (function bug$core$aaa(){
console.log("aaa start");

var G__4263_4266 = bug.core.get_obj();
var G__4264_4267 = "className";
var G__4265_4268 = "+++aaa+++";
goog.object.set(G__4263_4266,G__4264_4267,G__4265_4268);

return console.log("aaa end");
});
goog.exportSymbol('bug.core.aaa', bug.core.aaa);
bug.core.bbb = (function bug$core$bbb(){
console.log("bbb\u00A0start");

bug.core.get_obj().className = "+++bbb+++";

return console.log("bbb end");
});
goog.exportSymbol('bug.core.bbb', bug.core.bbb);
bug.core.ccc = (function bug$core$ccc(){
console.log("ccc start");

(bug.core.get_obj()["className"] = "+++ccc+++");

return console.log("ccc end");
});
goog.exportSymbol('bug.core.ccc', bug.core.ccc);

(fine so far). When we compile that with :optimizations :advanced, assignments are lost:

l("bug.core.aaa", function() {
  console.log("aaa start");
  return console.log("aaa end");
});
l("bug.core.bbb", function() {
  console.log("bbb start");
  return console.log("bbb end");
});
l("bug.core.ccc", function() {
  console.log("ccc start");
  return console.log("ccc end");
});

Expected: assignments are not eliminated.

Might be a GCC bug, but I can't build a minimal use-case to reproduce. Upgrading to [com.google.javascript/closure-compiler-unshaded "v20171023"] did not helped (same behaviour).



 Comments   
Comment by A. R [ 22/Nov/17 8:23 AM ]

Duplicate: CLJS-2031

TLDR: Add `:closure-warnings {:check-types :warning}` to your compiler options fixes this (which I've had on all my projects since that ticket)





[CLJS-2437] Update docstrings for str/replace and str/replace-first from Clojure Created: 08/Dec/17  Updated: 08/Dec/17  Resolved: 08/Dec/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: docstring

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

 Description   

Update the docstrings for clojure.string/replace and clojure.string/replace-first with relevant copy from Clojure's docstrings.



 Comments   
Comment by Mike Fikes [ 08/Dec/17 8:31 AM ]

With the patch:

cljs.user=> (doc clojure.string/replace)
-------------------------
clojure.string/replace
([s match replacement])
  Replaces all instance of match with replacement in s.

   match/replacement can be:

   string / string
   pattern / (string or function of match).

   See also replace-first.

   The replacement is literal (i.e. none of its characters are treated
   specially) for all cases above except pattern / string.

   For pattern / string, $1, $2, etc. in the replacement string are
   substituted with the string that matched the corresponding
   parenthesized group in the pattern.

   Example:
   (clojure.string/replace "Almost Pig Latin" #"\b(\w)(\w+)\b" "$2$1ay")
   -> "lmostAay igPay atinLay"

nil
cljs.user=> (doc clojure.string/replace-first)
-------------------------
clojure.string/replace-first
([s match replacement])
  Replaces the first instance of match with replacement in s.

   match/replacement can be:

   string / string
   pattern / (string or function of match).

   See also replace.

   The replacement is literal (i.e. none of its characters are treated
   specially) for all cases above except pattern / string.

   For pattern / string, $1, $2, etc. in the replacement string are
   substituted with the string that matched the corresponding
   parenthesized group in the pattern.

   Example:
   (clojure.string/replace-first "swap first two words"
                                 #"(\w+)(\s+)(\w+)" "$3$2$1")
   -> "first swap two words"

nil
Comment by David Nolen [ 08/Dec/17 3:25 PM ]

fixed https://github.com/clojure/clojurescript/commit/49f98a2cb03bc9f6d74b91226a0d14cb8dbd63ba





[CLJS-2411] Avoid using js/global Created: 22/Nov/17  Updated: 08/Dec/17  Resolved: 08/Dec/17

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

Type: Defect Priority: Minor
Reporter: Paulus Esterhazy Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Avoid-js-global.patch    
Patch: Code
Approval: Accepted

 Description   

When compiling ClojureScript's compiler output using GCC from the command line, GCC exits with an error, complaining that

```
out/cljs/core.js:35941: ERROR - variable global is undeclared
return cljs.core.find_ns_obj_STAR_(global,segs);
^^^^^^
```

The CLI command used:

```
java -cp cljs-1.9.946.jar com.google.javascript.jscomp.CommandLineRunner --compilation_level ADVANCED --js out/'**.js' --manage_closure_dependencies --closure_entry_point bug.core --closure_entry_point process.env --js_output_file advanced.js
```



 Comments   
Comment by David Nolen [ 08/Dec/17 3:24 PM ]

fixed https://github.com/clojure/clojurescript/commit/6362dd09e124f2445ada266915e510311a3924c9





[CLJS-2430] Foreign libs with Node target Created: 04/Dec/17  Updated: 08/Dec/17  Resolved: 08/Dec/17

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

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

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

 Description   

Before 1.9.844 (https://github.com/clojure/clojurescript/commit/fc0989f1b44b97547410a2d2c807f16430b47486#diff-db201d3634488851510ee2fbce327924) it was possible to use foreign libs with Node target. Now this fails with exception about Node require not being able to find the foreign library:

module.js:538
    throw err;
    ^

Error: Cannot find module 'react'
    at Function.Module._resolveFilename (module.js:536:15)
    at Function.Module._load (module.js:466:25)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at webpackUniversalModuleDefinition (/home/juho/Source/reagent/test-environments/node-foreign-libs/target/cljsbuild/node-test/out/cljsjs/create-react-class/development/create-react-class.inc.js:3:28)
    at Object.<anonymous> (/home/juho/Source/reagent/test-environments/node-foreign-libs/target/cljsbuild/node-test/out/cljsjs/create-react-class/development/create-react-class.inc.js:10:3)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)

Foreign libraries need to be loaded using nodeGlobalRequire instead of Node require.
To allow bootstrap script to choose the method, we can use Closure loadFlags.



 Comments   
Comment by Juho Teperi [ 04/Dec/17 7:34 AM ]

Attached patch. Tested with Reagent, works on Node with both Node Modules and Foreign libs.

Comment by Juho Teperi [ 04/Dec/17 8:17 AM ]
@param {boolean|!Object<string>=} opt_loadFlags Parameters indicating
how the file must be loaded.  The boolean 'true' is equivalent
to \{'module': 'goog'} for backwards-compatibility.  Valid properties
and values include {'module': 'goog'} and {'lang': 'es6'}.

So Closure-lib docstring about loadFlags sounds about right for this use case. They mention "valid properties", but as far as I can see, there isn't validation for these, they just read lang and module properties themselves.

Comment by David Nolen [ 08/Dec/17 3:13 PM ]

fixed https://github.com/clojure/clojurescript/commit/53070a2c5392e8bdc00bb81c45034268e1cd81f2





[CLJS-2389] Fix module processing after latest Closure changes Created: 27/Oct/17  Updated: 06/Dec/17

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

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

Attachments: Text File CLJS-2389-wip.patch    

 Description   

New Closure-compiler doesn't add goog.provide/require calls to processed modules:

https://github.com/google/closure-compiler/pull/2641

> ES6 and CommonJS modules no longer generate synthetic goog.require and goog.provide calls.

Currently {process-js-modules} uses {load-library}, which reads the goog.provide calls in the file, to determine the name for processed module, something like {module$absolute$path}.
Now that the file doesn't have goog.provide call, this breaks.

As the module name is based on file-path, we can determine the module name directly from filepath, Closure provides utility for this: {ModuleNames/fileToModuleName}.



 Comments   
Comment by Juho Teperi [ 27/Oct/17 5:53 AM ]

Attached patch fixes the first problem, which is updating {:js-module-index} mapping from Node name to processed module name.

Another problem is that analyzer/check-uses now fails when referring to symbols in processed modules:

ERROR in (test-module-name-substitution) (core.clj:4617)
expected: (= (compile (quote (ns my-calculator.core (:require [calculator :as calc :refer [subtract add] :rename {subtract sub}])))) (str "goog.provide('my_calculator.core');" crlf "goog.require('cljs.core');" crlf "goog.require('" (absolute-module-path "
src/test/cljs/calculator.js" true) "');" crlf))
  actual: clojure.lang.ExceptionInfo: Invalid :refer, var module$home$juho$Source$clojurescript$src$test$cljs$calculator/add does not exist

For some reason {missing-use?} doesn't work correctly.

Comment by Juho Teperi [ 06/Dec/17 12:47 PM ]

I'm looking into emitting those goof.provide/require calls from Cljs. I have provide working already and that fixes analyzer etc. But we also need require so that cljs_deps.js gets proper information for browser, but I haven't yet found a way to retrieve module requires from Closure.





[CLJS-2436] Print compiler options when running with :verbose true Created: 06/Dec/17  Updated: 06/Dec/17

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

Type: Enhancement Priority: Trivial
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie


 Description   

For the purpose of comparing different build tools it would be useful to easily see what compiler options are used.

David suggested in Slack that these could be printed when `:verbose true` has been supplied.






[CLJS-2362] Make node REPL debuggable/inspectable again Created: 15/Sep/17  Updated: 05/Dec/17  Resolved: 05/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: Andrea Richiardi Assignee: Andrea Richiardi
Resolution: Completed Votes: 0
Labels: nodejs

Attachments: File cljs-2362.diff    
Approval: Screened

 Description   

Currently, when trying to enable debug mode a warning is thrown:

Clojure 1.9.0-alpha17
user=> (require '[cljs.repl :as repl])
nil
user=> (require '[cljs.repl.node :as node])         
nil
user=> (repl/repl (node/repl-env :debug-port 5002))
(node:2152) [DEP0062] DeprecationWarning: `node --debug` and `node --debug-brk` are invalid. Please use `node --inspect` or `node --inspect-brk` instead.

I can take care of this easy patch.



 Comments   
Comment by Andrea Richiardi [ 15/Sep/17 12:24 AM ]

Patch attached, a bit rusty , hopefully all good

Comment by David Nolen [ 15/Sep/17 7:36 AM ]

Is this going to work for older version of Node?

Comment by Andrea Richiardi [ 15/Sep/17 1:46 PM ]

Should work for node >= 6.3, do you want me to branch on the version?

Comment by Thomas Heller [ 15/Sep/17 2:42 PM ]

Given that CLJS itself does not use this info and just passes it along it might be a better solution to provide a generic option that takes a seq of strings to use when launching the node process.

In shadow-cljs I have

(shadow/node-repl {:node-args ["--inspect" "--inspect-brk" "--whatever-you-need"]})

This is more flexible and doesn't have to deal with versioning issues since it is left to the user.

Comment by Andrea Richiardi [ 17/Sep/17 1:59 PM ]

I like Thomas' approach way better, branching on the version would need to execute node version,not really pretty.

Many boot tasks have a :options key for that (converting {:key} to --key). Thomas version is even simpler, the only difficulty would be to avoid breaking :debug-port as it is now.

Comment by David Nolen [ 05/Dec/17 1:30 PM ]

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





[CLJS-2433] HTTP server in cljs.browser.repl doesn't serve files with extensions other than specified in ext->mime-type Created: 04/Dec/17  Updated: 05/Dec/17

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

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

Attachments: Text File 0001-CLJS-2433-fallback-to-send-static-use-host-to-determ.patch     Text File 0002-CLJ-2433-send-fils-via-send-and-close.patch    

 Description   
$ cat deps.edn
{:deps {org.clojure/clojure {:mvn/version "1.9.0-RC2"}
        org.clojure/clojurescript {:mvn/version "1.9.946"}}
$ touch hello.xml
$ clojure -m cljs.browser.repl &
$ curl localhost:9000/hello.xml
<html><body><h2>Page not found</h2>No page /hello.xml found on this server.</body></html>

Adding an entry to cljs.repl.browser/ext->mime-type superficially fixes that.

1) Currently used MIME type identification is very simplistic and doesn't return correct MIME types for most files. Instead, this should be delegated to the host:

(ns cljs.repl.browser
  (:import [java.nio.file Files Paths]))

(defn get-content-type [path]
  (Files/probeContentType (Paths/get path (into-array String []))))

2) Since both files and sockets are abstractions over bytes, cljs.repl.server/send-and-close should be agnostic to file encoding and just send whatever is in the file. Currently this is handled via cljs.repl.browser/mime-type->encoding.

I will prepare a patch by tomorrow.






[CLJS-2435] Don't warn if arithmetic operations done on ^int Created: 05/Dec/17  Updated: 05/Dec/17

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

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


 Description   

If CLJC is being used and the ClojureScript compiler sees Clojure type annotations, it should effecively ignore those where there isn't a portability concern.

The specific case for this ticket is ^int and arithmetic, and a motivating example is:

cljs.user=> (let [^int v (alength (into-array []))] (+ v 3))
WARNING: cljs.core/+, all arguments must be numbers, got [int number] instead. at line 1 <cljs repl>
3

On the other hand, ^long should still cause warnings to be emitted given that it is not a "subtype" of ^number (such code might overflow, so warnings are helpful in that case).






[CLJS-2434] Update Closure-compiler Created: 05/Dec/17  Updated: 05/Dec/17  Resolved: 05/Dec/17

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

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


 Description   

After v20170910 Closure-compiler doesn't emit goog.provide or goog.require calls to the processed modules, process-js-modules currently parses module dependency information (generated module name) from those calls in the processed file so this will need a change.

Other minor changes might be needed.



 Comments   
Comment by Juho Teperi [ 05/Dec/17 7:11 AM ]

Duplicate of https://dev.clojure.org/jira/browse/CLJS-2389





[CLJS-1328] Support defrecord reader tags Created: 04/Jul/15  Updated: 04/Dec/17

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

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reader, readertags


 Description   

Currently, defrecord instances print similar to how they do in clojure

> (pr-str (garden.units/px 5))
#garden.types.CSSUnit{:unit :px, :magnitude 5}

This representation cannot be read by the compiler, nor at runtime by cljs.reader/read-string

> #garden.types.CSSUnit{:unit :px, :magnitude 5}
clojure.lang.ExceptionInfo: garden.types.CSSUnit {:type :reader-exception, :line 1, :column 22, :file "NO_SOURCE_FILE"}
...
> (cljs.reader/read-string "#garden.types.CSSUnit{:unit :px, :magnitude 5}")
#<Error: Could not find tag parser for garden.types.CSSUnit in ("inst" "uuid" "queue" "js")>
...

Analysis

The two requirements - using record literals in cljs source code and supporting runtime reading - can be addressed by using the analyzer to find defrecords and registering them with the two respective reader libraries.

Record literals

Since clojurescript reads and compiles a file at a time, clojure's behavior for literals is hard to exactly mimic. That is, to be able to use the literal in the same file where the record is defined.
A reasonable compromise might be to update the record tag table after each file has been analyzed. Thus the literal form of a record could be used only in requiring files.

EDIT: Record literals can also go into the constant pool

cljs.reader

To play well with minification, the ^:export annotation could be reused on defrecords, to publish the corresponding reader tag to cljs.reader.

Related Tickets



 Comments   
Comment by David Nolen [ 08/Jul/15 12:00 PM ]

It's preferred that we avoid exporting. Instead we can adopt the same approach as the constant literal optimization for keywords under advanced optimizations. We can make a lookup table (which won't pollute the global namespace like exporting does) which maps a string to its type.

I'm all for this enhancement.

Comment by Levi R. I. Tan Ong [ 04/Dec/17 11:23 PM ]

Trying my hand at this, but I'm stuck at an architecture conundrum.

Should the analyzer be able to add a tag reader to the tag table, or should the reader take a look at a the record-table to build tag readers on the fly?

The former seems easier to do with a macro, but it feels wrong to write reader code in the analyzer namespace.
The latter is more tedious because all the reader functions deref the tag reader table, so I'd have to go through all of them, but it feels less like a war crime than the former.

Or is there another option I'm not seeing?





[CLJS-2431] complement is tagged with boolean return type Created: 04/Dec/17  Updated: 04/Dec/17

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

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

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

 Description   

cljs.core/complement is has ^boolean meta. (It was inadvertently tagged when core predicates were being tagged.)






[CLJS-2432] docstring for cli->js should describe :keyword-fn Created: 04/Dec/17  Updated: 04/Dec/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, newbie


 Description   

CLJS-2215 added support in cljs.core/clj->js for a new :keyword-fn parameter. This feature should be described in the docstring.






[CLJS-2363] Warn user when a namespace requires itself Created: 16/Sep/17  Updated: 04/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: Christian Fortin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When a namespace requires itself, the compilation hangs without an error message.
It would be nice to either throw an error, or at least print a warning message to the user.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 3:28 PM ]

Cannot repro with 1.9.908 nor 1.9.946:

src/itself/core.cljs:

(ns itself.core
  (:require itself.core))
$ java -cp cljs.jar:src clojure.main -m cljs.repl.node
ClojureScript Node.js REPL server listening on 54877
To quit, type: :cljs/quit
cljs.user=> (require 'itself.core)
clojure.lang.ExceptionInfo: Assert failed: Circular dependency detected, cljs.user -> itself.core -> itself.core
Comment by Christian Fortin [ 04/Dec/17 11:21 AM ]

I still get the same behavior with 1.9.946.
Tried with figwheel and uberjar.
Both hang without an error message.

Could it be a problem with the toolchain, say Leiningen?

Comment by Mike Fikes [ 04/Dec/17 11:25 AM ]

Yes. With Figwheel, I'd try disabling :autoload (see https://github.com/bhauman/lein-figwheel/wiki/Configuration-Options#client)

If that allows you to isolate it to Figwheel, perhaps this JIRA can be closed.





[CLJS-2429] Self-host: Printing defrecord form AST memory Created: 03/Dec/17  Updated: 04/Dec/17  Resolved: 04/Dec/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bootstrap, performance


 Description   

If printing the AST associated with analyzing a defrecord form with the cljs.js API, memory use is excessive, with things ultimately failing.

Minimal repro:

$ java -jar cljs.jar -m cljs.repl.node
ClojureScript Node.js REPL server listening on 58683
To quit, type: :cljs/quit
cljs.user=> (require 'cljs.js)
nil
cljs.user=> (def ast (cljs.js/analyze-str (cljs.js/empty-state) "(defrecord Foo [])" nil {:eval cljs.js/js-eval :context :expr} identity))
#'cljs.user/ast
cljs.user=> ast

<--- Last few GCs --->

[94684:0x104800000]   108358 ms: Mark-sweep 1402.6 (1459.8) -> 1402.6 (1432.3) MB, 1534.5 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 1535 ms) last resort
[94684:0x104800000]   110009 ms: Mark-sweep 1402.6 (1432.3) -> 1402.6 (1432.3) MB, 1651.0 / 0.0 ms  last resort


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x2babe789cca1 <JSObject>
    1: pr_writer_impl(aka cljs$core$pr_writer_impl) [/Users/mfikes/Downloads/out/.cljs_node_repl/cljs/core.js:~31695] [pc=0x32b9750e856e](this=0x16be3f03b2b1 <Object map = 0x376490efaee9>,obj=1,writer=0x25fbdb95a329 <JSObject>,opts=0x25fbdb95a2e1 <JSObject>)
    2: cljs$core$pr_writer [/Users/mfikes/Downloads/out/.cljs_node_repl/cljs/core.js:~31824] [pc=0x32b9750e4a27](this=0x7540a2892a9 <JSGlob...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
 1: node::Abort() [/usr/local/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/usr/local/bin/node]
 3: v8::Utils::ReportOOMFailure(char const*, bool) [/usr/local/bin/node]
 4: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [/usr/local/bin/node]
 5: v8::internal::Factory::NewFixedArray(int, v8::internal::PretenureFlag) [/usr/local/bin/node]
 6: v8::internal::LoadIC::LoadFullChain(v8::internal::Handle<v8::internal::Map>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Name>, v8::internal::Handle<v8::internal::Smi>) [/usr/local/bin/node]
 7: v8::internal::LoadIC::UpdateCaches(v8::internal::LookupIterator*) [/usr/local/bin/node]
 8: v8::internal::LoadIC::Load(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Name>) [/usr/local/bin/node]
 9: v8::internal::Runtime_LoadIC_Miss(int, v8::internal::Object**, v8::internal::Isolate*) [/usr/local/bin/node]
10: 0x32b974f840dd
java.lang.IllegalArgumentException: Value out of range for char: -1
	at clojure.lang.RT.charCast(RT.java:1052)
	at cljs.repl.node$read_response.invokeStatic(node.clj:49)
	at cljs.repl.node$node_eval.invokeStatic(node.clj:55)
	at cljs.repl.node.NodeEnv._evaluate(node.clj:199)
	at cljs.repl$evaluate_form.invokeStatic(repl.cljc:518)
	at cljs.repl$eval_cljs.invokeStatic(repl.cljc:631)
	at cljs.repl$eval_cljs.invoke(repl.cljc:618)
	at cljs.repl$repl_STAR_$read_eval_print__6323.invoke(repl.cljc:880)
	at cljs.repl$repl_STAR_$fn__6331$fn__6340.invoke(repl.cljc:925)
	at cljs.repl$repl_STAR_$fn__6331.invoke(repl.cljc:924)
	at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:1271)
	at cljs.repl$repl_STAR_.invokeStatic(repl.cljc:882)
	at cljs.repl$repl.invokeStatic(repl.cljc:1004)
	at cljs.repl$repl.doInvoke(repl.cljc:936)
	at clojure.lang.RestFn.invoke(RestFn.java:410)
	at cljs.repl.node$_main.invokeStatic(node.clj:234)
	at cljs.repl.node$_main.invoke(node.clj:233)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.core$apply.invokeStatic(core.clj:646)
	at clojure.main$main_opt.invokeStatic(main.clj:314)
	at clojure.main$main_opt.invoke(main.clj:310)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)


 Comments   
Comment by Mike Fikes [ 04/Dec/17 6:26 AM ]

I think this isn't really a self-host issue, but just the fact that this is an extraordinarily large value to attempt to print when in a ClojureScript REPL (without using a streaming print approach like Fipp or some such).

You can work around it via:

(set! *print-level* 10)

Retracting this ticket.





[CLJS-2428] source fails on Vars whose source has ns-aliased keywords Created: 02/Dec/17  Updated: 02/Dec/17

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

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

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

 Description   

The source REPL macro fails on Vars whose source has ns-aliased keywords. (This is similar to CLJS-1488, but for the case when the source contains something like ::some-alias/foo and some-alias cannot be resolved.)

Concrete example:

cljs.user=> (source resolve)
clojure.lang.ExceptionInfo: [line 3269, col 68] Invalid keyword: ::ana/no-resolve. at line 1 <cljs repl> {:file "<cljs repl>", :line 1, :column 1, :root-source-info {:source-type :fragment, :source-form (source resolve)}, :tag :cljs/analysis-error}
...


 Comments   
Comment by Mike Fikes [ 02/Dec/17 7:45 PM ]

The implementation of source uses read to read the source form, and when doing so, it trips up when trying to resolve ns-aliased keywords. We actually don't need the resolution to succeed in any meaningful way because, in the end, we discard the form read, and instead use the :source meta produced by the source-logging-pushback-reader. Since we don't really know what the aliases are at this point in the code, the issue can be worked around by using an identity mapping for the alias map. That's what the attached patch does.





[CLJS-2427] docstring for clojure.string/split-lines needs escaping Created: 02/Dec/17  Updated: 02/Dec/17

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

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

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

 Description   
cljs.user=> (doc clojure.string/split-lines)
-------------------------
clojure.string/split-lines
([s])
  Splits s on
 or
.

nil


 Comments   
Comment by Mike Fikes [ 02/Dec/17 1:26 PM ]

With patch:

cljs.user=> (doc clojure.string/split-lines)
-------------------------
clojure.string/split-lines
([s])
  Splits s on \n or \r\n.

nil




[CLJS-2414] Self-host: Macro specs are instrumented Created: 24/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

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

 Description   

If you use cljs.spec.alpha/fdef to define a macro spec, self-host will apply that spec as expected during macroexpansion time. But another unintended consequence is that cljs.spec.test.alpha/instrument will unnecessarily instrument this spec.



 Comments   
Comment by Mike Fikes [ 24/Nov/17 3:24 PM ]

The attached patch adds some self-host-conditional code that checks that a Var is not a macro before instrumenting it.
The patch also adds a test. Things pass if applied against master, but for the test to actually be executed in the self-hosted suite, CLJS-2415 will need to be applied.

Comment by David Nolen [ 01/Dec/17 3:40 PM ]

Looks like this one needs a rebase.

Comment by Mike Fikes [ 01/Dec/17 6:50 PM ]

Attaching re-baselined CLJS-2414-2.patch

Comment by David Nolen [ 01/Dec/17 7:03 PM ]

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





[CLJS-2426] script/bootstrap fails Created: 01/Dec/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

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

 Description   
$ script/bootstrap
Fetching Clojure...
Copying clojure-1.9.0-alpha17/clojure-1.9.0-alpha17.jar to lib/clojure.jar...
cp: clojure-1.9.0-alpha17/clojure-1.9.0-alpha17.jar: No such file or directory


 Comments   
Comment by David Nolen [ 01/Dec/17 7:02 PM ]

fixed https://github.com/clojure/clojurescript/commit/566be7ce88145bcd612ecede31d090aa0ca4fe0a





[CLJS-2387] CLJS Analyzer does not correctly detect cache hits for analyzed spec files Created: 19/Oct/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

Type: Defect Priority: Minor
Reporter: Alexander Redington Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: spec
Environment:

OS X


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

 Description   

When Analyzing a cljs file which contains only (cljs.spec.alpha/def ...) forms and no clojure core (def ...) special forms, the Analyzer does not determine the source file to have been analyzed on https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L4021. When this is combined with the :cache-analysis true compiler option, it results in the analysis cache continually being repopulated from disk, and in certain degenerate cases, poor compilation performance.



 Comments   
Comment by Alexander Redington [ 19/Oct/17 1:30 PM ]

Steps to reproduce: have multiple cljs files require one namespace that contains only (s/def ...) forms, such that analysis is examined multiple times for the spec namespace. Run with compiler-options :verbose true and :cache-analysis true, and observe the file is analyzed multiple times and the cache is re-read multiple times.

Workarounds: Add a meaningless (def ...) form to the files which are repeatedly analyzed, and they will suddenly stop being analyzed more than once.

On my local machine doing this with the worst offendeding namespaces dropped compile times from 104 seconds to 34 seconds.

Comment by Mike Fikes [ 20/Nov/17 1:31 PM ]

There are a couple of places in the code that interpret the presence of defs in the analysis cache as implying that the namespace has been analyzed. (One is the spot pointed out by Alex, and the other is https://github.com/clojure/clojurescript/blob/2389e52049a9bd001d173a1cb4772ed8a25de196/src/main/clojure/cljs/analyzer.cljc#L2120 )

The attached patch essentially forces the issue, ensuring that there is at least an empty defs map upon successful analysis. (An alternative approach would be to revise the AST slightly by introducing an explicit :analyzed true key-value.)

Note that, in order to test with the attached patch, you'd need to ensure that the patch in CLJS-2405 is applied as well, as it affects proper cache loading, especially in the case where specs are involved.

Comment by Mike Fikes [ 24/Nov/17 6:35 PM ]

Another somewhat related issue to watch out for, even if this one is fixed: CLJS-2417

Comment by David Nolen [ 01/Dec/17 4:00 PM ]

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





[CLJS-2405] Register dumped specs fails Created: 20/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

Status: Closed
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: Completed Votes: 0
Labels: spec

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

 Description   

When specs that are dumped into analysis cache files are registered, this fails (with a swallowed ClassCastException clojure.lang.PersistentVector cannot be cast to java.util.Map$Entry).

You can see evidence of this if you run script/noderepljs revised to set :verbose true: There is a spec, :cljs.spec.alpha/kvs->map, for which the code attempts to register, throws, with the file subsequently being re-analyzed anyway because the cache couldn't be loaded. The evidence is this being emitted by :verbose true:

Reading analysis cache for file:/Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/spec/alpha.cljs
Analyzing file:/Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/spec/alpha.cljs

At its core, it involves code like {{(merge {5 6} '([1 2] [3 4]))}}, which fails in Clojure.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 11:44 AM ]

The patch revises it so at the bottom it involves a construct like {{(into {1 2, 3 4} '([1 7] [5 6]))}} which works in Clojure.

Comment by David Nolen [ 01/Dec/17 3:50 PM ]

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





[CLJS-2423] Allow custom :output-wrapper function Created: 27/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: Levi R. I. Tan Ong Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: compiler

Attachments: Text File CLJS-2423.patch    
Patch: Code and Test
Approval: Accepted

 Description   

In advanced optimization, `goog.global` is being assigned the value of `this` which will lose context when output-wrapper is used. To fix this, the output-wrapper function should be set to something like `'(function(){%output%}).call(window)'` so that `this` is `window`.



 Comments   
Comment by David Nolen [ 01/Dec/17 3:45 PM ]

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





[CLJS-2415] Self-host: Enable tests with instrument and cljs.spec.test-test Created: 24/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

Status: Closed
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: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-2415.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Background: With CLJS-1873 we moved some tests that were not yet runnable in self-hosted into a test namespace cljs.spec.test-test, and set things up so these tests would run in JVM ClojureScript, but with them commented for self-hosted testing (script/test-self-parity). Since then, the changes in test check (TCHECK-105) have been released, and we are now using the test.check artifact in ClojureScript.

We can now enable testing of the cljs.spec.test-test namespace for self-hosted. The only tricky thing we need to do is to provide an substitute for clojure.core/eval for use in the cljs.spec.test macros namespace (by instrument and unstrument) during self-hosted test execution.



 Comments   
Comment by David Nolen [ 01/Dec/17 3:39 PM ]

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





[CLJS-2416] Self-host: defn macro Var doesn't have :macro true meta Created: 24/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

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

 Description   

Under self host, if you do (meta #'cljs.core$macros/defn) you will see that it doesn't have :macro true. (This differs from defn-, for example.)

A consequence is that downstream self-host tooling may treat this Var differently (for example, (doc defn) in Planck and Lumo fail to indicate that it is a macro.)
Another consequence is that in self-host if you define a spec for defn (CLJS-2413, for example), and then run (cljs.spec.test.alpha/instrument), the guard that is in the patch in CLJS-2414, fails.

These are easily fixed by adding :macro true meta to the Var.



 Comments   
Comment by David Nolen [ 01/Dec/17 3:35 PM ]

fixed https://github.com/clojure/clojurescript/commit/0891643da6edf0be85213c9f719b9586fe2ea336





[CLJS-2418] cljs.analyzer/specials and cljs.core/special-symbol? out of sync Created: 25/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

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

 Description   

cljs.analyzer/specials is missing catch and finally
cljs.core/special-symbol? is missing case*

Recommend syncing them, and adding cross-comments in code for future devs revising them.



 Comments   
Comment by David Nolen [ 01/Dec/17 3:26 PM ]

fixed https://github.com/clojure/clojurescript/commit/32dfa11ab2698d7a60ae294b51d1441546866c94





[CLJS-2422] Remove build dependence on Clojure release zip file Created: 27/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: David Nolen
Resolution: Completed Votes: 2
Labels: build

Attachments: Text File cljs-clojure-jar.patch    
Patch: Code
Approval: Accepted

 Description   

Currently the ClojureScript build depends on the Clojure release zip file, but only uses it to extract the Clojure jar file. As part of changes in 1.9, we are going to stop building and releasing this zip file.

The attached patch updates the build to just directly download and use the jar file instead of the zip file which contains it.

Without this patch, the ClojureScript build will stop working when we remove the release zip.



 Comments   
Comment by David Nolen [ 01/Dec/17 3:24 PM ]

fixed https://github.com/clojure/clojurescript/commit/11265fdf19e7bd33001fd9698a0f86aa46ef9c95





[CLJS-2377] The CLJS compiled uses deprecated modules on Java 9 Created: 03/Oct/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

OSX Java 9


Attachments: Text File 0001-CLJS-2377-Remove-usage-of-java.xml.bind.DatatypeConv.patch     Text File CLJS-2377-2.patch     Text File CLJS-2377-3.patch     Text File CLJS-2377.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Running the shipping REPL fails with Java 9:

$ java -version
java version "9.0.1"
Java(TM) SE Runtime Environment (build 9.0.1+11)
Java HotSpot(TM) 64-Bit Server VM (build 9.0.1+11, mixed mode)
$ java -jar cljs.jar -m cljs.repl.node
Exception in thread "main" java.lang.ExceptionInInitializerError
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForName(RT.java:2177)
	at clojure.lang.RT.loadClassForName(RT.java:2196)
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$load_one.invoke(core.clj:5692)
	at clojure.core$load_lib$fn__5626.invoke(core.clj:5737)
	at clojure.core$load_lib.invokeStatic(core.clj:5736)
	at clojure.core$load_lib.doInvoke(core.clj:5717)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$load_libs.invokeStatic(core.clj:5774)
	at clojure.core$load_libs.doInvoke(core.clj:5758)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$require.invokeStatic(core.clj:5796)
	at cljs.repl.node$loading__5569__auto____6884.invoke(node.clj:9)
	at cljs.repl.node__init.load(Unknown Source)
	at cljs.repl.node__init.<clinit>(Unknown Source)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForName(RT.java:2177)
	at clojure.lang.RT.loadClassForName(RT.java:2196)
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$load_one.invoke(core.clj:5692)
	at clojure.core$load_lib$fn__5626.invoke(core.clj:5737)
	at clojure.core$load_lib.invokeStatic(core.clj:5736)
	at clojure.core$load_lib.doInvoke(core.clj:5717)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$load_libs.invokeStatic(core.clj:5774)
	at clojure.core$load_libs.doInvoke(core.clj:5758)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$require.invokeStatic(core.clj:5796)
	at clojure.main$main_opt.invokeStatic(main.clj:314)
	at clojure.main$main_opt.invoke(main.clj:310)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	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.ClassNotFoundException: javax.xml.bind.DatatypeConverter
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:582)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:185)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:496)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2181)
	at cljs.util$loading__5569__auto____42.invoke(util.cljc:9)
	at cljs.util__init.load(Unknown Source)
	at cljs.util__init.<clinit>(Unknown Source)
	... 58 more

For Quick Start, the very first build command under "Let's build some ClojureScript" fails:

$ java -cp cljs.jar:src clojure.main build.clj
Exception in thread "main" java.lang.ExceptionInInitializerError, compiling:(/Users/mfikes/Desktop/hello_world/build.clj:1:1)
	at clojure.lang.Compiler.load(Compiler.java:7391)
	at clojure.lang.Compiler.loadFile(Compiler.java:7317)
	at clojure.main$load_script.invokeStatic(main.clj:275)
	at clojure.main$script_opt.invokeStatic(main.clj:335)
	at clojure.main$script_opt.invoke(main.clj:330)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.ExceptionInInitializerError
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForName(RT.java:2177)
	at clojure.lang.RT.loadClassForName(RT.java:2196)
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$load_one.invoke(core.clj:5692)
	at clojure.core$load_lib$fn__5626.invoke(core.clj:5737)
	at clojure.core$load_lib.invokeStatic(core.clj:5736)
	at clojure.core$load_lib.doInvoke(core.clj:5717)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$load_libs.invokeStatic(core.clj:5774)
	at clojure.core$load_libs.doInvoke(core.clj:5758)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$require.invokeStatic(core.clj:5796)
	at cljs.build.api$loading__5569__auto____2609.invoke(api.clj:8)
	at cljs.build.api__init.load(Unknown Source)
	at cljs.build.api__init.<clinit>(Unknown Source)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForName(RT.java:2177)
	at clojure.lang.RT.loadClassForName(RT.java:2196)
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$load_one.invoke(core.clj:5692)
	at clojure.core$load_lib$fn__5626.invoke(core.clj:5737)
	at clojure.core$load_lib.invokeStatic(core.clj:5736)
	at clojure.core$load_lib.doInvoke(core.clj:5717)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$load_libs.invokeStatic(core.clj:5774)
	at clojure.core$load_libs.doInvoke(core.clj:5758)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$require.invokeStatic(core.clj:5796)
	at clojure.core$require.doInvoke(core.clj:5796)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval1.invokeStatic(build.clj:1)
	at user$eval1.invoke(build.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6927)
	at clojure.lang.Compiler.load(Compiler.java:7379)
	... 11 more
Caused by: java.lang.ClassNotFoundException: javax.xml.bind.DatatypeConverter
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:582)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:185)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:496)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2181)
	at cljs.util$loading__5569__auto____42.invoke(util.cljc:9)
	at cljs.util__init.load(Unknown Source)
	at cljs.util__init.<clinit>(Unknown Source)
	... 66 more

The javax.xml.bind package was deprecated in Java 9 and moved to a module, which requires adding --add-modules java.xml.bind to your REPL command line. It would be great if clojurescript didn't depend on the deprecated javax.xml.bind package in the first place.



 Comments   
Comment by Thomas Heller [ 04/Oct/17 2:36 AM ]

Since Closure now depends on JDK 8 anyways it should probably use java.util.Base64 instead.

Comment by David Nolen [ 29/Oct/17 8:37 AM ]

I agree with Thomas, let's get an updated patch thanks.

Comment by Nikita Prokopov [ 07/Nov/17 7:43 AM ]

Even URL-safe version of Base64 uses hyphen, underscore and equality sign, and is also case-sensitive. I see content-sha being used for namespace names, method names and file paths. Considering most filesystems are case-insensitive, and CLJS translating both hyphen and underscore to underscore when compiling namespace, this might lead to unwanted collisions. Are we happy with that?

Comment by Thomas Heller [ 07/Nov/17 11:40 AM ]

I might have confused the issue. The patch only fixes cljs.util but cljs.repl is also using DatatypeConverter.

cljs.util however only wants to print a hex string, which the java.util.Base64 class I suggested doesn't do.

So the fix is fine, cljs.repl just should use the Base64 class?

Comment by Mike Fikes [ 15/Nov/17 9:56 AM ]

Attached a patch (CLJS-2377.patch) which addresses an issue (extra argument) in previous patch, adds tests, and covers both places where the DatatypeConverter code was being used.

Comment by Mike Fikes [ 17/Nov/17 9:22 AM ]

I'm a bit concerned about the perf of bytes-to-hex-str in the previous CLJS-2377.patch because it is about 200 times slower than DatatypeConverter/printHexBinary and it can used by content-sha on fairly large strings (source files, for example). Attaching a revision CLJS-2377-2.patch which falls within a factor of 5 of DatatypeConverter/printHexBinary.

Comment by Mike Fikes [ 17/Nov/17 3:44 PM ]

Attached a CLJS-2377-3.patch that further brings the perf of bytes-to-hex-str to less than a factor of 2 of DatatypeConverter/printHexBinary.

Comment by David Nolen [ 01/Dec/17 3:17 PM ]

fixed https://github.com/clojure/clojurescript/commit/544c1b77d76d48f234cdb03746ea993158c46aff

Comment by Mike Fikes [ 01/Dec/17 3:22 PM ]

Confirmed fixed with downstream project and Java 9.





[CLJS-2420] Make dir work on aliases Created: 26/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

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

 Description   

Extend cljs.repl/dir to work with the aliases in the current namespace. (Port CLJ-1673).



 Comments   
Comment by Mike Fikes [ 26/Nov/17 11:21 AM ]

Forgot to handle case of macros namespace aliases, as in

cljs.user=> (require-macros '[clojure.spec.alpha :as s])
nil
cljs.user=> (dir s)

Assigned to myself to rectify.

Comment by Mike Fikes [ 26/Nov/17 11:24 AM ]

CLJS-2420-2.patch adds a fallback to checking for a macros namespace alias.

Comment by Mike Fikes [ 26/Nov/17 11:29 AM ]

This revision also works nicely in the case of clojure -> cljs namespace aliasing, preventing confusion that may otherwise arise:

cljs.user=> (require 'clojure.test)
nil
cljs.user=> (dir clojure.test)
*current-env*
IAsyncTest
are
async
...
Comment by David Nolen [ 01/Dec/17 3:21 PM ]

fixed https://github.com/clojure/clojurescript/commit/76efc272b0e3142e93fc27c423174cb34c3aa904





[CLJS-2425] Remove unnecessary zero? checks from nat-int? Created: 29/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

Status: Closed
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: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2425.patch    
Patch: Code and Test
Approval: Accepted

 Description   

nat-int? is satisfied if a number is not negative or zero. Since zero is not negative, the zero test can be removed.



 Comments   
Comment by Mike Fikes [ 29/Nov/17 9:08 AM ]

Added an additional test case in the unit tests that specifically checks for proper behavior with negative zero. Node that no new tests are added for goog Long and Integer because, if you attempt to construct either with negative zero, you end up with positive zero. (In other words, it is impossible to make a negative-zero variant of these goog numeric types.)

Comment by David Nolen [ 01/Dec/17 3:19 PM ]

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





[CLJS-2404] Travis JavaScriptCore unit tests failing Created: 19/Nov/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

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

Attachments: Text File CLJS-2404-2.patch     Text File CLJS-2404.patch    
Patch: Code and Test
Approval: Accepted

 Description   

The JavaScriptCore Travis tests are failing like this:

$ jsc core-advanced-test.js

Testing cljs.primitives-test
Segmentation fault (core dumped)

This appears to be related to code generate for tests added with CLJS-2391, triggering CLJS-910 (because the Travis environment runs an older version of JavaScriptCore).

It can be worked around by slightly modifying the test.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 9:22 AM ]

Assigning back to myself. Something failed in Travis with the attached patch.

Comment by Mike Fikes [ 19/Nov/17 10:23 AM ]

The attached CLJS-2404-2.patch contains a variant of the workaround that reliably works in the Travis environment.

Comment by David Nolen [ 01/Dec/17 3:15 PM ]

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





[CLJS-2215] Allow clj->js to preserve namespaces Created: 11/Jul/17  Updated: 01/Dec/17  Resolved: 01/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: Enzzo Cavallo Assignee: David Nolen
Resolution: Completed Votes: 2
Labels: cljs, enhancement, interop

Attachments: Text File clj-to-js.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Original issue
https://dev.clojure.org/jira/browse/CLJS-536

Keypoints from CLJS-536

  • IEncodeJS is powerfull, but for keywords, can break other libreries that expect trim-ns behavior (Le Wang)
  • With the introduction of specs, the namespaced keywords are being used more and more. This issue prevents streamlined edn->json->edn transformation. I think it should be reopened. IMO the 'lossy' method should never be a default one. (Jozef Wagner)
  • Should be possible do this without break existing code and using kwargs (Paulus Esterhazy)

An use example can be `(clj->js {:foo/bar 33} :keyword-fn #(.-fqn %)) ;=> #js {:foo/bar 33}`

PS: key->js should use key>js method, but I keep it with clj>js to avoid break things (it should be another bug?!).



 Comments   
Comment by Mike Fikes [ 30/Nov/17 7:53 AM ]

Looks like we are already set up for the opposite direction:

cljs.user=> (js->clj #js {"cljs.user/bar" 33 "cljs.user/baz" 12} :keywordize-keys true)
#:cljs.user{:bar 33, :baz 12}

Docstring should be updated for :keyword-fn.

Given that this might slow down clj->js, perhaps benchmarks should be added to cljs.benchmark-runner to show that there aren't huge perf regressions.

Comment by David Nolen [ 01/Dec/17 3:14 PM ]

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





[CLJS-2392] WARNING: out/inferred_externs.js: WARNING - name goog is not defined in the externs. Created: 30/Oct/17  Updated: 01/Dec/17

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

Type: Defect Priority: Minor
Reporter: Arne Brasseur Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

ClojureScript 1.9.946 (standalone cljs.jar)



 Description   

See demo and explanation here: https://github.com/lambdaisland/npmdemo/tree/goog-undefined-minimal

  • :target :nodejs
  • :optimizations :advanced
  • :infer-externs true
  • :install-deps true

The inferred_externs.js contains goog.isArrayLike and goog.global, and the compilation complains on these lines that "goog" is not defined.

Oct 30, 2017 1:46:36 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: out/inferred_externs.js:3: WARNING - name goog is not defined in the externs.
goog.isArrayLike;
^^^^

Oct 30, 2017 1:46:36 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: out/inferred_externs.js:4: WARNING - name goog is not defined in the externs.
goog.global;


 Comments   
Comment by Mike Rodriguez [ 01/Dec/17 3:09 PM ]

I don't have a full demo project for this like the description does.

However, I figured I'd add that I see similar with a build like:

:closure-defines {goog.DEBUG false}
:infer-externs true
:optimizations :advanced
:pretty-print false

Notice that there is no :target :nodejs or :intall-deps true configured here.

I have warnings for externs, such as

WARNING: /path/to/file/inferred_externs.js:33: WARNING - name goog is not defined in the externs.
goog.DEBUG;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:34: WARNING - name goog is not defined in the externs.
goog.string;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:35: WARNING - name goog is not defined in the externs.
goog.string.StringBuffer;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:36: WARNING - name goog is not defined in the externs.
goog.string.StringBuffer.prototype.append;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:37: WARNING - name goog is not defined in the externs.
goog.isArrayLike;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:38: WARNING - name goog is not defined in the externs.
goog.net;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:39: WARNING - name goog is not defined in the externs.
goog.net.XhrIo;
^^^^

The "inferred_externs.js" file has lines

goog.DEBUG;
goog.string;
goog.string.StringBuffer;
goog.string.StringBuffer.prototype.append;
goog.isArrayLike;
goog.net;
goog.net.XhrIo;

with goog never pre-defined. Of course it doesn't need to be of course.





[CLJS-2419] Self-host: `find-ns-obj` still broken for namespaces with 'a' as the first segment Created: 25/Nov/17  Updated: 30/Nov/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: António Nuno Monteiro
Resolution: Unresolved Votes: 0
Labels: None


 Description   

CLJS-2024 didn't fix this, which still has issues under Node.js



 Comments   
Comment by António Nuno Monteiro [ 25/Nov/17 3:54 PM ]

The attached patch removes the Node.js special case in `find-ns-obj` that uses `eval`, given that under simple optimizations the namespaces are in the `goog.global` scope.

The call to `js/eval` is no longer needed and was causing more problems that it solved.

Here's some more context on those problems: https://github.com/anmonteiro/lumo/issues/301

Comment by António Nuno Monteiro [ 25/Nov/17 4:21 PM ]

Removing the patch as it shows some regressions.





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

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





[CLJS-2421] Referred type can be replaced by same-named type in referring namespace Created: 27/Nov/17  Updated: 27/Nov/17

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

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


 Description   

Two namespaces:

(ns my.ns)

(defrecord A [foo bar])

(defn make-a [foo bar]
  (A. foo bar))
(ns other.ns
  (:require [my.ns :refer (A)]))

(println (my.ns/A. 1 2) (my.ns/make-a 1 2))

(defrecord A [baz quux])

(println (my.ns/A. 1 2) (my.ns/make-a 1 2))

Loading other.ns yields output:

#my.ns.A{:foo 1, :bar 2} #my.ns.A{:foo 1, :bar 2}
#my.ns.A{:baz 1, :quux 2} #my.ns.A{:baz 1, :quux 2}

The definition of other.ns.A is replacing that of my.ns.A.



 Comments   
Comment by Chas Emerick [ 27/Nov/17 12:39 PM ]

This may be a duplicate of CLJS-1558...?

Comment by Mike Fikes [ 27/Nov/17 12:40 PM ]

This ticket shares similarities with CLJS-1558 in that the compiler is essentially allowing you to re-def a referred Var. Perhaps for both tickets this should cause a compiler error as occurs with Clojure.

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

I struggle a little with this one in terms of defining what should be going on, and at the core of my lack of clarity is on whether my.ns/A is a Var or simply a type.

In the compiler state, it does show up under :defs, but it is flagged with :type true. (In other words, perhaps just because it is under :def doesn't imply it is a Var.) In Clojure, even though an underlying constructor exists that can be accessed via interop, no actual Var is created. (Evidence backing this assertion is that (var A) fails in Clojure, and no Var appears if you use dir at the REPL—you only see the associated synthetically-generated positional and map factory Vars.

Another argument that "it is not a Var" is what defrecord returns. defrecord doesn't return a Var (even in Clojure), but instead returns a symbol (which evaluates to itself, much like the symbol java.lang.String).

If you go with the argument that the underlying constructor is not a Var, then perhaps some things could be cleaned up (fixing dir in ClojureScript, and perhaps causing var to balk if applied to types).

Also, if you go with the argument that it is not a Var, then perhaps you'd argue that import is appropriate, while require is not. Inspired by http://puredanger.github.io/tech.puredanger.com/2010/06/30/using-records-from-a-different-namespace-in-clojure/, this variation seems to work fine:

(ns my.ns)

(defrecord A [foo bar])
(ns other.ns
  (:require [my.ns])
  (:import (my.ns.A)))

(println (my.ns.A. 1 2) (my.ns/->A 1 2))

(defrecord A [baz quux])

(println (my.ns.A. 1 2) (my.ns/->A 1 2))

(println (A. 3 4) (->A 3 4))

with this result:

cljs.user=> (require 'other.ns)
#my.ns.A{:foo 1, :bar 2} #my.ns.A{:foo 1, :bar 2}
#my.ns.A{:foo 1, :bar 2} #my.ns.A{:foo 1, :bar 2}
#other.ns.A{:baz 3, :quux 4} #other.ns.A{:baz 3, :quux 4}
nil

One thing I don't like is that you have to write (:import (my.ns.A)) as opposed to {{(:import (my.ns A))}, but perhaps that is a minor quirk that could be cleaned up if the "not a Var" approach were taken.





[CLJS-1558] Code allowed to re-define referred var Created: 31/Jan/16  Updated: 27/Nov/17

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




[CLJS-2413] Port core.specs.alpha to ClojureScript Created: 23/Nov/17  Updated: 25/Nov/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Mike Fikes
Resolution: Unresolved Votes: 2
Labels: spec

Attachments: Text File 0001-Experiment-Attach-ns-spec-to-internal-use-Var.patch     Text File CLJS-2413-1.patch    
Patch: Code

 Description   

Port https://github.com/clojure/core.specs.alpha to ClojureScript, adding a cljs.core.specs.alpha namespace that has the specs in the Clojure version, modified as needed for ClojureScript. (We would ideally make minimal revisions, preserving as much original structure as reasonable, so that future upstream changes can be more easily ported. Additionally, tests should be ported over.)

The specs should be usable from both JVM-based and self-hosted ClojureScript. (This likely implies a design making clever use of cljc and reader-conditionals so that macro specs work for both ClojureScript variants.}

This would essentially be an experimental feature which, while shipping with ClojureScript, can be explicitly opted into by end-users who wish to make use of that namespace. In other words, it would not be required by any existing production code in the ClojureScript tree, at least initially. If it ships with ClojureScript, this can facilitate more broad experimentation and rooting out any fine tuning that may be needed in the specs.

We could add checks for the require and require-macros macros, heavily based on the support that exists in that namespace for the Clojure ns macro, but of course heavily modified to reflect ClojureScript's constraints and special features (string require, etc.)

We might also be able to use this work to to provide inspiration what to do about the subject of specing the ns special form. (Do we turn ns into a macro that expands to some other special form, or do we revise the compiler's macroexpand check to also be able to do special form checks?)



 Comments   
Comment by Thomas Heller [ 23/Nov/17 11:10 AM ]

FWIW I have been using some core.specs in shadow-cljs for a while now (just the JVM side though).

See:
https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/cljs/devtools/cljs_specs.clj

One major issue with these (or specs in general) is that the errors are absolutely terrible. Some checks done by the cljs.anaylzer or cljs.compiler provide far better error messages but don't test as strict. Lately I have been testing expound [1] which is a bit better but still far from good.

I'm also using spec to parse for the ns form which has the same issue but otherwise works very nice.
https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/build/ns_form.clj

[1] https://github.com/bhb/expound/

Comment by Mike Fikes [ 24/Nov/17 8:28 PM ]

The attached CLJS-2413-1.patch ports over the main namespace (there is no test namespace), making revisions for ClojureScript. Additionally, since we can't spec the ns special form, I added a few additional specs for require, require-macros, use, and use-macros.

Using the new namespace is easy: Just require cljs.core.specs.alpha and then specs will exist for:

cljs.core/defn
 cljs.core/defn-
 cljs.core/fn
 cljs.core/if-let
 cljs.core/import
 cljs.core/let
 cljs.core/require
 cljs.core/require-macros
 cljs.core/use
 cljs.core/use-macros
 cljs.core/when-let
Comment by Mike Fikes [ 25/Nov/17 9:15 AM ]

I tried some experiments with attaching a spec the ns special form:

Turning ns into a macro that expands to some other special form is challenging (I'm not convinced it is impossible, but you hit special circular cases when trying to bootstrap an environment this way). Perhaps this would be the "cleanest" solution, if it can be pulled off correctly.

Attempting to simply revise the macroexpand-check path to also work on special forms is problematic because you actually need a Var to attach a spec to.

The attached 0001-Experiment-Attach-ns-spec-to-internal-use-Var.patch, which applies after CLJS-2413-1.patch, illustrates a third potential approach: It introduces a internal-use macro Var to stand in for ns, attaches a spec to that internal-use Var, and then in the macroexpand-check path, special cases the ns special form to check any spec that might be attached to the internal-use Var.

With this experimental patch applied, you can indeed verify that ns forms can be spec-checked properly using the specs in that namespace.





[CLJS-2417] Inter-ns s/fdef expansion side effect fails when load cached source Created: 24/Nov/17  Updated: 24/Nov/17

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

Like CLJS-1989, but for inter-ns s/fdef as opposed to intra-ns. When loading from cache, if an s/fdef specs a function in another namespace, the load can fail. The root cause is that analysis cache writing and reading is scoped by namespace, and one failure mode is as follows: When analysis cache is written for a given namespace A, specs set up by a different namespace B may not have yet been loaded. When that namespace B is subsequently loaded, if any cache is written for B, it will, due to the scoping, only write out information relevant to B.

To repro, the setup is similar to that in CLJS-1989, but involves splitting the defn and s/fdef across two namespaces:

Have src/foo/core.cljs with:

(ns foo.core)

(defn bar [x])

Have src/baz/core.cljs with:

(ns baz.core
  (:require [clojure.spec.alpha :as s]
            [foo.core]))

(s/fdef foo.core/bar :args (s/cat :x number?))

Then:

$ java -cp cljs.jar:src clojure.main -m cljs.repl.nashorn
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.test.alpha :as st] 'baz.core)
true
cljs.user=> (st/instrument)
[foo.core/bar]
cljs.user=> :cljs/quit

Now this has been cached. If you exit and try again, (st/instrument) fails:

$ java -cp cljs.jar:src clojure.main -m cljs.repl.nashorn
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.test.alpha :as st] 'baz.core)
true
cljs.user=> (st/instrument)
[]

Note, the above was reproduced using ClojureScript 1.9.655 as well, where CLJS-1989 landed, to be sure that this wasn't a subsequent regression.

As an aside, the motivating example where this inter-ns pattern arises is porting of clojure.core.specs.alpha to cljs.core.specs.alpha (CLJS-2413) where in each case, the specs are in a separate *...specs namespace.






[CLJS-2406] ^:export not supported on defrecord Created: 21/Nov/17  Updated: 24/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Víctor M. Valenzuela Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

^:export doesn't appear to have an effect on defrecord - I only see Closure emitting the original symbols for def and defn.

I guess that similarly, deftype, defprotocol etc also would be reasonable targets.

In any case, it would be good to explicitly state what works and what does currently not (advanced-compilation.adoc), so one doesn't find himself blindly trying out things.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 7:00 AM ]

You can export protocol methods. Proposed site change: https://github.com/clojure/clojurescript-site/pull/148

Comment by Mike Fikes [ 21/Nov/17 9:20 AM ]

^:export works on (and only on) Vars.

Protocol methods are Vars, so the above site change would help clarify how to export them.

The remaining Vars under consideration would be the map and positional factory functions and constructors that are synthetically generated for defrecord and deftype.

Perhaps a change could be accepted such that (defrecord ^:export Foo []) would export map->Foo, ->Foo, and Foo. And likewise (deftype ^:export Bar []) would export ->Bar and Bar.

Comment by Mike Fikes [ 22/Nov/17 6:10 AM ]

Workaround: Use goog/exportSymbol:

(ns my-ns.core)

(defrecord Foo [])
(goog/exportSymbol "my_ns.core.map__GT_Foo" my_ns.core.map__GT_Foo)
(goog/exportSymbol "my_ns.core.__GT_Foo" my_ns.core.__GT_Foo)
Comment by Víctor M. Valenzuela [ 23/Nov/17 3:34 PM ]

Hi Mike,

thanks so much for clarifying and taking care of this one! Appreciated.

^:export works on (and only on) Vars.

This would be an excelent piece of knowledge to include in the documentation IMO. Why not tell the whole story in a single place?

The original concern that made me open this issue was logging/debugging.

Say an application has a series of singleton defrecord instances, as part of a 'component' system (Sierra style).

Then, for logging, I might be tempted to (js/console.log (type the-instance)), so I can identify e.g. a buggy component.

But without language support, one is unable to get that (or something similar) to work without lots of boilerplate, or perhaps a hacky macro.

Would it be possible to make (type some-instance-of-a-defrecord-or-deftype) print the original name under advanced compilation?

Comment by Mike Fikes [ 24/Nov/17 9:16 AM ]

Hi Victor,

Thanks for the ticket intent clarification. ^:export is not for debugging, but is for making un-renamed symbols available to JavaScript. In fact, it doesn't change renamed and optimized JavaScript, but instead simply makes additional un-renamed aliases available which point into optimized / renamed code. (Thus, generally, debugging deals with the renamed / optimized code, even if there are un-renamed ^:exports sitting off to the side.)

Fortunately, :pseudo-names exists to help with debugging optimized code: https://clojurescript.org/reference/compiler-options#pseudo-names

For your example involving logging the type of the instance, instead of, for example [Function: ze] (under Node.js), you would get [Function: $my_ns$core$Foo$$] if :pseudo-names is enabled.

I've proposed revisions to the site: https://github.com/clojure/clojurescript-site/pull/150

If :pseudo-names satisfies the needs of this ticket, perhaps this ticket can be closed.





[CLJS-1676] Unused local in ObjMap / IKVReduce Created: 09/Jun/16  Updated: 24/Nov/17

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

Type: Defect Priority: Trivial
Reporter: Stuart Hinson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Local len isn't used in ObjMap / IKVReduce

https://github.com/clojure/clojurescript/blob/463de005b81d4a00951188e8b8d38a61d684c18e/src/main/cljs/cljs/core.cljs#L5792



 Comments   
Comment by Stuart Hinson [ 10/Aug/17 2:56 PM ]

BTW, recognize this is a trivial issue, but it's still around

https://github.com/clojure/clojurescript/blob/fe77a1349aba903856cbaa0a0f10e03b6fb30d92/src/main/cljs/cljs/core.cljs#L6150

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

Patch no longer applies; needs re-baseline.

Comment by Stuart Hinson [ 24/Nov/17 8:48 AM ]

Thanks for the heads up! Remade the patch





[CLJS-2412] Consuming npm module Created: 22/Nov/17  Updated: 22/Nov/17

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

Type: Defect Priority: Minor
Reporter: Jan Břečka Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When trying to :require the uuid (https://www.npmjs.com/package/uuid) npm package:

(:require ["uuid/v4" :as uuid-v4])

compiled javascript is failing on Reference error: require is not defined. It's caused because the library itself requiring two files:

var rng = require('./lib/rng');
var bytesToUuid = require('./lib/bytesToUuid');

While the require('./lib/bytesToUuid') call is properly replaced with "goog provide", the require('./lib/rng') is not touched at all. This is part of the package.json the library provides:

"browser": { "./lib/rng.js": "./lib/rng-browser.js", "./lib/sha1.js": "./lib/sha1-browser.js" }

The problem is that this "browser" alternative is ignored.



 Comments   
Comment by Thomas Heller [ 22/Nov/17 12:14 PM ]

There are 2 pending pull requests for the Closure Compiler related to this.

https://github.com/google/closure-compiler/pull/2622
https://github.com/google/closure-compiler/pull/2627





[CLJS-2409] Eliminate cljs.core/divide Created: 21/Nov/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: 0
Labels: None

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

 Description   

The cljs.core/divide inlining macro was introduced to work around an inability to specify cljs.core//.

For example: https://github.com/clojure/clojurescript/blob/d45012273029bd5df3973ea716394f368e1c44cc/src/main/cljs/cljs/core.cljs#L2593

This can now evidently be removed and thus avoid things like CLJS-594 and divide showing up as a completion in IDEs, etc.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 9:38 PM ]

CLJS-2409-2.patch adds unit tests.





[CLJS-2408] js->clj does not work on objects with null prototype Created: 21/Nov/17  Updated: 21/Nov/17

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

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


 Description   

Version: 1.9.946
Follow up from issue CLJS-1998

cljs.user=> (js->clj #js {})
{}

cljs.user=> (js->clj (.create js/Object nil))
#object[Object]

cljs.user=> (keys (js->clj (.create js/Object nil)))
org.mozilla.javascript.EcmaError: TypeError: Cannot find default value for object. (.cljs_rhino_repl/goog/../.cljs_rhino_repl/cljs/core.js#9915)
	 (.cljs_rhino_repl/cljs/core.cljs:2930:10)
	 cljs$core$seq (.cljs_rhino_repl/cljs/core.cljs:1212:13)
	 cljs$core$keys (.cljs_rhino_repl/cljs/core.cljs:8648:3)
	 (NO_SOURCE_FILE <cljs repl>:1:0)
	 (NO_SOURCE_FILE <cljs repl>:1:0)


 Comments   
Comment by Kurt Harriger [ 21/Nov/17 8:44 PM ]

For more context the source of the object with null prototype is generated in a third party library I am using here https://github.com/ProseMirror/prosemirror-model/blob/cdded3d/src/schema.js#L13





[CLJS-2403] Document and test that min-key, max-key return last Created: 19/Nov/17  Updated: 21/Nov/17

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

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

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

 Description   

Essentially port CLJ-2247 to ClojureScript.






[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: 0
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-1997] Outward function type hint propagation Created: 03/Apr/17  Updated: 20/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: None

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

 Description   

New feature: Detect the type of a function and arrange so that it is available for outward type hinting.

For reference, a small amount of discussion from Jan 2016 in the #cljs-dev Slack:

mfikes: Curious if "outward" ^boolean propagation has been debated / rejected. Example: If simple analysis detects that ^boolean can be applied as in for functions like (defn f [x] (not x)). Perhaps automatically propagating type hints in this direction would be unwelcome by users?

dnolen: @mfikes: we have already have outward propagation for numerics which broke because of changes for code motion

dnolen: absolutely nothing against that



 Comments   
Comment by Mike Fikes [ 03/Apr/17 7:38 PM ]

The attached patch will handle cases like

(defn foo? [x] (or (string? x) (number? x)))

and

(def baz (fn ([x] 1) ([x y] 2)))

but it doesn't attempt to address the case of multi-arity defn s, as these are implemented by dispatching to multiple single-arity functions that could have different return types.

It adds a :inferred-ret-tag to the AST only because the arguments to or on line 18 in the patch are not reversed (only to allow explicit hints to override inferred hints, but perhaps that is overcomplicating things and the inferred tag could be dropped directly into :ret-tag.)

Bronsa helpfully pointed out in Slack that outward type hint propagation is avoided in Clojure because it breaks things like:

(defn foo [] 1)
(defn bar [] (String/valueOf (foo)))
(defn foo [] "foo")
(bar)

for which an analogy in ClojureScript would be:

(defn foo? [] true)
(defn bar [] (if (foo?) :t :f))
(defn foo? [] "")
(bar)

where the patch causes it to yield :f.

This could be viewed as a REPL-only concern (perhaps addressed by another compiler flag along the lines of :static-fns).

But, arguably the problem already exists today, with type hinting in code like the following affecting the compilation of bar:

(def foo? true)
(defn bar [] (if foo? :t :f))
(def foo? "")
(bar)
Comment by David Nolen [ 07/Apr/17 11:15 AM ]

Thanks will think about it

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

I've been soaking this for a couple of months (with local builds of Planck), and haven't seen anything break, FWIW. So, I think it is at least a safe change. Assigning to David for further consideration.





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

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

Type: Defect Priority: Minor
Reporter: Jeremy R Sellars Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

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


Attachments: Text File cljs-1965_wrap_letfn_statements.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




[CLJS-1889] A lone ampersand `&` can be used to name a var, but throws when invoked. `(&)` Created: 15/Jan/17  Updated: 20/Nov/17

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

Type: Defect Priority: Trivial
Reporter: Aleksander Madland Stapnes Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

I can use the symbol & to name something, but if I try to invoke it, the following exception is thrown:

Exception in thread "main" clojure.lang.ExceptionInfo: failed compiling file:src/amp/core.cljs {:file #object[java.io.File 0x5e63cad "src/amp/core.cljs"]}, compiling:(/home/madstap/code/ampersand/build.clj:3:1)
	at clojure.lang.Compiler.load(Compiler.java:7391)
	at clojure.lang.Compiler.loadFile(Compiler.java:7317)
	at clojure.main$load_script.invokeStatic(main.clj:275)
	at clojure.main$script_opt.invokeStatic(main.clj:335)
	at clojure.main$script_opt.invoke(main.clj:330)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.ExceptionInfo: failed compiling file:src/amp/core.cljs {:file #object[java.io.File 0x5e63cad "src/amp/core.cljs"]}
	at clojure.core$ex_info.invokeStatic(core.clj:4617)
	at cljs.compiler$compile_file$fn__3147.invoke(compiler.cljc:1406)
	at cljs.compiler$compile_file.invokeStatic(compiler.cljc:1376)
	at cljs.closure$compile_file.invokeStatic(closure.clj:430)
	at cljs.closure$fn__4204.invokeStatic(closure.clj:497)
	at cljs.closure$fn__4204.invoke(closure.clj:493)
	at cljs.closure$fn__4146$G__4139__4153.invoke(closure.clj:389)
	at cljs.closure$compile_sources$iter__4315__4319$fn__4320.invoke(closure.clj:829)
	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:688)
	at clojure.core$next__4341.invokeStatic(core.clj:64)
	at clojure.core$dorun.invokeStatic(core.clj:3033)
	at clojure.core$doall.invokeStatic(core.clj:3039)
	at cljs.closure$compile_sources.invokeStatic(closure.clj:826)
	at cljs.closure$build.invokeStatic(closure.clj:1942)
	at cljs.build.api$build.invokeStatic(api.clj:198)
	at cljs.build.api$build.invoke(api.clj:187)
	at cljs.build.api$build.invokeStatic(api.clj:190)
	at cljs.build.api$build.invoke(api.clj:187)
	at user$eval24.invokeStatic(build.clj:3)
	at user$eval24.invoke(build.clj:3)
	at clojure.lang.Compiler.eval(Compiler.java:6927)
	at clojure.lang.Compiler.load(Compiler.java:7379)
	... 11 more
Caused by: clojure.lang.ExceptionInfo: No method in multimethod 'parse' for dispatch value: & at line 5 src/amp/core.cljs {:file "src/amp/core.cljs", :line 5, :column 1, :tag :cljs/analysis-error}
	at clojure.core$ex_info.invokeStatic(core.clj:4617)
	at cljs.analyzer$error.invokeStatic(analyzer.cljc:628)
	at cljs.analyzer$analyze_seq_STAR__wrap.invokeStatic(analyzer.cljc:2871)
	at cljs.analyzer$analyze_seq.invokeStatic(analyzer.cljc:2892)
	at cljs.analyzer$analyze_form.invokeStatic(analyzer.cljc:3011)
	at cljs.analyzer$analyze_STAR_.invokeStatic(analyzer.cljc:3056)
	at cljs.analyzer$analyze.invokeStatic(analyzer.cljc:3073)
	at cljs.compiler$emit_source.invokeStatic(compiler.cljc:1255)
	at cljs.compiler$compile_file_STAR_$fn__3124.invoke(compiler.cljc:1325)
	at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:1159)
	at cljs.compiler$compile_file_STAR_.invokeStatic(compiler.cljc:1316)
	at cljs.compiler$compile_file$fn__3147.invoke(compiler.cljc:1396)
	... 34 more
Caused by: java.lang.IllegalArgumentException: No method in multimethod 'parse' for dispatch value: &
	at clojure.lang.MultiFn.getFn(MultiFn.java:156)
	at clojure.lang.MultiFn.invoke(MultiFn.java:251)
	at cljs.analyzer$analyze_seq_STAR_.invokeStatic(analyzer.cljc:2867)
	at cljs.analyzer$analyze_seq_STAR__wrap.invokeStatic(analyzer.cljc:2870)
	... 43 more


 Comments   
Comment by Aleksander Madland Stapnes [ 16/Jan/17 1:59 AM ]

Better explanation as I can't seem to edit the description: https://gist.github.com/madstap/c77581185afa7fea8bbf2556f2d9fafe

Comment by Mike Fikes [ 20/Nov/17 7:45 PM ]

If ClojureScript's symbols follow Clojure's constraints (https://clojure.org/reference/reader#_symbols), then & is not a valid symbol.

Of course,

(type (second (second '(fn [f & r]))))
is cljs.core/Symbol, but perhaps that is an exception to the rule, reserved for use by Clojure.





[CLJS-1495] Internal ast? assertion for var in fn in def Created: 28/Nov/15  Updated: 20/Nov/17  Resolved: 20/Nov/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

Also affects 170 (not in pulldown yet)
Same behavior in regular and bootstrapped



 Description   

This form, when issued at the REPL, fails to compile, triggering an ana/ast? :pre violation:

(def f (fn [] #'f))

There appears to be nothing fundamentally wrong with the construct, as it can be worked around in multiple ways.

These workarounds, which move the var special out, succeed:

(def f (let [vf #'f] (fn [] vf)))
(declare f)
(let [x (fn [] #'f)] (def f x))

Also, these subtler workarounds succeed in avoiding the issue:

(def f (fn [] #'cljs.user/f))
(def f (fn x [] #'f))


 Comments   
Comment by Mike Fikes [ 28/Nov/15 7:49 PM ]

Analysis of the issue:

tl;dr: A synthetic local is shadowing the top-level var that the var special is being applied to.

Detailed analysis:

The form

(def f (fn [] #'f))

can be reduced to this simpler self-named form to consider, which also exhibits the issue:

(fn f [] #'f)

Analyzing (def f (fn [])) will show that :fn-self-name true is set.

A consequence is that the self name, f in the running example, is carried into the function body as a local symbol, and thus f is no longer a symbol resolving to the top-level var. This leads to a analyzing code that is, in essence, like this sequence

(declare g)
#'g ;; OK
(let [g 1] #'g) ;; exhibits error

Another way of saying the above: The locally introduced self-name, which is otherwise fine with respect to self-recursion, thwarts the var special in this situation, effectively shadowing the top-level var.

Comment by Mike Fikes [ 29/Nov/15 11:08 AM ]

A similar situation occurred for Clojure and that it defeated memoization until fixed http://dev.clojure.org/jira/browse/CLJ-809

Given this

(defn fib [n]
  (if (< n 2)
    n
    (+ (fib (dec n))
      (fib (dec (dec n))))))

Clojure takes a few seconds to compute (fib 41) but is instantaneous after (def fib (memoize fib)).

The synthetic local defeats this attempt at memoization in ClojureScript.

Comment by Mike Fikes [ 20/Nov/17 7:28 PM ]

No longer reproducible in 1.9.946.





[CLJS-1070] top-level boolean inference does not work Created: 28/Feb/15  Updated: 20/Nov/17

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

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


 Description   

Problem for using boolean Closure defines



 Comments   
Comment by Francis Avila [ 30/Mar/15 12:02 PM ]

I am unsure if this is the same issue, but forms like ^boolean (js/isFinite n) also do not seem to analyze correctly: if, and, and or will still emit a call to truth_.

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

It appears this is no longer reproducible:

$ java -jar cljs.jar -m cljs.repl.node
ClojureScript Node.js REPL server listening on 51236
To quit, type: :cljs/quit
cljs.user=> *clojurescript-version*
"1.9.946"
cljs.user=> (set! *print-fn-bodies* true)
true
cljs.user=> (fn [n] (if ^boolean (js/isFinite n) 1 2))
#object[ret__6224__auto__ "function (n){
if(isFinite(n)){
return (1);
} else {
return (2);
}
}"]
cljs.user=> (fn [n] (if (js/isFinite n) 1 2))
#object[ret__6224__auto__ "function (n){
if(cljs.core.truth_(isFinite(n))){
return (1);
} else {
return (2);
}
}"]




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

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

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

Android 4.3, Chrome 34, ClojureScript 2202



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

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

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

Let me know if any further details are required.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 7:13 PM ]

Hey John, we've committed a lot of optimizations over the past year or so. I wonder if this is still reproducible.





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

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

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

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



 Description   

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

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



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

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

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

will log: "Match: hello world"

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

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

Comment by Mike Fikes [ 20/Nov/17 7:05 PM ]

Perhaps this has been resolved. In ClojureScript 1.9.946:

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




[CLJS-375] loop doesn't seem to preserve tag information as evidenced by extra cljs.core.truth_ calls Created: 06/Sep/12  Updated: 20/Nov/17

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

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


 Comments   
Comment by Mike Fikes [ 20/Nov/17 6:11 PM ]

I can't find a way to repro this with 1.9.946.





[CLJS-872] Ordered collections printed as if they were unordered at the REPL Created: 13/Oct/14  Updated: 20/Nov/17

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

Type: Defect Priority: Trivial
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is due to cljs.repl's read-then-print processing of string representations of return values that come back from the JS env. As of release 2371, the relevant code fragment lives here:

https://github.com/clojure/clojurescript/blob/r2371/src/clj/cljs/repl.clj#L156

A larger snippet to demonstrate this:

ClojureScript:cljs.user> (array-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14)
{1 2, 3 4, 5 6, 7 8, 9 10, 11 12, 13 14}
ClojureScript:cljs.user> (array-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)
{7 8, 1 2, 15 16, 13 14, 17 18, 3 4, 11 12, 9 10, 5 6}
ClojureScript:cljs.user> (seq (array-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18))
([1 2] [3 4] [5 6] [7 8] [9 10] [11 12] [13 14] [15 16] [17 18])
ClojureScript:cljs.user> (hash-map 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)
{7 8, 1 2, 15 16, 13 14, 17 18, 3 4, 11 12, 9 10, 5 6}

This issue seems to be the most likely cause of the problem described in this StackOverflow question. It would be nice to print ordered collections in the "expected" way to prevent user confusion.



 Comments   
Comment by David Nolen [ 14/Oct/14 5:23 AM ]

How is this handled in Clojure?

Comment by Michał Marczyk [ 14/Oct/14 7:40 AM ]

The built-in REPL simply prints string representations of values to *out* without attempting to read them back in first.

Comment by David Nolen [ 14/Oct/14 7:54 AM ]

Well the result of REPL evaluation is a string unlike Clojure, in order to get a proper print at the ClojureScript REPL it seems necessary to read back the result. I will say it's not clear to me array-map promises anything about order anyway.

Comment by Michał Marczyk [ 14/Oct/14 8:02 AM ]

It does – see http://clojure.org/data_structures, where it says

ArrayMaps
When doing code form manipulation it is often desirable to have a map which maintains key order. An array map is such a map - it is simply implemented as an array of key val key val... As such, it has linear lookup performance, and is only suitable for very small maps. It implements the full map interface. New ArrayMaps can be created with the array-map function. Note that an array map will only maintain sort order when un-'modified'. Subsequent assoc-ing will eventually cause it to 'become' a hash-map.

This snippet has been there for as long as I can remember; and this particular feature comes up in various online discussions from time to time. The docstrings for array-map (the core function) are unambiguous in their promise to construct array maps in both Clojure and ClojureScript.

As for the issue at hand, I do think it's a very minor one (hence the "Trivial" priority), but it is real – the representation printed at the REPL is out of line with the string returned from pr-str on the same value. In fact, one way to fix this would be to use pr-str representations computed on the CLJS side. (There may be some complications here, though, which we'd have to work out.)

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

Thanks for pointing out the API promise. OK, yeah I don't really have a good idea for how do this but a patch that isn't too ugly is welcome

Comment by Mike Fikes [ 20/Nov/17 5:14 PM ]

I think this is no longer reproducible. For example (apply array-map (range 100)) prints in an ordered way. There is also no longer a read-string call in repl.cljc.





[CLJS-2000] Don't log deprecation warnings on recursive calls to the same function with a different arity Created: 05/Apr/17  Updated: 20/Nov/17

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

Type: Defect Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: warning


 Description   

If a function has two arity's where one calls the other, then if that function is marked with `^:deprecated`, then compile warnings about deprecations will always be emitted while that function exists. E.g.

(defn ^:deprecated test-deprecated
  ([]
    (test-deprecated nil))
  ([a]
    nil))

produces these logs:

WARNING: my.test/test-deprecated is deprecated. at line 3 src/my/test/error.cljs

I think that only outside references to a deprecated function should warn here. Otherwise, it's impossible to deprecate a multi-arity function and still get clean compiles.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 4:03 PM ]

A workaround: You can mark call forms with ^:deprecation-nowarn to suppress deprecation warnings.

Applied to the example in this ticket:

(defn ^:deprecated test-deprecated
  ([]
    ^:deprecation-nowarn (test-deprecated nil))
  ([a]
    nil))




[CLJS-2070] Dotted interop forms can leak invalid JS into output Created: 04/Jun/17  Updated: 20/Nov/17

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

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


 Description   

(println .aJSMethod) produces cljs.core.println.call(null,.aJSMethod);, which is not valid JavaScript.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 2:49 PM ]

For clarity, perhaps this should not be considered dotted interop. Dotted interop looks like (. "a" toUpperCase), for which we can alternatively rely on macroexpansion to write idiomatically as (.toUpperCase "a")

My preference would be to view (println .aJSMethod) as a call where .aJSMethod is a symbol. If ClojureScript follows the definition of symbols at https://clojure.org/reference/reader#_symbols, then an argument can be made that .aJSMethod is a reserved symbol.

Clojure doesn't appear to have a mechanism that prohibits the use of reserved symbols. For example, you can do the following in Clojure.

(def .aJSMethod "hello")
(println .aJSMethod)

Perhaps ClojureScript could be revised to also work like Clojure for such cases. Or, alternatively, perhaps an analysis error could be triggered for reserved symbols not in operator position.

Comment by Mike Fikes [ 20/Nov/17 2:58 PM ]

Ahh, even Clojure won't let you go down the path of "allow symbols starting with dot" if you do:

(def .quux inc)
(.quux 1)

So, I'd argue for treating things like (println .aJSMethod) and (def .aJSMethod "hello") as invalid code, with the underlying reason being the attempted use of a reserved symbol.





[CLJS-2397] Multi-arity function instrumentation fails with :static-fns true Created: 12/Nov/17  Updated: 20/Nov/17

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

Type: Defect Priority: Major
Reporter: Jeaye Wilkerson Assignee: Mike Fikes
Resolution: Unresolved Votes: 0
Labels: cljs, spec
Environment:

GNU/Linux


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

 Description   

The following code, in a cljc file, will pass all tests in Clojure, as well as ClojureScript with :optimizations :none, but will fail the second assertion when :optimizations :advanced is enabled.

(ns cljs-arity-spec-issue.core-test
  (:require #?@(:clj [[clojure.test :refer :all]
                      [clojure.spec.alpha :as s]
                      [clojure.spec.test.alpha :as st]]

              :cljs [[cljs.test
                      :refer-macros [deftest testing is use-fixtures]]
                     [cljs.spec.alpha :as s]
                     [cljs.spec.test.alpha :as st]])))

(defn arities
  ([a]
   (inc a))
  ([a b]
   (+ a b))
  ([a b c]
   0))
(s/fdef arities
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(deftest arities'
  (testing "Arity-1 Positive"
    (is (arities 1)))
  (testing "Arity-1 Negative"
    (is (thrown? #?(:clj RuntimeException :cljs :default)
                 (arities "bad"))))) ; This test fails with :advanced enabled


 Comments   
Comment by Jeaye Wilkerson [ 12/Nov/17 6:10 PM ]

It may be helpful to note that I've found this is likely not related to the spec, but instead the defn itself. My reasoning is that, if the defn is changed to single-arity, while the spec is left the same, the issue goes away.

(ns cljs-arity-spec-issue.core-test
  (:require #?@(:clj [[clojure.test :refer :all]
                      [clojure.spec.alpha :as s]
                      [clojure.spec.test.alpha :as st]]

              :cljs [[cljs.test
                      :refer-macros [deftest testing is use-fixtures]]
                     [cljs.spec.alpha :as s]
                     [cljs.spec.test.alpha :as st]])))

(defn arities [a]
  (inc a))
(s/fdef arities ; Same spec, but only a single arity function.
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(deftest arities' ; All tests will pass now.
  (testing "Arity-1 Positive"
    (is (arities 1)))
  (testing "Arity-1 Negative"
    (is (thrown? #?(:clj RuntimeException :cljs :default)
                 (arities "bad")))))
Comment by Thomas Heller [ 13/Nov/17 4:31 AM ]

I would suspect that is more likely related to :static-fns. Since the arity is known the compiler will directly dispatch to the appropriate fn instead of going though the dispatch fn which `fdef` might not cover?

Can you try with :none and :static-fns true?

Comment by Mike Fikes [ 13/Nov/17 5:55 AM ]

Minimal complete repro:

Put the following in src/hello_world/core.cljs:

(ns hello-world.core
  (:require [cljs.nodejs :as nodejs]
            [clojure.spec.alpha :as s]
            [clojure.spec.test.alpha :as st]))

(nodejs/enable-util-print!)

(defn arities
  ([a]
   (inc a))
  ([a b]
   (+ a b))
  ([a b c]
   0))

(s/fdef arities
        :args (s/or :arity-1 (s/cat :a number?)
                    :arity-2 (s/cat :a number? :b number?)
                    :arity-3 (s/cat :a string? :b boolean? :c map?))
        :ret number?)

(st/instrument)

(defn -main [& args]
  (try
    (prn (arities "bad"))
    (catch :default ex
      (prn ex))))

(set! *main-cli-fn* -main)

And this in node.clj:

(require 'cljs.build.api)

(cljs.build.api/build "src"
  {:main 'hello-world.core
   :output-to "main.js"
   :optimizations :none
   :static-fns true
   :target :nodejs})

With a copy of the shipping cljs.jar copied next to node.clj, compile with

java -cp cljs.jar:src clojure.main node.clj

and run with

node main.js

By revising node.clj and compling/running, you can see that it is :static-fns rather than :advanced. In particular, you can set :static-fns to false to override its setting under :advanced and instrumentation of this multi-arity function will work under :advanced.

As Thomas points out, this makes complete sense given the way that instrument works: It wraps the top-level function pointed to by the Var, not the individual direct-arity dispatch variants. (Take a look at how instrument-1 and instrument-1* work to set! the value of the Var to be a wrapped version of the original.)

It seems like this could either be fixed by detecting this situation and instead wrapping each of the dispatch variants, or by perhaps simply detecting when instrument is called while :static-fns is enabled and emitting a diagnostic (essentially indicating that this mode is unsupported).

Comment by Mike Fikes [ 14/Nov/17 10:10 AM ]

The attached CLJS-2397-1.patch is attached for discussion. It takes the approach of not fixing the issue by actually instrumenting each direct dispatch fn, but instead emitting a diagnostic and filtering for the problematic cases. It is odd in that it abuses things by triggering an analyzer warning, and it likewise doesn't attempt to change the return value of cljs.spec.test.alpha/instrumentable-vars, but it might be an approach with merit (especially, since instrumentation is often done during dev only).

Here is an illustration of how the patch behaves with :static-fns true:

ClojureScript Node.js REPL server listening on 50413
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as st])
nil
cljs.user=> (defn f [x] 1)
#'cljs.user/f
cljs.user=> (s/fdef f :args any?)
cljs.user/f
cljs.user=> (defn g ([x] 1) ([x y] 2))
#'cljs.user/g
cljs.user=> (s/fdef g :args any?)
cljs.user/g
cljs.user=> (defn h ([x] 1) ([x & ys] 2))
#'cljs.user/h
cljs.user=> (s/fdef h :args any?)
cljs.user/h
cljs.user=> (st/instrument)
WARNING: cljs.user/h cannot be instrumented (it is variadic and :static-fns is enabled) in file <cljs repl>
WARNING: cljs.user/g cannot be instrumented (it is multi-arity and :static-fns is enabled) in file <cljs repl>
[cljs.user/f]

Note that, in the above, the return value for st/instruement only reflects that f has been instrumented.

Here is the same without :static-fns true:

ClojureScript Node.js REPL server listening on 51318
To quit, type: :cljs/quit
cljs.user=> (require '[clojure.spec.alpha :as s] '[clojure.spec.test.alpha :as st])
nil
cljs.user=> (defn f [x] 1)
#'cljs.user/f
cljs.user=> (s/fdef f :args any?)
cljs.user/f
cljs.user=> (defn g ([x] 1) ([x y] 2))
#'cljs.user/g
cljs.user=> (s/fdef g :args any?)
cljs.user/g
cljs.user=> (defn h ([x] 1) ([x & ys] 2))
#'cljs.user/h
cljs.user=> (s/fdef h :args any?)
cljs.user/h
cljs.user=> (st/instrument)
[cljs.user/h cljs.user/f cljs.user/g]




[CLJS-2402] Duplicate source files passed to Closure Compiler causes ModuleLoader#resolvePaths to throw "Duplicate module path after resolving" Created: 19/Nov/17  Updated: 20/Nov/17

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

Type: Defect Priority: Major
Reporter: Corin Lawson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: modules, npm-deps

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

 Description   

I have a small (not minimal) repo here: https://github.com/au-phiware/boot-parsets

The repo includes a es6 module and a cljs file that both depend on a node module, which produces the following error.

Writing main.cljs.edn...
Compiling ClojureScript...
• main.js
                                       java.lang.Thread.run                  Thread.java:  748
         java.util.concurrent.ThreadPoolExecutor$Worker.run      ThreadPoolExecutor.java:  617
          java.util.concurrent.ThreadPoolExecutor.runWorker      ThreadPoolExecutor.java: 1142
                        java.util.concurrent.FutureTask.run              FutureTask.java:  266
                                                        ...                                   
                        clojure.core/binding-conveyor-fn/fn                     core.clj: 2022
                              adzerk.boot-cljs/compile-1/fn                boot_cljs.clj:  160
                                   adzerk.boot-cljs/compile                boot_cljs.clj:   72
                                          boot.pod/call-in*                      pod.clj:  413
                                                        ...                                   
org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke  ClojureRuntimeShimImpl.java:  102
org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke  ClojureRuntimeShimImpl.java:  109
                                                        ...                                   
                                          boot.pod/call-in*                      pod.clj:  410
                                      boot.pod/eval-fn-call                      pod.clj:  359
                                         clojure.core/apply                     core.clj:  657
                                                        ...                                   
                         adzerk.boot-cljs.impl/compile-cljs                     impl.clj:  151
                                       cljs.build.api/build                      api.clj:  205
                                         cljs.closure/build                  closure.clj: 2595
                             cljs.closure/handle-js-modules                  closure.clj: 2496
                            cljs.closure/process-js-modules                  closure.clj: 2389
                            cljs.closure/convert-js-modules                  closure.clj: 1680
                com.google.javascript.jscomp.Compiler.parse                Compiler.java:  995
          com.google.javascript.jscomp.Compiler.parseInputs                Compiler.java: 1731
      com.google.javascript.jscomp.deps.ModuleLoader.<init>            ModuleLoader.java:   92
com.google.javascript.jscomp.deps.ModuleLoader.resolvePaths            ModuleLoader.java:  276
java.lang.IllegalArgumentException: Duplicate module path after resolving: /home/corin/Projects/Demos/boot-parsets/node_modules/d3/d3.js
        clojure.lang.ExceptionInfo: Duplicate module path after resolving: /home/corin/Projects/Demos/boot-parsets/node_modules/d3/d3.js
    from: :boot-cljs
        clojure.lang.ExceptionInfo: Duplicate module path after resolving: /home/corin/Projects/Demos/boot-parsets/node_modules/d3/d3.js
    line: 33

Run `boot cljs` to reproduce the issue.

The patch attach removes duplicates from the set of input source files before they are preprocessed. With this patch the repo compiles correctly.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 6:05 PM ]

Hi Corin,

  1. Have you signed the CA? (Your name doesn't appear on https://clojure.org/community/contributors)
  2. Can you provide a minimal repro that doesn't employ downstream tooling? (Bug-filing details are at https://clojurescript.org/community/reporting-issues)
Comment by Corin Lawson [ 19/Nov/17 6:27 PM ]

1. I have signed the CA (I did it after filing this bug).
2. Not a problem, I'll get on to that later today. Is it okay to link to the project on GitHub, or should I upload a tarball?

Comment by Mike Fikes [ 19/Nov/17 6:48 PM ]

Hi Corin, it is not OK to link to GitHub, and any repro should not make use of any downstream tooling (Leiningen, Boot, etc.)—this means that a repro would ideally depend only on the shipping cljs.jar, executable like the examples at Quick Start https://clojurescript.org/guides/quick-start

Comment by Mike Fikes [ 19/Nov/17 6:59 PM ]

Hey Corin, you may want to submit the patch using the instructions at https://clojurescript.org/community/patches (your current patch won't apply using git am, which is what I suspect David uses in the end.

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

lein test is failing when applying patch

FAIL in (commonjs-module-processing) (module_processing_tests.clj:54)
processed modules are added to :libs
expected: (= {:foreign-libs [], :ups-foreign-libs [], :libs [(test/platform-path "out/src/test/cljs/reactJS.js") (test/platform-path "out/src/test/cljs/Circle.js")], :closure-warnings {:non-standard-jsdoc :off}} (env/with-compiler-env cenv (closure/process-js-modules {:foreign-libs [{:file "src/test/cljs/reactJS.js", :provides ["React"], :module-type :commonjs} {:file "src/test/cljs/Circle.js", :provides ["Circle"], :module-type :commonjs, :preprocess :jsx}], :closure-warnings {:non-standard-jsdoc :off}})))
  actual: (not (= {:foreign-libs [], :ups-foreign-libs [], :libs ["out/src/test/cljs/reactJS.js" "out/src/test/cljs/Circle.js"], :closure-warnings {:non-standard-jsdoc :off}} {:foreign-libs [], :closure-warnings {:non-standard-jsdoc :off}, :libs ["out/src/test/cljs/Circle.js" "out/src/test/cljs/reactJS.js"], :ups-foreign-libs []}))
Comment by Corin Lawson [ 19/Nov/17 7:48 PM ]

Thanks Mike,

I will make an effort follow all those instructions, but I do not see how I should provide the repro (no links, no attachments)... should it be inline code blocks?

Also, I shall be sure to run lein test before submitting the next patch. I note that the difference is only the order of the items in :libs vector, can you advise if order is important?

Comment by Mike Fikes [ 20/Nov/17 8:11 AM ]

Hi Corin,

Yes, inline code blocks are great. Anything minimal that doesn't depend on more than the shipping cljs.jar to demonstrate the issue (either directly in its REPL or via compiling using the build API). Here is a recent example using the build API: https://dev.clojure.org/jira/browse/CLJS-2397?focusedCommentId=47278&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-47278

There are actually other tests as well beyond lein test. See https://clojurescript.org/community/running-tests

I haven't looked into the details of this issue, so can't speak to to whether order of items is important.

Comment by Corin Lawson [ 20/Nov/17 8:47 AM ]

Steps to reproduce the problem.

Consider the following three source files:

src/distinct_inputs/core.cljs
(ns distinct-inputs.core
  (:require [d3]
            [circle :refer [circle]]))

(-> d3
    (.select "body")
    (.append "svg")
    (.attr "width" 200)
    (.attr "height" 200)
    (.call circle "steelblue"))
es6/circle.js
import * as d3 from 'd3';

export function circle(sel, color) {
  return sel
    .append("circle")
    .attr("cx", "100px")
    .attr("cy", "100px")
    .attr("r",  "100px")
    .attr("fill", color);
}
build.clj
(require 'cljs.build.api)

(cljs.build.api/build
  "src"
  {:main 'distinct-inputs.core
   :output-to "out/main.js"
   :install-deps true
   :foreign-libs [{:file "es6"
                   :module-type :es6}]
   :npm-deps     {:d3 "3.5.16"}})

Execute cljs:

java -cp cljs.jar:src clojure.main build.clj

Expected outcome

cljs should produce nothing to the standard output, exit cleanly and write the following files (approximately).

out/main.js
var CLOSURE_UNCOMPILED_DEFINES = {};
var CLOSURE_NO_DEPS = true;
if(typeof goog == "undefined") document.write('<script src="out/goog/base.js"></script>');
document.write('<script src="out/goog/deps.js"></script>');
document.write('<script src="out/cljs_deps.js"></script>');
document.write('<script>if (typeof goog == "undefined") console.warn("ClojureScript could not load :main, did you forget to specify :asset-path?");</script>');
document.write('<script>goog.require("process.env");</script>');
document.write('<script>goog.require("distinct_inputs.core");</script>');
out/distinct_inputs/core.js
goog.provide('distinct_inputs.core');
goog.require('cljs.core');
goog.require('module$distinct_inputs$node_modules$d3$d3');
goog.require('module$distinct_inputs$es6$circle');
module$distinct_inputs$node_modules$d3$d3.select("body").append("svg").attr("width",(200)).attr("height",(200)).call(module$distinct_inputs$es6$circle.circle,"steelblue");

//# sourceMappingURL=core.js.map
out/es6/circle.js
goog.provide("module$distinct_inputs$es6$circle");goog.require("module$distinct_inputs$node_modules$d3$d3");function circle$$module$distinct_inputs$es6$circle(sel,color){return sel.append("circle").attr("cx","100px").attr("cy","100px").attr("r","100px").attr("fill",color)}module$distinct_inputs$es6$circle.circle=circle$$module$distinct_inputs$es6$circle

Actual outcome.

cljs exits with exit code 1 and produces the following standard out.

stdout
Exception in thread "main" java.lang.IllegalArgumentException: Duplicate module path after resolving: /distinct_inputs/node_modules/d3/d3.js, compiling:(/distinct_inputs/build.clj:3:1)
	at clojure.lang.Compiler.load(Compiler.java:7391)
	at clojure.lang.Compiler.loadFile(Compiler.java:7317)
	at clojure.main$load_script.invokeStatic(main.clj:275)
	at clojure.main$script_opt.invokeStatic(main.clj:335)
	at clojure.main$script_opt.invoke(main.clj:330)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Duplicate module path after resolving: /distinct_inputs/node_modules/d3/d3.js
	at com.google.javascript.jscomp.deps.ModuleLoader.resolvePaths(ModuleLoader.java:276)
	at com.google.javascript.jscomp.deps.ModuleLoader.<init>(ModuleLoader.java:92)
	at com.google.javascript.jscomp.Compiler.parseInputs(Compiler.java:1731)
	at com.google.javascript.jscomp.Compiler.parse(Compiler.java:995)
	at cljs.closure$convert_js_modules.invokeStatic(closure.clj:1680)
	at cljs.closure$process_js_modules.invokeStatic(closure.clj:2371)
	at cljs.closure$handle_js_modules.invokeStatic(closure.clj:2495)
	at cljs.closure$build.invokeStatic(closure.clj:2592)
	at cljs.build.api$build.invokeStatic(api.clj:204)
	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 user$eval24.invokeStatic(build.clj:3)
	at user$eval24.invoke(build.clj:3)
	at clojure.lang.Compiler.eval(Compiler.java:6927)
	at clojure.lang.Compiler.load(Compiler.java:7379)
	... 11 more

None of the aforementioned expected files are produced.

Cause of the exception.

The exception emitted by ModuleLoader#resolvePaths is a result of the same input file (i.e. node_modules/d3/d3.js) having been specified more than once to Compiler#initModules. There happens to be this note in Compiler#getAllInputsFromModules:

src/com/google/javascript/jscomp/Compiler.java
// NOTE(nicksantos): If an input is in more than one module,
        // it will show up twice in the inputs list, and then we
        // will get an error down the line.

cljs.closure/process-js-modules is provided a :foreign-libs vector which contains a repeated entry for node_modules/d3/d3.js (and also it's package.json). That vector is a result of multiple invocations of cljs.closure/node-inputs; once for out/cljs$node_modules.js (which is presumably a result of the dependency in distinct_inputs/core) and again for es6/circle.js.

In short, the dependency on D3 is pulled in by both ClojureScript source files and JavaScript module source files.

Proposed solution.

In this scenario the :foreign-libs vector contains repeated entries dispite the use of distinct within cljs.closure/node-inputs. A possible solution would to remove the use of distinct within cljs.closure/node-inputs and require the caller of cljs.closure/node-inputs to use distinct.

Solution A
From 063e35080c14d35189ab7827f25f071e958ab5b4 Mon Sep 17 00:00:00 2001
From: Corin Lawson <corin@responsight.com>
Date: Tue, 21 Nov 2017 01:31:53 +1100
Subject: [PATCH] CLJS-2402: Ensure :foreign-libs vector contains distinct
 entries.

---
 src/main/clojure/cljs/closure.clj | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/main/clojure/cljs/closure.clj b/src/main/clojure/cljs/closure.clj
index a686f878..74a0cc86 100644
--- a/src/main/clojure/cljs/closure.clj
+++ b/src/main/clojure/cljs/closure.clj
@@ -2219,7 +2219,7 @@
      (when env/*compiler*
        (:options @env/*compiler*))))
   ([entries opts]
-   (into [] (distinct (mapcat #(node-module-deps % opts) entries)))))
+   (into [] (mapcat #(node-module-deps % opts) entries))))
 
 (defn index-node-modules
   ([modules]
@@ -2480,14 +2480,15 @@
         output-dir (util/output-directory opts)
         opts (update opts :foreign-libs
                (fn [libs]
-                 (into (if (= target :nodejs)
-                         []
-                         (index-node-modules node-required))
-                   (into expanded-libs
-                     (node-inputs (filter (fn [{:keys [module-type]}]
-                                            (and (some? module-type)
-                                              (not= module-type :amd)))
-                                    expanded-libs))))))
+                 (distinct
+                   (into (if (= target :nodejs)
+                           []
+                           (index-node-modules node-required))
+                         (into expanded-libs
+                               (node-inputs (filter (fn [{:keys [module-type]}]
+                                                      (and (some? module-type)
+                                                           (not= module-type :amd)))
+                                                    expanded-libs)))))))
         opts (if (some
                    (fn [ijs]
                      (let [dest (io/file output-dir (rel-output-path (assoc ijs :foreign true) opts))]
-- 
2.13.0

A more general solution is cljs.closure/process-js-modules must ensure the set of input files (i.e. js-modules) is distinct. This patch would be simpler (i.e. doesn't mess with code I don't understand) and closer to the call to Google Closure Compiler.

Solution B
From 6bf11a24b93642e118e6d29c5af8a137fa01ea94 Mon Sep 17 00:00:00 2001
From: Corin Lawson <corin@responsight.com>
Date: Sun, 19 Nov 2017 20:25:31 +1100
Subject: [PATCH] CLJS-2402: Ensure input source files are distinct.

---
 src/main/clojure/cljs/closure.clj | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/main/clojure/cljs/closure.clj b/src/main/clojure/cljs/closure.clj
index a686f878..24421bde 100644
--- a/src/main/clojure/cljs/closure.clj
+++ b/src/main/clojure/cljs/closure.clj
@@ -2364,7 +2364,8 @@
   (let [;; Modules from both :foreign-libs (compiler options) and :ups-foreign-libs (deps.cljs)
         ;; are processed together, so that files from both sources can depend on each other.
         ;; e.g. commonjs module in :foreign-libs can depend on commonjs module from :ups-foreign-libs.
-        js-modules (filter :module-type (concat (:foreign-libs opts) (:ups-foreign-libs opts)))]
+        js-modules (filter :module-type (concat (:foreign-libs opts) (:ups-foreign-libs opts)))
+        js-modules (distinct js-modules)]
     (if (seq js-modules)
       (util/measure (:compiler-stats opts)
         "Process JS modules"
-- 
2.13.0

FWIW: I prefer Solution B.

Comment by Corin Lawson [ 20/Nov/17 8:49 AM ]

Attached proposed Solution B

Comment by Corin Lawson [ 20/Nov/17 9:02 AM ]

Hi Mike,

I hope this is to your's (and BDFL's) satisfaction now; I ran lein test for both proposed solutions and I do not receive any failures. I do receive errors, however, that do not occur in assertions. I assume that the cause is something peculiar (or lack thereof) in my setup. Let me know if you require anything else from me.

Cheers,
Corin.

Comment by Mike Fikes [ 20/Nov/17 12:08 PM ]

Thanks Corin. The entire test suite passes for me with your latest patch.





[CLJS-2379] Print negative zero as -0.0 Created: 06/Oct/17  Updated: 20/Nov/17  Resolved: 20/Nov/17

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

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

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

 Description   

In Clojure, negative zero is printed as -0.0 (for example when evaluating -0.0).

This ticket asks for the same for ClojureScript.

Note: The question may arise: Why not print as -0? The ClojureScript reader behaves similarly to Clojure's in that -0 is read in as an integer value, which doesn't have signed zeros, and is thus read in as 0. To properly round trip, this ticket asks that negative zero be printed as -0.0 (presuming that there are to be no changes to the reader behavior.)



 Comments   
Comment by Mike Fikes [ 06/Oct/17 8:24 AM ]

Oh hey Erik, I had assigned this ticket to myself and have attached a patch. I see you attached an alternative. But, I think your alternative prints 0 as -0. Is that correct?

Comment by Erik Assum [ 06/Oct/17 8:27 AM ]

Yeah, I didn't see you assigned to yourself, so I made a patch solving the description as it was (when it specified -0.

Comment by Mike Fikes [ 06/Oct/17 9:05 AM ]

The attached patch doesn't attempt to make (str -0.0) match Clojure (presuming that changing str could pose a perf challenge). But, it does give us round tripping for -0.0.

Here are perf benchmarks:

Speedup summary:

                 true     10  ##NaN  ##Inf ##-Inf (js-obj)  -0.0      0
            V8:  1.02   0.97   1.00   1.02   1.04    1.01   1.00   1.00
  SpiderMonkey:  0.98   1.04   1.00   1.01   0.98    1.02   1.05   1.03
JavaScriptCore:  1.00   1.01   1.02   1.01   1.01    1.04   1.04   1.00
       Nashorn:  0.98   1.13   0.96   0.95   0.99    0.92   0.99   0.98 
    ChakraCore:  1.02   1.03   1.03   1.02   1.01    0.99   1.07   1.07

Before:

Before:

Benchmarking with V8
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 386 msecs
[x 10], (pr-str x), 1000000 runs, 561 msecs
[x js/NaN], (pr-str x), 1000000 runs, 641 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 634 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 593 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 898 msecs
[x -0.0], (pr-str x), 1000000 runs, 449 msecs
[x 0], (pr-str x), 1000000 runs, 530 msecs

Benchmarking with SpiderMonkey
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1321 msecs
[x 10], (pr-str x), 1000000 runs, 1330 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1293 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1307 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1297 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2143 msecs
[x -0.0], (pr-str x), 1000000 runs, 1340 msecs
[x 0], (pr-str x), 1000000 runs, 1312 msecs

Benchmarking with JavaScriptCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 502 msecs
[x 10], (pr-str x), 1000000 runs, 510 msecs
[x js/NaN], (pr-str x), 1000000 runs, 652 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 698 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 714 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 1266 msecs
[x -0.0], (pr-str x), 1000000 runs, 788 msecs
[x 0], (pr-str x), 1000000 runs, 562 msecs

Benchmarking with Nashorn
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1746 msecs
[x 10], (pr-str x), 1000000 runs, 1498 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1256 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1251 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1302 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2905 msecs
[x -0.0], (pr-str x), 1000000 runs, 1342 msecs
[x 0], (pr-str x), 1000000 runs, 1383 msecs

Benchmarking with ChakraCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1254 msecs
[x 10], (pr-str x), 1000000 runs, 1190 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1118 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1119 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1124 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2420 msecs
[x -0.0], (pr-str x), 1000000 runs, 1183 msecs
[x 0], (pr-str x), 1000000 runs, 1192 msecs

After:

After:

Benchmarking with V8
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 377 msecs
[x 10], (pr-str x), 1000000 runs, 577 msecs
[x js/NaN], (pr-str x), 1000000 runs, 642 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 623 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 570 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 887 msecs
[x -0.0], (pr-str x), 1000000 runs, 448 msecs
[x 0], (pr-str x), 1000000 runs, 530 msecs

Benchmarking with SpiderMonkey
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1354 msecs
[x 10], (pr-str x), 1000000 runs, 1282 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1320 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1298 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1322 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2093 msecs
[x -0.0], (pr-str x), 1000000 runs, 1280 msecs
[x 0], (pr-str x), 1000000 runs, 1277 msecs

Benchmarking with JavaScriptCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 504 msecs
[x 10], (pr-str x), 1000000 runs, 507 msecs
[x js/NaN], (pr-str x), 1000000 runs, 639 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 690 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 708 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 1216 msecs
[x -0.0], (pr-str x), 1000000 runs, 759 msecs
[x 0], (pr-str x), 1000000 runs, 563 msecs

Benchmarking with Nashorn
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1778 msecs
[x 10], (pr-str x), 1000000 runs, 1323 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1303 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1311 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1309 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 3168 msecs
[x -0.0], (pr-str x), 1000000 runs, 1360 msecs
[x 0], (pr-str x), 1000000 runs, 1405 msecs

Benchmarking with ChakraCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1233 msecs
[x 10], (pr-str x), 1000000 runs, 1150 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1084 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1094 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1117 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2445 msecs
[x -0.0], (pr-str x), 1000000 runs, 1106 msecs
[x 0], (pr-str x), 1000000 runs, 1119 msecs
Comment by David Nolen [ 29/Oct/17 8:35 AM ]

After some thought my primary concern here is that change is coming quite late and is likely to break tests.

Comment by Mike Fikes [ 29/Oct/17 8:44 AM ]

That's a good point for which I don't see a strong counterargument.

This ticket captures perf and pro/con rationales in case the issue comes up in the future, but perhaps for now this ticket should be closed as declined.





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

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

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

Attachments: Text File 0001-CLJS-1631-Make-str-handle-js-symbols.patch    
Patch: Code and Test

 Description   

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

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

Calling the toString method on a symbol directly works

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


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

Unit tests fail in (presumably older environments) in Travis:

JavaScriptCore:

ERROR in (test-cljs-1631) (core-advanced-test.js:3724:60)
expected: (= "Symbol(x)" (str (.for js/Symbol "x")))
  actual: #object[ReferenceError ReferenceError: Can't find variable: Symbol]

Nashorn:

ERROR in (test-cljs-1631) (ReferenceError:NaN:NaN)
expected: (= "Symbol(x)" (str (.for js/Symbol "x")))
  actual: #object[ReferenceError ReferenceError: "Symbol" is not defined]

Since this appears to only affect the tests, and not the production code, perhaps the tests could be conditional on (exists? js/Symbol).

Comment by Thomas Heller [ 20/Nov/17 6:43 AM ]

There is a lot of history behind the str fn/macro and why they do what they to.

See: https://dev.clojure.org/jira/browse/CLJS-890

The patch just uses toString which is not an option until we sort out CLJS-890.





[CLJS-1458] re-matches might give false negative when using match groups Created: 25/Sep/15  Updated: 20/Nov/17

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

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1458-re-matches-might-give-false-negative-2.patch     Text File cljs-1458-re-matches-might-give-false-negative.patch    
Patch: Code and Test

 Description   

Current behaviour:

(re-matches #"(a|aa)" "aa") => nil

Expected:

(re-matches #"(a|aa)" "aa") => ["aa" "aa"]

JVM version works as expected, only CLJS is affected



 Comments   
Comment by David Nolen [ 14/Oct/15 11:36 AM ]

This is the kind of ticket that tends to break existing code. We should get some people who are interested in this ticket to actually try it out.

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

FWIW, I gave cljs-1458-re-matches-might-give-false-negative.patch a try in bootstrapped ClojureScript and it is working fine there (each of the additional unit tests produce the expected results in bootstrapped).

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

Patch no longer applies.

Comment by Nikita Prokopov [ 20/Nov/17 3:47 AM ]

New patch





[CLJS-2319] cljs.core/mod handling of floats inconsistent with Clojure & JavaScript Created: 13/Aug/17  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: André Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2319-Fix-cljs.core-mod-handling-of-floats_INLINED.patch     Text File CLJS-2319-Fix-cljs.core-mod-handling-of-floats.patch    
Patch: Code and Test

 Description   

The workaround for negative numbers (https://dev.clojure.org/jira/browse/CLJS-417) results in annoying behavior for floats:

(mod 2.1 3) ; => 2.0999999999999996

Both Clojure and the standard JavaScript modulo return the expected 2.1 here.

Two possible solutions:

  • only do the double-mod workaround when the dividend is actually negative
  • check if the dividend is smaller than the divisor and just return it in that case


 Comments   
Comment by David Nolen [ 13/Aug/17 5:00 PM ]

Patch welcome.

Comment by André Wagner [ 14/Aug/17 8:00 AM ]

The patch renames cljs.core/mod to double-mod and redefines mod to invoke js-mod directly when both args have the same sign.
It includes test cases for the previously failing cases, but I've also tested more exhaustively against the Clojure impl: https://gist.github.com/aw7/a32bd69923c65bddc23fd63ee062833c

Comment by David Nolen [ 14/Aug/17 8:46 AM ]

Great thanks, have you submitted your Clojure CA?

Comment by André Wagner [ 14/Aug/17 9:06 AM ]

Yeah, 2h ago.

Comment by António Nuno Monteiro [ 14/Aug/17 5:42 PM ]

Nit: shouldn't the `double-mod` function be marked as private then?

Comment by André Wagner [ 15/Aug/17 4:06 AM ]

I guess there's no need for `double-mod` to exist as a separate function at all, I've added a patch where it's inlined into `mod`.

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

André, ClojureScript requires squashed patches (CLJS-2319-Fix-cljs.core-mod-handling-of-floats_INLINED.patch has two patches in it). See https://clojurescript.org/community/patches

Comment by Mike Fikes [ 19/Nov/17 9:13 PM ]

Arg... something in the patch is killing the older JavaScriptCore we run tests with in Travis. My only guess is the additional mod tests may be triggering CLJS-910, but that seems unlikely.





[CLJS-2360] ClojureScript ignores first two arguments passed to a macro when using vargs Created: 13/Sep/17  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Ethan McCue Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug


 Description   

The following code produces different results in clojure and in clojurescript

(defmacro beep [& args]
  (cons 'list args))
(print (beep 0 1 2 3))

In clojure that code outputs (0 1 2 3)
In clojurescript that code outputs (2 3)



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

Ethan, I suspect this ticket is invalid. Are you defining the macro in the REPL? If you do that, you will see the consequence that macros are really just functions called by the compiler (with two extra special arguments &env and &form, which you are passing as 0 and 1 in your example code).

Macros in ClojureScript need to be defined in a separate namespace and consumed using :require-macros. See more at https://clojurescript.org/about/differences#_macros





[CLJS-2365] Self-host: Unable to reload namespace while in it Created: 18/Sep/17  Updated: 19/Nov/17  Resolved: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bootstrap


 Description   

There is an unreleased self-host regression where requiring a namespace while in that namespace triggers circular dependency detection.

As a concrete example, let's say you are in a REPL, and you require a namespace, go into that namespace (using in-ns), exercise it a little, and then change the code to fix something and then reload it. This now fails on master for self-hosted code.

A repro following this example is the following:

(require 'cljs.js)

(def st (cljs.js/empty-state))

(cljs.js/eval st
  '(require (quote foo.core))
  {:context :expr
   :eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj
                :source "(ns foo.core) (def x 1)"}))}
  prn)

(cljs.js/eval st
  '(require (quote foo.core) :reload)
  {:context :expr
   :ns 'foo.core
   :eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj
                :source "(ns foo.core) (def x 2)"}))}
  prn)

This causes the following on master, but not with the 1.9.908 release:

{:error #error {:message "Circular dependency detected foo.core -> foo.core", :data {:tag :cljs/analysis-error}}}

(Strictly speaking, the above example is not minimal in that :reload is not required in order to reproduce it: It will happen if you simply attempt to require the namespace while "in" it.)



 Comments   
Comment by Mike Fikes [ 18/Sep/17 10:19 AM ]

Git bisect result implicates CLJS-2356:

238028ccc51afe45de98fb853be4396a71afb602 is the first bad commit
commit 238028ccc51afe45de98fb853be4396a71afb602
Author: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Date:   Sun Sep 10 21:24:22 2017 -0700

    CLJS-2356: Self-host: circular dependency detection is not correct

:040000 040000 a93d466e3f1ea042e730fb78ca911f92319880d0 29108cab4b45247e641d430d85dda85c436e95fe M	src
Comment by Mike Fikes [ 18/Sep/17 3:22 PM ]

Here is the result from some analysis by António and me: There are a few places in the self-host compiler code which make use of :def-emits-var to deduce that self-host is being used to implement a REPL. In particular, the code being exercised in the ticket description works if you include :def-emits-var true in the options passed. But, if you are evaluating a "load" form, such as require and others, if :def-emits-var is set, then it can be seen that code inside the loaded namespaces gets compiled with def (and derived) forms producing Vars. (This can be seen if you make use of the self-hosted caching option.) Perhaps this is incorrect behavior (I believe it doesn't occur with JVM ClojureScript, for example). With the current behavior, if a REPL chooses to not set :def-emits-var true for "load" forms, then you encounter the issue described in the ticket description. So, in short, the underlying issue in this ticket could either be fixed or deemed OK, and the other issue mentioned here regarding def forms inside required namespaces could be split off as a separate ticket that could be pursued on its own.

Comment by Mike Fikes [ 20/Sep/17 1:10 PM ]

With CLJS-2367, I suspect this ticket becomes a non-issue, because REPLs can unconditionally (always) set :def-emits-var.

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

Retracting this ticket as I no longer see it as an issue.





[CLJS-2398] cljs.core.Var shouldn't satisfy IWithMeta Created: 12/Nov/17  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File 0001-Remove-IWithMeta-implementation-from-Var.patch    
Patch: Code

 Description   

cljs.core.Var implements IWithMeta, which differs from vars in Clojure which do not have immutable metadata. with-meta shouldn't be supported on vars, only alter-meta! should.

Unlike Clojure, ClojureScript doesn't use dynamic dispatch for alter-meta!. The implementation of cljs.core/alter-meta! will happily mutate the metadata of any object with a "meta" field, even if it's not a ref/var/whatever identity. This means that if you want to have metadata on your custom type, you have to have a "meta" field, rather than implement methods for getting/changing metadata. That's not too ideal, but doesn't present an immediate problem for me.

The reason I care about IWithMeta is that I need to distinguish metadata handling between values and reference types. Ideally, there would be an IAlterMeta protocol as well, but (and (satisfies? IMeta x) (not (satisfies? IWithMeta x))) suffices. On the JVM, the same can be done with IMeta and IObj.



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

Unit test failure:

ERROR in (test-1248) (core-advanced-test.js:64:417)
Uncaught exception, not in assertion.
expected: nil
  actual: #object[Error Error: No protocol method IWithMeta.-with-meta defined for type object: #'cljs.core/first]




[CLJS-633] Missing Java type hints in closure.clj Created: 22/Oct/13  Updated: 19/Nov/17

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

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

Attachments: Text File cljs_ticket_633.patch    
Patch: Code

 Description   

A number of reflection warnings are generated from closure.clj. Fixing those probably would not improved performance but it would be cleaner anyway.



 Comments   
Comment by Julien Eluard [ 22/Oct/13 8:03 AM ]

Attached patch.

There are a couple warnings left, similar to:
Reflection warning, cljs/closure.clj:232:1 - reference to field url can't be resolved.
They appear to be due to some unexpected protocol method name? It looks harmless.

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

Patch no longer applies.





[CLJS-844] Optimize js->clj by switching to transients Created: 22/Aug/14  Updated: 19/Nov/17

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

Type: Enhancement Priority: Trivial
Reporter: Darrick Wiebe Assignee: Darrick Wiebe
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File js-to-clj-using-transients.patch     Text File use-transducers-in-js-to-clj.patch     Text File use-transducers-in-js-to-clj.patch    
Patch: Code

 Comments   
Comment by David Nolen [ 23/Aug/14 1:19 PM ]

Did you any benchmarks on other JS engines?

Comment by Darrick Wiebe [ 23/Aug/14 2:14 PM ]

No. My main concern was to check if there was any startup overhead that might offset the basic efficiency improvements for processing small collections. Using into with a transducer turned out to be faster in every scenario, significantly in most.

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

It would be nice to see a jsperf of before and after on this ticket. Thanks.

Comment by Darrick Wiebe [ 23/Aug/14 2:23 PM ]

Is there a existing one that I can work from?

Comment by David Nolen [ 23/Aug/14 2:35 PM ]

There is not. I generally just make a simple project, create an advanced compiled artifact and copy and paste it into jsperf.

Comment by Darrick Wiebe [ 24/Aug/14 7:24 PM ]

Turns out reducing into a transient is considerably better than using a transducer (which was itself a little faster) for this.

http://jsperf.com/js-clj-transducer-test

The code is at:

https://gist.github.com/pangloss/591d77231fda460c2fbe

Let me know if you want me to prepare an updated patch.

Comment by David Nolen [ 25/Aug/14 7:34 AM ]

Thanks for putting this together. Yes please provide an updated patch.

Comment by Darrick Wiebe [ 26/Aug/14 11:19 AM ]

Not sure whether the convention is to comment that I've uploaded a new patch. Regardless, I uploaded it yesterday.

Comment by Marcin Kulik [ 07/Oct/15 1:46 PM ]

I have tested and benchmarked the patch on a big js objects (5MB+ json files) and I confirm that new-js->clj3 function is more than 2x faster. It runs now in the player on https://asciinema.org and all seems good so far.

Comment by David Nolen [ 08/Oct/15 2:33 PM ]

Marcin thanks for the feedback. Experience reports always help push tickets forward.

Darrick, the patch need to be rebased to master. Please remove all patches except the one that will be applied.

Comment by Rohit Aggarwal [ 08/Oct/15 3:06 PM ]

[ClojureScript beginner here. so do ignore this if I am mistaken]

The patch of 25/Aug uses `(aget x k)` instead of `(goog.object/get x k)` for getting the value of a key if x is an object. I believe it isn't the right way even though it works.

This contains why the latter is preferred over the former.

Comment by David Nolen [ 09/Oct/15 12:33 PM ]

Rohit good catch. Yes that should be changed.

Comment by Thomas Spellman [ 21/Oct/16 3:01 AM ]

Added a patch, "js-to-clj-using-transients.patch", on Oct 16, 2016 that supersedes "use-transducers-in-js-to-clj.patch" from Aug, 2014. This patch changes cljs.core/js->clj to use transients. Also included is a change to both js->clj and clj->js to use gobject/get and gobject/set instead of aget and aset on JS object.

The JSperf shows a 17% speed improvement in Chrome on the first run, but about equal on the second run: https://jsperf.com/js-clj-transient-perf-test

The code for the JSperf is here: https://gist.github.com/thos37/41c0b38bea3270988a3275332686ab49

What would be the ideal test data for this?

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

js-to-clj-using-transients.patch no longer applies.





[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: 2
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-890] Incorrect behaviour of (str obj) when obj has valueOf method Created: 24/Nov/14  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Nikita Beloglazov Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File cljs-890.patch     File cljs-core-str-perf.diff    
Patch: Code

 Description   

Example

(str #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) ; => "42"

The problem in the fact that ClojureScript uses concatenation to convert values to strings and that doesn't work well with objects which have valueOf() method overriden.

Example in js:

var obj = {
    toString: function() { return 'hello'; },
    valueOf: function() { return 42; }
};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'

Potential solution might be to use String() function. Using toString() won't work as described in this issue: http://dev.clojure.org/jira/browse/CLJS-847



 Comments   
Comment by Kevin Neaton [ 24/Nov/14 10:34 AM ]

Is there a valid use case where toString and valueOf are not in sync? E.g.

(not= (.toString x) (js/String (.valueOf x))

If not, is it "incorrect" for the two methods to be out of sync?

Comment by Nikita Beloglazov [ 24/Nov/14 10:40 AM ]

Here is an example of such use case: https://github.com/processing-js/processing-js/blob/master/src/Objects/Char.js
That's how I found this bug.

Comment by Kevin Neaton [ 24/Nov/14 10:49 AM ]

Thanks for the link. I see what you mean.

Comment by David Nolen [ 02/Dec/14 5:08 AM ]

The problem with going with String appears to be a massive performance hit to printing http://jsperf.com/string-vs-tostring2/6.

Unless a brilliant idea is proposed this seems best solved / worked around in user code.

Comment by Nikita Beloglazov [ 02/Dec/14 6:41 AM ]

Append performs better on strings and numbers, but it performs worse on objects so it is not a clear performance hit. If I heavily work with objects and use (str) to convert them into strings then I actually lose on performance with current implementation.
Anyway current implementation of str is incorrect as it doesn't honor toString method. And this is what str function supposed to do. I believe a compiler should be correct first and then worry about performance.

Comment by David Nolen [ 02/Dec/14 7:38 AM ]

Sorry going back over this I believe the issue is really that we simply need to backout CLJS-801.

Comment by David Nolen [ 02/Dec/14 7:41 AM ]

reverted CLJS-801 in master

Comment by Francis Avila [ 02/Dec/14 10:32 AM ]

CLJS-801 only deals with the str macro. Aren't we still going to have str function problem because of CLJS-847? https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 (also Yet if we use toString there, Safari 6.0.5 blows up. Maybe we need {{[o].join('')}}? Depending on where the bug is this may be wrong in Safari 6.0.5 too.

What we need to do very specifically is somehow get the return value of the (in ECMASCRIPT-ese) ToString abstract operation on the object (or the underlying ToPrimitive abstract operation with the String hint). String concat with the add operator

Options as I see it are:

  • x.toString() : Bad because of CLJS-847
  • {{[x].join('')}} : Should work (and does right thing for null/undefined), but I think we should test in Safari 6.0.5. Also very slow.
  • String
  • String.prototype.concat
  • String.prototype.slice(x,0) String.prototype.substring(x,0) String.prototype.substr(x, 0)
  • x.toString() normally, but String if we detect that we'll trigger CLJS-847. (Can specialize on startup.)
Comment by David Nolen [ 02/Dec/14 10:35 AM ]

Is there any evidence that higher usage of str is actually problematic?

Comment by Francis Avila [ 02/Dec/14 10:44 AM ]

String concat using the addition operator uses an un-hinted ToPrimitive abstract call (which will try x.valueOf() first then x.toString(), usually) and then {{ToString}}s the result of that, so it's not an option unless we are concating primitive values.

Details:

Comment by David Nolen [ 02/Dec/14 10:50 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

Comment by Francis Avila [ 02/Dec/14 11:01 AM ]

Is there any evidence that higher usage of str is actually problematic?

Kevin Neaton, who opened CLJS-847, was using a patch in production which only addressed the higher order case and he said the patch fixed the issue for them. He was unaffected by the str macro case because it either used ''+x already (with CLJS-801 applied) or it used {{[x].join('')}} (which hasn't been tested with Safari 6.0.5 yet, but probably works).

So if we had a problem using ''+x with the str macro, we will certainly have a problem with ''+x with a string function as long as CLJS-847 is applied.

I haven't pulled down master yet, but here is a test case which I bet will fail with the CLJS-847 patch:

(def tricky-obj #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)})
(assert (= (apply str tricky-obj) "hello")) ;; will get "42"
Comment by Francis Avila [ 02/Dec/14 11:09 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

To be clear, there are two issues here:

CLJS-847: x.toString() fails on Safari 6.0.5. Workaround is ''+x (only done in str macro case).
CLJS-890: ''+x doesn't give expected results for objects which define valueOf. Expectation is that x.toString() is called, instead x.valueOf().toString(). Fix is to use array join instead of string concat in str macro, but it doesn't address the ''+x workaround from CLJS-847.

To make matters worse, it looks like the toString() error on Safari may only be triggered at certain JIT levels!

Comment by Francis Avila [ 02/Dec/14 11:10 AM ]

Workaround is ''+x (only done in str macro case).

I mean "Workaround is ''+x (only done in str function case)."

Comment by Nikita Beloglazov [ 08/Dec/14 6:14 PM ]

Can this bug be reopened meanwhile? If I understand correctly the fix should affect https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 but this code still present in HEAD.

Comment by David Nolen [ 08/Dec/14 6:37 PM ]

We've switched to goog.string.buildString in master https://github.com/clojure/clojurescript/commit/94eb8a960fef6aaca4ba44b251cefbfa04d0f6ac

Comment by Nikita Beloglazov [ 08/Dec/14 8:32 PM ]

Yes, that works. Cool, thanks!

Comment by Thomas Heller [ 01/Jan/15 7:12 AM ]

Sorry for re-opening.

I was doing some profiling of my code and noticed a warning in the profiling output about cljs.core/str.

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

goog.string.buildString = function(var_args) {
  return Array.prototype.join.call(arguments, '');
};

Given that we don't ever call it with more than one argument it is probably not best implementation choice.

Maybe skip the call and inline it ala

(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x] (if (nil? x)
         ""
         (.join #js [x] "")))
  ([x & ys]
    (loop [sb (StringBuffer. (str x)) more ys]
      (if more
        (recur (. sb  (append (str (first more)))) (next more))
        (.toString sb)))))

I didn't follow this issue but why are we not using .toString? The buildString/array approach seems kind of hackish?

I'm not too sure about the overall impact but since cljs.core/str showed up pretty high in my profile I think this should be investigated further.

Comment by Thomas Heller [ 01/Jan/15 7:28 AM ]

Before:

;;; str
[], (str "1"), 1000000 runs, 254 msecs
[], (str 1), 1000000 runs, 266 msecs
[], (str nil), 1000000 runs, 80 msecs
[], (str "1" "2" "3"), 1000000 runs, 753 msecs

After:

;;; str
[], (str "1"), 1000000 runs, 82 msecs
[], (str 1), 1000000 runs, 86 msecs
[], (str nil), 1000000 runs, 79 msecs
[], (str "1" "2" "3"), 1000000 runs, 242 msecs

But I only tested V8, probably needs some verification.

Comment by Thomas Heller [ 01/Jan/15 7:39 AM ]
(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x1]
     (.join #js [x1] ""))
  ([x1 x2]
     (.join #js [x1 x2] ""))
  ([x1 x2 x3]
     (.join #js [x1 x2 x3] ""))
  ([x1 x2 x3 x4]
     (.join #js [x1 x2 x3 x4] ""))
  ...)

Does perform even better.

;;; str
[], (str "1"), 1000000 runs, 40 msecs
[], (str 1), 1000000 runs, 43 msecs
[], (str nil), 1000000 runs, 96 msecs
[], (str "1" "2" "3"), 1000000 runs, 117 msecs

How many args should it inline?

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

I'd be OK with up to 4 then variadic.

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

There is some weird interaction between the code generated by the cljs.core/str macro and function.

The macro generates

(str "hello" 1 "world" :yo nil)

yields

[cljs.core.str("hello"),cljs.core.str((1)),cljs.core.str("world"),cljs.core.str(new cljs.core.Keyword(null,"yo","yo",1207083126)),cljs.core.str(null)].join('');

Given that str with 1 arg will basically unroll to

[["hello"].join(""), ...]

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

Comment by Francis Avila [ 02/Jan/15 11:14 AM ]

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

Chrome complains about the variadic function dispatch code in the same way, see CLJS-916 plus patch.

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

The Closure compiler is not smart enough to remove the intermediate array, which is why I filed CLJS-801 (which this ticket rolled back). I don't think JITs can do it either.

I am beginning to wonder if we should ignore the Safari 6.0.5 problem in CLJS-847 that started all this string mess. To recap:

  1. CLJS-801 is accepted, which removes [123, x].join('') in the str macro case in favor of ''+123+(cljs.core/str$arity$1 x) style code, which the closure compiler can precompute. At this time, the one-arg cljs.core/str function (not macro) calls toString on its argument.
  2. CLJS-847 is filed. On Safari 6.0.5 at higher JIT levels, calling toString on some things (possibly only unboxed numbers? definitely not strings) throws a TypeError. This is unquestionably a bug in Safari. David fixes by making one-arg cljs.core/str function call js-str instead of toString. js-str uses string-concat ''+x.
  3. However, this breaks for objects that define valueOf (issue in current ticket), because in js ''+x is the same as ''+x.valueOf().toString() not ''+x.toString().
  4. David considers using String() and variations but rejects because of performance hit.
  5. David rolls back CLJS-801 from the string-concat back to the array-join style to fix.
  6. Nikita and I point out that rolling back CLJS-801 only fixes the str macro, not the string function, which still uses js-str and hence string-concat.
  7. David fixes the str function to use goog.string.buildString, which has the behavior of array.join. Behavior is now correct even on Safari 6.0.5.
  8. Thomas points out that buildString uses arguments in a way unoptimizable by v8, and now the str function (not macro) has a performance regression. He suggests using [].join() directly.

So, there's a lot of back and forth on this issue, and it's all because of a bug in Safari 6.0.5 which no one has been able to reproduce first-hand because Safari 6.0.5 is old and rare. For some perspective, Safari 6.0.x was only available on Lion and Mountain Lion between July 25,2012 and June 11,2013. Before July 25,2012 Lion used Safari 5.1.x and there was no Mountain Lion. On June 11, 2013, both Lion and Mountain Lion switched to Safari 6.1.x which does not suffer from the toString TypeError bug (I checked--I have an iMac with Lion on it). The only machines on Safari 6.0.5 are (Mountain) Lion machines which used software updates until late 2012-early 2013 and then stopped. I can't imagine this is a large number of people.

It is theoretically possible for me to run Safari 6.0.x on my Lion machine to actually test this, but I can't find a way to downgrade from 6.1.x.

I think the options are:

  1. Use array.join() for all stringification and take the performance hit (which we should quantify). Include a comment that this is only for Safari 6.0.x (only confirmed second-hand on 6.0.4 and 6.0.5) for future generations, who are going to think this is weird.
  2. Use CLJS-801 and toString (status quo before CLJS-847), and ignore this problem for Safari 6.0.x.
  3. Use CLJS-801, but add a number? check (with comment) to cljs.core/str$arity$1 for Safari 6.0.5. The number case should use js-str, and the rest toString. I think this will work, but again we have no way to test--we really need to get our hands on a Safari 6.0.x browser.

Of course we should benchmark these approaches but my hunch is that 2 is faster than 3 is faster than 1.

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

We are not going to ignore Safari 6.0.X. Any decisions made about this ticket will include supporting it.

Comment by Francis Avila [ 10/Jan/15 4:12 AM ]

Update on some research I am doing into this.

I created a jsperf of alternative str implementations that I am trying out. Right now I've only looked at one-arg str. I discovered a few things:

  • {{''+[x]}} is a faster alternative to [x].join('').
  • Advanced compilation can compute {{''+[x]}} at compile time if x is a bool, str, undefined, null, or number, even through function calls! I.e. str_test.str_arr(123) compiles to "123" without macro magic.
  • However, using an intermediate array (even if a preallocated singleton) is still slower than the old (if (nil? x) "" (.toString x))
  • Using a switch statement is as least as fast as the str-tostr baseline, usually faster.
  • I am 99% sure all these implementations (except str-tostr, the baseline, which definitely fails) work on the dreaded Safari 6.0.x. If anyone has this version, point it at the jsperf link above and run the tests. I think Browserstack has this version of Safari.

I'm still investigating the variadic case (str x y z a b c). It might be better to use reduce instead of Stringbuffer+seq. (Stringbuffer just does ''+x now instead of an array-join.)

Comment by Thomas Heller [ 10/Jan/15 6:37 AM ]

Sorry, got side-tracked for a bit.

@Francis: Thanks for the recap.

Don't have Safari 6 available either, but seems wrong that we all have to suffer because is minor percentage still has this (667 users of 190k+ on my site). Don't have a solution cause I can't test whether it works, we might try String.concat.

"".concat(obj); // "42"
"".concat(obj, ""); // "hello"
String.prototype.concat(obj, "") // "hello"
String.prototype.concat("", obj) // "hello"

But no idea if String.concat works, also it behaves odd with regards to valueOf.

http://jsperf.com/js-string-concat-variants

Perf is also inconclusive since Firefox appears to be cheating.

Comment by Francis Avila [ 10/Jan/15 2:04 PM ]

Tested that jsperf with Safari 6.0.5 using Browserstack, results are there.

Note I could not reproduce CLJS-847 because str-tostr does not fail as expected. I will try harder now that I have a browser to test.

Comment by Francis Avila [ 10/Jan/15 6:55 PM ]

Still cannot reproduce CLJS-847.

This script includes my attempt at a minimum reproducible case. My theory was that certain types at higher jit levels would fail. I could not get any case to fail. I also tried flapping back and forth between types and using one type at a time, but still no failures.

In this thread I found this "minimal" script which the OP said he could get to fail reliably. I could not get it to fail. However the original post was from feb 15, 2013, which means the Safari he was using would have to be 6.0.2 or lower.

Hypotheses:

  1. This error does not affect 6.0.5 but maybe 6.0.4 or lower.
  2. BrowserStack's system somehow mitigates the bug, meaning we need a "real" Lion Safari 6.0.x to test.
  3. These tests only fail under the correct phase of the moon.

So I can code up a patch for str using the str-switch implementation (which is at least a bit faster on some browsers), but I have no idea if it may fail on Safari 6.0.5. I only know that it works so far. CLJS-801 should also be safe to reapply because the root cause of all issues is the implementation 1-arity of the cljs.core/str function.

I have also asked for Kevin's help back in CLJS-847. (Kevin was the original reporter of the Safari 6.0.x issue.)

Comment by Francis Avila [ 19/Jan/15 12:51 AM ]

Made a jsperf of variadic cases. Chrome seems to really prefer IReduce to seq+stringbuilder for vectors (other collections not tested), but there is no difference or a small slowdown on other browsers. Not sure if it's worth it.

Also updated arity-one cases with a str using switch and never using toString. Nearly 50% slower than using switch or toString on Chrome, smaller on Safari.

In terms of safety str-switch-notostr does not use toString at all so is probably safer. I think str-switch will likely work too, though, and is significantly faster. However I haven't been able to get any TypeErrors in Safari 6.0.5 so it's anyone's guess.

I suggest something like this as a new str (which doesn't use reduce, but could):

(defn str
 ([x]
  (case (js* "typeof ~{}" x)
   "string" x
   "object" (if (identical? nil x) "" (.toString x))
   ("boolean" "number") (js-str x)
   "undefined" ""
   (js-str #js [x])))                                       ;; insurance against Safari 6.0.x TypeError bug.
 ([a b] (js* "~{}+~{}" (str a) (str b)))
 ([a b c] (js* "~{}+~{}+~{}" (str a) (str b) (str c)))
 ([a b c d] (js* "~{}+~{}+~{}+~{}" (str a) (str b) (str c) (str d)))
 ([a b c d & more]
  (loop [s (str a b c d) [e f g h & r] more]
   (let [s' (js* "~{}+~{}+~{}+~{}+~{}" s e f g h)]
    (if (nil? r)
     s'
     (recur s' r))))))
Comment by Francis Avila [ 19/Jan/15 11:24 PM ]

First cut of a possible patch that resolves this while not breaking CLJS-847. Should wait for confirmation that this does not break Safari 6.0.x.

Comment by Francis Avila [ 19/Jan/15 11:34 PM ]

Oops forgot tests.

Comment by Francis Avila [ 03/Feb/15 10:24 AM ]

Update in CLJS-847: original reporter was not able to reproduce his original bug report in Safari 6.0.x running in BrowserStack. This may be because of BrowserStack, but it's the best we have.

Given how hard this bug is to reproduce, how few people it affects, and how significant the performance regression is, I still think we should go back to the simple (if (nil? x) "" (.toString x)) implementation. However, you could also try the patch on this ticket (using a typeof switch), which at least (handwaving) might fix this bug in Safari 6.0.x and is a little faster than a simple .toString in Chrome and not much slower elsewhere. (The reason I think it might avoid this bug in Safari is that it avoids calling .toString on non-Objects.)

Comment by Antonin Hildebrand [ 25/Apr/15 11:08 PM ]

I wonder if you considered swapping str function at runtime during CLJS init phase.

Implement str function using plain .toString() call (original solution). And at startup check for Safari 6.0.x presence and optionally swap str for implementation wrapping .toString() call in a try-catch block silencing TypeError exceptions by falling back to Safari 6.0.x friendly .toString() alternative.

We would get correct semantics in all cases. And price would be just slower printing execution on Safari 6.0.x not on all systems.

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

cljs-890.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-1009] Allow deps.cljs to declare a foreign lib as remote Created: 05/Feb/15  Updated: 19/Nov/17

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

Type: Enhancement Priority: Trivial
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-CLJS-1009-allow-remote-url-in-deps.cljs-foreign-libs.patch    
Patch: Code

 Description   

When using things like the Google Maps JS API [1] the Javascript that is required can't be bundled inside a jar as it depends on the used API key.

To be able to provide externs for those kind of libraries there should be a way to declare them as "remote" in deps.cljs.

[1] https://developers.google.com/maps/documentation/javascript/tutorial



 Comments   
Comment by Nathan Dao [ 08/Apr/17 6:43 PM ]

js_deps.cljs deliberately only allows upstream foreign-libs (defined in deps.cljs file) to be local file. Yet, this issue is still open, so I (hastily ) assume the local file enforcement was not intentional.

The patch should add back support for using url as foreign-libs in deps.cljs and remove unused codes in load-foreign-library*.

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

Patch no longer applies.





[CLJS-1141] memoization of js-dependency-index and get-upstream-deps needs knobs Created: 18/Mar/15  Updated: 19/Nov/17

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

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

Attachments: Text File CLJS_1141.patch     Text File CLJS-1141-with-js-dep-caching-latest.patch    
Patch: Code

 Description   

knobs should be exposed for more dynamic compilation environments like Figwheel which may desire to add dependencies to the classpath on the fly.



 Comments   
Comment by Bruce Hauman [ 21/Mar/15 3:51 PM ]

A patch that caches upstream dependencies in the compiler env.

Comment by Bruce Hauman [ 21/Mar/15 3:59 PM ]

Actually I'm going to submit another patch that includes the memoize calls in js-deps.

Comment by Bruce Hauman [ 28/Mar/15 12:50 PM ]

New patch that moves cljs.js-deps memoization to current env/compiler as well as get-upstream-deps.

Unfortunately there is a circular dep between cljs.env and cljs.js-deps, if we want to cache in env/compiler. I overcame this with a resolve.

Compile performance is either completely unchanged or slightly improved based on several test runs.

Comment by Bruce Hauman [ 28/Mar/15 2:22 PM ]

Hold off on this. Its not behaving as expected. Doesn't seem to be caching in certain situations.

Comment by David Nolen [ 28/Mar/15 2:26 PM ]

Thanks for the update. This will definitely not land until after the pending REPL/piggieback release anyhow.

Comment by Bruce Hauman [ 28/Mar/15 2:44 PM ]

Yeah there is an obvious bug and a subtle one. Hopefully will finish it up soonish.

Comment by Bruce Hauman [ 28/Mar/15 3:43 PM ]

Alright, this latest patch works. There was a subtle memoizing nil value bug.

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

Patch no longer applies.





[CLJS-1297] defrecord does not emit IKVReduce protocol Created: 03/Jun/15  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: Daniel Skarda Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: newbie

Attachments: Text File CLJS-1297-19-July-2015.patch    
Patch: Code and Test

 Description   

Records are maps and in Clojure they support reduce-kv (IKVReduce protocol).
This is not true in ClojureScript:

(defrecord Foobar [x y])
 (reduce-kv assoc {} (Foobar. 1 2))

Fails wit Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]



 Comments   
Comment by David Nolen [ 03/Jun/15 7:25 PM ]

Just seems like an oversight. Patch welcome, this one is a relatively easy one.

Comment by Daniel Skarda [ 04/Jun/15 2:53 AM ]

OK

I checked Clojure implementation. Records do not implement any reduce protocol on their own. For IKVReduce records use default implementation using reduce and destructuring. Is this approach OK?

Recently Alex Miller implemented many optimizations of reduce protocols in Clojure. Eg range returns an object which implements IReduce protocol so reduce (and transducers in general) can take advantage of it. Any plans for such optimizations in ClojureScript?

;;clojure/src/clj/clojure/core.clj:6523
;;slow path default
clojure.lang.IPersistentMap
(kv-reduce 
  [amap f init]
  (reduce (fn [ret [k v]] (f ret k v)) init amap))
Comment by David Nolen [ 04/Jun/15 9:05 AM ]

Going with the Clojure implementation is fine. Yes all of the optimizations in 1.7.0 are on the table for ClojureScript but these are separate issues from this one.

Comment by Samuel Miller [ 16/Jul/15 10:39 PM ]

Mind if I take this as my first cljs bug? Poking around quickly I think I know what needs to happen.

Comment by David Nolen [ 17/Jul/15 5:21 AM ]

Sure! Have you submitted your CA yet?

Comment by Samuel Miller [ 17/Jul/15 7:13 PM ]

Yes, I did yesterday.

Comment by Samuel Miller [ 20/Jul/15 9:52 PM ]

Here is a potential patch. I implemented a basic IKVreduce based on Daniel Skarda's comment. Note: I am a little fuzzy on macros still so please look over what I have. There is probably a better way. Also added a test for reduce-kv on records.

I ran the test on Linux on V8 and SpiderMonkey. I plan to get JSC and Nashorn working and tested this week but if someone wants to test them out before that would be great.

Comment by Sebastian Bensusan [ 23/Jul/15 6:45 PM ]

Experience report:

I just tested the patch in the Node Repl and it seems to work:

cljs.user=> (defrecord A [a b])
cljs.user/A
cljs.user=> (reduce-kv (fn [m k v] (assoc m k (inc v))) {} (A. 1 2))
{:a 2, :b 3}

and the provided tests passed in Spidermonkey, V8, and Nashorn (I don't have JSC installed).

For completeness: before applying the patch the same code fails with:

Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]
Comment by David Nolen [ 10/Aug/15 10:22 PM ]

Is this the same approach taken by Clojure?

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

You can see the relevant current Clojure code here...
https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L6526
I think it is the same. I literally just tried to translate it over into CLJS. I might of understood something wrong though.

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

Yes that's the slow path. Please use the implementation used by defrecord instead. If defrecord doesn't have one then this patch is OK.

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

As far as I can tell there is no implementation on defrecord itself however there are separate implementations on the the java classes PersistentVector, PersistentArrayMap, PersistentTreeMap, and PersistenHashMap in pure java. I am not sure if you would want to do something similar for Clojurescript.

I can also spend some time trying to make a more performant version.

Comment by António Nuno Monteiro [ 27/Jul/16 7:38 AM ]

Confirmed that Clojure uses the slow path via the IPersistentMap implementation in defrecord
https://github.com/clojure/clojure/blob/d920ad/src/clj/clojure/core.clj#L6712

Patch still applies and can also confirm it works for me.

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

CLJS-1297-19-July-2015.patch no longer applies.





[CLJS-1466] Absolute paths in :output-dir break Node.js shim for :none Created: 11/Oct/15  Updated: 19/Nov/17

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

Type: Defect Priority: Major
Reporter: Sebastian Bensusan Assignee: Sebastian Bensusan
Resolution: Unresolved Votes: 0
Labels: bug

Attachments: Text File cljs_1466.patch    
Patch: Code

 Description   

When compiling a trivial example with the following script:

(require 'cljs.build.api)

(cljs.build.api/build "src"
  {:main 'hello.core
   :output-to "main.js"
   :output-dir "/home/carlos/Playground/node-abs/out"
   :target :nodejs})

It generates code that tries to resolve the following path:

/home/carlos/Playground/node-abs/home/carlos/Playground/node-abs/out/goog/bootstrap/nodejs.js

We should check if the provided path for :output-dir is absolute before resolving it in the Node.js :none shim. The shim has a related ticket in CLJS-1444.

Even if it's uncommon for users to have absolute paths, tooling might need to.



 Comments   
Comment by Sebastian Bensusan [ 11/Oct/15 4:28 PM ]

The attach patch cljs_1466.patch solves the issue by using path.resolve which takes into account relative vs absolute paths when joining paths. Successfully tested in the example repo with both relative and absolute :output-dir

Comment by Martin Klepsch [ 14/Oct/15 3:57 AM ]

Looking at the patch it seems that it might break current behaviour in some cases? Have you thought about that?

CLJS-1444 [1] would probably also break the shim in some way so would be good to get these in together and be very clear about what will break etc. As long as we come up with a robust and predictable impl this is something worth breaking imo.

[1] http://dev.clojure.org/jira/browse/CLJS-1444 for

Comment by David Nolen [ 14/Oct/15 8:05 AM ]

Yes would like to get feedback from people already heavily invested in ClojureScript + Node.js before moving forward on this.

Comment by Sebastian Bensusan [ 14/Oct/15 10:31 AM ]

Martin Klepsch: I did think about breakage but I couldn't find any cases. Do you have an example one? In the example repo I've put together some tests (by running ./script/test.sh) but it boils down to path.join(path.resolve("."),paths) being equivalent to path.resolve(paths) for all relative paths, since the "Resolve to absolute" method is the same for both (process.cwd() inside of path.resolve). When considering absolute paths, only the new version does the right thing.

On the other hand, those tests also reveal that the proposed patch doesn't cover CLJS-1446 as I originally thought since

node main.js

succeeds while:

cd ..
node node-abs/main.js

fails.

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

Patch no longer applies.





[CLJS-1549] Printing #object tags should be consistent Created: 22/Jan/16  Updated: 19/Nov/17

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

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

Attachments: Text File cljs_1549.patch    
Patch: Code

 Description   

[CLJS-1376|http://dev.clojure.org/jira/browse/CLJS-1376) wanted to bring printing parity with Clojure 1.7.0.

There are two problems:
1) Atom and Volatile contain extra space between #object and "[" (that differs from Clojure 1.7.0)
2) Atom and Volatile printing was inconsistent with #object printing in pr-writer-impl (in goog/isFunction and :else branches) - there we have no space

I have introduced a function which handles printing object tags consistently without space. Also I have broken writing content into writer into logical tokens for easier cljs-devtools implementation.

Background info:
Exact printing behaviour is quite important for me when implementing alternative pr-writer-impl for cljs-devtools. For most cases I call original pr-writer-impl and then post-process its output to enhance its presentation in DevTools console:
https://github.com/binaryage/cljs-devtools/blob/2f1c0f4095a364bb11a833e73e5753a4e48add8f/src/devtools/format.cljs#L200-L226

In case of constructors or functions (goog/isFunction and :else branches of pr-writer-impl), I don't want to emit stringified content, but I want to provide real js-object reference and delegate further printing/expansion to native javascript formatter in DevTools.
In other cases like Atom or Volatile, I would like to easily detect "#object" as first token written into writer and provide better visual clues. I find printing "#object [...]" confusing for more complex data situations. Square brackets resemble vectors and may confuse clarity. Instead I will use color-coding or another form of visual styling to mark object instances and their boundaries. But for this I want to have nice token stream "#object" "[" ... "]" without having to use regexps.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 7:58 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-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-1627] jsdoc parsing fails to recognize union types, breaking resolution Created: 18/Apr/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: Patrick Killean Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: patch

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





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

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

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

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

 Description   

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

What do you think?

Roman

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

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

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

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

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

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

Start a Nashorn REPL.

./script/nashornrepljs

js/Symbol does not exist in Nashorn yet.

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

Load core-js shim into Nashorn.

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

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

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

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

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

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

Which implementations do support js/Symbol?

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

Nashorn and Rhino do not support js/Symbol.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Patch no longer applies.





[CLJS-1725] defrecord does not set cljs$lang$ctorStr Created: 04/Aug/16  Updated: 19/Nov/17

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

Type: Defect Priority: Trivial
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

While implementing some demunging code[1] for cljs-devtools, I stumbled upon inconsistency in type->str. It works for deftypes but not for defrecords.

The problem is that defrecord does not set! cljs$lang$ctorStr field for some reason. This has been likely overlooked, because type->str is not used often, just for some rare error exceptions as far I can see.

Anyways, this patch fixes it by copy&pasting it from deftype. A better solution to avoid future problems like this would be to extract shared code between deftypes and defrecords, but that seems to be a lot of work.

The patch also adds some tests. I had to add some utility macros to run those tests only in simple mode, because type->str is not supposed to work under advanced builds.
Also test for CLJS-1722 was added to be uncommented after potentially merging it.

[1] https://github.com/binaryage/cljs-devtools/blob/52899e61e33373df36be8dcb23c69377936821b2/src/lib/devtools/munging.cljs#L56-L60



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

Patch no longer applies; needs re-baseline.





[CLJS-1769] Use reduce pathway for arrays in js->clj Created: 01/Sep/16  Updated: 19/Nov/17

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

Type: Enhancement Priority: Trivial
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

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

 Description   

Currently js->cljs uses `(vec (map some-fn some-js-array))` on arrays. By using `(mapv some-fn some-js-array)` we take advantage of array-reduce and avoid a lazy-sequence.



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

Patch no longer applies; needs re-baseline.





[CLJS-1783] Unify List creation code Created: 20/Sep/16  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

There is some duplication and redundant functions around List creation.

In this patch a fromArray method was added to List, consistent with other persistent data structures in the code base.



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

Patch no longer applies; needs re-baseline.





[CLJS-1784] Cleanup set creation functions Created: 20/Sep/16  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Use .fromArray for consistency/speed when handling zeroed IndexedSeqs.

Use reduce as the default construction path to take advantage of reducible collections.



 Comments   
Comment by Rohit Aggarwal [ 26/Sep/16 4:13 PM ]

Thomas Mulvaney, could you provide some benchmarks for the speed assertion? It would be nice to run it on Chrome/Firefox/Safari.

Comment by Thomas Mulvaney [ 28/Sep/16 1:30 AM ]

Sure thing, I'll do some more benchmarks.

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

Patch no longer applies; needs re-baseline.





[CLJS-1793] Few misplaced docstrings Created: 24/Sep/16  Updated: 19/Nov/17

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

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

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

 Description   

A few docsctrings were placed after the binding form in the src/main/clojure/cljs/closure.clj namespace.



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

Patch no longer applies; needs re-baseline.





[CLJS-1810] Refactoring of find-and-cache-best-method Created: 05/Oct/16  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Andrey Zaytsev Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-refactor-find-and-cache-best-method.patch    
Patch: Code

 Description   

find-and-cache-best-method was pretty messy and confusing. cache reset is done in -get-method fn itself and it was basically a dead code. find-best-method is the replacement of it and operates with immutable data instead of internal multimethod's mutable state.
prefers* function didn't mutate the atom too, so now it takes an immutable value.
dominates is now an internal helper of find-best-method since it is private and not used by anything else.



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

Patch no longer applies; needs re-baseline.





[CLJS-1827] Improve reader performance on Firefox/Windows Created: 20/Oct/16  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Michael Sperber Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, reader
Environment:

Firefox on Windows


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

 Description   

cljs.reader/read-string speeds up by a factor of 2 on Firefox/Windows through this change without complicating the code.

(Other JS engines, including Firefox on Linux/Mac do not seem to be affected as significantly.)



 Comments   
Comment by David Nolen [ 20/Oct/16 10:33 AM ]

It would be nice to have a bit more information on this ticket as to what Google Closure does that's unnecessary or whether this path is actually a faithful port of Clojure behavior (copies the implementation of the EDN reader in these hot spots??). Finally the patch names David Frese, have they submitted a CA?

Thanks!

Comment by Michael Sperber [ 21/Oct/16 5:49 AM ]

I believe the Google functions are too general, work on strings in addition to characters etc.

It's not clear to us though why only Firefox on Windows benefits.

(David Frese is a co-worker - yes, has submitted a CA.)

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

Patch no longer applies; needs re-baseline.
Also, doesn't work with git am; recommend creating patch using https://clojurescript.org/community/patches





[CLJS-1846] Range issues Created: 10/Nov/16  Updated: 19/Nov/17

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

Type: Defect Priority: Major
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core

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

 Description   

Problem
There are a number of bugs with Range which occur when the step size is 0 or where negative.

Examples

cljs.user=> (count (range 10 0 0))
-Infinity  ;Expect Infinity

cljs.user=> (nth (range 10 0 -1) -1)
11 ; Expected IndexOutOfBounds

cljs.user=> (take 5 (sequence identity (range 0 10 0)))
() ; Expected (0 0 0 0 0)

cljs.user=> (into [] (take 5) (range 0 10 0))
[] ; Expected [0 0 0 0 0]


 Comments   
Comment by David Nolen [ 10/Nov/16 4:37 PM ]

This patch is headed in the right direction but it needs to be more vigilant about performance. I'm more than happy to talk it over via IRC or Slack. Thanks!

Comment by Thomas Mulvaney [ 15/Nov/16 8:24 AM ]

Updated patch with performance tweaks.

  • Added the ^boolean annotation to the `some-range?` helper.
  • Removed calls to methods of Range where possible.
  • Improved 2-arity reduces performance over master significantly by replacing the call to ci-reduce.
Comment by Mike Fikes [ 19/Nov/17 7:39 PM ]

Patch no longer applies; needs re-baseline.





[CLJS-1902] Add support for compiler option :inline-source-maps Created: 24/Jan/17  Updated: 19/Nov/17

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

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

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

 Description   

1. refactor `emit-source-map` and break it into multiple functions
2. fix logic for relative path computation (see `strip-prefix-path`)
3. add support for `:inline-source-maps` option
4. add tests

Related: CLJS-1402, CLJS-1901



 Comments   
Comment by Antonin Hildebrand [ 24/Jan/17 8:09 PM ]

Patches:
https://github.com/darwin/clojurescript/commit/de1da63072c8049b9812d41cce750e5a972c0b0b.patch
https://github.com/darwin/clojurescript/commit/2389fb7839a18299d842d381b135a669a7091869.patch
https://github.com/darwin/clojurescript/commit/3ba594d8c7215ef8c8276737b7e5d8008c6b3a98.patch

Comment by Antonin Hildebrand [ 24/Jan/17 8:27 PM ]

Full review: https://github.com/clojure/clojurescript/compare/darwin:inline-source-maps~3...darwin:inline-source-maps

Also please note that the first patch testing original functionality fails in one test because there was a bug in timestamp formatting in :source-map-url case:
https://github.com/clojure/clojurescript/compare/master...darwin:inline-source-maps#diff-55b85385d2d0bfb6dc20d59ed982d5c8L1239

Comment by Antonin Hildebrand [ 25/Jan/17 10:25 AM ]

Today when testing Dirac I realised we need to embed sources contents as well.

Additional patch:
https://github.com/darwin/clojurescript/commit/c1df38f14a33d02fe2d421f80db0b421b17286bb.patch

New review URL: https://github.com/darwin/clojurescript/compare/inline-source-maps~4...darwin:inline-source-maps

Tested in DevTools and works like a charm.

Comment by Dusan Maliarik [ 06/Feb/17 10:00 AM ]

This would be helpful for us as well.

Comment by Andrea Richiardi [ 01/May/17 3:49 PM ]

I have run across this one as well by following this tutorial.

Without either this patch or Dirac's complicated setup it is not currently possible to use node --inspect and debug correctly. The symptom I see on our side is that source maps are detected but for some reason Chrome DevTools does not show them in the Tree View.

The content of one of it is:

{"version":3,"file":"\/Users\/user\/cqrs-engine-cljs\/out\/cqrs\/event_store.js","sources":["event_store.cljs"], ...
Comment by David Nolen [ 16/Jun/17 10:29 AM ]

Linking to patches outside of JIRA is not proper for tickets. Please add a single squashed patch to this ticket directly.

Comment by Antonin Hildebrand [ 16/Jun/17 2:25 PM ]

Attached it as a patch file.

Took https://github.com/darwin/clojurescript/compare/inline-source-maps~4...darwin:inline-source-maps.diff and applied it to current master. It applied cleanly without conflicts. Tests are still passing on my machine.

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

Patch no longer applies; needs re-baseline.





[CLJS-2001] Add map-entry? predicate Created: 06/Apr/17  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Thomas Mulvaney
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

map-entry? has existed in Clojure since 1.8 would be nice to have it in ClojureScript.



 Comments   
Comment by Thomas Mulvaney [ 06/Apr/17 6:00 AM ]

The attached patch looks more like the first implementation of `map-entry?` as per CLJ-1831.

This is because ClojureScript returns PersistentVectors when iterating over PAM and PHM maps.

Comment by David Nolen [ 07/Apr/17 11:06 AM ]

This patch is no good. 2 element vectors are not MapEntry.

Comment by Francis Avila [ 07/Apr/17 7:22 PM ]

Given that Clojure still returns MapEntry (CLJ-1831 was backed out later) and CLJS returns vectors, it is probably impossible for this predicate to be portable. If we can't consider count-2 vectors map-entry?=true, then the only possible cljs impl is (defn map-entry? [x] false). Given this, perhaps the best solution is not to have map-entry? in cljs, to discourage people from using it in portable code.

Comment by David Nolen [ 12/Apr/17 1:03 PM ]

I'm fine with adding a MapEntry type which implements all the vector protocols and returning that instead. That work should be a separate issue though and then we can come back to this one.

Comment by Thomas Mulvaney [ 19/Apr/17 8:00 AM ]

This came about as I was porting some Clojure code. I was probably misusing/abusing map-entry? anyway. The code could be rewritten to check if something is a map first and then do the appropriate thing on the sequence of entries rather than doing the check from "with in" the collection.

As mentioned, a 2 element vector != MapEntry. So, I've opened an issue to track adding a MapEntry type: CLJS-2013

Comment by David Nolen [ 16/Jun/17 9:48 AM ]

Now that we have MapEntry we can do this one correctly.

Comment by Thomas Mulvaney [ 13/Jul/17 5:40 AM ]

Updated patch attached which checks if x is an instance of the recently added MapEntry type.

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

Patch no longer applies; needs re-baseline.





[CLJS-2004] Minor fix for test-simple script Created: 10/Apr/17  Updated: 19/Nov/17

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





[CLJS-2131] Calling empty on a ChunkedSeq returns a vector not an empty list Created: 27/Jun/17  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

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

 Description   

Calling empty on a ChunkedSeq returns an empty vector rather than an empty list.



 Comments   
Comment by Thomas Mulvaney [ 27/Jun/17 3:56 AM ]

Metadata was also being carried on to the empty seq which is not expected behaviour for seqs.

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

The attached patch no longer applies. Needs to be re-baselined.





[CLJS-2285] Relative path exception thrown on Windows using :preload Created: 30/Jul/17  Updated: 19/Nov/17

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

Type: Defect Priority: Major
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Windows


Attachments: Text File CLJS-2285b.patch     Text File CLJS-2285.patch    
Patch: Code

 Description   

Seems like a small bug in appending "/" in the strip-user-dir helper. This code was recently updated.

Steps to repeat from CLJS-2036 can be used. I think it boils down to:

1. Add a :preload namespace to the build
2. Require a foreign lib in the preload ns

Expect: Builds fine

Actual: `IllegalArgumentException: C:\Repos\canidep\src\uilib.js is not a relative path` full exception below.

Works on 1.9.671.

Fails on 1.9.854.

Exception in thread "main" java.lang.IllegalArgumentException: C:\Repos\canidep\src\uilib.js is not a relative path, compiling:(C:\Repos\canidep\scripts\build.clj:36:1)
        at clojure.lang.Compiler.load(Compiler.java:7391)
        at clojure.lang.Compiler.loadFile(Compiler.java:7317)
        at clojure.main$load_script.invokeStatic(main.clj:275)
        at clojure.main$script_opt.invokeStatic(main.clj:335)
        at clojure.main$script_opt.invoke(main.clj:330)
        at clojure.main$main.invokeStatic(main.clj:421)
        at clojure.main$main.doInvoke(main.clj:384)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:379)
        at clojure.lang.AFn.applyToHelper(AFn.java:154)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
        at java.lang.reflect.Method.invoke(Unknown Source)
        at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
        at clojure.lang.Reflector.invokeStaticMethod(Reflector.java:207)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
        at java.lang.reflect.Method.invoke(Unknown Source)
        at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
        at clojure.lang.Reflector.invokeStaticMethod(Reflector.java:207)
        at user$eval5.invokeStatic(form-init8570637412295703479.clj:1)
        at user$eval5.invoke(form-init8570637412295703479.clj:1)
        at clojure.lang.Compiler.eval(Compiler.java:6927)
        at clojure.lang.Compiler.eval(Compiler.java:6917)
        at clojure.lang.Compiler.load(Compiler.java:7379)
        at clojure.lang.Compiler.loadFile(Compiler.java:7317)
        at clojure.main$load_script.invokeStatic(main.clj:275)
        at clojure.main$init_opt.invokeStatic(main.clj:277)
        at clojure.main$init_opt.invoke(main.clj:277)
        at clojure.main$initialize.invokeStatic(main.clj:308)
        at clojure.main$null_opt.invokeStatic(main.clj:342)
        at clojure.main$null_opt.invoke(main.clj:339)
        at clojure.main$main.invokeStatic(main.clj:421)
        at clojure.main$main.doInvoke(main.clj:384)
        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.IllegalArgumentException: C:\Repos\canidep\src\uilib.js is not a relative path
        at clojure.java.io$as_relative_path.invokeStatic(io.clj:414)
        at clojure.java.io$file.invokeStatic(io.clj:426)
        at clojure.java.io$file.invoke(io.clj:418)
        at cljs.closure$write_javascript.invokeStatic(closure.clj:1698)
        at cljs.closure$write_javascript.invoke(closure.clj:1691)
        at cljs.closure$source_on_disk.invokeStatic(closure.clj:1740)
        at cljs.closure$source_on_disk.invoke(closure.clj:1735)
        at cljs.closure$build$fn__6925.invoke(closure.clj:2536)
        at clojure.core$map$fn__4785.invoke(core.clj:2646)
        at clojure.lang.LazySeq.sval(LazySeq.java:40)
        at clojure.lang.LazySeq.seq(LazySeq.java:49)
        at clojure.lang.Cons.next(Cons.java:39)
        at clojure.lang.RT.next(RT.java:688)
        at clojure.core$next__4341.invokeStatic(core.clj:64)
        at clojure.core$dorun.invokeStatic(core.clj:3033)
        at clojure.core$doall.invokeStatic(core.clj:3039)
        at clojure.core$doall.invoke(core.clj:3039)
        at cljs.closure$build.invokeStatic(closure.clj:2536)
        at cljs.closure$build.invoke(closure.clj:2444)
        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 user$eval8894.invokeStatic(build.clj:36)
        at user$eval8894.invoke(build.clj:36)
        at clojure.lang.Compiler.eval(Compiler.java:6927)
        at clojure.lang.Compiler.load(Compiler.java:7379)
        ... 42 more


 Comments   
Comment by Oliver George [ 30/Jul/17 5:24 AM ]

The problem is highlighted by this:

In cljs.util/relative-name the user-path uses "/" as the separator

(.getFile (.toURL (io/file (System/getProperty "user.dir"))))

=> "/C:/Repos/canidep/"

But the code which appends "/" if it's missing uses File/separator

File/separator

=> "\\"
Comment by Oliver George [ 30/Jul/17 8:27 PM ]

I prefer the patch on CLJS-2221 to solve this problem.

Comment by Oliver George [ 30/Jul/17 8:35 PM ]

Make test for util/relative-path platform specific. Test specific Windows
behaviour.

Does not fix bug.

Comment by Inge Solvoll [ 15/Aug/17 7:10 AM ]

Something similar happens when following this tutorial https://ajpierce.github.io/posts/react-figwheel-npm-2/

When starting figwheel, I get this stacktrace:

java.lang.IllegalArgumentException: c:\dev\splort\node_modules\react-dom\index.js is not a relative path
at clojure.java.io$as_relative_path.invokeStatic (io.clj:414)
clojure.java.io$file.invokeStatic (io.clj:426)
clojure.java.io$file.invoke (io.clj:418)
cljs.closure$write_javascript.invokeStatic (closure.clj:1698)
cljs.closure$write_javascript.invoke (closure.clj:1691)
cljs.closure$process_js_modules$fn__15415.invoke (closure.clj:2348)

Is this the same entry point to the error, or does it need a separate issue?

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

CLJS-2285b.patch no longer applies





[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-2149] aget produces different results from Clojure for non-integer and out-of-bounds indexes Created: 02/Jul/17  Updated: 18/Nov/17

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

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


 Description   

Like CLJS-2113, but for aget:

Clojure:

user=> (aget (to-array [nil 1]) -1)
ArrayIndexOutOfBoundsException -1  clojure.lang.RT.aget (RT.java:2336)
user=> (aget (to-array [nil 1]) 0)
nil
user=> (aget (to-array [nil 1]) 0.5)
nil
user=> (aget (to-array [nil 1]) 1)
1
user=> (aget (to-array [nil 1]) 1.5)
1
user=> (aget (to-array [nil 1]) 2)
ArrayIndexOutOfBoundsException 2  clojure.lang.RT.aget (RT.java:2336)

ClojureScript

cljs.user=> (aget (to-array [nil 1]) -1)
nil
cljs.user=> (aget (to-array [nil 1]) 0)
nil
cljs.user=> (aget (to-array [nil 1]) 0.5)
nil
cljs.user=> (aget (to-array [nil 1]) 1)
1
cljs.user=> (aget (to-array [nil 1]) 1.5)
nil
cljs.user=> (aget (to-array [nil 1]) 2)
nil

Also note that Clojure acts as if rounding indices down to the nearest integer while ClojureScript does not:

(aget (to-array [1 2]) 0.5)

yields 1 in Clojure and nil in ClojureScript.

(Presumably, similar results hold for aset.)



 Comments   
Comment by Mike Fikes [ 18/Nov/17 10:02 AM ]

The fact that Clojure's aget happens to work on non-integer indices may not be intentional. An int cast may be present only to ease interop with the default use of long integral values in Clojure, and while this cast causes the observed behavior (rounding down of passed double s), this may not reflect the intended API.

Here is a commit that speaks to "hints": https://github.com/clojure/clojure/commit/742619e583400400e69cd46ab9e9536c10afb738





[CLJS-2401] build with :optimizations is not working on Windows Created: 18/Nov/17  Updated: 18/Nov/17

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

Type: Defect Priority: Major
Reporter: Dmitry Gusarov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

I got cljs.jar from:
https://github.com/clojure/clojurescript/releases/tag/r1.9.946


Attachments: Zip Archive prj.zip    

 Description   

Just went through Quick Start and got an error at 'Production Builds' section.
Looks like it currently unable to compile with :optimizations

Exception in thread "main" java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/prj/out/cljs/core.js, compiling:(C:\prj\release.clj:3:1)
        at clojure.lang.Compiler.load(Compiler.java:7391)
        at clojure.lang.Compiler.loadFile(Compiler.java:7317)
        at clojure.main$load_script.invokeStatic(main.clj:275)
        at clojure.main$script_opt.invokeStatic(main.clj:335)
        at clojure.main$script_opt.invoke(main.clj:330)
        at clojure.main$main.invokeStatic(main.clj:421)
        at clojure.main$main.doInvoke(main.clj:384)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:379)
        at clojure.lang.AFn.applyToHelper(AFn.java:154)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
Caused by: java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/prj/out/cljs/core.js
        at sun.nio.fs.WindowsPathParser.normalize(Unknown Source)
        at sun.nio.fs.WindowsPathParser.parse(Unknown Source)
        at sun.nio.fs.WindowsPathParser.parse(Unknown Source)
        at sun.nio.fs.WindowsPath.parse(Unknown Source)
        at sun.nio.fs.WindowsFileSystem.getPath(Unknown Source)
        at com.google.javascript.jscomp.SourceMapResolver.getRelativePath(SourceMapResolver.java:73)
        at com.google.javascript.jscomp.SourceMapResolver.extractSourceMap(SourceMapResolver.java:53)
        at com.google.javascript.jscomp.JsAst.parse(JsAst.java:168)
        at com.google.javascript.jscomp.JsAst.getAstRoot(JsAst.java:55)
        at com.google.javascript.jscomp.CompilerInput.getAstRoot(CompilerInput.java:122)
        at com.google.javascript.jscomp.Compiler.hoistNoCompileFiles(Compiler.java:1992)
        at com.google.javascript.jscomp.Compiler.orderInputs(Compiler.java:1890)
        at com.google.javascript.jscomp.Compiler.parseInputs(Compiler.java:1793)
        at com.google.javascript.jscomp.Compiler.parseForCompilationInternal(Compiler.java:982)
        at com.google.javascript.jscomp.Compiler.access$300(Compiler.java:102)
        at com.google.javascript.jscomp.Compiler$6.call(Compiler.java:964)
        at com.google.javascript.jscomp.Compiler$6.call(Compiler.java:961)
        at com.google.javascript.jscomp.CompilerExecutor$2.call(CompilerExecutor.java:101)
        at java.util.concurrent.FutureTask.run(Unknown Source)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
        at java.lang.Thread.run(Unknown Source)





[CLJS-2400] test-module-processing and test-global-exports fail under Java 9 Created: 17/Nov/17  Updated: 17/Nov/17

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

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


 Description   

If you apply the fix in CLJS-2377, and then run script/test under Java 8, things pass, but under Java 9 test-module-processing and test-global-exports fail:

ERROR in (test-module-processing) (TypeError:NaN:NaN)
expected: (= (array/nth #js [1 2 3] 1) 2)
  actual: #object[TypeError TypeError: g4.ki is not a function]

ERROR in (test-global-exports) (TypeError:NaN:NaN)
expected: (= (plus 1 2) 3)
  actual: #object[TypeError TypeError: Cannot read property 'add' of undefined]


 Comments   
Comment by Mike Fikes [ 17/Nov/17 6:13 PM ]

For the first error, under Java 8, the :advanced JavaScript (in builds/out-adv/core-advanced-test.js) has this bit of code in it

var b=f4.nth([1,2,3],1);

while under Java 9, we evidently have a rename failure:

var b=g4.ki([1,2,3],1);




[CLJS-2394] npm modules not inserted into cljs_deps when compiler is restarted Created: 02/Nov/17  Updated: 17/Nov/17  Resolved: 17/Nov/17

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

Type: Defect Priority: Major
Reporter: Hendrik Poernama Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2394-patch-1.patch    

 Description   

When restarted, compiler would rewrite cljs_deps and remove all npm modules dependency. Problem only appears when rebuilding, not from clean compilation.

Workaround: Do a clean build every time compiler has to be restarted.

Minimal example: https://github.com/poernahi/cljsbuild_wrongdeps

Steps to reproduce using above example:
1. Copy/symlink clojurescript uberjar into project root as cljs.jar
2. make clean && make && wc -l out/cljs_deps.js # Should return 186 lines
3. touch src/cljsbuild_wrongdeps/core.cljs && make && wc -l out/cljs_deps.js # Now returns only 4 lines!!

I think this is related to CLJS-2339. I narrowed down to this line
https://github.com/clojure/clojurescript/commit/245bdee2c35e19a9685b7a0849f26fce8bdaf7ca#diff-84ffd22349e5ca1fe6322cb0e379b3b1R2250

When a new compiler instance is run, the cache in ::transitive-dep-set is empty and index-node-modules return nil.



 Comments   
Comment by Hendrik Poernama [ 02/Nov/17 6:49 AM ]

Fix attempt

Comment by David Nolen [ 17/Nov/17 3:17 PM ]

fixed https://github.com/clojure/clojurescript/commit/2389e52049a9bd001d173a1cb4772ed8a25de196





[CLJS-2396] Configurable reader features Created: 11/Nov/17  Updated: 17/Nov/17  Resolved: 17/Nov/17

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

Type: Enhancement Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

The reader features for .cljc files is currently hard coded to #{:cljs} and Clojure will use :clj. With the intent being that host-specific features can be chosen at read-time. Given that there are a variety of different JS host platforms it would be useful to allow setting build-specific reader features so that builds targeting node can use different code than using the browser.

We can not always rely on the Closure Compiler eliminating all the code for us and we currently have no other way to get conditional requires.

Kevin Lynagh described his use-case here: https://gist.github.com/lynaghk/0608a43f173558b6fcf30c3be53d77dd

I had a very similar use case in mind for conditional requires related to excluding some code from a node.js targeted build for doing SSR that otherwise should use the same code as the browser uses.

My suggestion would be to add :reader-features #{:kw1 kw1} to the compiler options which will be added to the default :cljs at read-time.



 Comments   
Comment by David Nolen [ 17/Nov/17 1:38 PM ]

This is serious language level feature request. Such ideas need to be discussed around Clojure first.

Comment by Thomas Heller [ 17/Nov/17 2:10 PM ]

I was under the impression that JIRA is the place to have these discussions so they don't get lost in Slack Archives.

The Clojure Reader and tools.reader already support everything we need so no changes to either are necessary.

Only the :features option would become configurable, nothing more. Clojure itself does not need this feature since it only runs on the JVM, has its :clj option and already ignores every other option. I do think it is useful to have this for CLJS given the wide variety of host platforms.

https://github.com/clojure/clojurescript/blob/245bdee2c35e19a9685b7a0849f26fce8bdaf7ca/src/main/clojure/cljs/analyzer.cljc#L3652 and very few other places would need to change.





[CLJS-2391] Unable to :stub a function using stest/instrument Created: 29/Oct/17  Updated: 17/Nov/17  Resolved: 17/Nov/17

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

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

clojurescript 1.9.946


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

 Description   

It should be possible to use instruments :stub option to replace the function with a generator (using the :ret spec to generate results). This works on clojure but fails on clojurescript.

(defn add2 [a b] 1)
(s/fdef add2 :args (s/cat :a int? :b int?) :ret #{2})
(println ::a (add2 1 2)) ; expect 1, got 1.  good.
(stest/instrument `add2 {:stub #{`add2}})
(println ::b (add2 1 2)) ; expect 2, got 1.  bad.
(stest/unstrument `add2)
(println ::c (add2 1 2)) ; expect 1, got 1.  good.


 Comments   
Comment by Oliver George [ 29/Oct/17 5:21 PM ]

Pretty sure the problem is (some #{sym} stub) not matching inside cljs.spec.test.alpha/instrument-choose-fn. How symbols are managed isn't immediately clear but that's my guess at the cause. The stub arg looks like I expect (e.g. #{cljs.spec.test-test/add2}) but the sym arg looks like #object[B0] (via nashorn). So no surprise the if statement doesn't do what we expect.

Comment by Oliver George [ 29/Oct/17 5:25 PM ]

This adds a test for instrument stubbing

Comment by Oliver George [ 29/Oct/17 10:44 PM ]

Patch including fix and test

Comment by David Nolen [ 17/Nov/17 1:44 PM ]

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





[CLJS-2393] process.env preload is added after all existing preloads Created: 31/Oct/17  Updated: 17/Nov/17

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

Type: Defect Priority: Minor
Reporter: Eugene Pakhomov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: npm-deps


 Description   

When there're other preloads and the check for process-shim returns true, `process.env` is added to the end of the vector of all preloads.
That causes problems when one of the previous preloads requires something that checks `process.env` during loading.

E.g. I have a Reagent application with `:npm-deps` set to React and a few other libraries. And I have a preload exactly like this one: https://github.com/flexsurfer/re-frisk/blob/master/src/re_frisk/preload.cljs
The issue is that `re-frisk.core` requires Reagent, which requires React, which checks `process.env` - all before `process.env` was actually created.

I think that `process.env` should go before all existing preloads.



 Comments   
Comment by Hendrik Poernama [ 02/Nov/17 8:02 AM ]

Workaround is to manually add process.env to the first of :preloads vector





[CLJS-2399] :foreign-libs with module conversion does not works properly (possible regression) Created: 16/Nov/17  Updated: 16/Nov/17

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

Type: Defect Priority: Major
Reporter: Andrey Antukh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

OpenJDK 8, Linux, node-v8.7.0



 Description   

This is the same issue as https://dev.clojure.org/jira/browse/CLJS-1682, so all the instructions for reproduce the error can be found in the referenced issue.

Here some specific details of the different errors on different cljs versions:

With `cljs==1.9.845` the compilation terminates successfully in both cases (`:foreign-libs` in `build.clj` and in `deps.cljs`) but on executing the generated javascript, an exception is raised:

$ node out/main.js                            
out/src/vendor/greeter.js:3
exports.sayHello = function(name) {
                 ^

TypeError: Cannot set property 'sayHello' of undefined
    at out/src/vendor/greeter.js:3:18
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at Object.runInThisContext (vm.js:139:38)
    at Object.nodeGlobalRequire (/home/niwi/tmp/greeter/out/goog/bootstrap/nodejs.js:85:8)
    at Object.cljs$core$load_file [as load_file] (/home/niwi/tmp/greeter/out/cljs/core.js:341:13)
    at Object.<anonymous> (/home/niwi/tmp/greeter/out/testapp/core.js:5:11)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)

With `cljs==1.9.908` the compilation terminates successfully in both cases but on executing the generated javascript and other exception is raised:

$ node out/main.js                            
/home/niwi/tmp/greeter/out/testapp/core.js:28
return cljs.core.println.call(null,module$src$vendor$greeter.sayHello("Ciri"));
                                                             ^

TypeError: module$src$vendor$greeter.sayHello is not a function
    at Function.testapp.core._main.cljs$core$IFn$_invoke$arity$variadic (/home/niwi/tmp/greeter/out/testapp/core.js:28:62)
    at testapp$core$_main (/home/niwi/tmp/greeter/out/test