<< Back to previous view

[CLJ-1524] SeqIterator constructor change broke binary compatibility in 1.7.0-alpha2 Created: 09/Sep/14  Updated: 09/Sep/14  Resolved: 09/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1524.diff    
Patch: Code
Approval: Ok

 Description   

Running code AOT-compiled against Clojure 1.6.0 (or older) with 1.7.0-alpha2 runtime will encounter this error with SeqIterator:

CompilerException java.lang.NoSuchMethodError: clojure.lang.SeqIterator.<init>(Lclojure/lang/ISeq;)V, compiling:(form-init5913779045640355531.clj:1:11)

Cause: This is due to a type change in the constructor of SeqIterator from ISeq to Object (commit: https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c ).

Proposed: Add the ISeq constructor back so that calls into that constructor retain backwards binary compatibility.

Patch: clj-1524.diff

Screened by:

More: From Datomic mailing list - https://groups.google.com/forum/#!topic/datomic/KZqhY6hUHz0



 Comments   
Comment by Alex Miller [ 09/Sep/14 11:06 AM ]

Patch not applied, but similar change applied directly here:

https://github.com/clojure/clojure/commit/ba41f25b6f3f32729c55f7f7ceb179be597acf94





[CLJ-1518] Patch for removing transient thread owner check broke rrb-vector Created: 03/Sep/14  Updated: 04/Sep/14  Resolved: 04/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1518.diff    
Patch: Code
Approval: Ok

 Description   

The patch for CLJ-1498 changed the field types inside the persistent data structures, which inadvertently broke core.rrb-vector, which relies on reusing some of those internals. It is not necessary to change the type to satisfy the patch, so we would like to rollback that aspect of the change to minimize breakage.



 Comments   
Comment by Alex Miller [ 03/Sep/14 2:05 PM ]

In the patch I rolled back the changes in the Persistent*.java from CLJ-1498 and re-applied. The only "real" changes after the rollback are in ensureEditable(). Tests were left of course.





[CLJ-1498] Remove birth-thread check from transients Created: 08/Aug/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: collections, transient

Attachments: File clj-1498-2.diff     File clj-1498-3.diff     File clj-1498.diff    
Patch: Code
Approval: Ok

 Description   

Transients protect themselves from use by any thread other than the one that creates them. This is good for safety, however it eliminates certain valid usages of transients. For example, usage in a go-block might occur in subsequent invocations across multiple OS threads (but only one logical thread of control).

Current simple test:

user> (def v (transient []))
#'user/v
user> (persistent! @(future (conj! v 1)))
IllegalAccessError Transient used by non-owner thread  clojure.lang.PersistentVector$TransientVector.ensureEditable (PersistentVector.java:464)

Proposal: Remove the owner check from transient collections. (Leave the edit after persistent check as is.) The test above should succeed.

After:

user=> (def v (transient []))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

The clj-1498-3.diff version of the patch also replaces the AtomicReference<Thread> with AtomicBoolean as we can now track just ownership, not who owns it.

Doc update: Various pieces of documentation will need to be updated with this change, namely http://clojure.org/transients

Patch: clj-1498-3.diff

Alternative: Another idea would be to make this check optional with some kind of option on the transient call (transient coll :check-owner true). Not sure whether what the default would be for that.



 Comments   
Comment by Jozef Wagner [ 09/Aug/14 7:08 AM ]

I suggest to add a functionality to pass ownership of a transient to the different thread, or to release the ownership by passing nil.

user=> (def v (pass! (transient []) nil))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

pass! has to be called by current owner thread, or by any thread if the transient is currently released.

Comment by Alex Miller [ 13/Aug/14 1:42 PM ]

New patch that replaces AtomicReference<Thread> with AtomicBoolean.

Comment by Stuart Halloway [ 19/Aug/14 11:05 AM ]

Alex, can you please expand the example test you provided to a generative test that covers the following combinations:

  1. different collection sizes (above and below the ArrayMap size boundary)
  2. different shapes (vector vs. map)
  3. successful use across threads (positive use case this ticket enables)

data_structures.clj has helpers for generating transient interactions that you can build on.

Comment by Alex Miller [ 20/Aug/14 8:59 AM ]

Enhanced existing generative tests to test random actions against sets, vectors, and both PHM and PAM. Added additional actions to do transient modification actions in other threads as well as originating thread.





[CLJ-1497] sequence with transducers realizes n+2 elements Created: 08/Aug/14  Updated: 08/Aug/14  Resolved: 08/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

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

Attachments: File clj-1497.diff     File clj-1497v2.diff    
Approval: Ok

 Description   

The first element is realized at creation time:

user=> (def a (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
#'user/a

Fully realizing the sequence realizes the other n-1 elements, and 2 more:

user=> a
(~2
~3
1 ~4
2)

Compare with version using seq operations:

user=> (def a (sequence (take 2 (map #(do (println (str "~" %)) %) (iterate inc 1)))))
#'user/a
user=> a
(~1
~2
1 2)

Transduce also doesn't seem to exhibit this issue:

user=> (def a (transduce (take 2) conj [] (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
~2
#'user/a
user=> a
[1 2]


 Comments   
Comment by Alex Miller [ 08/Aug/14 10:02 AM ]

Patch attached that improves the issue - will now only realize n+1 elements.

Comment by Nicola Mometto [ 08/Aug/14 10:16 AM ]

Nice, I added a commit on top of yours to delay the realization of the first element of the lazyseq to the first .next call instead of on SeqIteration creation

Comment by Alex Miller [ 08/Aug/14 11:12 AM ]

Fixed by Rich directly, not by patch.





[CLJ-1494] remove flatmap in favor of mapcat Created: 07/Aug/14  Updated: 03/Sep/14  Resolved: 03/Sep/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-remove-flatmap-use-1-arity-mapcat-instead.patch    
Patch: Code

 Description   

While all the transducers functions are implemented as an arity in the matching clojure core sequence, for mapcat a new function has been added: flatmap.
The reason for this is, as Rich said in a HN comment, "because mapcat's signature was not amenable to the additional arity".
This patch changes the mapcat signature to take at least one collection so that it's possible to add the 1-arity for the transducer function, eliminating the need for a different function, flatmap.

There has been no loss by removing the 1-arity version of mapcat as a sequence function since trying to use (mapcat f) as currently defined (not as redefined with this patch) would fail before transducers, and after transducers:
Before transducers (mapcat f) would result in a call to (map f) which would fail with an ArityException
After transducers that (map f) call would return a function, which then would be used as an argument to (apply concat the-f), resulting in a IllegalArgumentException since apply expects a sequence but it's been given a fn.



 Comments   
Comment by Alex Miller [ 03/Sep/14 11:02 AM ]

Done as a result of https://github.com/clojure/clojure/commit/7d84a9f6f35a503cddf98487b6544d18937c669e





[CLJ-1430] Improve performance of partial Created: 23/May/14  Updated: 05/Sep/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: performance

Attachments: File partial-perf.diff    
Patch: Code
Approval: Ok

 Description   

This patch improves performance of partial by only using apply when needed. The code structure follows that of juxt.

Performance benchmark:

(ns partial-test.core
  (:require [criterium.core :refer [bench]])
  (:gen-class))

(defn -main []
  (let [f (partial + 1 1)]
    (println "Starting")
    (bench (f 1 1))
    (println "Done")))

Results for 1.6.0:

Evaluation count : 228751140 in 60 samples of 3812519 calls.
             Execution time mean : 266.700063 ns
    Execution time std-deviation : 2.966851 ns
   Execution time lower quantile : 262.641023 ns ( 2.5%)
   Execution time upper quantile : 274.207916 ns (97.5%)
                   Overhead used : 1.610513 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

Results for 1.7.0 with this patch:

 Evaluation count : 348208140 in 60 samples of 5803469 calls.
              Execution time mean : 171.210533 ns
     Execution time std-deviation : 2.011660 ns
    Execution time lower quantile : 168.819526 ns ( 2.5%)
    Execution time upper quantile : 176.015584 ns (97.5%)
                    Overhead used : 2.644128 ns

 Found 3 outliers in 60 samples (5.0000 %)
 	low-severe	 3 (5.0000 %)
  Variance from outliers : 1.6389 % Variance is slightly inflated by outliers

Benchmarks performed via lein uberjar + running via the commandline.

Patch: partial-perf.diff

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 23/May/14 10:46 AM ]

Screened, looks as expected.

Comment by Andy Fingerhut [ 02/Jun/14 10:50 AM ]

Timothy, just a nit that I would not have noticed except for my program that checks for name and email address of patch authors, to see if they are on my contributor's list, but do you really have both of the email addresses tbaldridge@gmail.com and tbaldidge@gmail.com (note the spelling difference)? The latter is the one on this patch.

Comment by Timothy Baldridge [ 02/Jun/14 11:04 AM ]

fixed email

Comment by Timothy Baldridge [ 02/Jun/14 11:05 AM ]

nice catch! it was a typeo in my .gitconfig defaults. I've fixed the patch.

Comment by Alex Miller [ 02/Jun/14 11:19 AM ]

Tim (and anyone really) - please let someone know if you need to change a screened patch! Looks fine here, but screener should be notified so they can re-screen.

Comment by Alex Baranosky [ 05/Sep/14 9:11 PM ]

Very nice patch. I've gotten into the habit of not using partial anymore for performance sensitive code. Perhaps this change means I need to rethink that.





[CLJ-1429] Cache unknown multimethod value default dispatch Created: 22/May/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: performance

Attachments: Text File clj-1429.patch    
Patch: Code
Approval: Ok

 Description   

Multimethods maintain a cache from dispatch value (result of the dispatch function) to dispatch method. If the dispatch value does not find a match in the available methods, it falls through to a lookup using the default dispatch value and returns that method. This default dispatch case is NOT recorded in the cache. This means that every case that falls through to the default case incurs a scan of the methodTable (and the class inheritance checks that involves).

Perf test:

(defmulti mm class)
(defmethod mm String [s] s)
(defmethod mm Long [l] l)
(defmethod mm :default [v] v)

(defn perf [reps size]
  (let [data (take size (cycle ["abc" 5 :k]))]
    (dotimes [_ reps]
      (time (doall (map mm data))))))

And results:

;; Without patch:
user=> (perf 5 100000)
"Elapsed time: 1301.262 msecs"
"Elapsed time: 928.888 msecs"
"Elapsed time: 942.905 msecs"
"Elapsed time: 858.513 msecs"
"Elapsed time: 832.314 msecs"

;; With patch:
user=> (perf 5 100000)
"Elapsed time: 134.169 msecs"
"Elapsed time: 28.859 msecs"
"Elapsed time: 45.452 msecs"
"Elapsed time: 13.189 msecs"
"Elapsed time: 13.42 msecs"

Attached patch caches the mapping of unknown value -> default dispatch method and significantly improves the performance for this case.

Patch: clj-1429.patch
Screened by: Stu






[CLJ-1388] equality bug on records created with nested calls to map->record Created: 18/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: Text File 0001-FIX-CLJ-1388.patch     Text File CLJ-1388.patch     Text File CLJ-1388-record-equality-and-map-record-factory.patch     Text File CLJ-1388v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

Depending on the type of the map passed to a record map constructor, records will not correctly compare for equality:

user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)  ;; expected => {:a 1}
#user.a{:a 1}

Cause: The type of the map passed into the map constructor leaks into the __extmap, affecting equality comparison of the record. This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: CLJ-1388-record-equality-and-map-record-factory.patch

Screened by: Alex Miller



 Comments   
Comment by Steve Miner [ 28/Apr/14 3:06 PM ]

The proposed patch 0001-FIX-CLJ-1388.patch makes every map->Record method pay to copy the argument map every time. However, according to my tests, the problem only occurs with records without any fields. So it should be sufficient to generate the (into {} m#) case only when `fields` is empty. [Update: this is wrong, explained below.]

Comment by Steve Miner [ 28/Apr/14 3:10 PM ]

It would be better to fix the problem in the Java Record/create method, but I couldn't figure out how that worked. On the other hand, this bug seems like a fairly rare edge case so I think my patch is acceptable.

Comment by Alex Miller [ 28/Apr/14 3:23 PM ]

Moving out of Screened due to new patch

Comment by Nicola Mometto [ 28/Apr/14 3:35 PM ]

Steve, the problem doesn't occur with records without any fields, your testing was reporting that only because you are only using one record type.

Here's an example that returns true with my patch, but still returns false with yours.

user=> (defrecord a [a])
user.a
user=> (defrecord b [b])
user.b
user=> (def x1 (map->a {:a 1 :b 2}))
#'user/x1
user=> (def x2 (map->a (map->b {:a 1 :b 2})))
#'user/x2
user=> x1
#user.a{:a 1, :b 2}
user=> x2
#user.a{:a 1, :b 2}
user=> (= x1 x2)
false
user=> (.__extmap x2)
#user.b{:b 2}
Comment by Nicola Mometto [ 28/Apr/14 3:37 PM ]

It should also be noted that the overhead of copying the record map is probably insignificant.

Comment by Nicola Mometto [ 28/Apr/14 3:42 PM ]

I also thought at first to fix the problem either on the /create method or on the 3-arity ctor but given that:

  • a fix there would involve messing with the bytecode emitted and thus would be harder to implement than this simple 1-line patch
  • neither the /create method nor the 3-arity ctor is documented and thus should be considered implementation details

I think patching the map->record function is the best way to go.

Comment by Steve Miner [ 28/Apr/14 3:56 PM ]

Nicola, thanks for the correction. I missed the case with multiple records. I withdrew my patch. I'd still like to find a more finely tuned patch, but first I'll have to improve my tests as you demonstrated.

Comment by Steve Miner [ 29/Apr/14 10:17 AM ]

Attached CLJ-1388-record-equality-and-map-record-factory.patch that checks arg to map->Record for MapEquivalence, uses (into {} m#) when necessary. This makes equiv test work correctly with records as the argument (and other map-like values). Added tests with variety of args to map->Record.

Comment by Steve Miner [ 29/Apr/14 10:46 AM ]

A few comments about the new patch... I think the basic issue is a bad interaction between = for records and the generated Record/create method. Everything works when the interal __extmap is a regular map (MapEquivalence), but it fails if __extmap is another record. I think that's because of equiv calling = on the __extmap's.

The user expects to create a new record using the value of another record because it's just like a map. However, = on records respects the record type so it's not = to a map.

The general work-around is to use (into {} x) on the argument to the map->Record. To meet the user's expectation, that `into` call can be incorporated into the map->Record. But I didn't like the defensive copy as most of the time it's unnecessary – the argument is typically a regular map. The `into` work-around is only necessary if the arg is not a MapEquivalence.

There might be a better way to fix the Record/create method but I couldn't figure it out.

Comment by Nicola Mometto [ 29/Apr/14 1:52 PM ]

Steve's last comment made me realize that the root of the problem is on the record .equiv method, where the extmaps are compared via `=`

This new patch (CLJ-1388.patch) addresses this issue by comparing the extmaps with Utils/equiv rather than `=`, which compares maps in a type-indipendent way.

There's still a case where we need recreate the given map, that is when the given map is not an IPersistentMap but simply a java.util.Map.

Steve, my new patch incorporates my fix and your tests, I modified your patch to include only the tests (that were really comprehensive) since I figured it's fair to keep your authorship on those, let me know if that's a problem with you.

Comment by Steve Miner [ 29/Apr/14 2:10 PM ]

Whatever works for you regarding the tests is fine by me.

Comment by Alex Miller [ 30/Apr/14 12:07 AM ]

It seems weird to me that a record should ever contain another record as its extmap. We should be considering the performance aspect but I'm concerned that not locking down extmap more just invites other weirder problems later.

In CLJ-1388.patch, you mention Utils/equiv in your comment but the patch calls Utils/equals - which did you mean?

Also, that patch currently checks if m# is an IPersistentMap - I can't imagine what case we would want to allow where a valid m# is NOT an IPersistentMap?

Comment by Nicola Mometto [ 30/Apr/14 4:15 AM ]

Alex, the Utils/equiv in my comment is wrong (it's easy to confuse between equiv/equals, sorry), Utils/equals in the patch is the right method to use since it compares in a type agnostic way.

Since __extmap is an implementation detail and is only used internally by defrecord for its methods, I don't think it's going to be a problem whether it's a record or a regular clojure map. (Clojure only requires it to be an IPersistentMap)

Regarding the check for m# being an IPersistentMap, Steve in his tests had a case where the map->record ctor was invoked with a java.util.Map, I went to look into the docs for defrecord and it only mentions that the argument to map->record has to be a "map", it doesn't specify that it has to be a clojure map/IPersistentMap, so it seemed right to allow for java maps too and wrap them in IPersistentMaps internally.

Comment by Steve Miner [ 30/Apr/14 8:27 AM ]

My test with java.util.Map was an extension of the idea that anything map-like could be used to initialize a record. That might be a bridge too far, but my patch was testing for MapEquivalence to handle records so it made sense to allow j.u.Map, etc. With Nicola's latest patch, it's probably unnecessary to support non-IPersistentMaps so map->Record doesn't actually need to change.

Comment by Nicola Mometto [ 30/Apr/14 3:57 PM ]

CLJ-1388v2.patch is like CLJ-1388.patch except it doesn't copy non IPersistentMaps in a clojure map.

To summarize, here's the status of the different patches for this ticket:

  • 0001-FIX-CLJ-1388.patch copies the argument of map->record in a clojure map via `(into {} m#)`, be it already a clojure map, a record, or a java.util.Map
  • CLJ-1388-record-equality-and-map-record-factory.patch adopts the same approach except it only copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.MapEquivalence
  • CLJ-1388.patch fixes the issue by changing the function that compares __extmaps from `=` (type aware) to `clojure.lang.Utils/equals` (type agnostic), this patch also copies the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersistentMap
  • CLJ-1388v2.patch is the same as CLJ-1388.patch except it doesn't copy the arg of map->record into a clojure map if the arg doesn't satisfy clojure.lang.IPersitentMap, thus map->record will not work with bare java.util.Maps (which is the behaviour it has already)
Comment by Alex Miller [ 05/May/14 1:59 PM ]

Are these patches all still in play? Having 4 active patches does not help move a ticket forward.

Can someone re-summarize at this point what questions exist?

Comment by Nicola Mometto [ 06/May/14 5:26 AM ]

0001-FIX-CLJ-1388.patch should be superseded by the other 3 patches since they solve the same problem in a more performant way.

To pick between the other patches, we need to chose which approach to go with.
Patches CLJ-1388.patch and CLJ-1388v2.patch fix the issue in the equiv method of the defrecord, patch CLJ-1388-record-equality-and-map-record-factory.patch fixes the issue in the map->record ctor by converting maps that don't implement MapEquivalence to a clojure map.

I'd go with either CLJ-1388.patch or CLJ-1388v2.patch since they both avoid copying alltoghether in the cases where map->record currently works, while CLJ-1388-record-equality-and-map-record-factory.patch needs to copy the arg into a map if the arg is a custom IPersistentMap or a record.

To pick between CLJ-1388.patch or CLJ-1388v2.patch we need to decide whether or not the current behaviour of map->record to require strictly an IPersistentMap is the way to go: if we decide that it's ok to pass non IPersitentMap maps like java.util.Map to map->record then pick CLJ-1388.patch otherwise CLJ-1388v2.patch

Comment by Alex Miller [ 06/May/14 10:22 AM ]

From brief conversation with Rich, we should not allow arbitrary map types in __extmap so would prefer to force a clean map and rely on standard equality checking. I think CLJ-1388-record-equality-and-map-record-factory.patch is the preferred path based on that, which still seems like it should avoid copying in nearly all common cases.

Comment by Alex Miller [ 23/May/14 11:19 AM ]

Screened specifically CLJ-1388-record-equality-and-map-record-factory.patch - use map as is if it supports MapEquivalence (and can thus be compared under a map) and otherwise dump into a clojure map.





[CLJ-1387] reduce-kv on hash map ignores reduced objects in large maps Created: 18/Mar/14  Updated: 22/Mar/14  Resolved: 22/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 0
Labels: None
Environment:

JRE 7


Attachments: File clj-1387.diff     File clj-1387-v2.diff     File clj-1387-v3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Larger hash maps have nested INodes. As kvreduce implementations in INodes dereference reduced objects, parent INodes continue to reduce.

user=> (defn test-reduce-kv [m] (reduce-kv (fn [_ k v] (when (== 1 k) (reduced :foo))) nil m))
#'user/test-reduce-kv
user=> (test-reduce-kv (zipmap (range 3) (range 3)))
:foo
user=> (test-reduce-kv (zipmap (range 300) (range 300)))
nil

Dereferencing reduced objects should happen only PersistentHashMap/kvreduce - intermediate nodes should pass the Reduced object along.

Patch: clj-1387-v3.diff
Screened-by:



 Comments   
Comment by Alex Miller [ 18/Mar/14 5:11 PM ]

I updated the patch to use a generative test that will try many combinations of map size and the reduced index to bail out on. This test failed before applying the source patch and passes with it.

Comment by Rich Hickey [ 21/Mar/14 7:33 AM ]

if(root != null){ - return root.kvreduce(f,init); + init = root.kvreduce(f,init); + if(RT.isReduced(init)) + return ((IDeref)init).deref(); }

Turns code that always had a return into code that sometimes does.

Comment by Alex Miller [ 21/Mar/14 9:07 AM ]

Added new version of patch that retains the return flow and doesn't fall through.





[CLJ-1384] clojure.core/set should use transients Created: 15/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: performance

Attachments: Text File CLJ-1384-p1.patch     Text File CLJ-1384-p2.patch     File set-bench.tar    
Patch: Code
Approval: Ok

 Description   

CLJ-1384-p2 uses transients for both create and createWithCheck. This is consistent with the current implementation for map.

clojure.core/vec calls (more or less) PersistentVector.create(...), which uses a transient vector to build up the result.

clojure.core/set on the other hand, calls PersistentHashSet.create(...), which repeatedly calls .cons on a PersistentHashSet, with all the associated speed/GC issues.

Operation count now w/transients
set 5 1.771924 µs 1.295637 µs
into 5 1.407925 µs 1.402995 µs
set 1000000 2.499264 s 1.196653 s
into 1000000 0.977555 s 1.006951 s

Patch: CLJ-1384-p2.patch
Screened by: Stu



 Comments   
Comment by Gary Fredericks [ 15/Mar/14 10:13 PM ]

PersistentHashSet has six methods for creating sets – one for each combination of {with check, without check} and {array (varargs), List, ISeq}. Each of them does not use transients but could.

I believe clojure.core/set only depends on the (without check, ISeq) version.

Should all of these be changed? Three of them? One of them?

Comment by Andy Fingerhut [ 15/Mar/14 10:21 PM ]

I believe that the 'with check' versions are only intended to be used when reading set literals in Clojure source code, and give an error if there are duplicate elements. If you find examples where those set creation functions are called in other situations, I would be interested to learn about them to find out where my misunderstanding lies, or whether that is a problem with the current code.

If the belief above is correct, I would suggest not changing the 'with check' versions, since their speed isn't as critical.

Comment by Gary Fredericks [ 15/Mar/14 10:23 PM ]

Thanks Andy, I'll submit a patch that changes the three non-checked methods.

Comment by Gary Fredericks [ 15/Mar/14 10:46 PM ]

Attached CLJ-1384-p1.patch, which updates the three non-check create methods.

I also added benchmarks. It's about 2-3 times faster for large collections.

Comment by Ambrose Bonnaire-Sergeant [ 11/Apr/14 11:15 AM ]

Added benchmark suite (set-bench.tar).

FWIW results are similar to gfrederick's on my machine:

Clojure 1.6

Small collections (5 elements)

set averages 1.220601 µs
into averages 1.597991 µs

Large collections (1,000,000 elements)

set averages 2.429066 sec
into averages 1.006249 sec

After transients

Small collections (5 elements)

set averages 999.248325 ns
into averages 1.162889 µs

Large collections (1,000,000 elements)

set averages 1.003792 sec
into averages 889.993185 ms

Add full output to the tar.

Comment by Ghadi Shayban [ 11/Apr/14 11:35 AM ]

CLJ-1192 is related to this, but and Andy seems to be indicating the use of reduce as the means to better performance there.

Comment by Gary Fredericks [ 11/Apr/14 11:41 AM ]

Oh that's a good point about reduce. The difference should only apply to chunked seqs, right? It's worth noting that the benchmarks above used range which creates chunked seqs, so that might be why into looks faster on the large collections?

So this change only makes set act like vec; I think whether either/both of them should use reduce is a different question.





[CLJ-1382] Vector sort order should be specified sufficiently to embrace sort-by-juxt Created: 13/Mar/14  Updated: 15/Mar/14  Resolved: 15/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: data-structures, documentation, idiom


 Description   

Vectors of equal length sort in a way that seems natural – by
comparing their 0th elements, then their 1st, etc., until something
is different:

user> (def vv '(["c" 9] ["a" 100] ["a" 33] ["b" 8]))
#'user/vv
user> (sort vv)
(["a" 33] ["a" 100] ["b" 8] ["c" 9])

This property enables a blisteringly wonderful idiom for sorting
records by multiple keys:

user> (def mm (->> vv (map (fn [[p q]] {:p p :q q}))))
#'user/mm
user> (sort-by (juxt :p :q) mm)
({:p "a", :q 33} {:p "a", :q 100} {:p "b", :q 8} {:p "c", :q 9})

The sort-by-juxt idiom was described on briancarper.net[1], where it
was refined for Clojure 1.1 by Malcolm Sparks. Andy Fingerhut has
also written about it, thoroughly.[2]

Such lore gives it the odor of respectability, but the sort-by-juxt
idiom takes liberties beyond the documented behavior ("contract") of
vectors. APersistentVector indulges the idiom, but the clojure.org
Data Structures page does not say how vectors should compare.

The vector specification should be bolstered with enough traits of
vectors' sorting behavior to make the sort-by-juxt idiom safe to use
wherever Clojure might be implemented.

[1] http://briancarper.net/blog/488/sort-a-clojure-map-by-two-or-more-keys

[2] https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md



 Comments   
Comment by Alex Miller [ 13/Mar/14 9:52 PM ]

I don't understand what this ticket is asking for.

Comment by Andy Fingerhut [ 13/Mar/14 10:32 PM ]

It sounds like he is asking that the doc of clojure.core/compare say that vectors of equal length are compared in lexicographic order.

Comment by Phill Wolf [ 15/Mar/14 1:07 PM ]

"(sort-by (juxt" relies on a feature of vectors that the Clojure documentation does not guarantee. But using juxt in this way is part of the cultural fabric and helps make concise programs that "read like a definition" of the problem, to quote Halloway in "Programming Clojure". Therefore, let's document the sort order of equal-length vectors. I looked for this information on the Data Structures page, which has a section on vectors.

Comment by Alex Miller [ 15/Mar/14 11:11 PM ]

I added a line to the Vectors section on the Data Structures (http://clojure.org/data_structures) page: "Vectors are compared first by length, then each element is compared in order."





[CLJ-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code and Test
Approval: Ok

 Description   

Example showing how a local fn can be hinted but an anonymous function cannot:

;; OK
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)
            f (fn [])]
        (.submit ex ^Runnable f))
nil
;; ERROR - this should work the same as the previous
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)]
        (.submit ex #()))
CompilerException java.lang.IllegalArgumentException: More than one matching method found: submit, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init7901279404687292754.clj:3:9)

Cause: Functions have metadata, but Compiler does not look in them for type hints. Var expressions and local bindings use :tag metadata to override return of getJavaClass(). Compiler parses #() into a FnExpr, which always return AFunction as its class.

Proposed: Change FnExpr.getJavaClass() to return tag as type if it is available.

Patch: clj-1378-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 12/Mar/14 4:03 AM ]

Functions do have metadata, but Compiler does not look in them for type hints.

user=> (with-meta #() {:foo :bar})
#<clojure.lang.AFunction$1@779325ee>

When compiler is determining which native method to use, it matches method signature with classes of given args. There is a getJavaClass() method in Compiler.java which returns a class for given expression. Vars expressions and local bindings use :tag metadata to override this class, but most other expressions don't. Compiler parses #() into a FnExpr, which always return AFunction as its class.

Most of time this approach is OK, as AFunction implements Runnable and Callable so there is no need for type hint. However, in this particular case, there are overrides for both Runnable and Callable, and as AFunction can be either of them, the expression is ambiguous.

Comment by Jozef Wagner [ 12/Mar/14 4:17 AM ]

Patch added, following expression will now run without error

(.submit (java.util.concurrent.Executors/newCachedThreadPool) ^Runnable #())
Comment by Alex Miller [ 12/Mar/14 9:34 AM ]

Could you add a test to the patch?

Comment by Jozef Wagner [ 12/Mar/14 2:53 PM ]

Attached patch clj-1378-v2.diff which contains both fix and test.





[CLJ-1369] CLJ-738 is marked Closed is not implemented Created: 04/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: David Welte Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X, Java 6



 Description   

CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false. See CLJ-738 for many details.



 Comments   
Comment by Alex Miller [ 04/Mar/14 3:20 PM ]

Thanks for letting us know about this - I concur that 738 was incorrectly closed without being applied and I have resurrected that ticket. I am closing this one. In the future, feel free to just comment on a ticket directly, or better (for a closed ticket), comment on one of the mailing lists.





[CLJ-1365] New collection hash functions are too slow Created: 20/Feb/14  Updated: 11/Mar/14  Resolved: 11/Mar/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1365-v1.patch     Text File clj-1365-v2.patch     Zip Archive testclj1365.zip    
Patch: Code
Approval: Ok

 Description   

As reported ( https://groups.google.com/d/msg/clojure-dev/t6LAmVe-RLM/ekLTKxYfU5UJ ) by Mark Engelberg, the new collection hashing functions are slower than invoking the Murmur3 functions directly. See the attached zip for performance tests.

Approach: Made mix-collection-hash, hash-ordered-coll, and hash-unordered-coll use primitive type hints to avoid the bulk of the time.

Patch: clj-1365-v2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 20/Feb/14 11:26 AM ]

Added to 1.6 release.

Comment by Alex Miller [ 20/Feb/14 12:40 PM ]

Made hash functions inline for performance.

Comment by Rich Hickey [ 20/Feb/14 7:55 PM ]

Reported where?

This looks like bad benchmarking.

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] (clojure.lang.Murmur3/mixCollHash x y)))))

and

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] #_(clojure.lang.Murmur3/mixCollHash x y)))))

take the same time on my machine.

I'd need to see tests where the return was definitely used, it seems this is just more easily ignored by hotspot when not used.

We probably only need to hint count and the return for decent results.

Comment by Alex Miller [ 20/Feb/14 8:55 PM ]

It was reported by Mark Engelberg in his Instaparse rework - he observed these calls taking noticeably longer and overall times 10-20% down. I will ask him to chime in here.

Comment by Rich Hickey [ 04/Mar/14 8:44 AM ]

Could someone please test hinting hint count and the return? I'd hate for the answer to anyone's perf issues be inlining.

Comment by Alex Miller [ 04/Mar/14 9:06 AM ]

I will provide some more data for consideration of the options.

Comment by Alex Miller [ 04/Mar/14 11:07 AM ]

Test project for different variants

Comment by Alex Miller [ 04/Mar/14 11:11 AM ]

Attached a test project with different variants for testing and better benchmarking. To run:

unzip testclj1365.zip
cd clj1365
lein uberjar
java -server -cp target/clj1365-0.1.0-SNAPSHOT-standalone.jar clj1365.core

Results:

mix-collection-hash original
"Elapsed time: 57.777 msecs"
"Elapsed time: 18.034 msecs"
"Elapsed time: 20.591 msecs"
"Elapsed time: 25.179 msecs"
"Elapsed time: 21.781 msecs"
mix-collection-hash hints
"Elapsed time: 14.983 msecs"
"Elapsed time: 8.871 msecs"
"Elapsed time: 8.793 msecs"
"Elapsed time: 8.92 msecs"
"Elapsed time: 8.873 msecs"
mix-collection-hash inline
"Elapsed time: 10.04 msecs"
"Elapsed time: 7.117 msecs"
"Elapsed time: 7.306 msecs"
"Elapsed time: 7.324 msecs"
"Elapsed time: 7.175 msecs"
Murmur3/mixCollHash
"Elapsed time: 9.522 msecs"
"Elapsed time: 7.288 msecs"
"Elapsed time: 7.397 msecs"
"Elapsed time: 7.364 msecs"
"Elapsed time: 7.345 msecs"

From these results, I infer that the unhinted version is slower (21 ms) than a static call (7 ms). Inlining gives you same perf as static. Hinting inputs and return gives almost the same perf (9 ms).





[CLJ-1363] Field access via .- in reflective case does not work Created: 18/Feb/14  Updated: 28/Feb/14  Resolved: 28/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

Attachments: Text File clj-1363-v1.patch     Text File clj-1363-v2.patch     Text File clj-1363-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a)  ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a)      ;; as expected (prefer method)
"method"
user> (. t -a)     ;; WRONG - should return "field"
"method"

Approach: This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

Patch: clj-1363-v3.patch

Screened by:



 Comments   
Comment by Rich Hickey [ 20/Feb/14 7:24 PM ]

You can't change the semantics of invokeNoArgInstanceMember - they are correct when not using '-'. We need to feed the info that '-' was used through InstanceFieldExpr and make field-first conditional on that.

Comment by Alex Miller [ 21/Feb/14 5:42 AM ]

Updated with new patch to thread this case through InstanceFieldExpr.

Comment by Andy Fingerhut [ 28/Feb/14 6:02 AM ]

A patch for this ticket has been committed as part of Clojure 1.6.0-beta2: https://github.com/clojure/clojure/commit/5fda6cb262d1807566ecadd3af9aaafb58ee5544

It appears this ticket could be closed now.





[CLJ-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4, Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Nathan Davis Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1362-v1.patch    
Patch: Code and Test
Approval: Ok

 Description   

In some cases, reduce over a sequence from a primitive vector created with vector-of will return incorrect answers:

user=> (into [] (drop 32 (into [] (range 33))))
[32]
user=> (into [] (drop 32 (into (vector-of :int) (range 33))))
[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32]

Second call should return [32] just like the first one.

Cause: VecSeq (seq on primitive Vec obtained with vector-of) maintains two flags: i is the total number of elements prior to the current node in this seq. offset is the offset in the current anode. When using internal-reduce on a VecSeq, the starting index for the reduce was using offset and ignoring i.

Solution: Use (+ i offset) as the starting index.

Patch: clj-1362-v1.patch

Screened by:



 Comments   
Comment by Alex Miller [ 18/Feb/14 10:18 PM ]

We did some debugging on this at the St. Louis Clojure Meetup tonight and suspect the problem is happening when drop walks through the chunked seq over the vector. Specifically, in the VecSeq's implementation of IChunkedSeq.chunkedNext() at https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj#L116 particularly the offset 0 at the end.

Comment by Alex Miller [ 19/Feb/14 2:41 PM ]

Upon further review, the VecSeq seems to be created properly during chunking. The real issue is in internal-reduce where the starting index is improperly computed.

Comment by Stuart Sierra [ 25/Apr/14 1:05 PM ]

Screened.





[CLJ-1359] Fix changelog typos for 1.6 Created: 18/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1359.patch    
Patch: Code
Approval: Ok

 Description   

Some reported problems in the 1.6 changelog:

1) two different issues are both called CLJ-935
2) two issues that are probably different are both called CLJ-1328
3) "Make range consistently return () with a step of 0." This is slightly incorrect. Range now consistently returns an infinite sequence of start with a 0 step.

