<< Back to previous view

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

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

Type: Defect Priority: Minor
Reporter: Alex Coventry Assignee: Michał Marczyk
Resolution: Unresolved 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    

 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.





[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-787] cljs.reader does not read blank string as nil Created: 20/Mar/14  Updated: 20/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

Attachments: File CLJS-787.diff    

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

Several tests fail when I apply this patch.





[CLJS-101] Node fails with :whitespace, works with :simple Created: 17/Nov/11  Updated: 19/Apr/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





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

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

Type: Defect Priority: Major
Reporter: Shahar Assignee: Unassigned
Resolution: Unresolved 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: {}





[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-798] regression: upstream dependencies are no longer honored 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: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved 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.





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






Generated at Thu Apr 24 09:50:51 CDT 2014 using JIRA 4.4#649-r158309.