<< 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: 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-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-685] Cannot call method 'fromArray' of undefined -- Clojurescript 0.0-2030 Created: 17/Nov/13  Updated: 26/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Minor
Reporter: John Chijioke Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler, bug, errormsgs
Environment:

Linux 3.2.0-52-generic x86_64 GNU/Linux, java 1.7, clojure 1.5.1



 Description   

Clojurescript 0.0-2030

This line from compile cljs.core is causing problems:

cljs.core.PersistentQueue.EMPTY = (new cljs.core.PersistentQueue(null, 0, null, cljs.core.with_meta(cljs.core.PersistentVector.EMPTY, cljs.core.PersistentArrayMap.fromArray([new cljs.core.Keyword(null, "end-line", "end-line", 2693041432), 3820, new cljs.core.Keyword(null, "end-column", "end-column", 3799845882), 69], true)), 0));

error message: Uncaught TypeError: Cannot call method 'fromArray' of undefined.

That's the first mention of fromArray in that file. I don't know if it's an ordering problem.



 Comments   
Comment by John Chijioke [ 17/Nov/13 11:10 PM ]

I solved it by replacing [] with cljs.core.PersistentVector.EMPTY. I think this must be a reader problem.

Comment by David Nolen [ 17/Nov/13 11:32 PM ]

This ticket needs more details, how can this error be reproduced?

Comment by Peter Taoussanis [ 22/Nov/13 3:03 AM ]

Hi, I'm seeing the same problem with tools.reader 0.8.0.

Any Clojurescript file (even an empty file) will produce the error.

Clojure: 1.6.0-alpha2
Clojurescript: 0.0-2030
Cljsbuild: 1.0.0
tools.reader: 0.8.0

Tried `lein cljsbuild clean`.

Problem is resolved by dropping back to tools.reader 0.7.10.

Update: have created an issue on the tools.reader GitHub page: https://github.com/clojure/tools.reader/issues/7

Update 2: this isn't something specific to Cljs 0.0-2030 btw, tools.reader 0.8.0 seems to produce the same error against at least Cljs 0.0-2060, 0.0-2027, 0.0-2024.

Comment by Nicola Mometto [ 22/Nov/13 6:49 AM ]

tools.reader 0.8.0 introduces end-column/end-line metadata, this needs to be elided as per line/column to avoid this bootstrapping issue.

Comment by David Nolen [ 22/Nov/13 8:02 AM ]

fixed, http://github.com/clojure/clojurescript/commit/36d401797f85c99794eef8a71239641930c36871

Comment by Peter Taoussanis [ 22/Nov/13 10:30 AM ]

Thanks a lot David, Nicola - much appreciated! Cheers

Comment by John Chijioke [ 26/Nov/13 6:32 AM ]

Thanks David. Cheers!





[CLJS-639] Produce errors when records are initialized incorrectly Created: 27/Oct/13  Updated: 19/Nov/13  Resolved: 19/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Scott Feeney Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs


 Description   

There are a couple of wrong ways you can use a record initializer that don't produce errors in ClojureScript, just nils:

ClojureScript:cljs.user> (defrecord Thing [foo bar])
cljs.user/Thing
ClojureScript:cljs.user> (Thing. 1 2)  ; correct usage
#cljs.user.Thing{:foo 1, :bar 2}
ClojureScript:cljs.user> (Thing. 1)    ; wrong number of fields
#cljs.user.Thing{:foo 1, :bar nil}
ClojureScript:cljs.user> (Thing 1 2)   ; forgetting the dot
nil

Compare Clojure:

user=> (defrecord Thing [foo bar])
user.Thing
user=> (Thing. 1 2)
#user.Thing{:foo 1, :bar 2}
user=> (Thing. 1)

CompilerException java.lang.IllegalArgumentException: No matching ctor found for class user.Thing, compiling:(/tmp/form-init7089728177345913731.clj:1:1) 
user=> (Thing 1 2)

RuntimeException Expecting var, but Thing is mapped to class user.Thing  clojure.lang.Util.runtimeException (Util.java:219)

It would make debugging easier if ClojureScript followed Clojure's example here and gave a useful error immediately in case of the last 2 examples.



 Comments   
Comment by David Nolen [ 29/Oct/13 6:40 PM ]

Thanks for the report, these warnings would indeed be nice.

Comment by David Nolen [ 19/Nov/13 4:41 PM ]

fixed, http://github.com/clojure/clojurescript/commit/8c6dd2468a3913e316d021fc0b09745bd3ac7dcd

Comment by David Nolen [ 19/Nov/13 4:43 PM ]

Wrong arity constructor issue fixed. Separate ticket need for invoking ClojureScript ctors as functions.





Generated at Tue Sep 30 13:13:08 CDT 2014 using JIRA 4.4#649-r158309.