Patch: clj-1359.patch - updated for these issues, may want to hold this and update for any post-beta1 changes too.






[CLJ-1356] clojure.org/agents calls out deprecated funcs Created: 17/Feb/14  Updated: 17/Feb/14  Resolved: 17/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Ryan Macy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: agents, documentation, website


 Description   

""If any exceptions are thrown by an action function, no nested dispatches will occur, and the exception will be cached in the Agent itself. When an Agent has errors cached, any subsequent interactions will immediately throw an exception, until the agent's errors are cleared. Agent errors can be examined with agent-errors and cleared with clear-agent-errors.""

While it is true and those functions will do what it describes, they are listed as deprecated in the docs. Should we update this paragraph to reflect usage of `agent-error` and `restart-agent` instead?



 Comments   
Comment by Ryan Macy [ 17/Feb/14 11:38 AM ]

I hope I put this in the right place!

Comment by Alex Miller [ 17/Feb/14 12:32 PM ]

Yep, thanks!

Comment by Alex Miller [ 17/Feb/14 12:40 PM ]

Fixed.





[CLJ-1355] Restore symbol and keyword hashCode to avoid breaking compiled case expressions Created: 17/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

clojure 1.6.0-beta1


Attachments: File clj-1355-cached.diff     Text File clj-1355-v2.patch    
Patch: Code
Approval: Ok

 Description   

case expressions compiled in Clojure 1.5 are broken if run with Clojure 1.6 where hashCode behavior has diverged from hasheq. In particular, Symbol and Keyword fall into this category.

Approach: Cache both hashCode (with 1.5 calculation) and hasheq (new 1.6 calculation) in Symbol and just hasheq in Keyword. In 1.5, these were the same and case expressions compiled with 1.5 will store the old hash calculation. In 1.6, the hashCode of an expression will be used for comparison.

I tested this by AOT compiling a project in clojure 1.5.1 with this function:

(defn check [v]
  (case v
    :k "keyword match"
    'k "symbol match"
    "k" "string match"
    "no match"))

I verified that (check :k) and (check 'v) incorrectly returned "no match" on Clojure 1.6.0-beta1. I then verified that they returned "keyword match" and "symbol match" respectively on Clojure 1.6.0-master with this patch applied.

Patch: clj-1355-v2.patch



 Comments   
Comment by Alex Miller [ 17/Feb/14 9:38 AM ]

Add patch that caches a new hash field for both Symbol and Keyword that retains Clojure 1.5 computations.

Comment by Alex Miller [ 17/Feb/14 10:42 AM ]

There is a concern here that we are adding a new int field to every Symbol and Keyword (and keyword holds a symbol, so it's really 2 for each keyword).

Comment by Rich Hickey [ 20/Feb/14 7:27 PM ]

I don't think we need to cache in keyword, it's just an add

Comment by Alex Miller [ 20/Feb/14 9:24 PM ]

Updated patch to only cache hashCode in symbol and compute in Keyword.





[CLJ-1354] Make the class APersistentVector.SubVector public Created: 17/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch    
Patch: Code
Approval: Ok

 Description   

The patch marks APersistentVector.SubVector public so that it can be used as a type hint for reflection-free access to subvec internals. I missed this in CLJ-1150.

core.rrb-vector needs access to the internals of the built-in vector types in order to support their efficient concatenation and (true, RRB-style) slicing.

Patch: 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 17/Feb/14 7:30 AM ]

This is the exact spot where I'm trying to get at SubVector internals in core.rrb-vector:

https://github.com/clojure/core.rrb-vector/blob/core.rrb-vector-0.0.10/src/main/clojure/clojure/core/rrb_vector/rrbt.clj#L976

With 1.6.0-alpha3, {{(fv/catvec (subvec [0 1 2 3] 1 2) [:foo])}} results in IllegalAccessError tried to access class clojure.lang.APersistentVector$SubVector from class clojure.core.rrb_vector.rrbt$eval2476$fn__2477 clojure.core.rrb-vector.rrbt/eval2476/fn--2477 (rrbt.clj:978). With this patch applied, it works as expected, returning [1 :foo].





[CLJ-1353] Prevent test app from appearing in Mac OS X dock Created: 16/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Mac OS X


Attachments: Text File CLJ-1353-no-mac-dock.patch     Text File CLJ-1353-v2.patch     Text File clj-1353-v3.patch     Text File clj-1353-v4.patch    
Patch: Code
Approval: Ok

 Description   

During a local ant build of Clojure (tested with master after release of 1.6.0-beta1), the script/run_test.clj is executed. As a side-effect on the Mac, the Java coffee cup app icon is placed in the Dock, and the test app becomes the active application on the desktop. This is slightly annoying.

Even with this property set, activation of awt causes focus to switch temporarily then switch back (at least on Mac).

Solution: Set the following properties during the build:

java.awt.headless=true

Patch: clj-1353-v4.patch



 Comments   
Comment by Steve Miner [ 16/Feb/14 1:39 PM ]

CLJ-1353-no-mac-dock.patch adds a line to script/run_tests.clj to set the apple.awt.UIElement property. This prevents the test app from appearing in the Dock on Mac OS X.

Comment by Steve Miner [ 16/Feb/14 2:18 PM ]

CLJ-1349 might rearrange the affected source, which would force an update to this patch. Still just a one-liner so maybe it could be added to the patch for CLJ-1349.

Comment by Alex Miller [ 16/Feb/14 5:20 PM ]

I also find this highly annoying.

Comment by Andy Fingerhut [ 17/Feb/14 11:21 AM ]

Patch CLJ-1353-v2.patch is identical to Steve Miner's CLJ-1353-no-mac-dock.patch, except it adds another line to build.xml to set the property there, too. At least on my Mac systems, an icon appears in the dock during compilation, not only during testing, and this added line prevents that. Keeps Steve as the patch author.

Comment by Andy Fingerhut [ 17/Feb/14 11:22 AM ]

I tested CLJ-1353-v2.patch on a Linux system, too, and at least the messages that appear on the console during the execution of "ant" are identical with and without this patch, so no extra warnings appear due to these extra properties being set that are likely ignored by the JVM there.

Comment by Steve Miner [ 17/Feb/14 1:45 PM ]

Adding the sysproperty setting to build.xml sounds like a good idea. Thanks.

Comment by Alex Miller [ 18/Feb/14 1:42 PM ]

I found that even with this property, I still see focus change, then come back, during the build due to the activation of awt. Adding the java.awt.headless=true property made that stop. I updated the patch in both locations and now on Mac focus is never stolen during the build.

FYI: If you see the Java "Allow incoming network connections?" dialog on Mac during the tests in response to creating the Sockets in test/clojure/test_clojure/java/io.clj (test-socket-iofactory), this procedure makes that stop:

http://techblog.willshouse.com/2012/10/17/how-to-allow-java-in-the-firewall-on-os-x-mountain-lion/

Beware tracking down the correct version of Java (for example the 1.6 version) instead of the easier to find 1.7 version - the permissions are separate for each version.

Comment by Andy Fingerhut [ 24/Feb/14 2:35 PM ]

In my testing, the addition of the java.awt.headless=true properties in both build.xml and src/script/run_tests.clj was sufficient to avoid the additional icon appearing, and also avoiding any change of focus. Setting apple.awt.UIElement=true appears to be unnecessary (but harmless).

Comment by Steve Miner [ 24/Feb/14 3:28 PM ]

Yes, it seems that java.awt.headless=true is a better, more general solution for the build process. I think apple.awt.UIElement would be appropriate if you actually needed AWT for user interaction but didn't want the dock icon.

Comment by Alex Miller [ 25/Feb/14 11:33 AM ]

Added v4 patch that only sets java.awt.headless=true and drops the apple property.





[CLJ-1352] clojure.test/test-vars runs :each fixtures for vars without :test metadata Created: 14/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

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

Attachments: File tcrawley-fixtures-with-non-test-vars-2014-02-14.diff    
Patch: Code and Test
Approval: Ok

 Description   

The patch for CLJ-866 introduced a bug with :each fixtures and non-test vars: the fixtures are invoked for every var, not just ones with :test metadata.

Patch: tcrawley-fixtures-with-non-test-vars-2014-02-14.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 24/Feb/14 2:37 PM ]

The patch for this ticket has been committed: https://github.com/clojure/clojure/commit/919a7100ddf327d73bc2d50d9ee1411d4a0e8921

but the ticket has not yet been closed.

Comment by Alex Miller [ 24/Feb/14 3:09 PM ]

yeah, I noticed that too. I was going to mention it to Stu the next time we talked.





[CLJ-1348] Add functions for external collection hashing Created: 10/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1348-1.patch     Text File clj-1348-2.patch     Text File clj-1348-3.patch    
Patch: Code
Approval: Ok

 Description   

External collections wishing to implement hasheq appropriately must follow the advice at http://clojure.org/data_structures#hash. To simplify the implementation (and avoid unwanted dependencies on the internal Murmur3 class), add two new functions hash-ordered-coll and hash-unordered-coll that provide a proper collection hasheq over entire collections.

Patch: clj-1348-3.patch (fixes [k v])



 Comments   
Comment by Alex Miller [ 10/Feb/14 9:27 AM ]

Added patch. Will need to be refreshed once other patches go in.

Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Rich Hickey [ 12/Feb/14 10:53 AM ]

[k,v] => [k v]

Comment by Alex Miller [ 12/Feb/14 11:46 AM ]

New patch fixing [k v].





[CLJ-1345] Add 1.6 beta changelog updates Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1345-2.patch     Text File clj-1345.patch    
Patch: Code
Approval: Ok

 Description   

Update changelog for 1.6 beta.

Patch: clj-1345-2.patch



 Comments   
Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Alex Miller [ 12/Feb/14 12:47 PM ]

Updated patch to fix if-some and when-some definitions.





[CLJ-1344] defrecord still uses old hashing algorithm Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

Attachments: Text File clj-1344-1.patch    
Patch: Code
Approval: Ok

 Description   

defrecord implements hasheq by calling clojure.lang.APersistentMap/mapHasheq. mapHasheq uses the old map hash calculation instead of the new one. At least one external collection (data.avl) also calls this function. It should be updated to match the new hasheq calculations.

I considered changing defrecord to call Murmur3 directly, but this would create a case where the generated class does not work with older Clojure runtimes so I left it at calling mapHasheq instead.

Patch: clj-1344-1.patch



 Comments   
Comment by Alex Miller [ 07/Feb/14 1:33 PM ]

Attached patch to make mapHasheq use new hash map calculation.

Comment by Alex Miller [ 10/Feb/14 4:01 PM ]

oops





[CLJ-1343] Add some?, when-some, if-some for (not (nil? x)) conditions Created: 07/Feb/14  Updated: 15/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1343-1.patch     Text File clj-1343-2.patch     Text File clj-1343-3.patch     Text File clj-1343-4.patch    
Patch: Code and Test
Approval: Ok

 Description   

Sometimes it is useful to have a form for non-nil conditions (as opposed to the existing logical true conditions).
Three additions to support this case:

  • some? - same as (not (nil? x))
  • if-some - like if-let, but checks (some? test) instead of test
  • when-some - like when-let, but checks (some? test) instead of test

Patch: clj-1343-4.patch



 Comments   
Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Tassilo Horn [ 11/Feb/14 2:32 AM ]

At least to me, the name `some?` doesn't convey the same information as "not nil", so I'd rather prefer a more explicit name like `non-nil?`.

Also, I'm not convinced of the benefit of something like `(when-some x ...)` compared to `(when-not (nil? x) ...)`. A little shorter and one pair of parens less, but IMHO not as clear.

Comment by Jozef Wagner [ 11/Feb/14 2:59 AM ]

In my opinion, some? should be defined as (not (empty? coll)), and used as in "are there 'some' items in this collection?". This will also play nicely with some, which also takes collection as an argument.

Comment by Tassilo Horn [ 12/Feb/14 1:02 AM ]

Jozef, for that purpose, you'd use `seq`. Actually, the definition of `empty?` is `(not (seq coll))`, so your suggestion would boil down to `some?` being `(not (not (seq coll)))`.

Comment by Rich Hickey [ 12/Feb/14 10:56 AM ]

if-some and when-some are supposed to be like if-let and when-let respectively. Changelog will need updating as well

Comment by Alex Miller [ 12/Feb/14 12:38 PM ]

Updated patch to make if-some and when-some similar to if-let and when-let.

Comment by Alex Miller [ 14/Feb/14 10:01 AM ]

New patch that does not use "some?" in if-some and when-some.

Comment by Alex Miller [ 14/Feb/14 10:39 AM ]

New patch that adjusts when-some impl.

Comment by Kyle Kingsbury [ 15/Feb/14 1:04 PM ]

I'd like to echo Jozef Wagner's and Steve Losh's confusion here.

```
user=> (some odd? [1 2 3])
true
user=> (some? odd? [1 2 3])

ArityException Wrong number of args (2) passed to: user$some-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)
```

I might expect (some?) to behave like (some), except returning a boolean instead of a logically true value, but this is clearly not the case. In no other case in the stdlib can I think of two functions which differ only by punctuation yet have completely different semantics.

```
user=> (some? [])
true
```

Given (some)'s association with sequences, I might interpret (some?) to mean "are there some elements here?"; but that's definitely wrong. Given we have (not=), (not-any?), (not-empty), and (not-every?), can we please name this function (not-nil?)? It's only three characters, but makes the interpretation unambiguously clear.

```
user=> (def x nil)
#'user/x
user=> (def y nil)
#'user/y
user=> (some? [x y])
true
user=> (when-some [x y] :i-expect-true)
nil
```

The fact that (when-some) and (if-some) behave like let bindings is, erm, quite surprising to me. The other binding conditionals have -let in their name; perhaps it would be appropriate to use -let here as well?

For that matter, is this use case all that common? I think I reach specifically for a nil? test fewer than 1 in 20 conditionals--in those cases, why not just write

```
(when-let [x (not-nil? y)]
...)
```

instead of

```
(when-some [x y]
...)
```

