<< Back to previous view

[CLJS-830] Hashing issue in Mobile Safari clients Created: 30/Jul/14  Updated: 31/Jul/14

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

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


 Description   

Duplicate key errors occurring in Mobile Safari client no minimal case as of yet.



 Comments   
Comment by Thomas Heller [ 30/Jul/14 4:45 PM ]

I made a list of affected User Agents sending me maps with duplicate keys.

Full List available here:
https://gist.github.com/thheller/8cae79cabd9ac74958ca
WARNING: That is a list of ALL user agents that send me a map with duplicate keys. Some of those are from cljs-2268, but the mobile versions definitly still have this problem on 2277. Will try to make a better list. The examples below are from 2277 though.

Examples:
Mozilla/5.0 (iPad; CPU OS 7_1_2 like Mac OS X) AppleWebKit/537.51.2 (KHTML, like Gecko) Version/7.0 Mobile/11D257 Safari/9537.53
Mozilla/5.0 (iPhone; CPU iPhone OS 7_1_2 like Mac OS X) AppleWebKit/537.51.2 (KHTML, like Gecko) Version/7.0 Mobile/11D257 Safari/9537.53
Mozilla/5.0 (Linux; U; Android 4.1.2; de-de; GT-N8010 Build/JZO54K) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Safari/534.30

Comment by David Nolen [ 31/Jul/14 7:03 PM ]

Are you sure this isn't because clients have cached versions of your script this looks exactly like this bug: https://bugs.webkit.org/show_bug.cgi?id=126345. Mobile clients tend to have aggressive script caching policies.

Comment by Thomas Heller [ 31/Jul/14 7:27 PM ]

Yes, definitly not a cache issue. JS files are versioned. Just tried in Private Mode after wiping the cache. Still able to reproduce it.

I can give you an URL and a step-by-step if you want to try it. Its not exactly minimal but its better than nothing.

Comment by David Nolen [ 31/Jul/14 7:29 PM ]

Please do, I'd prefer to get this resolved before cutting another release. Thanks.





[CLJS-829] :emit-constants Keywords are emitted without hash value Created: 29/Jul/14  Updated: 29/Jul/14  Resolved: 29/Jul/14

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

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

Attachments: File emits-keyword.diff    
Patch: Code

 Description   

The cljs.compiler emits keywords as new cljs.core.Keyword(ns, name, str, hash) except when using :emit-constants which will emit new cljs.core.Keyword(ns, name, str).

The reason is a bit of duplicate code, the patch creates a new function emits-keyword which is then used in both places.

See:
https://github.com/clojure/clojurescript/blob/f0dcc75573a42758f8c39b57d1747a2b4967327e/src/clj/cljs/compiler.clj#L204
https://github.com/clojure/clojurescript/blob/f0dcc75573a42758f8c39b57d1747a2b4967327e/src/clj/cljs/compiler.clj#L1080



 Comments   
Comment by David Nolen [ 29/Jul/14 1:45 PM ]

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





[CLJS-822] "TypeError: invalid 'in' operand a" Created: 05/Jul/14  Updated: 29/Jul/14  Resolved: 29/Jul/14

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

Type: Defect Priority: Major
Reporter: Zack Piper Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: clojure.browser.repl, javascript
Environment:

Source: http://github.com/zackp30/crateincinerator
OS: Linux openSUSE 13.1, Bottle
Browser: Firefox 25.0



 Description   

When using `clojure.browser.repl` in a ClojureScript project, the evaluation fails due to the error `TypeError: invalid 'in' operand a @ http://127.0.0.1:3000/js/site.js:6559`
Source of line:
`if (d in a && !/^https?:\/\//.test(a[d])) {`
^ fails here, definitely syntax error.

Sorry if I have put this in completely the wrong place...

Thanks!



 Comments   
Comment by David Nolen [ 16/Jul/14 4:33 PM ]

This ticket need more information - complete simple steps to reproduce with no external dependencies beyond ClojureScript, thanks.

Comment by Zack Piper [ 28/Jul/14 8:06 AM ]

Wow, sorry for this, but it now works. I couldn't reproduce the error in this ticket, however, I got one ([14:05:57.342] TypeError: parentElm is null @ http://localhost:3000/js/main.js:33501), indicating that it was initializing before the document was ready (I blame JavaScript), so I put it at the bottom of the file, and bam, fixed.
Again, sorry.
Thanks!





[CLJS-828] Rhino support broken with new google-closure library Created: 25/Jul/14  Updated: 25/Jul/14  Resolved: 25/Jul/14

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

Type: Defect Priority: Minor
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

org.clojure/clojurescript "0.0-2268"
org.clojure/google-closure-library "0.0-20140226-71326067"



 Description   

cljs.repl.rhino/repl-env reads two files: goog/base.js and goog/deps.js. Unfortunatelly both are deprecated and empty in the latest version of google-closure-library.

Rhino ends with message:

org.mozilla.javascript.EcmaError: ReferenceError: "goog" is not defined. (bootjs#1)

Startup code has to be updated to reflect these changes.

goog/base.js
// This is a dummy file to trick genjsdeps into doing the right thing.
// TODO(nicksantos): fix this
goog/deps.js
/**
 * @deprecated This file is deprecated. The contents have been
 * migrated to the main deps.js instead (which is auto-included by
 * base.js).  Please do not add new dependencies here.
 */


 Comments   
Comment by Daniel Skarda [ 25/Jul/14 6:37 AM ]

Ups. It seems I have not been investigating the issue enough.

The problem is probably related to #826. Google Closure Library contains correct files, but it is google-closure-third-party which contains dummy files. I will investigate if the patch from #826 closes the issue with Rhino as well.

Comment by Daniel Skarda [ 25/Jul/14 7:21 AM ]

Confirmed. I deleted the files and Rhino works again (and cljs-nashorn as well). Please release the fix for #826 soon.

Comment by David Nolen [ 25/Jul/14 1:29 PM ]

Cutting a release today





[CLJS-824] Unsigned hash for keywords produced with (keyword s) Created: 06/Jul/14  Updated: 16/Jul/14  Resolved: 16/Jul/14

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

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

Attachments: Text File cljs_824-2.patch     Text File cljs_824.patch    

 Description   

ClojureScript will produce unsigned hash values at times (in particular for keywords produced with the keyword fn), and this is inconsistent with hashes produced on literal keywords and inconsistent with Clojure. The hash values will have the same 32-bit hex value, so this is perhaps not that critical for many uses.

Examples:

cljs.user=> (hash :op)
;=> -1882987955

cljs.user=> (hash (keyword "op"))
2411979341

This unsigned value is produced on Safari (shipping and WebKit nightly), and thus is in theory produced via both variants of the imul implementation (see CLJS-823). The unsigned value is also produced by Firefox.

In either case the value is equivalent to 0x8fc3e24d and works for many use cases, but it fails an = test.

user=> (= (hash :op) (hash (keyword "op")))
;=> true

cljs.user=> (= (hash :op) (hash (keyword "op")))
;=> false



 Comments   
Comment by Mike Fikes [ 06/Jul/14 8:27 PM ]

Not sure of the perf, but the attached cljs_824.patch appears to resolve the issue.

Comment by Mike Fikes [ 06/Jul/14 10:04 PM ]

Attached a simpler cljs_824-2.patch:

The JavaScript idiom to coerce to signed 32-bit is x|0. The revised patch does this, but by simply invoking the cljs core int fn (which in turn does a bit-or with 0).

Comment by David Nolen [ 16/Jul/14 4:32 PM ]

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





[CLJS-825] Conflict between cljs/nodejs.cljs & cljs/nodejs.js Created: 09/Jul/14  Updated: 16/Jul/14  Resolved: 16/Jul/14

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

Type: Defect Priority: Major
Reporter: Boris Kourtoukov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: nodejs


 Description   

This is the gist with errors and bare minimal example: https://gist.github.com/BorisKourt/7b1434ec490cfa6c8397



 Comments   
Comment by Boris Kourtoukov [ 09/Jul/14 2:15 PM ]

Noticed this while working with the main.cljs file linked in the above gist.

As described by David Nolen: cljs/nodejs.cljs cljs/nodejs.js are getting mixed up by the compiler even though they are completely different.

This explains why the error is intermittent, but the warning is consistent.

This is reproducible with an even more minimal example as well. Just wanted to link a basic but functional one.

Comment by John Wang [ 12/Jul/14 12:21 AM ]

As a more convenient test case, this also happens with samples/nodehello.cljs when following the build instructions given in the comments of that file.

Comment by David Nolen [ 16/Jul/14 4:27 PM ]

fixed https://github.com/clojure/clojurescript/commit/95122e1206d4a64f392cff03bfd73712ab7f3a33





[CLJS-813] Warn about reserved JS keyword usage in namespace names Created: 11/Jun/14  Updated: 16/Jul/14

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

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


 Description   

If a namespace is identified as foo.long.core it will get munged into foo.long$.core. This is unexpected and a source of confusion when goog.require("foo.long.core") fails.



 Comments   
Comment by Max Veytsman [ 15/Jul/14 4:50 PM ]

I'm starting to take a look at this.

Would it be most appropriate to add this check into compiler.cljs where the munging actually happens, or into analyzer.cljs where most of the warnings of this type live?

Comment by Mike Fikes [ 15/Jul/14 5:34 PM ]

If a solution is identified that eliminates “overly aggressive” munging for certain cases, then CLJS-689 could benefit.

Comment by Max Veytsman [ 16/Jul/14 2:44 PM ]

Currently, when munging "foo.bar.baz", we map over ["foo", "bar", "baz"] and check if each is a reserved word (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L94-L95)

According to my understanding of https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Reserved_word_usage and https://es5.github.io/#x7.6 reserved words are only disallowed in Identifiers. MemberExpressions and CallExpressions (the things with "."s in them) do not ban reserved words except in the first Identifier.

For our purposes, it could be enough to check if the entire token and the first period-seperated element is reserved. I.e. long becomes long$, long.night.ahead becomes long$.night.ahead, but foo.long.core remains foo.long.core.

Mike, this unfortunately won't affect CLJS-689

Does that sound like a good approach?





[CLJS-827] wrap macro expansion in try/catch Created: 14/Jul/14  Updated: 16/Jul/14  Resolved: 16/Jul/14

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

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

Attachments: Text File cljs_827.patch    

 Description   

This would allow us to report more accurate error file/line location when macroexpansions fails in Clojure, i.e. in the case of `defn`.



 Comments   
Comment by Max Veytsman [ 15/Jul/14 12:28 AM ]

It turned out that there was already a `wrapping-errors` macro that did what we needed. I used it within the body of `macroexpand-1` in order to annotate macro expansion errors with line numbers.

Comment by David Nolen [ 16/Jul/14 7:53 AM ]

fixed https://github.com/clojure/clojurescript/commit/13501318b4967eb5d0e486907e6cb35da81d60b9





[CLJS-826] Closure goog/base.js in third party closure release Created: 09/Jul/14  Updated: 16/Jul/14  Resolved: 16/Jul/14

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

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

Recent google/closure-library checkout


Attachments: Text File cljs-closure-release-script-fix.patch    
Patch: Code

 Description   

Not sure when this started but the more recent "org.clojure/google-closure-library-third-party" releases started containing a dummy "goog/base.js" which break build using my shadow-build library. The "make-closure-library-jars.sh" script used to delete that base.js but I guess the directory changed (maybe when the repo moved to github).

Patch just deletes the new location. Could possibly just forget about the old location, happy to provide a patch that replaces the paths instead of adding them.

Proof:

jar -tvf org/clojure/google-closure-library-third-party/0.0-20140226-71326067/google-closure-library-third-party-0.0-20140226-71326067.jar 
     0 Fri Mar 07 15:18:58 CET 2014 META-INF/
    68 Fri Mar 07 15:18:58 CET 2014 META-INF/MANIFEST.MF
   533 Fri Mar 07 15:18:56 CET 2014 AUTHORS
 10173 Fri Mar 07 15:18:56 CET 2014 LICENSE
   293 Fri Mar 07 15:18:56 CET 2014 README
     0 Fri Mar 07 15:18:56 CET 2014 goog/
   101 Fri Mar 07 15:18:56 CET 2014 goog/base.js
   822 Fri Mar 07 15:18:56 CET 2014 goog/deps.js


 Comments   
Comment by Thomas Heller [ 09/Jul/14 7:11 PM ]

https://github.com/clojure/clojurescript/commit/bae40ab9a9924a05a7425a734de2315c3185a5f6

This commit is to blame I guess, so its fine to just change the paths instead of keeping the old ones arround (since they never existed).

Comment by David Nolen [ 16/Jul/14 7:50 AM ]

fixed https://github.com/clojure/clojurescript/commit/13501318b4967eb5d0e486907e6cb35da81d60b9





[CLJS-823] Hashing regression with JavaScriptCore Created: 05/Jul/14  Updated: 06/Jul/14  Resolved: 06/Jul/14

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

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


 Description   

With https://github.com/clojure/clojurescript/commit/6ed5857aa49f28dc81390e3522e85b497be0f9de Weasel (https://github.com/tomjakubowski/weasel) functionality regressed for JavaScriptCore/Safari (but not with Firefox or Chrome), where the result appears to the end user as a Weasel hang when issuing a browser REPL command.

At its core, it appears to be related to a failure to parse / read a Weasel command of the form {:op :eval-js, :code "cljs.core.pr_s ... <omitted for brevity>"} within the browser, where the :op :eval-js keyword/value pair is not parsed, resulting in a subsequent multimethod dispatching on :op in Weasel's process-message multimethod to derail.

More detail is in https://github.com/james-henderson/simple-brepl/issues/4, but my plan is to attempt to formulate a minimal setup to reproduce the failure.



 Comments   
Comment by Mike Fikes [ 05/Jul/14 9:48 PM ]

Perhaps a minimal reproduction of the failure is achieved by invoking the cljs reader directly in JavaScript as described below:

Within the Safari JavaScript REPL, evaluating the following

> cljs.reader.read_string.call(null, "{:op :eval-js :code :x}");

results in a JavaScript object representing the map. Prior to the hashing commit referenced in this ticket's description, this map representation has 4 elements in its root arr (representing the 2 key/val pairs), where element 0 is "op", with a _hash value of 1013907795, which matches the :op keyword expression that is emitted in various places in my JavaScript output (new cljs.core.Keyword(null, "op", "op", 1013907795)).

After the hashing commit, evaluating the same at the JavaScript REPL, the resulting map representation is a bit "wonky" in Safari with respect to hash values (but is similar to that described in the previous paragraph, in Firefox). By "wonky", it's root arr has 2 elements, the 1st being null, and the 2nd containing an arr of length 4 (which appears to be the 2 key/val pairs, simply a bit lower in the trie), but where the hash is 1013904242 for both the item representing the "op" keyword and for the item representing the "code" keyword, and since this hash differs from that for the :op keyword expression (new cljs.core.Keyword(null, "op", "op", -1882987955)), the attempt to get :op from the map fails. (Note that in the Firefox debugger, the element associated with "op" has a hash of 2411979341, which actually matches -1882987955 (the first being an unsigned and the 2nd being 2's complement representation of the same 32-bit hex value).

Comment by Mike Fikes [ 05/Jul/14 11:22 PM ]

An equivalent, but perhaps simpler reduction of the bug:

(ns foo.bar
(:require [cljs.reader :as reader]))

(println "Expecting :js-eval here:" (:op (reader/read-string "{:op :js-eval :code :x}")))

With 0.0-2261 this will print

Expecting :js-eval here: nil

but with 0.0-2234 this will print

Expecting :js-eval here: :js-eval

if you run this in the shipping version of JavaScriptCore (Safari or iOS).

Note that this bug does not occur if you use a WebKit nightly. Thus, automated integration / unit tests for this against WebKit nightly fail to detect the problem.

Comment by Mike Fikes [ 06/Jul/14 8:12 AM ]

I think this is valid simpler reduction, focusing on the hash of a read keyword (these numbers reflect those 2 comments back):

(println (hash :op))
;=> -1882987955

(println (hash (let [r (reader/push-back-reader "op ")]
(reader/read-keyword r nil))))
;=> 1013904242

Comment by Mike Fikes [ 06/Jul/14 8:24 AM ]

A simpler reduction, independent of the reader code:

(hash :op)
;=> -1882987955

(hash (keyword "op"))
;=> 1013904242

Comment by Mike Fikes [ 06/Jul/14 8:58 AM ]

Perhaps it is related to hash cacheing rather than hash computation:

(hash (keyword "op"))
;=> 1013904242

(hash (keyword "code"))
;=> 1013904242

(hash (keyword "abc"))
;=> 1013904242

Comment by David Nolen [ 06/Jul/14 9:51 AM ]

All released versions of Safari 7 have a broken implementation of Math.imul, fall back to non-native imul implemented now in master https://github.com/clojure/clojurescript/commit/e92e8064813ed9a74c6dcf5bfd3adf5b85df1aea





[CLJS-821] specify/specify! fails for Object methods Created: 04/Jul/14  Updated: 04/Jul/14  Resolved: 04/Jul/14

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

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


 Comments   
Comment by David Nolen [ 04/Jul/14 1:59 PM ]

not a issue





[CLJS-790] Advanced compilation broken with latest closure libra Created: 04/Apr/14  Updated: 03/Jul/14  Resolved: 03/Jul/14

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

Type: Defect Priority: Critical
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None
Environment:

cljs >= 2197


Attachments: Text File cljs-790.patch     Text File cljs-790-updated.patch    

 Description   

CLJS releases >= 2197 are depending on the most recent packaged closure lib [org.clojure/google-closure-library "0.0-20140226-71326067"].

This is bad,



 Comments   
Comment by Francis Avila [ 04/Apr/14 3:46 PM ]

ARG, submitted too soon by accident.