I'm just not convinced that this pattern is common enough to warrant the confusion of (when-some) having nothing to do with (when (some ...)), haha. What do y'all think? Have I missed some symmetry between (some?) and (some) that helps this all make sense?

Comment by Alex Miller [ 15/Feb/14 4:36 PM ]

Summarizing comments here, mailing list, Twitter, etc:

  • some uses a truthy comparison. some->, some->> use a not nil comparison. This difference existed in 1.5 some?/if-some/when-some follow the latter. This split is unfortunate, but existed before this addition.
  • not-nil?, non-nil?, nnil?, exists?, and all other alternatives I've seen mentioned were considered as options before the existing names were chosen by Rich. Many people have expressed negative feedback about the name choices and I will channel that to Rich for consideration, but ultimately the choice is his.
  • if-some and when-some are likely more useful than some?. In particular, it is commonly needed when reading from core.async channels where nil is a special value (but false is not).
(go
  (if-some [v (<! c)]
    ...))




[CLJ-1339] Empty primitive vectors throw NPE on .equals with non-vector sequential types Created: 04/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1339.patch    
Patch: Code and Test
Approval: Ok

 Description   

Primitive vectors have several equality cases. In the case where the compared value is not a vector or random access collection but is a sequential or list, an empty primitive vector will throw an NPE:

user> (.equals (vector-of :long) [])   ;; ok
true
user> (.equals (vector-of :long) '())  ;; broken
NullPointerException   clojure.core.Vec (gvec.clj:135)

Cause: In this case of the primitive vector equals() method, seq is called on itself, then .equals() is invoked on the result. seq will return null for an empty primitive vector, causing an NPE.

Solution: Check for this condition and compare with (nil? (seq o)) on the other object.

Patch: CLJ-1339.patch






[CLJ-1338] New Murmur3 class is not public Created: 04/Feb/14  Updated: 07/Feb/14  Resolved: 07/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

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

Attachments: Text File clj-1338.patch    
Patch: Code
Approval: Ok

 Description   

The new Murmur3 class added for hashing is not public, which is problematic for code that needs to call it in several other tickets. To separate out this overlapping change, I have provided it here by itself.






[CLJ-1336] Allow external collections to use standard collection hashing Created: 31/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1336-1.patch     Text File clj-1336-2.patch     Text File clj-1336-3.patch     Text File clj-1336-4.patch    
Patch: Code and Test
Approval: Ok

 Description   

With the change in new hashing algorithms in 1.6, we need to provide a public hook for collections implemented outside of core to participate in the same hash mixing behavior as core collections.

Patch: clj-1336-4.patch
Depends on: CLJ-1338, CLJ-1339, CLJ-1335, CLJ-1331



 Comments   
Comment by Alex Miller [ 04/Feb/14 10:42 AM ]

Updated patch for a couple issues. However, in testing the use of this I discovered that the hash-basis must be an int and the basis accumulation must be based on int-accumulation with int-overflow, so it is not possible to do this in pure Clojure so this function is not currently useful.

I think the best solution would be to provide functions that encapsulate the ordered and unordered algorithms (Murmur3/hashOrdered and Murmur3/hashUnordered basically) such that external collections can implement hasheq correctly and with good performance.

Comment by Alex Miller [ 04/Feb/14 2:45 PM ]

Add new patch that makes Murmur3 class public so it will work for users of mix-collection-hash. Also adds generative tests for comparing the external collection hashing algorithm with hashes produced by internal ordered and unordered collections. These tests currently fail due to CLJ-1335 (empty list and empty lazy seq return wrong hash code).





[CLJ-1335] PersistentList$EmptyList and empty LazySeq still returns old value for hasheq Created: 30/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1335-v1.diff     Text File clj-1335-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

After late Jan 2014 changes to hash functions, PersistentList$EmptyList and (lazy-seq) were left behind:

user=> (= '() (lazy-seq) [])
true
user=> (map hash ['() (lazy-seq) []])
(1 1 -2017569654)
user=> (map class ['() (lazy-seq) []])
(clojure.lang.PersistentList$EmptyList clojure.lang.LazySeq clojure.lang.PersistentVector)

PersistentQueue/EMPTY was updated, so should not need any change.

Solution: Update LazySeq.hasheq() and make EmptyList implement IHashEq. EmptyList now creates a static constant for the hash value of an empty ordered collection and returns the constant for hasheq. An alternative would be to have Murmur3 have this constant instead.

Patch: clj-1335-v2.patch
Depends on: CLJ-1338, CLJ-1339, CLJ-1331 (must be applied first)

Patch:



 Comments   
Comment by Andy Fingerhut [ 30/Jan/14 6:33 PM ]

Patch clj-1335-v1.diff adds tests that assume the patch clj-1331-v1.diff on ticket CLJ-1331 have already been committed. If it is desired to combine these into one patch, or commit this one without that one, I can eliminate that dependency.

Makes PersistentList$EmptyList implement IHashEq interface with a straightforward implementation of hasheq(), comments out empty LazySeq special case check that caused it to return old hash value, and fixes a NullPointerException for primitive vectors discovered by the new tests added.





[CLJ-1331] Primitive vectors should use new hash Created: 29/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1331-v1.diff     Text File clj-1331-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

Primitive vectors created via vector-of still use Java hashCode for hasheq.

Solution: Make primitive vectors implement IHashEq and call Murmur3.hashOrdered().

Patch: clj-1331-v2.patch
Depends on: CLJ-1338 (must be applied first)



 Comments   
Comment by Andy Fingerhut [ 29/Jan/14 6:03 PM ]

Patch clj-1331-v1.diff is one way to change primitive vectors to use Murmur3 hash.





[CLJ-1328] Make some Clojure tests independent of hash function used Created: 20/Jan/14  Updated: 07/Feb/14  Resolved: 07/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1328-v3.diff     File clj-1328-v4.diff    
Patch: Code and Test
Approval: Ok

 Description   

The most interesting failures with the new hash function are probably the 3 deftest's in multimethods.clj that define the same multimethod name 'simple', and thus whether they pass or fail depends upon the order that they are executed. They are currently executed in an order that allows them to pass. Found this while testing murmurHash3 changes to Clojure, which caused the deftest's to execute in a different order and fail.

Simplest way to eliminate this dependency on order is to make the multimethod names unique in each test, so none of them depends upon state left behind by the others.



 Comments   
Comment by Andy Fingerhut [ 20/Jan/14 1:18 PM ]

Patch clj-1328-v1.diff makes all defmulti names unique in multimethods.clj, so that no deftest result depends upon state left behind by another.

Comment by Andy Fingerhut [ 29/Jan/14 8:11 PM ]

Updates some, but not all, tests that were recently modified or disabled due to change in hash function.

Comment by Andy Fingerhut [ 29/Jan/14 10:52 PM ]

Updates one more test than the previous patch.

Comment by Andy Fingerhut [ 31/Jan/14 3:43 PM ]

clj-1328-v4.diff is identical to clj-1328-v3.diff, except it adds a comment explaining why the case hash collision tests don't need to change much, and it puts in a couple of missing (is ...) around some equality tests.





[CLJ-1325] Report warnings if *unchecked-math* and boxing happens Created: 16/Jan/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 1
Labels: errormsgs, math

Attachments: File boxed.diff     Text File boxedmath.txt     Text File clj-1325.patch     Text File clj-1325-v2.patch     Text File clj-1325-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. The proposed enhancement here is to emit new warnings if *unchecked-math* is on and boxed math is occurring.

Approach: In the compiler, when compiling a StaticMethodExpr, if *unchecked-math* is true and the class is clojure.lang.Numbers and one of the parameters of static method is of type java.lang.Object or java.lang.Number, then emit a warning at compile-time.

In addition, there is a new WarnBoxedMath Java annotation - a small number of methods on Numbers with Object parameters use this annotation to indicate that warning should not take place. The same annotation can be (but is not currently) used to mark methods on Numbers without Object/Number params that should warn. See boxedmath.txt for a list of methods and categories.

Patch: clj-1325-v3.patch

Screened by:



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:56 PM ]

Moving to 1.7.

Comment by Alex Miller [ 15/Apr/14 10:17 AM ]

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.

Comment by Alex Miller [ 14/May/14 2:34 PM ]

Ready for screening.

Comment by Alex Miller [ 16/May/14 11:19 AM ]

clj-1325-v2.patch is identical to last except for a cleaned up the commit message.

Comment by Alex Miller [ 16/May/14 11:51 AM ]

Added v3 patch that just reworks block/indentation style to match surrounding code better.

Comment by Stuart Sierra [ 16/May/14 1:15 PM ]

Screened. Comments:

1) There is no way to get both overflow checks and boxed-math warnings at the same time. Maybe this doesn't matter.

2) The error messages aren't ideal, because they refer to clojure.lang.Numbers, but we can assume that anyone savvy enough to be using *unboxed-math* will also be savvy enough to know what clojure.lang.Numbers is.

3) This doesn't protect me from autoboxing in arbitrary Java method calls, but normal reflection warnings should catch most real-world cases, since few Java APIs overload on primitive and Object.





[CLJ-1318] Support destructuring maps with namespaced keywords Created: 06/Jan/14  Updated: 23/Feb/14  Resolved: 31/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1318-1.diff     File clj-1318-2.diff     File clj-1318-3.diff     File clj-1318-4.diff     File clj-1318-5.diff     File clj-1318-6.diff    
Patch: Code and Test
Approval: Ok

 Description   

Current :keys destructuring expects symbols and creates local bindings based on those symbols. This works fine with maps that use non-namespaced keyword keys. This enhancement is to add support for destructuring maps with namespaced keyword keys.

;; typical key destructuring for keyword keys without namespaces
(let [{:keys [a b]} {:a 1 :b 2}] (+ a b))

;; WANT some way to destructure map with namespaced keys
(let [{:keys [????]} {:x/a 1 :y/b 2}] (+ a b))

Approach: Allow keywords (with or without namespaces) in :keys destructuring. Destructure to bindings with the name of the keyword (namespace is ignored).

;; this now works
(let [{:keys [x/a y/b]} {:x/a 1 :y/b 2}] (+ a b))

;; add support for putting keywords into :keys as well to support ::keywords
(let [{:keys [:x/a :y/b]} {:x/a 1 :y/b 2}] (+ a b))
(let [{:keys [::a]} {:user/a 1}] a)

;; syms will also now support namespaced symbols
(let [{:syms [x/a y/b]} {'x/a 1 'y/b 2}] (+ a b))

Patch: clj-1318-6.diff

Screened by: Stuart Sierra. See comments, below.

Doc TODO: Will need to update http://clojure.org/special_forms#binding-forms with new binding form.



 Comments   
Comment by Nicola Mometto [ 06/Jan/14 11:58 AM ]

Why {:keys [:a/b]} and not {:keys [a/b}}?
Also, this should probably be extended to :syms for consistency

Comment by Alex Miller [ 06/Jan/14 12:28 PM ]

Good questions both. For the first question, we want to make locally namespaced keywords (::foo) work and there is no way to say that as a symbol.

I am waiting to hear back from Rich whether support for namespaced :syms is desirable. I think the change to support it is identical to the change to support namespaced keywords as symbols. I'm going to proactively update the patch to support that too.

Comment by Alex Miller [ 06/Jan/14 12:50 PM ]

Added new patch - now supports namespaced symbols or keywords in :keys and namespaced symbols in :syms.

Comment by Rich Hickey [ 06/Jan/14 1:00 PM ]

Should (also) support symbols for names, e.g. {:keys [a/b]}, only limitation is you can't get ns alias resolution. :syms support makes sense, but may seem weird to provide keywords for local names (where it doesn't as much for keywords), but would allow reaching aliases. My preference is no keyword names support for :syms, i.e. {:syms [a/b]} ok, {:syms [:a/b]} not.

Comment by Nicola Mometto [ 06/Jan/14 1:10 PM ]

To me {:syms [:a/b]} doesn't feel any more weird than writing {:keys [:a/b]}.
If this is going to be added, I think it should be consistent for :keys and :syms.
I understand that :syms is rarely used and this should not be an issue realistically, but I would expect everything that works for :keys to work for :syms too and adding only half a feature to :syms might cause unnecessary confusion.

Comment by Nicola Mometto [ 07/Jan/14 2:16 PM ]

With this patch this will now work:

user=> (let [:a/b 1] b)
1

I don't think this is desiderable.

Comment by Alex Miller [ 07/Jan/14 3:52 PM ]

Right, that is a consequence of allowing keywords in the :keys. At a glance this seems hard to address without significant changes unless we catch it prior to processing. Will consider.

Comment by Alex Miller [ 07/Jan/14 4:40 PM ]

Added new patch variant that catches keywords as let binding keys and throws an Exception.

Comment by Alex Miller [ 10/Jan/14 2:24 PM ]

Added one test in -4 showing example of auto-resolved keywords in :keys.

Comment by Stuart Sierra [ 10/Jan/14 3:00 PM ]

Screened. A few comments:

1. The examples in the tests use {:keys (a b)} with lists instead of {:keys [a b]} with vectors. Both forms are accepted both before and after the patch, but the docs at Clojure - special_forms only show vectors.

2. I would like this to work, but it would add some complexity:

(ns com.example.myproject.foo)

  (def data
    {::a 1 ::b 2})

  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
  (ns com.example.myproject.bar
    (:require [com.example.myproject.foo :as foo]))

  ;; I would like this to work:
  (let [{:keys [foo/a foo/b]} foo/data]
    [a b])
  ;;=> [nil nil]

  ;; This is good enough, however:
  (let [{:keys [::foo/a ::foo/b]} foo/data]
    [a b])
  ;;=> [1 2]

3. This doesn't produce an error, which is logically consistent but perhaps not desirable:

(let [{:a ::foo/a} foo/data]
    [a])
Comment by Rich Hickey [ 24/Jan/14 10:11 AM ]

please change the tests to use vectors

Comment by Alex Miller [ 24/Jan/14 10:28 AM ]

Added new -5 diff that uses vectors instead of lists in :keys tests.

Comment by Alex Miller [ 24/Jan/14 11:07 AM ]

And also fixing :syms [] in -6 diff.

Comment by Alex Miller [ 24/Jan/14 11:08 AM ]

Changed examples in description to use [].

Comment by Fogus [ 07/Feb/14 2:23 PM ]

A potential point of confusion here is illustrated by the following:

(let [m {:x/a 1, :y/b 2, :x/b 3000}
        {:keys [x/a y/b x/b]} m]
  (+ a b))

//=> 3

To get the answer 3001 one needs to remove the conflicting binding :y/b. Maybe this is not a big deal, but expect questions for the next 100 years.

Comment by David Nolen [ 23/Feb/14 5:01 PM ]

Ported to ClojureScript with CLJS-745





[CLJ-1304] Fixed minor typos in documentation and code comments Created: 09/Dec/13  Updated: 04/Feb/14  Resolved: 04/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Trivial
Reporter: Vipul A M Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: File clj-1304-v2.diff     File doc-comment-typos.diff    
Patch: Code
Approval: Screened

 Description   

Fixed minor typos in documentation and code comments across multiple files.



 Comments   
Comment by Andy Fingerhut [ 11/Jan/14 2:53 PM ]

Patch clj-1304-v2.diff dated Jan 11, 2014 is identical to Vipul's patch doc-comment-typos.diff dated Dec 9, 2013, except it applies cleanly to latest master. The only changes are that it removes the part of the patch for files in the ASM library, which was updated in a recent commit to Clojure master.

Comment by Alex Miller [ 04/Feb/14 9:21 PM ]

reopen so that I can set the fix version which didn't get set.

Comment by Alex Miller [ 04/Feb/14 9:22 PM ]

re-close now that fix version is set





[CLJ-1302] keys and vals consistency not mentioned in docstring Created: 04/Dec/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: docstring

Attachments: Text File clj-1302-1.patch     Text File clj-1302-2.patch    
Patch: Code
Approval: Ok

 Description   

(keys m) and (vals m) appear to return stuff in a consistent order, so (= m (zipmap (keys m) (vals m))). This consistency is a useful property. The API docs should state whether it is part of the functions' contract.

Patch: clj-1302-2.patch



 Comments   
Comment by Gary Fredericks [ 11/Dec/13 7:18 PM ]

One thing to keep in mind is that the functions can be used on arbitrary instances of java.util.Map, which, aside from being mutable, could hypothetically (though not realistically) generate their entry sets nondeterministically.

I don't know what any of this means about what the docstring should say. It could claim the consistency for clojure's collections at least.

Comment by Andy Fingerhut [ 11/Dec/13 7:44 PM ]

The ticket creator might already realize this, but note that (= m (zipmap (keys m) (vals m))) is guaranteed for Clojure maps, where m is the same identical map, at least by the current implementation. I am not addressing the question whether it is part of the contract, but I think it would be good to make this explicit if it is part of the contract.

The following is not guaranteed for Clojure maps: (= m1 m2) implies that (= (keys m1) (keys m2)).

The set of keys/vals will be equal, but the order of keys/vals could be different for two otherwise equal maps m1, m2.

Comment by Steve Miner [ 27/Dec/13 11:10 AM ]

I think you can depend on a slightly stronger contract: The order of the results from `keys` and `vals` follows the order of the results from `seq`. As with any pure function, `seq` returns consistent results across multiple calls with the same (identical?) map. The order may be arbitrary for a non-sorted map, but it should be consistent.

Some time ago, I looked for this guarantee in the documentation, but I couldn't find it explicitly stated. However, after looking at the implementation, I think it's safe to depend on this invariant.

Comment by Stuart Halloway [ 31/Jan/14 12:48 PM ]

The absence of this property in the docs is correct. You should not rely on this.

Comment by Nicola Mometto [ 31/Jan/14 7:43 PM ]

I have to say this surprises me, I was relying on this undocumented behaviour expecting it to be implicit.

I did a quick search in github and the number of (zipmap (keys m) (do-something (vals m))) is significant, even some experienced clojure developers seem to have given this property for granted (https://groups.google.com/forum/?fromgroups#!topic/clojure/s1sFVF7dAVs).

Could we at least explicitely document the absence of this property in the docs in order to avoid further confusion?

Comment by Peter Taoussanis [ 01/Feb/14 3:21 AM ]

Big surprise here too. Could someone (Stu?) possibly motivate a little why this couldn't/shouldn't be a contractual property? It seems like it has utility. Perhaps more importantly, it seems to be an intuitively reasonable assumption. That's subjective, sure, but I feel like I've seen this pattern come up quite widely.

Anecdotally, am quite sure I've made use of the assumption before (i.e. that `(keys m)` and `(vals m)` will return sequences as per pair order).

Would need to review code to see how frequently I've made the error.

To clarify: not disagreeing, just want to understand the thought that's gone in.

> Could we at least explicitely document the absence of this property in the docs in order to avoid further confusion?

That'd be a big help I think. I'd generally take an

Comment by Peter Taoussanis [ 01/Feb/14 3:58 AM ]

End of comment got mangled somehow.

Was just going to point out that I'm a big fan of how cautious + deliberate Clojure's design tends to be. Being hesitant to pick up needless or half-baked contractual obligations, etc. is a huge strength IMO.

Comment by Rich Hickey [ 01/Feb/14 9:36 AM ]

keys order == vals order == seq order

Comment by Alex Miller [ 05/Feb/14 11:25 AM ]

Tweaked doc.





[CLJ-1301] case expression fails to match a BigDecimal Created: 23/Nov/13  Updated: 26/Jan/14  Resolved: 26/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: Compiler

Attachments: Text File case-alt.patch     File clj-1301-1.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

In 1.5.1 (anywhere before the CLJ-1118 patch), this is the behavior on BigDecimal case matching:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "BigDecimal" "none")

In 1.6 the behavior (post CLJ-1118 patch) has changed:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "none" "none")

In 1.6 after CLJ-1118, I expect to see: ("Long" "BigDecimal" "BigDecimal") as they now have the same hash and hasheq.

Cause: The case constants are hashed in the clojure.core/case macro using clojure.core/hash which calls clojure.lang.util/hasheq(). In Compiler.emitExprForHashes(), a call to clojure.lang.Util/hash(). In Clojure 1.5 these hash values are the same (hash of 1.0M == hasheq of 1.0M == 311). In Clojure 1.6, they are different (hash of 1.0M = 311, hasheq of 1.0M = 31).

In any cases where Java's hashCode and Clojure's hasheq return different values, the case statement can fail to do the correct thing.

Approach: Change Compiler.java to use clojure.lang.Util hasheq() to match the case macro use of clojure.core/hash (which calls clojure.lang.Util.hasheq()).

Patch: clj-1301-1.diff

Screened by:



 Comments   
Comment by Andy Fingerhut [ 23/Nov/13 5:00 PM ]

Patch clj-1301-1.diff modifies Compiler.java so that case* statements use hasheq on the test expression value, rather than Java's hashCode. It also adds a test case that currently fails with latest Clojure master, but passes with the patch.

Comment by Andy Fingerhut [ 23/Nov/13 5:01 PM ]

This bug is also the root cause for the recent failures of tests for the test.generative library.

Comment by Alex Miller [ 10/Dec/13 3:22 PM ]

Putting in 1.6 release per Rich.

Comment by Alex Miller [ 13/Dec/13 3:36 PM ]

Andy, I talked to Rich and the conclusion was that we should make the opposite change here such that the case macro should route to the Java hashcode version clojure.lang.util.hash() and the Compiler should be left as is. Can you update the patch?

Comment by Alex Miller [ 13/Dec/13 3:38 PM ]

And in case you were wondering, the reason is that the Java hashcode is generally faster (case is all about speed) and there are easy opportunities for you to properly cast your expression and/or case constants (where as the situations with collections where boxing is difficult to fix generically, that is not true).

Comment by Andy Fingerhut [ 13/Dec/13 5:14 PM ]

Alex, unless I am missing something, changing case to use Java's hashCode() would also require changing its current equality comparison from Clojure = (aka equiv()) to something consistent with hashCode(), which I think must be Java's equals().

Such a change would mean that all of the things that are = but not equals() will not match each other in a case statement, e.g. a case value of (Integer. 5) will not match a (Long. 5) value to compare against in a case branch.

Is that really what is desired here? I almost hesitate to create such a patch, for fear it might be committed

Comment by Alex Miller [ 17/Dec/13 12:06 PM ]

Based on discussion comments, move back to Incomplete until we resolve.

Comment by Alex Miller [ 16/Jan/14 9:37 AM ]

Added better example demonstrating the problem (the specific problem exposed by CLJ-1118).

Comment by Alex Miller [ 16/Jan/14 11:50 AM ]

Simplified examples.

Comment by Alex Miller [ 16/Jan/14 12:29 PM ]

Re Andy's comments above, I walked down that path a bit and built such a patch, however we currently have tests in clojure.test-clojure.control:

(testing "test number equivalence"
    (is (= :1 (case 1N 1 :1 :else))))

which clearly seems to expect Clojure equiv() behavior over Java equals() behavior in case constant matching. So either that is a bad test or this is not a viable approach (it also suggests we could break existing code with this change).

Comment by Andy Fingerhut [ 16/Jan/14 12:55 PM ]

One could consider having the default behavior of case to use hasheq and clojure.core/= everywhere, but add a 'fast' option to use hashCode and Java equals.

Comment by Alex Miller [ 24/Jan/14 9:46 AM ]

Alternative patch in the direction of using hashcode/equals instead of hasheq/equiv. Note that this test causes some test failures. This is not yet a candidate patch - further work needs to be done in evaluating this path.





[CLJ-1285] Persistent assoc/conj on a transient-created collision node Created: 28/Oct/13  Updated: 11/Nov/13  Resolved: 11/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: transient

Attachments: File persistent-assoc-after-collision.diff     File transient-generative-test.diff    
Patch: Code and Test
Approval: Ok

 Description   

Bug reported by Zach Tellman https://groups.google.com/d/msg/clojure-dev/HvppNjEH5Qc/1wZ-6qE7nWgJ

Since transients were introduced the invariant array.length == count*2 doesn't hold for HashCollisionNode.
However persistent .without still relies on it.

Hence persistent dissoc on a collision node created by transients fails.

(let [a (reify Object (hashCode [_] 42))
      b (reify Object (hashCode [_] 42))]
      (= (-> #{a b} transient (disj! a) persistent! (conj a))
       (-> #{a b} transient (disj! a) persistent! (conj a))))

returns false.

Patch: persistent-assoc-after-collision.diff

Generative test patch: transient-generative-test.diff

The generative test reliably reproduces the error. It is simpler than the original test that found the bug but tests a series conj/disj/transient/persistent actions on a set. I've included it separately in case we decide not to apply.

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 29/Oct/13 9:58 PM ]

I can confirm that the patch works for me. As per our #clojure conversation, I've done the ClojureScript port; see CLJ-648.

Comment by Reid Draper [ 29/Oct/13 11:28 PM ]

I've run Zach's original test, as well as my own simple-check test. Both are passing.

Comment by Alex Miller [ 30/Oct/13 9:33 AM ]

I don't suppose we could get a generative test (prob need to use test.generative which is already included) to test this stuff similar to the original test that found the bug?

Very much hoping to get this into 1.6.

Comment by Andy Fingerhut [ 31/Oct/13 10:52 AM ]

Alex, I suspect clojure-dev would reach a much wider audience for your request than a comment on this ticket, which only has 3 watchers, and I don't think many people besides you and I watch the stream of all ticket state changes as they go by.

Comment by Michał Marczyk [ 01/Nov/13 5:19 AM ]

Just wanted to note that this patch, apart from preventing the hash-based collections from failing Zach's test suite, also makes avl.clj collections pass (now that I've released fixes for the two bugs uncovered by the test suite in avl.clj 0.0.9). This provides some cross-validation, I think.

(The built-in sorted collections pass either way, because they don't support transient ops.)

Also, David Nolen has merged the ClojureScript port of the patch.

Comment by Alex Miller [ 01/Nov/13 7:35 AM ]

I'm going to take a crack at repro with test.generative this morning - wish me luck!

Comment by Alex Miller [ 03/Nov/13 10:40 PM ]

Added a simplified version of a test-generative test and marked screened.

Comment by Alex Miller [ 11/Nov/13 11:17 AM ]

Patch was applied to master for 1.6.





[CLJ-1281] Clojure 1.6 - reconsider what is "alpha" in core Created: 23/Oct/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File alpha.patch    
Patch: Code
Approval: Ok

 Description   

In Clojure 1.5.1, the following things are marked as "alpha - subject to change". We should consider this list and whether some of them are no longer alpha and update them appropriately.

  • Watches (1.0): add-watch, remove-watch
  • Transients (1.1): transient, persistent!, conj!, assoc!, dissoc!, pop!, disj!
  • Exceptions (1.4): ex-info, ex-data
  • Promises (1.1): promise, deliver
  • Compiler warnings (1.4): :disable-locals-clearing
  • Records (1.3) defrecord
  • Types (1.3): deftype
  • Pretty print (1.3): print-table
  • clojure.reflect (1.3) (all)
  • Reducers (1.5) (all)

Patch: alpha.patch

  • Removes alpha marking for everything except reducers, disable-locals-clearing, and clojure.reflect. If Stu wants to remove for clojure.reflect, he should do so.

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 04/Nov/13 8:28 AM ]

Pulling into 1.6 as Rich has given me some feedback on what to change here.

Comment by Alex Miller [ 07/Nov/13 10:29 AM ]

Added patch that removes alpha designation from everything but reducers, disable-locals-clearing, and clojure.reflect (still TBD).

Comment by Andy Fingerhut [ 07/Nov/13 10:58 AM ]

definline is marked experimental in its doc string, and has been marked so since Clojure 1.0. Is it ready to be 'promoted', too?

Comment by Alex Miller [ 07/Nov/13 11:03 AM ]

Excellent question, will find out.

Comment by Alex Miller [ 07/Nov/13 12:40 PM ]

Rich says definline is still experimental, so no change.





[CLJ-1274] Unable to set compiler options via system properties except for AOT compilation Created: 02/Oct/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: compiler

Attachments: Text File CLJ-1274.patch    
Patch: Code
Approval: Ok

 Description   

The code that converts JVM system properties into keys under the *compiler-options* var is present only inside the clojure.lang.Compile class. This is a problem when using a debugger inside an IDE and not AOT compiling; specifying -Dclojure.compiler.disable-locals-clearing=true has no effect here when it would be most useful!

Patch: CLJ-1274.patch
Screened by: Stu



 Comments   
Comment by Howard Lewis Ship [ 02/Oct/13 4:52 PM ]

Obviously, that's supposed to be *compiler-options*.

Comment by Howard Lewis Ship [ 02/Dec/13 4:03 PM ]

Changes initialization of *compiler-options* to occur statically inside Compiler; now available to all forms of Clojure, not just AOT compilation; however, the initial *compiler-options* value is now defined as a root binding, rather than a per-thread binding, which has slightly different semantics.

Comment by Stuart Halloway [ 27/Jun/14 1:45 PM ]

Patch is straightforward, marking screened.

I am left wondering if other options that are set only in Compile.java ought also to be moved.





[CLJ-1268] Require Java 1.6 as minimum for Clojure Created: 28/Sep/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Blocker
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: build

Attachments: Text File clj-1268.patch    
Patch: Code
Approval: Ok

 Description   

In Clojure 1.6, we plan to move to JDK 1.6 as the minimum JDK.

Patch: clj-1268.patch

This patch changes the build configurations for both Maven and Ant to assume JDK 1.6 as the "source" and "target" runtimes.

Configuration changes will be necessary on Hudson. We already build Clojure and contrib libraries on JDK 1.6 by default, but we will need to remove matrix test builds for JDK 1.5. See for example clojure-test-matrix and data.csv-test-matrix – coordinate with Stuart Sierra for this change.



 Comments   
Comment by Stuart Sierra [ 04/Oct/13 8:45 AM ]

Screened.

I have verified that both the Ant and Maven builds still work (running on JDK 1.7) and that the output .class files contain the bytecode header for JDK 1.6.





[CLJ-1264] Minor change to Clojure source code to prevent warnings when compiled with JDK 8 Created: 17/Sep/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Critical
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

JDK 8


Attachments: File clj-1264-1.diff     Text File clj-1264-1.txt    
Patch: Code
Approval: Ok

 Description   

When compiling the Clojure source code using the early access version of JDK 8 (I saw this with 1.8.0-ea-b103 in particular), there are 6 warnings produced because the character _ is used as an identifier in Java source code.

    [javac] /home/jafinger/clj/latest-clj/clojure/src/jvm/clojure/lang/PersistentHashMap.java:1089: warning: '_' used as an identifier
    [javac] 	Box _ = new Box(null);
    [javac] 	    ^
    [javac]   (use of '_' as an identifier might not be supported in releases after Java SE 8)

The warning implies that this is likely to continue to be a warning for the lifetime of JDK 8, and could become an error with later JDKs.

Eliminating these warnings is as simple as changing the identifier name used on those 6 lines of one Java source file.

Patch: clj-1264-1.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 17/Sep/13 11:23 PM ]

Patch clj-1264-1.txt changes the identifier _ used on 6 lines of file PersistentHashMap.java to the name addedLeaf, which is used for a similar purpose elsewhere in the file.





[CLJ-1261] Invalid defrecord results in exception attributed to namespace that imports namespace with defrecord Created: 12/Sep/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.7

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord, errormsgs

Attachments: File clj-1261-2.diff     File clj-1261-3.diff     File clj-1261-4.diff     File clj-1261-5.diff    
Patch: Code and Test
Approval: Ok

 Description   

I was introducing a namespace that included a defrecord.

My defrecord was wrong; it used a keyword to define a field, not a symbol. Minimal test case:

% cat src/useclj16/init.clj
(ns useclj16.init)

(defrecord Application [:shutdown-fn])
% cat src/useclj16/app.clj 
(ns useclj16.app
  (:require [useclj16.init :as init]))

However, the exception was perplexing:

% java -cp clojure-1.6.0-master-SNAPSHOT.jar:src clojure.main
user=> (require 'useclj16.app)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)

user=> (pst *e 100)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj
        clojure.core/with-meta (core.clj:214)
        clojure.core/defrecord/fn--147 (core_deftype.clj:362)
        clojure.core/map/fn--4210 (core.clj:2494)
        clojure.lang.LazySeq.sval (LazySeq.java:42)
        clojure.lang.LazySeq.seq (LazySeq.java:60)
        clojure.lang.RT.seq (RT.java:484)
        clojure.lang.LazilyPersistentVector.create (LazilyPersistentVector.java:31)
        clojure.core/vec (core.clj:354)
        clojure.core/defrecord (core_deftype.clj:362)
        clojure.lang.Var.invoke (Var.java:427)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.lang.Compiler.macroexpand1 (Compiler.java:6483)
        clojure.lang.Compiler.macroexpand (Compiler.java:6544)
        clojure.lang.Compiler.eval (Compiler.java:6618)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        useclj16.app/eval322/loading--4916--auto----323 (app.clj:1)
        useclj16.app/eval322 (app.clj:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6623)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        user/eval318 (NO_SOURCE_FILE:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6597)
        clojure.core/eval (core.clj:2864)
        clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
        clojure.main/repl/read-eval-print--6594 (main.clj:260)
        clojure.main/repl/fn--6603 (main.clj:278)
        clojure.main/repl (main.clj:278)
        clojure.main/repl-opt (main.clj:344)
        clojure.main/main (main.clj:442)
        clojure.lang.Var.invoke (Var.java:411)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.main.main (main.java:37)
nil

The error was attributed to app.clj (useclj16.app), a namespace which requires useclj16.init, the namespace containing the defrecord.

No indication that this concerned a defrecord, or even what namespace contained the error, was present in the exception.

Patch: clj-1261-5.diff

Approach: Check explicitly that the fields are all symbols, for both defrecord and deftype, and throw a CompilerException with file, line, and column number if not. Example of exception after patch is applied, in the case give above:

user=> (require 'useclj16.app)
CompilerException java.lang.AssertionError: defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, compiling:(useclj16/init.clj:3:1)

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 12/Sep/13 8:58 PM ]

Can you include an example of the defrecord definition just so we're clear what it looks like?

Comment by Alex Miller [ 12/Sep/13 8:59 PM ]

Also, "feedback" is not a useful label. Please use "errormsgs" for stuff like this. See the list of many commonly used labels here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Howard Lewis Ship [ 13/Sep/13 10:42 AM ]

"Feedback" is my own personal crusade http://tapestryjava.blogspot.com/2013/05/once-more-feedback-please.html

In my case, my invalid code was:

(defrecord Application [:shutdown-fn])

And the mistake was that :shutdown-fn should be a symbol, not a keyword.

Here it is, more completely:

(ns novate.services.initialization
  "Infrastructure for system-as-transient state.")

(defrecord Application [:shutdown-fn])

and

(ns novate.services.activator
  "Responsible for bootstrapping the application by loading certain namespaces and invoking certain functions, guided by data in JAR manifests."
  (:gen-class)
  (:require [clojure.edn :as edn]
            [clojure.java.io :as io]
            [novate.util.logging :as l]
            [novate.services
             [initialization :as init]
             [ordering :as o]])
  (:import [java.io PushbackReader]))

The error was attributed to this file.

Comment by Andy Fingerhut [ 13/Sep/13 11:48 AM ]

Patch clj-1261-v1.txt throws an exception if any fields given to defrecord or deftype are not symbols. They are CompilerExceptions, so include an accurate file, line, and column number.

Comment by Andy Fingerhut [ 13/Sep/13 11:57 AM ]

Updated description to give minimal test case.

Comment by Andy Fingerhut [ 23/Nov/13 1:06 AM ]

Patch clj-1261-2.diff is identical to clj-1261-v1.txt except that it applies cleanly to latest master. The only change was to some lines of context due to recent commits to Clojure.

Comment by Alex Miller [ 18/Apr/14 1:16 PM ]

I think the patch is ok but I have two suggestions in the error message - first, include the record/type ns+name (I think the classname in the patched fn is what you want). Second, I think the wording could be adjusted a bit and the parens should go away - those look like but don't actually have meaning in the original context (since you are filtering out the symbols). Maybe something like:

"defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, :foo-bar"

Comment by Andy Fingerhut [ 30/Apr/14 9:32 AM ]

Patch clj-1261-3.diff attempts to incorporate Alex's suggested error message changes.

There are other errors caught by function validate-fields that could have more details like the namespace and record/type name added to them, but I don't want to go out of scope for the ticket. I can create another patch that does that if there is interest.

Comment by Alex Miller [ 30/Apr/14 2:56 PM ]

Can you update the "after" example in the Approach section of the description to match new?

Comment by Andy Fingerhut [ 01/May/14 4:18 PM ]

Updated example output at end of description to be what is seen after patch clj-1261-3.diff is applied.

Comment by Alex Miller [ 05/May/14 1:56 PM ]

Description looks good, patch looks good. Test?

Comment by Andy Fingerhut [ 05/May/14 2:03 PM ]

I'd be happy to write one, if I had a "similar" one to pattern them on.

By similar, I mean: are there any existing tests that require a namespace that isn't already loaded & compiled when the tests begin running, and catch exceptions thrown during the require?

Comment by Alex Miller [ 05/May/14 3:56 PM ]

I think you should be able to test the right error message here by just invoking the defrecord form.

Otherwise, maybe https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/ns_libs.clj#L87 ?

Comment by Andy Fingerhut [ 10/May/14 6:43 PM ]

Patch clj-1261-4.diff is identical to clj-1261-3.diff except that it adds a couple of unit tests verifying that an exception of the desired type and with an appropriate message is thrown when keywords are used as defrecord or deftype fields.

Comment by Alex Miller [ 13/May/14 10:41 PM ]

same as -4 but changed final defrecord to deftype in test (seemed like a typo)

Comment by Andy Fingerhut [ 13/May/14 11:40 PM ]

Thanks for the catch on that typo in the tests. You changed it to what I had intended.

Comment by Alex Miller [ 13/May/14 11:54 PM ]

seemed pretty clear





[CLJ-1251] The update function: like update-in, for first level Created: 03/Sep/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Michael O. Church Assignee: Ambrose Bonnaire-Sergeant
Resolution: Completed Votes: 3
Labels: None

Attachments: Text File CLJ-1251.patch     Text File update.patch    
Patch: Code and Test
Approval: Ok

 Description   

update-in is useful for updating nested structures. Very often we just want to update one level, so an update function optimised for this use case is useful.

It operates identically to update-in with a key path of length one so these are the same:

(update-in m [k] f args...)
(update m k f args...)

Patch: CLJ-1251.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 06/Sep/13 9:56 AM ]

I like this - kind of halfway between assoc and update-in.

Comment by Michael O. Church [ 07/Sep/13 12:41 PM ]

It's very useful. I assumed that its non-inclusion was for a reason (hence was hesitant to submit the patch) but it comes in handy a lot. One project I'd like to do with some free time is a library for turn-based strategy games, which use update frequently to express game-state changes.

The downside of this change is that 'update is probably a defined function in a good number of modules written by other people. IMO the strongest reason not to include it is that it's such a common name; but the benefits (in my view) outweigh the downsides.

Comment by Andy Fingerhut [ 14/Feb/14 11:50 AM ]

Patch update.patch dated Sep 3 2013 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.

Comment by Ambrose Bonnaire-Sergeant [ 06/May/14 2:36 PM ]

The vararg validation should be done in the same way as `assoc`.

Comment by Alan Malloy [ 06/May/14 2:41 PM ]

The most obvious reason, to me, that clojure.core/update doesn't exist already is that it's not clear what it should do when given more than 3 arguments. Consider, for example, (update m a b c d). What does this do? There are at least three reasonable interpretations: (update-in m [a] b c d), passing c and d as extra args to the function b; (-> m (update-in [a] b) (update-in [c] d)), treating the args as alternating key/function pairs; (reduce (fn [m k] (update-in m [k] a)) m [b c d]), treating a as a function to apply to each of b, c, and d.

Any of these are plausible meanings for the vague name "update", and there's no obvious behavior to choose, whereas there's only one reasonable way for assoc and assoc-in to behave. If one of them were chosen, it would be a little bit nontrivial to read code using it, at least until it became so well-known that everyone thinks it's obvious. I don't have anything against this function that Michael Church has written, or including it in core, but I don't like naming it update, as if it were the only possible dual to update-in.

Comment by Kyle Kingsbury [ 06/May/14 4:09 PM ]

I'd like to second Alan Malloy's concern; I've defined (update m k f arg1 arg2) in most of my Clojure work to be "change the value for this key to be (f current-value arg1 arg2 ...)"; this is consistent with swap!, update-in, etc., and is in my experience the most common need for update. It also composes well with swap! and other higher-order friends. I suggest we use that variant instead, and rely on assoc or -> threading when updating multiple fields.

Comment by Michael O. Church [ 07/May/14 10:32 AM ]

I agree with Kyle and Alan. There are several interpretations of how update should behave and while it's not clear which one is "correct", Kyle's is most consistent with the rest of the language and therefore probably more right than the one I started with.

The issue I see with including an "update" function is that it will break code for others who've defined it for themselves. Kyle's interpretation is more consistent with the rest of Clojure and will probably involve the least breakage. I'd be happy using his version, and renaming mine to something else.

Comment by Rich Hickey [ 13/May/14 6:09 AM ]

I am in favor, and it should work like everything else: (update m k f args...)

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 7:18 AM ]

I'm working on a new patch.

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 7:39 AM ]

update-like-update-in.patch is the new patch as Rich requests.

Comment by Alex Miller [ 13/May/14 8:56 AM ]

Ambrose, I think the example in the description no longer follows the (update m k f args...) form right? Can you update?

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 9:46 AM ]

Alex, I'm not sure what you're referencing?

Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 9:47 AM ]

If you mean the docstring, I did try and update it for update by copying update-in and change and plural keys to singular.

Comment by Alex Miller [ 13/May/14 10:18 AM ]

I mean the description for this ticket needs to be updated to reflect what we are currently considering.

Comment by Alex Miller [ 13/May/14 12:57 PM ]

In the patch, the docstring has "If the key does not exist, a hash-map will be created." which is not applicable in update right? I think it would be more accurate to say that the fn will be invoked on nil.

This line occurs twice in the tests:

{:a [1 2]}   (update {:a [1]} :a conj 2)

There is no test for what happens when the key is absent. For example:

(update {:a 1} :b str)
=> {:b "", :a 1}
Comment by Ambrose Bonnaire-Sergeant [ 13/May/14 1:30 PM ]

I removed the mention of creating hash-maps, and replaced it with the explicit behaviour of passing `nil` for missing keys.

FWIW I proposed a similar wording in the patch for http://dev.clojure.org/jira/browse/CLJ-373

Added a test for missing key. Removed the duplicate test.

Comment by Gary Fredericks [ 16/May/14 8:45 PM ]

Is it worth unrolling several arities for the sake of premature optimization? e.g., https://github.com/Prismatic/plumbing/blob/master/src/plumbing/core.clj#L33-41

Comment by Alex Miller [ 22/May/14 8:14 AM ]

I think that's probably worth doing - who can update the patch with multiple arities?

Comment by Alex Miller [ 23/May/14 11:25 AM ]

Ambrose, can you (or anyone else really) update the patch to unroll small arities?

Comment by Ambrose Bonnaire-Sergeant [ 23/May/14 11:40 AM ]

Yes will do now.

Comment by Ambrose Bonnaire-Sergeant [ 23/May/14 12:16 PM ]

Add multiple arities + tests (CLJ-1251.patch)





[CLJ-1248] Show type information in reflection warning messages when available Created: 24/Aug/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Christoffer Sawicki Assignee: Unassigned
Resolution: Completed Votes: 12
Labels: errormsgs

Attachments: Text File clj-1248-2.patch     Text File Include-type-information-in-reflection-warning-messa.patch    
Patch: Code and Test
Approval: Ok

 Description   

The reflection warning messages currently don't show any type information. I think adding this would make the messages more helpful by making it more obvious what the problem is. I suggest these changes:

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

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah on java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap on java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: java.util.regex.Pattern).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: unknown).

Patch: clj-1248-2.patch



 Comments   
Comment by Alex Miller [ 25/Aug/13 8:31 AM ]

I think these are a good idea. I think it would be better to separate the reflected class from the field though since we are referring to fields that don't exist.

For example:
1) field: "reference to field blah in java.lang.String can't be resolved."
2) method: "call to method zap in java.lang.String can't be resolved."
3) static method: "call to method valueOf in java.lang.Integer can't be resolved."

Your 3rd example actually highlights something more interesting though. In this case the problem is not actually that Integer/valueOf does not exist but rather that it is being called with the wrong types. In these cases, what I want to know is: what is the type of the parameters being passed.

In #2 there are actually two possible sources of error - an unexpected type for x or an unexpected type or arity for the parameters. It would be useful to check whether the method of this name exists and at what arity to determine which of these cases exists and give a more precise error.

In any case, the implementation needs to be supplied as a patch, not a link. See: http://dev.clojure.org/display/community/Developing+Patches

Comment by Christoffer Sawicki [ 25/Aug/13 3:25 PM ]

+1 on all points. I'll begin work on an updated patch.

Yes, there is a deeper more interesting problem lurking here.

One question is what should result in reflection warnings and what in compile-time errors. (I think some types of reflection warnings should be promoted to errors.)

Here are some examples of current behavior with comments:

(fn [] (Integer/valueOff :foo))
CompilerException java.lang.IllegalArgumentException: No matching method: valueOff, compiling:(NO_SOURCE_PATH:1:8) 

^ Error because the compiler can statically see this is never going to work. Fine.

(fn [] (Integer/valueOf :foo))
Reflection warning, NO_SOURCE_PATH:1:8 - call to valueOf can't be resolved.

^ This is never going to work either, but only gives a warning. A bit surprising; I'd prefer an error.