Rundown is this:

  1. CLJS >= 2197 depends on the latest packaged closure lib (March 17th release, "0.0-20140226-71326067").
  2. This closure lib version is incompatible with the closure compiler closurescript depends on at least because of changes to how goog.base works. See commit 5bdb54d221dbf7c6177ba5ba6901c012981501ec on the closure compiler library (and many more after this one with a similar commit message).
  3. When you advanced-compile a cljs project, goog classes which internally use the goog.base(this, 'superMethod') form will all be munged incorrectly and throw exceptions. Minimal test case: (ns myproj (:require goog.net.XhrIo)) (goog.net.XhrIo/send "http://example.org" #(js/console.log %)). This will fail at a "disposeInternal" call. (Notice that the output js still has the "disposeInternal" string!)
  4. Oddly, the compiler does not emit any warnings about this!
  5. Additionally, the clojurescript project's POM and project.clj specify different versions of the closure library starting at https://github.com/clojure/clojurescript/commit/523a0c5b18138c9a4d23c5104a77b65488bc28c3 The POM specifies the new lib, but the project.clj the older one.

A workaround is to declare an explicit dependency in your project to [org.clojure/google-closure-library "0.0-20130212-95c19e7f0f5f"]. If you do this everything will start working again.

Basically, cljs can't use the new closure lib until its closure compiler has an updated release. (There is some hope that this may be easier to do in the future: https://groups.google.com/d/msg/closure-compiler-discuss/tKZQ1eLUixA/3urgnli84SYJ)

Comment by David Nolen [ 08/May/14 6:21 PM ]

Has this been resolved by a new closure compiler release yet? Thanks!

Comment by Francis Avila [ 11/May/14 11:48 AM ]

Donno, will give v20140407 release a try tonight.

Comment by Francis Avila [ 13/May/14 8:13 AM ]

Seems to work. Ran the test suite, benchmark, and some other projects I had which were failing before. Patch attached with updated dependencies.

Comment by Stephen Nelson [ 17/Jun/14 10:21 PM ]

This bug (or a similar one) is still affecting clojurescript 0.0-2234. Another example:

(ns myproj
(:require goog.net.XhrManager))
(.send (goog.net.XhrManager.) "xhr-request" "myproj.js" "GET" nil nil nil #(.log js/console %))

The workaround suggested above (using the old closure library) works, but versions of clojurescript newer than 2197 cannot be used in production without the workaround. If this bug is going to remain open in new releases, please consider documenting the workaround in the release notes.

Comment by David Nolen [ 18/Jun/14 10:42 AM ]

Can we get a new patch with a more recent release? http://search.maven.org/#artifactdetails%7Ccom.google.javascript%7Cclosure-compiler-parent%7Cv20140508%7Cpom

Comment by Francis Avila [ 24/Jun/14 10:30 AM ]

Bumped version. Note that this patch is unnecessary in the 1.6.0 branch, as its versions are up to date and its pom and project.clj agree. This commit is likely conflict when 1.6.0 is merged into master (the master side should be ignored if that happens).

Comment by David Nolen [ 01/Jul/14 9:25 PM ]

this should be resolved in master

Comment by Francis Avila [ 03/Jul/14 1:55 PM ]

It is. You can close.

Comment by David Nolen [ 03/Jul/14 2:10 PM ]

fixed in master with 1.6.0 work merge





[CLJS-677] cljs.reader doesn't support keywords starting with a digit Created: 12/Nov/13  Updated: 02/Jul/14

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

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


 Description   
ClojureScript:foo> (r/read-string ":0")
"Error evaluating:" (r/read-string ":0") :as "cljs.reader.read_string.call(null,\":0\")"
org.mozilla.javascript.EcmaError: TypeError: Cannot read property "0.0" from null (file:/home/chas/dev/clojure/cljs/.repl/cljs/reader.js#451)

The topic of leading digits in keywords came up separately, as they've been supported in Clojure for some time, but can now be considered part of the spec, as it were. See CLJ-1286.

BTW, this is another simple-check win...



 Comments   
Comment by Chas Emerick [ 21/Nov/13 9:38 AM ]

This is not a simple regex change, as I had hoped given the recent flurry in Clojure. The symbol pattern in cljs.reader is faithful to Clojure HEAD, but the processing of matches isn't. I think it may be a wash as to whether it'd be easier to fix what's there vs. porting clojure.tools.reader.impl.commons/parse-symbol (which incidentally doesn't use a regex)…either way, leaving it for another day (or someone else, if they're up for it).

Comment by Francis Avila [ 02/Jul/14 12:35 AM ]

I think I fixed the match processing issue you're talking about (CLJS-775 CLJS-776)? However I'm still confused by this and CLJ-1286. The clojure reader docs and edn spec still say they should reject `:0`, but 1.6.0 doesn't. What's the expected behavior? Is the spec going to be fixed, or clojure reader fixed once downstream packages are fixed?

Comment by Jozef Wagner [ 02/Jul/14 1:50 AM ]

AFAIK EDN specs do not reject :0 (no rule that the second character cannot be a digit). See https://github.com/wagjo/serialization-formats for my interpretation of existing specs.

Comment by Francis Avila [ 02/Jul/14 1:35 PM ]

Ah, I think I see the source of the confusion. Both EDN and the clojure reader spec both say something like "keywords are like symbols, except beginning with a colon." The confusion lies in whether we interpret that as meaning

  1. First character is a colon, then the second character and after are matched against the symbol definition.
  2. The first character is a colon, and the whole form is matched against the symbol definition.

CLJ-1003 CLJ-1252 and CLJ-1286 and myself all seem to understand the first meaning. This might be because when we say "the first character of a keyword" we typically mean the first character after the colon, as if the colon is "special" and not part of the keyword (e.g. like a reader macro character).

However clojure 1.6 seems to be following the second meaning (and explains why `:0/a` is ok but not `:0/0`), and I'm not sure from the cited tickets and google group discussions whether this is because of downstream breakage or if this is the intended interpretation and the patch from CLJ-1252 was accepted by Alex Miller erroneously.

Note if we accept the second interpretation, then the restriction "A symbol can contain one or more non-repeating ':'s." from the clojure reader docs is incorrect for keywords. (EDN doesn't allow namespace-expanded keywords, it seems, so it's not an issue there.)

Also EDN allows contiguous colons in symbols, whereas clojure 1.6 and the reader spec do not.

Comment by Francis Avila [ 02/Jul/14 2:11 PM ]

Also clojure 1.6 allows a/:a and :a/:a (where name part violates first-character rule for symbols), even though the specs do not. (This is something your table doesn't mention. Very thorough work BTW! I wish the reader spec was more formalized and unambiguous...)

Comment by Francis Avila [ 02/Jul/14 3:08 PM ]

I think this pattern follows the specs:

#"(?x)
(?!///) # edge case: / only allowed in name part.
# name or namespace part of symbol or keyword
(?:
 #division symbol
 (/
 # normal symbol
 |[a-zA-Z*!_?$%&=<>][0-9a-zA-Z*!_?$%&=<>\#:+.-]*
 # symbol starting with [-+.]
 |[-+.](?:[a-zA-Z*!_?$%&=<>\#:+.-][0-9a-zA-Z*!_?$%&=<>\#:+.-]*)?)
 # keyword
 |(::?)([0-9a-zA-Z*!_?$%&=<>\#:+.-]+))
# name part when namespace is present
(?:/(/ # division symbol
    |[a-zA-Z*!_?$%&=<>][0-9a-zA-Z*!_?$%&=<>\#:+.-]*
    |[-+.](?:[a-zA-Z*!_?$%&=<>\#:+.-][0-9a-zA-Z*!_?$%&=<>\#:+.-]*)?))?
# groups:
# 1: symbol name or namespace 2: keyword colon(s) 3: keyword name or namespace
# 4: keyword or symbol name (and groups 1 and 3 are namespaces)"

Problems:

  1. Does not enforce no-repeating-colon rule (but it is easy to validate after matching).
  2. Rejects violations of first-character-rule in symbols which clojure accepts.
  3. Accepts a trailing colon on namespace (unlike clojure).
  4. Accepts foo// or :foo//, which are not clearly addressed by the specs. (Jozef's table has more background). These are both allowed in Clojure 1.6, but not 1.5 or (arguably) edn.
Comment by Francis Avila [ 02/Jul/14 6:28 PM ]

Another problem: Accepts :::a/b, which I think is ok per the specs but is not read by 1.6. Crazy example:

user=> (require ['clojure.core :as (symbol ":a")])
nil
user=> :::a/map

RuntimeException Invalid token: :::a/map  clojure.lang.Util.runtimeException (Util.java:221)
user=> (resolve (symbol ":a" "map"))
#'clojure.core/map

Theoretically I might expect :::a/map to be read as :clojure.core/map?





[CLJS-810] re-matches returns [] if string is nil Created: 04/Jun/14  Updated: 02/Jul/14  Resolved: 02/Jul/14

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

Type: Defect Priority: Major
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

clojurescript 0.0-2227


Attachments: Text File cljs-810-nil.patch     Text File cljs-810.patch     Text File cljs-810-throw.patch    

 Description   

(re-matches #"no-match" "") => nil
(re-matches #"no-match" nil) => []

In clojure calling re-matches on nil throws an exception, but in clojurescript it returns [].



 Comments   
Comment by David Nolen [ 06/Jun/14 9:32 AM ]

this is a platform distinction, otherwise we need to put checks for nil and throw exceptions in several places.

Comment by Stephen Nelson [ 08/Jun/14 9:27 PM ]

I understand the need for efficiency and recognise that it is reasonable to have differences between platforms, but I don't think it's reasonable to return truthy when given null – the regex did not match, the function should return falsey.

I think the performance implication can be avoided using a case switch on the result of the existing length test:

Replace:

(if (== (count matches) 1)
(first matches)
(vec matches))

With:

(case (count matches)
0 nil
1 (first matches)
(vec matches))

Comment by Stephen Nelson [ 08/Jun/14 9:33 PM ]

Just to note, this is not a hypothetical problem, we encountered a bug in production where a 'local url' regex test passed when it was unexpectedly given nil. This bug was hard to track down because it's not obvious even from reading the implementation that the function could return truthy when the regex didn't match.

Comment by David Nolen [ 09/Jun/14 5:28 PM ]

Oh, I didn't realize that re-matches differs from re-find in this regard. Is there any reason that re-matches can't follow re-find? If it can, yes, patch welcome.

Comment by Francis Avila [ 09/Jun/14 6:06 PM ]

`re-find` and `re-matches` are both completely broken for nil because of js coersion of nil to the string "null". Your suggested fix (using a case statement) is not radical enough to solve this.

(re-find #"." nil)
; => "n"
(re-matches #"." nil)
; => nil

Note that `re-matches` itself doesn't work as you might expect: CLJS-776. You should use re-find with an explicit `^` and `$` instead.

I think both re-find and re-matches should have an explicit nil check on their string argument.

Comment by Francis Avila [ 12/Jun/14 2:51 AM ]

The fundamental issue here is that RegExp.exec(x) in javascript will x.toString() its argument before applying the regex. This leads to surprising behavior for anything which is not a string.

Options are:

  1. Call it a platform difference and leave current behavior as-is.
  2. Return nil if argument is not a string (unlike clojure, which throws).
  3. Throw if argument is not a string (like clojure and re-seq in cljs).

Attached patch takes option 2 and does nothing about re-seq (which currently throws). This is inconsistent with clojure but nil-punning is convenient. Option 3 seems equally valid, though.

Comment by David Nolen [ 12/Jun/14 10:13 AM ]

I think I prefer option 3 if it aligns us with Clojure, thanks!

Comment by Francis Avila [ 24/Jun/14 10:16 AM ]

Added patch which throws TypeError instead of nil for non-string.

Comment by David Nolen [ 01/Jul/14 9:29 PM ]

Can we rebase this patch to master? Thanks!

Comment by Francis Avila [ 01/Jul/14 11:27 PM ]

Rebased to master.

Comment by David Nolen [ 02/Jul/14 7:55 AM ]

fixed https://github.com/clojure/clojurescript/commit/774d4588581f6d29a1b6b2555171d3f3d36c2c83





[CLJS-769] clojure.core/unsigned-bit-shift-right not excluded in cljs.core Created: 17/Feb/14  Updated: 01/Jul/14

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

Type: Defect Priority: Trivial
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: None

Attachments: File CLJS-769.diff    
Patch: Code

 Description   

EOM



 Comments   
Comment by Kyle VanderBeek [ 17/May/14 8:34 PM ]

This defect does cause a warning with lein-cljsbuild 1.0.3 under Clojure 1.6.0 and ClojureScript 0.0-2202.

WARNING: unsigned-bit-shift-right already refers to: #'clojure.core/unsigned-bit-shift-right in namespace: cljs.core, being replaced by: #'cljs.core/unsigned-bit-shift-right
Comment by Francis Avila [ 01/Jul/14 10:57 PM ]

This is fixed in r2261 (the clojure 1.6 release).





[CLJS-715] Numbers are always emitted as literals Created: 06/Dec/13  Updated: 01/Jul/14  Resolved: 01/Jul/14

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

Type: Defect Priority: Minor
Reporter: George Fraser Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript 2080


Attachments: File cljs_715_00.diff    

 Description   

At the REPL:

> (.toString 1)
"Error evaluating:" (.toString 1) :as "1.toString()"
org.mozilla.javascript.EvaluatorException: missing ; before statement (<cljs repl>#3)

The emitted code `1.toString()` is a parse error, it should be `(1).toString()`



 Comments   
Comment by Alan Dipert [ 19/Jun/14 8:31 PM ]

I independently encountered this bug and came to the same conclusion about how to fix.

Attached is a patch that parenthesizes numbers without a trailing decimal, making method calls on them syntactically valid JavaScript.

Comment by David Nolen [ 01/Jul/14 9:34 PM ]

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





[CLJS-817] Warning on use of undeclared var when creating recursive definition Created: 17/Jun/14  Updated: 01/Jul/14  Resolved: 01/Jul/14

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

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

Attachments: Text File cljs_817.patch    

 Description   

If you create a definition that recursively refers to itself, a warning is issued. This is simply a warning; the generated code still works. The same code works without such a warning in Clojure.

Here is an example:

(ns foo.bar)

(def fib-seq (lazy-cat [0 1] (map + (rest fib-seq) fib-seq)))

This generates the following warning:

Compiling "js/main.js" from ["src/cljs" "src/clj"]...
WARNING: Use of undeclared Var foo.bar/fib-seq at line 3 src/cljs/foo/bar.cljs
WARNING: Use of undeclared Var foo.bar/fib-seq at line 3 src/cljs/foo/bar.cljs
Successfully compiled "js/main.js" in 5.819 seconds.



 Comments   
Comment by Mike Fikes [ 17/Jun/14 7:09 PM ]

An easy workaround is to forward declare. For the example given in the description,

(declare fib-seq)

suffices.

Comment by David Nolen [ 19/Jun/14 10:49 AM ]

Would be happy to take a patch that propagates the def'ed var so that the initializer expression has it during analysis.

Comment by Mike Fikes [ 19/Jun/14 3:58 PM ]

Patch attached.

Note that I'm a new contributor (I signed the online CLA), but I have no experience with the ClojureScript compiler codebase. In other words, increased review for this patch is appropriate IMHO.

Comment by David Nolen [ 19/Jun/14 4:45 PM ]

The patch initially looks OK will give a closer look tomorrow. Thanks!

Comment by Mike Fikes [ 20/Jun/14 9:54 AM ]

Thanks David. Let me know if the patch could use further work. Glad to revise as necessary.

Comment by David Nolen [ 01/Jul/14 9:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/054b0142e303586761d3da01849a2e24896ad7dd





[CLJS-818] Externs don't get loaded when running under immutant as cljs.js-deps/find-js-classpath fails Created: 18/Jun/14  Updated: 01/Jul/14

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

Type: Defect Priority: Major
Reporter: James Cash Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Java 1.8.0_05, Clojure 1.6.0, clojurescript 0.0-2234, immutant 1.1.1


Attachments: Text File 818.patch     PNG File Screen Shot 2014-06-18 at 20.14.59 .PNG    

 Description   

When compiling clojurescript that relies on library-provided externs (e.g. Om needing React.js externs), the clojurescript is compiled without errors, but the generated javascript fails to work, due to the externs not being loaded. Externs don't get loaded, as cljs.js-deps/find-js-classpath doesn't find the javascript externs file. This occurs because it uses cljs.js-deps/all-classpath-urls, which filters out the immutant classloader, since org.immutant.core.ImmutantClassLoader is not an instance of java.net.URLClassLoader (and hence lacks a .getURLs method anyway).



 Comments   
Comment by Toby Crawley [ 19/Jun/14 9:23 AM ]

Chas: Is there a reason not to depend on dynapath here? This exact case is kinda why it exists

Comment by David Nolen [ 19/Jun/14 10:47 AM ]

Patch welcome for this.

Comment by James Cash [ 19/Jun/14 2:12 PM ]

Simply replacing cljs.js-deps/all-classpath-urls with dynapath.util/all-classpath-urls worked for me. I don't know if there are policies around adding dependencies to cljs, but the following patch is working for me. Would it be preferable to re-implement the functionality instead?

Comment by David Nolen [ 19/Jun/14 2:19 PM ]

We are not going to take on a dependency for this. The code should be copied over, thanks.

Comment by James Cash [ 19/Jun/14 3:46 PM ]

Due to the way dynapath works, I don't think a straightforward copying of the code will work, since it relies on a protocol. Backing up a step though, would it be reasonable for externs to be loaded via io/resource, in the same way that the :preamble is?

Comment by Toby Crawley [ 19/Jun/14 3:54 PM ]

Unfortunately, the code can't be copied over. Dynapath works by providing a protocol that providers/users of funky classloaders can implement, allowing libraries that use dynapath to access the dynamic features of those classloaders without having to care about the loader's concrete type. Dynapath itself provides implementations for j.n.URLClassLoader and c.l.DynamicClassloader by default, so libraries don't have to do anything special to access the dynamic features of those classes.

java.classpath also provides a similar mechanism that the Immutant classloader implements as well. If you are more open to dependencies that are under org.clojure, using that will work as well. Ideally, I'd like to see java.classpath subsume dynapath.

Comment by James Cash [ 19/Jun/14 4:23 PM ]

Made a new patch that sidesteps the all-classpath-urls issue by just using io/resource instead of iterating over all urls

Comment by David Nolen [ 01/Jul/14 9:26 PM ]

Can people chime in whether the patch works for them, thanks.





[CLJS-754] Assess Murmur3 for Collections Created: 29/Jan/14  Updated: 01/Jul/14  Resolved: 01/Jul/14

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

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


 Description   

We should assess the performance implications of adopting Murmur3 for hashing - http://github.com/clojure/clojure/commit/dff9600387b962f16fc78e6477e10e34651fd366



 Comments   
Comment by Francis Avila [ 19/Feb/14 5:19 AM ]

I have a (lightly tested, probably untrustworthy) gist of clj.lang.murmur3 ported to javascript. The two other murmur3 js libs I saw both had flaws which I attempt to address.

Found a jsperf comparing murmur3 libs and added a goog.string.hashCode case as a baseline. This jsperf test isn't very good because:

  1. it only tests a tiny string
  2. the murmur libraries are both flawed:
    1. one of them assumes strings are 8-bit only (consumes 4 chars at a time for hashing)
    2. the other one treats strings as utf-16, but does not account for integer overflows.

However, the results are still informative--string hashing with murmur3 in js will probably be half as fast as goog.string.hashCode, except on firefox where it is about the same.

Not nearly enough data for a true assessment, but I hope it helps.

Comment by Francis Avila [ 19/Feb/14 10:20 AM ]

It occurs to me that the slowdown is probably entirely due to integer multiplication, and that spidermonkey's jit is detecting and optimizing that pattern of code to Math.imul while the others are not. Math.imul is still experimental but supported on many browsers already--it might be better to polyfill it instead of unconditionally inlining the bit-shifting. Will try later.

Comment by David Nolen [ 19/Feb/14 2:23 PM ]

There's very little need to write this logic in JavaScript - it should just be done in ClojureScript. I think modern JS engines can probably handle Murmur3, here are some perf numbers for simulating multiplication of 2 unsigned 32bit ints that I found on StackOverflow http://jsperf.com/bit-multiply

Comment by Francis Avila [ 19/Feb/14 3:14 PM ]

I agree re: implementing in clojurescript. It looked hairier when I started.

You're also right about the bit multiply. I extended that case with larger input values and a comparison against Math.imul and it seems to be compiling to the same code. Maybe inlining the bit-multiply is confusing the jits, or maybe it's something else entirely. I'll give it another shot when I get a chance.

Any advice on setting up tests for this that would work in the clojurescript project's environment? Probably what we want is a generative test which ensures the hash value in cljs is the same as in clj?

Comment by David Nolen [ 19/Feb/14 4:34 PM ]

Generative tests that guarantee the hashes are the same in both CLJS & CLJ is a nice to have and not really a priority. Murmur3 is pretty straightforward looking, I'm sure we can get it right

Comment by David Nolen [ 20/Feb/14 9:57 AM ]

One of the V8 engineer did a more accurate version of the benchmark. It looks like Math.imul is the way to go and we should shim in something for older browsers.

http://jsperf.com/bit-multiply/5

Comment by Francis Avila [ 22/Feb/14 2:20 AM ]

I thought those results looked too good. 32-bit maths in js is always tricky, hence desire for tests, especially if one of the use cases is comparing compile-time and run-time hashes.

Cleaned up js implementation of murmur3 just to get something up quickly to assess murmur3 performance. It uses Math.imul or a shim for integer multiplication. Then created a pair of jsperfs:

In both tests, setup code is the same: pretty-printed closure-advanced-compiled code from my js-murmur3 implementation, and copy-pasted code from cljs's current number and string hashing.

Results are...interesting. So interesting that I am suspicious that something is wrong with my benchmarks or code. Perhaps you can have Mr. Egorov take a look? In summary:

  • murmur3 int hash is about an 8% performance regression in firefox and safari.
  • murmur3 string hash is more than double the speed of goog.string.hashCode on ff and safari for small strings, and even better for large ones.
  • Except in chrome, where murmur3 is abysmal--about an order of magnitude regression on both ints and strings!

All these browsers have a native Math.imul, and Chrome's imul is definitely working. There must be something else making chrome's jit cranky.

I did not expect murmur3 to perform so well or so badly!

Comment by Francis Avila [ 26/Feb/14 3:37 AM ]

Maintaining a fork with murmur3 hashing (see murmur3 branch). So far numbers, strings, and collections (ordered and unordered) hash identically to clojure 1.6-beta1, but symbols and keywords do not. I suspect integer-math differences in cljs's hash-combine vs clojure.lang.Util/hashCombine, but I have not isolated the problem yet.

Comment by Francis Avila [ 26/Feb/14 3:06 PM ]

Nevermind, cljs and clj reverse the order of ns+name hashing and I just didn't notice.

This branch appears to produce hashes identical to clojure 1.6 for the following types:

  • strings
  • keywords and symbols
  • vectors, lists, persistentqueue, seqs
  • maps, sets
  • integers representable with doubles in js (i.e., about 53 bits of signed integer)

Types which do not hash the same (likely incomplete list):

  • uuids
  • instances
  • floating-point types. Clojure 1.6 still uses Java's native hashCode for these, and checking for a non-int is a bit involved in js, so I just hash all js numbers as if they were longs. This might be a bad idea if we have a collection of "real" doubles.
Comment by David Nolen [ 08/Jun/14 12:15 PM ]

Murmur3 is now implemented in the 1.6.0 branch http://github.com/clojure/clojurescript/tree/1.6.0

Comment by David Nolen [ 01/Jul/14 9:25 PM ]

fixed in master





[CLJS-819] cljs.reader cannot handle character classes beginning with slashes in regex literals Created: 20/Jun/14  Updated: 01/Jul/14  Resolved: 01/Jul/14

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

Type: Defect Priority: Critical
Reporter: Ziyang Hu Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug, cljs, reader

Attachments: Text File cljs-819.patch    

 Description   

cljs.user> (cljs.reader/read-string "#\"\\s\"")
Compilation error: Error: Unexpected unicode escape \s



 Comments   
Comment by Ziyang Hu [ 20/Jun/14 10:03 AM ]

This in particular means that (cljs.reader/read-string (str [#"\s"])) won't work

Comment by Francis Avila [ 25/Jun/14 11:46 AM ]

Patch and test.

Comment by David Nolen [ 01/Jul/14 9:25 PM ]

fixed https://github.com/clojure/clojurescript/commit/32259c5ff3f86ea086ae3949403df80c2f518c7e





[CLJS-820] MetaFn invoke without arguments is missing Created: 01/Jul/14  Updated: 01/Jul/14  Resolved: 01/Jul/14

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

Type: Defect Priority: Minor
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript from github aa9e5fe (post r2234)


Attachments: Text File 0001-CLJS-820-Missing-invoke-without-arguments-in-MetaFn.patch    

 Description   

MetaFn lacks definition of invoke without arguments. Missing invoke breaks asynchronous tests in cemerick.cljs.test.
Fix is trivial, patch attached.



 Comments   
Comment by David Nolen [ 01/Jul/14 9:22 PM ]

fixed https://github.com/clojure/clojurescript/commit/08832549ce2ed386300fa637d7f88a75d57cc344





[CLJS-816] clojure.set/rename-keys accidentally deletes keys when there's a collision. Created: 13/Jun/14  Updated: 24/Jun/14  Resolved: 24/Jun/14

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

Type: Defect Priority: Minor
Reporter: Brian Kim Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-816.patch    

 Description   

Same as the error on the clojure side (http://dev.clojure.org/jira/browse/CLJ-981). rename-keys will drop keys when there's a mutual overwrite.



 Comments   
Comment by Brian Kim [ 13/Jun/14 11:26 PM ]

Probably messed this up. Sorry!

Comment by David Nolen [ 16/Jun/14 11:05 AM ]

Can we please include a test with the patch? Thanks.

Comment by Brian Kim [ 23/Jun/14 9:07 PM ]

with test.

Comment by David Nolen [ 24/Jun/14 11:26 AM ]

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





[CLJS-776] re-matches is incorrect Created: 28/Feb/14  Updated: 24/Jun/14

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

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


 Description   

The re-matches function does not have the correct semantics: it performs a search (not match) against the string and returns nil if the string and matched-string are unequal. This is not the same as true matching, which is like inserting "^" and "$" at the beginning and end of the pattern.

Example in Clojure:

user=> (re-find #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
"0"
user=> (re-matches #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
"0x1"

Compare Clojurescript:

ClojureScript:cljs.user> (re-find  #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
"0"
ClojureScript:cljs.user> (re-matches #"0|[1-9][0-9]+|0[xX][0-9a-zA-Z]+" "0x1")
nil

This bug is (one of the) reasons why CLJS-775.

I'm not completely sure what to do here. My first thought is to have re-matches inspect the -source property of its regex input, wrap the string with "^$", then carefully copy all flags over to a new regexp.

Questions:

  1. Are there any valid patterns where this is not safe? E.g., where we could not put ^ first? Is "^^abc$$" ok?
  2. Can we avoid cloning if ^ and $ are already the first and last chars of the pattern?
  3. How does multiline mode play in to this, if at all?
  4. regexinstance.lastIndex is a piece of mutability on regex instances (or the RegExp global on older browsers) which is used as a string offset for multiple invocations of exec() on the same string. I have no idea what to do if re-* gets a regex with the global flag set. (BTW, this is a very good reason to reject CLJS-150: allowing clojure to accept the global flag makes regular expression objects stateful, and would completely screw up re-seq for example.)


 Comments   
Comment by Francis Avila [ 24/Jun/14 7:37 AM ]

I would like to propose a somewhat radical suggestion that would: fix this issue and CLJS-810, put us in a better position to resolve CLJS-485 CLJS-746 CLJS-794 (clojure.string/replace woes), allow us to add some regex-as-a-value niceties to patterns in js (CLJS-67 and CLJS-68), and bring clojurescript's regular expression handling closer to clojure's by implementing more of the re-* functions.

Example implementation (not a patch) at this cljsfiddle: http://cljsfiddle.net/fiddle/favila.regexp

Essential points:

  1. Create a Pattern object, created by re-pattern, which provides methods to create regexps for search (re-find) or exact match (re-matches) or repeated searches (re-seq, re-matcher + re-find). Each of these must be a different RegExp object in javascript even though they are similar regular expression strings. The re-find and re-matches patterns can be cached. All can generate RegExps lazily.
  2. regular expression literals emit these Pattern objects instead of RegExp objects.
  3. Create a Matcher object to correspond to the currently-unimplemented re-matcher. It combines a global-flagged RegExp object, a search string, and a done flag. If it keeps the last match (similar to java), cljs can also implement re-groups.
  4. Make re-seq use the Matcher object and thus the .lastIndex that native RegExps provide for global matches. (Its implementation no longer requires string slicing after every match.)
  5. If re-find is given a native RegExp object instead of a pattern, it will use it as-is. This matches current behavior.
  6. If re-matches is given a native RegExp object and it isn't suitable for exact-matching, a new RegExp is cloned from the input RegExp with ^ and $ prepended and appended and the global flag added. (This technique is used in clojure.string/replace, but incorrectly.)

Thoughts?





[CLJS-713] optimized case Created: 04/Dec/13  Updated: 23/Jun/14

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

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

Attachments: Text File 0001-CLJS-713-Allow-test-expressions-for-case-to-be-chars.patch     Text File 0001-CLJS-713-first-cut-at-compiling-case-to-switch.patch    

 Description   

With the advent of asm.js many engines will like compile switch statements over integers into jump tables. We should provide a real `case*` ast node that compiles to JS `switch` when possible - i.e. numbers, strings, keywords etc.



 Comments   
Comment by Michał Marczyk [ 18/Feb/14 5:56 PM ]

First cut impl also available here:

https://github.com/michalmarczyk/clojurescript/tree/713-compile-case-to-switch

With this patch applied, case expressions are compiled to switch + some extra bits when all tests are numbers or strings, otherwise old logic is used.

For example, {{(fn [] (let [x 1] (case x 1 :foo (2 3) :bar :quux)))}} gets compiled to

function () {
    var x = 1;
    var G__6469 = x;
    var caseval__6470;
    switch (G__6469) {
      case 1:
        caseval__6470 = new cljs.core.Keyword(null, "foo", "foo", 1014005816);
        break;
      case 2:
      case 3:
        caseval__6470 = new cljs.core.Keyword(null, "bar", "bar", 1014001541);
        break;
      default:
        caseval__6470 = new cljs.core.Keyword(null, "quux", "quux", 1017386809);
    }
    return caseval__6470;
}

The existing test suite passes, but I suppose it wouldn't hurt to add some tests with case in all possible contexts.

Comment by Michał Marczyk [ 18/Feb/14 6:05 PM ]

As a next step, I was planning to arrange things so that numbers/strings are fished out from among the tests and compiled to switch always, with any leftover tests put in an old-style nested-ifs-based case under default:. Does this sound good?

It seems to me that to deal with symbols and keywords in a similar manner we'd have to do one of two things:

1. check symbol? and keyword? at the top, then compile separate switches (the one for keywords would extract the name from the given keyword and use strings in the switch);

2. use hashes for dispatch.

Which one sounds better? Or is there a third way?

Comment by Michał Marczyk [ 18/Feb/14 6:11 PM ]

Of course we'd need to compute hashes statically to go with 2. I'd kind of like it if it were impossible (randomized seed / universal hashing), but currently it isn't.

Comment by Francis Avila [ 19/Feb/14 12:22 AM ]

At least on v8, there are surprisingly few cases where a switch statement will be optimized to a jump table. Basically the type of the switched-over value must always (across calls) match the type of every case, and there must be fewer than 128 cases, and integer cases must be 31-bit ints (v8's smi type). So mixing string and number cases in the same switch guarantees the statement will never be compiled. In many cases an equivalent if-else will end up being significantly faster on v8 just because the optimizing jit recognizes them better. There's an oldish bug filed against v8 switch performance. Looking at the many jsperfs of switch statements, it doesn't seem that v8 has improved. Relevant jsperf

Firefox is much better at optimizing switch statements (maybe because of their asm.js/emscripten work) but I don't know what conditions trigger (de)optimization.

I suspect the best approach is probably going to be your option one: if-else dispatch on type if any case is not a number, and then a switch statement covering the values for each of the keyword/string/symbol types present (no nested switch statements, and outlining the nested switches might be necessary). Even with a good hash, to guarantee v8 optimizing-compilation you would need to truncate the hashes into an smi (signed-left-shift once?) inside the case*.

Comment by David Nolen [ 19/Feb/14 12:50 AM ]

There's no need for invention here. We should follow the strategy that Clojure adopts - compile time hash calculation.

Comment by Francis Avila [ 19/Feb/14 3:09 PM ]

The problem, as Michal alluded to, is that the hash functions in cljs's runtime environment are not available at compile-time (unlike in Clojure). This might be a good opportunity to clean up that situation or even use identical hash values across Clojure and Clojurescript (i.e. CLJS-754), but that's a much bigger project. Especially considering it will probably not bring much of a speedup over an if-else-if implementation except in very narrow circumstances.

Comment by David Nolen [ 19/Feb/14 4:38 PM ]

Francis Avila I would make no such assumptions about performance without benchmarks. One of the critical uses for case is over keywords. Keyword hashes are computed at compile time, so that's one function call and a jump on some JavaScript engines. This is particularly useful for the performance of records where you want to lookup a field via keyword before checking the extension map.

This ticket should probably wait for CLJS-754 before proceeding.

Comment by Francis Avila [ 22/Feb/14 4:44 AM ]

Record field lookup is a good narrow use case to test. I put together a jsperf to compare if-else (current) vs switch with string cases vs switch with int cases (i.e., hash-compares, assuming perfect hashing).

Comment by David Nolen [ 10/May/14 3:59 PM ]

I've merged the case* analyzer and emitter bits by hand into master.

Comment by David Nolen [ 10/May/14 4:42 PM ]

I'v merged the rest of the patch into master. If any further optimizations are done it will be around dispatching on hash code a la Clojure.

Comment by Francis Avila [ 11/May/14 12:53 AM ]

Your keyword-test optimization has a bug: https://github.com/clojure/clojurescript/commit/9872788b3caa86f639633ff14dc0db49f16d3e2a

Test case:

(let [x "a"] (case x :a 1 "a"))
;=> 1
;;; Should be "a"

Github comment suggests two possible fixes.

Comment by David Nolen [ 11/May/14 10:50 AM ]

Thanks fix in master.

Comment by Christoffer Sawicki [ 23/Jun/14 3:41 PM ]

case over "chars" is currently not being optimized to switch (in other words: (case c (\a) :a :other) uses if instead of switch).

Given that ClojureScript chars are just strings of length 1, could this perhaps simply be fixed by tweaking https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/core.clj#L1187 ?

Comment by Christoffer Sawicki [ 23/Jun/14 4:11 PM ]

OK, I couldn't resist trying and it seems to be that easy. Would be great if somebody more knowledgeable could look at it and say if it has any side-effects. (See patch with name 0001-CLJS-713-Allow-test-expressions-for-case-to-be-chars.patch.)

Comment by David Nolen [ 23/Jun/14 4:15 PM ]

The patch looks good I would have applied it if I hadn't already gone and done it master myself just now

Comment by Christoffer Sawicki [ 23/Jun/14 4:22 PM ]

Hehe. Thanks! Don't forget to update the "case* tests must be numbers or strings" message on line 496 too.

Comment by David Nolen [ 23/Jun/14 4:48 PM ]

The existing docstring is inaccurate - case supports all compile time literals.





[CLJS-575] cljsc.bat emit FileNotFoundException when compile samples in windows Created: 25/Aug/13  Updated: 19/Jun/14

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

Type: Defect Priority: Major
Reporter: Park Sang Kyu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, bug, patch
Environment:

in windows 7


Attachments: File cljsc.bat.diff     File cljsc-path.bat    
Patch: Code

 Description   

cljsc.bat emit FileNotFoundException when it compile samples of the ClojureScript project in windows like below.

------------------------------------------------
Exception in thread "main" java.io.FileNotFoundException: Could not locate cljs/closure__init.class
or cljs/closure.clj on classpath:
------------------------------------------------

It is caused by lack of a backslash in the end of path of the system environment variable, %CLOJURESCRIPT_HOME% set by a user.
In the case CLASSPATH is set to "C:\\clojure\clojurescriptsrc\clj;C:\\clojure\clojurescriptsrc\cljs" and this make it impossible for javac to find cljs/clojure.clj file.

So it can be solved by adding a backslash to the path of %CLOJURESCRIPT_HOME%.

I attached the patched file, "cljsc-path.bat"



 Comments   
Comment by David Nolen [ 04/Sep/13 11:04 PM ]

Can we please get a proper git diff thanks (and please send in your CA)! Also would be nice to get Windows users to check this out.

Comment by Park Sang Kyu [ 15/Sep/13 3:16 AM ]

git diff

Comment by David Nolen [ 05/Oct/13 11:55 AM ]

Thank you! Have you sent in your CA? http://clojure.org/contributing

Comment by Park Sang Kyu [ 19/Jun/14 10:24 AM ]

Yes i have sent my CA.

Comment by David Nolen [ 19/Jun/14 10:27 AM ]

Excellent, the patch is not correctly formatted. Can we get a new patch that conforms to http://github.com/clojure/clojurescript/wiki/Patches





[CLJS-812] Recurring from a case statement emits invalid JavaScript Created: 09/Jun/14  Updated: 13/Jun/14  Resolved: 13/Jun/14

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

Type: Defect Priority: Critical
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug, compiler


 Description   

In 0.2227, compiling the following form produces syntactically invalid JavaScript:

(defn reproduce
  [value]
  (case value
    :a (recur :b)
    :b 0))

Yields:

bug_repro_test.reproduce = (function reproduce(value) {
    while (true) {
        var G__5832 = (((value instanceof cljs.core.Keyword)) ? value.fqn : null);
        var caseval__5833;
        switch (G__5832) {
            case "b":
                caseval__5833 = 0
                break;
            case "a":
                caseval__5833 = {
                    var G__5834 = cljs.core.constant$keyword$67;
                    value = G__5834;
                    continue;
                }

                break;
            default:
                caseval__5833 = (function () {
                    throw (new Error(("No matching clause: " + cljs.core.str.cljs$core$IFn$_invoke$arity$1(value))))
                })()
        }
        return caseval__5833;
        break;
    }
});

When evaluated in any JavaScript environment (including the Google Closure compiler) environment, this yields a syntax error in this code:

caseval__5833 = {
                    var G__5834 = cljs.core.constant$keyword$67;
                    value = G__5834;
                    continue;
                }


 Comments   
Comment by David Nolen [ 10/Jun/14 7:56 AM ]

Good catch. I suspect that throw may be similarly problematic.

Comment by David Nolen [ 13/Jun/14 3:59 PM ]

fixed in master https://github.com/clojure/clojurescript/commit/cc11c7996ba8522d6767fb45df2f76e20e4c1773





[CLJS-815] Range is overwritten rather than shadowed Created: 13/Jun/14  Updated: 13/Jun/14

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

Type: Defect Priority: Major
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

cljs-0-0-2173
https://github.com/jamii/range-bug/blob/master/project.clj



 Description   

https://github.com/jamii/range-bug

https://github.com/jamii/range-bug/blob/master/src/range/core.cljs defines a (deftype Range ...)

https://github.com/jamii/range-bug/blob/master/out/range/core.js should contain

range.core.Range = ...

but instead contains

cljs.core.Range = ...

This breaks cljs.core/range.

The only warning given is 'WARNING: >Range already refers to: />Range being replaced by: range.core/->Range at line 3 src/range/core.cljs' which is correct even in the expected case of shadowing Range rather than overwriting.

Adding (:refer-clojure :exclude [Range]) produces the correct result.






[CLJS-814] clojure.string/reverse breaks surrogate pairs Created: 12/Jun/14  Updated: 12/Jun/14

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

Type: Defect Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: string
Environment:

r2227, NOT under Rhino.


Attachments: Text File cljs-814.patch    
Patch: Code and Test

 Description   

clojure.string/reverse will reverse a string by code units instead of code points which causes surrogate pairs to become reversed and unmatched (i.e., invalid utf-16).

For example, in clojurescript (clojure.core/reverse "a\uD834\uDD1Ec") will produce the invalid "c\uDD1E\uD834a" instead of "c\uD834\uDD1Ea". Clojure produces the correct result because the underlying Java String.reverse() keeps surrogate pairs together.

Note that clojurescript running under Rhino will produce the same (correct) result as clojure, probably because Rhino is using String.reverse() internally.

Attached patch gives clojure.string/reverse the exact same behavior in clj and cljs (including non-commutativity for strings with unmatched surrogates).

(Also, neither clojure nor clojurescript nor java reverse combining characters correctly--the combining character will "move" over a different letter. I suspect this is a WONTFIX for both clojure and clojurescript, but it is fixable with another regex replacement.)



 Comments   
Comment by Francis Avila [ 12/Jun/14 1:27 AM ]

Forget what I said about Rhino, it's just as broken there. I think I got my repls confused or something.





[CLJS-811] cljs.js-deps/goog-resource doesn't get the correct classloader when running under immutant Created: 05/Jun/14  Updated: 06/Jun/14  Resolved: 06/Jun/14

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

Type: Defect Priority: Minor
Reporter: James Cash Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: classloader, cljsc
Environment:

Java 1.8.0_05, Clojure 1.6.0, clojurescript 0.0-2202, immutant 1.1.1



 Description   

When attempting to call cljs.closure/build when running under immutant, cljs.js-deps/goog-dependencies* fails, because the classloader goog-resources uses doesn't seem to be correct: In a repl, I can see that ClassLoader/getSystemClassLoader doesn't find goog/deps.js, but using (.getContextClassLoader (Thread/currentThread)) (as in clojure.java.io/resource) works:

user=> (enumeration-seq (.getResources (ClassLoader/getSystemClassLoader) "goog/deps.js"))
nil
user=> (enumeration-seq (.getResources (.getContextClassLoader (Thread/currentThread)) "goog/deps.js"))
(#<URL vfs:/Users/james/Work/LeanPixel/Next36/blocks/tmp_mounts/blocks.clj/google-closure-library-third-party-0.0-20140226-71326067.jar/goog/deps.js> #<URL vfs:/Users/james/Work/LeanPixel/Next36/blocks/tmp_mounts/blocks.clj/google-closure-library-0.0-20140226-71326067.jar/goog/deps.js>)



 Comments   
Comment by David Nolen [ 06/Jun/14 9:50 AM ]

fixed https://github.com/clojure/clojurescript/commit/737ccb0aab55816ce7f49dfb86130fbb34445bb8





[CLJS-809] tools.reader 0.8.4 causes clojurescript to stop working in mysterious ways Created: 04/Jun/14  Updated: 06/Jun/14  Resolved: 06/Jun/14

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

Type: Defect Priority: Major
Reporter: James Cash Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X 10.9.3, Java 1.8, Clojure 1.6.0



 Description   

If tools.reader 0.8.4 is included in a project's dependencies, then clojurescript fails to work in a browser, with the error that cljs.core.PersistentArrayMap is undefined. I have created a sample project demonstrating this at https://github.com/jamesnvc/cljs-toolreader-debugging.

It was suggested by Nicola Momento on tools.reader that the issue is due to "tools.reader 0.8.4 introduced :file metadata, clojurescript needs to dissoc it here https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1490 to work with this version."






[CLJS-449] stack traces for ex-info Created: 23/Dec/12  Updated: 05/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Tom Jack Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-hack-ex-info-stacktraces.patch    

 Description   

Currently, ex-info exceptions have no stack traces, which makes them.. much less useful.

Attached patch (0001-hack-ex-info-stacktraces.patch) is one hackish way of getting stack traces back when Error.captureStackTrace is available (at least in Chrome, presumably in Node as well).

But this seems unacceptable — all the stuff emitted for the deftype (making the constructor print as a type) gets overwritten.

Is there a better way?

Will this maybe be made irrelevant by source maps?



 Comments   
Comment by David Nolen [ 23/Dec/12 11:51 AM ]

Isn't the stack trace recoverable from the original exception?

Comment by Tom Jack [ 23/Dec/12 4:01 PM ]

I hadn't considered that. If a cause is provided, that seems reasonable. The only trouble is that you still have to know where the ExceptionInfo is being thrown, so that you can catch it and throw the cause. Chrome's debugger would make it easy to at least find where the ExceptionInfo is thrown, but it's still more trouble than having the appropriate stack trace directly on the ExceptionInfo.

If no cause is provided, of course that won't work. But I suppose one could write (ex-info msg data (js/Error.)) instead of (ex-info msg data)? Personally, I don't expect to pass a cause very often — I expect the binary arity to be much more common in my cljs libraries.

If we rely on the cause for the stack trace, maybe we could have the cause default to a new Error if not supplied? It seems sort of conceivable that we could also wire up the ExceptionInfo to proxy .stack to that Error so that the stacktrace will come through, without needing to override the ExceptionInfo constructor.





[CLJS-808] Warning from `find-classpath-lib` mistakenly included in generated source Created: 02/Jun/14  Updated: 02/Jun/14

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

Type: Defect Priority: Major
Reporter: Joshua Ballanco Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

clojurescript 0.0-r2227
target: node


Attachments: Text File warn-with-out-str-fix.patch    
Patch: Code

 Description   

When compiling for node.js, `find-classpath-lib` generates a warning because `cljs/nodejs.js` doesn't contain a `goog.provide` declaration. However, this warning ends up being emitted into the JS files being generated (caused by a call to `with-out-str`), later causing a critical failure in the Closure compiler.

To reproduce:

  • `lein new cljs-node buggy`
  • Update clojurescript to r2227, lein-cljsbuild to 1.0.3
  • `lein cljsbuild once`

Result:

  • Multiple warnings
  • Generated "buggy.js" file contains only node.js shebang

Fix:

  • The simplest patch (attached) is to rebind `out` to `err` when emitting a warning
  • A better fix would be to write a function for error printing that wraps this idiom
  • Ideally, cljs/nodejs.js should have a `goog.provides` line added to prevent the warning in the first place


 Comments   
Comment by David Nolen [ 02/Jun/14 7:45 AM ]

A patch that adds a goog.provide to the file is preferred, thanks!





[CLJS-656] Look for goog-style JavaScript dependencies on the classpath automatically Created: 01/Nov/13  Updated: 21/May/14  Resolved: 20/May/14

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJS-656.diff    
Patch: Code

 Description   

Right now, only actual goog.* JavaScript dependencies (as shipped w/ GClosure or in its "third-party" library and specified in its deps.js file) are available via (:require ...), etc.; other dependencies that contain goog.provide "declarations" can only ever be loaded via :libs and the classpath scanning (or direct filesystem reference) it provides.

This should change so that any referenced Closure-safe JavaScript dependencies may be resolved from their expected location on the classpath. e.g. (:require foo.bar) would resolve to a foo/bar.js resource on the classpath, which presumably would be required to contain a goog.provide("foo.bar") declaration.

With this change, :libs would be downgraded to support only direct references to filesystem files. (Q: should the option be renamed to reflect this, and to provide an easy configuration validation check for people that are using :libs as it exists now?)

This would be a breaking change to downstream tooling.

(The above is a summary of #clojure discussion with David Nolen.)



 Comments   
Comment by Chas Emerick [ 01/Nov/13 3:06 PM ]

This would eliminate the need for CLJS-526

Comment by Michael Alyn Miller [ 01/Nov/13 11:27 PM ]

I would really like to see this change. I am building a ClojureScript wrapper around a JavaScript library (which I GClosure-ified) and the only real way to do this at the moment is with your {{:libs [""]}} trick. That works, but it also pollutes the project.clj of every consumer of the library.

Comment by David Nolen [ 08/Nov/13 12:26 PM ]

Related information, it appears I was wrong about GClosure following Java classpath convention for layout. In order to resolve the location of Google Closure Library files, closure.clj parses deps.js in the Google Closure Library JAR.

Still I think we should move forward with the approach outlined here.

Comment by Chas Emerick [ 08/Nov/13 2:25 PM ]

My current understanding is that deps.js is an optional delivery mechanism, and perhaps is relatively unused outside of GClosure itself. I still need to dig into the GClosure packaging guidelines, probably won't happen pre-Conj; until then, I can't add much except that I also much prefer the current proposal than attempt to find, consume, and produce these deps.js files. It's not as if people are banging down the door for GClosure-packaged library compat, etc.

Comment by David Nolen [ 08/Nov/13 2:37 PM ]

Pretty much summarizes my thoughts as well.

Comment by Brenton Ashworth [ 11/Nov/13 9:35 AM ]

In my opinion this is a good change. What follows is some context that I have on the purpose of the :libs option. I hope this information is both accurate and helpful.

At the birth of ClojureScript it was not clear how much we would depend on the Java classpath. I think it has become clear that the compiler is typically used as a library which has access to the project's classpath. We should embrace this and make it more friendly to use in this environment.

In the first version of ClojureScript, the compiler could compile a file without resolving dependencies. Problems with deps where either found at run time or when files are handed to the GClosure compiler for compilation. This may no longer be true. I am ashamed to say that I haven't looked at the source in a while. It is important to remember that GClosure deals only with JavaScript sources. Once all ClojureScript sources have been compiled to JavaScript, a set of files is handed to the GClosure compiler for optimization. The purpose of the `:libs` option is to allow any other JavaScript sources which are GClosure compatible, and upon which the sources depend, to be found and handed to the GClosure compiler.

In the beginning, we added GClosure support (dealing with JavaScript files) on top of the CLJS compiler. This change would allow the CLJS compiler to contribute useful information to the GClosure compilation process and remove the need to specify known information elsewhere.

Comment by Chas Emerick [ 12/Nov/13 5:11 AM ]

Thanks for the background, Brenton. Assuming you'll be at the Conj, I may have a couple other more minor questions for you.

I think the results from the (still ongoing) community survey will further support the use of the classpath as a reasonable option easily available from the tools people are actually using.

I've dug around the GClosure library docs w.r.t. their build tools, and they seem uniformly directed at people building applications; outside of supporting application use cases, I don't see any discussion of distributing the artifacts of e.g. closurebuilder.py & co. as libraries. A (hardly comprehensive) search of libraries distributed with deps.js files yielded nothing; in fact, the only mentions of deps.js that I can find on github are in .gitignore s and HTML pages of app repos, and in files related to build processes that are reading deps.js shipped with GClosure and its third-party-library.

So, I think we can safely not think about deps.js again, except for the special case of the upstream GClosure libs, which is already taken care of.

One piece of "prior art" of which I was not fully cognizant until yesterday (!!) is this:

https://github.com/emezeske/lein-cljsbuild/issues/143

Essentially, cljsbuild implicitly sets up e.g. /closure-js/libs as a classpath :libs root, same for /closure-js/externs. I've not tested this myself yet, but others apparently have had success. Seems like a very reasonable approach, absent something built-in to the compiler. Externs are certainly out of scope here, but some discussion of what can be done to similarly ease their use is warranted; perhaps adopting a variant of cljsbuild's approach…

Comment by Chas Emerick [ 15/Feb/14 8:31 AM ]

I've started working on this in earnest. It's turning out to be more complicated and delicate than I expected. I see no reason why it won't be resolved successfully as discussed previously, but I thought I'd show my face here so people know that progress is being made.

Comment by Thomas Heller [ 16/Feb/14 3:14 AM ]

I implemented a different approach to this problem in https://github.com/thheller/shadow-build. It has been working well for me so let me explain what I do.

1) The user specifies all source paths
https://github.com/thheller/shadow-build/blob/master/dev/build.clj#L53-55

Each source path is scanned for .cljs and .js files. JS files are just scanned for goog.provide/require statements via regexp, CLJS files assume the NS form is first and is scanned as such to extract the require/provides. No further analysis is done.

2) Out of all this information a dependency graph is built (plus a lookup index, eg. where can I find goog.string).

3) The user specifies his "main" namespaces. A main is simply a namespace that either exports functions for outside use or runs code on its own behalf. Those main namespaces are sorted in topological order so we have the entire graph needed for the compilation. If a namespace is not found required but not provided I do not attempt to build anything but throw an exception.

4) All files are "compiled" in reverse order. Since only CLJS requires compilation, JS files are skipped. CLJS compilation is also a lot "safer" than cljs.closure since dependencies are "guaranteed" to exist (eg. can't compiled a file we don't have all requires for, which may or may not be ideal).

5) Optionally everything may be grouped into modules and either flushed unoptimized and fed to the closure optimizer and then output to disk.

Although I must admit I never used the ```:libs``` directive I'm pretty sure its not needed by using this approach, just put it into a "source path" and let the regexp scan it. As long as it has goog.provide/require its good to go. You can even make it a "main" by just specifying its goog.provide, not required to make a CLJS main.

I've been pretty busy lately and shadow-build is missing some cleanup I want to do but its working very well for my project (> 10k CLJS LOC).

Maybe some of the work I did could help here.

Comment by Chas Emerick [ 17/Feb/14 9:51 AM ]

Thanks, Thomas. I haven't looked at shadow-build much before, but I've stuck a pin in it for further review. The biggest complication of this change is not the primary objective, but my wanting to accomplish it with minimal impact on cljs.closure. There are many things that I would like to change there, but absent an effective set of tests, any simplifying refactor is fairly risky. The first step to accomplishing that would probably be to invert the existing dataflow to produce a (testable) build plan instead of actually building anything. But again, work for another day.

Anyway, I think I have things pretty well settled out locally, though there's some policy decisions to be made. (Putting those in a separate comment.) My biggest hangup was stepping on some helper functions that were being used by the externs stuff in ways I wasn't aware.

Comment by Chas Emerick [ 17/Feb/14 11:34 AM ]

Progress so far on this is available @ https://github.com/cemerick/clojurescript/tree/CLJS-656

Changes on that branch:

  • goog-style JavaScript libraries are no longer resolved via classpath scanning (though it is still used for loading externs and such)
  • I've replaced the system-property-based classpath discovery with one that looks at the actual classloaders in use; this should avoid all sorts of confusing/incorrect behaviour when the compiler is being used in environments where the effective classpath is dynamic.
  • JavaScript dependencies are resolved in one of two ways:
  • resolution through the goog deps.js, the consumption of which remains untouched (though it looks like the classpath-scanning mechanism yields effectively equivalent results as reading deps.js, so we could probably eliminate goog-dependencies entirely in the future…)
  • direct lookup on the classpath based on the "namespace" being :require}}d; e.g. {{(:require foo.bar) will look for a foo/bar.js resource on the classpath, which must contain a corresponding goog.provide("foo.bar") declaration

This all works nicely with the couple of libraries I have GClosure-ified (pprng is the only public instance of that at the moment).

As noted in a comment in the WIP change, a problem with the direct-lookup policy is that goog-style JS dependencies that aren't part of the "official" GClosure library can provide one and only one name, the one corresponding to their position on the classpath. Not a problem from my perspective, but it does cut off the practice of libraries providing type ctors or pools of constants directly, e.g. goog.string.Unicode. This is really just a policy question:

Q1: Should ClojureScript support requiring JavaScript namespaces that don't correspond with the naming/location of their source files?

IMO, the answer to that should be 'yes'; saying 'no' means that we either need to prevent such requires w.r.t. official GClosure library, or that that collection of libraries is effectively a special case from the user's perspective. 'Yes' dumps us back to doing classpath scanning to discover goog-style JS dependencies; not a problem IMO (it would make the solution to this issue effectively equivalent to CLJS-526), but something that David said in the IRC conversation preceding this issue's creation should be eliminated. I think that secondary objective preceded the understanding that GClosure lib doesn't use Java-style file layout itself.

Comment by David Nolen [ 17/Feb/14 11:48 AM ]

Just a heads up that this will likely conflict with CLJS-615, which I would like to land first.

Comment by Chas Emerick [ 17/Feb/14 11:52 AM ]

That's fine, this patch will be small either way and not difficult to rebase.

Comment by Thomas Heller [ 18/Feb/14 3:58 AM ]

IMHO the direct lookup is not very useful. Discovering/scanning all JS files it not that expensive and its quite common in Closure to have many provides per file. Not a big fan of "special cases" either, so I think every JS file should be treated equal regardless of Closure-Library or not.

The real problem is that cljs.closure/build is not a very flexible interface and calling it repeatedly (lein cljsbuild auto) causes problems because all of the sudden cljs.closure has to take care of caching, reloading, etc. IMHO something that should probably be left to tooling and not pollute cljs.analyzer/compiler.

Comment by Chas Emerick [ 18/Feb/14 6:42 AM ]

I agree; I don't see any way (or really, good reason) to avoid the classpath scanning without diverging in important ways from user-dev expectations re: how the goog provide/require system works. Now that CLJS-615 has landed, I'll tweak up my changes to reflect that, and apply cleanly to master in a proper patch.

Comment by Chas Emerick [ 18/Feb/14 9:49 AM ]

Patch attached. It does all that I described above, but does not constrain the location of goog-style JavaScript dependencies within the classpath; the goog.provide declarations in such libraries is the sole constraint on their visibility from user-dev code.

FWIW, the classpath scanning takes ~600ms on my machine the first time, and ~7ms after that (resource lookup from the classpath is pretty heavily cached in the JVM), so I don't think we need to worry about it being a problem in compiler performance.

Comment by Thomas Heller [ 18/Feb/14 10:30 AM ]

FWIW I use reducers to bring that 600ms to around 200ms on my machine, should be possible to make that even faster since right now I (and you I presume) open JAR files ONCE to extract all names and then ONCE per URL. Would probably be faster to open/scan once in total.

But like you said, since its done on init it probably is no problem.

Comment by Chas Emerick [ 19/Feb/14 3:48 AM ]

No, we touch the jars each time. You're right that it could be sped up for the first compile very obviously (I just try to keep my patches as slim as possible, so I generally don't touch what I don't need to). I'll see about it in addressing CLJS-770.

Comment by Chas Emerick [ 24/Feb/14 4:30 AM ]

New patch attached that applies cleanly, now that CLJS-770 / cljs.js-deps is merged.

Comment by David Nolen [ 07/Mar/14 2:39 PM ]

After some thought I still think we should special case Google Closure Library. Everything else must use Java classpath conventions. There's just not enough incentive to support Google Closure idiosyncracies when we have simpler and more sensible conventions in ClojureScript itself. The universe of libraries that follow Google Closure idiosyncrasies is not exactly large, and the ClojureScript ecosystem is evolving at a quick clip. Integration of large non-Closure compatible libraries like React is already straightforward due to :preamble. Let's wrap this one up and move on.

Comment by Chas Emerick [ 12/Mar/14 6:25 AM ]

Update forthcoming shortly.

Comment by Chas Emerick [ 17/Mar/14 11:32 AM ]

New patch attached. This one requires that any non-extern libraries loaded via the classpath contain a goog.provide declaration that corresponds to its relative location on the classpath and to the name used for that library in ClojureScript (e.g. in ns forms).

:libs continues to exist, though is now limited to pulling in libraries from the filesystem, and enforces no constraints on goog.provide declarations or relative position on the filesystem.

Comment by David Nolen [ 10/May/14 2:17 PM ]

Finally got around to trying this one out! It looks like this patch introduces some issues with extracting source from JARs? Have you tried the version of ClojureScript with this patch applied on an existing project?

Comment by Chas Emerick [ 11/May/14 8:10 PM ]

I did use a build containing this patch for a couple of days to hopefully shake out any issues, and the projects in question definitely had dependencies. That said, I'll take a fresh look shortly, maybe I missed something.

Does "some issues" mean that .cljs files aren't resolved from the classpath at all, or something else?

Comment by David Nolen [ 20/May/14 7:49 PM ]

When trying this in an existing project that uses core.async and om I get the following stack trace when trying to build my project:

java.util.zip.ZipException: error in opening zip file
	at java.util.zip.ZipFile.open(Native Method)
	at java.util.zip.ZipFile.<init>(ZipFile.java:220)
	at java.util.zip.ZipFile.<init>(ZipFile.java:150)
	at java.util.zip.ZipFile.<init>(ZipFile.java:164)
	at sun.reflect.GeneratedConstructorAccessor1.newInstance(Unknown Source)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:408)
	at clojure.lang.Reflector.invokeConstructor(Reflector.java:180)
	at cljs.js_deps$jar_entry_names_STAR_.invoke(js_deps.clj:20)
	at clojure.lang.AFn.applyToHelper(AFn.java:161)
	at clojure.lang.AFn.applyTo(AFn.java:151)
	at clojure.core$apply.invoke(core.clj:617)
	at clojure.core$memoize$fn__5049.doInvoke(core.clj:5735)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at cljs.js_deps$find_js_jar.invoke(js_deps.clj:33)
	at cljs.js_deps$find_js_classpath$fn__679.invoke(js_deps.clj:58)
	at clojure.core.protocols$fn__6034.invoke(protocols.clj:143)
	at clojure.core.protocols$fn__6005$G__6000__6014.invoke(protocols.clj:19)
	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
	at clojure.core.protocols$fn__6026.invoke(protocols.clj:54)
	at clojure.core.protocols$fn__5979$G__5974__5992.invoke(protocols.clj:13)
	at clojure.core$reduce.invoke(core.clj:6177)
	at cljs.js_deps$find_js_classpath.invoke(js_deps.clj:51)
	at cljs.js_deps$find_js_resources.invoke(js_deps.clj:70)
	at cljs.closure$load_externs$filter_js__2752$iter__2753__2759$fn__2760.invoke(closure.clj:191)
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:60)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$map$fn__4207.invoke(core.clj:2479)
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:60)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$concat$fn__3923.invoke(core.clj:678)
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:60)
	at clojure.lang.LazySeq.cons(LazySeq.java:103)
	at clojure.lang.LazySeq.cons(LazySeq.java:17)
	at clojure.lang.RT.conj(RT.java:562)
	at clojure.core$conj.invoke(core.clj:83)
	at clojure.core.protocols$fn__6022.invoke(protocols.clj:79)
	at clojure.core.protocols$fn__5979$G__5974__5992.invoke(protocols.clj:13)
	at clojure.core$reduce.invoke(core.clj:6177)
	at clojure.core$into.invoke(core.clj:6230)
	at cljs.closure$load_externs.invoke(closure.clj:204)
	at cljs.closure$optimize.doInvoke(closure.clj:584)
	at clojure.lang.RestFn.applyTo(RestFn.java:139)
	at clojure.core$apply.invoke(core.clj:619)
	at cljs.closure$build.invoke(closure.clj:977)
	at cljs.closure$build.invoke(closure.clj:922)
	at cljsbuild.compiler$compile_cljs$fn__3185.invoke(compiler.clj:58)
	at cljsbuild.compiler$compile_cljs.invoke(compiler.clj:57)
	at cljsbuild.compiler$run_compiler.invoke(compiler.clj:159)
	at user$eval3311$iter__3329__3333$fn__3334$fn__3346.invoke(form-init2476314382473357751.clj:1)
	at user$eval3311$iter__3329__3333$fn__3334.invoke(form-init2476314382473357751.clj:1)
	at clojure.lang.LazySeq.sval(LazySeq.java:42)
	at clojure.lang.LazySeq.seq(LazySeq.java:60)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$dorun.invoke(core.clj:2780)
	at clojure.core$doall.invoke(core.clj:2796)
	at user$eval3311.invoke(form-init2476314382473357751.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6619)
	at clojure.lang.Compiler.eval(Compiler.java:6609)
	at clojure.lang.Compiler.load(Compiler.java:7064)
	at clojure.lang.Compiler.loadFile(Compiler.java:7020)
	at clojure.main$load_script.invoke(main.clj:294)
	at clojure.main$init_opt.invoke(main.clj:299)
	at clojure.main$initialize.invoke(main.clj:327)
	at clojure.main$null_opt.invoke(main.clj:362)
	at clojure.main$main.doInvoke(main.clj:440)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:419)
	at clojure.lang.AFn.applyToHelper(AFn.java:163)
	at clojure.lang.Var.applyTo(Var.java:532)
	at clojure.main.main(main.java:37)
Subprocess failed
Comment by Chas Emerick [ 20/May/14 8:21 PM ]

Thanks for the second ping. Looking at this now.

Comment by David Nolen [ 20/May/14 11:09 PM ]

fixed https://github.com/clojure/clojurescript/commit/883b1b76d93a514d317f7b4d99d263e02af430f7

Comment by Chas Emerick [ 21/May/14 7:56 AM ]

Was just about to reply that the patch works as expected for me, but I now see https://github.com/clojure/clojurescript/commit/058051802e84b1abe3c358260bef1fa992dc963f. Interesting, what non-zip-file files did you have on the classpath? I'm not sure I'd have ever found that.





[CLJS-798] regression: upstream dependencies are no longer honored Created: 16/Apr/14  Updated: 20/May/14  Resolved: 20/May/14

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: File restore-upstream-deps.diff    

 Description   

The latest cljs no longer honors upstream deps. This is a regression introduced by https://github.com/clojure/clojurescript/commit/e16a3da. The js-dependency-index is now calculated once, before the upstream deps are loaded. One potential fix is to regenerate the index after loading the upstream deps, but I'm not familiar enough with the compiler to know if that's a bad idea. I'll attach a patch with that change for review.



 Comments   
Comment by David Nolen [ 17/Apr/14 11:03 AM ]

Out of curiosity are you relying on this functionality and in what way?

Comment by Toby Crawley [ 17/Apr/14 11:48 AM ]

I currently use a deps.cljs in https://github.com/vert-x/mod-lang-clojure - it provides a cljs library that relies on two javascript libraries, and I want users to be able to use my cljs library without having to specify its upstream js deps as configuration to their cljs build. If there is another way to achieve that, I'm open to suggestions, especially given that deps.clj is experimental.

Comment by David Nolen [ 20/May/14 7:38 PM ]

fixed https://github.com/clojure/clojurescript/commit/65f9d988f915cab0bff9c961e85495400ee1e542





[CLJS-792] cljs.core.reducers/reduce does not support cljs.core/PersistentArrayMap Created: 07/Apr/14  Updated: 20/May/14  Resolved: 20/May/14

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

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

Attachments: File CLJS-792.diff    

 Description   
((require '[clojure.core.reducers :as r])
(into [] (r/map identity {}))
;; Clojure: []
;; ClojureScript: Error: No protocol method IReduce.-reduce defined for type cljs.core/PersistentArrayMap: {}


 Comments   
Comment by David Nolen [ 08/May/14 6:20 PM ]

Thanks for the patch, have you submitted a CA? Thanks!

Comment by Jonas Enlund [ 14/May/14 6:03 AM ]

Yes, as the author of the patch I have submitted the CA.

Comment by David Nolen [ 20/May/14 7:36 PM ]

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





[CLJS-807] Emitter cannot emit BigInt or BigDecimal Created: 13/May/14  Updated: 13/May/14

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

Type: Defect Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: numerics
Environment:

r2202


Attachments: Text File cljs-807.patch    
Patch: Code and Test

 Description   

The reader understands BigInt and BigDecimal literals, but the emitter will throw an exception when it finds them.

I know CLJS does not have a proper number tower, but it should at least be able to accept literals like "1N" or "1.5M".

Attached is a patch which will cause the emitter to coerce BigInt and BigDecimal to double-approximations before emitting.






[CLJS-801] str macro emits unoptimizable js code Created: 27/Apr/14  Updated: 11/May/14  Resolved: 11/May/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: string
Environment:

r2202


Attachments: Text File cljs-801.patch     Text File cljs-801-v2.patch     Text File cljs-801-v3.patch     Text File cljs-801-v4.patch    
Patch: Code and Test

 Description   

Clojurescript's str macro emits javascript code which is inefficient and which the closure compiler cannot optimize.

Currently it emits code like {{[cljs.core.str(arg1),cljs.core.str(arg2)].join('')}}. The problems with this:

  1. The emitted function is the arity-dispatch str wrapper instead of the 1-arg implementation of str. The closure compiler cannot eliminate the dispatch.
  2. An intermediate array is always created; the closure compiler cannot optimize it out.
  3. The closure compiler can evaluate constant string expressions (e.g. 'a'+1 to 'a1'), but cannot in this case because it cannot eliminate the str call.

The attached patch rewrites the str macro to generate js code that looks like this:

(str arg1 "constant" \space true nil 123 456.78)

(""+cljs.core.str.cljs$core$IFn$_invoke$arity$1(arg1)+"constant "+true+123+456.78)

This has a number of benefits:

  1. No short-lived array or Array.join operation.
  2. No arity dispatch is invoked. I have also observed that it can (but won't necessarily) inline the function body.
  3. The compiler can perform constant evaluation. For example, in advanced mode the compiler will emit the above example as (""+w.c(a)+"constant true123456.78") where w.c is the munged cljs.core.str.cljs$core$IFn$_invoke$arity$1


 Comments   
Comment by Francis Avila [ 28/Apr/14 12:17 AM ]

Updated patch adds booleans to the str test case, and does not eagerly stringify bools anymore. (It emits as literals and lets the closure compiler decide to stringify at compile time if it wants.)

Comment by David Nolen [ 10/May/14 2:23 PM ]

Need the patch rebased to master.

Comment by Francis Avila [ 11/May/14 1:38 AM ]

Updated patch.

Comment by David Nolen [ 11/May/14 10:52 AM ]

Sorry because of the last commit to master this ticket will not apply, please rebase again. I'll refrain from adding tests until this one gets merged in

Comment by Francis Avila [ 11/May/14 11:22 AM ]

No problem, updated patch.

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

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





[CLJS-804] Binding *print-length* breaks str Created: 07/May/14  Updated: 10/May/14  Resolved: 10/May/14

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

Type: Defect Priority: Major
Reporter: Adam Clements Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

clojurescript 0.0-2202



 Description   

(binding [*print-length* 10] (str {:foo "bar"}))

Breaks with an IMapEntry -key method unimplemented



 Comments   
Comment by David Nolen [ 10/May/14 2:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/337d30cf3a98a0c28f658e230855fc2e09abdeaa





[CLJS-775] cljs.reader parses radix form of int literals (e.g. 2r101) incorrectly Created: 27/Feb/14  Updated: 10/May/14  Resolved: 10/May/14

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

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

Attachments: Text File cljs-775-initial.patch     Text File cljs-775.patch    
Patch: Code and Test

 Description   
ClojureScript:cljs.user> (cljs.reader/read-string "2r10")
"Error evaluating:" (cljs.reader/read-string "2r10") :as "cljs.reader.read_string.call(null,\"2r10\")"
org.mozilla.javascript.JavaScriptException: Error: Invalid number format [2r10] (file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js#107)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:107 (anonymous)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:112 (anonymous)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:374 (read_number)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:650 (read)
	at file:/Users/favila/wcs/clojurescript/.repl/cljs/reader.js:677 (read_string)
	at <cljs repl>:1 (anonymous)
	at <cljs repl>:1


 Comments   
Comment by Francis Avila [ 28/Feb/14 1:42 AM ]

Turns out other integer literals were broken besides radix due to a re-match problem (CLJS-776). Patch includes fix and tests for all the different integer literal forms.

Floats, ratios, and symbols/keywords might also have parsed incorrectly in certain cases, but I did not produce failing tests to confirm.

Comment by David Nolen [ 08/May/14 6:42 PM ]

Can we get a new patch rebased on master? Thanks!

Comment by Francis Avila [ 10/May/14 11:36 AM ]

Rebased patch.

Comment by David Nolen [ 10/May/14 2:10 PM ]

fixed https://github.com/clojure/clojurescript/commit/56ea020fd9b15df220ba247f73b68873f041d8ef





[CLJS-806] support ^:const Created: 09/May/14  Updated: 09/May/14

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

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


 Description   

Currently def's do not support ^:const annotations, this is useful in conjunction with case.






[CLJS-805] add-watch returns map of watch fns instead of watched reference Created: 09/May/14  Updated: 09/May/14  Resolved: 09/May/14

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

Type: Defect Priority: Minor
Reporter: Immo Heikkinen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   
ClojureScript:cljs.user> (def a (atom nil))
#<Atom: nil>
ClojureScript:cljs.user> (add-watch a :foo nil)
{:foo nil}
ClojureScript:cljs.user> (add-watch a :bar nil)
{:bar nil, :foo nil}

In Clojure add-watch returns the watched reference.



 Comments   
Comment by David Nolen [ 09/May/14 8:27 AM ]

fixed https://github.com/clojure/clojurescript/commit/2406ba841db4776cfaa59eb37bec2e8ae543c466





[CLJS-773] Use unchecked-*-int functions for real 32-bit math Created: 26/Feb/14  Updated: 08/May/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: numerics
Environment:

r2173



 Description   

Currently the unchecked-* functions and macros simply alias the primitive js operators. It would be nice if the unchecked-*-int family of functions and macros implemented C/Java-like signed int operations with silent overflows (just like in Clojure) using asm.js coersion idioms. This should also allow us to share such code between clojure and clojurescript without worrying about their different numerics.

A use case is that porting hash algorithms from java to clojurescript is trickier and more verbose than it needs to be.



 Comments   
Comment by David Nolen [ 08/May/14 6:43 PM ]

This sounds interesting, would like to see more thoughts on approach, benchmarks etc.





[CLJS-787] cljs.reader does not read blank string as nil Created: 20/Mar/14  Updated: 08/May/14  Resolved: 08/May/14

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

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

Attachments: File CLJS-787.diff    

 Comments   
Comment by David Nolen [ 20/Apr/14 6:23 PM ]

Several tests fail when I apply this patch.

Comment by David Nolen [ 08/May/14 6:33 PM ]

fixed https://github.com/clojure/clojurescript/commit/279157ac526f7aa0b01b95091821491f574024eb





[CLJS-784] PersistHashMap's -conj implementation recurses infinitely if element to be conjed is not a vector. Created: 15/Mar/14  Updated: 08/May/14  Resolved: 08/May/14

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

Type: Defect Priority: Minor
Reporter: Alex Coventry Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: errormsgs
Environment:

Happens on cljsfiddle, among other environments.


Attachments: Text File 0001-CLJS-784-make-conj-on-maps-behave-as-it-does-in-Cloj.patch     Text File 0002-CLJS-784-Fix-Map.-conj-for-map-entry-seqs-that-don-t.patch     Text File 0003-CLJS-784-use-reduce-in-conj-on-maps.patch    

 Description   

In commit b8681e8 the implementation is

ICollection
  (-conj [coll entry]
    (if (vector? entry)
      (-assoc coll (-nth entry 0) (-nth entry 1))
      (reduce -conj coll entry)))

Thus, e.g., (-conj {} "foo") results in an infinite recursion, and a stack overflow. This causes things like (merge {} "foo") to fail for the same reason.

Not sure what the purpose of the not-vector branch could be. I can't think of a situation where it would give a useful result. Maybe it could throw a more helpful error message.



 Comments   
Comment by Michał Marczyk [ 22/Apr/14 6:13 AM ]

This actually applies to all three map types. (In fact, in (-conj {} "foo"), the map is an array map.) In Clojure, conj on a map works with a number of argument types:

1. map entries;

2. two-element vectors;

3. seqables of map entries.

The final case is, perhaps surprisingly, the oldest one. Merging maps falls under it, since for map arguments it boils down to merge minus special treatment of nil (merge uses conj to merge pairs of maps); but arbitrary seqables of map entries are supported. (NB. these must be actual map entries, not two-element vectors!) This allows one, for example, to filter a map and conj the result of that into another map.

So, we want to support the legitimate use cases while maybe complaining about code that wouldn't work in Clojure if it's not too much of a problem performance-wise. An example of a call that we'd probably like to throw: {{(conj {} (list (list [:foo 1])))}}.

The attached patch makes the -conj implementations in all the map types use an explicit loop in the non-vector branch and adds some test for the resulting behaviour.

Comment by David Nolen [ 05/May/14 5:07 PM ]

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

Comment by Herwig Hochleitner [ 06/May/14 6:11 AM ]

With patch 0001 (3d4405b), map conj fails for seqs, that don't implement -next.

(merge {:a 1} (hash-map :b 2))
;;=> Error: No protocol method INext.-next defined for type cljs.core/NodeSeq: ([:b 2])
...
at cljs.core.PersistentArrayMap.cljs$core$ICollection$_conj$arity$2 (http://localhost:6030/:15569:42)
...
Comment by Herwig Hochleitner [ 06/May/14 6:46 AM ]

I guess implementing INext is not part of the contract for ISeqable.-seq, which means NodeSeq doesn't have to implement it, right?
In that case, the right fix is to use next instead of -next inside of Map.-conj, when dealing with a (possibly user defined) seq of MapEntries.

Attached patch 0002 uses next instead of -next and adds tests for map-entry seqs not implementing INext

Comment by Michał Marczyk [ 06/May/14 3:04 PM ]

Good catch, thanks!

Another approach would be to use reduce, hopefully benefiting from IReduce speed boosts. Of course we'd need to use a custom reduction function wrapping -conj with a vector? check. The attached patch implements this.

Comment by Michał Marczyk [ 06/May/14 3:49 PM ]

Actually, scratch the part about IReduce speed boosts – sorry for the confusion!

Having run better benchmarks with the two patches on a recent build of V8 and I have to say that there doesn't seem to be much of a difference and actually the next-based approach comes out ahead sometimes. In Clojure, a hand-rolled loop-based "map-seq-conj" loses to a hand-rolled reduce-based impl consistently, as far as I can tell, although only by ~3-5%. I've been conj-ing seqs over vectors of vectors, which should be friendly to reduce.

Comment by Herwig Hochleitner [ 08/May/14 4:10 AM ]

Despite no direkt speed boost in benchmarks, I'm fond of using reduce here. GC Pressure is hard to benchmark.

Comment by David Nolen [ 08/May/14 6:18 PM ]

going to go with the next based patch.

Comment by David Nolen [ 08/May/14 6:18 PM ]

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





[CLJS-802] Enable generatePseudoNames compiler option for advanced debugging Created: 30/Apr/14  Updated: 05/May/14  Resolved: 05/May/14

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-802-Add-pseudo-names-compiler-option.patch    

 Description   

The closure compiler option generatePseudoNames [1] can be used to generate readable names in advanced compilation.

a call 

(get {.. ..} :some-kw)

would be emitted like

$cljs$core$get$$.$cljs$core$IFn$_invoke$arity$2$($map__11005__$1$$inline_1045$$, $cljs$core$constant$0keyword$072$$);

this is a great aid when debugging issues in advanced mode, hence a clojurescript compiler option should be added.

[1] http://closure-compiler.googlecode.com/svn/trunk/javadoc/com/google/javascript/jscomp/CompilerOptions.html#generatePseudoNames



 Comments   
Comment by Herwig Hochleitner [ 30/Apr/14 8:01 AM ]

Patch adds :pseudo-names compiler option

Comment by David Nolen [ 05/May/14 5:03 PM ]

fixed https://github.com/clojure/clojurescript/commit/4381c04cce82fc3f8e8bc41e8e0370b8ef0b1065





[CLJS-803] Spurious undeclared Var warning in REPL Created: 05/May/14  Updated: 05/May/14

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

Type: Defect Priority: Major
Reporter: Paul Butcher Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

OS X Mavericks, Safari 7.0.3, Chrome 34.0.1847.131



 Description   

With the following minimal Compojure/ClojureScript project:

https://github.com/paulbutcher/csrepl

Compile with "lein cljsbuild once", then run "lein trampoline cljsbuild repl-listen". In another window, run "lein ring server".

The src/csrepl/core.cljs file defines an atom called app-state. This can be successfully examined in the REPL as follows:

$ lein trampoline cljsbuild repl-listen
Running ClojureScript REPL, listening on port 9000.
To quit, type: :cljs/quit
ClojureScript:cljs.user> csrepl.core/app-state
#<Atom: {:text "hello world"}>

But, if you switch to the csrepl.core namespace and do the same, a spurious "undeclared Var" warning is generated:

ClojureScript:cljs.user> (ns csrepl.core)
nil
ClojureScript:csrepl.core> app-state
WARNING: Use of undeclared Var csrepl.core/app-state at line 1 <cljs repl>
#<Atom: {:text "hello world"}>
ClojureScript:csrepl.core> (:text @app-state)
WARNING: Use of undeclared Var csrepl.core/app-state at line 1 <cljs repl>
"hello world"

Further, once you're in the csrepl.core namespace, the same warning is generated even when using the fully qualified name:

ClojureScript:csrepl.core> csrepl.core/app-state
WARNING: Use of undeclared Var csrepl.core/app-state at line 1 <cljs repl>
#<Atom: {:text "hello world"}>





[CLJS-101] Node fails with :whitespace, works with :simple Created: 17/Nov/11  Updated: 04/May/14  Resolved: 19/Apr/14

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

Type: Defect Priority: Minor
Reporter: Ulrik Sandberg Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

NodeJS 0.6.1
MacOSX Lion



 Description   

Running the example from http://mmcgrana.github.com/2011/09/clojurescript-nodejs.html. Node fails if using :whitespace optimizations, but works when using :simple. The problem seems to be missing object literals.

The source is:

$ cat src/testing-cljs-node/hello.cljs 
(ns testing-cljs-node.hello)

(defn start [& _]
  (println "Hello World!"))

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

Here is the compilation that results in the broken code:

$ cljsc src/testing-cljs-node/hello.cljs '{:optimizations :whitespace :pretty-print true :target :nodejs}' > out/hello.js
$ node out/hello.js

node.js:201
        throw e; // process.nextTick error, or 'error' event on first tick
              ^
TypeError: Cannot set property 'Unicode' of undefined
    at Object.<anonymous> (/Users/ulrik/Source/testing-cljs-node/out/hello.js:487:21)
    at Module._compile (module.js:432:26)
    at Object..js (module.js:450:10)
    at Module.load (module.js:351:31)
    at Function._load (module.js:310:12)
    at Array.0 (module.js:470:10)
    at EventEmitter._tickCallback (node.js:192:40)

When changing the optimizations to :simple, however, it works:

$ cljsc src/testing-cljs-node/hello.cljs '{:optimizations :simple :pretty-print true :target :nodejs}' > out/hello.js
$ node out/hello.js
The "sys" module is now called "util". It should have a similar interface.
Hello World!

The offending line 487 in the :whitespace code:

goog.provide("goog.string");
goog.provide("goog.string.Unicode");
goog.string.Unicode = {NBSP:"\u00a0"};  // line 487

In the working :simple code, the corresponding lines looks like this:

goog.string = {};
goog.string.Unicode = {NBSP:"\u00a0"};  // line 367

If I fix that by adding a line that creates the object, I get another error on line 901:

goog.provide("goog.userAgent.jscript");
goog.require("goog.string");
goog.userAgent.jscript.ASSUME_NO_JSCRIPT = false;  // line 901

In the working code, it looks like this:

goog.userAgent = {};
goog.userAgent.jscript = {};
goog.userAgent.jscript.ASSUME_NO_JSCRIPT = !1;  // line 683

And if I fix that, I get another error on line 975:

goog.provide("goog.debug.Error");
goog.debug.Error = function(opt_msg) {  // line 975

The working code:

goog.debug = {};
goog.debug.Error = function(a) {  // line 730

etc etc



 Comments   
Comment by David Nolen [ 19/Apr/12 11:49 PM ]

I'm not sure how to fix this since the implementation of goog.provide has no meaning in Node.js. As a compromise perhaps we should complain if you try to target node with :whitespace optimizations?

Comment by Ulrik Sandberg [ 20/Apr/12 5:50 AM ]

Sure, that would probably be sufficient to point the user to what the problem really is.

Perhaps it's obvious to everyone else that :whitespace doesn't make sense for Node, but it wasn't to me. But I can now deduce that :simple maybe resolves all provide/require into a single file, whereas :whitespace is an intermediate format that is not applicable for Node.

Comment by David Nolen [ 20/Apr/12 11:29 AM ]

I don't think it's obvious at all and many people have encountered this issue.

Comment by Tom Jack [ 16/Aug/12 2:54 AM ]

Something that seems to work is

#!/usr/bin/nodejs
var _cljsGlobal_ = {};
with (_cljsGlobal_) {
var COMPILED = false;
_cljsGlobal_.goog = {};
goog.global = _cljsGlobal_;
// ... rest of source
}

Does this cause any problems other than forcing e.g. ;module.exports = _cljsGlobal_.mori; in whitespace mode?

Comment by David Nolen [ 16/Aug/12 8:30 AM ]

Using with is probably not an acceptable solution.

Comment by David Nolen [ 28/Sep/12 10:08 PM ]

nclosure actually sounds kind of promising! Do we need to do anything more then recommend that people install nclosure if they want to work :whitespace or :none for the optimization level? If the nclosure approach is simple enough we could perhaps duplicate the strategy?

Comment by Paul deGrandis [ 29/Sep/12 5:27 PM ]

Haha I deleted the comment after I started digging into the code. I'll try to capture my adventure here.

CLJS's node compat layer is built in the opposite direction as nclojure - that is, we provide an externs file for node, and assume interop will happen that way.
nclosure's approach is to hijack the base interactions with goog, and decide to dispatch accordingly to Node or to the Closure lib, but it expects you're writing node-based code. Everything is pushed into shared object off of the node `globals`.

My hypothesis was: I can use the nclosure stuff, but allow ALL dispatching to go to closure (since we have externs file), which very well may call node stuff in the end. The small shim would allow for skirting around the issues described here.

I hacked together the js files and copied the result in to my CLJS-compiled-JS file. All looked good except - the nclosure JS code needs to be inserted into the file before compilation happens. The nclosure stuff redefines core goog interactions, which is why it works. Technically, we could do a preamble hook when we're gathering all the the files together (much like how main-cli-fn is at the tail of all files).

That said, when inserted into the right spot, it prevents the errors.

Comment by David Nolen [ 19/Apr/14 10:14 AM ]

fixed https://github.com/clojure/clojurescript/commit/0c7b31ada01237de33cef77b817ccef3f2b3576d

Comment by Quiz Dr [ 04/May/14 3:43 AM ]

FWIW glad I found this page; I am getting the exact same "type error" error on this line:

goog.string.Unicode = {NBSP:"\u00a0"};

When I use the "whitespace" optimization and run my compiled JS inside Qt 5.2 which now offers Javascript as a native language for building device GUIs (it's pretty neat, and using ClojureScript for this is even neater if I can get it to work).

The type error went away when I switched to "simple" optimization. I am using CS 2202 with cljsbuild 1.0.3.





[CLJS-666] :require-macros should throw a sensible error if no macro file exists Created: 06/Nov/13  Updated: 25/Apr/14

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

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


 Comments   
Comment by Jonas Enlund [ 25/Apr/14 1:29 PM ]

What would you consider to be a sensible error? The resulting stacktrace after a failed (:require [cljs.reader :include-macros true]) looks like this:

Exception in thread "main" clojure.lang.ExceptionInfo: failed compiling file:test/cljs/test_runner.cljs {:file #<File test/cljs/test_runner.cljs>}
at clojure.core$ex_info.invoke(core.clj:4327)
at cljs.compiler$compile_file.invoke(compiler.clj:1006)
at cljs.compiler$compile_root.invoke(compiler.clj:1059)
at cljs.closure$compile_dir.invoke(closure.clj:337)
at cljs.closure$eval2820$fn__2821.invoke(closure.clj:377)
at cljs.closure$eval2757$fn_2758$G2748_2765.invoke(closure.clj:288)
at cljs.closure$eval2807$fn__2808.invoke(closure.clj:391)
at cljs.closure$eval2757$fn_2758$G2748_2765.invoke(closure.clj:288)
at cljs.closure$build.invoke(closure.clj:940)
at cljs.closure$build.invoke(closure.clj:909)
at user$eval2998.invoke(cljsc.clj:21)
at clojure.lang.Compiler.eval(Compiler.java:6619)
at clojure.lang.Compiler.load(Compiler.java:7064)
at clojure.lang.Compiler.loadFile(Compiler.java:7020)
at clojure.main$load_script.invoke(main.clj:294)
at clojure.main$script_opt.invoke(main.clj:356)
at clojure.main$main.doInvoke(main.clj:440)
at clojure.lang.RestFn.invoke(RestFn.java:436)
at clojure.lang.Var.invoke(Var.java:423)
at clojure.lang.AFn.applyToHelper(AFn.java:167)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.main.main(main.java:37)
Caused by: clojure.lang.ExceptionInfo: Could not locate cljs/reader__init.class or cljs/reader.clj on classpath: at line 1 /home/jonas/dev/clojure/clojurescript/test/cljs/foo/ns_shadow_test.cljs {:tag :cljs/analysis-error, :file "/home/jonas/dev/clojure/clojurescript/test/cljs/foo/ns_shadow_test.cljs", :line 1, :column 1}
at clojure.core$ex_info.invoke(core.clj:4327)
at cljs.analyzer$error.invoke(analyzer.clj:267)
at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1415)
at cljs.analyzer$analyze$fn__1517.invoke(analyzer.clj:1506)
at cljs.analyzer$analyze.invoke(analyzer.clj:1499)
at cljs.analyzer$analyze.invoke(analyzer.clj:1494)
at cljs.analyzer$analyze_file$fn__1534.invoke(analyzer.clj:1575)
at cljs.analyzer$analyze_file.invoke(analyzer.clj:1569)
at cljs.analyzer$analyze_deps.invoke(analyzer.clj:963)
at cljs.analyzer$eval1306$fn__1308.invoke(analyzer.clj:1131)
at clojure.lang.MultiFn.invoke(MultiFn.java:241)
at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1417)
at cljs.analyzer$analyze$fn__1517.invoke(analyzer.clj:1506)
at cljs.analyzer$analyze.invoke(analyzer.clj:1499)
at cljs.analyzer$analyze.invoke(analyzer.clj:1494)
at cljs.compiler$compile_file_STAR_.invoke(compiler.clj:885)
at cljs.compiler$compile_file.invoke(compiler.clj:999)
... 20 more
Caused by: java.io.FileNotFoundException: Could not locate cljs/reader__init.class or cljs/reader.clj on classpath:
at clojure.lang.RT.load(RT.java:443)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5018.invoke(core.clj:5530)
at clojure.core$load.doInvoke(core.clj:5529)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5336)
at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
at clojure.core$load_lib.doInvoke(core.clj:5374)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$load_libs.doInvoke(core.clj:5413)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$require.doInvoke(core.clj:5496)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cljs.analyzer$eval1306$fn__1308.invoke(analyzer.clj:1137)
at clojure.lang.MultiFn.invoke(MultiFn.java:241)
at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1417)
... 34 more





[CLJS-789] Advanced compilation broken with latest closure libra Created: 04/Apr/14  Updated: 22/Apr/14  Resolved: 22/Apr/14

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

Type: Defect Priority: Critical
Reporter: Francis Avila Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

cljs >= 2197



 Description   

CLJS releases >= 2197 are depending on the most recent packaged closure lib [org.clojure/google-closure-library "0.0-20140226-71326067"].

This is bad,



 Comments   
Comment by Francis Avila [ 04/Apr/14 3:50 PM ]

I have no idea how this was submitted both too soon and twice! I'm very sorry for the mess. Please mark duplicate of CLJS-790 and close.





[CLJS-793] memoize doesn't correctly cache non-truthy return values Created: 08/Apr/14  Updated: 18/Apr/14  Resolved: 18/Apr/14

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

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

Attachments: Text File CLJS-793.patch     Text File CLJS-793.patch     Text File CLJS-793.patch     Text File patch_commit_2ba4dac94918.patch    
Patch: Code

 Description   

ClojureScript's memoize fn currently uses `(get @mem args)` to check for the existence of a cache entry. This prevents falsey values from being cached correctly.

A direct copy of Clojure's `memoize` code fixes this, patch attached.

This is the first issue+patch I've submitted, so please double check for mistakes - thanks.



 Comments   
Comment by David Nolen [ 08/Apr/14 9:23 AM ]

The patch is not properly formatted - please follow these instructions http://github.com/clojure/clojurescript/wiki/Patches

Comment by Peter Taoussanis [ 08/Apr/14 9:41 AM ]

Updated patch formatting, thanks!

Comment by David Nolen [ 08/Apr/14 10:55 AM ]

Thanks, I've updated the instructions to be clearer on what the commit message should look like. We've been inconsistent in the past and I would like for people to follow a basic guideline. Thanks.

Comment by Peter Taoussanis [ 08/Apr/14 11:01 AM ]

No problem, thanks! Just updated to fit the new spec exactly.

Comment by David Nolen [ 08/Apr/14 7:37 PM ]

Looking at this more closely I would prefer that this be done with a closed over sentinel value via (js-obj), and to perform an identical? check on (get @mem args sentinel) to see if it matches the sentinel.

Comment by Peter Taoussanis [ 09/Apr/14 12:18 AM ]

Sure, updated the patch. Please note that I used the global `lookup-sentinel` instead of closing over a new one. Let me know if there was some reason you wanted a unique sentinel & I'll update!

Cheers

Comment by David Nolen [ 10/Apr/14 12:23 PM ]

Looks good thanks!

Comment by David Nolen [ 18/Apr/14 8:32 AM ]

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





[CLJS-800] Printing PersistentQueueSeq causes StackOverflowError Created: 18/Apr/14  Updated: 18/Apr/14  Resolved: 18/Apr/14

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

Type: Defect Priority: Minor
Reporter: Chad Taylor Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

A cljs.core.PersistentQueueSeq is generated correctly, but printing it causes a java.lang.StackOverflowError. This can be verified in the REPL:

(rest (conj cljs.core.PersistentQueue.EMPTY 1 2 3))  ;; prints stack trace

(map #(* % %) (rest (conj cljs.core.PersistentQueue.EMPTY 1 2 3))) ;; => (4 9)

Cause: PersistentQueueSeq does not implement IPrintWithWriter protocol

Solution: Provide an implementation of IPrintWithWriter for PersistentQueueSeq

Patch: CLJS-800.patch



 Comments   
Comment by Chad Taylor [ 18/Apr/14 1:00 AM ]

Added patch with test and code.

Comment by David Nolen [ 18/Apr/14 8:30 AM ]

fixed https://github.com/clojure/clojurescript/commit/2907190e5414fd53a0e0a07424f342360eb31ed9





[CLJS-783] Confusing error messages when ns compilation fails due to a missing dependency Created: 11/Mar/14  Updated: 17/Apr/14  Resolved: 17/Apr/14

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

Type: Defect Priority: Minor
Reporter: Michael Klishin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: error-reporting, errormsgs, usability


 Description   

I have a namespace proj.a which requires proj.b. proj.b, in turn, relies on core.async. I did not have
core.async listed in project.clj by accident, and the resulting error message was

goog.require could not find: proj.b.

This is not incredibly helpful. I've wasted over an hour trying to understand why one ns in my project
cannot reference another one.

Expected outcome: compilation must fail instead of swallowing exceptions. If it matters, I use lein-cljsbuild 1.0.2.



 Comments   
Comment by David Nolen [ 12/Mar/14 8:36 AM ]

And which version of ClojureScript are you using?

Comment by Michael Klishin [ 12/Mar/14 8:52 AM ]

0.0-2138

Comment by Michael Klishin [ 12/Mar/14 8:53 AM ]

I strongly disagree with the severity change. Anything that can waste beginners hours of time is not a minor priority.

Comment by David Nolen [ 12/Mar/14 10:18 AM ]

That is a fairly old release of ClojureScript, can you replicate the issue with 0.0-2173? When you change your dependency please make sure to run "lein cljsbuild clean" first.

Comment by David Nolen [ 17/Apr/14 3:53 PM ]

Closing unless I hear step on how to reproduce this in more recent ClojureScript releases. Feel free to request a re-open if you can demonstrate that this isn't resolved.





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

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.






[CLJS-799] Having a namespace end with ".cljs" produces wrong source map Created: 16/Apr/14  Updated: 16/Apr/14

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

Type: Defect Priority: Minor
Reporter: Sven Richter Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: maps, namespace, source
Environment:

Windows 7
JDK 1.7
CLJS version: 0.0-2138 and 0.0-2156 (probably hits other versions too, but I only tested these two)



 Description   

When an clojurescript namespaces ends with ".cljs" I cannot see the source file in google chrome.
Repro steps:

1. Create a new luminus project with: lein new luminus cljsbug +cljs +http-kit
2. Change the project.clj cljsbuild -> compiler setting to:
:compiler
{:output-to "resources/public/js/site.js"
:output-dir "resources/public/js/out"
:optimizations :none
:source-map true
}
3. Change cljsexample.html file to:
<script type="text/javascript" src="js/out/goog/base.js"></script>
<script type="text/javascript" src="servlet-context/js/site.js"></script>
<script type="text/javascript">goog.require("cljsbug.main");</script>

4. Now start the server with "lein run -dev" and "lein cljsbuild auto"
5. Open localhost:3000/cljsexample
6. Check for the source file in google chrome

It should be there now and correct.
Now to reproduce the problem do this:

7. Change the namespace of the main.cljs file to: ns cljsbug.main.cljs
8. Change the cljsexample.html goog.require line to: <script type="text/javascript">goog.require("cljsbug.main.cljs");</script>

9. Restart the cljsbuild with: lein do cljsbuild clean, cljsbuild auto
10. Reload the /cljsexample page in google chrome and the source mapping wont be there anymore.



 Comments   
Comment by Sven Richter [ 16/Apr/14 2:38 PM ]

Just to clear things up. Steps 1 to 6 are not needed to reproduce the problem. It is sufficient to go through steps 7 to 10 on any project that uses a similar cljsbuild setting.
It is important that optimizations are set to :none.

Short repro version.

1. Have cljs project with the following settings:
:compiler
{:output-to "resources/public/js/site.js"
:output-dir "resources/public/js/out"
:optimizations :none
:source-map true
}

2. Change any cljs file namespace and add ".cljs" to the namespace.
3. Have the new namespace required in the html file by google like this: goog.require("cljsbug.main.cljs")

4. Open a page in the browser and see that the source maps are missing.





[CLJS-795] Enhance multimethod performance Created: 11/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Jonas Enlund Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File CLJS-795.diff    

 Description   

Multimethods can be made more performant by implementing (the long list of) IFn methods. There is one benchmark (the last one) covering multimethods in script/benchmark and I saw a drop from ~250msecs to ~25msecs with the suggested changes.

If it seems to be too much repetition I'm sure its possible to throw some macros at the implementation to make most of the boilerplate disappear.



 Comments   
Comment by David Nolen [ 14/Apr/14 4:29 PM ]

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





[CLJS-796] (get [42] nil) => 42 Created: 11/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug
Environment:

r2156



 Comments   
Comment by Francis Avila [ 12/Apr/14 9:23 PM ]

This is a duplicate of CLJS-728 and is fixed by this commit, which is included in r2197 and above.





[CLJS-794] RegExp flags are being dropped by `string/replace` Created: 09/Apr/14  Updated: 09/Apr/14

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

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


 Description   

`clojure.string/replace` accepts either a string or pattern argument to match against.

For pattern arguments, the current implementation discards the original RegExp and creates a new one:
`(.replace s (js/RegExp. (.-source match) "g") replacement)`

This is killing any flags on the original pattern (case insensitivity, for example). As a result, things like `(str/replace "Foo" #"(?i)foo" "bar")` currently fail. The result is "Foo", it should be "bar".

Can I submit a patch that'll check for and preserve other (i/m/y) flags?

Thanks






[CLJS-791] Cross-target ClojureScript to the DartVM. Created: 06/Apr/14  Updated: 08/Apr/14  Resolved: 08/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Yesudeep Mangalapilly Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: cross-target, dart, enhancement, vm
Environment:

Any.



 Description   

ClojureScript is a better language than many others, I figure.
The DartVM is picking up speed. Can CLJS be retargetted to generate
Dart code as well?



 Comments   
Comment by David Nolen [ 08/Apr/14 9:22 AM ]

Queries like this are best directed toward the mailing list before opening tickets. Thanks.





[CLJS-788] spurious namespace warnings about goog namespaces in 2197 Created: 31/Mar/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

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


 Comments   
Comment by David Nolen [ 01/Apr/14 1:35 PM ]

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





[CLJS-24] Windows commandline scripts are missing Created: 21/Jul/11  Updated: 20/Mar/14  Resolved: 30/Jul/11

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

Type: Enhancement Priority: Minor
Reporter: Marko Kocić Assignee: Brenton Ashworth
Resolution: Completed Votes: 0
Labels: None
Environment:

Any Windows version


Attachments: Text File 0001-Add-commandline-scripts-for-windows.patch     File bootstrap.bat     File cljsc.bat     File repl.bat     File repljs.bat    

 Description   

Right now, all scripts in bin and scripts folders are sh scripts, and those not work under Windows.



 Comments   
Comment by Benny Tsai [ 23/Jul/11 2:48 AM ]

Hi all,

I put together the following Windows batch files and tested them on my machine. They allowed me to do almost everything in the Quick Start guide (didn't test the Node.js-related items, as I don't have Node.js installed). I've attached the batch files for your consideration.

bin\cljsc.bat
script\bootstrap.bat
script\repl.bat
script\repljs.bat

Some things of note:

  • The original bootstrap script used curl for downloading and unzip for unzipping. bootstrap.bat uses jar -xf for unzipping, but currently expects wget.exe in clojurescript\bin for downloading. I'm open to suggestions!
  • When calling cljsc.bat with an option map, the map needs to be enclosed in " instead of ' (as currently shown in the Quick Start guide).
  • Spaces in the path are bad. I think the problem lies with calls to (io/resource) in closure.clj and compiler.clj, which encodes paths like "C:\Documents and Settings\Benny\clojurescript\..." to "C:\Documents%20and%Settings\Benny\clojurescript\...", which can't be resolved. For now, I just put everything in C:\clojurescript\ to work around the issue.
Comment by Marko Kocić [ 25/Jul/11 7:39 AM ]

The following patch adds cljsc.bat, repl.bat and repljs.bat files.
Those files correctly infer CLOJURESCRIPT_HOME using script file name as a base.

Also, they use java 1.6 classpath feature that you don't have to list individual jar files anymore when setting up classpath.

Comment by Brenton Ashworth [ 30/Jul/11 9:47 AM ]

Benny and Marko,

Thank you for taking the time to report this issue and submit patches. We welcome patches from the community but you must follow the process outlined here:

http://clojure.org/contributing

We can only accept patches from those whose names appear in the list at the bottom of that page. It's simple to get on that list, you only have to sign a CA. After you have signed a CA, please send patches instead of plain files.

I have added batch files for cljsc, repl and repljs.

Creating a bootstrap batch file is a problem. I don't know that it can be done without depending on external programs. Since this only needs to be done once and the things which it does can be done manually, we may want to supply a wiki page for Windows users which describes how to get set up without using script/bootstrap.

Comment by Benny Tsai [ 30/Jul/11 10:25 AM ]

Hi Brenton,

First, thank you for working on this issue.

I'll sign and send the CA pronto. Mail delivery seems quite slow between Canada and the states, so it might take a while before it gets there. Ah well.

W.r.t. bootstrap external dependencies: Perhaps we could take a page from lein's approach, and create a Windows package that includes the required program(s)? AFAICT, the only program that would need to be included is wget.exe or something equivalent, since the unzipping can be done via "jar -xf".

Comment by Benny Tsai [ 30/Jul/11 10:41 AM ]

Also, I noticed that in the latest .bat files, CLASSPATH is being modified and overwritten. It might be better to create and use CLJSC_CP like the original sh scripts.

Comment by Tim Visher [ 22/Nov/13 10:23 AM ]

I just found this issue because I was trying to work on a FS bug.

For very simple things like the repl script and such I think it makes some sense to maintain separate sh and bat files, but for the bootstrap script, why not move to something like Ant?

All it's doing is fetching some files, unzipping them, jarring them up potentially, and cleaning up after itself, which Ant is perfectly capable of doing in a cross-platfrom way. Ant apparently even has svn support at this point.

See:

Get Task: http://ant.apache.org/manual/Tasks/get.html
Unzip Task: http://ant.apache.org/manual/Tasks/get.html
Jar Task: http://ant.apache.org/manual/Tasks/jar.html
svnant extension: http://subclipse.tigris.org/svnant/svn.html
Copy task: http://ant.apache.org/manual/Tasks/copy.html
Delete Task: http://ant.apache.org/manual/Tasks/delete.html

How would we feel about porting this to ant so we get cross-platform support for 'free' from one source.

I'm mainly asking this as a way to gauge interest, I think implementation would be trivial.

Comment by Phongphan Phuttha [ 20/Mar/14 11:10 AM ]

Sorry for adding more comments on completed ticket.

But for bootstrap.bat, it could be done without installing any third party tools by just using Windows Script Host.

I have created a proof of concept by forking current master and add bootstrap.bat with download and extracting tools written in VBScript here.





[CLJS-622] defining zero-arity protocol method produces incomprehensible error message Created: 16/Oct/13  Updated: 19/Mar/14

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

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

Clojurescript 0.0-1913


Attachments: File cljs-622-20131104.diff     File protocol-warning-20131017-2.diff     File protocol-warning-20131017.diff     File protocol-warning-20131018.diff    

 Description   

In Clojure:
> (defprotocol P (f []))
IllegalArgumentException Protocol fn: f must take at least on arg [...]

In ClojureScript
> (defprotocol P (f []))
[ 100 lines of incomprehensible stack traces ]

Would be nice if it gave a better error message.



 Comments   
Comment by Travis Thieman [ 17/Oct/13 11:02 AM ]

Patch: 20131017

Moves the exception to the defprotocol macro itself and throws an exception matching Clojure's.

Comment by David Nolen [ 17/Oct/13 2:30 PM ]

It's preferable to throw ex-info, look around the compiler for an example. Also this error should include file, line and column information - again look around for ex-info examples to see what I mean. Thanks!

Comment by Travis Thieman [ 17/Oct/13 3:59 PM ]

v2 of the 20131017 patch uses ex-info to throw a more descriptive error message. To support this, ana/cljs-file is now bound in closure.clj/compile-file.

Comment by David Nolen [ 17/Oct/13 4:13 PM ]

The `binding` to setup `cljs.analyzer/cljs-file` is not necessary. If it is necessary you will need to show why it is. `cljs.compiler/compile-file` calls `cljs.compiler/compile-file*` which already does this.

Comment by Travis Thieman [ 17/Oct/13 4:26 PM ]

I encountered this as an issue because of the specific way I was testing compilation. I create a test file and compiled it using "/bin/cljsc test.cljs". This compiles from source files.

However, when it calls compile-file, no output file is specified, instead writing to stdout, e.g. opts contains {:output-to :print} and does not contain :output-file. As a result, the second branch of the if gets called, which compiles a sequence of forms instead of a file. As a result, the cljs-file never gets bound, even though there are input files.

The binding should still be moved to only affect the compile-form-seq branch.

(defn compile-file
  "Compile a single cljs file. If no output-file is specified, returns
  a string of compiled JavaScript. With an output-file option, the
  compiled JavaScript will written to this location and the function
  returns a JavaScriptFile. In either case the return value satisfies
  IJavaScript."
  [^File file {:keys [output-file] :as opts}]
  (binding [ana/*cljs-file* (.getPath ^java.io.File file)]
    (if output-file
      (let [out-file (io/file (output-directory opts) output-file)]
        (compiled-file (comp/compile-file file out-file opts)))
      (compile-form-seq (ana/forms-seq file)))))
Comment by Travis Thieman [ 18/Oct/13 10:05 AM ]

Patch: 20131018

Kept the ex-info throw, but removed the additional fix to closure.clj. I'll make a separate issue for the compilation problem.

Comment by David Nolen [ 18/Oct/13 10:23 AM ]

Upon further reflection, it's undesirable for this to throw. We should just convert it into a warning. I need to refactor warnings a bit so patch for this ticket should hold off until I get around to that.

Comment by Travis Thieman [ 18/Oct/13 10:42 AM ]

If the exception is not thrown here, a more generic exception is still thrown further down the line after the defprotocol macroexpansion, something about an invalid dot form. Is the plan to also fix that so that (defprotocol P (f [])) would fully compile? If not, what are the reasons for not wanting to throw here?

Comment by David Nolen [ 28/Oct/13 8:51 AM ]

CLJS-636 must be addressed first.

Comment by David Nolen [ 04/Nov/13 8:22 AM ]

CLJS-636 is resolved can we get an updated patch?

Comment by Travis Thieman [ 04/Nov/13 9:21 AM ]

Patch: 20131104

Add warning type :illegal-protocol-method and throw it on zero-arity protocol methods.

Comment by Chas Emerick [ 19/Mar/14 9:41 AM ]

This patch no longer applies, due to application of the patch from CLJS-627. Can you attach an updated patch?





[CLJS-786] cljs.core/into doesn't preserve metadata Created: 17/Mar/14  Updated: 19/Mar/14

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

Type: Defect Priority: Minor
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

cljs.core/into doesn't preserve metadata:

(meta (into ^{:foo :bar} [] [1 2 3])) ;; => nil


 Comments   
Comment by Chas Emerick [ 19/Mar/14 9:15 AM ]

Confirmed. Both TransientVector and TransientHashMap need to have meta fields added that are passed on when instances are made persistent!.

Comment by Kevin Marolt [ 19/Mar/14 9:33 AM ]

Not sure what's the preferred way to handle this. The transients in Clojure don't support metadata either. Instead, Clojure simply uses

(defn into
  "Returns a new coll consisting of to-coll with all of the items of
   from-coll conjoined."
  {:added "1.0"
   :static true}
  [to from]
  (if (instance? clojure.lang.IEditableCollection to)
    (with-meta (persistent! (reduce conj! (transient to) from)) (meta to))
    (reduce conj to from)))

See also [#CLJ-916] into loses metadata.





[CLJS-523] IHash not extended to js/Date Created: 17/Jun/13  Updated: 19/Mar/14  Resolved: 19/Mar/14

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

Type: Defect Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Makes for fun times, like:

cljs.user=> ((hash-map #inst "1990-01-29T00:39:33.938-00:00" 5) #inst "1990-01-29T00:39:33.938-00:00")
nil

Happy to put together a patch implementing a sane IHash impl for js/Date. Have not done so already only because js/Date does have a place in core.cljs where core protocols are being extended to host types, but IHash just isn't included; perhaps there was a reason?



 Comments   
Comment by David Nolen [ 17/Jun/13 11:10 AM ]

This implies modifying the prototype of a native in core, I'm not very excited about that. I'm inclined to say that users should deal with js/Date themselves.

Comment by Fogus [ 17/Jun/13 12:52 PM ]

> users should deal with js/Date themselves

Seconded.

Comment by Chas Emerick [ 17/Jun/13 1:41 PM ]

Didn't that ship sail already with IEquiv?

Comment by David Nolen [ 17/Jun/13 1:47 PM ]

No and we will probably remove that. Our modification of the String prototype has already wrecked havoc as far as interop goes. There is simply no justification for modifying natives in core.

Comment by David Nolen [ 17/Jun/13 2:00 PM ]

Actually we should just add Date to the list of natives that are specially handled along with object, boolean, string etc.

Comment by Chas Emerick [ 17/Jun/13 2:00 PM ]

Opened CLJS-525 per dnolen's suggestion in irc, to allow things like this to be supported without touching built-in prototypes.

Comment by Chas Emerick [ 19/Mar/14 9:32 AM ]

wontfix, closing. CLJS-525





[CLJS-485] clojure.string/replace ignores regex flags Created: 12/Mar/13  Updated: 19/Mar/14

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

Type: Defect Priority: Minor
Reporter: Esa Virtanen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, patch, test

Attachments: Text File 0001-Take-regex-flags-m-i-into-account-in-clojure.string-.patch    
Patch: Code and Test

 Description   

The replace function in namespace clojure.string ignores regex flag provided in the match pattern. For example:

CLJS
clojure.string/replace "I am NOT matched" #"(?i)not " "")
=> "I am NOT matched"
CLJ
clojure.string/replace "I am NOT matched" #"(?i)not " "")
=> "I am matched"

The attached patch fixes this by parsing the m and i flags, if set, from the match object, instead of explicitly setting only "g".



 Comments   
Comment by Chas Emerick [ 19/Mar/14 9:29 AM ]

I can confirm the bug. The attached patch applies cleanly, and works as expected.

Esa, sorry for the long delay (this one must have slipped through the cracks)! Could you please submit a contributor's agreement, so that your patch can be merged? More info is here:

http://clojure.org/contributing





[CLJS-145] Cannot create more than one browser evaluation environment Created: 06/Feb/12  Updated: 19/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

When testing multi-user applications, it may be desirable to run multiple browser-connected REPLs from the same Clojure process. Currently, the socket connection for the browser-connected REPL is stored in a global atom, so it is not possible to create multiple browser-connected REPLs on different ports.



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

More powerful browser REPLs may be better handled now by external tools.

Comment by Chas Emerick [ 19/Mar/14 9:22 AM ]

If you haven't already, check out Austin, which does exactly this, and other helpful things.





[CLJS-785] :refer-macros in conjunction with :refer not working Created: 17/Mar/14  Updated: 17/Mar/14

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

Type: Defect Priority: Minor
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

The :refer-macros directive isn't working when used in conjunction with the :refer directive. Compiling a ns-form like

(ns foo
  (:require
    [bar :refer [baz] :refer-macros [quux]]))

produces the compiler error

Each of :as and :refer options may only be specified once in :require / :require-macros; offending spec: (bar :refer [baz] :refer [quux])

The problem seems to be with analzyer.cljs/desugar-ns-specs. Invoking

(desugar-ns-specs '((:require [bar :refer [baz] :refer-macros [quux]])))

returns

'((:require-macros (bar :refer [baz] :refer [quux])) (:require (bar :refer [baz])))

instead of

'((:require-macros (bar :refer [quux])) (:require (bar :refer [baz])))

Furthermore, there seems to be a typo in the local remove-sugar function on line 1094 [1]: It should probably be

(let [[l r] (split-with ...)] ...) ;; no '&' before 'r'

instead of

(let [[l & r] (split-with ...)] ...)

Otherwise, something like

(desugar-ns-specs '((:require [bar :include-macros true :as b])))

becomes

((:require-macros (bar :as b)) (:require (bar)))

instead of

((:require-macros (bar :as b)) (:require (bar :as b)))

[1] https://github.com/clojure/clojurescript/blob/78d20eebbbad17d476fdce04f2afd7489a507df7/src/clj/cljs/analyzer.clj#L1094






[CLJS-664] The array? predicate appears to be incomplete Created: 05/Nov/13  Updated: 12/Mar/14  Resolved: 12/Mar/14

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

Type: Defect Priority: Minor
Reporter: Rasmus Buchmann Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Probably all?


Attachments: Text File cljs_644_v3.patch     Text File cljs_644_v4.patch     Text File cljs_664.patch     Text File cljs_664.patch     Text File isArray.patch    

 Description   

The following command with fs the filesystem module from node still returns a js array:
js->clj (.readdirSync fs ".")))
A short irc discussion with dnolen revealed that the array? predicate is probably to blame. instanceof Array returns false for the result, while result.constructor == Array.



 Comments   
Comment by David Nolen [ 05/Nov/13 7:19 PM ]

This is likely because we're getting an Array from a different context. Node.js has Array.isArray and we could modify array? to emit this if the compile target is :nodejs.

Comment by Travis Vachon [ 09/Dec/13 1:44 PM ]

It looks like Array.isArray is available in some browsers as well, would it make sense to change the implementation to use that when possible and then fall back to goog.isArray?

Comment by Travis Vachon [ 09/Dec/13 1:46 PM ]

also, I found this, which seems helpful/relevant:

http://web.mit.edu/jwalden/www/isArray.html

this would probably mean that some behavior in older browsers would be different from newer browsers, but that seems unavoidable given the state of array detection in environments without Array.isArray...

Comment by Travis Vachon [ 09/Dec/13 4:52 PM ]

Use Array.isArray when building for node.js

This seemed to be the easiest way to do this, but I'm open to other ideas.

Comment by Travis Vachon [ 10/Dec/13 9:49 AM ]

added {{git am}}able patch

Comment by David Nolen [ 20/Jan/14 2:00 PM ]

This patch need to be rebased to master.

Comment by Travis Vachon [ 05/Mar/14 5:57 PM ]

sure! uploaded a rebased patch

Comment by Travis Vachon [ 05/Mar/14 6:10 PM ]

oh actually, I just noticed this uses an older style of function access that causes warnings now, will update

Comment by Travis Vachon [ 05/Mar/14 6:17 PM ]

uploaded patch with same functionality, less warnings

Comment by David Nolen [ 12/Mar/14 8:34 AM ]

Why do we need to mutate compiler-env can't you just add the target purely on the next line? Otherwise patch looks OK.

Comment by Travis Vachon [ 12/Mar/14 2:06 PM ]

Hm I don't think we can do it purely on that next line because we're still handing around the atom, but I do think I can just make it part of the mutation we're already doing below. Does that sound right?

Comment by David Nolen [ 12/Mar/14 2:14 PM ]

Actually I do see what you mean. Yes let's do the swap! inside.

Comment by Travis Vachon [ 12/Mar/14 2:26 PM ]

cool! added an updated patch

Comment by David Nolen [ 12/Mar/14 2:51 PM ]

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





[CLJS-242] Copy over mapv & filterv from clojure.core Created: 07/May/12  Updated: 12/Mar/14  Resolved: 12/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-242-copy-over-mapv-filterv-from-clojure.core.patch    
Patch: Code

 Comments   
Comment by Michał Marczyk [ 07/May/12 9:10 AM ]

Same patch w/ ticket number and an extra line in the commit message.

Comment by David Nolen [ 07/May/12 11:39 AM ]

fixed, https://github.com/clojure/clojurescript/commit/bf2c682398101bdc93d2f97cdad410cb214987f6





[CLJS-335] user defined tagged literals in CLJS Created: 06/Jul/12  Updated: 12/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: kovas boguta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I'm trying to make my own tagged literals for cljs, and its a rough scene. I've probably spend 15 hours trying to get this to work properly.

The quick hack of binding cljs-data-readers is a pretty bad solution, and doesn't work with cljsbuild.

One solution is to just copy the mechanism of data_readers.clj, instead calling it cljs_data_readers.clj. That would help.

A better solution is to just pass through all the tagged literals, and resolve them on the cljs side, eg `((cljs.core/get (cljs.core/deref cljs.reader/tag-table) ~tag) ~data))

That way the definition of the tags doesn't have to be repeated twice, as is done now (once in cljs.reader, and once in the clj-side cljs.tagged-literals) .

The problem with this is that clj itself is incapable of passing through undefined tags without exploding, which is also a near showstopper. Having something like cljs_data_readers.clj would get the job done in the meantime.

(another minor issue is that the cljs reader doesn't handle namespaced symbols, so in #a/b, b is what is used as the key into cljs-data-readers)



 Comments   
Comment by kovas boguta [ 06/Jul/12 12:52 AM ]

I've never used this before, so just assume everything in bold has earmuffs around it.

Comment by David Nolen [ 30/Aug/12 7:38 PM ]

This seems like a lot of separate issues rolled into one. Can we break this ticket apart into simpler actionable tickets?

Comment by kovas boguta [ 15/Dec/12 10:35 PM ]

Now that we have default-data-reader-fn, its possible to have a clean fix. Just set default-data-reader-fn to the function described above.

Should I generate a patch?

Comment by David Nolen [ 21/Dec/12 4:58 PM ]

What function defined above?

Comment by kovas boguta [ 25/Dec/12 1:30 PM ]

the function is
(fn [tag data] `((cljs.core/get (cljs.core/deref cljs.reader/tag-table) ~tag) ~data))

The proposal is to bind default-data-reader-fn to the above, in compiler.clj for the compile-file* function. And get rid of the binding for data-readers in that function as well.

It should be possible to get rid of the tagged_literals.clj file as well, since those definitions are no longer needed after this change.

Comment by David Nolen [ 12/Mar/14 8:31 AM ]

Is this still an issue today?





[CLJS-150] Regular expressions don't support Javascript mode flags Created: 16/Feb/12  Updated: 12/Mar/14  Due: 24/Feb/12

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

Type: Defect Priority: Minor
Reporter: Bobby Calderwood Assignee: Bobby Calderwood
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently, the compiler and cljs.core allow for Java mode flags. Javascript doesn't support many of these, and supports one flag not supported by Java - 'g'.

ClojureScript regular expressions should support only Javascript regex mode flags: 'i', 'm', and 'g'. This applies to Regex literals in the compiler as well as (re-pattern).

This is a defect in the implementation of CLJS-116.



 Comments   
Comment by David Nolen [ 16/Feb/12 3:33 PM ]

The defect existed prior to CLJS-116. The problem is that we're using the Clojure reader and g is not a valid flag for a Java RegexPattern.

Comment by Francis Avila [ 28/Feb/14 1:04 AM ]

This ticket should be rejected. A regular expression created with the global flag is stateful (i.e., the lastIndex property is checked and used by the exec and test methods.) On sufficiently old browsers (pre js 1.5), this makes the RegExp object itself stateful, i.e., not instances, but the RegExp constructor is mutated!

Using a regex with the global flag set will already ruin the results of re-seq, re-find, etc. I could see re-seq using a clone of the input regex with the global flag set as an optimization to avoid string slicing, but we certainly shouldn't provide a public interface to create them.

See also CLJS-776





[CLJS-344] clojure.reflect is asynchronous (should use CrossPageChannel) Created: 25/Jul/12  Updated: 12/Mar/14

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

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


 Description   

This makes printing at the REPL a bit annoying. We should be using the CrossPageChannel and not a ajax request to the built in webserver.



 Comments   
Comment by Brandon Bloom [ 03/Sep/12 3:36 PM ]

Was this ticket supposed to be filed against Clojure, not ClojureScript?

Comment by David Nolen [ 03/Sep/12 3:51 PM ]

No. This ticket refers to the HackerSchool work to provide a reflection hook for ClojureScript to make the REPL interactions more friendly. Look at clojure.reflect in the ClojureScript repo.

Comment by Frank Siebenlist [ 08/Oct/12 4:55 PM ]

As we've changed the topic of this issue a little bit..., I'd like to add the additional requirement that the reflection facility should be pluggable, meaning that if I have additional help/reflection functions that I want to make available to the cljs-repl users, then I should not have to wait for acceptance in a new clojurescript version.

Maybe some form of registration API thru which I can add new cljs-functions that communicate with their clj-counterparts thru some naming convention . Not sure about the details here...

Comment by David Nolen [ 08/Oct/12 5:06 PM ]

That's really a separate issue that probably warrants it's own discussion.

Comment by Frank Siebenlist [ 08/Oct/12 5:24 PM ]

Well... you started to add the (seemingly unrelated) CrossPageChannel requirement to the asynchronous one

But seriously, the pluggability requirement is probably important to keep in mind when any implementation for asynchronous clojure/reflect is designed and implemented - I'll open-up another issue for pluggability to keep it clean.





[CLJS-345] clojure.reflect support for Rhino REPL Created: 25/Jul/12  Updated: 12/Mar/14

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

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


 Description   

clojure.reflect should work for the Rhino REPL






[CLJS-433] Throw error when user tries to use wrong syntax for multiple arity functions in extend-type Created: 29/Nov/12  Updated: 12/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Right now it allows to mix declarations, sometimes allowing an illegal syntax (-nth shouldn't be allowed in the following example)

example from jayq:

(extend-type js/jQuery

  IIndexed
  (-nth [this n] ...)
  (-nth [this n not-found] ...)

  ILookup
  (-lookup
    ([this k] ...)
    ([this k not-found] ...))

It already throws an error with some protocols such as IFn, but not for all of them.






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

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

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





[CLJS-716] Lookup for Date keys does not work in PersistentMaps and PersistentSets Created: 06/Dec/13  Updated: 12/Mar/14

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

Type: Defect Priority: Minor
Reporter: Sunil Gunisetty Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug
Environment:

Clojurescript version "0.0-2080"
Browser : Firefox 25.0.1
: Chrome 31.0.1650.63



 Description   

Lookup of js/Date objects fails, if there are more than 8 elements in the map. Works correctly if the map contains 8 elements or less.

Examples :

1. Map with more than 8 elements

cljs.user> (def test-map
{
:a 1
:b 2
#inst "2013-12-19T05:00:00.000-00:00" 3
:d 4
:e 5
#inst "2013-12-06T05:00:00.000-00:00" 6
:g 7
:h 8
:i 9
:j 10
})

cljs.user> (test-map #inst "2013-12-19T05:00:00.000-00:00")
nil

2. Map with 8 elements

cljs.user> (def test-map
{
:a 1
:b 2
#inst "2013-12-19T05:00:00.000-00:00" 3
:d 4
:e 5
#inst "2013-12-06T05:00:00.000-00:00" 6
:g 7
:h 8
})

cljs.user> (test-map #inst "2013-12-19T05:00:00.000-00:00")
3



 Comments   
Comment by David Nolen [ 06/Dec/13 5:07 PM ]

This is because JS Dates don't hash consistently, http://dev.clojure.org/jira/browse/CLJS-523





[CLJS-774] cljs.reader should js js/parseInt with radix specified Created: 27/Feb/14  Updated: 12/Mar/14  Resolved: 12/Mar/14

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

Type: Defect Priority: Major
Reporter: Tatu Tarvainen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-774.patch    

 Description   

In cljs.reader, the parse-int calls js/parseInt without a radix.

The radix is implementation defined if not specified.
This causes problems for example in older Android browsers which parse "08" as 0 instead of 8.

This makes reading timestamps fail. For example:
Trying to read: #inst "2013-07-08T21:00:00.000-00:00"
causes:
Error: timestamp day field must be in range 1..last day in month Failed: 1<=0<=31

in js console:
parseInt("08") => 0
parseInt("08", 10) => 8



 Comments   
Comment by Francis Avila [ 27/Feb/14 10:38 PM ]

Discovered integer-with-radix parsing problem but split it into CLJS-775 since it's not a simple parseInt issue.

This patch should fix ratio and inst parsing, but I don't have access to a browser which infers octals in parseInt.

Existing tests are sufficient to catch this problem for #inst.

There are no ratio tests for reader at all currently, but since ratios are shaky ground in cljs anyway I didn't add any.

Comment by Dave Della Costa [ 12/Mar/14 1:14 AM ]

Just chiming in to mention that I have experienced this issue in Internet Explorer 8 and was going to submit a patch myself. Would be interested in getting this into the next version as for now we have a customized reader that we use to get past this.

Comment by David Nolen [ 12/Mar/14 8:26 AM ]

fixed https://github.com/clojure/clojurescript/commit/004224511b4351acbfe051c7dea913fb0c183c04





[CLJS-782] cljs.core.UUID should have a toString method Created: 10/Mar/14  Updated: 12/Mar/14  Resolved: 12/Mar/14

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

Type: Enhancement Priority: Trivial
Reporter: James Reeves Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Sometimes it's useful to turn a UUID into a string, particularly when interoperating with Javascript. Currently the toString implementation for UUIDs is quite opaque:

=> (str #uuid "f47ac10b-58cc-4372-a567-0e02b2c3d479")
"[object Object]"

Mimicking the behavior in Clojure/Java would be preferable:

=> (str #uuid "f47ac10b-58cc-4372-a567-0e02b2c3d479")
"f47ac10b-58cc-4372-a567-0e02b2c3d479"


 Comments   
Comment by David Nolen [ 12/Mar/14 8:23 AM ]

fixed https://github.com/clojure/clojurescript/commit/2870101b1a4ad4ef70ff06df3c04cda5d621b2cc





[CLJS-510] Problems with :optimizations :whitespace & :output-wrapper true Created: 28/May/13  Updated: 11/Mar/14

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

Type: Defect Priority: Minor
Reporter: Aku Kotkavuo Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

OS X 10.8.3, Google Chrome 28



 Description   

When I have both output wrapper and :whitespace optimizations enabled, I get the following error when loading the produced output:

goog.provide("goog.string");
goog.provide("goog.string.Unicode");
goog.string.Unicode = {NBSP:"\u00a0"};
^- Uncaught TypeError: Cannot set property 'Unicode' of undefined

Any ideas on what is going wrong? The error disappears if I use :simple or :advanced optimizations or if I set :output-wrapper to false (combined with any optimization level).



 Comments   
Comment by Aku Kotkavuo [ 28/May/13 10:14 AM ]

ClojureScript version used is 0.0-1806.

Comment by Aku Kotkavuo [ 28/May/13 10:30 AM ]

I'm also seeing this:

GET http://localhost:7070/deps.js 404 (Not Found)
goog.writeScriptTag_
goog.importScript_
goog.importScript_(goog.basePath + "deps.js")

I have no idea what deps.js is, something related to Google Closure Compiler?

Comment by David Nolen [ 19/Nov/13 9:23 PM ]

Is this problem still present? Thanks.

Comment by Immo Heikkinen [ 11/Mar/14 3:59 AM ]

The problem is still present in clojurescript master. To reproduce, compile https://github.com/clojure/clojurescript/tree/master/samples/hello using

cljsc src {:output-wrapper true :optimizations :whitespace} > hello.js




[CLJS-781] ClojureScript Browser REPL analysis regression Created: 07/Mar/14  Updated: 07/Mar/14  Resolved: 07/Mar/14

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

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


 Comments   
Comment by David Nolen [ 07/Mar/14 3:29 PM ]

Latest 2173 has an analysis regression that results in spurious warnings about cljs.core symbols that did not exist in 2156. I strongly suspect this is related to http://dev.clojure.org/jira/browse/CLJS-615. When that patch landed I tested only the behavior of the browser REPL from within the repo and did not test the behavior when used with CLJS dependencies in a project.

Comment by David Nolen [ 07/Mar/14 3:37 PM ]

After closer inspection this does appear to be a bug in ClojureScript itself, rather lein-cljsbuild's browser REPL support.





[CLJS-780] Incorrect behaviour of apply-to for (>= argc 6) Created: 03/Mar/14  Updated: 05/Mar/14  Resolved: 05/Mar/14

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

Type: Defect Priority: Major
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None


 Description   

The cljs.core/apply-to function exhibits incorrect behaviour when called with an "argument count" argc (the second argument to apply-to) greater than or equal to 6.

This is because the cljs.core/gen-apply-to macro produces (essentially) to following function:

(defn apply-to
  [f argc args]
  (let [args (seq args)]
    (if (== argc 0)
      (f)
      (let [[a & args] args]
        (if (== argc 1)
          (f a)
          (...
            (if (== argc 6)
              (let [[f % args] args] ;; f is being overshadowed
                (f a b c d e f)
                (if (== argc 7)
                  (let [[g & args] args]
                    (f a b c d e f g)
                    (...)))))))))))

For (>= argc 6) the f in

(defn apply-to [f argc args] ...)

is being overshadowed by the one in

(let [[f % args] args] ...)

The obvious solution would be to output something like

(defn apply-to
  [func argc args]
  (let [args (seq args)]
    (if (== argc 0)
      (func)
      (let [[a & args] args]
        (if (== argc 1)
          (func a)
          (...
            (if (== argc 6)
              (let [[f % args] args]
                (func a b c d e f)
                (if (== argc 7)
                  (let [[g & args] args]
                    (func a b c d e f g)
                    (...)))))))))))

The following changes to gen-apply-to-helper and gen-apply-to should do the trick:

(defn gen-apply-to-helper
  ([] (gen-apply-to-helper 1))
  ([n]
     (let [prop (symbol (core/str "-cljs$core$IFn$_invoke$arity$" n))
           func (symbol (core/str "cljs$core$IFn$_invoke$arity$" n))]
       (if (core/<= n 20)
         `(let [~(cs (core/dec n)) (-first ~'args)
                ~'args (-rest ~'args)]
            (if (core/== ~'argc ~n)
              (if (. ~'func ~prop)
                (. ~'func (~func ~@(take n cs)))
                (~'func ~@(take n cs)))
              ~(gen-apply-to-helper (core/inc n))))
         `(throw (js/Error. "Only up to 20 arguments supported on ifntions"))))))
         
(defmacro gen-apply-to []
  `(do
     (set! ~'*unchecked-if* true)
     (defn ~'apply-to [~'func ~'argc ~'args]
       (let [~'args (seq ~'args)]
         (if (zero? ~'argc)
           (~'func)
           ~(gen-apply-to-helper))))
     (set! ~'*unchecked-if* false)))


 Comments   
Comment by David Nolen [ 04/Mar/14 11:33 AM ]

apply-to is an internal helper do you have an example where this is a problem in practice? Thanks.

Comment by Kevin Marolt [ 04/Mar/14 5:26 PM ]

Sure, try this with :optimizations :none (I haven't tested it with other optimization settings):

(def a (atom {:foo (with-meta [] {:bar '(1 2 3)})}))
(swap! a update-in [:foo] vary-meta update-in [:bar] vector)
(prn (= @a {:foo (with-meta [] {:bar [1 2 3]})})) ;; prints "false"
(prn (= @a [{:foo (with-meta [] {:bar '(1 2 3)})}
            [:foo]
            vary-meta
            update-in
            [:bar
            vector])) ;; prints "true"

The goal was to vectorize the list '(1 2 3), but swap! is applying update-in to the arguments

{:foo (with-meta [] {:bar '(1 2 3)})} ;; a
[:foo]                                ;; b
vary-meta                             ;; c
update-in                             ;; d
[:bar]                                ;; e
vector                                ;; f

and thus, because vector is in sixth position (and therefore the f-argument), what's actually happening is that vector is incorrectly called with those same arguments instead.

Comment by David Nolen [ 05/Mar/14 8:51 AM ]

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





[CLJS-779] Permit omitting hashbang with nodes target. Created: 03/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-779.patch    

 Description   

In some environments (such as parse.com's Code Cloud CLJS-722), the node.js hashbang should be omitted. Currently if the target is "nodejs", a hashbang will always be applied at the top of the file.



 Comments   
Comment by Michael Glaesemann [ 03/Mar/14 5:48 PM ]

Following brief discussion on CLJS-771 and <https://groups.google.com/d/msg/clojurescript/CuInr2L5yFo/562AIcRsrksJ>, I've worked up a patch that omits the hashbang if :hashbang false is added as a compiler option.

Another option would be to go back to the original behavior that if the :preamble option is provided, it assumes the necessary hashbang is provided in the preamble. I find this a bit unintuitive (which is why I provided a patch to allow preambles with the node.js target anyway), and think that this is a better, more explicit solution.

Comment by David Nolen [ 04/Mar/14 11:44 AM ]

fixed https://github.com/clojure/clojurescript/commit/00a9b7bde0c8823175560b453a00e7be09ddd250





[CLJS-778] Unable to call `set` on `RSeq`s Created: 03/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Defect Priority: Minor
Reporter: Omri Bernstein Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

ClojureScript release 1798+
(Probably not relevant: Leiningen 2.3.4, Java 1.7.0_51, Ubuntu 12.04, Toshiba Satallite L755)



 Description   

Error when I run, at a REPL:

user> (set (reverse [0]))
"Error evaluating:" (set (reverse [0])) :as "cljs.core.set.call(null,cljs.core.reverse.call(null,new cljs.core.PersistentVector(null, 1, 5, cljs.core.PersistentVector.EMPTY_NODE, [0], null)))"
org.mozilla.javascript.JavaScriptException: Error: No protocol method INext.-next defined for type cljs.core/RSeq: (0) (cljs/core.cljs#266)
at cljs/core.cljs:266 (anonymous)
at cljs/core.cljs:260 (_next)
at cljs/core.cljs:6368 (set)
at <cljs repl>:1 (anonymous)
at <cljs repl>:1
nil

(Specifically, this is in a SublimeREPL Rhino REPL.)

The problem seems to happen for any `cljs.core/RSeq` types. I've tried loading different ClojureScript release versions. The problem seems to exist for releases 1798 and higher but not for 1586 and lower. If someone could explain how to load non-released ClojureScript commits, I would be happy to narrow the window even further.



 Comments   
Comment by David Nolen [ 04/Mar/14 11:41 AM ]

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





[CLJS-777] multimethods should be ifn? Created: 01/Mar/14  Updated: 01/Mar/14  Resolved: 01/Mar/14

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

Type: Defect Priority: Major
Reporter: Jacob Maine Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

In regular Clojure, this passes

(defmulti mm :type)
(assert (ifn? mm))

But it fails in ClojureScript.

Note that (fn? mm) is false for Clojure, so this can't be fixed by adding the Fn marker protocol to MultiFn.



 Comments   
Comment by David Nolen [ 01/Mar/14 3:26 PM ]

fixed https://github.com/clojure/clojurescript/commit/525154f2a4874cf3b88ac3d5755794de425a94cb





[CLJS-771] Allow nodejs target preambles Created: 21/Feb/14  Updated: 26/Feb/14  Resolved: 21/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-771.2.patch     Text File CLJS-771.patch    

 Description   

Currently targeting nodejs precludes the use of a preamble. There are cases where you might want to add arbitrary code (besides the node hashbang) at the top of the compiled file. (see brief discussion at https://groups.google.com/forum/#!topic/clojurescript/CuInr2L5yFo)



 Comments   
Comment by David Nolen [ 21/Feb/14 1:16 PM ]

Thanks but it appears your patch is dirty - it includes compiler version information. Will apply if we get a cleaned up version. Thanks!

Comment by Michael Glaesemann [ 21/Feb/14 1:23 PM ]

Updated (git -a is not always helpful).

Comment by David Nolen [ 21/Feb/14 1:40 PM ]

fixed http://github.com/clojure/clojurescript/commit/4b7cdd4fb3cfae1c2e325a78eae54de0cb164d78

Comment by Travis Vachon [ 26/Feb/14 12:59 PM ]

Arg, sorry I missed this - the problem is that not all node-like environments need a hashbang. As this code used to be it was up to the creator of the preamble to include the hashbang if they wanted one - the idea was that the hashbang was just the default preamble in a node environment.

I've replied to the list addressing the original motivation for this patch, but I'd very much like to see it reconsidered (except for the test, which is great! though of course I'd want it updated for the behavior I think is correct ;-D )

happy to submit a followup patch if needed

Comment by Michael Glaesemann [ 26/Feb/14 1:29 PM ]

That'd be an option, though it's also possible to set the value of the hashbang. Does :hashbang "" do what you want?

Comment by Michael Glaesemann [ 26/Feb/14 1:31 PM ]

Scratch that, it doesn't, does it. The code prepends #! to the path to node.





[CLJS-772] Enhance Node.js Support Created: 24/Feb/14  Updated: 24/Feb/14  Resolved: 24/Feb/14

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

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


 Description   

Google Closure now comes with a Node.js bootstrap script that makes require and provide work https://code.google.com/p/closure-library/wiki/NodeJS. This enhancement requires a newer version of Closure Library.



 Comments   
Comment by David Nolen [ 24/Feb/14 7:48 AM ]

fixed https://github.com/clojure/clojurescript/commit/0c7b31ada01237de33cef77b817ccef3f2b3576d





[CLJS-728] (get [1 2 3] nil) -> 1 Created: 16/Dec/13  Updated: 24/Feb/14  Resolved: 24/Feb/14

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

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

Attachments: File cljs-728-minimal.diff     Text File cljs-728-more-minimal.patch     Text File cljs-728-nth-number.patch     Text File cljs-728-only-nth-lookup.patch     Text File cljs-728-only-nth-lookup.patch    

 Comments   
Comment by Francis Avila [ 26/Dec/13 4:49 PM ]

This is due to javascript coercing null to 0 in the various places where -assoc and -nth check (<= 0 n) or (bit-and n MAGIC-NUMBER). E.g. in PersistentVector:

IIndexed
  (-nth [coll n]
    (aget (array-for coll n) (bit-and n 0x01f)))
  (-nth [coll n not-found]
    (if (and (<= 0 n) (< n cnt))
      (-nth coll n)
      not-found))

I can track all these down and patch them by changing the guards to (and (not (nil? n)) (<= 0 n) (< n cnt)) (or maybe even (or (zero? n) (< 0 n cnt))) and adding (when-not (nil? n) ...) guards where appropriate.

However I wasn't sure if there's any intention to make the comparison operators <= < > >= null-safe. I.e. instead of adding these extra nil checks, should we instead make sure comparing against nil always returns false or throws an exception? (We would still need nil checks to make sure (get [1] nil) returns nil like Clojure.)

Comment by Francis Avila [ 26/Dec/13 11:26 PM ]

Currently failing test cases.

(assert (nil? (get [1 2] nil)))
(assert (= :fail (try (nth [1 2] nil) (catch js/Error e :fail))))
(assert (= 4 (get [1 2] nil 4)))
(assert (= 4 (nth [1 2] nil 4)))
(assert (= :fail (try (assoc [1 2] nil 4) (catch js/Error e :fail))))

(assert (nil? (get (transient [1 2]) nil)))
(assert (= :fail (try (nth (transient [1 2]) nil) (catch js/Error e :fail))))
(assert (= 4 (get (transient [1 2]) nil 4)))
(assert (= 4 (nth (transient [1 2]) nil 4)))
(assert (= :fail (try (assoc! (transient [1 2]) nil 4)
                   (catch js/Error e :fail))))

(assert (nil? (get (subvec [1 2] 1) nil)))
(assert (= :fail (try (nth (subvec [1 2] 1) nil) (catch js/Error e :fail))))
(assert (= 4 (get (subvec [1 2] 1) nil 4)))
(assert (= 4 (nth (subvec [1 2] 1) nil 4)))
(assert (= :fail (try (assoc (subvec [1 2] 1) nil 4)
                   (catch js/Error e :fail))))
Comment by Francis Avila [ 27/Dec/13 2:11 AM ]

Actually this fails for other values that may coerce to zero as well (e.g. false, numeric strings, etc). I solved this with a number? type check in appropriate places. Patch attached with comprehensive tests.

Comment by David Nolen [ 30/Dec/13 8:54 AM ]

Putting number? checks on critical paths is unacceptable for performance reasons. Ideally we only add one number? check. And even then we'd want to know the performance implications.

Comment by Francis Avila [ 30/Dec/13 1:04 PM ]

Is number? really that slow? At least on v8 it seems to be twice as fast as nil?. Some repl checks in v8 don't seem to show a significant performance regression, but without a proper profiling harness and test cases I don't feel comfortable saying that for sure.

I think we can give up some type safety but still catch silent-coerce-to-zero cases by expressing the conditional as (and (< i (.-cnt pv)) (or (< 0 i) (zero? i))). I'll try that later today and try to compare the performance.

Your concern about checking bounds only once can be resolved by moving the guts of array-for into unchecked-array-for and editable-array-for into unchecked-editable-array-for. This is probably a good idea even if we do nothing about (get [1] nil) --at least -pop and -kv-reduce would benefit along with the not-found arities of -nth and -lookup. Perhaps a new ticket for this?

Comment by David Nolen [ 30/Dec/13 1:09 PM ]

No coercions. Any check should be higher up the chain, like at nth and nowhere else.

Comment by Francis Avila [ 30/Dec/13 1:31 PM ]

What do you mean by "no coercions"? Do you mean edge cases like {{(get [1 2] "1") => 2}} are unacceptable even if it means we can avoid a typeof check?

Comment by David Nolen [ 30/Dec/13 1:40 PM ]

No coercions means no coercions. The only valid key to an IVector instance for lookup is a JavaScript number.

Comment by Francis Avila [ 30/Dec/13 1:53 PM ]

OK, then in that case a number? check somewhere is absolutely required, but I can move code around a bit to ensure that no type or bounds checking is done more than once for persistent and transient vectors (there is already a fair amount of redundant bounds checking). I'll work on this tonight.

(I sent a CA about 10 days ago but I see my name isn't on the CA page yet. It's probably just delayed by holiday stuff.)

Comment by Francis Avila [ 31/Dec/13 12:02 AM ]

Updated patch.

  • I've removed all redundant bounds checking that I could easily eliminate using unchecked-array-for in place of array-for.
  • IVector and IIndexed protocol implementations don't have number? checks in them. nth does the check.
  • But the number? check is in the IAssociative and ILookup implementations for these types.
  • The logic here is that for protocols which semantically require a numeric index, we assume the user of the protocol (nth or ci-reduce for example) has already done the necessary type checking on the outside. But for protocols which don't have this semantic (e.g. ILookup), we need to do the check in the protocol implementation.
  • I ran script/benchmark and compared master and this patch on the v8 and spidermonkey engines. I didn't see any performance regressions.
Comment by David Nolen [ 07/Jan/14 7:09 AM ]

Can we remove old patches, thanks.

Comment by Francis Avila [ 07/Jan/14 8:24 AM ]


JIRA won't let me delete them. The latest patch is cljs-728-nth-number.patch

Comment by David Nolen [ 16/Jan/14 5:31 PM ]

This patch does too much. Why are we not just checking in nth?

Comment by Francis Avila [ 16/Jan/14 7:04 PM ]

Remember the problem isn't just (get [1] nil), it's also (get [1] :a) and also on Range, subvectors, and transientvector types (any non-numeric lookup on an indexed type). So a nil? check in nth is nowhere near enough.

For indexed types, get calls -lookup calls -nth (nth is not called), and you didn't want to do a nil? or number? check inside -nth (for good reason, as we can assume callers of -nth know they need to supply a number). This patch puts the number check on indexed types in -lookup (rationale given in my previous comment) and makes sure that other core code is calling -nth safely without checks.

The patch also removes some bounds and type checking that was redundant (the unchecked-array-for and -assoc-n stuff) which I noticed because I had to examine all those calling paths.

I suppose an alternative that would change less code is to have get use IIndexed if present and do the number check there, but then it's impossible for public code to use ILookup if there were a type where there was a difference. (e.g., Suppose an xml-element type wanted to provide access attributes with get and child elements with nth.)

Comment by David Nolen [ 16/Jan/14 10:39 PM ]

The only code that needs to change it seems to me is nth and implementations of ILookup for IIndexed types.

Comment by Francis Avila [ 29/Jan/14 6:49 PM ]

-assoc and -assoc! need fixing too on various types because (assoc [:a] nil :b) => [:b].

I have attached a smaller patch which does not include the hunks related to redundant bounds checking and will move those to CLJS-757. This is the smallest patch which will pass all the tests related to using non-numeric indexes with indexed types.

Comment by David Nolen [ 29/Jan/14 8:26 PM ]

-assoc and -assoc! do not need fixing. Only -assoc-n and -assoc-n! do. The patch needs to be far more minimal if it hopes to get accepted.

Comment by Francis Avila [ 30/Jan/14 11:46 AM ]

It doesn't make sense for indexed types to have -lookup call -nth but -assoc-n call -assoc (as PersistentVector and Subvec do, but not TransientVector). This means if we know we are using a numeric key we call -nth on read, but call -assoc on writes instead of -assoc-n. -assoc-n is what has the required "key is a number" semantic, not -assoc, and this leads to an unnecessary number? check being done in the implementation of Subvec -conj. (There are also other places, e.g. Subvec -assoc, where -assoc-n could be called instead of assoc or -assoc, but I don't address those.)

In other words, it would be best if -assoc-n/-assoc-n! consistently meant "numeric key" 1) so that callers could avoid a number? check if they know their key is numeric, 2) so there is a sane analogy -lookup:-nth :: -assoc:-assoc-n :: -assoc!:-assoc-n!

Comment by Francis Avila [ 30/Jan/14 11:49 AM ]

New patch cljs-728-more-minimal.patch

Hunk-by-hunk justification of changes:

  1. core_test.cljs
    1. test (get x n) (get x n nf) (nth x n) (nth x n nf) and (assoc x n v) with non-numeric n against PersistentVector, Subvec, TransientVector, and Range types.
  2. core.cljs
    1. number? check in nth
    2. number? check in PersistentVector -lookup
    3. number? check in PersistentVector -assoc
    4. number? check in Subvec -lookup
    5. number? check in Subvec -assoc
    6. number? check in TransientVector -assoc!
    7. number? check in TransientVector -lookup

Although it does not swap -assoc-n and -assoc on Subvec and PersistentVector, I would hardly describe this patch as
"far more minimal" than the previous one--I hope that does not mean it is unacceptable. The only way I can see to
make this patch smaller is to file a different ticket for each hunk and corresponding test, which seems crazy.

If the patch is still not acceptable, could you please be much more specific about what exactly is wrong with it so we can close this bug.

Comment by David Nolen [ 30/Jan/14 12:03 PM ]

That the ClojureScript vector types implement assoc in terms of assoc-n and not the other way around should be considered a flaw. Please refer to Clojure's PersistentVector.java, RT.java for a guide about how the logic should be implemented. I'll take a patch that aligns ClojureScript's logic closer to Clojure. Thanks.

Comment by Francis Avila [ 17/Feb/14 11:54 AM ]

Patch that aligns ClojureScript vector type's -assoc-n/-assoc with Clojure's is in CLJS-767, and patch that fixes assoc! on TransientVectors is in CLJS-768.

I will cut conflicting hunks out of this CLJS-728 patch (hunks 2.3, 2.5 and 2.6 as outlined above) so all can apply cleanly with one another. (I'll have to do it this evening though.)

Full list of related tickets and patches (stuff that was factored out of the initial patches):

  1. CLJS-757: remove redundant bounds checking in indexed types (will need a rebase once the others are in).
  2. CLJS-767: Fix assoc on PersistentVector and Subvec
  3. CLJS-768: Fix assoc! on TransientVector
Comment by David Nolen [ 17/Feb/14 12:00 PM ]

Francis Avila, many thanks will look over these.

Comment by Francis Avila [ 17/Feb/14 10:49 PM ]

Patch rebased and trimmed: cljs-728-only-nth-lookup.patch.

Comment by Francis Avila [ 24/Feb/14 12:41 AM ]

Rebased cljs-728-only-nth-lookup.patch again. (core_test.cljs changes would have "conflicted".)

Comment by David Nolen [ 24/Feb/14 6:04 AM ]

fixed https://github.com/clojure/clojurescript/commit/71f781c75bf6e9d19be8d0e529ed0275fa523942





[CLJS-757] Redundant bounds checking in indexed types Created: 29/Jan/14  Updated: 24/Feb/14  Resolved: 24/Feb/14

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

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

r2156


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

 Description   

PersistentVector and TransientVector contain code paths where a bounds check is performed more than once unnecessarily.



 Comments   
Comment by Francis Avila [ 29/Jan/14 11:44 PM ]

Patch gives a minor across-the-board speedup on vector operations.

I also discovered and fixed an off-by-one check in PersistentVector's -seq, which causes an unnecessary ChunkedSeq to be created when vector length is exactly 32.

Comment by David Nolen [ 23/Feb/14 4:34 PM ]

Can we get a rebased version of this patch? Thanks.

Comment by Francis Avila [ 24/Feb/14 12:22 AM ]

Rebased cljs-757 with a change to unchecked-array-for-longvec (now first-array-for-longvec) and extra comments documenting invariants.

Comment by David Nolen [ 24/Feb/14 5:59 AM ]

fixed https://github.com/clojure/clojurescript/commit/0f943b616e36c65e0a8921fb838b71368fe9674c





[CLJS-739] fn/recur bug Created: 01/Jan/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

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


 Description   
(defn create-functions [arr names]
  (let [name (first names)]
    (if name
      (recur (conj arr (fn [] (println name)))
             (rest names))
      arr)))
 
(doseq [fn (create-functions [] [:a :b :c :d])] (fn))

Generates the following incorrect code, the local name is not closed over:

cljs_play.let_bug.core.create_functions = function create_functions(arr, names) {
  while (true) {
    var name = cljs.core.first.call(null, names);
    if (cljs.core.truth_(name)) {
      var G__4744 = cljs.core.conj.call(null, arr, function(arr, names) {
        return function() {
          return cljs.core.println.call(null, name);
        };
      }(arr, names));
      var G__4745 = cljs.core.rest.call(null, names);
      arr = G__4744;
      names = G__4745;
      continue;
    } else {
      return arr;
    }
    break;
  }
};


 Comments   
Comment by David Nolen [ 01/Jan/14 9:43 PM ]

This is because we don't do two passes to determine if a function uses recur. loop/recur works because we know up front.

Comment by David Nolen [ 23/Feb/14 6:04 PM ]

I actually can't reproduce this in master - I've added tests to confirm https://github.com/clojure/clojurescript/commit/aaae2688aa2646bb252a9a4dd321e9fa8dbedb6a





[CLJS-745] Support keywords and namespaced keywords in destructuring Created: 06/Jan/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

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


 Description   

http://dev.clojure.org/jira/browse/CLJ-1318, slated for 1.6



 Comments   
Comment by David Nolen [ 23/Feb/14 4:52 PM ]

fixed https://github.com/clojure/clojurescript/commit/95d60acf610b255dbb4079ccf60739c81a611922





[CLJS-743] Allow language_in and language_out options to be passed to the Google Closure compiler Created: 04/Jan/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Ivan Willig Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-743.patch    

 Description   

While I was messing around with ClojureScript and nodejs, I
noticed that the Google Closure compiler complains when JavaScript
reserved words are used. Newer versions of JavaScript engines allow
reserved words (like "static", "class") to be used as object
properties. The Closure Compiler knows this and does not complain when
you set the language_in option to the correct version of JavaScript.



 Comments   
Comment by David Nolen [ 07/Jan/14 7:07 AM ]

Excellent thanks.

Comment by David Nolen [ 09/Jan/14 5:17 PM ]

Ivan have you signed and sent in your CA?

Comment by Chas Emerick [ 20/Feb/14 9:23 AM ]

@iwillig Looking forward to seeing this, def send in that CA.

Comment by Ivan Willig [ 20/Feb/14 10:06 PM ]

I apologize for letting this slip, work got busy. CA is in the mail.

Comment by David Nolen [ 23/Feb/14 4:33 PM ]

fixed, https://github.com/clojure/clojurescript/commit/b33bde39fe838d367939b55b9dadb0480ea00a7c





[CLJS-768] (persistent! (assoc! (transient [0]) nil 1)) => [1] Created: 17/Feb/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

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

r2156


Attachments: Text File cljs-768.patch     Text File cljs-768.patch    
Patch: Code and Test

 Description   

assoc!-ing a non-numeric index into a transient vector is the same as assoc!-ing index 0. This should throw an exception instead. Patch and tests attached.



 Comments   
Comment by David Nolen [ 20/Feb/14 11:41 AM ]

This patch no does not cleanly apply on master. Can we get a new patch? Thanks.

Comment by Francis Avila [ 21/Feb/14 9:31 AM ]

Rebased patch.

Comment by David Nolen [ 23/Feb/14 4:30 PM ]

fixed https://github.com/clojure/clojurescript/commit/8419d8aeb6d2e975029fc693f96137775d2442c6





[CLJS-770] Include :js-dependency-index in result of `(env/default-compiler-env)` Created: 18/Feb/14  Updated: 23/Feb/14  Resolved: 23/Feb/14

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

Type: Defect Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: File CLJS-770.diff    
Patch: Code

 Description   

It looks like the change from CLJS-615 requires the addition of something like this prior to using any of the dependency machinery:

(swap! env/*compiler* assoc :js-dependency-index (cljsc/js-dependency-index opts))

That the dependency index is now in the compiler environment isn't a problem (on the contrary), but cljs.env/default-compiler-env is there for exactly this sort of thing: the environment that fn returns should have :js-dependency-index added already. This would necessitate pulling all of the JS dependency resolution machinery up into cljs.env (or perhaps another namespace, cljs.js-deps?), but would eliminate the user-facing complication of getting a useful compiler environment with a side benefit of getting all the JavaScript dependency stuffs out of cljs.closure and into a home of their own.

I'm mucking with this in conjunction w/ CLJS-656; shouldn't be difficult to do afterwards.

Interested?



 Comments   
Comment by David Nolen [ 18/Feb/14 8:11 AM ]

I'm ok with pulling this stuff into it's own namespace.

Comment by Chas Emerick [ 23/Feb/14 9:35 AM ]

Attached patch refactors goog-style JS dependency stuff into cljs.js-deps, and changes cljs.env/default-compiler-env to populate :js-dependency-index based on the provided set of options.

I didn't move the externs stuff, seemed like it should stay in cljs.closure.

Note that this doesn't change the fact that CLJS compiler API consumers will need to change: cljs.env/default-compiler-env now takes a map argument of options, which needs to be provided if such consumers need to have their e.g. :libs, :foreign-libs options picked up. The advantage is default-compiler-env again does the job it's intended to do. The refactoring / slimming of cljs.closure is a nice side effect, tho.

Comment by David Nolen [ 23/Feb/14 4:24 PM ]

fixed https://github.com/clojure/clojurescript/commit/90cf724d8f5d4fa5029aad204a9e2bcfe36174d4





[CLJS-615] Warning when required lib does not exist on disk Created: 08/Oct/13  Updated: 22/Feb/14  Resolved: 17/Feb/14

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Timothy Pratley
Resolution: Completed Votes: 0
Labels: patch

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

 Description   

Currently when analyzing dependencies in analyzer.clj we do not warn when the library does not exist on disk. The error should be similar to the one emitted by Clojure so that the user is aware how the namespace maps to directories and filenames.

Problem
==

hello.cljs:

(ns hello
(:require [typomina :refer [x]]))

Expected: Should be reported when typomina is a namespace never provided.
Actual: when optimization is none or whitespace, no warnings are given.
Expected: when x does not exist in typomina, hello should not compile
Actual: x is not verified even if typomina exists

Background
==

Dependencies are not resolved at cljs compile time.
cljs.closure docstring:
"build = compile -> add-dependencies -> optimize -> output"

Dependencies are deferred to Closure
https://github.com/clojure/clojurescript/wiki/Differences-from-Clojure:
"designed to leverage Google's Closure library, and participates in its dependency/require/provide mechanism"

Closure compiler detects missing dependencies when optimizations simple or advanced are applied:
./bin/cljsc hello.cljs '{:optimizations :simple :output-to "hello.js"}'
Dec 31, 2013 4:10:03 PM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: hello:3: ERROR - required "typomina" namespace never provided
goog.require('typomina');
^
ERROR: JSC_MISSING_PROVIDE_ERROR. required "typomina" namespace never provided at hello line 3 : 0

But not for whitespace or none. For none, Closure is not even called by cljs "optimize".
./bin/cljsc hello.cljs '{:optimizations :whitespace :output-to "hello.js"}'

The compile phase of Clojurescript looks for things to compile by accumulating anything it finds in :import :require :use :require-macros :use-macros. However if it can't find those dependencies it is not considered an error and this is important to preserve because those dependencies can be provided by JavaScript. Clojurescript makes no distinction between import and require.

Solution
==

(1) If there are unprovided dependencies after get-dependencies, let the user know:
closure.clj:add-dependencies
provided (mapcat :provides (concat required-js required-cljs))
unprovided (clojure.set/difference (set requires) (set provided))] (when unprovided
(apply println "ERROR, required namespace never provided for" (sort unprovided)))

(2) Build the JavaScript index prior to cljs compilation, and use it to determine if a require is not provided by either JS or CLJS
analyzer.clj:analyze-deps

(3) A useful warning for :undeclared-ns-form was hidden due to typos in the 'type' checking. 'type' checking outside of (warning ) was redundant as the (warning ) handler checks if that 'type' is enabled. External checks removed.

(4) no-warn was used for subdeps in analyze-deps
It seems this will just hide real issues.

(5) There was an opts flag :warnings which would disable :undeclared-var :undeclared-ns :undeclared-ns-form
when undefined. Tools like leincljsbuild do not enable it, and I can't find the option documented or recommended. This patch removes the :warnings option in favor of reporting them. Obviously this means you see more warnings reported now. They are real warnings. I recommend a separate ticket/patch to expose and document warning controlling options (:warnings should control all warnings, not just undeclared - and there should be the ability to turn on/off individual or sets of warnings.) Also (repl) function takes a "warn-on-undeclared" optional key which if missing will disable error checking - I think I should take this out or default it to true when not present.

Some discussion here:
https://groups.google.com/forum/#!topic/clojurescript/7vaEETMW5xg



 Comments   
Comment by David Nolen [ 02/Jan/14 1:00 PM ]

This looks nice and comprehensive, but it will take some time for me to review. Will try to give it a look over the weekend.

Comment by Timothy Pratley [ 03/Jan/14 3:40 PM ]

O.K. great
Some things to consider:

a) Many more warnings are displayed now. To me that is a very good thing. It might be a shock to some users with existing codebases. Things like:
(JSON/stringify x) [should be (js/JSON.stringify x)]
canvas/offsetWidth [should be (.-offsetWidth canvas)]

b) Warnings are propagated from libraries e.g.:
WARNING: clone already refers to: /clone being replaced by: domina/clone at line 147
Again to me this is a very good thing, but it might shock users.

c) Behavior of (1) does not work well with cljsbuild. It is fine for a clean build, but with multiple dependencies and only touching one file, cljsbuild only recompiled that particular file and reports the other dependencies as missing. I plan to investigate further. We don't really need to post check dependencies provided (2) is acceptable and could just remove (1).

I think we will need to discuss further on turning on/off warnings, how that should be exposed to users, and what the default behavior should be.

Comment by Timothy Pratley [ 03/Jan/14 10:24 PM ]

building a directory results in spurious require warnings - investigating further

Comment by Timothy Pratley [ 05/Jan/14 2:42 AM ]

Additional changes to address directory build warnings

Comment by David Nolen [ 07/Jan/14 7:11 AM ]

Is CLJS-740 also addressed by this patch?

Comment by Timothy Pratley [ 07/Jan/14 10:27 AM ]

Yes, that is correct.

Comment by Timothy Pratley [ 15/Jan/14 10:58 AM ]

Hi David, have you had a chance to look at the patch? Let me know if it requires any adjustments.

Comment by David Nolen [ 15/Jan/14 11:47 AM ]

I will review but honestly what we need are some testers besides myself.

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

Ok I reviewed the patch. It looks good however I cannot apply it with `git am`.

Comment by David Nolen [ 17/Jan/14 9:21 AM ]

The problem with the patch is that's not plain text. I copied to a file on a machine to fix that. I applied it and I now get a series of warnings when running the tests. These need to be addressed by the patch. In addition when I run the ClojureScript REPL I get many many errors.

Comment by David Nolen [ 17/Jan/14 9:26 AM ]

I've gone and applied CLJS-740 to master as it's a simpler fix for an immediate issue. Let's rebase this one and address the issues. Thanks!

Comment by Timothy Pratley [ 03/Feb/14 12:39 AM ]

Attaching rebased patch which resolves tests emitting warnings and repl not launching.

Comment by David Nolen [ 03/Feb/14 8:22 AM ]

With the patch applied running the tests still emit warnings for goog.string, the constants table, and line 1956 in the core_test.cljs. These need to be addressed.

Comment by Timothy Pratley [ 16/Feb/14 2:20 PM ]

./scripts/test warnings suppressed. Not sure why this patch doesn't have the previous patch included in it, presumably I need to do something to combine them.

Comment by David Nolen [ 16/Feb/14 2:43 PM ]

High level question - what is the logic for the suppression of the warnings? Also do I need to apply two patches?

Comment by Timothy Pratley [ 16/Feb/14 5:33 PM ]

Hi David, the warnings around goog.string were suppressed by requiring goog.string in test_core. This is necessary because the with-out-str macro expands to goog.string/stuff. In one sense this seems correct in that code relying on good.string should require it, but in another sense feels bad because using core should not require other stuff. Alternatively if there is a list of things we know we need like goog.string perhaps it should just be treated as known. In fact for the constants table dependency I have to treat it as known because it is compiler internal. For the line 1956 there was a self-referential def form, and so the warning seemed appropriate. To silence that one I added an empty def before the self-referential def.

I think the reason why there are two patches is I must have applied the original to my master to check it, I'll post a fresh one soon as I sort it out.

Comment by David Nolen [ 16/Feb/14 5:39 PM ]

Ok, I think the preferred solution is to make any Google Closure we import/require in core.cljs to be implicit so we don't have to do this in test_core.cljs as you've done.

Comment by Timothy Pratley [ 16/Feb/14 5:44 PM ]

Single file patch

Comment by Timothy Pratley [ 16/Feb/14 5:46 PM ]

Ok sure no problem will change that

Comment by Timothy Pratley [ 17/Feb/14 1:17 AM ]

Added goog.string to the special list in confirm-ns that do not need to be required, as with-out-str macro expands to that.

Comment by David Nolen [ 17/Feb/14 8:24 AM ]

Patch looks good but could we please get a squashed patch? Thanks.

Comment by Timothy Pratley [ 17/Feb/14 11:24 PM ]

Squashed patch attached

Comment by David Nolen [ 17/Feb/14 11:40 PM ]

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

Comment by David Nolen [ 17/Feb/14 11:40 PM ]

Awesome patch many thanks!

Comment by Chas Emerick [ 18/Feb/14 7:44 AM ]

The addition of :js-dependency-index to the compiler environment raises a minor complication that I'd like to address; see CLJS-770.

Comment by Gary Trakhman [ 22/Feb/14 12:20 AM ]

I made a note on github, and I'll repeat it here.

The patch in question causes errors when the rhino repl is initialized:

I saw the behavior first by using piggieback, then verified it with the test that was disabled in this commit:
https://github.com/clojure/clojurescript/commit/1b3e1c6168cba66c12749f5bcb6747ef0b8a2950

Reverting the patch in this ticket fixed the problem.

Comment by Gary Trakhman [ 22/Feb/14 12:31 AM ]

I also verified that reverting my patch from #764 has no effect.

Comment by Chas Emerick [ 22/Feb/14 8:17 AM ]

Gary, what are the errors you see? I know Austin won't work because of what's described in CLJS-770, but perhaps there's more going on?

Comment by Gary Trakhman [ 22/Feb/14 4:00 PM ]

the workaround in CLJS-770 seems to fix the problem I'm seeing.

Made my test do this, and it passes:
(doto (env/default-compiler-env)
(swap! assoc :js-dependency-index (cljs.closure/js-dependency-index {})))





[CLJS-764] repl/evaluate-form should pass along file-information Created: 16/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Major
Reporter: Gary Trakhman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

I was getting missing file info for core defs in a repl context, where normally (a full compile) would show the correct information.

This is required for repl-level file-metadata, which cider needs for jump-to-definition.
https://github.com/clojure-emacs/cider/issues/462

With the current patch, cider can jump through cljs files and jar resources correctly.



 Comments   
Comment by Gary Trakhman [ 16/Feb/14 9:44 AM ]

fix and repl-based test

Comment by Gary Trakhman [ 16/Feb/14 9:00 PM ]

In piggieback, load-stream seems to miss file/line for transitive requires, but it looks fine when I try to replicate it in a cljs test. Will work on it.

Comment by Gary Trakhman [ 19/Feb/14 12:40 PM ]

Further testing shows that file+line is indeed making it through, I think the current patch is enough.

Comment by David Nolen [ 20/Feb/14 11:43 AM ]

Because of whitespace changes it's impossible to see what changed. Can you explain the changes? Thanks!

Comment by Gary Trakhman [ 20/Feb/14 12:09 PM ]

The relevant line is '(binding [ana/*cljs-file* filename]'.

It sets the var so code down the line can pick it up.

Comment by David Nolen [ 20/Feb/14 12:20 PM ]

fixed https://github.com/clojure/clojurescript/commit/953819ff7e4250e593489445ae650856cd0ce1b7





[CLJS-761] no method may be called on number literals Created: 02/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Minor
Reporter: Ryan Mulligan Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

http://himera.herokuapp.com/index.html
Chromium 32.0.1700.77 (244343)
Linux hostname 3.12.7-2-ARCH #1 SMP PREEMPT Sun Jan 12 13:09:09 CET 2014 x86_64 GNU/Linux



 Description   

Himera REPL v0.1.5
cljs.user> (.toString 1)
Compilation error: SyntaxError: Unexpected token ILLEGAL

The problem is that it compiles down to

1.toString()

and the parser interprets "1." as a literal float, followed by junk. This can be fixed with parenthesis around the literal:

(1).toString()



 Comments   
Comment by Ryan Mulligan [ 02/Feb/14 1:11 PM ]

At dc4ba2e this happens at the REPL started by following the instructions at https://github.com/clojure/clojurescript/wiki/Quick-Start

ClojureScript:cljs.user> (.toString 1)
"Error evaluating:" (.toString 1) :as "1.toString()"
org.mozilla.javascript.EvaluatorException: missing ; before statement (<cl
js repl>#1)

Comment by Francis Avila [ 03/Feb/14 10:36 AM ]

Duplicate of CLJS-715





[CLJS-766] Fix broken NodeJS samples Created: 16/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

Type: Defect Priority: Minor
Reporter: Jeff Dik Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Broken by CLJS-722



 Comments   
Comment by David Nolen [ 20/Feb/14 11:47 AM ]

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





[CLJS-756] undeclared ns warning needs to check for existing required namespaces Created: 29/Jan/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

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


 Description   

with-out-str currently produces a warning even though goog.string is loaded by core.clj.



 Comments   
Comment by David Nolen [ 29/Jan/14 11:46 AM ]

This means we need to always know the entire set of namespaces that are currently required. This will eliminate a long outstanding bug with respect to resolving symbols generated by macros that are in namespaces not explicitly required by users as they are implicit when loading the main library namespace. This is of course known for ClojureScript libraries, but we need special casing for Google Closure libs, Closure compatible libs, and foreign libs that the user provides a namespace for.

Comment by David Nolen [ 14/Feb/14 9:04 PM ]

See http://dev.clojure.org/jira/browse/CLJS-615

Comment by David Nolen [ 20/Feb/14 11:46 AM ]

Resolved by CLJS-615





[CLJS-758] Browser REPL doesn't properly catch errors Created: 31/Jan/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

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


 Description   

Brandon Bloom has a patch here http://gist.github.com/brandonbloom/8744556/raw/2efb894b11bb25e9724e3514c7bccc872e30b28d/open+0001-Use-catch-default-in-browser-repl-and-reflect.patch



 Comments   
Comment by David Nolen [ 20/Feb/14 11:45 AM ]

fixed https://github.com/clojure/clojurescript/commit/203d853f1e9c5673cfd987feb7cd07a4762e1479





[CLJS-767] vector and subvector implement assoc-n with assoc; (assoc [0] nil 1) -> [1] Created: 17/Feb/14  Updated: 20/Feb/14  Resolved: 20/Feb/14

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

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

r2156


Attachments: Text File cljs-767.patch    
Patch: Code and Test

 Description   

PersistentVector and Subvec implement their IVector protocol (-assoc-n) with IAssociative (-assoc); it should be the other way around:

  1. Because IVector has an additional numeric index contract that IAssociative does not. The neglected number check causes (assoc [0] nil 1) to return [1] instead of throwing an exception.
  2. Because Java Clojure does not do this (APersistentVector.assoc does a numeric check then calls PersistentVector.assocN).

See comments in CLJS-728 for more detailed rationales.

Patch attached which reverses this, and also fixes the error when a non-number is used as a key/index to assoc on PersistentVector and Subvec. Note: (assoc! (transient [0]) nil) is still broken.



 Comments   
Comment by David Nolen [ 20/Feb/14 11:40 AM ]

fixed, https://github.com/clojure/clojurescript/commit/a7f7ea3b5be1dd450d5503766024cbde9c731147





[CLJS-760] Add an IAtom protocol Created: 01/Feb/14  Updated: 17/Feb/14  Resolved: 17/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Add an IAtom protocol with a -reset! method and a fast path for Atom in cljs.core/reset!.

See jsperf here - http://jsperf.com/iatom-adv

Latest chrome and firefox versions suffer ~20-30% slowdown. Older firefox versions suffer up to 60-70%.



 Comments   
Comment by David Nolen [ 02/Feb/14 11:40 AM ]

This is not a properly formatted patch - http://github.com/clojure/clojurescript/wiki/Patches.

Comment by Jamie Brandon [ 02/Feb/14 11:51 AM ]

Fixed

Comment by David Nolen [ 02/Feb/14 6:33 PM ]

fixed https://github.com/clojure/clojurescript/commit/33692b79a114faf4bedc6d9ab38d25ce6ea4b295

Comment by Jozef Wagner [ 03/Feb/14 2:40 AM ]

What is the rationale behind this? Do you plan to have multiple Atom types?

Comment by David Nolen [ 03/Feb/14 8:13 AM ]

There's no good reason to lock down atom behavior to the one provided by ClojureScript.

Comment by Jozef Wagner [ 03/Feb/14 10:25 AM ]

I agree that such behavior should be polymorphic, I was just surprised that it is a pressing issue in a single threaded CLJS. Clojure itself does not provide such abstraction. Anyway, if you want to get it right, I suggest also to include swap! and compare-and-set! in the protocol (in order to not lock down the sharing and change policy), and to name it something like IMutableReference, as these methods are generic enough to be used outside atom, (as atom promises additional features besides those of a simple mutable reference). See https://gist.github.com/wagjo/8786305 for inspiration.

Comment by David Nolen [ 03/Feb/14 11:21 AM ]

I question the utility of compare-and-set! given most JS hosts are single threaded (but who knows? things are changing quickly in the JS world). It's something to consider in the future. IMutableReference is just more to type and I'm not really convinced it offers anything semantically over IAtom. Atoms do support more functionality but this is covered in other protocols.

Comment by David Nolen [ 03/Feb/14 11:21 AM ]

We need to include -swap! in the IAtom protocol.

Comment by Jozef Wagner [ 03/Feb/14 12:02 PM ]

The usefulness of the atom and protocols behind it should 'transcendent' the notion of single/multiple threads. Prime example is the Avout and its zk-atom and zk-ref. If e.g. implementing Avout in CLJS is a reasonable and useful thing to do, we should prepare for it by designing protocols to handle such use cases.

Comment by David Nolen [ 03/Feb/14 12:30 PM ]

Avout is an third party library which has no relation to the direction of ClojureScript. This ticket is a conservative change that opens up the door for extending atom-like capabilities to user types, that's it. Further enhancements may or may not come in the future.

Comment by David Nolen [ 17/Feb/14 1:49 PM ]

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





[CLJS-765] Subvec should implement IReversible Created: 16/Feb/14  Updated: 16/Feb/14  Resolved: 16/Feb/14

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

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

Attachments: Text File 0001-CLJS-765-implement-IReversible-for-Subvec.patch    

 Description   

Reported by Sunil Nandihalli in this thread:

https://groups.google.com/d/msg/clojure/1XkYDB-FDMA/AAWHMukms8MJ

rseq doesn't work on Subvec, because no implementation of IReversible is provided.

Patch forthcoming.



 Comments   
Comment by Michał Marczyk [ 16/Feb/14 1:54 PM ]

Oops, didn't include tests in the last patch. Same fix + tests this time.

Comment by David Nolen [ 16/Feb/14 2:46 PM ]

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





[CLJS-763] Docstring does not mention argument names in description. Created: 13/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Ryan Mulligan Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

http://clojuredocs.org/clojure_core/clojure.core/rem

The arguments to rem are named num and div and the docstring refers to numerator and denominator.



 Comments   
Comment by Ryan Mulligan [ 14/Feb/14 9:30 AM ]

Sorry, I meant to report this against the Clojure project, not the Clojurescript one. I recommend closing this! Thanks.





[CLJS-762] "goog.require could not find: cljs_gwt.css" since compiler generates wrong code Created: 06/Feb/14  Updated: 08/Feb/14  Resolved: 08/Feb/14

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

Type: Defect Priority: Major
Reporter: Sergey Stupin Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

[org.clojure/clojure "1.5.1"]
[org.clojure/clojurescript "0.0-2156"]

OSX 10.9.1
Chrome Version 32.0.1700.102



 Description   

The project for which the problem occurred is here https://github.com/hsestupin/cljs-gwt.
Steps to reproduce the problem:
1) from one console: lein ring server
2) from other console: lein cljsbuild clean & lein cljsbuild auto dev
3) http://localhost:3000/ in Google Chrome with dev-tools console. There will be an error: "goog.require could not find: cljs_gwt.css"

It was happened because of cljs/cljs_gwt/core.cljs is beginning with lines

(ns cljs-gwt.core
  (:require [cljs-gwt.css :as css]))

And then ClojureScript compiler emits the following from previous 2 lines resources/public/js/cljs_gwt.js

goog.require("cljs_gwt.css");
goog.require("cljs_gwt.css");

instead of

goog.provide("cljs_gwt.css");
goog.require("cljs_gwt.css");

therefore an error occurred. That's it.



 Comments   
Comment by Francis Avila [ 06/Feb/14 4:15 PM ]

There is no goog.provide("cljs_gwt.css") emitted because there is no file that provides that namespace. Your css.clj file is misnamed--it should be css.cljs, otherwise clojurescript won't see it. (Also it has unbalanced parens and an illegal set!, so it won't compile anyway.)

There are two minor issues that you did hit upon, though:

  • You should have gotten a compiler warning for the missing namespace. I'm not sure why you didn't.
  • The cljs compiler may emit the same goog.require multiple times. This doesn't harm anything and the google closure compiler will remove them all.
Comment by Sergey Stupin [ 06/Feb/14 5:00 PM ]

Thanks for explanation, it helps a lot.





[CLJS-592] Clojurescript compiler cannot be AOT compiled Created: 19/Sep/13  Updated: 04/Feb/14  Resolved: 04/Feb/14

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

Type: Defect Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File aot-metadata.diff    

 Description   

With Clojurescript 0.0-1859:

user=> (binding [*compile-files* true] (require 'cljs.analyzer 'cljs.compiler))
nil
user=> (cljs.compiler/with-core-cljs)
AssertionError Assert failed: set! target must be a field or a symbol naming a var
targetexpr  cljs.analyzer/fn--1668 (analyzer.clj:593)


 Comments   
Comment by David Nolen [ 05/Oct/13 2:41 PM ]

Is there any more information about what exactly is required or expected? Or hints about what needs to be done to resolve this issue? Thanks!

Comment by Ambrose Bonnaire-Sergeant [ 04/Feb/14 9:02 AM ]

The root of this issue seems to be the call to `import-macros` at the top of cljs.core.

Under AOT compilation, the macros are imported as regular functions. This seems to be due to a weird property under AOT where attaching metadata to a var def must be done with alter-meta!, rather than by attaching metadata to the second argument of `def`.

This is a telling symptom:

user=> (binding [*compile-files* true] (require 'cljs.core))
nil
user=> (-> #'cljs.core/when meta :macro)
nil

The patch adds a redundant call to `alter-meta!`.

Behaviour before patch (https://github.com/clojure/clojurescript/commit/070b677a2192912d4f9e933f34c19055b571d101):

user=> (binding [*compile-files* true] (require 'cljs.core))
nil
user=> (require '[cljs [analyzer :as ana] [compiler :as comp] [env :as env]] :verbose)
...
nil
user=> (do (env/ensure (comp/with-core-cljs)) nil)
ExceptionInfo Can't recur here at line 158 file:/home/ambrose/.m2/repository/org/clojure/clojurescript/0.0-SNAPSHOT/clojurescript-0.0-SNAPSHOT.jar!/cljs/core.cljs  clojure.core/ex-info (core.clj:4327)

After patch:

user=> (binding [*compile-files* true] (require 'cljs.core))
nil
user=> (require '[cljs [analyzer :as ana] [compiler :as comp] [env :as env]] :verbose)
(clojure.core/in-ns 'user)
(clojure.core/alias 'ana 'cljs.analyzer)
(clojure.core/in-ns 'user)
(clojure.core/alias 'comp 'cljs.compiler)
(clojure.core/in-ns 'user)
(clojure.core/alias 'env 'cljs.env)
nil
user=> (do (env/ensure (comp/with-core-cljs)) nil)
nil
Comment by David Nolen [ 04/Feb/14 9:08 AM ]

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





[CLJS-759] Exceptions are reported at weird locations in the presence of bad quasiquoting Created: 31/Jan/14  Updated: 31/Jan/14

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

Type: Defect Priority: Minor
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

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



 Description   

(defn foo [x]
`(f ~@(for [i (range 10)] ~i)))

(foo 1)

This code has an extra unquote next to the i causing:

TypeError: Cannot call method 'call' of undefined
at aurora.compiler.foo.cljs.core.with_meta.call.cljs.core.seq.call.cljs.core.concat.call.iter_7602auto_ (/home/jamie/aurora/src/aurora/compiler.cljs[eval362]:229:100)

Compare this to:

(defn foo [x]
(bar x))

(foo 1)

Which causes:

TypeError: Cannot call method 'call' of undefined
at foo (/home/jamie/aurora/src/aurora/compiler.cljs[eval376]:212:67)

In the first case, the symbol is mangled and the line number and column are incorrect.






[CLJS-526] Automatically scan the classpath for non-goog JavaScript libraries Created: 19/Jun/13  Updated: 30/Jan/14  Resolved: 30/Jan/14

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

Type: Enhancement Priority: Trivial
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Right now, adding JavaScript dependencies to a ClojureScript project is doable, but more difficult than it should be. Even assuming the use of lein-cljsbuild, one must always add appropriate :libs and/or :foreign-libs and/or :externs options to each build invocation...not just once (e.g. when packaging a JavaScript library as part of a ClojureScript library), but in every situation where such dependencies are used.

Addressing this for foreign libs is probably not practical, but we should be able to extend the treatment of goog.* dependencies (embodied in cljs.closure/goog-dependencies*) to any Google Closure-ready JavaScript library/module. In short, this would look like:

1. At load-time in cljs.closure, scan the classpath for all .js files
2. Use parse-js-ns to obtain each JavaScript file's provides and requires.
3. Include the resulting maps in js-dependency-index.

The end result should be that, if a JavaScript file is on your classpath that contains e.g. goog.provide('foo.bar');, you can use it simply by adding (:require [foo.bar]) to an ns declaration, without touching any build configuration.

Sound sane? Not sure if it'll work or not, but if it does, it'd make packaging and use of JavaScript libraries from ClojureScript much easier IMO.



 Comments   
Comment by Chas Emerick [ 19/Jun/13 2:11 PM ]

This definitely works, and it's remarkably easy. All the pieces are already there (I hadn't grokked exactly what was going on with the existing classpath scanning for named :libs and such). You can see the behaviour that would apply right now by adding :libs [""] to your build config; the criteria being used now is that the string in :libs is the prefix of the classpath entries that are found and used automatically.

Comment by David Nolen [ 03/Jul/13 7:59 PM ]

I don't have strong opinions about this. This another ticket that needs some feedback from users. Perhaps ask people on the ClojureScript mailing list to try this out and find out if it addresses pain points? Thanks Chas.

Comment by Chas Emerick [ 03/Jul/13 9:32 PM ]

I'll see what I can do to drum up interest and such (though I get the distinct impression that very, very few people are actively trying to use JavaScript libraries as dependencies, as opposed to "simply" vendoring them into their apps).

Anyway, in the meantime, I downgraded the priority to 'trivial', since there's an easy workaround (as described in my previous comment and documented e.g. here).

Comment by Chas Emerick [ 30/Jan/14 8:30 AM ]

This is subsumed by CLJS-656, closing.





[CLJS-755] Expose Clojure's hash calculation helper API Created: 29/Jan/14  Updated: 29/Jan/14

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

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


 Description   

To aid in collection portability between Clojure implementations.






[CLJS-752] Implement specify! to enable extending instances that don't implement ICloneable Created: 28/Jan/14  Updated: 28/Jan/14  Resolved: 28/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Implementing specify! would enable instances of built-in objects (DOM objects, etc) to be extended with protocols without polluting the environment.



 Comments   
Comment by David Nolen [ 28/Jan/14 10:59 PM ]

fixed, https://github.com/clojure/clojurescript/commit/6e5896cb6cd3549d069473e86e424104c107223d





[CLJS-753] Too many open files exception Created: 28/Jan/14  Updated: 28/Jan/14  Resolved: 28/Jan/14

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

Type: Defect Priority: Blocker
Reporter: Roman Scherer Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: File too-many-open-files.diff    
Patch: Code

 Description   

This patch uses `io/reader` in conjunction with `with-open` to prevent
"too many open files" errors. This happens in a browser REPL when
evaluating a file that references a couple of other files multiple
times.

From the Google Group discussion at: https://groups.google.com/forum/#!topic/clojurescript/r2iGPh2Lv0U

----------------------------

Hello Clojure Scripters,

with my current REPL setup I get some really annoying "Too many
files open" errors. I'm not sure if it's a problem with
ClojureScript itself, Austin the browser REPL, nREPL or my own
miss-configuration of something.

When I connect to the browser REPL via AUSTIN and eval a whole
ClojureScript file the first time a lot of Ajax requests are sent
over the wire and my main namespace is getting compiled and
shipped to the browser. So far so good, my Java process is at
around 18676 open files. I don't care yet.

Compiling the same file again and again increases the open files

not sure if this is a problem with my setup, but I

18676, 19266, 22750, 21352, 33097, 62913, 64398, 64398, 64398,
64398, 64398 up to 171977, where some ulimit is reached and I get
an exception like this:

java.io.FileNotFoundException: .repl/5614/request/routes.js (Too many open files)

and my ClojureScript hangs up and I have to do a
cider-restart. Ok maybe I shouldn't eval whole files too often
over the XHR connection, but this seems not right.

I used the command "lsof -n | grep java | wc -l" to watch the
above numbers while evaluating the file again and again.

Does someone had a similar problem, knows how to solve that, or
has any ideas how to track this one down?

Thanks for your help, Roman.



 Comments   
Comment by Roman Scherer [ 28/Jan/14 5:56 PM ]

Hi David, can you review this. Thanks.

Comment by David Nolen [ 28/Jan/14 6:19 PM ]

Looks good to me will apply soon and test.

Comment by David Nolen [ 28/Jan/14 10:51 PM ]

fixed, https://github.com/clojure/clojurescript/commit/905c64445faa1c60e21b66fb226982759b0d4001





[CLJS-710] port clojure.pprint Created: 02/Dec/13  Updated: 28/Jan/14

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

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





[CLJS-731] clojure.walk/walk does not descend into js-arrays or js-objects Created: 21/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

r2127



 Description   

The clojure.walk/walk function does not descend into plain javascript objects and arrays--it'd be nice if it did using the same semantics as other collections.

I needed this functionality and ended up creating my own walk function. With a small patch I could add this to the native clojure.walk/walk, but I wanted to test interest here first.

Possibly the right thing to do instead is to make js-obj and array seqable?



 Comments   
Comment by David Nolen [ 23/Dec/13 1:40 PM ]

I'd rather see ClojureScript's walk be protocol-ized so that people can always solve this problem themselves.

Comment by David Nolen [ 23/Jan/14 8:08 AM ]

Closing.





[CLJS-734] Add multi-arg versions of conj! disj! assoc! and dissoc! Created: 26/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

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

Attachments: Text File bench-cljs-734.txt     File benchmark_runner.cljs     Text File bench-master.txt     Text File bench-variadic.txt     Text File cljs-734-2.patch    
Patch: Code

 Description   

The transient collection manipulation functions conj! disj! assoc! and dissoc! don't have an "& rest" argument form, so it is not possible to write for example (conj! t-vec :a :b).

Besides being inconvenient, this is also different from Clojure.

Patch attached.



 Comments   
Comment by David Nolen [ 26/Dec/13 12:47 PM ]

Please include a properly formatted squashed patch that includes tests -https://github.com/clojure/clojurescript/wiki/Patches. Thanks!

Comment by Francis Avila [ 26/Dec/13 4:29 PM ]

Patch amended, tests added.

Comment by Jozef Wagner [ 28/Dec/13 1:36 PM ]

Let's step back a little. Transient variants of conj, dissoc, etc. were created just for performance, not convenience. Clojure version of conj! DOES NOT support multi arg version and IMO it should not, as the transient variants should do one thing and do it fast, and not fiddle with seqs and deconstruct... I would go as far as to propose to remove multi arg versions from Clojure, rather than adding them in CLJS.

Comment by Francis Avila [ 28/Dec/13 4:39 PM ]

I didn't realize conj! wasn't variadic in Clojure--assoc! dissoc! and disj! are though. I would rather patch Clojure's conj! to add a variadic form.

I think we should support variadic forms of these functions because their non-transient counterparts do. Code typically starts out non-transient and then becomes transient later. If conj/conj! assoc/assoc! etc support the same function signatures then getting a working transient version is trivial--wrap the initial structure with transient, the result with persistent!, and add bangs to your conj/disj/assoc/dissoc names. It's an unnecessary annoyance to not have a variadic form in this case. This is precisely the annoying scenario that I've encountered enough times to drive me to write this patch.

We lose nothing by having a variadic form--if you want to avoid messing with seqs in your transient code you still can by calling the non-variadic form. But if you're using transients directly you're almost certainly have a seq somewhere in your code path anyway, so what's the harm in letting conj!/disj! etc consume it from the inside instead of forcing the user to do it on the outside.

Comment by David Nolen [ 30/Dec/13 8:25 AM ]

It would be nice to see actual performance numbers. If the performance difference is minor under V8 and JavaScriptCore I'm inclined to merge in this simple enhancement and consistency patch.

Comment by Francis Avila [ 04/Jan/14 2:02 AM ]
  • Patch updated:
    • Made variadic assoc! faster
    • rebased to latest master.
  • Wrote a benchmark, benchmark_runner.cljs, which I run using script/benchmark. Results on my machine attached: linux with x64 v8 and spidermonkey 17 (no JSCore benches).
    • On V8, there is no difference between master and my patch for non-variadic calls.
    • On spidermonkey, non-variadic vec assoc!, set disj!, and map dissoc! are slower with the patch; the rest are the same. I can't explain why!
    • The variadic calls (using apply) are in some cases faster than loop/recur with non-variadic calls, which I also can't understand.
Comment by David Nolen [ 07/Jan/14 7:08 AM ]

Can we remove the old patches thanks.

Comment by Francis Avila [ 07/Jan/14 8:17 AM ]

Actually I don't think I can, or at least I see no way to remove them in JIRA. The latest patch is cljs-734-2.patch

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

K thanks, I've cleaned up the stale ones.

Comment by David Nolen [ 23/Jan/14 8:08 AM ]

fixed, https://github.com/clojure/clojurescript/commit/8b2ed1d57b8904341727d4b5b251c8815e618a10





[CLJS-722] Support parse.com's Cloud Code environment in :nodejs builds Created: 09/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Travis Vachon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs

Attachments: Text File cljs_722.patch     Text File cljs_722v2.patch     Text File cljs_722v3.patch     Text File parse.patch    
Patch: Code

 Description   

parse.com's Cloud Code is very similar to node.js but expects no hashbang and doesn't provide util.js, so provide a compiler argument to elide the header and wrap the util.js require in a try...catch



 Comments   
Comment by David Nolen [ 10/Dec/13 9:21 AM ]

Please format the patch so it can be applied with git am. See http://github.com/clojure/clojurescript/wiki/Patches

Comment by Travis Vachon [ 10/Dec/13 9:47 AM ]

git am-able patch

Comment by Travis Vachon [ 10/Dec/13 9:47 AM ]

sorry about that! will update all the tickets I've filed recently as well

Comment by Travis Vachon [ 10/Dec/13 1:22 PM ]

so actually, I think a better solution to this will be to add support for :preamble as proposed here:

https://groups.google.com/forum/#!searchin/clojurescript/preamble/clojurescript/rUIlLfcYZvE/Yskfh4znL_0J

I'm imagining that the default preamble for the :nodejs target will be the hashbang

that will let us a) redefine the require function in our compiled output to squelch the util import and b) avoid writing the hashbang

Comment by David Nolen [ 11/Dec/13 10:52 AM ]

so is this ticket superseded by CLJS-723?

Comment by Travis Vachon [ 11/Dec/13 12:02 PM ]

yeah - if I can write arbitrary js at the beginning of my compiled file I don't need this patch. Can close.

Comment by Travis Vachon [ 12/Dec/13 8:22 AM ]

arg, it looks like I spoke too soon - redefining require in the cloud code env appears to be impossible. I spent a couple hours trying every scoping trick I could find - it looks like the only way to redefine require is by defining a function (assignment just doesn't seem to take) and that function is always hoisted to the very top, so there's no way to get a handle on the original require function. Parse's sandbox doesn't seem to have another way to reference require, either.

How would you feel about moving {{ (set! cljs.core/string-print (.-print (require "util"))) }} into a function node.js people can call rather than doing it when the module is loaded?

Comment by David Nolen [ 12/Dec/13 8:38 AM ]

Honestly I'd be happy to remove it altogether and let people just handle this themselves. Or allow people to disable it if they are using the nodejs target option.

Comment by Travis Vachon [ 12/Dec/13 10:07 AM ]

ok great. I'm inclined to leave it in a function because it feels sort of non-obvious to me - would be nice to help people avoid reinventing in every codebase. I'll attach a new patch.

Comment by Travis Vachon [ 12/Dec/13 10:53 AM ]

add new patch that just moves the problematic require into a function. the hashbang issue the original patch addressed is better fixed with the preamble work in http://dev.clojure.org/jira/browse/CLJS-723

Comment by David Nolen [ 16/Jan/14 5:33 PM ]

Isn't this a breaking change for existing Node.js users?

Comment by Travis Vachon [ 22/Jan/14 2:00 PM ]

it is yeah, I assumed that was ok because you said you'd be happy to remove it altogether - I'm ok with removing it if you think that's a preferable breaking change.

Comment by David Nolen [ 22/Jan/14 2:11 PM ]

Lets rename it to enable-util-print! to match enable-console-print!.

Comment by Travis Vachon [ 22/Jan/14 2:13 PM ]

cool! will do. I'll update the patch today.

Comment by Travis Vachon [ 22/Jan/14 7:22 PM ]

rename function to enable-util-print!

Comment by David Nolen [ 23/Jan/14 7:52 AM ]

fixed, https://github.com/clojure/clojurescript/commit/b299a6b1e9a522de0b1c3345f54f164bc3524f8f





[CLJS-736] Functions folder and reducer broken for types nil and array + fix for typo Created: 29/Dec/13  Updated: 18/Jan/14

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

Type: Defect Priority: Major
Reporter: Jonas De Vuyst Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-736-alt.patch     Text File CLJS-736.patch     Text File CLJS-736-patch-1-redux.patch     Text File CLJS-alt-satisfies.patch    
Patch: Code and Test

 Description   

1. This currently doesn't work:

(->> nil
(r/map identity)
(r/reduce + 0))
; org.mozilla.javascript.JavaScriptException: Error: No protocol method IReduce.-reduce defined for type null

The reason for this is that reducers created by r/reducer or r/folder, invoke -reduce (of IReduce) directly. They thereby bypass the special case for nil in the function r/reduce.

2. An entirely analogous problem exists for collections of type array.

3. The patch CLJS-700 mistakenly defined coll-fold for the type cljs.core/IPersistentVector. This should have been cljs.core/PersistentVector. (There exists no protocol IPersistentVector in ClojureScript.)

I will shortly attach a patch that addresses all of the above problems by implementing IReduce for nil and array. The patch also includes unit tests.



 Comments   
Comment by Jonas De Vuyst [ 29/Dec/13 2:22 PM ]

Alternative patch in which r/reduce and r/fold treat arrays and nil as special cases – as opposed to having arrays and nil implement IReduce and CollFold.

The functions r/reducer, r/folder, and the protocol methods of r/Cat now call r/reduce and r/fold instead of calling -reduce and coll-fold directly.

This patch also fixes a bug in the coll-fold implementation for Cat, which previously used (reducef) as the initial value rather than (combinef). The new code is copied and pasted from the Clojure implementation and uses the fork-join stubs.

Comment by David Nolen [ 30/Dec/13 8:23 AM ]

The implements? should probably be a satisfies? in the second patch. Have you run any benchmarks of before/after the patch?

Comment by Jonas De Vuyst [ 30/Dec/13 11:24 AM ]

If I understand correctly then (satisfies? x y) is roughly equivalent to (or (implements? x y) (natively-satisfies? x y)).

If native types (nil, array, object currently) are treated as special cases then implements? seems more appropriate.

satisfies? works also, however, so I have attached a new 'alt' patch.

Comment by Jonas De Vuyst [ 30/Dec/13 11:26 AM ]

The first patch is in fact faster when running the following code:

(time (->> (repeat 1000 (vec (range 1000)))
vec
(r/mapcat identity)
(r/map inc)
(r/filter even?)
(r/fold +)))

This takes about 700 msecs. Using the first patch this terminates 100-300 msecs faster. This is after repeated (but informal) testing.

I guess the worry is that the first patch would slow down other random code since it involves extending the types nil, array, and object. I'm not sure what exactly I should test for though.

(Note that the 2nd and 3rd patch also contain a fix for Cat and include more unit tests. The first patch should preferably not be applied as-is.)

Comment by David Nolen [ 30/Dec/13 11:35 AM ]

Yeah you're timing too many things, including vec, range, lazy sequences. Also testing a small N. Take a look at the reducers example on the Mori README - https://github.com/swannodette/mori. Thanks.

Comment by Jonas De Vuyst [ 30/Dec/13 12:52 PM ]

I tried running the following code:

(let [coll (vec (repeat 1000 (vec (range 10))))]
  (time (doseq [n (range 1000)]
               (->> coll
                    (r/mapcat identity)
                    (r/map inc)
                    (r/filter even?)
                    (r/fold +)))))

Some of the last results I got were:

1st patch: 75680 msecs
2nd patch: 76585 msecs

Truth be told, although the first patch seemed to win most of the times, sometimes the second patch was faster.

One other thing I tried was removing the implements?/satisfies? check from the second patch and overriding the protocol method coll-fold for the type object instead (as in the first patch). This 'hybrid' approach generally (but not always) seemed to result in a slowdown.

I'm not sure how I should proceed. Should I perhaps just run both patches simultaneously for several minutes?

Comment by