(fn [x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:9 - reference to field foo can't be resolved.

^ This could work. Fine.

(fn [^String x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:17 - reference to field foo can't be resolved.

^ This is never going to work if x is a String but x can be of any type at run-time. Personally, I think this should be an error...

Comment by Alex Miller [ 25/Aug/13 4:36 PM ]

You should take any warning/error differences to one (or more) new tickets where they can be evaluated individually. The more you put in one ticket, the more likely it is to get bogged down and/or rejected. My gut feeling is that there would not be a lot of support for the warning->error changes you suggest.

Comment by Christoffer Sawicki [ 28/Aug/13 3:05 PM ]

Here's an updated patch that changes the messages like this:

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah of java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap of java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [java.util.regex.Pattern]).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [unknown]).

(I wish I could edit the issue description.)

Comment by Alex Miller [ 28/Aug/13 3:18 PM ]

Updated description per latest patch.

Comment by Alex Miller [ 12/Feb/14 1:12 AM ]

I used this in a local build to resolve an issue tonight and found it very helpful. One comment I have is that in the message part "(argument types: [java.util.regex.Pattern])", I would like to remove the outer [ ] around the argument types. In the case I was working on the first type was actually a long[] which has class name "[J" so I found the outer []s distracting.

Comment by Christoffer Sawicki [ 12/Feb/14 5:41 AM ]

Thanks for the success story!

I think I choose to use a vector to disambiguate the case with one argument that is unknown, i.e. "(argument types: [unknown])". Without the vector, it's not obvious if there's one argument of unknown type or if all of multiple arguments are of unknown type.

Given your input and some more thought, it's probably not worth making every other message worse for this single case. I'll update the patch tonight.

Comment by Alex Miller [ 12/Feb/14 9:49 AM ]

I went ahead and updated the patch. I also fixed a couple whitespace issues and changed the word "of" to "on" before the type as I think it reads better.

Comment by Christoffer Sawicki [ 12/Feb/14 1:57 PM ]

OK, thanks!





[CLJ-1247] Document the availability/usage of *e, *1, *2, ... in REPL Created: 24/Aug/13  Updated: 29/Aug/13  Resolved: 24/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Jakub Holy Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: documentation, repl


 Description   

For new users of Clojure, it is very hard to find out that evaluation results in any Clojure REPL are bound to *1 - *3 and the latest exception to *e. Since it is a pretty useful feature, it should be documented at a visible place. Where that place is, I am not sure.

One possibility would be to add it to the docstring of clojure.main/repl and make http://clojure.org/repl_and_main link to it (i.e. in the "Launching a REPL" section, we could add something like "Read the <link to=somewhere>docstring of clojure.main/repl</link> to learn about options and available vars. See also utility functions/macros in the clojure.repl namespace."

Note: This was originally reported under nREPL in NREPL-43.



 Comments   
Comment by Alex Miller [ 24/Aug/13 2:52 PM ]

I updated the http://clojure.org/repl_and_main page to include much of this info.

Comment by Jakub Holy [ 29/Aug/13 3:16 AM ]

Lovely, thanks!





[CLJ-1246] type-reflect with AsmReflector throws exceptions for classes with annotations Created: 21/Aug/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: interop

Attachments: File clj-1246-fix-type-reflect-exception-patch-v1.diff     Text File clj-1246-fix-type-reflect-exception-patch-v1.txt    
Patch: Code and Test
Approval: Ok

 Description   

REPL session reproducing the problem:

% java -cp clojure.jar clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (use 'clojure.reflect)
nil
user=> (import '[clojure.reflect AsmReflector])
clojure.reflect.AsmReflector
user=> (def cl (.getContextClassLoader (Thread/currentThread)))
#'user/cl
user=> (def asm-reflector (AsmReflector. cl))
#'user/asm-reflector
user=> (type-reflect 'java.lang.SuppressWarnings :reflector asm-reflector)
AbstractMethodError clojure.reflect.AsmReflector$reify__9181.visitAnnotation(Ljava/lang/String;Z)Lclojure/asm/AnnotationVisitor;  clojure.asm.ClassReader.accept (ClassReader.java:593)

Issue discovered when trying out a build of Clojure source code with JDK8 early access version b103. In Clojure's set of tests, one of the classes tested with type-reflect fails with JDK8 because annotations were added to that class in JDK8, but it did not have annotations in earlier JDK versions. The same issue exists with JDK6 and JDK7 for any class with annotations, though – it is not unique to JDK8.

Analysis:

Definition of AsmReflector type in src/clj/clojure/reflect/java.clj has a reify for a ClassVisitor with no definition for a visitAnnotation method. This method is called for classes with annotations.

Patch: clj-1246-fix-type-reflect-exception-patch-v1.diff
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 21/Aug/13 8:04 PM ]

Patch clj-1246-fix-type-reflect-exception-patch-v1.txt dated Aug 21 2013 eliminates the exception by implementing the visitAnnotation method in the ClassVisitor object of the AsmReflector implementation. The implementation simply returns nil, which is enough for the caller to keep going through the class definition, ignoring any annotations.

Comment by Alex Miller [ 22/Oct/13 9:25 PM ]

Is there any reason for the addition of SuppressWarnings in the test file?

Comment by Andy Fingerhut [ 22/Oct/13 9:52 PM ]

I added that because that new test fails on JDK6 and 7 without the patch. Without the additional test, the bug is not exposed by any existing tests unless you run on JDK8.

Comment by Alex Miller [ 23/Oct/13 10:56 PM ]

What new test? I just see the SuppressWarnings import?

Comment by Andy Fingerhut [ 24/Oct/13 9:05 AM ]

The line where java.lang.SuppressWarnings is added is not an import, but naming another class in a sequence of classes being iterated over via doseq in a test.

Comment by Alex Miller [ 24/Oct/13 2:14 PM ]

Ah, thank you. Diff blindness.





[CLJ-1245] Disable GitHub "Issues" feature to avoid non-CA/non-JIRA issue submissions Created: 14/Aug/13  Updated: 17/Aug/13  Resolved: 15/Aug/13

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

Type: Task Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

I couldn't find any better project to file this against so I'm filing it against the Clojure project proper. I believe this applies to all sub-projects governed by the Clojure CA, hosted on GitHub.

The GitHub issues feature is a magnet for people who would like to help out (in what is a very common and expected manner). Unfortunately, the rejection / redirection process seems to annoy some people unnecessarily and takes time and attention of people who know the actual process to direct them to read/sign the CA and submit via JIRA.

Since GitHub allows repo administrators to disable the issues feature it seems prudent to do so. (e.g. https://github.com/clojure/clojure/settings -> uncheck "Issues"; repeat for other CA projects).



 Comments   
Comment by Aaron Brooks [ 14/Aug/13 8:42 PM ]

I misparsed a pull request as an issue via the GitHub email notifications (they look similar). I see that the Clojure repo does have "Issues" disabled and that there is no way to disable pull requests (that I can see). Please feel free to close this issue without further thought.

Comment by Alex Miller [ 17/Aug/13 7:33 AM ]

BTW, there is an existing ticket out there to add a contributing.md file to specify the policy via GitHub - CLJ-1122.

Comment by Aaron Brooks [ 17/Aug/13 3:03 PM ]

Thanks for pointing that out. Clearly this is a very difficult piece of markdown to deploy...





[CLJ-1241] NPE when AOTing overrided clojure.core functions Created: 30/Jul/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Defect Priority: Major
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: aot, compiler

Attachments: Text File 0001-fix-CLJ-1241.patch    
Patch: Code
Approval: Ok

 Description   

When performing AOT compilation on a namespace that overrides a clojure.core function without excluding the original clojure.core function from the ns, you get a NullPointerException.

To reproduce aot compile a namespace like "(ns x) (defn get [])"

For example:

$ lein new aot-get
$ cd aot-get
$ sed -i s/foo/get/
$ lein compile :all
WARNING: get already refers to: #'clojure.core/get in namespace: aot-get.core, being replaced by: #'aot-get.core/get
Exception in thread "main" java.lang.NullPointerException
	at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4858)
	at clojure.lang.Compiler$DefExpr.emit(Compiler.java:428)
	at clojure.lang.Compiler.compile1(Compiler.java:7152)
	at clojure.lang.Compiler.compile(Compiler.java:7219)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)

Cause: DefExpr.parse does not call registerVar for vars overridding clojure.core ones, thus when AOT compiling the var is not registered in the constant table.

Proposed: The attached patch makes DefExpr.parse call registerVar for vars overridding clojure.core ones.

Patch: 0001-fix-CLJ-1241.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 30/Jul/13 7:29 PM ]

DefExpr.parse was not calling registerVar for vars overridding clojure.core ones.

Comment by Alex Miller [ 31/Jul/13 12:25 AM ]

Verified on Clojure 1.5.1.

Comment by Javier Neira Sanchez [ 27/Aug/13 8:34 AM ]

Reproduced with `key` function without `(:refer-clojure :exclude [key])`

Comment by Rich Hickey [ 05/Sep/13 8:32 AM ]

This doesn't meet triage guidelines - i.e. there is this problem, therefore we will fix it by _____ so it then does _____

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

This is still present in the 1.6 release. I think it's mis-classified as low priority.

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

See for instance the cascalog mailing list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Comment by Andy Fingerhut [ 26/Mar/14 1:07 PM ]

It may help if someone could clarify Rich's comment.

Does it mean that the ticket should include a plan of the form "therefore we will fix it by _____ so it then does _____", but this ticket doesn't have that?

Or perhaps it means that the ticket should not include a plan of that form, but this ticket does? If so, I don't see it, except perhaps the very last sentence of the description. If that is a problem for vetting a ticket, perhaps we could just delete that sentence and proceed from there?

Something else?

Comment by Nicola Mometto [ 26/Mar/14 1:13 PM ]

Andy, I added the two last lines in the description after reading Rich's comment to explain why this bug happens and how the patch I attached works around this.

I don't know if this is what he was asking for though.

Comment by Alex Miller [ 27/Mar/14 11:00 AM ]

I think Rich meant that a ticket should have a plan of that form but does not. My own take on "triaged" is that it should state actual and expected results demonstrating a problem - I don't think it needs to actually describe the solution (as that can happen later in development). It is entirely possible that Rich and I differ in our interpretation of that. I will see if I can rework the description a bit to match what I've been doing elsewhere.

Comment by Andy Fingerhut [ 31/Mar/14 9:34 AM ]

Alex, I have looked through the existing wiki pages on the ticket tracking process, and do not recall seeing anything about this desired aspect of a triaged ticket. Is it already documented somewhere and I missed it? Not that it has to be documented, necessarily, but Rich saying "triage guidelines" makes it sound like a filter he applies that ticket creators and screeners maybe should know about.

Comment by Alex Miller [ 31/Mar/14 11:57 AM ]

To me, Triage (and Vetting) is all about having good problem statements. For a defect, it is most important to demonstrate the problem (what happens now) and what you expect to happen instead. I do not usually expect there to necessarily be "by ____" in the ticket - to me that is part of working through the solution (although it is typical to have this in an enhancement). This ticket, as it stands now, seems to have both a good problem statement and a good cause/solution statement so seems to exceed Triaging standards afaik.

Two places where I have tried to write about these things in the past are http://dev.clojure.org/display/community/Creating+Tickets and in the Triage process on the workflow page http://dev.clojure.org/display/community/JIRA+workflow.





[CLJ-1238] Allow EdnReader to read foo// (CLJ-873 for EdnReader) Created: 28/Jul/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: edn, reader

Attachments: Text File 0001-Fix-CLJ-873-for-EdnReader-too.patch     File clj-1238-2.diff    
Patch: Code
Approval: Ok

 Description   

The patch that has been applied in 1.6 for CLJ-873 predated the introduction of EdnReader, as such it only patched LispReader.

This patch makes the same change to allow foo// in EdnReader, and removes two constants in clojure.lang.RT that are not needed anymore after this patch.

To reproduce:

user=> (require 'clojure.edn)
user=> (defn / [& x] 42)
user=> (clojure.edn/read-string "(user// 4 2)")
RuntimeException Invalid token: user//  clojure.lang.Util.runtimeException (Util.java:219)

Approach: copy changes from LispReader in CLJ-873. After:

user=> (require 'clojure.edn)
nil
user=> (defn / [& x] 42)
WARNING: / already refers to: #'clojure.core// in namespace: user, being replaced by: #'user//
#'user//
user=> (clojure.edn/read-string "(user// 4 2)")
(user// 4 2)

Patch: 0001-Fix-CLJ-873-for-EdnReader-too.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 29/Jul/13 9:37 AM ]

Alex, this title might be misleading – the main purpose of this patch is to allow "foo//" to be read by clojure.edn/read

Comment by Alex Miller [ 30/Jul/13 3:12 PM ]

@Nicola - Please fix!

Comment by Nicola Mometto [ 30/Jul/13 6:28 PM ]

Done - I also changed the description to better describe what the patch actually does

Comment by Andy Fingerhut [ 25/Oct/13 12:27 PM ]

I've not looked into details yet, but screened 0001-Fix-CLJ-873-for-EdnReader-too.patch fails to apply cleanly as of Oct 25, 2013 latest Clojure master.

Comment by Alex Miller [ 25/Oct/13 2:05 PM ]

Updated patch to apply cleanly to master as of Oct 25, 2013.

Comment by Andy Fingerhut [ 31/Oct/13 4:37 PM ]

With the undoing of the change for CLJ-1252 earlier today, the patch 0001-Fix-CLJ-873-for-EdnReader-too.patch applies cleanly again, and clj-1238-2.diff no longer does.

Comment by Alex Miller [ 31/Oct/13 4:48 PM ]

Andy you are uncanny - I was just on my way to update this.

Comment by Alex Miller [ 31/Oct/13 4:51 PM ]

Switching back to prior patch.





[CLJ-1234] Accept whitespace in Record and Type reader forms Created: 24/Jul/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader

Attachments: File clj-1234-v1.diff     Text File clj-1234-v1.txt    
Patch: Code and Test
Approval: Ok

 Description   

Whitespace should be allowed in the record reader form as with tagged literals. Following example shows the problem:

user=> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user=> (defrecord B [x])
user.B
user=> (B. 2)
#user.B{:x 2}
user=> #user.B{:x 3}
#user.B{:x 3}
user=> #user.B {:x 3}
RuntimeException Unreadable constructor form starting with "#user.B "  clojure.lang.Util.runtimeException (Util.java:219)
user=> #inst "2013"
#inst "2013-01-01T00:00:00.000-00:00"
user=> #inst"2013"
#inst "2013-01-01T00:00:00.000-00:00"

Patch: clj-1234-v1.diff

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 25/Jul/13 2:40 PM ]

Stack trace in case it's useful:

user=> (pst *e)
ReaderException java.lang.RuntimeException: Unreadable constructor form starting with "#user.B "
clojure.lang.LispReader.read (LispReader.java:220)
clojure.core/read (core.clj:3407)
clojure.core/read (core.clj:3405)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn-589/fn-592 (interruptible_eval.clj:52)
clojure.main/repl/read-eval-print-6588/fn-6589 (main.clj:257)
clojure.main/repl/read-eval-print--6588 (main.clj:257)
clojure.main/repl/fn--6597 (main.clj:277)
clojure.main/repl (main.clj:277)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--589 (interruptible_eval.clj:56)
clojure.core/apply (core.clj:617)
clojure.core/with-bindings* (core.clj:1788)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate (interruptible_eval.clj:41)
Caused by:
RuntimeException Unreadable constructor form starting with "#user.B "
clojure.lang.Util.runtimeException (Util.java:219)
clojure.lang.LispReader$CtorReader.readRecord (LispReader.java:1224)
clojure.lang.LispReader$CtorReader.invoke (LispReader.java:1174)
clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:619)
clojure.lang.LispReader.read (LispReader.java:185)
clojure.core/read (core.clj:3407)

Comment by Andy Fingerhut [ 08/Sep/13 3:40 AM ]

Patch clj-1234-v1.txt adds a test that fails without the patch, and passes with it, verifying that white space is allowed after #record.name but before {.

Very straightforward patch, especially since fogus was nice enough to write 2 commented-out lines that did the trick simply by uncommenting them.

Comment by Alex Miller [ 08/Sep/13 3:14 PM ]

Any LispReader fix likely also affects EdnReader.

Comment by Nicola Mometto [ 08/Sep/13 4:57 PM ]

EdnReader doesn't read record/type literals so in this case it's not affected

Comment by Alex Miller [ 09/Sep/13 1:26 AM ]

Thanks for checking!





[CLJ-1233] Allow ** as a valid symbol name without triggering "not declared dynamic" warnings Created: 23/Jul/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Trivial
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler
Environment:

All


Attachments: File clj-1233-minimal.diff     File clj-1233-with-test.diff     File clj-1233-with-test-v2.diff     Text File clj-1233-with-test-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

Currently declaring a symbol ** triggers a compiler warning:

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

** is a useful symbol in many domains, e.g. as an exponent function or as a matrix multiplication operator.

Cause: This warning checks for a def of a symbol that starts and ends with *.

Solution: Change check for name length >2 to skip this particular case.

Patch: clj-1233-with-test-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Mike Anderson [ 23/Jul/13 6:09 AM ]

Link to discussion on Clojure-Dev
https://groups.google.com/forum/#!topic/clojure-dev/OuTMsZQkxN4

Comment by Mike Anderson [ 23/Jul/13 12:29 PM ]

Minimal patch for the ** case only

Comment by Alex Miller [ 24/Jul/13 12:17 PM ]

Minimal patch would be slightly less minimal with a test.

Comment by Mike Anderson [ 25/Jul/13 5:16 AM ]

Hmmm... is there a standard/reliable method for testing the presence / non-presence of emitted warnings?

Comment by Alex Miller [ 25/Jul/13 3:21 PM ]

There are some test helpers in Clojure's test/clojure/test_helper.clj for capturing error messages.

Comment by Mike Anderson [ 29/Jul/13 12:32 PM ]

New patch with a test that includes using "with-err-print-writer" to detect the avoidance of the warning.

Comment by Andy Fingerhut [ 05/Sep/13 6:24 PM ]

Patch clj-1233-with-test-v2.txt is identical to Mike Anderson's clj-1233-with-test.diff (preserving his authorship), except it avoids git warnings when applying by eliminating trailing whitespace in added lines.





[CLJ-1228] Fix typos in 4 namespaces and 2 docs Created: 01/Jul/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Gabriel Horner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File clj-1228-fix-multiple-typos-2.patch     Text File clj-1228-fix-multiple-typos.patch    
Patch: Code
Approval: Ok

 Description   

I used a spell checker I wrote for clojure projects, lein-spell, and found a few typos. No code changes.

Patch: clj-1228-fix-multiple-typos-2.patch

Screened by: Alex Miller



 Comments   
Comment by OHTA Shogo [ 01/Jul/13 8:07 PM ]

s/unnecesary/unnecessary/

Comment by Gabriel Horner [ 02/Jul/13 6:40 AM ]

@OHTA Shogo - Ha. Good catch.

Uploaded clj-1228-fix-multiple-typos-2.patch with that fix





[CLJ-1222] Multiplication overflow issues around Long/MIN_VALUE Created: 21/Jun/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: math

Attachments: File min_value_multiplication.diff    
Patch: Code and Test
Approval: Ok

 Description   

Long story short (ha!), there are several related issues where overflow checking doesn't work correctly with Long/MIN_VALUE. The cause of this missed check is that in Java, these both pass:

assertTrue(Long.MIN_VALUE * -1 == Long.MIN_VALUE);
assertTrue(Long.MIN_VALUE / -1 == Long.MIN_VALUE);

This causes the following results in Clojure that do not match their siblings where the order of operands is switched:

user=> (* Long/MIN_VALUE -1)
-922337203685477580 ;; should throw
user=> (*' Long/MIN_VALUE -1)
-9223372036854775808 ;; should be positive
user=> (* Long/MIN_VALUE -1N)
-9223372036854775808N ;; should be positive
user=> (* (bigint Long/MIN_VALUE) -1N)
-9223372036854775808N ;; should be positive

Mailing list discussion here: https://groups.google.com/d/msg/clojure-dev/o1QGR1QK70M/A1KykR9OQqwJ

Patch: min_value_multiplication.diff

Approach: Modifies BigInt.multiply() and multiply/multiplyP in Numbers to take into account Long.MIN_VALUE.

After:

user=> (* Long/MIN_VALUE -1)
ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1390)
user=> (*' Long/MIN_VALUE -1)
9223372036854775808N
user=> (* Long/MIN_VALUE -1N)
9223372036854775808N
user=> (* (bigint Long/MIN_VALUE) -1N)
9223372036854775808N

Screened by: Alex Miller (should also include CLJ-1225, CLJ-1253, CLJ-1254)



 Comments   
Comment by Andy Fingerhut [ 21/Jun/13 12:56 PM ]

Is there some proof that Long/MIN_VALUE and -1 is the only pair of values that causes a problem with the way overflow-checking is currently done? It might be, but it would be nice to be sure if a proposed patch relies on that.

Comment by Colin Jones [ 21/Jun/13 1:54 PM ]

I don't have a proof, just a half-hour or so of digging for an alternate case (found none), plus taking a look at what Guava does in checkedMultiply: https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/common/math/LongMath.java#522

I didn't do the leading zeros optimization they're doing to avoid even the division in the overflow check we already have; that felt like a bigger change, and not one that impacts correctness.

Comment by Andy Fingerhut [ 25/Jun/13 11:06 AM ]

I have created CLJ-1225 for the corresponding issues with / and quot with these arguments. I think that Long/MIN_VALUE and -1 should be the only arguments that cause problems for multiplication overflow detection, too, since it is based upon division, and I give some reasons in CLJ-1225 why I believe these are the only arguments that give the numerically wrong answer for the Java division operator / on longs.

Comment by Rich Hickey [ 22/Nov/13 7:28 AM ]

user=> (*' Long/MIN_VALUE -1)
9223372036854775808N

Is wrong - there should be no coercion, this is an overflowing operation.

Comment by Colin Jones [ 22/Nov/13 8:14 AM ]

Rich, can you clarify? My understanding is that *' is an auto-promoting operation. For instance, in 1.5.1, *' auto-promotes in the positive direction:

user=> (*' Long/MAX_VALUE Long/MAX_VALUE)
85070591730234615847396907784232501249N

and in the negative:

user=> (*' Long/MIN_VALUE -2)
18446744073709551616N

The only outlier is the one addressed in this ticket. Am I missing something? What should the result be here if not the one given by this patch?

Comment by Rich Hickey [ 22/Nov/13 10:14 AM ]

missed the ' earlier





[CLJ-1205] Update Maven build for Nexus 2.4 Created: 22/Apr/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Task Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-nexus-2.4-releases.patch    
Approval: Ok

 Description   

These additions to the build configuration are necessary to support changes to the Sonatype Nexus server at oss.sonatype.org, which we use to promote our build artifacts into the Maven Central Repository.

See Sonatype's announcement at https://groups.google.com/d/msg/clojure-dev/lBpfII2u6vM/LQvr_rO5UGgJ



 Comments   
Comment by Stuart Halloway [ 19/Jun/13 10:07 AM ]

Am I right in assuming that the only way we will know this works is by trying it in a release?

Comment by Stuart Sierra [ 19/Jun/13 10:51 AM ]

Yes.





[CLJ-1202] protocol fns with dashes may get compiled into property access when used higher order Created: 16/Apr/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

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

Attachments: Text File CLJ-1202.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Protocol functions with leading hyphens may be incorrectly compiled into field accesses.

Demonstration:

(defprotocol Foo (-foo [x]))

(deftype Bar [] Foo (-foo [_] "foo"))

(map -foo (repeatedly 3 ->Bar))
;; IllegalArgumentException No matching field found: foo for class user.Bar  
;; clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Cause: This was caused by CLJ-872, dash property support.

Solution: Emit forms like (. foo (bar)) instead of (. foo bar), so that names starting with a - don't look like field accesses.

Patch: CLJ-1202.patch



 Comments   
Comment by Alan Malloy [ 18/Apr/13 7:18 PM ]

Attached patch fixes the issue, and adds regression test for it.

Comment by Gabriel Horner [ 10/May/13 3:19 PM ]

Verified patch works

Comment by Stuart Sierra [ 02/Aug/13 2:21 PM ]

Screened and cleaned up description.

Comment by Alex Miller [ 08/Aug/13 1:07 AM ]

More cleansing.





[CLJ-1200] ArraySeq dead code cleanup and ArraySeq_short gap filling Created: 14/Apr/13  Updated: 31/Jan/14  Resolved: 31/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 6
Labels: None

Attachments: Text File no-getComponentType--v001.patch     Text File no-getComponentType--v002.patch    
Patch: Code
Approval: Ok

 Description   

The ArraySeq constructor and all methods retain support for primitive ArraySeq but that code has all been shunted into type-specific ArraySeq primitive array variants (ArraySeq_long, etc). The ArraySeq_short variant is currently missing.

Approach: Remove the vestigial primitive array code paths in ArraySeq and fields (ct and oa). This may provide a performance benefit but it has been hard to find a measurable impact.

The patch also fills in the missing ArraySeq_short variant. Before this patch, reduce on an array of primitive short would throw ClassCastException.

Patch: no-getComponentType--v002.patch
Screened by: Stuart Sierra



 Comments   
Comment by Zach Tellman [ 07/Jun/13 5:40 PM ]

As a data point, I was recently profiling a Hadoop job where the code made heavy use of 'partial', which apparently unlike 'comp' and 'juxt', always uses apply [1]. As a result, .getComponentType accounted for 41% of the overall compute time. This might be as much a comment on the implementation of partial as anything else, but it certainly shows that this can have a significant effect on performance.

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L2388

Comment by Kevin Downey [ 07/Jun/13 5:46 PM ]

prepRet usage appears to be all about enforcing canonical Boolean values, so I think using Object.class is not the best. Maybe splitting ArraySeq in to ArraySeq and ArraySeqBoolean would be better. ArraySeq would no longer do a prepRet and ArraySeqBoolean would

Comment by Kevin Downey [ 07/Jun/13 7:19 PM ]

ArraySeq actually already has specialized implementations, and interestingly the specialized boolean implementation doesn't call prepRet

Comment by Kevin Downey [ 07/Jun/13 7:35 PM ]

The ArraySeq split I proposed above will fail if you have an array of Objects that happen to be Booleans, it seems like the best bet would be something like preRet that did a instanceof Boolean check without the requirement of passing in a class

Comment by Brandon Bloom [ 15/Jul/13 11:43 AM ]

I traced the surrounding code & iirc, the result was always Object[], so the return value was always Object.class. I'm pretty sure that code wasn't actually doing anything useful here.

Comment by Brandon Bloom [ 17/Jul/13 4:54 PM ]

What does "triaged" mean in this context? Doesn't triage require some sort of decision or classification?

Comment by Andy Fingerhut [ 17/Jul/13 5:19 PM ]

See http://dev.clojure.org/display/community/JIRA+workflow for way more than the answer to your question (especially the flowchart in the "Workflow" section), but basically it means that a screener believes the ticket description represents an actual problem to be addressed, and they ask that Rich examine the ticket to see if he agrees.

Comment by Brandon Bloom [ 17/Jul/13 5:22 PM ]

Gotcha. Thanks.

Comment by Alex Miller [ 07/Oct/13 10:25 PM ]

The existing ArraySeq class has support for different two modes - as an array of Objects and as an array of primitives. In the Object case, this.oa will be set to the array and this.ct will be set to the component type of the array. From running a bunch of code, this.ct is not always Object - it is easy to find cases of Class and many other cases as well.

However, any kind of non-primitive array will cause this.oa to be set and this prevents the calls to prepRet from being called with ct in every method where this is done. All primitive arrays are now being handled via the switch in ArraySeq.createFromObject.

The only thing prepRet is effectively doing is returning canonical Boolean objects. It is possible to create an ArraySeq on a Boolean[]. However, even in this case, Boolean[] is an instanceof Object[] and the object path is triggered.

Thus, I agree that prepRet is never being called from ArraySeq and this path can be removed, which also allows us to remove this.ct and importantly the array.getClass().getComponentType() check. This also allows us to go further though and remove the oa field entirely and all of the prepRet calls.

I have attached an updated patch that makes this change. I also found (based on some test failures) that the ArraySeq_short was inexplicably missing so I added that in as well.

ArraySeq could be made even cleaner with the addition of another variant that specifically handled the case of a null array (ArraySeq_null). I have no idea if that would be worth doing and I have not done that here.

Comment by Brandon Bloom [ 08/Oct/13 9:43 AM ]

Hey Alex: Thanks for following through on the vestiges removal.

Comment by Rich Hickey [ 22/Nov/13 1:44 PM ]

please add perf tests that demonstrate benefit

Comment by Alex Miller [ 17/Dec/13 11:40 AM ]

Removed performance angle and focus on clean-up and gap-filling (ArraySeq_short) aspects instead.

Comment by Alex Miller [ 11/Jan/14 8:54 AM ]

Point to considered patch, this got lost at some point in the description.





[CLJ-1193] bigint, biginteger throw on double values outside of long range Created: 07/Apr/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1197-make-bigint-work-on-all-doubles-v1.txt    
Patch: Code and Test
Approval: Ok

 Description   

The bigint and biginteger functions throw on double values outside of long range

This works fine:

user> (bigint (* 0.99 Long/MAX_VALUE))
9131138316486227968N

but passing any Float or Double values outside the range of a long throw an exception:

user> (bigint (* 1.01 Long/MAX_VALUE))
IllegalArgumentException Value out of range for long: 9.315605757223324E18  clojure.lang.RT.longCast (RT.java:1178)

Cause: bigint and biginteger cover a series of possible input cases but did not have an explicit case for Float or Double, so was falling back to default.

Solution: Add check for Float/Double case that coerces the input to double, then uses BigDecimal.valueOf(), then converts to a BigInteger and so on as with other types.

Patch: clj-1197-make-bigint-work-on-all-doubles-v1.txt



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 4:38 PM ]

Patch clj-1197-make-bigint-work-on-all-doubles-v1.txt dated Apr 7 2013 changes bigint and biginteger so that they return the correct value for all float and double values, and no longer throw an exception.

Comment by Gabriel Horner [ 17/May/13 10:52 AM ]

Looks good. Tests pass and the failing example now converts correctly

Comment by Alex Miller [ 08/Aug/13 1:51 AM ]

Cleaned up the description a bit.





[CLJ-1191] Improve apropos to show some indication of namespace of symbols found Created: 04/Apr/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5, Release 1.6
Fix Version/s: Release 1.7

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: repl

Attachments: Text File clj-1191-patch-v1.txt     Text File clj-1191-patch-v2.txt     Text File clj-1191-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

apropos does find all symbols in all namespaces that match the argument, but the return value gives no clue as to which namespace the found symbols are in. It can even return multiple occurrences of the same symbol, which only gives a clue that the symbol exists in more than one namespace, but not which ones. For example:

user=> (apropos "replace")
(postwalk-replace prewalk-replace replace re-quote-replacement replace replace-first)

user=> (apropos 'macro)
(macroexpand-all macroexpand macroexpand-1 defmacro)

It would be nice if the returned symbols could indicate the namespace.

With the screened patch clj-1191-v3.patch applied the output for the examples above becomes:

user=> (apropos "replace")
(clojure.core/replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (apropos 'macro)
(clojure.core/defmacro clojure.core/macroexpand clojure.core/macroexpand-1 clojure.walk/macroexpand-all)

Patch: clj-1191-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Apr/13 8:25 PM ]

Path clj-1191-patch-v1.txt enhances apropos to put a namespace/ qualifier before every symbol found that is not in the current namespace ns. It also finds the shortest namespace alias if there is more than one. Examples of output with patch:

user=> (apropos "replace")
(replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (require '[clojure.string :as str])
nil
user=> (apropos "replace")
(replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace str/re-quote-replacement str/replace str/replace-first)

user=> (in-ns 'clojure.string)
#<Namespace clojure.string>
clojure.string=> (clojure.repl/apropos "replace")
(re-quote-replacement replace replace-by replace-first replace-first-by replace-first-char replace-first-str clojure.core/replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

Comment by Colin Jones [ 05/Apr/13 1:34 PM ]

+1

apropos as it already stands is quite helpful for already-referred vars, but not for vars that are only in other nses.

This update includes the information someone would need to further investigate the output.

Comment by Alex Miller [ 20/Aug/14 11:22 AM ]

If you have "use"d many namespaces (which is not uncommon at the repl), this updated apropos still doesn't help you understand where a particular function is coming from (as the ns will be omitted). It's cool that this patch is "unresolving" and finding the shortest-alias etc but I think it's actually doing too much. In my opinion, simply providing the full namespace for all matches would actually be more useful (and easier).

Comment by Andy Fingerhut [ 20/Aug/14 12:27 PM ]

Patch clj-1191-patch-v2.txt dated Aug 20 2014 modifies apropos so that every symbol returned has a full namespace qualifier, even if it is in clojure.core. Before this patch:

user=> (apropos "replace")
(prewalk-replace postwalk-replace replace replace-first re-quote-replacement replace)

user=> (apropos 'macro)
(macroexpand-all macroexpand macroexpand-1 defmacro)

After this patch:

user=> (apropos "replace")
(clojure.core/replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (apropos 'macro)
(clojure.core/defmacro clojure.core/macroexpand clojure.core/macroexpand-1 clojure.walk/macroexpand-all)
Comment by Alex Miller [ 20/Aug/14 1:34 PM ]

Some comments on the code itself:

1) I don't think we should do anything special for ns - there are plenty of ways to search your current ns. I think it unnecessarily adds a lot of complexity without enough value.
2) Rather than finding vars and work back to syms, I think this should instead retain the ns context as it walks the ns-publics keys so that you can easily reassemble a fully-qualified symbol name.
3) Why do you need the set at the end? Seems like symbols should already be unique at this point?

Comment by Andy Fingerhut [ 20/Aug/14 5:02 PM ]

Patch clj-1191-patch-v3.txt attempts to address Alex Miller's comments on the v2 patch.

Perhaps the diff will get down to a 1-line change

Comment by Andy Fingerhut [ 20/Aug/14 5:05 PM ]

Patch clj-1191-v3.patch is identical to clj-1191-patch-v3.txt mentioned in the previous comment, but conforms to the requested .patch or .diff file name ending.





[CLJ-1190] Javadoc for public Java API Created: 03/Apr/13  Updated: 11/Jan/14  Resolved: 11/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Enhancement Priority: Critical
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: documentation

Attachments: File clj-1190-2.diff     File clj-1190-3.diff     Text File clj-1190.patch    
Patch: Code
Approval: Ok

 Description   

Publish javadoc for http://dev.clojure.org/jira/browse/CLJ-1188.

  • Add javadoc for public API classes (Clojure and IFn)
  • Add ant build target to generate the javadoc
  • Publish javadoc to the clojure project github pages (will be done manually)

Patch: clj-1190-3.diff (javadoc updates and a new Ant target)

Screened by: Stuart Halloway

Updated by: Alex Miller (moved clojure.api.API to clojure.java.api.Clojure and tweaked one example per Rich's suggestion)



 Comments   
Comment by Andy Fingerhut [ 18/Aug/13 3:57 AM ]

It seems that when this ticket is completed, it may also complete CLJ-19, too.

Comment by Alex Miller [ 04/Oct/13 1:46 PM ]

Maven javadoc is something like this (configuration as in http://maven.apache.org/plugins/maven-javadoc-plugin/javadoc-mojo.html):

<plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-javadoc-plugin</artifactId>
        <version>2.9.1</version>
        <configuration>
          <show>public</show>
          <verbose>true</verbose>
          <sourceFileIncludes>
            <include>API.java</include>
          </sourceFileIncludes>
          <sourcepath>${basedir}/src/jvm/clojure/api</sourcepath>
        </configuration>
      </plugin>

What is desired for inclusion in the javadoc? Just the clojure.api package and API class? If so is it worth automating the publishing of this API or just documenting a process to put someplace static?

Comment by Alex Miller [ 21/Oct/13 10:14 PM ]

Javadoc can be told what to doc either by package or by java source file. The Maven plugin only supports the source file version if the files are in the same directory (as far as I can tell).

Ant was a little easier to get going:

<target name="javadoc" depends="build"
         description="Creates javadoc for Clojure API.">
    <javadoc destdir="javadoc">
      <classpath>
        <path location="${build}"/>
      </classpath>
      <fileset dir="src/jvm">
        <include name="clojure/api/API.java"/>
        <include name="clojure/lang/IFn.java"/>
      </fileset>
    </javadoc>
  </target>

However, there is no way to omit the nested IFn primitive interfaces in either case. That's just not a thing. Presumably it would be possible to customize the standard doclet to omit whatever you wanted although it makes me throw up in my mouth a little to even suggest that as a necessity.

Comment by Stuart Halloway [ 22/Oct/13 9:12 AM ]

Including nonpublic interfaces is worse than having no Javadoc at all, IMO. We plan to change this API almost never, so one possibility it to just generate from a copy of the IFn source file that omits the primitives.

API should have a class level document.

The goal here is to publish the official Javadoc on the web. Following the javadoc jar convention is a potential separate goal, but out of scope here.

Comment by Alex Miller [ 22/Oct/13 9:36 AM ]

When I next get a chance I'll take a look at what customizing the standard doclet would take. If it looks ugly I'll bail out.

Comment by Alex Miller [ 22/Oct/13 9:43 PM ]

Modifying the standard doclet (a mystery shrouded in a thicket of Oracle docs) to generate javadoc for 2 classes is ridiculous. As Stu suggested, I just hacked the inner interfaces out of IFn and generated the attached javadoc with the ant target prior in the comments. I think we could probably turn off a few more of the bits and improve the title,etc then manually publish this set of files in the github pages somewhere.

Comment by Alex Miller [ 23/Oct/13 8:48 PM ]

Added an Ant build target that strips the inner interfaces of IFn and generates the javadoc. Still need to add class javadoc for both API and IFn.

Comment by Alex Miller [ 23/Oct/13 10:49 PM ]

Added a new patch that adds javadoc for the clojure.lang package, the IFn and API classes. It also has an Ant task that generates the javadoc in target/javadoc which you can run with ant javadoc.

Comment by Alex Miller [ 21/Nov/13 3:32 PM ]

The API will live here (prelim version there now): http://clojure.github.io/clojure/javadoc/

Comment by Rich Hickey [ 22/Nov/13 10:37 AM ]

We should never show things like (lookup and simultaneously use var):

API.var("clojure.core", "deref").invoke(printLength);

because people will put that in a loop.

Also, API.var seems like a lot of CAPS. I understand that's proper Java conventions but maybe we need to pick another name?

Comment by Alex Miller [ 05/Dec/13 10:58 AM ]

At Stu's request, I made the updates from our prior conversation, namely:
1) Moved clojure.api.API to clojure.java.api.Clojure and updated all references.
2) Made the example change suggested by Rich to avoid creating and invoking the var in one step.

Comment by Alex Miller [ 07/Dec/13 4:22 PM ]

Andy, please let me know before modifying screened patches as they need to be re-screened.

Comment by Alex Miller [ 07/Dec/13 4:23 PM ]

In this case, it looks like you didn't leave the old one so I'm not even sure what's different?

Comment by Alex Miller [ 07/Dec/13 4:24 PM ]

Oh, I'm misreading the comment history, you just changed the patch field. Carry on.





[CLJ-1185] `reductions should respect `reduced Created: 16/Mar/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.7

Type: Defect Priority: Critical
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File CLJ-1181-v001.patch     Text File CLJ-1181-v002.patch    
Patch: Code and Test
Approval: Ok

 Description   

This returns 16:

(reduce (fn [acc x]
          (let [x' (* x x)]
            (if (> x' 10)
              (reduced x')
              x')))
        (range))

But replacing reduce with reductions will never terminate:

(reductions (fn [acc x]
              (let [x' (* x x)]
                (if (> x' 10)
                  (reduced x')
                  x')))
            (range))

Cause: reductions ignores clojure.lang.Reduced, it never tests for reduced?

Patch: CLJ-1181-v002.patch

Screened by: Alex Miller



 Comments   
Comment by Brandon Bloom [ 16/Mar/13 6:10 PM ]

Attaching patch

Comment by Satshabad Khalsa [ 13/Apr/14 1:53 AM ]

Would love some progress on this!

Comment by Andy Fingerhut [ 13/Apr/14 11:37 AM ]

It isn't guaranteed to help, but it can't hurt to vote on the ticket, and encourage anyone else you know who wants this fixed to vote on it.

Comment by Alex Miller [ 14/Jun/14 7:38 AM ]

Needs a test

Comment by Brandon Bloom [ 14/Jun/14 4:10 PM ]

New patch includes tests.





[CLJ-1184] Evaling #{do ...} or [do ...] is treated as the do special form Created: 16/Mar/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Trivial
Reporter: Jiří Maršík Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler

Attachments: Text File CLJ-1184-p1.patch     Text File CLJ-1184-p2.patch     Text File CLJ-1184-p3.patch     Text File CLJ-1184-p4.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Evaluating a persistent collection for which the function first returns the symbol do leads to that collection being treated as the do special form, even though it may be a vector or even a set. IMHO, the expected result would be to report that do cannot be resolved.

[do 1 2]
;=> 2

#{"hello" "goodbye" do}
;=> "hello"
; Wat?

Cause: The check for do is checking for IPersistentCollection instead of ISeq.

Solution: Change the cast (occurs in two places) for the do form check from IPersistentCollection to ISeq:

if(form instanceof IPersistentCollection && Util.equals(RT.first(form), DO))

to

if(form instanceof ISeq && Util.equals(RT.first(form), DO))

Current patch: CLJ-1184-p4.patch

Screened by: Alex Miller



 Comments   
Comment by Gary Fredericks [ 26/May/13 2:13 PM ]

Attached a patch that changes IPersistentCollection to ISeq on the referenced line, and a regression test.

Comment by Nicola Mometto [ 09/Jun/13 2:52 PM ]

As found out on #clojure, there are still some cases where the symbol "do" behaves in unexpected ways that this patch doesn't address.

user=> ((fn [do] do) 1)
nil
user=> (let [do 1] do)
nil

Comment by Gary Fredericks [ 09/Jun/13 3:00 PM ]

Presumably the same issue is the difference between

(let [x 5] do x)

which returns 5 and

(let [x 5] do do x)

which gives a compile error.

Comment by Nicola Mometto [ 09/Jun/13 4:31 PM ]

This is actually a different bug.
I created another ticket with patch+test see http://dev.clojure.org/jira/browse/CLJ-1216

Comment by Alex Miller [ 02/Jul/13 7:11 PM ]

There is a similar case that shows up in Compiler.compile1() around line 7139. Should this also change?

Whatever case that is, would also be nice to have a test for it too.

Comment by Gary Fredericks [ 02/Jul/13 8:59 PM ]

Good catch; this one's a bit trickier to test, since you either have to have a resource on the classpath to compile using clojure.core/compile, or else call the lower-level Compiler/compile directly. To keep the filesystem clean I'm opting for the second one. Path coming shortly.

Comment by Gary Fredericks [ 02/Jul/13 9:52 PM ]

Attached a replacement patch with the second fix. For testing I couldn't call Compiler.compile directly without getting an NPE (that I couldn't reproduce at the repl), so instead I opted to write to a file, use clojure.core/compile, and cleanup the files inside the test.

Comment by Andy Fingerhut [ 05/Jul/13 3:06 PM ]

Presumptuously changing back to its former Vetted state, since the latest patch seems to address the reasons it was marked Incomplete on July 2, 2013.

Comment by Alex Miller [ 23/Jul/13 10:49 PM ]

Updated patch very slightly to fix a spelling typo.

Comment by Alex Miller [ 23/Jul/13 10:55 PM ]

Updated description to help it along a bit and marked Screened.

Comment by Andy Fingerhut [ 14/Aug/13 7:55 PM ]

All 3 of the attached patches no longer apply cleanly to latest master as of Aug 14 2013. This may simply be due to extra tests added to file compilation.clj by the patch for CLJ-1154, which was committed earlier today. If so, it should be pretty straightforward to update the stale patch(es). See the section "Updating Stale Patches" at http://dev.clojure.org/display/community/Developing+Patches

Comment by Gary Fredericks [ 14/Aug/13 9:23 PM ]

Attached a new patch (p4) that should apply. Also halfway reverted a change that Alex made regarding which files are cleaned up after the test. When I run the tests on my machine with his version, several class files are leftover.

Comment by Alex Miller [ 17/Aug/13 7:50 AM ]

Screened.





[CLJ-1177] java.io URL to File coercion and encoding of non-ASCII characters Created: 07/Mar/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Trevor Wennblom Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: io

Attachments: Text File clj-1177-patch-v1.txt     File clj-1177-patch-v2.diff     Text File clj-1177-patch-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

clojure.java.io/resource corrupts path containing UTF-8 characters without issuing warning. (The behavior in the example below is not specific to JDK 8 or Clojure 1.5.0. It is seen with the latest Clojure master as of Sep 15, 2013, and with JDK 6 and JDK 7.)

user=> (System/getProperty "java.runtime.version")
"1.8.0-ea-b79"
user=> (clojure-version)
"1.5.0"
user=> (System/getProperty "user.dir")
"/dir/déf"
user=> (clojure.java.io/resource "myfile.txt")
#<URL file:/dir/d%c3%a9f/resources/myfile.txt>
user=> (slurp (clojure.java.io/resource "myfile.txt") :encoding "UTF-8")
FileNotFoundException /dir/déf/resources/myfile.txt (No such file or directory)  java.io.FileInputStream.open (FileInputStream.java:-2)

Analysis:

The implementation of method as-file of protocol Coercions for class java.net.URL transforms each occurrence of '%xy', where x and y are hex digits in ASCII, to a separate character in the result. The correct behavior is to treat sequences of more than one '%xy' as a byte sequence encoded in UTF-8, where single Unicode code points (i.e. 'Unicode characters') are encoded with anywhere from 1 to 4 bytes.

Patch: clj-1177-patch-v2.diff

Approach:

Change method as-file for class java.net.URL to use method java.net.URLDecoder.decode to decode the contents of a URL string.

http://docs.oracle.com/javase/6/docs/api/java/net/URLDecoder.html#decode%28java.lang.String,%20java.lang.String%29

The only issue with java.net.URLDecoder.decode's behavior is that it changes plus-sign characters to spaces, which according to at least one of the existing unit tests should not happen in as-file. To work around this, first explicitly encode any plus-sign characters in the given URL string, using method java.net.URLEncoder.encode. After that, pass the result to method decode.

http://docs.oracle.com/javase/6/docs/api/java/net/URLEncoder.html#encode%28java.lang.String,%20java.lang.String%29

Other approaches:

Patch clj-1177-patch-v1.txt represents an alternate approach that does its own 'unescaping' of UTF-8 encoded URL strings, without relying on class java.net.URLDecoder. As a result, it is longer and more detailed.

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 08/Mar/13 12:10 AM ]

Below is a workaround, at least. I don't know, but perhaps the as-file method for URLs in io.clj of Clojure, the part that converts %hh sequences to a character with code point in the range 0 through 255, is at least partly at fault here. I don't know right now if it is possible to modify that code to handle the general case of whatever character encoding munging is going on here to when .getResource creates the URL object.

clojure.java.io/resource is documented to return a Java object of type java.net.URL, which seems like it does %hh escaping of many characters. Reference [1] to a Java bug from 2001 where a Java user was surprised by the then-recent change in behavior of the getResource method [2].

Doing a little searching I found this StackOverflow question [3], which has what might be a workaround. I tried it on my Mac OS X 10.6 system running JDK 1.6 and it seemed to work:

(slurp (.getContent (clojure.java.io/resource "abcíd/foo.txt")))

That getContent is a method for class java.net.URL [4]

[1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4466485
[2] http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/Class.html#getResource%28java.lang.String%29
[3] http://stackoverflow.com/questions/13013629/best-international-alternative-to-javas-getclass-getresource
[4] http://docs.oracle.com/javase/1.5.0/docs/api/java/net/URL.html#getContent%28%29

Comment by Trevor Wennblom [ 08/Mar/13 9:56 AM ]

Hi Andy,

Thanks for the background and suggestions, that's very helpful.

I'm gradually learning Clojure with no Java experience. In this case I was searching for the preferred Clojure way to access items in directories declared under :resource-paths in a Leiningen project.clj file. Perhaps clojure.java.io/resource isn't the best way to do this as it's possibly too tied to the expectation for a URI instead of a more general IRI.

You're suggested workaround did work for my use case:

(slurp (.getContent (clojure.java.io/resource "abcíd/foo.txt")))

but hopefully there would be more native/direct Clojure way to accomplish the same eventually.

I don't know if java.net.IDN would be useful internally as a fix in clojure.java.io/resource — I'm assuming not since it wasn't added until Java 6.[1]

user=> (import 'java.net.IDN)
java.net.IDN
user=> (java.net.IDN/toASCII "/dir/déf")
"xn--/dir/df-gya"
user=> (java.net.IDN/toUnicode "xn--/dir/df-gya")
"/dir/déf"

[1]: http://docs.oracle.com/javase/6/docs/api/java/net/IDN.html

Comment by Andy Fingerhut [ 08/Mar/13 1:30 PM ]

Patch clj-1177-patch-v1.txt dated Mar 8 2013 is an attempt to solve this issue, in what I think may be a correct way. As specified in RFC 3986, when taking a Unicode string and making a URL of it, it should be encoded in UTF-8 and then each individual byte is subject to the %HH hex encoding. This patch reverses that to turn URLs into file names.

Tested on Mac OS X 10.6 with a command line like this (it doesn't work without the -Dfile.encoding=UTF-8 option on my Mac, probably because the default encoding is MacRoman):

% java -cp clojure.jar:path/to/resource -Dfile.encoding=UTF-8 clojure.main
user=> (require '[clojure.java.io :as io])
nil
user=> (io/resource "abcíd/foo.txt")
#<URL file:/Users/jafinger/clj/clj-ns-browser/resource/abc%c3%add/foo.txt>
user=> (slurp (io/resource "abcíd/foo.txt"))
"The quick brown fox jumped over the lázy dög!\n"

Comment by Alex Miller [ 24/Jul/13 10:08 PM ]

I think the original code and all of these suggestions are missing more obvious answers already in the JDK (and better).

1. URLs can be converted to URIs which can be passed to the File constructor:

(java.io.File. (.toURI (io/resource "abcíd/foo.txt")))

2. Or we could also leverage URLDecoder instead of that nasty escaping mess currently in the code.

(java.io.File. 
  (URLDecoder/decode 
    (.getFile (io/resource "abcíd/foo.txt")) 
    "UTF-8")))
Comment by Alex Miller [ 24/Jul/13 10:41 PM ]

One big caveat: the alternatives I gave above only work for absolute URLs. Relative URLs would need some massaging. I think to cover those, #2 would be better as it gives you a hook to look at the output of getFile and decide whether it's relative.

Comment by Andy Fingerhut [ 25/Jul/13 8:46 PM ]

On my system (Mac OS X 10.8.4, JVM 1.7.0_15):

#1 has the same problem of munging characters as the current code does. At least, I got errors trying to open a file with an accented "a" in it, because it tried to open a file with a name that had two characters in place of the accented "a".

#2 is better, but it fails with one of the tests that calls (clojure.java.io/as-file (URL. "file:bar+baz")). With your version #2, URLDecoder/decode changes the plus to a space, and the test comparison to the expected result of (File. "bar+baz") fails. I don't know if that is a good test or not, but if it is, the documentation I read for URLDecoder/decode suggests that it will always change plus to space, regardless of whether it is an absolute or relative URL.

Comment by Andy Fingerhut [ 01/Sep/13 10:51 AM ]

Patch clj-1177-patch-v2.txt dated Sep 1 2013 uses URLDecoder/decode to do the decoding of the URL, but only after encoding any plus signs in the URL first, so that they remain plus signs in the returned file name, and are not changed to spaces.

This patch also adds one new test for as-file.





[CLJ-1176] clojure.repl/source fails when *read-eval* bound to :unknown Created: 06/Mar/13  Updated: 31/Jan/14  Resolved: 31/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: repl

Attachments: Text File 0001-CLJ-1176-Bind-read-eval-true-in-clojure.repl-source-.patch     Text File clj-1176-source-read-eval-2.patch     Text File clj-1176-source-read-eval-3.patch    
Patch: Code and Test
Approval: Ok

 Description   

clojure.repl/source is broken in Clojure 1.5.1 when *read-eval* is bound to :unknown, since source-fn reads without binding.

user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown  clojure.lang.Util.runtimeException (Util.java:219)

Approach: Throw explicit error stating the cause in this case.

Patch: clj-1176-source-read-eval-3.patch

Screened by: Stuart Sierra



 Comments   
Comment by Tim McCormack [ 06/Mar/13 4:04 PM ]

The attached patch just binds *read-eval* to true inside source-fn.

Comment by Stuart Halloway [ 29/Mar/13 6:24 AM ]

Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.

Comment by Tim McCormack [ 29/Mar/13 6:37 AM ]

Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.

Comment by Tim McCormack [ 04/May/13 10:40 PM ]

I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.

Comment by Gabriel Horner [ 24/May/13 9:42 AM ]

Looks good

Comment by Alex Miller [ 18/Aug/13 2:55 PM ]

Would like to screen this one again. I think it has open questions and is worth a discussion somewhere.

Comment by Alex Miller [ 11/Oct/13 4:47 PM ]

To me, this seems like we would be opening a security hole and a cleverly concocted resource could exploit it.

Other alternatives:
1) do nothing (user can always bind read-eval around a call to source if they want to do this safely)
2) add a source-unsafe or other wrapper function that did this
3) change source-fn to use edn/read instead? This may introduce some cases where source using non-edn features could not be read. I'd be ok with that.

Comment by Andy Fingerhut [ 16/Oct/13 8:24 PM ]

Maybe this is well known to everyone already, but in case not, doing a require or use on a namespace containing the following function on a Unix-like system to create and/or update the last modification time of the file bartleby.txt. If you remove that file, and then do (source badfn) while read-eval is bound to true, you can see that it will do the shell command again. Obviously much more dangerous side effects could be performed instead of that.

(ns bad.fn)

(defn badfn [x]
  (let [y [#=(clojure.java.shell/sh "touch" "bartleby.txt")]]
    x))

Avoiding that behavior in source-fn, yet still showing the source code, would require a different implementation of read other than clojure.core/read and clojure.edn/read.

Comment by Alex Miller [ 17/Oct/13 8:21 AM ]

Based on comments on the mailing list, most people are not concerned about this from a security point of view. I'm going to let this one through and Rich can decide further.

Comment by Rich Hickey [ 25/Oct/13 7:20 AM ]

If you haven't set read-eval and you need to read-eval, then you'd better set it, right? We're not going to do that for you. The only patch that will be accepted for this is one that generates a better error message.

Comment by Alex Miller [ 29/Dec/13 10:47 PM ]

Updated with new patch that detects and throws an error if calling source with read-eval is false.

Comment by Stuart Sierra [ 10/Jan/14 4:37 PM ]

Screened OK.

Note that this only changes the error message when read-eval is bound to false, not when it is bound to :unknown.

Comment by Stuart Sierra [ 10/Jan/14 4:44 PM ]

Changed my mind: resetting to 'incomplete'.

This patch doesn't fix the situation in the original report. To improve the error message, it should handle the :unknown case.

If *read-eval* is false, then source still works as long as the source form doesn't contain #=

Comment by Alex Miller [ 14/Jan/14 9:01 AM ]

Stuart - totally good catch. Things did not work how I thought they worked! I have updated the patch.

Comment by Stuart Sierra [ 17/Jan/14 9:44 AM ]

Screened ✔





[CLJ-1175] NPE in clojure.lang.Delay/deref Created: 06/Mar/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Stuart Halloway
Resolution: Completed Votes: 3
Labels: None

Attachments: Text File delayed-exceptions.patch    
Patch: Code and Test
Approval: Ok

 Description   

Delays get into a corrupt state if an exception is thrown, allowing deref to behave differently on subsequent calls.

This can manifest in multiple ways, depending on the expression being delayed:

;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d) 
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero> 
 #<NullPointerException java.lang.NullPointerException>]
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d) 
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date> 
 nil]

Cause: clojure.core/delay creates a ^:once function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

Solution: Make Delay remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception e being thrown, and so this should happen every time.

Patch: delayed-exceptions.patch



 Comments   
Comment by Alex Miller [ 08/Aug/13 2:07 AM ]

Cleaned up description.





[CLJ-1171] Compiler macro for clojure.core/instance? disregards lexical shadows on class names Created: 27/Feb/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1171-Tests-for-clojure.core-instance-compiler-ma.patch     Text File 0002-CLJ-1171-Obey-lexical-scope-for-class-argument-in-in.patch     Text File 0003-CLJ-1171-Check-arity-in-instance-compiler-macro.patch    
Patch: Code and Test
Approval: Ok

 Description   

Summary

The compiler tries to emit jvm native instanceof expressions for direct clojure.core/instance? calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.
This is incongruent to the default implementation in core.clj

Patches

[Stu] All three patches should be applied IMO.

  • 0002 makes instance? respect lexical bindings
  • 0003 makes instance?'s compiled form check arity, consistent with higher-order behavior
  • 0001 has a minimal test for both 0002 and 0003.

Data

Test case

user=> (let [Long String] (instance? Long "abc"))
false
;; expected true as in
user=> (let [Long String] (apply instance? [Long "abc"]))
true

Culprit method

https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

List Discussion

https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion

Tangent

This was discovered because the same compiler macro also omits the arity check implicit in the default definition. This could also conveniently be fixed when touching that method:

user=> (instance? String)
false
;; expected
user=> (apply instance? [String])
ArityException Wrong number of args (1) passed to: core$instance-QMARK-  clojure.lang.AFn.throwArity (AFn.java:437)

EDIT elaborated on ticket title and description; added tangent



 Comments   
Comment by Herwig Hochleitner [ 27/Feb/13 8:11 PM ]

Attached patches test and fix issue + tangent

Comment by Herwig Hochleitner [ 04/Mar/13 3:51 PM ]

Note: Patch 0003 just adds the arity check, hence is optional, but if it's omitted from the patchset, the corresponding test from patch 0001 will fail.

Comment by Stuart Halloway [ 29/Mar/13 7:31 AM ]

Summarizing the decisions in these patches:

  • instance? and apply instance? should be consistent
  • they should check arity (matching apply instance? existing behavior)
  • they should allow lexical shadowing of the class argument (matching apply instance? existing behavior)

It is possible (although unlikely) that existing code relies on the current eccentric behavior of instance?. I think it would be fair to categorize programs relying on this behavior as buggy, but that is easy for me to say.





[CLJ-1169] Report line,column, and source in defmacro errors Created: 22/Feb/13  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.7

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     Text File CLJ-1169-code-and-test-1.patch     File defn_error_message.clj    
Patch: Code
Approval: Ok

 Description   

Summary This patch grew out of a desire to have defn report filename and line numbers for parameter declaration errors, but the approach chosen does something more broad, and likely very useful: Anytime defmacro is throwing a non-CompilerException, wrap it in a CompilerException that captures LINE, COLUMN, and SOURCE. Presumably this would improve reporting for many other macros as well. The patch also tweaks errors messages to add quotes, e.g. "problem" instead of problem, which seems useful.

Screened By Stu
Patch CLJ-1169-code-and-test-1.patch, which aggregates the work in other patches to a single patch that works on current master.

When mistyping parameter list in defn declaration, e.g.

(defn test
 (some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
        at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
        at clojure.core$sigs.invoke(core.clj:220)
        at clojure.core$defn.doInvoke(core.clj:294)
        at clojure.lang.RestFn.invoke(RestFn.java:467)
        at clojure.lang.Var.invoke(Var.java:427)
        at clojure.lang.AFn.applyToHelper(AFn.java:172)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
        at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
        at clojure.lang.Compiler.eval(Compiler.java:6495)
        at clojure.lang.Compiler.load(Compiler.java:6952)
        at clojure.lang.Compiler.loadFile(Compiler.java:6912)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$init_opt.invoke(main.clj:288)
        at clojure.main$initialize.invoke(main.clj:316)
        at clojure.main$null_opt.invoke(main.clj:349)
        at clojure.main$main.doInvoke(main.clj:427)
        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)


 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests

Comment by Stuart Halloway [ 27/Jun/14 2:40 PM ]

Andrei, can you please sign the CA (e-form at http://clojure.org/contributing) so that we can consider this patch?

Thanks!

Comment by Andrei Kleschinski [ 27/Jun/14 3:05 PM ]

Ok, I have signed the CA.

Comment by Alex Miller [ 27/Jun/14 4:06 PM ]

I can confirm that Andrei has signed the CA. Back in Vetted.





[CLJ-1168] Setting clojure.read.eval to unknown on JVM cmd line causes --eval option to fail Created: 19/Feb/13  Updated: 22/Feb/13  Resolved: 22/Feb/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1168-patch-v1.txt     Text File CLJ-1168-with-read-known.patch    
Patch: Code
Approval: Ok

 Description   

First discovered by Tim McCormack with Clojure 1.5.0-beta13 and -RC15:

$ java -Dclojure.read.eval=unknown -jar ~/.m2/repository/org/clojure/clojure/1.5.0-RC16/clojure-1.5.0-RC16.jar -e "(+ 1 2)"
Exception in thread "main" java.lang.RuntimeException: Reading disallowed - read-eval bound to :unknown

This would prevent a Leiningen user from putting the following line in their project.clj file in order to look for read or read-string calls that don't explicitly bind read-eval to true or false:

:jvm-opts ["-Dclojure.read.eval=unknown"]



 Comments   
Comment by Andy Fingerhut [ 19/Feb/13 7:42 PM ]

Patch clj-1168-patch-v1.txt dated Feb 19 2013 has been tested manually with these results:

% java -cp clojure.jar clojure.main

(ran some simple tests to ensure repl still works, and obeys -Dclojure.read.eval=unknown setting)

% java -Dclojure.read.eval=unknown -jar clojure.jar -e '(println (+ 1 2))'
3

Comment by Steve Miner [ 20/Feb/13 4:52 PM ]

The patch works for me, but I think it would be safer for the patch to treat *read-eval* :unknown as false for the purpose of -e. It's unlikely that anyone wants to -e '#=(boom)'. Also, there's no need for the let to capture the cur-read-eval. I suggest something like this for the patch (updated):

(defn- read-never-unknown [read-fn & args]
      (binding [*read-eval* (true? *read-eval*)]
         (apply read-fn args)))
Comment by Andy Fingerhut [ 21/Feb/13 6:10 AM ]

I have asked on the Leiningen email list whether treating read-eval :unknown as false for the purpose of -e would break Leiningen functionality, and will report back here when I find out.

Comment by Andy Fingerhut [ 21/Feb/13 3:02 PM ]

I don't have an answer from Leiningen folks yet, but I do have another thought related to Steve's comment:

The current behavior is to treat *read-eval* as true for the purposes of reading lines in the REPL, if -Dclojure.read.eval=unknown was specified on the command line. Why should expressions given after -e be any more restricted? Whoever is invoking the JVM has complete control.

If they really want *read-eval* to be false when reading the expressions after -e, just use -Dclojure.read.eval=false instead of =unknown.

Comment by Steve Miner [ 21/Feb/13 3:45 PM ]

You're right, the user has control. I was just thinking that the user might not appreciate the details and it would be safer to default to the false behavior. However, I failed to consider the REPL treatment – that's a good point.

In any case, the logic for the binding in the patch could be simplified. I think this would work for the intended result:

(defn- read-never-unknown [read-fn & args]
      (binding [*read-eval* (and *read-eval* true)]
         (apply read-fn args)))

Sorry, if my comments have descended into bike-shedding.

Comment by Stuart Halloway [ 22/Feb/13 9:00 AM ]

I implemented "with-read-known" patch after input from Rich. Helpers that establish bindings should take a closure or fn body, rather than carrying around explicit fns and arg lists as the original 19 Feb patch does.

Marking screened as of the Feb 22 with-read-known patch.

Comment by Tim McCormack [ 22/Feb/13 9:55 AM ]

Steve: More importantly, any time you call (eval (read ...)), there's no reason for *read-eval* to be anything other than true.

Comment by Tim McCormack [ 22/Feb/13 10:12 AM ]

Newer patch works. My verification procedure:

  • Patched master (should probably use an RC instead)
  • Compiled and installed (mvn install) with version = 1.5.0-clj1168 in pom.xml
  • Set up fresh Leiningen project including these options:
    :dependencies [[org.clojure/clojure "1.5.0-clj1168"]]
    :jvm-opts ["-Dclojure.read.eval=unknown"]
  • Verify: `lein repl` succeeds
    • Verify: => (read-string "foo") fails with "Reading disallowed - *read-eval* bound to :unknown"
    • Verify: => #=(+ 1 2) prints 3 (read is fully available to REPL)
  • Write -main fn in project as
    (defn -main [& args]
      (println *read-eval*)
      (println (read-string (first args))))
  • Verify: `lein run foo` prints :unknown and then fails with "Reading disallowed" error.
  • Bind *read-eval* to false in -main
  • Verify: `lein run foo` prints foo.




[CLJ-1164] typos in instant.clj Created: 14/Feb/13  Updated: 24/May/13  Resolved: 24/May/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1164-typos-instant.patch    
Patch: Code
Approval: Ok

 Description   

There are a few typographical mistakes in instant.clj.



 Comments   
Comment by Steve Miner [ 14/Feb/13 12:01 PM ]

Fixes a couple of typos. No code changes.

Comment by Gabriel Horner [ 10/May/13 11:25 AM ]

For anyone wondering about the UTC change see CLJ-928





[CLJ-1163] Updates to changes.md for Clojure 1.5.0-RC15 Created: 13/Feb/13  Updated: 13/Feb/13  Resolved: 13/Feb/13

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File update-changes-md-v1.txt    
Patch: Code

 Description   

Attached patch contains additional changes that have been made since late Oct 2012 up through Clojure 1.5.0-RC15. Also a few minor formatting tweaks to recently added section on edn reader.






[CLJ-1160] reducers/mapcat ignores Reduced Created: 11/Feb/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Completed Votes: 1
Labels: None

Attachments: File lazy-rmapcat2.diff     File lazy-rmapcat.diff    
Patch: Code and Test
Approval: Ok

 Description   

Problem: clojure.core.reducers/mapcat does not stop on reduced? values.

Demonstration:

(require '[clojure.core.reducers :as r])

(->> (concat (range 100) (lazy-seq (throw (Exception. "Too eager"))))
             (r/mapcat (juxt inc str))
             (r/take 5)
             (into []))
;; Exception Too eager

;; Expected return value: [1 "0" 2 "1" 3]

Cause: r/mapcat introduces an intermediate reduce which swallows the reduced value coming from r/take.

Patch: lazy-rmapcat2.diff



 Comments   
Comment by Stuart Sierra [ 02/Aug/13 2:31 PM ]

Cleaned up description.

Comment by Stuart Sierra [ 02/Aug/13 2:36 PM ]

Original patch lazy-rmapcat.diff had whitespace errors, which I have fixed in a new patch lazy-rmapcat2.diff. I also added the ticket number to the commit message.

Comment by Stuart Sierra [ 02/Aug/13 2:37 PM ]

Screened.





[CLJ-1158] generative tests for read and edn/read Created: 09/Feb/13  Updated: 09/Feb/13  Resolved: 09/Feb/13

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File reader-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   

This patch gets some generative read and edn/read roundtripping tests into the build, and the generators should serve as a basis for further extension. The patch is broken into three commits:

  • adopt the latest test.generative and data.generators, which have been enhanced to include generators more types (e.g. uuid, date, ratio)
  • tests
  • increase the duration of generative tests in the build

At the time I am posting this, test.generative 0.4.0 is not in Maven Central, so to build Clojure with this patch you will need to either wait on Central, or grab the latest test.generative bits, back up to the commit tagged 0.4.0, and mvn clean install test.generative.






[CLJ-1154] Compile.java closes out preventing java from reporting exceptions Created: 31/Jan/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: Release 1.5, Release 1.6

Type: Defect Priority: Major
Reporter: Robert (Bobby) Evans Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File CLJ-1154.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: I was trying to compile a project that has some native dependencies (using clojure-maven-plugin version 1.3.13 with Maven 2.0). I forgot to set java.library.path properly so that the native library could be found, and only got an error of

[INFO] ------------------------------------------------------------------------
[ERROR] BUILD ERROR
[INFO] ------------------------------------------------------------------------
[INFO] Clojure failed.
[INFO] ------------------------------------------------------------------------

Cause: Compile.java flushes and closes RT.out in the finally block. When out is closed it is unable to write out the stack trace for the UnsatisfiedLinkError that was being thrown. This made it very difficult to debug what was happening.

Solution: Flush, but do not close RT.out in Compile/main. Test is included, but may or may not be worth keeping.

Patch: CLJ-1154.patch



 Comments   
Comment by Stuart Halloway [ 01/Mar/13 10:45 AM ]

I have encountered this problem as well. Did not verify the explanation, but sounds reasonable.

Comment by Alex Miller [ 24/Jul/13 10:39 AM ]

Updated description a bit and added a patch. The code change is just to stop closing RT.out in Compile.main. The test to invoke Compile and check the stream has an annoying amount of setup and teardown so it's a bit messy. Would love to have more io facilities available (with managed cleanup)!

Comment by Stuart Sierra [ 02/Aug/13 8:34 AM ]

Screened. I confirmed with an independent test, by compiling code which spawned a future which would write to STDOUT:

(ns foo)

(future
  (Thread/sleep 1000)
  (.. System out (println "This is the future of Foo.")))

With or without the patch, creating the future prevents the compile process from terminating, but after the patch the output is visible.





[CLJ-1150] Make some PersistentVector, SubVector internals public Created: 19/Jan/13  Updated: 01/Mar/13  Resolved: 04/Feb/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.5

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Make-some-PersistentVector-s-and-APersistentVector.S.patch    
Patch: Code
Approval: Ok

 Description   

This should help with RRBT-PV interop.






[CLJ-1143] Minor correction to doc string of ns macro Created: 10/Jan/13  Updated: 24/May/13  Resolved: 24/May/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: Release 1.6

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1143-ns-doc-string-correction-v1.txt    
Patch: Code
Approval: Ok

 Description   

The doc string of ns says "If :refer-clojure is not used, a default (refer 'clojure) is used." 'clojure should be replaced with 'clojure.core

Clojure group thread: https://groups.google.com/forum/?fromgroups=#!topic/clojure/rDjZodxOMh8



 Comments   
Comment by Andy Fingerhut [ 10/Jan/13 1:34 PM ]

clj-1143-ns-doc-string-correction-v1.txt dated Jan 10 2013 replaces (refer 'clojure) with (refer 'clojure.core) in the doc string of the ns macro.





[CLJ-1140] {:as x} destructuring with an empty list raises exception Created: 30/Dec/12  Updated: 01/Mar/13  Resolved: 01/Feb/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.5

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

Attachments: File empty-list-destructuring-CLJ-1140-12.30.12.diff    
Patch: Code and Test
Approval: Ok

 Description   
user=> (clojure-version)
"1.4.0"
user=> (let [{:as x} '()] x)
{}

...

user=> (clojure-version)
"1.5.0-RC1"
user=> (let [{:as x} '()] x)
IllegalArgumentException No value supplied for key: null  clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

The bug was introduced by a change[1] to support duplicate keys in map
destructuring. Using PersistentHashMap/create here introduces the above
bug, since it does not properly handle empty lists.

[1]: https://github.com/clojure/clojure/commit/93c795fe10ee5c92a36b6ec6373b3c80a31135c4



 Comments   
Comment by Toby Crawley [ 02/Jan/13 11:02 AM ]

There's been some discussion on clojure-dev around this issue: https://groups.google.com/d/topic/clojure-dev/qdDRNfEVfQ8/discussion

Comment by Toby Crawley [ 01/Feb/13 10:05 AM ]

An issue I brought up in the email thread is consistency: vector binding works with anything nthable, map binding works with anything associative. With my current patch (empty-list-destructuring-CLJ-1140-12.30.12.diff), only ISeqs are supported for kwarg map binding.

I'd prefer it work with anything seqable, and can provide a patch that does that. I would go ahead and do so, but now that this ticket is now Approval: OK, I didn't want to alter what had been OK'ed. Let me know if you want a patch that adds support for anything seqable.





[CLJ-1135] Add previous changelog items back to changes.md Created: 19/Dec/12  Updated: 02/Feb/13  Resolved: 02/Feb/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.5

Type: Enhancement Priority: Trivial
Reporter: Cosmin Stejerean Assignee: Stuart Halloway
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File add-changelog-items.patch    
Patch: Code
Approval: Vetted




[CLJ-1125] Clojure can leak memory when used in a servlet container Created: 11/Dec/12  Updated: 11/Jan/14  Resolved: 11/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Defect Priority: Critical
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 14
Labels: memory

Attachments: File threadlocal-removal-tcrawley-2012-12-11.diff     File threadlocal-removal-tcrawley-2013-06-14.diff     File threadlocal-removal-tcrawley-2013-11-24.diff    
Patch: Code
Approval: Ok

 Description   

When used within a servlet container (Jetty/Tomcat/JBossAS/Immutant/etc), the thread locals Var.dvals (used to store dynamic bindings) and LockingTransaction.transaction (used to store the currently active transaction(s)) prevent all of the classes loaded by an application's clojure runtime from being garbage collected, resulting in a memory leak.

Cause: The issue comes from threads living beyond the lifetime of a deployment - servlet containers use thread pools that are shared across all applications within the container. Currently, the dvals and transaction thread locals are not discarded when they are no longer needed, causing their contents to retain a hard reference to their classloaders, which, in turn, causes all of the classes loaded under the application's classloader to be retained until the thread exits (which is generally at JVM shutdown).

Solution: I've attached a patch that does the following:

  • Var.dvals is initialized to a canonical TOP Frame
  • Var.dvals is now removed when the thread bindings are popped to the TOP
  • The outer transaction in LockingTransaction.transaction now removes the thread local when it is finished

There is still the opportunity for memory leaks if agents or futures are used, and the executors used for them are not shutdown when the app is undeployed. That's a solvable problem, but should probably be solved by the containers themselves (and/or the war generation tools) instead of in clojure itself.

This patch has a small performance impact: its use of a try/finally around running transactions to remove the outer transaction adds 4-6 microseconds to each transaction call on my hardware.

Providing an automated test for this patch is difficult - I've tested it locally with repeated deployments to a container while monitoring GC and permgen. All of clojure's tests pass with it applied.

The above is a condensation of:
https://groups.google.com/d/topic/clojure-dev/3CXDe8_9G58/discussion

Patch: threadlocal-removal-tcrawley-2013-11-24.diff

Screened by: Alex Miller - the new patch (since prior screening) has no changes in the LockingTransaction code but has been updated in Var to address the regression logged in CLJ-1299.



 Comments   
Comment by Colin Jones [ 13/May/13 7:30 PM ]

This patch works great for me to avoid OOM/PermGen crashes from classloaders being retained [mine is a non-servlet use case].

Comment by Stuart Halloway [ 24/May/13 9:43 AM ]

Does Tomcat create warnings for Clojure, as described e.g. here?

If so, does this patch make the warnings go away?

Comment by Toby Crawley [ 24/May/13 9:56 AM ]

Stu: that's a good question. I'll take a look at Tomcat this afternoon.

Comment by Stuart Halloway [ 24/May/13 10:04 AM ]

The code that calls transaction.remove() seems unncessarily subtle. There are two exits from the method, and only one is protected by the finally block.

If the "outer" case was a top-level if, the logic would be more clear, and only the "outer" case would need try/finally, which might reduce the performance penalty in the case of deeply nested dosyncs.

Did your transaction overhead of 4-6 microseconds test only one level of dosync, or many?

Comment by Stuart Halloway [ 24/May/13 10:13 AM ]

Because the unwind code calls remove at the top (as opposed to set(null)), the code should now be safe for use with Clojure-defined ThreadLocal subclasses.

Therefore, Var's use of an initialValue should be irrelevant to this patch, and it should be possible to fix this bug with a patch half the size of the current patch, touching only LockingTransaction.runInTransaction and Var.popThreadBindings.

Comment by Toby Crawley [ 14/Jun/13 7:38 AM ]

re: Tomcat ThreadLocal warnings

With Clojure 1.5.1 using my test app (linked below), I see:

Jun 14, 2013 6:35:22 AM org.apache.catalina.loader.WebappClassLoader checkThreadLocalMapForLeaks
SEVERE: The web application [/leak] created a ThreadLocal with key of type [clojure.lang.Var$1] (value [clojure.lang.Var$1@4902919]) and a value of type [clojure.lang.Var.Frame] (value [clojure.lang.Var$Frame@147a2aa6]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
Jun 14, 2013 6:35:22 AM org.apache.catalina.loader.WebappClassLoader checkThreadLocalMapForLeaks
SEVERE: The web application [/leak] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@608602ca]) and a value of type [clojure.lang.LockingTransaction] (value [clojure.lang.LockingTransaction@7e214d47]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.

With the original patch (threadlocal-removal-tcrawley-2012-12-11.diff) and the one attached today (threadlocal-removal-tcrawley-2013-06-14.diff), I no longer see these warnings.

re: the LockingTransaction.runInTransaction changes

In today's patch (threadlocal-removal-tcrawley-2013-06-14.diff), I modified runInTransaction to have one exit point, and only wrap a call to run with a try/finally in the outer transaction case. It does introduce two locations where run can be called to preserve the case where an inner transaction has null info:

static public Object runInTransaction(Callable fn) throws Exception{
	LockingTransaction t = transaction.get();
        Object ret;
	if(t == null) {
            transaction.set(t = new LockingTransaction());
            try {
                ret = t.run(fn);
            } finally {
                transaction.remove();
            }
        } else {
            if(t.info != null) {
                ret = fn.call();
            } else {
                ret = t.run(fn);
            }
        }

        return ret;
}

However, this will likely not reduce the speed penalty I observed in my testing, as I was only using a single level of dosync when capturing timing data.

re: removing initialValue from dvals

My original solution kept initialValue, but I then apparently discovered cases where the leak still occurred (see the mailing list thread).

Unfortunately, I can neither recreate that case, nor find in my notes, test code, or the clojure code a reason why keeping initialValue would allow the ThreadLocals to leak when popThreadBindings is patched (assuming one doesn't call Var.getThreadBindings from Java without calling Var.popThreadBindings).

Therefore, I've attached a simpler patch (threadlocal-removal-tcrawley-2013-06-14.diff) that just patches LockingTransaction.runInTransaction and Var.popThreadBindings.

I've also created a project that demonstrates the leak with 1.5.1, and that the leak does not appear with this patch applied to 1.6.0-master. See its README for usage details.

The patched version of 1.6.0-master is available as [org.clojars.tcrawley/clojure "1.6.0-clearthreadlocals"] if anyone wants to give it a try in their own projects. Note that since its group isn't 'org.clojure', you may need to add exclusions to your project to prevent another version of clojure being included.

Comment by Andy Fingerhut [ 14/Jun/13 10:56 AM ]

Presumptuously changing ticket approval from Incomplete back to its former Vetted state, since Toby's comments and new patch seem to address the comments that led Stu to change it to Incomplete.

Comment by Toby Crawley [ 02/Aug/13 10:30 AM ]

Stu:

Is there anything else you need from me for this to be applied?

Comment by Chas Emerick [ 04/Aug/13 5:52 PM ]

FWIW, using Toby's Clojure dep with Immutant has eliminated the out-of-permgen errors I used to occasionally get after N app redeployments.

Comment by Alex Miller [ 23/Aug/13 11:08 AM ]

I looked at the updated patch and it seems good to me. In the LockingTransaction.runinTransaction code the cases are driven by where t=null and t.info=null. Of those 4 cases, I believe the same call is being made in all but the case of t == null (where a new LockingTransaction is created) and t.info != null. However, I believe since a new txn is created and t.info should start as null, this case does not actually exist in practice.

Greatly appreciate Chas's experience feedback and all of Toby and Stu's work to make this change solid!

Marking screened.

Comment by Alex Miller [ 22/Nov/13 7:59 PM ]

Reverted in 1.6.0-alpha3 based on CLJ-1299 report.

Comment by Toby Crawley [ 24/Nov/13 5:22 PM ]

I just attached a new patch (threadlocal-removal-tcrawley-2013-11-24.diff) that achieves the same ThreadLocal removal as the previous patch, but addresses the issues with binding conveyance reported in CLJ-1299.





[CLJ-1122] Add contributing.md file to github repository (shows clear message on issues/pull request create form) Created: 09/Dec/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File contributing.patch     Text File contributing-v2.patch    
Patch: Code
Approval: Ok

 Description   

Problem: New contributors are often unaware of the process for submitting tickets and patches for Clojure. In particular, almost daily a contributor sends a pull request to Clojure or a contrib library and the repo owner must them notify them that we don't accept patches.

Solution: Github supports a special contributors markdown file (see https://github.com/blog/1184-contributing-guidelines) that will be shown when users want to create a pull request or issue. The patch provides this file and invites the user to read the guidelines. The content of the file is just a markdown version of http://clojure.org/contributing

This patch covers clojure, but similar could be done on all contrib libraries as well.

Preview (I think this is first patch) here: https://github.com/mpenet/clojure/blob/aef170ca5eca1b71a2eb1ef320223d1277df0e5e/CONTRIBUTING.md

Patch: contributing-v2.patch
Screened by: Alex Miller



 Comments   
Comment by Stuart Halloway [ 02/Jan/13 6:31 AM ]

Please change this to link to the definitive prose, so we don't have to maintain that in two places.

Comment by Max Penet [ 02/Jan/13 6:51 AM ]

Feel free to correct the wording, I used something simple.

Comment by Gabriel Horner [ 17/May/13 9:04 AM ]

Stu, he linked to clojure.org as you requested so I'm moving this along.

Comment by Alex Miller [ 28/Jul/13 9:43 PM ]

Reworded the description to make it follow our guidelines better and point to the patch to look at.





[CLJ-1121] -> and ->> have unexpected behavior when combined with unusual macros Created: 06/Dec/12  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Minor
Reporter: Gary Fredericks Assignee: Stuart Halloway
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File 0001-CLJ-1121-Reimplement-and-without-recursion.patch     Text File CLJ-1121-v002.patch    
Patch: Code and Test
Approval: Ok

 Description   

My intuitive understanding of the classic threading macros is that the meaning of forms like (-> a b c) can be understood syntactically independent of the meaning of the symbols involved or the fact that the two threading macros are defined recursively. However the recursive definition breaks that expectation. After

(macroexpand-1 (macroexpand-1 '(-> a b c)))

=> (c (-> a b))

c is now in control if it is a macro, and is now seeing the argument (-> a b) rather than (b a) as would be the case if we had written (c (b a)) originally.

Admittedly I do not know of a realistic example where this is an important distinction (I noticed this when playing with a rather perverse use of ->> with macros from korma), but at the very least it means that the behavior of the threading macros isn't quite as easy to accurately explain as I thought it was.



 Comments   
Comment by Gary Fredericks [ 07/Dec/12 9:19 AM ]

I just realized that my patch also implements CLJ-1086.

Comment by Stuart Halloway [ 22/Dec/12 9:48 AM ]

Would be nice if tests also demonstrated that metadata is preserved correctly.

Comment by Gary Fredericks [ 26/Dec/12 8:16 PM ]

New patch in response to stuarthalloway feedback.

Comment by Tim McCormack [ 09/Jan/13 6:47 PM ]

This patch also prevents an infinite loop in the macroexpander when fed the following expression:

(->> a b (->> c d))

Edit: Far simpler example.





[CLJ-1118] inconsistent numeric comparison semantics between BigDecimal and other Numerics Created: 30/Nov/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Arthur Ulfeldt Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: math

Attachments: Text File clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt     Text File clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt     Text File clj-1118-v6.txt     Text File clj-1118-v7.patch    
Patch: Code and Test
Approval: Ok

 Description   

BigDecimal does not have consistent comparison semantics with other numeric types.

user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false                  <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
user> (== 1.0M 1.00M)
false    ;; potentially surprising

Patch: clj-1118-v7.patch

Approach: Change equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The javadoc for these methods explicitly states that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies hasheq() to return the same value for all numerically equal BigDecimal values, by calling BigDecimal.stripTrailingZeros() on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

Screened by: Alex Miller



 Comments   
Comment by Arthur Ulfeldt [ 30/Nov/12 1:51 PM ]

I understand that the definition of equality between bigDecimals is dependent on both value and scale as in this case:

user> (== 0.000000M 0.0M)
false

I just want to make sure the decission to propagate that semantic across types is intentional. If this is on purpose than this is not a bug.

Comment by Arthur Ulfeldt [ 30/Nov/12 2:03 PM ]

this could be fixed by calling stripTrailingZeros on bigDecimals before comparing them to Longs or BigInts.

(== 2 (double (. 2.0M stripTrailingZeros)))
true

Edited by Andy Fingerhut: Unfortunately that fails for BigDecimal values equal to 0, unless they happen to have a scale that matches what you are comparing it to.

I think a more complete solution is to use BigDecimal's compareTo method, e.g.:

(zero? (.compareTo 2.0M (bigdec 2)))
true

Comment by Timothy Baldridge [ 03/Dec/12 11:31 AM ]

It seems we need some more eyes on this issue, can you bring this up on clojure-dev and see what they think?

Comment by Andy Fingerhut [ 14/Apr/13 4:03 AM ]

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt dated Apr 14 2013 changes equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal.

They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

I'm not sure if this is the preferred behavior for Clojure, but if it is, this patch should do it.

Comment by Andy Fingerhut [ 15/Apr/13 12:18 AM ]

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt dated Apr 14 2013 is same as clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt described in previous comment, except it also has some new tests included.

Comment by Andy Fingerhut [ 15/Apr/13 9:07 PM ]

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt dated Apr 15 2013 is the same as the the previous patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt, except for the following:

By changing == behavior for BigDecimal by modifying the BigDecimalOps.equiv() method, that also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return same value for all numerically equal BigDecimal values. This patch should achieve that.

Comment by Andy Fingerhut [ 14/Aug/13 7:29 PM ]

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt dated Apr 15 2013 is simply updating the stale -v3.txt patch. The only reason it no longer applied cleanly with git is because a few lines of context changed in the test code.

Comment by Andy Fingerhut [ 27/Sep/13 11:55 AM ]

Quick comment that some of the tests in patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt were written before CLJ-1036 was declined as out of scope for Clojure, and should probably be cut down a bit, at least to eliminate biginteger and float occurrences. I will wait and see if there are any other comments from screening before creating any new patches.

Comment by Alex Miller [ 27/Sep/13 12:21 PM ]

Andy, the tests in the patch don't actually pass for me?

Many failures in the generative tests, but for example:

[java] {:clojure.test/vars (equality-tests),
     [java]  :thread/name "main",
     [java]  :pid 5719,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :message
     [java]  "Test that 0 (class java.lang.Byte) #'clojure.core/== 0.0 (class java.math.BigDecimal)",
     [java]  :level :warn,
     [java]  :test/actual false,
     [java]  :test/expected (equal-var val1 val2),
     [java]  :line 58,
     [java]  :tstamp 1380298603830,
     [java]  :file "numbers.clj"}
Comment by Andy Fingerhut [ 27/Sep/13 12:26 PM ]

Could you tell me the OS, version, and JDK version, and the command you used to run this test? The reason I ask is that this patch has been tested on 3 OS/JDK version combos via my prescreen testing, using 'ant', and it passes on all of them, so if it fails on some OS/JDK I am not testing I would like to figure out why.

Comment by Alex Miller [ 27/Sep/13 12:42 PM ]

Sorry - user error. They're passing.

Comment by Alex Miller [ 27/Sep/13 3:12 PM ]

Slightly tweaked patch to remove some of the float/double stuff that I'm not expecting to change.

Marking screened...

Comment by Andy Fingerhut [ 27/Sep/13 7:54 PM ]

Patch clj-1118-v6.txt is tiny modification of Alex Miller's clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt

It removes hash consistency tests for floats and BigIntegers, with a comment explaining why they should not be there.

Comment by Rich Hickey [ 22/Oct/13 7:55 AM ]

Please don't use .txt as an extension for diffs, it keeps them from loading with appropriate mode in various editors.

v5 was screened but v6 alters it?

Comment by Alex Miller [ 22/Oct/13 8:01 AM ]

Andy's last patch just added a comment. I updated a new version that is identical to v6 but has a .patch extension instead. Marking Screened again.

Comment by Andy Fingerhut [ 22/Oct/13 8:37 AM ]

It was not only adding a comment, but removing tests that should not have been there and could potentially mislead people into thinking that Clojure guarantees hash being consistent with = for BigInteger and Float/Double, which by CLJ-1036 it does not.

Regarding the .txt file extensions, first I've heard of the preference. I can make sure future screened tickets don't have this, but changing currently screened tickets would likely lead to confusion about which ones are screened. M-x diff-mode in emacs will force the mode regardless of file name.

Comment by Alex Miller [ 22/Oct/13 8:53 AM ]

Oh right, that is ok so can stay screened.





[CLJ-1116] More REPL-friendly 'ns macro Created: 29/Nov/12  Updated: 01/Mar/13  Resolved: 11/Dec/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3, Release 1.4
Fix Version/s: Release 1.5

Type: Defect Priority: Major
Reporter: Laurent Petit Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: None

Attachments: File dynamic-ns-patch2.diff    
Patch: Code
Approval: Ok

 Description   

The 'ns macro is not as dynamic as it could be.
For instance, the following line typed in a repl (ns a)(ns b (:require a)) currently (1.4, 1.3, etc.) fails with an exception because the (:require a) call tries to reach the filesystem for file a.clj or a__init.class.

The attached patch ( dynamic-ns-patch2.diff ) allows a successful call to (ns a) behave the same as a successful call to (require 'a), adding namespace a to the list of loaded-libs.

Discussion on googlegroup's mailing list: http://groups.google.com/group/clojure-dev/browse_thread/thread/fb231e6fab4a5ad



 Comments   
Comment by Rich Hickey [ 29/Nov/12 9:31 AM ]

screeners, please make sure this has tests before passing on

Comment by Laurent Petit [ 29/Nov/12 10:41 AM ]

Tests added

Comment by Timothy Baldridge [ 30/Nov/12 11:35 AM ]

Patch applies, fixes the bug. All tests pass.

Screened

Comment by Herwig Hochleitner [ 03/Dec/12 7:52 AM ]

I see this has already been screened, but FWIW I tested the repl with this patch and the behavior works for me.





[CLJ-1114] reify ignores :pre and :post Created: 25/Nov/12  Updated: 15/Dec/12  Resolved: 15/Dec/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

reify ignores :pre and :post assertions

user=> ((reify clojure.lang.IFn (invoke [this] {:pre [false]} (println "hello"))))
hello
nil
user=> ((fn [this] {:pre [false]} (println "hello")) 0)
AssertionError Assert failed: false user/eval39/fn--40 (NO_SOURCE_FILE:13)

Expected exception to be thrown



 Comments   
Comment by Stuart Halloway [ 25/Nov/12 6:52 PM ]

reify is not documented to support these, so this should be classified as an enhancement

Comment by Timothy Baldridge [ 30/Nov/12 2:51 PM ]

Vetted.





[CLJ-1111] Loops returning primtives are boxed even in return position Created: 21/Nov/12  Updated: 22/Dec/12  Resolved: 22/Dec/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: Release 1.5

Type: Defect Priority: Major
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Completed Votes: 3
Labels: None

Attachments: File prim-loop.diff    
Patch: Code
Approval: Ok

 Description   

Reported here: https://groups.google.com/d/topic/clojure/atoFzbyuyos/discussion

(defn first-bit?
  {:inline (fn [n] `(== 1 (clojure.lang.Numbers/and ~n, 1)) )}
  [^long n]
  (== 1 (clojure.lang.Numbers/and n, 1)))

(defn exp-int
  ^double [^double x, ^long c]
  (loop [result 1.0, factor x, c c]
    (if (> c 0)
        (recur
         (if (first-bit? c)
           (* result factor)
           result),
         (* factor factor),
         (bit-shift-right c 1))
      result)))

Last lines of the Java bytecode of `exp-int`:

59 dload 5;               /* result */
61 invokestatic 40;       /* java.lang.Double java.lang.Double.valueOf(double c) */
64 checkcast 85;          /* java.lang.Number */
67 invokevirtual 89;      /* double doubleValue() */
70 dreturn;

The compiler doesn't currently infer the primitive type as soon as there is a recur:

(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}
(expression-info '(loop [a 1] (if true a (recur a)))
;=> nil

Patch attached.



 Comments   
Comment by Stuart Halloway [ 25/Nov/12 7:12 PM ]

Tests pass, code looks reasonable. Would appreciate additional review.

Comment by Timothy Baldridge [ 26/Nov/12 10:59 AM ]

Tests also pass here. Looked through the code and played with a patched version of Clojure. I can't see a problem with it.

Comment by Christophe Grand [ 27/Nov/12 4:40 AM ]

FYI, gvec.clj has two loops which return primitives and thus was formerly boxed.





[CLJ-1110] let-> needs improvement Created: 20/Nov/12  Updated: 11/Dec/12  Resolved: 29/Nov/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Nixon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

The function clojure.core/let-> is suboptimal in a few regards:
1. It's name lends you to believe it is "let-like", but it is not - it does not take a binding form, and its arguments are 'backwards'
2. It's docstring doesn't make clear that it is intended for use solely in a threading-macro expression
3. It arbitrarily does not support destructuring, where it easily could.

Possible solutions:

  • rename it (e.g. to "bind->") to make clear it is not let-like
  • allow destructuring of the 'name' parameter

Google groups discussion: https://groups.google.com/d/topic/clojure/67JQ7xSUOM4/discussion



 Comments   
Comment by Yongqian Li [ 11/Dec/12 12:22 AM ]

Has it been decided whether as-> supports de-structuring or not? Right now it is inconsistent.

(as-> [0 1]
      [a b]
  [(inc a) (inc b)])

 => [1 2]



(as-> {:a 0 :b 1}
      {:keys [a b]}
  {:a (inc a) :b (inc b)})

 => {:keys [1 2]}
Comment by Yongqian Li [ 11/Dec/12 12:50 AM ]

Also, what about changing the name to >as? ->as reads like "pipeline as name", whereas as> implies that you are starting a new pipeline.

(-> 0
  (->as name 
    (...))

Makes it clearer that you are consuming a pipeline and the expressions within ->as are no longer being influenced by ->.

Comment by Yongqian Li [ 11/Dec/12 1:03 AM ]

Has the addition of ->>as been considered? That way you can use it inside a ->> like this:

(->> a 
  (->>as name
    (...)))

(-> a 
  (->as name
    (...)))

The signature would be [name forms init-expr].





[CLJ-1109] Oracle Java 5 fails to run tests when building Clojure Created: 19/Nov/12  Updated: 15/Dec/12  Resolved: 15/Dec/12

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Oracle Java 1.5.0_22, at least


Attachments: Text File patch-enable-java-5-to-pass-tests-v1.txt    
Patch: Code and Test

 Description   

This is due to the way that reducers.clj currently handles the ForkJoin library.



 Comments   
Comment by Andy Fingerhut [ 19/Nov/12 3:50 PM ]

Patch patch-enable-java-5-to-pass-tests-v1.txt dated Nov 19 2012 enables at least Oracle Java 1.5.0_22 to build and pass all tests in latest master.

No, tt doesn't implement a ForkJoin library. It simply declares some Clojure fj functions to throw an exception if they are called. In today's Clojure tests they never are.

Comment by Andy Fingerhut [ 20/Nov/12 3:45 PM ]

Updated patch with proper name and email address.

Comment by Timothy Baldridge [ 30/Nov/12 3:05 PM ]

IMO, more tests are always good. Vetted.

Comment by Rich Hickey [ 15/Dec/12 7:33 AM ]

This is the wrong approach to this problem. A better approach is to make reducers work even if no fj support is present by simply defining fold as sequential reduce in that case.





[CLJ-1106] Broken equality for sets Created: 09/Nov/12  Updated: 13/Sep/13  Resolved: 08/Feb/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.5

Type: Defect Priority: Major
Reporter: Justin Kramer Assignee: Aaron Bedra
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File 0001-CLJ-1106-Use-Util.hasheq-in-hashCode-for-persistent-.patch     Text File 0001-Fixing-set-equality.patch     File setequals.diff    
Patch: Code and Test
Approval: Ok

 Description   

Equality for sets is not consistent with other persistent collections when the hashCode differs:

(= {(Integer. -1) :foo} {(Long. -1) :foo})
=> true

(= [(Integer. -1)] [(Long. -1)])
=> true

(= #{(Integer. -1)} #{(Long. -1)})
=> false

Attached is what I believe to be a basic fix. It removes a hashCode check from APersistentSet.equals. A similar check was removed from APersistentMap in the following commit:

https://github.com/clojure/clojure/commit/b5f5ba2e15dc2f20e14e05141f7de7c6a3d91179

With this patch, set equality works as expected and all tests pass.

My understanding of the implications of this change are limited, so someone more knowledgeable would need to chime in about its correctness.



 Comments   
Comment by Andy Fingerhut [ 09/Nov/12 9:48 AM ]

Can you add some unit tests that fail with the current code but succeed with the change?

Comment by Justin Kramer [ 09/Nov/12 10:24 AM ]

Revised patch with unit test that fails in master and succeeds with patch

Comment by Timothy Baldridge [ 03/Dec/12 9:04 AM ]

vetting

Comment by Gary Fredericks [ 29/Jan/13 5:32 PM ]

This came up when we were trying to diff two datasets by putting them in sets and comparing. We had Integers because they came back from JDBC that way.

Comment by Aaron Bedra [ 29/Jan/13 5:35 PM ]

I'm going to take a look and try to get this shipped. It hit us and I would love to to see it in 1.5

Comment by Paul Stadig [ 29/Jan/13 7:14 PM ]

I applied the patch on master (ca465d6d) and it applied cleanly and fixes the issue.

Comment by Bo Jeanes [ 30/Jan/13 11:19 AM ]

Instead of removing m.hashCode() != hashCode(), perhaps we should use `Util.hasheq()` instead. It already seems to special case numbers (https://github.com/clojure/clojure/blob/f48d024e97/src/jvm/clojure/lang/Util.java#L164-L172) and won't have the potential performance impact that skipping hashCode checks could bring.

Comment by Bo Jeanes [ 30/Jan/13 11:39 AM ]

Attaching a patch with an alternate fix for this issue that does not skip hashCode checking.

It passes all existing tests and fixes this issue.

I want to benchmark the difference, too.

Comment by Bo Jeanes [ 30/Jan/13 1:32 PM ]

On further thought and comparison to APersistentMap, I'm not sure if that's the best use of Util.hasheq(). I can't find good reference on the semantic differences and am not familiar enough with the Clojure source to infer it, yet.

Comment by Aaron Bedra [ 02/Feb/13 12:33 PM ]

Bo, I don't see you listed on the contributors list. Did you send in a CA?

Comment by Aaron Bedra [ 02/Feb/13 1:36 PM ]

Both of the patches above are not complete. APersistentSet equiv calls equals. APersistentMap has two separate ways of handling this. This is the path that the fix should take.

Comment by Aaron Bedra [ 02/Feb/13 2:37 PM ]

This patch addresses the difference between equals and equiv.

Comment by Stuart Halloway [ 04/Feb/13 9:55 PM ]

The Set equality code currently in Clojure uses .hashCode to quickly bail out of comparisons in both .equals and .equiv. This feels wrong since .equals and .equiv presume different hashes. The map equality code avoids the problem by not checking any hash.

Aaron's 2/13 patch makes set equality work like map equality, not checking the hash. (I think a much shorter patch that accomplishes the same thing merely drops the call to hash.)

It seems to me a separate problem that both hash and set are broken for Java .equals rules, because of the equiv-based handling of their keys.

I think this needs more consideration.

Comment by Stuart Halloway [ 05/Feb/13 8:57 AM ]

Notes from discussion with Rich:

1. Aaron's 2/2/13 patch, which mimics map logic into set, is the preferred approach.

2. The broader issue I was worried about is the semantic collision between Map's containsKey and Associative's containsKey. This is out of scope here, and perhaps ever.

Comment by Aaron Bedra [ 05/Feb/13 9:38 AM ]

Any chance this will make 1.5?

Comment by Gary Fredericks [ 13/Sep/13 9:42 AM ]

This is apparently still present wrt BigInteger:

(let [n1 -5, n2 (BigInteger. "-5")]
  [(= n1 n2) (= #{n1} #{n2}) (= [n1] [n2])])
;; => [true false true]

Should this be a new ticket?

Comment by Gary Fredericks [ 13/Sep/13 9:48 AM ]

Created CLJ-1262.





[CLJ-1105] clojure.walk should support records Created: 08/Nov/12  Updated: 14/Feb/14  Resolved: 22/Nov/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: Release 1.6

Type: Enhancement Priority: Minor
Reporter: Jouni K. Seppänen Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: walk

Attachments: Text File 0001-CLJ-1105-Support-records-in-clojure.walk.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: clojure.walk throws exceptions if used on records.

user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo  user.Foo (NO_SOURCE_FILE:1)

Current Patch: 0001-CLJ-1105-Support-records-in-clojure.walk.patch adds a special case for records.

See also: CLJ-1239 "faster, more flexible dispatch for clojure.walk" which could supersede this ticket.

Screened by: Alex Miller - I think this will likely be superseded by CLJ-1239 in the future, but this is a reasonable short-term step.



 Comments   
Comment by Nicola Mometto [ 08/Nov/12 2:35 PM ]

maybe clojure should follow clojurescript's footsteps and move empty out of IPersistentCollection and create an
interface IEmptyableCollection extends IPersistentCollection { IEmptyableC