<< Back to previous view

[CLJ-1804] take transducer optimization Created: 25/Aug/15  Updated: 26/Aug/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Karsten Schmidt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, transducers

Attachments: Text File clj-1804-1.patch     Text File clj-1804-2.patch    
Patch: Code
Approval: Triaged

 Description   

A basic refactoring to remove the let form and only requires a single counter check for each iteration, yields an 25% performance increase. With the patch, 2 checks are only required for the last iteration (in case counter arg was <= 0)...

;; master
(quick-bench (into [] (take 1000) (range 2000)))
WARNING: Final GC required 34.82584189073624 % of runtime
Evaluation count : 13050 in 6 samples of 2175 calls.
             Execution time mean : 46.921254 µs
    Execution time std-deviation : 1.904733 µs
   Execution time lower quantile : 45.124921 µs ( 2.5%)
   Execution time upper quantile : 49.427201 µs (97.5%)
                   Overhead used : 2.367243 ns

;; w/ patch
(quick-bench (into [] (take 1000) (range 2000)))
WARNING: Final GC required 34.74448252054369 % of runtime
Evaluation count : 18102 in 6 samples of 3017 calls.
             Execution time mean : 34.301193 µs
    Execution time std-deviation : 1.714105 µs
   Execution time lower quantile : 32.341349 µs ( 2.5%)
   Execution time upper quantile : 37.046851 µs (97.5%)
                   Overhead used : 2.367243 ns


 Comments   
Comment by Karsten Schmidt [ 25/Aug/15 10:35 AM ]

Proposed patch, passes all existing tests

Comment by Alex Miller [ 25/Aug/15 11:28 AM ]

From a superficial skim, I see checks for pos? and then neg? which both exclude 0 and that gives me doubts about that branch. That may actually be ok but I will have to read it more closely.

Comment by Karsten Schmidt [ 25/Aug/15 11:58 AM ]

Hi Alex, try running the tests... AFAICT it's all still working as advertised: For (take 0) or (take -1) then the pos? check fails, but we must ensure to not call rf for that iteration. For all other (take n) cases only the pos? check is executed apart from the last iteration (which causes a single superfluous neg? call) The current path/impl always does 2 checks for all iterations and hence is (much) slower.

Comment by Alex Miller [ 25/Aug/15 7:22 PM ]

The only time that neg? case will be hit is if take is passed n <= 0, so I think this is correct. However, you might consider some different way to handle that particular case - for example, it could be pulled out of the transducer function entirely and be created as a separate transducer function entirely. I'm not sure that's worth doing, but it's an idea.

Comment by Karsten Schmidt [ 26/Aug/15 6:01 AM ]

Good idea, Alex! This 2nd patch removes the neg? check and adds a quick bail transducer for cases where n <= 0. It also made it slightly faster still:

(quick-bench (into [] (take 1000) (range 2000)))
Evaluation count : 20370 in 6 samples of 3395 calls.
             Execution time mean : 30.302673 µs

(now ~35% faster than original)





[CLJ-1803] Enable destructuring of sequency maps Created: 22/Aug/15  Updated: 26/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring

Attachments: Text File 0001-Enable-destructuring-of-seq-map-types.patch    
Patch: Code
Approval: Triaged

 Description   

At present, Clojure's destructuring implementation will create a hash-map from any encountered value satisfying clojure.core/seq? This has the I argue undesirable side effect of making it impossible to employ destructuring on a custom Associative type which is also a Seq. This came up when trying to destructure instances of a tagged value class which for the purpose of pattern matching behave as [k v] seqs, but since the v is known to be a map, are also associative on the map part so as to avoid the syntactic overhead of updates preserving the tag.

;; A sketch of such a type
(deftype ATaggedVal [t v]
  clojure.lang.Indexed
  (nth [self i]    (nth self i nil))
  (nth [self i o] (case i (0) t (1) v o))

  clojure.lang.Sequential
  clojure.lang.ISeq
  (next  [this]     (seq this))
  (first [this]     t)
  (more  [this]     (.more (seq this)))
  (count [this]     2)
  (equiv [this obj] (= (seq this) obj))
  (seq   [self]     (cons t (cons v nil)))

  clojure.lang.Associative
  (entryAt [self key] (.entryAt v key))
  (assoc [_ sk sv]    (ATaggedVal. t (.assoc v sk sv)))

  clojure.lang.ILookup
  (valAt [self k]   (.valAt v k))
  (valAt [self k o] (.valAt v k o))

  clojure.lang.IPersistentMap
  (assocEx [_ sk sv] (ATaggedVal. t (.assocEx v sk sv)))
  (without [_ sk]    (ATaggedVal. t (.without v sk))))

So using such a thing,

(let [{:keys [x]} (ATaggedVal. :foo {:x 3 :y 4})] x)
;; expect 3
=> nil

Since for any type T such that clojure.core/get will behave, T should satisfy clojure.core/map? it should be correct simply to change the behavior of destructure to only build a hash-map if map? isn't already satisfied.

The attached patch makes this change.



 Comments   
Comment by Alex Miller [ 22/Aug/15 1:21 PM ]

Probably worth watching CLJ-1778 too which might cause this not to apply anymore.

Could you add an example of what doesn't work to the description?

Comment by Ragnar Dahlen [ 24/Aug/15 1:20 AM ]

The current patch for CLJ-1778 does not address this issue.

The idea seems sound to me, if we're map destructuring a value that's
already a map (as determined by map?), we don't need to create a
new one by calling seq and HashMap/create, unless there's a
really good reason it should be exactly that map implementation (I
don't see one).

I don't think the current patch is OK though as it makes an
(unneccessary) breaking change to the behaviour of map destructuring.
Previously, destructuring a non-seqable value returned nil, but with
patch, seq is always called on value and for non-seqble types this
will instead throw an exception. It should be trivial to change the
patch to retain the original behaviour.

1.8.0-master:

user=> (let [{:keys [x]} (java.util.Date.)] x)
nil

with 0001-Enable-destructuring-of-seq-map-types.patch:

user=> (let [{:keys [x]} (java.util.Date.)] x)
IllegalArgumentException Don't know how to create ISeq from: java.util.Date  clojure.lang.RT.seqFrom (RT.java:528)
Comment by Reid McKenzie [ 26/Aug/15 1:15 PM ]

I contend that the behavior broken is, at best, undefined behavior consequent from the implementation and that the failure to cast exception is at least clearer than the silent nil behavior of the original implementation.

I would personally prefer to extend the destructuring checks logically to (cond map? x seq? (hash-map (seq x)) :else (throw "Failed to destructure non-map type") but I think that change is sufficiently large that it would meaningfully decrease the chances of this patch being accepted.





[CLJ-1800] Doc that lazy-seq with-meta forces realization Created: 13/Aug/15  Updated: 19/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJ-1800-no-realize-v1.patch     Text File CLJ-1800-v2.patch    
Approval: Triaged

 Description   

Applying meta to a lazy-seq causes realization:

(def x (vary-meta (lazy-seq (prn :realized)) assoc :foo :bar))
:realized

This might be surprising, so modify docstring of lazy-seq to mention it.

Patch:



 Comments   
Comment by Alex Miller [ 13/Aug/15 9:02 AM ]

I think it's likely that seq() is called here so that the old LazySeq instance and the new one share the sequence. Otherwise the pre-meta and post-meta versions would be performing the same function calls on the same inputs but would be disconnected, which seems bad.

Comment by Alex Miller [ 13/Aug/15 9:03 AM ]

I'm not really sure where this would be documented. Maybe on the http://clojure.org/metadata page?

Comment by Max Penet [ 13/Aug/15 9:18 AM ]

That would make sense yes and on the docstring of lazy-seq as well.

Comment by Alex Miller [ 13/Aug/15 9:47 AM ]

I added a sentence to the metadata page and updated the description to be more applicable here to a docstring change.

Comment by Michael Blume [ 13/Aug/15 1:29 PM ]

With this patch, with-meta doesn't realize the seq, but realization still only happens once – would this be an acceptable approach?

Comment by Michael Blume [ 19/Aug/15 4:46 PM ]

Added test





[CLJ-1799] Replace refs in pprint Created: 13/Aug/15  Updated: 14/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, print

Attachments: Text File CLJ-1799.patch    

 Description   

I noticed that pprint uses refs and dosync transactions in a number of places, which seems unlikely to be necessary. It seems like these could be replaced by atoms, or even volatiles, given that printing typically happens in a single thread. Presumably this would improve performance of pprint significantly.



 Comments   
Comment by dennis zhuang [ 14/Aug/15 11:28 AM ]

I develop a patch to fix this issue.I run all the tests in clojure and clojure.data.json, and no one fails.

Use criterium to do a simple benchmark as below:

(use 'criterium.core)
(require '[clojure.data.json :as json])
(bench (json/write-str 
  {:a 1 :b 2 :c (range 10) :d "hello world"
   :e (apply hash-set (range 10))}))

before patch:

Evaluation count : 6180060 in 60 samples of 103001 calls.
             Execution time mean : 10.302604 µs
    Execution time std-deviation : 597.958933 ns
   Execution time lower quantile : 9.631444 µs ( 2.5%)
   Execution time upper quantile : 11.618551 µs (97.5%)
                   Overhead used : 1.724553 ns

After patch:

Evaluation count : 6000900 in 60 samples of 100015 calls.
             Execution time mean : 10.212543 µs
    Execution time std-deviation : 564.874941 ns
   Execution time lower quantile : 9.528383 µs ( 2.5%)
   Execution time upper quantile : 11.334033 µs (97.5%)
                   Overhead used : 1.827143 ns




[CLJ-1798] The RetryEx in LockingTransaction should be static Created: 13/Aug/15  Updated: 13/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: STM, performance
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.10.4


Attachments: PNG File profile.png     Text File static_retryex.patch    
Patch: Code
Approval: Triaged

 Description   

We are using clojure.data.json, and we profiled our project with jprofiler, it shows that there is a hotspot in LockingTransaction.Too many RetryEx instances were created during the profiling.
The retryex instance variable should be static as a class variable to avoid creating when a new STM transaction begins.

The attacments are the profile result screen snapshot and the patch to make the retryex to be static.
I don't do any benchmark right now,but maybe a clojure.data.json performance benchmark will approve the patch works better.



 Comments   
Comment by Alex Miller [ 13/Aug/15 8:04 AM ]

I think the suggestion here is sound, but I have a hard time believing it will make much of a difference. My real question is why pprint is using dosync.

Comment by dennis zhuang [ 13/Aug/15 9:03 AM ]

I found it uses many dosync in writer as below:

https://github.com/clojure/clojure/blob/master/src/clj/clojure/pprint/pretty_writer.clj

It seems that using the dosync to change some mutable states.Maybe they can be rewrote into other forms such as atom,java object/lock etc.
But it's another story.





[CLJ-1796] Protocol functions fail to find future extensions when assigned to a local or new var Created: 08/Aug/15  Updated: 10/Aug/15

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

Type: Defect Priority: Minor
Reporter: Nathan Marz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Approval: Triaged

 Description   
(defprotocol TestProtocol
  (tester [o]))

(let [t tester]
  (defn another-tester [o]
  	(t o)))

(def another-tester2 tester)

(extend-protocol TestProtocol
  String
  (tester [o] (println "Strings work!")))

(another-tester "A") ;; Error
(another-tester2 "A") ;; Error
(tester "A") ;; Works fine

(let [t tester]
  (defn another-tester [o]
  	(t o)))

(another-tester "A") ;; Works fine

(def another-tester2 tester)

(another-tester2 "A") ;; Works fine

(extend-protocol TestProtocol
  Long
  (tester [o] (println "Longs work!")))

(another-tester "A") ;; Works fine
(another-tester 3) ;; Error
(another-tester2 3) ;; Error


 Comments   
Comment by Nathan Marz [ 08/Aug/15 12:47 PM ]

This issue appears to be Clojure specific – I did some testing in CLJS and was unable to reproduce the issue.

Comment by Ghadi Shayban [ 09/Aug/15 9:51 AM ]

Nathan,
Not sure if you tried this, but using:

(def another-handle #'the-protocol-function)
rather than dereffing outright.

Comment by Nathan Marz [ 09/Aug/15 6:25 PM ]

That's a good workaround but it does seem that my test case should work. I ran into this because I was passing around functions dynamically and saving them for later execution – and this issue popped up with protocol methods. Having to pass around protocol methods differently than regular functions doesn't seem right.

Comment by Kevin Downey [ 10/Aug/15 11:21 AM ]

this is a result of the protocol implementation in clojure, protocol extension mutates the vars, once you have taken then value of the var (which happens once for top level forms) you will not see further mutations of the var so no more protocol extension





[CLJ-1792] array-map and hash-map differ in handling of keys comparing identical but not equal Created: 04/Aug/15  Updated: 04/Aug/15

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-do-pointer-check-in-Util.EquivPred.patch    
Patch: Code

 Description   
user=> (let [a (array-map Double/NaN 1)] (assoc a (key (first a)) "foo"))
{NaN 1, NaN "foo"}
user=> (let [a (hash-map Double/NaN 1)] (assoc a (key (first a)) "foo"))
{NaN "foo"}

Approach: Array-map's comparison skips identity-check and always delegates to .equals calls. The attached patch alignes array-map behaviour with hash-map by doing the appropriate pointer checks before delegating to .equals






[CLJ-1791] Issue defining a defrecord protocol method named "clear" Created: 04/Aug/15  Updated: 08/Aug/15

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There seems to be a problem in trying to define a protocol with a method named "clear"

(defprotocol PClear
(clear [o]))
=> PClear

(defrecord Foo []
PClear
(clear [o] o))
=> CompilerException java.lang.ClassFormatError: Duplicate method name&signature in class file xxxx/Foo, compiling:(NO_SOURCE_PATH:1:1)

I assume this is due to a name conflict with the Java method Collection.clear() in the underlying implementation. However the error is very unclear about this, and the potential for conflict appears to be undocumented as far as I can see.

There seem to be two possible approaches to fixing this:
a) Disallow the use of "clear" as a protocol method name (in which case the error should be more informative, and the rule should be documented)
b) Find a way to support this in the class file format (possibly by overloading on JVM return types, since Collection.clear() returns void??)



 Comments   
Comment by Nicola Mometto [ 04/Aug/15 6:58 AM ]

Mike, the jvm doesn't support return type overloading so your second suggestion is not technically possible.

Reading the doc for defrecord " The class will have implementations of several (clojure.lang)
interfaces generated automatically: IObj (metadata support) and
IPersistentMap, and all of their superinterfaces.
"

Perharps java.util.Collection (or even better, java.util.Map) should be mentioned here.

Comment by Alex Miller [ 04/Aug/15 7:46 AM ]

I think this should be a doc enhancement request.

Comment by OHTA Shogo [ 08/Aug/15 3:42 AM ]

It might be out of the scope of this ticket, but protocol method conflicts can cause some other kinds of errors:

user=> (defprotocol P1 (finalize [this]))
P1
user=> (defrecord R1 [] P1 (finalize [this]))

CompilerException java.lang.VerifyError: (class: user/R1, method: finalize signature: ()Ljava/lang/Object;) Unable to pop operand off an empty stack, compiling: ...
user=> (defprotocol P2 (wait [this]))
P2
user=> (defrecord R2 [] P2 (wait [this]))
user.R2
user=> (def r (->R2))
#'user/r
user=> (wait r)
CompilerException java.lang.IllegalArgumentException: No single method: wait of interface: user.P2 found for function: wait of protocol: P2, compiling: ...
user=>

IMHO it would be nicer if defprotocol would warn method conflicts with a more informative message.





[CLJ-1789] Use transients with select-keys if possible Created: 28/Jul/15  Updated: 28/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance


 Description   

Currently select-keys uses conj to add entries. If the map is editable, conj! could be used instead to improve select-keys performance.

Additionally keyseq is traversed as a seq but could be traversed via reduce instead, which might be faster.






[CLJ-1776] Test that collections are valid statements Created: 08/Jul/15  Updated: 08/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: compiler, test

Attachments: Text File clj-1776-v1.patch    
Patch: Code and Test

 Description   

It's possible to break the compiler such that vectors are emitted incorrectly when they're in statement position (I accidentally did this). This doesn't break any part of the Clojure test suite, but does break valid Clojure code (for me it hit taoensso's encore). Add tests to the test suite so defects of this kind are caught.






[CLJ-1775] Add any? Created: 07/Jul/15  Updated: 09/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Joshua Griffith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Given the presence of `every?` and `not-every?`, it seems that `any?` should also exist given `not-any?`. While similar to `some`, indeed `(def any? (comp boolean some))`, it would make the interface consistent. Also, having `any?` would be nice when writing typed Clojure, where one often wants a boolean.



 Comments   
Comment by Colin Taylor [ 09/Jul/15 5:34 PM ]

Previous discussion - https://groups.google.com/d/msg/clojure/02msnrXJsSg/BdgfoeyVX7sJ.
My bad example notwithstanding, I still think this is symmetric and useful for interop. The typed Clojure point is valid too..





[CLJ-1768] quote of an empty lazyseq produces an error when evaled Created: 24/Jun/15  Updated: 24/Jun/15

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

Type: Defect Priority: Minor
Reporter: Tim Engler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   
user=> (eval `'())
()
user=> `'~(map identity ())
(quote ())
user=> (eval `'~(map identity ()))    ;; expected: ()
CompilerException java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)
user=> (prn *e)
#error {
 :cause "Unknown Collection type"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)"
   :at [clojure.lang.Compiler analyzeSeq "Compiler.java" 6730]}
  {:type java.lang.UnsupportedOperationException
   :message "Unknown Collection type"
   :at [clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]}]
 :trace
 [[clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]
  [clojure.lang.Compiler$BodyExpr emit "Compiler.java" 5905]
  [clojure.lang.Compiler$FnMethod doEmit "Compiler.java" 5453]
  [clojure.lang.Compiler$FnMethod emit "Compiler.java" 5311]
  [clojure.lang.Compiler$FnExpr emitMethods "Compiler.java" 3843]
  [clojure.lang.Compiler$ObjExpr compile "Compiler.java" 4489]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 3983]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6721]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler eval "Compiler.java" 6779]
  [clojure.lang.Compiler eval "Compiler.java" 6745]
  [clojure.core$eval invoke "core.clj" 3081]
  ;; elided rest
nil
user=> (eval `'~(map identity '(x)))
(x)

Cause: In the empty list case, the compiler here sees a LazySeq. I suspect something earlier in the stack should be producing an empty list instead, but haven't tracked it back yet.






[CLJ-1767] Documentation Issues with sort Created: 22/Jun/15  Updated: 22/Jun/15

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

Type: Defect Priority: Minor
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

The documentation for sort seems to be incomplete:

  • sort can return nil in some situations. There is a discussion in CTYP-228 with more backstory. There is a repro case here: http://sprunge.us/VIFc?clj supplied by Nicola Mometto. The doc string states that sort "Returns a sorted sequence of the items in coll.", which does not indicate that sort can return nil.
  • It is stated that the "comparator must implement java.util.Comparator.", but this is not true - the comparator can be any IFn that can accept 2 arguments and return either a Boolean or a Number.

For the first issue (nil return), changing the implementation to never return nil might be the neatest fix. For the second issue, the docstring could reference a description of how what functions are valid comparison functions, which can be referenced by other functions that also accept comparators (e.g., sort-by, sorted-set-by, sorted-map-by).



 Comments   
Comment by Alex Miller [ 22/Jun/15 7:51 AM ]

The first issue is being covered by CLJ-1763, so I would remove it from the ticket.

The second is technically true - Arrays.sort() will invoked and it takes a Comparator. The tricky bit is that AFn base class implements Comparator so all function implementations that extend from it that support a 2-arg function satisfy this constraint. But it might be helpful to call that out better.





[CLJ-1764] partition-by runs infinite loop when one element of infinite partition is accessed Created: 19/Jun/15  Updated: 23/Jul/15

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

Type: Defect Priority: Minor
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie

Attachments: Text File clj-1764.patch    
Approval: Triaged

 Description   
(def x (partition-by zero? (range)))
(first x)
;-> (0)
(first (second x))
;-> infinite loop

The reason is that partition-by counts and thus realizes each current partition to call itself recursively.

It seems like unexpected behavior, since the user may not intend to realize the entire partition.

It can be fixed by changing seq to lazy-seq in its last line.



 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:36 PM ]

Patch as suggested by Leon, + test.





[CLJ-1763] clojure.core/sort is not thread-safe on Java collections with backing arrays Created: 19/Jun/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File 0001-CLJ-1763-make-sort-thread-safe.patch    
Patch: Code
Approval: Triaged

 Description   

If a (mutable) Java collection that exposes it's backing array is passed to c.c/sort in multiple threads, the collection will be concurrently modified in multiple threads.

user=> (def q (java.util.concurrent.ArrayBlockingQueue. 1))
#'user/q
user=> (future (loop [] (.add q 1) (.remove q 1) (recur)))
#object[clojure.core$future_call$reify__4393 0x4769b07b {:status :pending, :val nil}]
user=> (take 3 (distinct (repeatedly #(sort q))))
((1) () nil)

Approach: Convert coll to a seq before converting it to an array, thus preserving the original collection.

Patch: 0001-CLJ-1763-make-sort-thread-safe.patch

Alternate approaches:

1. Document in sort that, like Java arrays, Java collections backed by arrays are modified in-place.
2. Change RT.toArray() to defensively copy the array returned from a (non-IPersistentCollection) Java collection. This has a number of potential ramifications as this method is called from several paths.
3. For non-Clojure collections, could also use Collections.sort() instead of dumping to array and using Arrays.sort().



 Comments   
Comment by Alex Miller [ 19/Jun/15 10:55 AM ]

The docstring says "If coll is a Java array, it will be modified. To avoid this, sort a copy of the array." which also seems like solid advice in this case.

Creating a sequence view of the input collection would significantly alter the performance characteristics.

Comment by Alex Miller [ 19/Jun/15 10:59 AM ]

I guess what I'm saying is, we should not make the performance worse for persistent collections in order to make it safer for arbitrary Java collections. But it may still be useful to make it safer without affecting persistent collection performance and/or updating the docstring.

Comment by Nicola Mometto [ 19/Jun/15 11:02 AM ]

Alex, no additional sequence is being created, the seq call was already there

Comment by Alex Miller [ 19/Jun/15 11:53 AM ]

Well, that's kind of true. The former use did not force realization of the whole seq, just the first element. That said, from a quick test the extra cost seems small on a set (vector seqs are actually faster due to their structure).

Comment by Stuart Halloway [ 30/Jul/15 9:11 AM ]

I think this should be a docstring change, if anything at all.





[CLJ-1762] Implement IKVReduce for java.util.map Created: 18/Jun/15  Updated: 18/Jun/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Chen Guo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, performance

Attachments: Text File reduce-kv-java-map.patch    
Patch: Code
Approval: Triaged

 Description   

reduce works on Java maps, but reduce-kv does not:

user=> (def clojure-map {1 2 3 4})
#'user/clojure-map
user=> (def java-map (java.util.HashMap. {1 2 3 4}))
#'user/java-map
user=> (reduce (fn [sum [k v]] (+ sum k v)) 0 java-map)
10
user=> (reduce-kv + 0 clojure-map)
10
user=> (reduce-kv + 0 java-map)

IllegalArgumentException No implementation of method: :kv-reduce of protocol: #'clojure.core.protocols/IKVReduce found for class: java.util.HashMap  clojure.core/-cache-protocol-fn\
 (core_deftype.clj:544)

It's trivial to destructure arguments in a regular reduce, but there are performance implications. The following example yields a 7x speed up when run with the implementation of reduce-kv for java.util.Map as implemented in this patch:

user=> (def big-clojure-map (into {} (map #(vector % %) (range 10000))))
#'user/big-clojure-map
user=> (def big-java-map (java.util.HashMap. big-clojure-map))
Reflection warning, /tmp/form-init7130245387362554027.clj:1:19 - call to java.util.HashMap ctor can't be resolved.
#'user/big-java-map
user=> (defn reduce-sum [m] (reduce (fn [sum [k v]] (+ sum k v)) 0 m))
#'user/reduce-sum
user=> (defn reduce-sum-kv [m] (reduce-kv (fn [sum k v] (+ sum k v)) 0 m))
#'user/reduce-sum-kv
user=> (time (dotimes [_ 1000] (reduce-sum big-java-map)))
"Elapsed time: 2624.692113 msecs"
nil
user=> (time (dotimes [_ 1000] (reduce-sum-kv big-java-map)))
"Elapsed time: 376.802454 msecs"
nil





[CLJ-1759] macroexpand throws runtime exception on symbol bound to a class Created: 17/Jun/15  Updated: 18/Jun/15

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

Type: Defect Priority: Minor
Reporter: W. David Jarvis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

OSX 10.10.3, Leiningen 2.5.1, Java 1.8.0_45 64-bit.


Attachments: Text File clj-1759.patch    
Patch: Code
Approval: Triaged

 Description   

The use of macroexpand on short class name symbols triggers a RuntimeException.

user=> (import 'java.net.URI)
java.net.URI
user=> (macroexpand '(java.net.URI "http://google.com")) ;; fine
(java.net.URI "http://google.com")
user=> (macroexpand '(URI "http://google.com")) ;; huh?
java.lang.RuntimeException: Expecting var, but URI is mapped to class java.net.URI
user=> (pst *e)
RuntimeException Expecting var, but URI is mapped to class java.net.URI
	clojure.lang.Util.runtimeException (Util.java:221)
	clojure.lang.Compiler.lookupVar (Compiler.java:7092)
	clojure.lang.Compiler.isMacro (Compiler.java:6571)
	clojure.lang.Compiler.macroexpand1 (Compiler.java:6626)
	clojure.core/macroexpand-1 (core.clj:3870)
	clojure.core/macroexpand (core.clj:3879)

Neither of these should throw an error during macroexpansion (basically should be same after expansion. Both should throw the same error when evaluated (ClassCast trying to invoke a Class as an IFn).

Approach: Throw the runtime error in lookupVar only if internNew is true. In that case we unexpectedly found something other than a var and should still report. Otherwise, just let lookupVar flow through to return a null (no var found.



 Comments   
Comment by Alex Miller [ 18/Jun/15 6:19 AM ]

The compiler is trying to determine if the thing in function position is a var that is a macro that requires expansion in Compiler.isMacro().

In the case of (java.net.URI "http://google.com"), lookupVar determines that java.net.URI is an unmapped symbol and does nothing, meaning no expansion is necessary (this of course will fail at evaluation time with "ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn").

In the case of (URI "http://google.com"), lookupVar finds a symbol mapped to something that's not a var and throws the RuntimeException that is seen.

I would expect that neither of these should throw an error during macroexpansion (basically the same thing they start as) and that both should throw the same error when evaluated. Attaching a patch that will only throw the error if internNew - in that case you unexpectedly found something other than a var and you should still report (otherwise, just return null - lookupVar didn't find a var).

The internNew case comes up with:

(import java.net.URI) 
(def URI "abc") ;; java.lang.RuntimeException: Expecting var, but URI is mapped to class java.net.URI

with the patch:

user=> (macroexpand '(java.net.URI "http://google.com")) 
(java.net.URI "http://google.com") 
user=> (macroexpand '(URI "http://google.com")) 
(URI "http://google.com") 
user=> (java.net.URI "http://google.com") 
ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn user/eval9 (NO_SOURCE_FILE:6) 
user=> (URI "http://google.com") 
ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn user/eval11 (NO_SOURCE_FILE:7)




[CLJ-1755] Calling nth on TransientVector with a default will not check if the transient has been made persistent Created: 16/Jun/15  Updated: 16/Jun/15

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

Type: Defect Priority: Minor
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transient

Attachments: Text File transient-vector-nth.patch    
Patch: Code
Approval: Triaged

 Description   

Invoking nth with arity two on a TransientVector will ensure that the transient is editable. However, invoking with arity three will return the supplied not-found value if the index is out of range.



 Comments   
Comment by Alex Miller [ 16/Jun/15 9:42 AM ]

Can you add an example to the description and a test to the patch?





[CLJ-1752] realized? return true for an instance that is not IPending Created: 09/Jun/15  Updated: 09/Jun/15

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

Type: Enhancement Priority: Minor
Reporter: Logan Linn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

To safely test if an arbitrary seq is realized (non-lazy), we need a wrapper like:

(defn seq-realized? [s]
  (if (instance? clojure.lang.IPending s)
    (realized? s)
    true))

If realized? returned true for an (ISeq?) instance that is not IPending there would be less surprising behavior for cases such as, (realized? (range 10)) which throws exception.

NB: A follow-up to CLJ-1751.






[CLJ-1748] Change clojure.core/reverse to return rseq for args that are Reversible Created: 07/Jun/15  Updated: 07/Jun/15

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There may be issues with this suggestion about concrete types of return values, or doc strings that promise things that you want to preserve that cannot be preserved with this suggested change.

However, if those issues are not show stoppers, changing clojure.core/reverse to check if its arg is Reversible, and if so, return rseq, else do as reverse does today, could be faster in many situations.






[CLJ-1747] Eduction's print-method expects its collection to be Iterable/Sequential Created: 07/Jun/15  Updated: 07/Jun/15

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

Type: Defect Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1747-eduction-print.patch    
Patch: Code

 Description   

eduction expects its source collection to be Iterable [1], and its print-method goes through print-sequential [2]. This implies a promise that may restrict the use-case of an eduction over a virtual collection, e.g. an IReduceInit source that may be backed by I/O or some other resource. I have found it useful to construct these I/O reducibles and wrap them with an eduction. If you are careful not to call seq on it, this works fine. However, printing it at the REPL will call seq. Does the print-method impl for eduction promise too much? This is a only minor annoyance more than anything else, obviously I could create my own flavor of eduction.

Totally hypothetical example:

(defn database-index
  [name]
  (reify clojure.lang.IReduceInit
    (reduce [_ f init]
      (with-open [rdr (fressian/create-reader (io/input-stream name))]
       (loop [] ...reduce impl...)))))

(eduction (filter (as-of #inst "2012-01-01")) (database-index "eavt.fress"))
;; ^ throws when printed by repl

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7336-L7338
[2] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L7360

Proposed Approach:
Let eduction print like an opaque #object






[CLJ-1746] new keyword for `require` that both refers other namespace's symbol and exclude the same in clojure.core Created: 06/Jun/15  Updated: 09/Jun/15

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

Type: Enhancement Priority: Minor
Reporter: Hoang Minh Thang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: enhancement


 Description   

I find myself repeat code like this

(ns foo.bar
(:refer-clojure :exclude [doseq])
(:require [clojure.core.typed :refer [doseq]]))

and just think why not something like:

(ns foo.bar
(:require [clojure.core.typed :override [doseq]]))



 Comments   
Comment by Mike Anderson [ 09/Jun/15 10:40 AM ]

I agree this is very annoying.

I think it is a duplicate of my issue though: The patch for CLJ-1257 would make this unnecessary (it would allow the user to override any vars, without getting an exception).





[CLJ-1737] Omit java exception class from CompilerException message Created: 23/May/15  Updated: 23/May/15

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

Type: Enhancement Priority: Minor
Reporter: John Hume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs, patch, usability

Attachments: Text File clearer-CompilerException-messase.patch     File compiler_exception_examples.clj    
Patch: Code

 Description   

A CompilerException is always created with a cause exception. Currently the message is built using cause.toString(), which for all examples I've examined is the cause class, followed by a colon, followed by the cause message. In all those examples, the message of the cause is informative, and the class name provides no additional help.

I propose to switch to using cause.getMessage() rather than cause.toString(). This would make it easier for tools to present compiler errors that don't leak implementation details that may confuse a new user. The cause class would still be shown in the stack trace.

Here are the examples I looked at, with the output from before the attached patch:

Example source '(ns foo)

(def'
Exception message:
 java.lang.RuntimeException: EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 java.lang.RuntimeException: Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 java.lang.RuntimeException: Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 java.lang.RuntimeException: No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 java.lang.IllegalArgumentException: Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.ClassCastException: java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 java.lang.RuntimeException: Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 java.lang.NumberFormatException: Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

And the output with the attached patch applied:

Example source '(ns foo)

(def'
Exception message:
 EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)





[CLJ-1734] Display more descriptive error message when trying to use reader conditionals in a non-cljc file Created: 19/May/15  Updated: 20/May/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, reader


 Description   

I spent a few puzzled minutes trying to understand the following message from the Clojure compiler:

CompilerException java.lang.RuntimeException: Conditional read not allowed, compiling: <filename>

Eventually I realised it was because I was trying to use reader conditionals in a .clj file that I hadn't renamed to cljc. I think it would be really helpful for people working in mixed clj and cljc codebases to have this error message extended to something like:

"Conditional read not allowed because file does not have extension .cljc"



 Comments   
Comment by Alex Miller [ 19/May/15 11:45 PM ]

The reader doesn't know this - it can be called in multiple ways (from repl, via clojure.core/read, via clojure.core/read-string, load/compile .cljc, load/compile .clj) so that description would actually be wrong in some of those. It seems like you're getting a pretty good error message already - it told you the problem and gave you the file name.

The message could be tweaked to something like "Reader conditionals not allowed in this context" which might give you a better clue.

Comment by Daniel Compton [ 20/May/15 3:12 PM ]

Perhaps I'm not understanding how the reader determines whether reader conditionals are allowed or not, but those would all seem to have different reasons for not being allowed and would be caught by different checks. Each of these checks could give a more specific warning explaining why the read wasn't allowed?

Counteracting my point, it looks like there is only one place where this exception is thrown - https://github.com/clojure/clojure/blob/7b9c61d83304ff9d5f9feddecf23e620c0b33c6e/src/jvm/clojure/lang/LispReader.java#L1406. I'm not sure if this could be extended to give more details in different error cases or if that information isn't available at that point?

Comment by Alex Miller [ 20/May/15 3:36 PM ]

The reader is invoked with an options map which will (or will not) have {:read-cond :allow} or {:read-cond :preserve}. That's the only info the reader has - if either of those is set and a reader conditional is encountered, it throws.

The compiler decides how to initialize these options when it's calling the reader. Users of read and read-string similarly decide which options are allowed when they call it. It would be possible to pass more info into the reader or to catch and rethrow in the compiler where more context is known, but both of those complicate the code for what is already a decent error imho.

Comment by Daniel Compton [ 20/May/15 4:03 PM ]

I agree it is a reasonable error message, I guess we can wait and see how other people find it once 1.7 is released. If it turns out to be an issue for lots of other people then we could revisit this then?





[CLJ-1732] Add docstring explanation of (isa? [x1 x2...] [parent1 parent2...]) Created: 17/May/15  Updated: 17/May/15

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The "Multimethods and Hierarchies" page mentions that "isa?" has special behavior when aimed at two vectors[1]. But the docstring of "isa?" does not mention it[2].

[1] http://clojure.org/multimethods
[2] http://clojure.github.io/clojure/clojure.core-api.html#clojure.core/isa?






[CLJ-1724] Reuse call to seq() in LazySeq/hashcode for else case Created: 04/May/15  Updated: 04/May/15

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

Type: Enhancement Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, ft, performance

Attachments: File clj-1724.diff    
Patch: Code
Approval: Triaged

 Description   

In LazySeq/hashCode, seq() is called twice for non-empty seqs. First call to seq() can be reused in else case.






[CLJ-1715] Use AFn.applyToHelper rather than IFn.applyTo in InvokeExpr.eval Created: 23/Apr/15  Updated: 23/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1715-Use-AFn.applyToHelper-rather-than-IFn.apply.patch    
Patch: Code

 Description   

Because of implementation details of how def forms are compiled, invokations in its args are evaluated using applyTo rather than directly using the invoke method. This forces IFn implementors to implement applyTo even when not necessary.

The proposed patch changes InvokeExpr.eval to use AFn.applyToHelper, so that invoke rather than applyTo is used when possible.

Example of currently failing code that will work with patch:

user=> (deftype x [] clojure.lang.IFn (invoke [_] 1))
user.x
user=> (def a ((x.)))
AbstractMethodError   clojure.lang.Compiler$InvokeExpr.eval (Compiler.java:3553)





[CLJ-1714] Some static initialisers still run at compile time if used in type hints Created: 22/Apr/15  Updated: 12/Aug/15

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, typehints

Attachments: Text File CLJ-1714.patch     Text File CLJ-1714-v2.patch    
Patch: Code
Approval: Triaged

 Description   

AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time.

I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).



 Comments   
Comment by Alex Miller [ 22/Apr/15 10:53 AM ]

I think this might have been logged already but I'm not sure.

Comment by Michael Blume [ 22/Apr/15 12:30 PM ]

Patch won't apply to master for me

Comment by Adam Clements [ 22/Apr/15 2:39 PM ]

Really sorry, don't know what happened there. I checked out a fresh copy of the repo and re-applied the changes, deleted the old patch as it was garbage. Try the new one, timestamped 2:37pm

Comment by Stuart Halloway [ 30/Jul/15 1:52 PM ]

Please add an example of the problem, and if possible a failing test.

Comment by Alex Miller [ 30/Jul/15 5:14 PM ]

Reset to "Open" as moving from Triaged->Incomplete is not valid in our current workflow.

Comment by Adam Clements [ 31/Jul/15 10:56 AM ]

Example problem:
AOT compiling on an x86 machine to be run on an ARM machine when a Java dependency has a native component and the class with the native dependency is used in a type hint.

In this situation, the only native library available on the classpath is the ARM dependency, and obviously won't load on the compiling x86 machine. Java libraries tend to load the native dependencies in the static initialiser of the class, which will fail in this situation as the architecture is x86 and the dependencies are ARM, for which reason CLJ-1315 made the change to not run static initialisers at compile time.

This covers a case which didn't come up as part of CLJ-1315, that the same problem occurs if rather than constructing the class, you simply use it as a type hint (which IMO is doubly surprising as something to have a side-effect).

This patch fixes that - happy to try and create a test, but would appreciate some advice on the shape such a test would take - presumably loading a java native library would be undesirable. I could simply check for static initialisers being run, but first would need some agreement that this is universally undesirable at compile time.

I have been using this patch in production for over a year with no adverse effects (as has anybody using the clojure-android build of clojure).

Comment by Stuart Halloway [ 31/Jul/15 11:34 AM ]

Hi Adam,

Thanks for the quick response. I think checking for static initializers being run is OK for a test.

Comment by Adam Clements [ 12/Aug/15 9:12 AM ]

Added failing tests which now pass





[CLJ-1708] Volatile mutable in deftype is not settable when using try..finally and returning this Created: 17/Apr/15  Updated: 31/Jul/15

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

Type: Defect Priority: Minor
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype
Environment:

clojure 1.6.0, clojure 1.7.0-beta1


Approval: Triaged

 Description   

Reproducible Code: https://gist.github.com/patrickgombert/1bcb8a051aeb3e82d855

When using a volatile-mutable field in deftype, compilation fails if the field is set! in a method call that uses both try..finally and returns itself from the method call. Leaving out either the try..finally or returning itself from the method causes compilation to succeed.

Expected behavior: set! should set the volatile-mutable variable and compilation should succeed.



 Comments   
Comment by Kevin Downey [ 17/Apr/15 7:15 PM ]

this must be the same issue as CLJ-1422 and CLJ-701, it has nothing to do with returning `this`, but with the try being in a tail position or not. if the try is not in a tail position the compiler hoists it out in to a thunk. effectively the code is

(deftype SomeType [^:volatile-mutable foo]
  SomeProtocol
  (someFn [_] ((fn [] (try (set! foo 1))))))

which the compiler also rejects, because it doesn't let you mutate fields from functions that are not the immediate protocol functions





[CLJ-1705] vector-of throws NullPointerException if given unrecognized type Created: 14/Apr/15  Updated: 06/May/15

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

Type: Defect Priority: Minor
Reporter: John Croisant Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs, newbie
Environment:

MacOS X Version 10.9.5

java version "1.8.0_11"
Java(TM) SE Runtime Environment (build 1.8.0_11-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.11-b03, mixed mode)


Attachments: Text File clj-1705-2.patch     Text File clj-1705-3.patch     Text File CLJ-1705.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Summary:

If the user passes an unrecognized type keyword to vector-of, it will throw a NullPointerException with no message. This gives no indication to the user of what the problem is, which can frustrate the user and make debugging harder than it needs to be.

Example:

user=> (vector-of :integer 1 2 3) ; user meant (vector-of :int 1 2 3)

NullPointerException   clojure.core/vector-of (gvec.clj:472)

Approach: Throw more informative error message than NPE with more info.

After:

user=> (vector-of :integer 1 2 3)
IllegalArgumentException Unrecognized type :integer  clojure.core/vector-of (gvec.clj:498)

Patch: clj-1705-3.patch



 Comments   
Comment by Johan Mena [ 05/May/15 12:12 AM ]

Here’s my proposed solution: https://github.com/jhn/clojure/commit/0522fffd7893e8c79969f5c6ac91a333484c8e23 — It works as expected and the tests pass.

One more thing I'm not sure about is how to create a patch for this. The ticket mentions "Release 1.4, Release 1.6” as affected versions, but I found this to still be present in 1.7 and in 1.5 too. Do I checkout only these two tags (1.4, 1.6) from the git repo and create a patch for each one separately? Or what's the usual way to go about it? I read through the Issue Tracking section of the wiki but it's still not very clear.

Thanks.

Johan

Comment by Alex Miller [ 05/May/15 7:09 AM ]

in general, just work from master. We don't generally back port to older versions.

You should make a patch following the instructions at http://dev.clojure.org/display/community/Developing+Patches

Comment by Andy Fingerhut [ 05/May/15 10:20 AM ]

Johan, are you listed as "johan (jhn)" on the contributor list here? http://clojure.org/contributing

If so, it would be good to update that listing to something like "Johan Mena (jhn)" for the time when someone needs to check that you have signed a Clojure CA, before your patch is committed.

Comment by Alex Miller [ 05/May/15 10:35 AM ]

I believe that's correct - I have updated the contributing page.

Comment by Johan Mena [ 05/May/15 1:57 PM ]

The patch I attached yesterday did start from master, so I think it's good to go for review.

Thanks Alex & Andy!

Comment by Alex Miller [ 06/May/15 9:56 AM ]

Ok, I looked at the patch. Functionally, seems ok other than that the find+destructuing doesn't seem to have a use over get.

However, I also looked at performance for the happy path using criterium and the following test:

(quick-bench (vector-of :int 1 2))

Before the patch: 27.418350 ns
After the patch: 79.883695 ns

The reason for this is that the prior code created a single map and constructed the array managers in there once, whereas the patch causes the array manager to be created each time through the code.

Re-extracting the map and replacing the find with get, I was back to: 34.320916 ns

I think that's too much of a hit for this change so I looked at a couple variants:

  • def'ing each am separately and using a `case` expression - 36.566750 ns
  • using `or` instead of if-let - 29.680252 ns

I think that last one is pretty close, but we're paying a hit just to invoke the new function, so my final stab was turning that into a macro - 28.411626 ns, which seems tolerable to me. Because I used a macro, I also had to move the type hints in vector-of to avoid reflection.

Since I had everything sitting here, I went ahead and made a new patch. I feel bad about replacing yours, but not sure what else makes sense to do.

Comment by Alex Miller [ 06/May/15 10:02 AM ]

Ok, I split up the patch into test and code commits so you have credit for some of the changes! Since it's got my stuff in here, this will have to wait till it gets added to 1.8 to get screened by someone else.

Comment by Johan Mena [ 06/May/15 11:04 AM ]

Haha, you shouldn't have bothered, but thanks!

And thanks for the detailed explanation! Actually 'get' was my first approach too but the (get map key not-found) form kept throwing the exception in the `not-found` part even in the happy case. I asked around and found out the else part needs to be evaluated in this case too. Someone suggested using a macro but I'm not very familiar with that just yet, so I went with if-let, which seemed readable. Just out of curiosity, when you tried the get approach, did you use (get map key) and check for nil to throw the exception?

I ran my code through the irc channel and got positive feedback, so completely forgot about performance. Will keep it in mind next time.





[CLJ-1704] Clarify cond documentation to explain about using :else Created: 14/Apr/15  Updated: 14/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

The documentation for cond doesn't explicitly mention that you can use :else (or any other keyword) to catch any values that don't match the previous conditions. While it is true that the documentation does say that a test will evaluate and return the value of logical true, it could be more helpful by pointing out that a keyword like :else will always be logical true.

I'm not 100% sure about whether this is necessary, but wanted to see what others thought and whether it would be helpful or not.






[CLJ-1693] into: merge metadata Created: 03/Apr/15  Updated: 03/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Gregg Reynolds Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: function, interop
Environment:

all


Approval: Triaged

 Description   

Currently (into to from) preserves to's metadata but discards from's metadata. The enhancement would be to have 'into' do something like (merge (meta to) (meta from)). Justification: as with data, so with metadata. Use case: Using deftype, I have a class EntityMap that clojurizes a native Java class (App Engine's Entity class), making it behave just like a clojure map. This includes using into to convert an EntityMap to an ordinary PersistentMap; the problem is that key information for the EntityMap is really metadata, so I need (into {} em) to put that metadata into the new PersistentMap.

See also CLJ-916






[CLJ-1688] Object instance members should resolve to Object Created: 30/Mar/15  Updated: 30/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reflection, typehints


 Description   
(defn unparse-pattern ^String [pattern] (.toString pattern))
Reflection warning, ring/swagger/coerce.clj:22:41 - reference to field toString can't be resolved.

Reflection isn't really necessary here, we could just special-case the methods on Object.






[CLJ-1687] Clojure doesn't resolve static calls even when it has all information needed to do so Created: 30/Mar/15  Updated: 30/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reflection, typehints


 Description   

If I create a class with two methods, one of which takes (String, String), and the other taking (String, Number), and then write a function

(defn foo
  [x ^String y]
  (Thing/hello x y))

it seems obvious that I'm trying to call the first method and not the second. But on lein check, clojure prints

Reflection warning, resolve_fail/core.clj:6:3 - call to static method hello on resolve_fail.Thing can't be resolved (argument types: unknown, java.lang.String).

unless I also type-hint x.



 Comments   
Comment by Nicola Mometto [ 30/Mar/15 2:32 PM ]

I have looked at this countless times while working on tools.analyzer and hacking the reflector and found out that there doesn't seem to be a way to make things like this "work" without breaking other cases





[CLJ-1682] clojure.set/intersection occasionally allows non-set arguments. Created: 24/Mar/15  Updated: 14/Jul/15

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

Type: Defect Priority: Minor
Reporter: Valerie Houseman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs

Approval: Triaged

 Description   

clojure.set/intersection, by intent and documentation, is meant to be operations between two sets. However, it sometimes allows (and returns correct opreations upon) non-set arguments. This confuses the intention that non-set arguments are not to be used.

Here's an example with Set vs. KeySeq:
If there happens to be an intersection, you'll get a result. This may lead someone coding this to think that's okay, or to not notice they've used an incompatible data type. As soon as the intersection is empty, however, an appropriate type error ensues, albeit by accident because the first argument to clojure.core/disj should be a set.

user=> (require '[clojure.set :refer [intersection]])
nil
user=> (intersection #{:key_1 :key_2} (keys {:key_1 "na"}))   ;This works, but shouldn't
(:key_1)
user=> (intersection #{:key_1 :key_2} (keys {:key_3 "na"}))   ;This fails, because intersection assumes the second argument was a Set
ClassCastException clojure.lang.APersistentMap$KeySeq cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1449)

(disj (keys {:key_1 "na"}) #{:key_1 :key_2})   ;The assumption that intersection made
ClassCastException clojure.lang.APersistentMap$KeySeq cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1449)

Enforcing type security on a library that's clearly meant for a particular type seems like the responsible thing to do. It prevents buggy code from being unknowingly accepted as correct, until the right data comes along to step on the bear trap.



 Comments   
Comment by Andy Fingerhut [ 24/Mar/15 7:19 PM ]

CLJ-810 was similar, except it was for function clojure.set/difference. That one was declined with the comment "set/difference's behavior is not documented if you don't pass in a set." I do not know what core team will judge ought to be done with this ticket, but wanted to provide some history.

Dynalint [1] and I think perhaps Dire [2] can be used to add dynamic argument checking to core functions.

[1] https://github.com/frenchy64/dynalint
[2] https://github.com/MichaelDrogalis/dire

Comment by Alex Miller [ 24/Mar/15 9:00 PM ]

Now that `set` is faster for sets, I think we could actually add checking for sets in some places where we might not have before. So, it's worth looking at with fresh eyes.

Comment by Jason Wolfe [ 28/May/15 2:54 AM ]

Back in 2009 I submitted a patch to the set functions with explicit `set?` checks and Rich's response was "the fact that these functions happen to work when the second argument is not a set is an implementation artifact and not a promise of the interface, so I'm not in favor of the set? testing or any other accommodation of that." Not sure if that is still accurate though.





[CLJ-1680] quot and rem handle doubles badly Created: 24/Mar/15  Updated: 27/Jul/15

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

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

Attachments: Text File clj-1680_no_div0_jre17.patch    
Patch: Code and Test
Approval: Triaged

 Description   

quot and rem in the doubles case (where any one of the arguments is a floating point) gives strange results for non-finite arguments:

(quot Double/POSITIVE_INFINITY 2) ; Java: Infinity
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(quot 0 Double/NaN) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(quot Double/POSITIVE_INFINITY Double/POSITIVE_INFINITY) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(rem Double/POSITIVE_INFINITY 2) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(rem 0 Double/NaN) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(rem 1 Double/POSITIVE_INFINITY) ; The strangest one. Java: 1.0
=> NaN

quot and rem also do divide-by-zero checks for doubles, which is inconsistent with how doubles act for division:

(/ 1.0 0)
=> NaN
(quot 1.0 0) ; Java: NaN
ArithmeticException Divide by zero  clojure.lang.Numbers.quotient (Numbers.java:176)
(rem 1.0 0); Java: NaN
ArithmeticException Divide by zero  clojure.lang.Numbers.remainder (Numbers.java:191)

Attached patch does not address this because I'm not sure if this is intended behavior. There were no tests asserting any of the behavior mentioned above.

Fundamentally the reason for this behavior is that the implementation for quot and rem sometimes (when result if division larger than a long) take a double, coerce it to BigDecimal, then BigInteger, then back to a double. The coersion means it can't handle nonfinite intermediate values. All of this is completely unnecessary, and I think is just leftover detritus from when these methods used to return a boxed integer type (long or BigInteger). That changed at this commit to return primitive doubles but the method body was not refactored extensively enough.

The method bodies should instead be simply:

static public double quotient(double n, double d){
    if(d == 0)
        throw new ArithmeticException("Divide by zero");
    double q = n / d;
    return (q >= 0) ? Math.floor(q) : Math.ceil(q);
}

static public double remainder(double n, double d){
    if(d == 0)
        throw new ArithmeticException("Divide by zero");
    return n % d;
}

Which is what the attached patch does. (And I'm not even sure the d==0 check is appropriate.)

Even if exploding on non-finite results is a desirable property of quot and rem, there is no need for the BigDecimal+BigInteger coersion. I can prepare a patch that preserves existing behavior but is more efficient.

More discussion at Clojure dev.



 Comments   
Comment by Francis Avila [ 24/Mar/15 12:55 PM ]

More testing revealed that n % d does not preserve the relation (= n (+ (* d (quot n d)) (rem n d))) as well as (n - d * (quot n d)), which doesn't make sense to me since that is the very relation the spec says % preserves. % is apparently not simply Math.IEEEremainder() with a different quotient rounding.

Test case: (rem 40.0 0.1) == 0.0; 40.0 % 0.1 == 0.0999... (Smaller numerators will still not land at 0 precisely, but land closer than % does.)

Updated patch which rolls back some parts of the simplification to remainder and adds this test case.

Comment by Andy Fingerhut [ 04/Jul/15 12:12 AM ]

Francis, your patch clj-1680_no_div0.patch dated 2015-Mar-24 uses the method isFinite(), which appears to have been added in Java 1.8, and does not exist in earlier versions. I would guess that while the next release of Clojure may drop support for Java 1.6, it is less likely it would also drop support for Java 1.7 at the same time. It might be nice if your patch could use something like !(isInfinite() || isNaN()) instead, which I believe is equivalent, and both of those methods exist in earlier Java versions.

Comment by Francis Avila [ 26/Jul/15 11:22 PM ]

Updated patch with a java 1.7-compatible version, also rebased against master.

No tests fail except this one, which I don't think is related to this patch:

[java] FAIL in (gen-interface-source-file) (genclass.clj:151)
     [java] expected: (= "examples.clj" (str sourceFile))
     [java]   actual: (not (= "examples.clj" ""))
Comment by Michael Blume [ 27/Jul/15 1:34 PM ]

Francis, I tried downloading your patch and I didn't see any test failures at all. Do you see the same failure if you check out the master branch from the Clojure repo? Do you still see it if you mvn clean first? If so, it might be worth opening a ticket for it and seeing if anyone else can reproduce it.

Comment by Andy Fingerhut [ 27/Jul/15 1:41 PM ]

Yes, and if you do see a failure with unmodified Clojure for 'mvn clean test', or './antsetup.sh ; ant clean; ant', please let us know the OS and JVM you are using. I haven't seen that on the OS/JVM combos I have tried.

Comment by Francis Avila [ 27/Jul/15 2:51 PM ]

Nevermind, failing test went away after a clean. All tests pass.





[CLJ-1678] Update failing tests for IBM JDK 1.7 and 1.8 Created: 19/Mar/15  Updated: 20/Mar/15

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test
Environment:

IBM JDK 1.7 and 1.8


Approval: Triaged

 Description   

For Sun/Oracle JDKs, and IBM JDK 1.6, we have this:

user=> (.hashCode 9223372039002259457N)
1

For IBM JDKs 1.7 and 1.8, it changed to this (I do not know why):

user=> (.hashCode 9223372039002259457N)
33

This causes a few example-based tests in Clojure to fail when run on those IBM JDK versions. There does not appear to be any bug in Clojure here. Those tests were written with particular constant values that are different, but have equal .hashCode values, to test Clojure's code generated that selects between branches in a case. In particular, these tests in control.clj fail:

;; line 386 in Clojure 1.6.0 and 1.7.0-master-SNAPSHOT as of Mar 19 2015:
    (is (== (.hashCode 1) (.hashCode 9223372039002259457N)))

;; and later on line 423 in the same file:
  (testing "test warn for hash collision"
    (should-print-err-message
     #"Performance warning, .*:\d+ - hash collision of some case test constants; if selected, those entries will be tested sequentially..*\r?\n"
     (case 1 1 :long 9223372039002259457N :big 2)))

There are other tests in the same file with the same constant 9223372039002259457N that do not fail with IBM JDKs 1.7 and 1.8, but they do not test hash collisions as they were intended to.

Some possibilities for what could be changed:

1. Pick a different pair of number other than 1 and 9223372039002259457N when running tests on IBM JDKs 1.7 and 1.8, so that the hash values do collide. For example, 33 and 9223372039002259457N.

2. skip these tests completely when running on IBM JDKs 1.7 and 1.8.



 Comments   
Comment by Alex Miller [ 20/Mar/15 4:03 AM ]

I think my preference would be to skip these tests for the ibm jdk.





[CLJ-1673] Improve clojure.repl/dir-fn to work on namespace aliases in addition to canonical namespaces. Created: 11/Mar/15  Updated: 04/May/15

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

Type: Enhancement Priority: Minor
Reporter: Jason Whitlark Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, repl

Attachments: Text File clj-1673-2.patch     Text File improve_dir.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Extend clojure.repl/dir to work with the aliases in the current namespace

After:

user=> (require '[clojure.string :as s])
nil
user=> (dir s)
blank?
capitalize
...etc

Patch: clj-1673-2.patch
Screened by: Alex Miller



 Comments   
Comment by Jason Whitlark [ 11/Mar/15 4:00 PM ]

Possible unit test, since clojure.string is aliased in the test file:

(is (= (dir-fn 'clojure.string) (dir-fn 'str)))

Comment by Alex Miller [ 29/Apr/15 2:12 PM ]

Please add a test...

Comment by Jason Whitlark [ 02/May/15 10:35 PM ]

Updated patch, tweaked dir-fn, added test.

Comment by Jason Whitlark [ 02/May/15 10:38 PM ]

Should be good to go. I removed the old patch.

Comment by Alex Miller [ 04/May/15 4:38 PM ]

Tweaked patch just to remove errant blank line, otherwise same.





[CLJ-1672] Better error message when passing a list to update-in Created: 11/Mar/15  Updated: 11/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: John Gabriele Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs
Environment:

OpenJDK 1.7 on GNU/Linux



 Description   

This one confused me when I'd accidentally passed a list (returned by a function) in to `update-in` instead of a vector.

Example:

some-app.core=> (update-in [:a :b :c] [1] name)
[:a "b" :c]
some-app.core=> (update-in '(:a :b :c) [1] name)

NullPointerException   clojure.core/name (core.clj:1518)

Similar result if passing in another function; for example:

some-app.core=> (update-in ["a" "b" "c"] [1] str/capitalize)
["a" "B" "c"]
some-app.core=> (update-in '("a" "b" "c") [1] str/capitalize)

NullPointerException   clojure.string/capitalize (string.clj:199)


 Comments   
Comment by Alex Miller [ 11/Mar/15 9:26 AM ]

I think this is effectively a dupe of CLJ-1107 re throwing on get with a non-Associative collection?





[CLJ-1668] ns macro throws NPE if empty reference is specified Created: 02/Mar/15  Updated: 02/Mar/15

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

Type: Defect Priority: Minor
Reporter: Philipp Meier Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, macro, namespace


 Description   

The following invocations of `ns` will all throw a NPE

(ns foo ())
(ns foo [])
(ns foo (:require clojure.core) ())

;; throw


1. Unhandled java.lang.NullPointerException
   (No message)

                      core.clj: 1518  clojure.core/name
                      core.clj: 5330  clojure.core/ns/process-reference
                      core.clj: 2559  clojure.core/map/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                       RT.java:  484  clojure.lang.RT/seq
                      core.clj:  133  clojure.core/seq
                      core.clj:  694  clojure.core/concat/cat/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                     Cons.java:   39  clojure.lang.Cons/next
                       RT.java: 1654  clojure.lang.RT/boundedLength
                      AFn.java:  148  clojure.lang.AFn/applyToHelper
                      Var.java:  700  clojure.lang.Var/applyTo

I'd expect an exception that is describing the cause of the error, not an "symptom".






[CLJ-1664] Inconsistency in overflow-handling between type-hinted and reflective calls Created: 19/Feb/15  Updated: 19/Feb/15

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: numerics, reflection


 Description   
(import 'java.io.DataOutputStream)
(import 'java.io.ByteArrayOutputStream)

(defn- ->bytes
  "Convert a Java primitive to its byte representation."
  [write v]
  (let [output-stream (ByteArrayOutputStream.)
        data-output (DataOutputStream. output-stream)]
    (write data-output v)
    (seq (.toByteArray output-stream))))

(defn int->bytes [n]
  (->bytes 
    #(.writeInt ^DataOutputStream %1 %2)
    n))

(defn int->bytes-ref [n]
  (->bytes 
    #(.writeInt %1 %2)
    n))

user=> (int->bytes 5)
(0 0 0 5)
user=> (int->bytes-ref 5)
(0 0 0 5)
user=> (int->bytes (inc Integer/MAX_VALUE))

IllegalArgumentException Value out of range for int: 2147483648  clojure.lang.RT.intCast (RT.java:1115)
user=> (int->bytes-ref (inc Integer/MAX_VALUE))
(-128 0 0 0)

So it looks like type-hinting the DataOutputStream results in bytecode calling RT.intCast, which throws because the value is too large. In the reflective case, we locate the method writeInt at runtime, and then do not call RT.intCast, but instead allow the long to be downcast without bounds checking.

It seems like we should be calling RT.intCast in both cases?






[CLJ-1661] Varargs protocol impls can be defined but not called Created: 17/Feb/15  Updated: 19/Feb/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Reno Reckling Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1661-v1.patch    
Patch: Code

 Description   

The compiler accepts this:

(deftype foo []
clojure.lang.IFn
(invoke [this & xs]))

However calling ((foo.) :bar) will throw an AbstractMethodError. Wouldn't some checking be desirable?



 Comments   
Comment by Reno Reckling [ 17/Feb/15 11:09 AM ]

This is a clone of http://dev.clojure.org/jira/browse/CLJ-1024 because the original with its attached patches was forgotten with the reason that "It has to wait and cannot be applied in 1.5" which is 2 major versions ago now, with 1.7 underway.

I would like to reopen it, or continue working on it in this ticket because i just stumbled over this issue the second time and the debugging sessions that follow this are annoying.

Comment by Andy Fingerhut [ 19/Feb/15 12:23 PM ]

Fix Version/s was Release 1.5, but that field should only be set by Clojure screeners.

Comment by Reno Reckling [ 19/Feb/15 12:41 PM ]

Yes, i just cloned the original issue. Later i realized that I'm unable to edit any of the fields.
The issue is just concerned with a missing warning/error when trying to compile protocols with "&" in the argument list as they are interpreted as a variable name "&" instead of a varargs placeholder which the user probably expects.

Comment by Michael Blume [ 19/Feb/15 2:17 PM ]

Here's a forward-port of the 1024 patch





[CLJ-1655] Dorun's behavior when called with two argument's is both unintuitive and undocumented. Created: 04/Feb/15  Updated: 04/Feb/15

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Dorun can be called as (dorun n coll). When called this way, dorun will force n+1 elements from coll, which seems unintuitive. I can't necessarily call this a defect, though. It doesn't deviate from the documented behavior because there is no documented behavior – the two-argument arity is not mentioned in the docstring.

user=> (defn printing-range [n] (lazy-seq (println n) (cons n (printing-range (inc n)))))
#'user/printing-range
user=> (dorun 0 (printing-range 1))
1
nil
user=> (dorun 3 (printing-range 1))
1
2
3
4
nil





[CLJ-1653] str of an empty list is not "()" Created: 02/Feb/15  Updated: 29/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, ft, print

Attachments: Text File clj-1653-2.patch     Text File CLJ-1653-toString-for-EmptyList.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The str of an empty list is surprisingly not "()". This is inconsistent with the result for the empty map {} or empty vector (). It would be convenient if `(str ())` returned "()". The work-around is to use `pr-str`, which is arguably the "correct" thing to do. However, there doesn't seem to be any reason that Clojure couldn't return "()".

(str ())
;=> "clojure.lang.PersistentList$EmptyList@1"

(str {} [] ())
;=> "{}[]clojure.lang.PersistentList$EmptyList@1"

;; Work-around: use `pr-str` instead of `str`

(pr-str () {} [])
"() {} []"

Patch: clj-1653-2.patch

Screened by: Alex Miller



 Comments   
Comment by Steve Miner [ 02/Feb/15 3:30 PM ]

PersistentList$EmptyList should have a toString method that returns "()".

Comment by Steve Miner [ 02/Feb/15 3:45 PM ]

add toString() for EmptyList

Comment by Steve Miner [ 02/Feb/15 3:45 PM ]

patch and test for toString() method on EmptyList.

Comment by Nicola Mometto [ 02/Feb/15 4:03 PM ]

Not sure how this is different from

user=> (str (range 10))
"clojure.lang.LazySeq@9ebadac6"

pr-str works fine on both () and (range 10) btw.

Comment by Steve Miner [ 02/Feb/15 5:09 PM ]

I agree in principle that pr-str is the right thing to use. I will counter the Slippery Slope argument by invoking the Principle of Least Astonishment. My argument for the proposed patch is that () is a common value and the current behavior is inconsistent with similar empty values, {} and []. I think it would be convenient and useful, especially for beginners, to fix just this one case of the empty list. On the other hand, it's a minor issue so I won't push it.

Comment by Michael Blume [ 02/Feb/15 5:59 PM ]
user=> (str '())
"clojure.lang.PersistentList$EmptyList@1"
user=> (str '(1 2))
"(1 2)"

This really makes empty list seem like a special case.

Comment by Alex Miller [ 29/Apr/15 2:17 PM ]

Added -2 patch just to fix trailing whitespace warnings, identical otherwise.





[CLJ-1651] Erroneous error message when using into to create a map. Created: 29/Jan/15  Updated: 29/Jan/15

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

Type: Defect Priority: Minor
Reporter: Justin Glenn Smith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting


 Description   

If you provide a sequence instead of a vector type for the entries provided to into for creating a hash-map, the error message is misleading.

org.noisesmith.orsos=> (into {} '((:a 0) (:b 1)))

ClassCastException clojure.lang.Keyword cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)

As we see, it reports the type of the first item in the entry, rather than the actual error, the type of the entry itself, which can be particularly confusing if the key in the entry is actually a valid type to be an entry:

=> (into {} '((["a" 1] ["b" 2]) (["c" 3] ["d" 4])))

ClassCastException clojure.lang.PersistentVector cannot be cast to java.util.Map$Entry clojure.lang.ATransientMap.conj (ATransientMap.java:44)






[CLJ-1649] Hash/equality inconsistency for floats & doubles Created: 23/Jan/15  Updated: 23/Jan/15

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

Type: Defect Priority: Minor
Reporter: Michael Gardner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: numerics


 Description   

This is closely related to CLJ-1036, but there was a suggestion to make a new ticket.

The issue is that for a float f and a double d, we can have (= f d) but not (= (hash f) (hash d)), which breaks a fundamental assumption about hash/equality consistency and leads to weirdness like this (from Immo Heikkinen's email to the Clojure mailing list):

(= (float 0.5) (double 0.5))
=> true
(= #{(float 0.5)} #{(double 0.5)})
=> true
(= {:a (float 0.5)} {:a (double 0.5)})
=> true
(= #{{:a (float 0.5)}} #{{:a (double 0.5)}})
=> false

One way to resolve this would be to tweak the hashing of floats and/or doubles, but that suggestion has apparently been rejected.

An alternative would be to modify = so that it never returns true for float/double comparisons. One should never compare floats with doubles using = anyway, so such a change should have minimal impact beyond restoring hash/equality consistency.






[CLJ-1647] infinite loop in 'partition' and 'partition-all' when 'step' or 'n' is not positive Created: 20/Jan/15  Updated: 27/Jul/15

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

Type: Defect Priority: Minor
Reporter: Kevin Woram Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: checkargs, newbie

Attachments: Text File clj-1647.patch     Text File kworam-clj-1647.patch    
Patch: Code
Approval: Triaged

 Description   

If you pass a non-positive value of 'n' or 'step' to partition, you get an infinite loop. Here are a few examples:

(partition 0 [1 2 3])
(partition 1 -1 [1 2 3])

To fix this, I recommend adding 'assert-args' to the appropriate places in partition and partition-all:

(assert-args
(pos? n) "n must be positive"
(pos? step) "step must be positive" )



 Comments   
Comment by Alex Miller [ 03/Feb/15 5:34 PM ]

Also see CLJ-764

Comment by Alex Miller [ 29/Apr/15 12:02 PM ]

Needs a perf check when done.

Comment by Kevin Woram [ 16/May/15 1:58 PM ]

patch file to fix clj-1647

Comment by Kevin Woram [ 16/May/15 2:19 PM ]

Since 'n' and 'step' remain unchanged from the initial function call through all of the recursive self-calls, I only need to verify that they are positive once, on the initial call.

I therefore created functions 'internal-partition' and 'internal-partition-all' whose implementations are identical to the current versions of 'partition' and 'partition-all'.

I then added preconditions that 'step' and 'n' must be positive to the 'partition' and 'partition-all' functions, and made them call 'internal-partition' and 'internal-partition-all' respectively to do the work.

Comment by Alex Miller [ 17/May/15 8:14 AM ]

There are a lot of unrelated whitespace changes in this patch - can you supply a smaller patch with only the change at issue? Also needs tests.

Comment by Kevin Woram [ 17/May/15 2:05 PM ]

I will supply a patch file without the whitespace changes.

I know there are some existing functionality tests for 'partition' and 'partition-all' in test_clojure\sequences.clj and test_clojure\transducers.clj. I don't think I need to add more functionality tests, but I think I should add:

1. Tests that verify that non-positive 'step' and 'n' parameters are rejected.
2. Tests that show that 'partition' and 'partition-all' performance has not degraded significantly.

Could you give me some guidance on how to develop and add these tests?

Comment by Alex Miller [ 17/May/15 3:31 PM ]

You should add #1 to the patch. For #2, you can just do the timings before/after (criterium is a good tool for this) and put the results in the description.

Comment by Kevin Woram [ 22/May/15 4:04 PM ]

I have coded up the tests for #1 and taken some 'before' timings for #2 using criterium.

I have been stumped by a problem for hours now and I need to get some help. I made my changes to 'partition' and 'partition-all' in core.clj and then did 'mvn package' to build the jar. I executed 'target>java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main' to test out my patched version of clojure interactively. The (source ...) function shows that my source changes for both 'partition' and 'partition-all' are in place. My change to 'partition-all' seems to be working but my change to 'partition' is not. As far as I can tell, they should both throw an AssertionError with the input parameters I am providing.

Any help would be greatly appreciated.

user=> (source partition)
(defn partition
"Returns a lazy sequence of lists of n items each, at offsets step
apart. If step is not supplied, defaults to n, i.e. the partitions
do not overlap. If a pad collection is supplied, use its elements as
necessary to complete last partition upto n items. In case there are
not enough padding elements, return a partition with less than n items."
{:added "1.0"
:static true}
([n coll]
{:pre [(pos? n)]}
(partition n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step coll))
([n step pad coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step pad coll)))
nil
user=> (partition -1 [1 2 3])
()
user=> (source partition-all)
(defn partition-all
"Returns a lazy sequence of lists like partition, but may include
partitions with fewer than n items at the end. Returns a stateful
transducer when no collection is provided."
{:added "1.2"
:static true}
([^long n]
(internal-partition-all n))
([n coll]
(partition-all n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition-all n step coll)))
nil
user=> (partition-all -1 [1 2 3])
AssertionError Assert failed: (pos? n) clojure.core/partition-all (core.clj:6993)

Comment by Alex Miller [ 22/May/15 4:47 PM ]

Did you mvn clean? Or rm target?

Comment by Kevin Woram [ 24/May/15 11:46 PM ]

Yes, I did mvn clean and verified that clojure-1.7.0-master-SNAPSHOT.jar had the expected date-time stamp before doing the interactive test. I even went so far as to retrace my steps on my Macbook on the theory that maybe there was a Windows-specific build problem.

My change to partition-all works as expected but my change to partition does not. However, if I copy the result of the call to (source partition) and execute it (replacing clojure.core/partition with user/partition), user/partition works as expected. I don't understand why my change to clojure.core/partition isn't taking effect.

Comment by Andy Fingerhut [ 25/May/15 1:27 AM ]

Kevin, I do not know the history of your Clojure source tree, but if you ever ran 'ant' in it, that creates jar files in the root directory, whereas 'mvn package' creates them in the target directory. It wasn't clear from your longer comment above whether the 'java -cp ...' command you ran pointed at the one in the target directory. That may not be the cause of the issue you are seeing, but I don't yet have any guesses what else it could be.

Comment by Alex Miller [ 13/Jul/15 1:17 PM ]

What's the status of this?

Comment by Kevin Woram [ 16/Jul/15 10:20 PM ]

Alex, I moved to Seattle and took a permanent position with Microsoft recently. This has kept me very busy and I haven't been able to spend time on Clojure at all. I probably won't be able to devote time to Clojure for another month or two.

Comment by Alex Miller [ 16/Jul/15 10:46 PM ]

Thanks for the heads up.

Comment by Matthew Gilliard [ 23/Jul/15 2:51 PM ]

Kevin, Alex, I could pick this up if you like?

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:41 PM ]

Sorry, browser fail

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Matthew Gilliard [ 27/Jul/15 11:30 AM ]

New patch: clj-1647.patch

Includes tests, fewer whitespace changes, manually thrown IAEs. I'll do some basic benchmarking soon, although I expect the overhead to be quite low as we're only checking the arguments once.

Kevin, the reason your patch was working for partition-all but not partition is that partition is defined early-ish in the bootstrapping process and the {:pre .. :post ..} maps aren't read by defn until it's enhanced later on.

Comment by Kevin Woram [ 27/Jul/15 12:10 PM ]

Thanks for solving that mystery Matthew!





[CLJ-1628] Accept list as lib specification in clojure.core/require and clojure.core/use Created: 28/Dec/14  Updated: 30/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Petr Gladkikh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File NS-macro-accept-lists-as-libspecs.patch    
Patch: Code

 Description   

Currently function clojure.core/load-libs treats '(a.namespace.name) as prefix list so this construct has no effect at all (as if it was prefix list without suffixes). At the same time '[a.namespace.name] causes require call (require 'a.namespace.name). In other cases functions require or use are ambivalent about differences between [] and (). In this particular case there is difference between no-op and lib loading. E.g. Clojure validation tool Eastwood includes rule for this case since the behavior of (require '(a.namespace.name)) is not obvious.

The suggested change lets to avoid this special case in require or use calls (including ones that stem from ns macro expansion). Accepting both list and vector as library specification makes behavior uniform and similar to the way suffix items are handled in prefix lists.

The patch is minimal in order to avoid reordering sequential? functions in clojure/core.clj
Should I include tests for these cases also?



 Comments   
Comment by Petr Gladkikh [ 30/Dec/14 2:39 AM ]

If on the other hand representing prefix lists as Clojure lists is intentional and list-for-prefix, vector-for-libspec-or-suffix should be distinguishing feature then we should issue error when

  • prefix list is enclosed in Clojure vector
  • libspec or suffix is in Clojure list

If backwards compatibility is important then one may at least write a warning in ':verbose' mode.

Also there should be error or warning if prefix list is empty.





[CLJ-1624] Support get from arbitrary java.util.List data types Created: 23/Dec/14  Updated: 23/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop

Attachments: File clj-1624.diff    
Patch: Code
Approval: Triaged

 Description   

Currently "get", "get-in" and related functions in clojure.core work on Clojure vectors, maps and Java arrays, but do not work on instances of java.util.List

(def al (java.util.Arrays/asList (object-array [1 2 3 4])))
(get al 2)
=> nil

This makes it inconvenient to work with nested structures of Java objects that could otherwise be viewed as similar to nested Clojure data structures.

This is also inconsistent with other clojure.core functions that do support arbitrary java.util.List instances (e.g. "nth" and "count")

With a small change to RT.java, it is possible to allow core functions to operate on arbitrary instances of java.util.List. There does not appear to be any significant downside to this change (it is not on the fast path so will not affect regular ILookup or Map checks).



 Comments   
Comment by Mike Anderson [ 23/Dec/14 12:31 AM ]

Patch for CLJ-1624





[CLJ-1623] Support zero-depth structures for update and update-in Created: 22/Dec/14  Updated: 24/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently "update" and "update-in" assume a nested associative structure at least 1 level deep.

For greater generality, it would be preferable to also support the case of 0 levels deep (i.e. a nested associative structure where there is only a leaf node)

examples:

;; Zero-length paths would be supported in update-in
(update-in 1 [] inc) => 2

;; update would get an extra arity which simply substitutes the new value
(update :old :new) => :new



 Comments   
Comment by Ghadi Shayban [ 23/Dec/14 7:56 AM ]

Duplicate of CLJ-373 which has been declined?

Comment by Alex Miller [ 23/Dec/14 8:19 AM ]

Rich has declined similar requests in the past.

Comment by Mike Anderson [ 23/Dec/14 7:50 PM ]

I disagree with the reasons for rejecting the previous patch. Can we revisit this?

Yes, it is a (very minor) behaviour change for update-in, but only only on undefined implementation behaviour, and even then only on the error case. If people are relying on this then their code is already very broken.

On the plus side, is makes the behaviour more logical and consistent. There is clearly demand for the change (see the various comments in favour of improving this in CLJ-373)

As an aside: if you really want to keep the old behaviour of disallowing empty paths then it would be better to convert the NullPointerException into a meaningful error message e.g. "Empty key paths are not allowed"

Also, I am proposing a corresponding change to update which doesn't have the above concern (since it is introducing a whole new arity)

Comment by Alex Miller [ 24/Dec/14 7:55 AM ]

Sorry, Rich has said he's not interested.





[CLJ-1615] transient set "keys" and "values" wind up with different metadata Created: 12/Dec/14  Updated: 13/Dec/14

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, meta, transient

Attachments: Text File 0001-CLJ-1615-ensure-transient-set-keys-and-values-have-c.patch     Text File 0001-demonstrate-CLJ-1615.patch     Text File CLJ-1615-entryAt.patch    
Patch: Code and Test

 Description   
(let [s (-> #{} 
          transient 
          (conj! (clojure.core/with-meta [-7] {:mynum 0}))
          (conj! (clojure.core/with-meta [-7] {:mynum -1})) 
          persistent!)]
  [(meta (s [-7])) (meta (first s))])
=> [{:mynum -1} {:mynum 0}]

basically it looks like the "key" (the value we get by seqing on the set) retains the metadata from the first conj! but the "value" (what we get by calling invoke with the "key") carries the metadata from the second conj!. This does not match the behavior if we don't use transients:

(let [s (-> #{} 
          (conj (clojure.core/with-meta [-7] {:mynum 0}))
          (conj (clojure.core/with-meta [-7] {:mynum -1})))]
  [(meta (s [-7])) (meta (first s))])
=> [{:mynum 0} {:mynum 0}]

(found playing with zach tellman's collection-check)



 Comments   
Comment by Michael Blume [ 12/Dec/14 5:07 PM ]

Attached patch demonstrating problem (not a fix)

Comment by Michael Blume [ 12/Dec/14 5:40 PM ]

More investigation:

The difference between "keys" and "vals" arises from the fact that clojure sets use maps under the covers.

The difference between persistent and transient seems to be because PersistentHashSet.cons short-circuits on contains (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/PersistentHashSet.java#L97) and ATransientSet.conj does not (https://github.com/clojure/clojure/blob/clojure-1.6.0/src/jvm/clojure/lang/ATransientSet.java#L27)

Adding a contains check to ATransientSet.conj makes the behavior consistent and passes the attached test, but I imagine this could cause a performance hit. Thoughts?

Comment by Michael Blume [ 12/Dec/14 5:43 PM ]

Attached proposed fix – note that this may cause a performance hit for transient sets.

Comment by Michael Blume [ 13/Dec/14 2:40 PM ]

Attaching an alternative fix – instead of doing a contains check on every transient conj, back set.get with entryAt. More invasive but possibly faster.





[CLJ-1613] :or defaults should refer to enclosing scope in map destructuring Created: 12/Dec/14  Updated: 30/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-1613-evaluate-or-defaults-in-enclosing-scope-in-.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Michael Blume noticed that :or defaults can depend on the values of other keys, see https://groups.google.com/d/msg/clojure/6kOhpPOpHWM/ITjWwQFS_VQJ

Michael's Gist https://gist.github.com/MichaelBlume/4891dafdd31f0dcbc727 displays a case where an associative form involving :keys and :or compiles or not depending on the order of symbols in :keys. By tweaking that case one can arrive at expressions which always compile, but produce different values depending on :keys:

(let [foo 1
       bar 2
       {:keys [bar foo]
        :or {foo 3 bar (inc foo)}} {}]
  {:foo foo :bar bar})
;= {:foo 3, :bar 4}

(let [foo 1
      bar 2
      {:keys [foo bar]
       :or {foo 3 bar (inc foo)}} {}]
  {:foo foo :bar bar})
;= {:foo 3, :bar 2}

I believe that the most natural solution is to demand that :or defaults be evaluated in an enclosing scope where none of the destructuring-introduced locals are present. This approach is taken by the 0001 patch.



 Comments   
Comment by Michael Blume [ 12/Dec/14 2:27 AM ]

I suspect that this is the right thing to do but I think it's important to note that this will break existing code https://github.com/ngrunwald/ring-middleware-format/blob/master/src/ring/middleware/format_params.clj#L214

Comment by Michael Blume [ 06/Apr/15 4:43 PM ]

Update on my previous comment – ring-middleware-params has updated so that it doesn't depend on this behavior. I think we should definitely merge this patch so no one else depends on it.

Comment by Max Penet [ 08/Apr/15 10:46 AM ]

Since this involves :or keys evaluation, this might be worth checking if this should/could have an impact on http://dev.clojure.org/jira/browse/CLJ-1676 as well.

Comment by Stuart Halloway [ 30/Jul/15 11:11 AM ]

This is a behavior change, the docs do not promise the requested behavior and existing code may depend on the current behavior.

Comment by Andy Fingerhut [ 30/Jul/15 12:47 PM ]

Isn't this a case where if existing code works, it works by accident of the seq order of an unordered map? If so, any code that depends upon the existing behavior sometimes breaks, sometimes does not break, when the Clojure seq order on maps changes, which occurred Clojure 1.5.1 to Clojure 1.6.0, and again from 1.6.0 to 1.7.0.

Comment by Michael Blume [ 30/Jul/15 1:08 PM ]

Yes, it does, and I've seen existing code break due to those changes, hence the discussion that lead to this ticket.





[CLJ-1607] docstring for clojure.core/counted? should be more specific Created: 29/Nov/14  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: docstring

Attachments: Text File CLJ-1607-p1.patch    
Patch: Code
Approval: Triaged

 Description   

The docstring for counted? currently says:

Returns true if coll implements count in constant time

This tempts the user into thinking they can use this function to determine whether or not calling count on any collection is a constant-time operation, when in fact it only reflects whether or not an object implements the clojure.lang.Counted interface. Since count special-cases a handful of platform types, there are common cases such as Arrays and Strings that are constant time but will return false from counted?.

Proposed:

Returns true if coll, a Clojure collection, implements count in constant time. Note that this function will return false for host types even if the count function can return their size in constant time (as with arrays and strings).



 Comments   
Comment by Gary Fredericks [ 29/Nov/14 9:01 AM ]

Attached CLJ-1607-p1.patch with my first draft of a better docstring.

Comment by Gary Fredericks [ 29/Nov/14 9:08 AM ]

What would be the most accurate language to describe the exceptions? I used "some collections" in the first patch but perhaps "native collections" or "host collections" would be more helpful?

Comment by Alex Miller [ 29/Nov/14 9:44 AM ]

While I understand where you're coming from, I think the intent of "counted?" is not to answer the question "is this thing countable in constant time" for all possible types, but specifically for collections that participate in the Clojure collection library. This includes both internal collections like PHM, PHS, PV, etc but also external collections that mark their capabilities using those interfaces.

I believe count handles more cases than just collections that are counted in constant time (like seqs) so is not intended to be symmetric with counted?.

Comment by Gary Fredericks [ 29/Nov/14 9:55 AM ]

Sure, I wasn't suggesting changing what the function does – just changing the docstring to make it less likely to be misleading.

Comment by Gary Fredericks [ 29/Nov/14 10:00 AM ]

What about this sort of wording?

Returns true if coll, a Clojure collection, implements count in constant time.
Note that this function will return false for host types even if the count 
function can return their size in constant time (as with arrays and strings).
Comment by Alex Miller [ 30/Nov/14 9:52 PM ]

I think it's unlikely to pass vetting, but that's just my guess.

Comment by Gary Fredericks [ 01/Dec/14 8:53 AM ]

I'm trying to figure out where the disagreement is here; are you arguing any of these points, or something different?

  1. The docstring is not likely to confuse people by making them think it gives meaningful responses for host collections
  2. It's not a problem for us to solve if the docstring confuses people
  3. It is a problem we should solve, but the changes I've suggested are a bad solution
Comment by Alex Miller [ 01/Dec/14 9:18 AM ]

In general, the docstrings prefer concision and essence over exhaustive cases or examples. My suspicion is that the docstring says what Rich wants it to say and he would consider the points you've added to be implicit in the current docstring, and thus unnecessary. Specifically, "coll" is used pretty consistently to mean a Clojure collection (or sequence) across all of the docstrings. And there is an implicit else in the docstring that counted? will return false for things that are not Clojure collections. The words that are there (and not there) are carefully chosen.

I agree with you that more words may be necessary to describe fully what to expect from this or any other function in core. My experience from seeing Rich's response on things like this is that he may agree with that too, but he would prefer it to live somewhere outside the doc string in reference material or other sources. Not to say that we don't update docstrings, as that does happen pretty regularly; I just don't think this one will be accepted. I've asked Stu to give me a second set of eyes too.

Comment by Gary Fredericks [ 01/Dec/14 9:36 AM ]

That was helpful detail, thanks!

Comment by Reid McKenzie [ 01/Dec/14 12:42 PM ]

I think this one is fine as-is, because the docstring for count explicitly notes "Also works on ..." which are implied not to be counted?.





[CLJ-1598] Make if forms compile directly to the appropriate branch expression if the test is a literal Created: 24/Nov/14  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, performance, primitives

Attachments: Text File 0001-if-test-expr-of-an-if-statement-is-a-literal-don-t-e.patch    
Patch: Code
Approval: Triaged

 Description   

This allows expressions like `(cond (some-expr) 1 :else 2)` to be eligible for unboxed use, which is not currently possible since the cond macro always ends up with a nil else branch that the compiler currently takes into account.

With the attached patch, the trailing (if :else 2 nil) in the macroexpansion will be treated as 2 by the Compiler, thus allowing the unboxed usage of the cond expression.






[CLJ-1597] Allow ISeq args to map conj Created: 22/Nov/14  Updated: 22/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, maps

Attachments: Text File 0001-allow-ISeq-args-to-map-conj.patch    
Patch: Code and Test

 Description   

Currently conj on maps can take only maps or vectors, this patch allows it to take any ISeq:

user=> (conj {} '(1 2))
{1 2}





[CLJ-1595] Nested doseqs leak with sequence of huge lazy-seqs Created: 20/Nov/14  Updated: 25/May/15

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

Type: Defect Priority: Minor
Reporter: Andrew Rudenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File doseq_leaks.diff    
Patch: Code

 Description   

Hello!

This little snippet demonstrates the problem:

(doseq [outer-seq (list (range)) inner-seq outer-seq])

That's it. It is not just eats my processor, but also eats all available memory. Practically it can affect (and it is) at consuming of complex lazy structures like huge XML-documents.

I think this is at least non trivial behaviour.

It can be fixed by this small patch. We can get next element before current iteration, not after, so outer loop will not hold reference to the head of inner-seq.

This patch doesn't solve all problems, for example this code:

(doseq [outer-seq [(range)] inner-seq outer-seq])

leaks. Because chunked-seqs (vector in this case) holds current element by design.



 Comments   
Comment by Andy Fingerhut [ 25/May/15 3:15 PM ]

Andrew, sorry but I do not know whether this ticket is of interest to the Clojure core team.

I do know that patches are only considered for inclusion in Clojure if the submitter has signed the contributor agreement (CA). If you were interested in doing that, you can do it fairly quickly on-line here: http://clojure.org/contributing





[CLJ-1592] Ability to suppress warnings on name conflict with clojure.core Created: 14/Nov/14  Updated: 14/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In numerical code, it is often useful and idiomatic to replace clojure.core functions with augmented versions (e.g. clojure.core.matrix.operators defines + in a way that works with whole arrays, not just scalar numbers)

Currently there seems to be no way to avoid a warning in client code when a library does this, e.g.:

;; library namespace
(ns foo
  (:refer-clojure :exclude [+]))
(def + clojure.core/+)

;; later on, in some other namespace
(require '[foo :refer :all])
=> WARNING: + already refers to: #'clojure.core/+ in namespace: bar, being replaced by: #'foo/+

A workaround exists by using (:refer-clojure :exclude ...) in the user namespace, however this creates unnecessary work for the user and requires maintenance of boilerplate code.

Proposed solution is to allow vars to be annotated with additional metadata (e.g. ^:replace-var ) that when added to library functions will suppress this warning. This will allow library authors to specify that a function should work as a drop-in replacement for clojure.core (or some other namespace), and that a warning is therefore not required.



 Comments   
Comment by Andy Fingerhut [ 14/Nov/14 9:46 PM ]

Duplicate with CLJ-1257 ?

Comment by Mike Anderson [ 14/Nov/14 9:53 PM ]

Hi Andy, it refers to the same warning - but the scope of the solution is different:

  • CLJ-1257 is more like a global way to turn off this warning
  • CLJ-1592 is for suppressing this warning on specific vars

If CLJ-1257 is implemented and the warning is off be default, CLJ-1592 becomes mostly unnecessary. Without CLJ-1257 or if the warning defaults to on, CLJ-1592 is needed.





[CLJ-1591] Symbol not being bound in namespace when name clashes with clojure.core Created: 14/Nov/14  Updated: 28/Dec/14

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

Type: Defect Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Approval: Triaged

 Description   

The following code fails (both in 1.6 and latest 1.7-alpha4):

user=> (ns foo)
nil
foo=>  (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc

;; Note inc is unbound at this point, which causes the exception below
foo=> inc
#<Unbound Unbound: #'foo/inc>
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
bar=> (inc 8)

IllegalStateException Attempting to call unbound fn: #'foo/inc  clojure.lang.Var$Unbound.throwArity (Var.java:43)

Further investigation shows that foo/inc is unbound:

foo/inc
=> #<Unbound Unbound: #'foo/inc>

Further investigation also shows that replacing the (def inc inc) with almost anything else, e.g. (def inc dec), (def inc clojure.core/inc), or (def inc (fn [n] (+ n 1))), causes no exception (but the warnings remain).

I would expect:
a) foo/inc should be bound and have the same value as clojure.core/inc
b) No error when requiring foo/inc
c) bar/inc should be bound to foo/inc



 Comments   
Comment by Nicola Mometto [ 14/Nov/14 10:04 PM ]

The second error should be expected, the right syntax should be (require ['foo :refer ['inc]]) (note the leading quote before inc)

Comment by Mike Anderson [ 14/Nov/14 10:20 PM ]

Thanks for the catch Nicola - I've edited the description. Still get the same error however (just with a slightly different message)

Comment by Alex Miller [ 14/Nov/14 10:22 PM ]

See comment...

Comment by Mike Anderson [ 14/Nov/14 10:24 PM ]

@Alex what comment? Note that the error still occurs even with the right syntax....

Comment by Mike Anderson [ 14/Nov/14 10:26 PM ]

Appears to have been closed prematurely

Comment by Alex Miller [ 14/Nov/14 10:39 PM ]

I can't reproduce with the correct syntax:

Clojure 1.7.0-master-SNAPSHOT
user=> (ns foo)
nil
foo=> (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
Comment by Mike Anderson [ 14/Nov/14 10:55 PM ]

The problem is that the var is still unbound and causes e.g. the following error:

=> (foo/inc 8)
IllegalStateException Attempting to call unbound fn: #'foo/inc clojure.lang.Var$Unbound.throwArity (Var.java:43)

I don't think that should be expected - or am I missing something?

Comment by Alex Miller [ 14/Nov/14 10:57 PM ]

Ah, will take a look. But not right now.

Comment by Andy Fingerhut [ 15/Nov/14 1:09 PM ]

Updated the description with a few more details. The exception goes away if you do (def inc (fn [n] (+ n 1))) instead of (def inc inc), for example. The warnings remain.

Comment by Tom Crayford [ 20/Nov/14 11:07 AM ]

Unsure if this is the same issue (I think it might be?), but I reproduced the exact same error message with AOT compilation involved:

reproduced in this git repository: https://github.com/yeller/compiler_update_not_referenced_bug

clone it, run `lein do clean, uberjar, test`, and that error message will show up every time for me

Comment by Andy Fingerhut [ 20/Nov/14 5:43 PM ]

Mike, I think replacing (def inc inc) in your example with (def inc clojure.core/inc) should be considered as a reasonable workaround for this issue, unless you have some use case where you need to def inc to something that is not in clojure.core (and if so, why?)

The reason (def inc inc) behaves this way is, if not absolutely necessary, at least commonly used in Clojure programs to define recursive functions, e.g. (defn fib [n] (if (<= n 1) 1 (+ (fib (dec n)) (fib (- n 2))))), so that the occurrences of fib in the body are resolved to the fib being defined.

Comment by Alex Miller [ 22/Nov/14 9:05 AM ]

Moving to 1.7 until I can look at this more deeply.

Comment by Mike Anderson [ 23/Nov/14 6:08 PM ]

Andy - yes the workaround is fine for me right now.

I don't think this is an urgent issue but it may be exposing a subtle complexity regarding assumptions about the state of the namespace at different times. Perhaps the semantics should be something like:

  • The def statement itself should be run before the var is interned. e.g. (def inc (inc 5)) should result in (def inc 6)
  • Anything complied / deferred to run after completion of the def statement should use the new var (i.e. the new var should be referenced by fns, lazy sequences etc.)
Comment by Andy Fingerhut [ 23/Nov/14 6:36 PM ]

I'm not sure what your proposal means in a case like this:

(def inc (fn [x] (inc x)))

Is the second inc to be interpreted/resolved before or after the new inc is created? Because it is (fn ...) it should be the after-behavior? What else besides fn should cause the after-behavior, rather than the before-behavior?

Even more fun (not saying that people often write code like this, but the compiler can handle it today):

(def inc (if (> (inc y) 5)
           (fn [x] (inc x))
           (fn [x] (dec x))))

I think the current compiler behavior of 'in the body of a def, the def'd symbol always refers to the new var, not any earlier def'd vars' is fairly straightforward to explain.

Comment by Tom Crayford [ 23/Nov/14 9:15 PM ]

Should I file the AOT issue reproduced in that thing as a new issue?

Comment by Andy Fingerhut [ 24/Nov/14 5:16 PM ]

Tom: Alex Miller or another screener would be best to say whether the AOT issue should be a separate ticket, but my best guess would be "go for it". I tried to look at the link you gave but it seems not to point to anything. Could you double-check that link?

Comment by Tom Crayford [ 24/Nov/14 6:48 PM ]

Andy,

Great. I'll write one up tomorrow sometime. I accidentally left that repo as private, should be visible now.

Comment by Andy Fingerhut [ 24/Nov/14 8:11 PM ]

This comment is really most relevant for ticket CLJ-1604, where it has been copied:

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 25/Nov/14 3:44 PM ]

Tom, I've opened a ticket with a patch fixing the AOT issue: http://dev.clojure.org/jira/browse/CLJ-1604





[CLJ-1587] PersistentArrayMap's assoc doesn't respect HASHTABLE_THRESHOLD Created: 12/Nov/14  Updated: 12/Nov/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, data-structures, maps

Attachments: Text File 0001-PersistentArrayMap-s-assoc-doesn-t-respect-HASHTABLE.patch    
Patch: Code

 Description   

Currently a map with more than 8 elements will be converted from a PersistentArrayMap to a PersistentHashMap, but if using assoc, it will take 9 elements before the conversion happens:

user=>  (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentArrayMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap

After patch:

user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap





[CLJ-1585] Report boxed math warning on function that boxes primitive return value Created: 11/Nov/14  Updated: 28/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, math

Attachments: Text File clj-1585.patch    
Patch: Code
Approval: Triaged

 Description   

With the new :warn-on-boxed (CLJ-1325), these examples do not report a boxed math warning although they each do boxing:

user=> (defn f1 [^long x] (inc x))
f1
user=> (defn f2 [x] (aget (long-array [1 2]) 0))
f2
user=> (defn f3 [x] (aget (int-array [1 2]) 0))
f3
user=> (defn f4 [^String s] (.indexOf s "a"))

Cause: emitBoxReturn has a hard-coded call to box a prim return value.

Solution: If *unchecked-math* is set to :warn-on-boxed, emit a warning on boxing of primitive numeric return types.

Patch:



 Comments   
Comment by Alex Miller [ 12/Nov/14 12:39 AM ]

Attached patch does the job, but from trying it out on some real code, it finds both problematic cases and lots of cases that could safely be ignored and/or where there is no obvious way to fix the warning. I think it may need some more tuning to reduce the rate of unfixable things a bit.





[CLJ-1579] source-fn can fail due to reading namespace-aliased keywords while in another namespace context Created: 05/Nov/14  Updated: 21/Nov/14

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

Type: Defect Priority: Minor
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Read-src-in-appropriate-ns-context.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.repl/source-fn functions by using a custom reader to read a source form at the location specified by line & file metadata on a given symbol. While this works well for most things, I encountered an issue when applying source-fn to code containing keyword namespace aliases ala ::T/foo. ::T/foo is a legitimate namespace keyword in the context where it occurs, because a namespace alias to T is created in the ns header. When the keyword ::T/foo is read then, it resolves to :my-other.ns/foo as one would expect because ns has the appropriate alias. However when attempting to read source via clojure.repl/source-fn, ns may no longer be the original read context of the indicated form thus leading to the erroneous exception java.lang.RuntimeException: Invalid token: ::T/foo.

The solution is that the reading operation of clojure.repl/source-fn must be wrapped in (binding [*ns* (.ns v)] ...) so that source reading will take place in the original load reading context thus preventing this error.

A patched equivalent function exists here, https://github.com/clojure-grimoire/lein-grim/blob/master/src/grimoire/doc.clj#L50-L74, and I will submit a patch against 1.6.0 in the morning.






[CLJ-1577] Some hints accept both symbols and class objects, others only symbols Created: 30/Oct/14  Updated: 30/Oct/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: typehints


 Description   

In order to hint primitives, such as longs, you can hint with the symbol 'long. In some places, you can also use the class object java.lang.Long/TYPE. However, in some places, that doesn't work. This is particularly problematic when working with hints in macros, where subtle changes to when metadata is evaluated can lead to changes in whether or not hints are respected.

user=> (set! *unchecked-math* :warn-on-boxed)
:warn-on-boxed

user=> (defmacro mac []
         (let [field (with-meta 'x {:tag 'long})]
           (-> field meta :tag class prn)
           `(deftype Foo# [~field]
              clojure.lang.IDeref
              (deref [this#]
                (inc ~(with-meta field nil))))))
#'user/mac

user=> (mac)
clojure.lang.Symbol
#<java.lang.Class@1c76c583 class user.Foo__13651__auto__>

user=> (defmacro mac []
         (let [field (with-meta 'x {:tag java.lang.Long/TYPE})]
           (-> field meta :tag class prn)
           `(deftype Foo# [~field]
              clojure.lang.IDeref
              (deref [this#]
                (inc ~(with-meta field nil))))))
#'user/mac

user=> (mac)
java.lang.Class
Boxed math warning, /private/var/folders/43/mnwlkd2s7r1gbjwq6t__mgt40000gn/T/form-init5463347341158437534.clj:1:1 - call: public static java.lang.Number clojure.lang.Numbers.unchecked_inc(java.lang.Object).
#<java.lang.Class@74626b21 class user.Foo__13663__auto__>





[CLJ-1575] Using a (def ^:const instance) of a deftype that implements IPersistentCollection, triggers compiler errors Created: 29/Oct/14  Updated: 30/Oct/14

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

fresh repl


Attachments: Text File 0001-Test-for-analyzer-bug-CLJ-1575.patch    

 Description   

The compiler has a lot of assumptions about the possible types of IPersistentCollection literals and rightfully so. The strange thing with this case is, that taking the (constant) value works as soon as count is defined, but using it as an argument hits a closed dispatch for emitting the empty variants of the various literals.

> (deftype T [] clojure.lang.IPersistentCollection (count [_] 0)
> (def ^:const t (T.))
> (meta t)
java.lang.UnsupportedOperationException: Unknown Collection type
Compiler.java:2860 clojure.lang.Compiler$EmptyExpr.emit
Compiler.java:3632 clojure.lang.Compiler$InvokeExpr.emitArgsAndCall
...

EDIT updated the ticket after some investigation
NOTE attached test patch doesn't even implement (count []) for the deftype, which just triggers a rightful AbstractMethodError



 Comments   
Comment by Herwig Hochleitner [ 29/Oct/14 10:00 PM ]

The test had a typo, sorry

Comment by Alex Miller [ 30/Oct/14 7:14 AM ]

Looks like a variant of CLJ-1093.





[CLJ-1573] Support (Java) transient fields in deftype, e.g. for hashcodes Created: 26/Oct/14  Updated: 29/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: compiler, deftype

Attachments: Text File 0001-transient-field-deftype.patch    
Patch: Code and Test

 Description   

Enhance deftypes to allow fields to be marked ACC_TRANSIENT.

strawman syntax:
(deftype AType [^:transient hash])

Came across this need while experimenting with a reified range written in a deftype, not in Java.

Patch doesn't include docstring change, but has a test.



 Comments   
Comment by Adrian Medina [ 29/Dec/14 11:54 AM ]

Perhaps ^:transient-mutable would be a more appropriate modifier name to be consistent with the ^:unsynchronized-mutable and ^:volatile-mutable field modifiers. In any event, this feature would eliminate the need to drop down to Java for types that require transient fields.

Comment by Andy Fingerhut [ 29/Dec/14 12:07 PM ]

Roberto, there is a "Vote" word you can click on to actually vote for tickets, and ticket wranglers actually look at those votes at times to examine popular ones sooner. +1 comments don't do that.





[CLJ-1566] Documentation for clojure.core/require does not document :rename Created: 16/Oct/14  Updated: 02/Jul/15

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

Type: Defect Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File refer.patch    
Patch: Code

 Description   

By contrast, clojure.core/use does mention :rename.

I attach a patch



 Comments   
Comment by Andy Fingerhut [ 16/Oct/14 1:33 PM ]

James, your patch removes any mention of the :all keyword, and that keyword is not mentioned in the doc string for clojure.core/refer.

I haven't checked whether refer can take :all as an argument, but clojure.core/require definitely can.

Comment by James Laver [ 16/Oct/14 1:39 PM ]

Ah, you're quite right. Fixed now. See updated patch in a sec.

Comment by Andy Fingerhut [ 16/Oct/14 8:16 PM ]

For sake of reduced confusion, it would be better if you could either name your patches differently, or delete obsolete ones with identical names as later ones. JIRA allows multiple patches to have the same names, without replacing the earlier ones.

Comment by James Laver [ 17/Oct/14 12:44 AM ]

Okay, that's done. The JIRA interface is a bit tedious in places.

Comment by Bozhidar Batsov [ 19/Oct/14 1:34 AM ]

Seems to me the sentence should end with a dot.

Comment by James Laver [ 19/Oct/14 4:36 AM ]

Added a dot.

Comment by Andy Fingerhut [ 02/Jul/15 3:56 PM ]

James, I do not know whether this change is of interest to the Clojure core team, but see this link for instructions on creating patches in the expected format (yours is not in that format): http://dev.clojure.org/display/community/Developing+Patches





[CLJ-1563] How About Default Implementations on Protocols Created: 11/Oct/14  Updated: 12/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: David Williams Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Consider this example

user=> (defprotocol Foo (foo [x] x))
Foo
user=> (defrecord Bar [gaz waka] Foo)
user.Bar
user=> (def bar (Bar. 1 2))
#'user/bar
user=> (.foo bar)

AbstractMethodError user.Bar.foo()Ljava/lang/Object;  sun.reflect.NativeMethodAccessorImpl.invoke0 (NativeMethodAccessorImpl.java:-2)
user=>

What about the default implementation.



 Comments   
Comment by David Williams [ 11/Oct/14 8:48 PM ]

As it stands you have to workaround with this

http://stackoverflow.com/questions/15039431/clojure-mix-protocol-default-implementation-with-custom-implementation

Comment by Jozef Wagner [ 12/Oct/14 1:01 AM ]

I don't think we need it. What's the rationale behind extending some protocol, not implementing its methods, and then calling those methods, expecting them not to throw. Be explicit about what yout type should do, whether it is a default or custom behavior. You basically have three options

(defn default-foo 
  [this] 
  :foo)

(defprotocol P
  (-foo [this]))

(deftype T
  P
  (-foo [this] (default-foo))

(defn foo 
  [x]
  (-foo x))

or

(defprotocol P
  (-foo [this]))

(deftype T)

(defn foo 
  [x]
  (if (satisfies? P x)
    (-foo x)
    :foo))

or

(defprotocol P
  (-foo [this]))

(extend-protocol P
  java.lang.Object
  (-foo [this] :foo))

(deftype T)

(defn foo 
  [x]
  (-foo x))

I think however that my first approach is unidiomatic and you should prefer the latter ones.

Comment by David Williams [ 12/Oct/14 12:36 PM ]

I agree, this is a low priority enhancement. I think it could make the Protocol experience more DWIMy, and Java 8 has default implementations on interfaces for the same kind of convenience.





[CLJ-1560] Forbid closing over mutable fields completely Created: 10/Oct/14  Updated: 19/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Closing over mutable fields should be forbidden completely (and generate compiler exception), not just when trying to set! them. As the change of the mutable field does not propagate into closed over ones, this leads to surprising bugs:

(defprotocol P 
  (-set [this]) 
  (-get [this]) 
  (-get-fn [this]))

(deftype T [^:unsynchronized-mutable val] 
  P 
  (-set [this] (set! val :bar)) 
  (-get [this] val) 
  (-get-fn [this] (fn [] val)))

(def x (->T :foo))

(def xf (-get-fn x))

user> (-set x)
:bar
user> (-get x)
:bar
user> (xf)
:foo ;; should be :bar !!!


 Comments   
Comment by Jozef Wagner [ 10/Oct/14 1:42 PM ]

related issue CLJ-274

Comment by Ghadi Shayban [ 28/Dec/14 10:42 PM ]

In the given example, the close-over happens before the set!, so the closure gets the value, not an assignable container. This is consistent with the rest of the language (pass-by-value not by mutable container)

Comment by Jozef Wagner [ 29/Dec/14 2:21 AM ]

Thanks for explanation. The ticket is about a proposal that closing over a mutable field should result in error being thrown, an not in a value. If value is desired, an explicit let binding will have to be used. So far, I haven't found a valid use case where closing over mutable field and getting the value closed over is the intended and wanted behavior.





[CLJ-1556] Add instance check functions to defrecord/deftype Created: 09/Oct/14  Updated: 09/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord, deftype

Attachments: Text File 0001-CLJ-1556-Generate-type-functions-with-instance-check.patch    
Patch: Code

 Description   

It is often necessarty to test for instance? on deftypes/defrecords, this patch makes the two macros automatically generate a type? function implemented as (fn [x] (instance? type x)), to complement ->type and map->type
Example:

user=>(deftype x [])
user.x
user=>(x? (x.))
true


 Comments   
Comment by Jozef Wagner [ 09/Oct/14 9:11 AM ]

What about camel cased types? predicate SomeType? does not look like an idiomatic type predicate. I suggest to have this type predicate function and its name optional, through e.g. :predicate metadata on a type name. Moreover, it is far more useful to have such predicate on protocols, rather than types.

Comment by Nicola Mometto [ 09/Oct/14 9:17 AM ]

I don't think camel cased types should pose any issue. we use ->SomeType just as fine, I don't see why SomeType? should be problematic.

I disagree that it's more useful to have a predicate for protocols since protocols are already regular Vars and it's just a matter of (satisfies? theprotocol x), the value of the predicate on types/record is to minimize the necessity of having to import the actual class





[CLJ-1550] Classes generated by deftype and defrecord don't play nice with .getPackage Created: 07/Oct/14  Updated: 22/Jul/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6, Release 1.7, Release 1.8
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 15
Labels: classloader, deftype

Attachments: Text File 0001-CLJ-1550-define-package-for-class-in-DynamicClassLoa.patch     Text File CLJ-1550-v2.patch    
Patch: Code and Test

 Description   

Classes generated loaded by DynamicClassLoader return nil for .getPackage

(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

This seems like a bug to me as it's not obvious why the class generated by deftype should exhibit different behaviour.

Patch: CLJ-1550-v2.patch



 Comments   
Comment by Alex Miller [ 07/Oct/14 8:54 AM ]

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Comment by Bozhidar Batsov [ 07/Oct/14 9:05 AM ]

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Comment by Kevin Downey [ 07/Oct/14 11:46 AM ]

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Comment by Kevin Downey [ 07/Oct/14 11:49 AM ]

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Comment by Alex Miller [ 07/Oct/14 12:13 PM ]

Yep, I believe that's correct.

Comment by Stuart Halloway [ 21/Jul/15 8:01 AM ]

There is no problem statement here. What is package information needed for?

Comment by Bozhidar Batsov [ 21/Jul/15 8:05 AM ]

I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.
This might not be problem when running your apps, but it's definitely a problem when inspecting their state...

Comment by Michael Blume [ 22/Jul/15 12:32 PM ]

s/Packate/Package





[CLJ-1548] primitive type hints on protocol methods break call sites Created: 04/Oct/14  Updated: 04/Oct/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
user=> (defprotocol P (f [this ^long x]))
P
user=> (deftype T [] P (f [_ x] x))
#<java.lang.Class class user.T>
user=> (f (T.) 5)

ClassCastException user$eval7289$fn__7290$G__7280__7297 cannot be cast to clojure.lang.IFn$OLO  user/eval7313 (NO_SOURCE_FILE:1)





[CLJ-1543] Type tags on argument vector appear to help avoid reflection when used with defn, but not with def foo (fn ...) Created: 30/Sep/14  Updated: 02/Oct/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, typehints


 Description   

I would have expected that both of the Java interop calls below would avoid reflection, but only the first involving f1 does.

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3

Not sure if this has anything to do with CLJ-1232, but was discovered when testing variants of that issue.



 Comments   
Comment by Andy Fingerhut [ 30/Sep/14 9:08 PM ]

What a nice number for a ticket, 1543. The year Copernicus's most celebrated book was published: http://en.wikipedia.org/wiki/Nicolaus_Copernicus

Comment by Jozef Wagner [ 01/Oct/14 4:05 AM ]

Isn't type hinting of arg vector meant only for primitive type hints? AFAIK non-primitive type hints should be on a function name, everything else is non idiomatic.

Comment by Nicola Mometto [ 01/Oct/14 7:05 AM ]

This isn't an issue of arg vector hinting vs function name hinting.
The issue here is that return type hinting cannot be put on anonymous functions but only on defns as the :arglists will be added by defn on the Var's metadata.

This is one of the reasons why I'd like to have that information as a field on the fn rather than as metadata on the Var

Comment by Andy Fingerhut [ 01/Oct/14 10:55 AM ]

Jozef, you may be correct that non-primitive type hints on the argument vector are non idiomatic. Do you have any source for that I could read?

Comment by Tassilo Horn [ 02/Oct/14 12:19 AM ]

Only the version with hints on the argument vectors is documented at http://clojure.org/java_interop#Java Interop-Type Hints. However, in the case you have just one arity (or all arities return a value of the same type) the hint on the var name also works. But the two versions seem to have different semantics. Have a look at CLJ-1232.

Comment by Jozef Wagner [ 02/Oct/14 5:48 AM ]

Type hinting is a very intricate part of Clojure but you can almost always apply a 'place hint on a symbol' idiom. Type hinting on an arg vector must be done only in two cases:

  • primitive hints
  • different return classes for different arities

In the first case, compiler needs type hints when compiling fn* (see [1]), not later, thus you must specify them on arg vector.

Second case, which is the issue discussed here, must be used only when defining with defn. Compiler first looks for the tag in the metadata of a var, and if it does not find one, it has a special case in which it looks for a return class inside :arglist metadata. This is clearly a very special case [2] to handle situations where you have different return classes for different arities. Obviously, using def instead of defn won't create an :arglist metadata for you thus you see a reflection warning. Example:

user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f2 [2 3 4]))
Reflection warning, /tmp/form-init.clj:1:1 - reference to field size can't be resolved.
3
user=> (alter-meta! #'f2 assoc :arglists '(^java.util.LinkedList [coll]))
{:ns #<Namespace user>, :name f2, :file "/tmp/form-init.clj", :column 1, :line 1, :arglists ([coll])}
user=> (.size (f2 [2 3 4]))
3

BTW CLJ-1491 has a discussion slightly relevant to this topic.

[1] https://github.com/clojure/clojure/blob/03cd9d159a2c49a21d464102bb6d6061488b4ea2/src/jvm/clojure/lang/Compiler.java#L5134
[2] https://github.com/clojure/clojure/blob/03cd9d159a2c49a21d464102bb6d6061488b4ea2/src/jvm/clojure/lang/Compiler.java#L3572

Comment by Jozef Wagner [ 02/Oct/14 7:15 AM ]

Andy, I've found sources that speak against my recommendations See CLJ-811 and [1].

[1] https://groups.google.com/d/msg/clojure/b005zQCPxOQ/6G0AlWKKKa0J





[CLJ-1542] Docstring for deliver should describe its return value Created: 30/Sep/14  Updated: 30/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

It is presumably useful when delivering a promise to know if the delivery was successful or not (where it might be unsuccessful if it was already delivered, perhaps on another thread).

The deliver function seems to currently communicate this by returning a truthy value (the promise itself) on success and a falsy value (nil) on failure. If this is intentional, the docstring should say so so that users can comfortably rely on it.

In CLJ-1038 Rich elected for the docstring to not describe the return value; I'm not sure if that was a reluctance to fully specify the return value (promise vs nil) even if partially describing it (truthy vs falsy) would be okay.






[CLJ-1538] Set literal duplicate check occurs too early. Created: 27/Sep/14  Updated: 09/Oct/14

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

Type: Defect Priority: Minor
Reporter: Chhi'mèd Künzang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader


 Description   

I cannot use literal syntax to create a set/map with unique members/keys if the elements are generated with an identical form. Examples of such legal forms: (rand), (read), (clojure.core.async/<!!), etc. I will use (rand) in these examples.

user=> #{(rand) (rand)}
IllegalArgumentException Duplicate key: (rand)  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:68)

user=> {(rand) 1, (rand) 2}

IllegalArgumentException Duplicate key: (rand)  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

It appears that the input is being checked for duplicates before the arguments to the collection constructors are evaluated. However, this doesn't prevent the need to run the check again later.

Note that duplicates are still (correctly) detected, after evaluation, even if duplicates do not appear as literals in the source:

user=> #{(+ 1 1) 2}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)
user=> {(+ 1 1) :a, 2 :b}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

The first duplicate check therefore seems to be both redundant and incorrect.

Note that this eager duplicate-checking seems to have higher precedence even than the syntax-quote reader macro.

user=> `#{~(rand) ~(rand)}

IllegalArgumentException Duplicate key: (clojure.core/unquote (rand))  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:68)

user=> `{~(rand) 1, ~(rand) 2}

IllegalArgumentException Duplicate key: (clojure.core/unquote (rand))  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

This is odd – since syntax-quote should not realize a collection at all at read time:

For Lists/Vectors/Sets/Maps, syntax-quote establishes a template of the corresponding data structure. Within the template, unqualified forms behave as if recursively syntax-quoted, but forms can be exempted from such recursive quoting by qualifying them with unquote or unquote-splicing, in which case they will be treated as expressions and be replaced in the template by their value, or sequence of values, respectively. (http://clojure.org/reader)

Definitions aside, based on the apparent expansion of syntax-quote, I would expect the previous to have worked correctly.

If I fake the expected macroexpansion by manually substituting the desired inputs, I get the expected results:

user=> '`#{~:a ~:b}
(clojure.core/apply clojure.core/hash-set (clojure.core/seq (clojure.core/concat (clojure.core/list :b) (clojure.core/list :a))))
user=> (clojure.core/apply clojure.core/hash-set (clojure.core/seq (clojure.core/concat (clojure.core/list (rand)) (clojure.core/list (rand)))))
#{0.27341896385866227 0.3051522362827035}
user=> '`{~:a 1, ~:b 2}
(clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat (clojure.core/list :a) (clojure.core/list 1) (clojure.core/list :b) (clojure.core/list 2))))
user=> (clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat (clojure.core/list (rand)) (clojure.core/list 1) (clojure.core/list (rand)) (clojure.core/list 2))))
{0.12476921225204185 2, 0.5807961046096718 1}

It seems to me that there is a superfluous duplicate check being run before the set/map reader macros evaluate their arguments. This check should seemingly be removed. Even if the check did not catch some false-positive duplicates (as it does), it would be unnecessary since the apparent second post-evaluation check would catch all true duplicates.

All that said, it's unclear that this check should happen at all. If I try to create sets/map with duplicate members/keys, I don't get an error. The duplicates are silently removed or superseded.

user=> (set (list 1 1))
#{1}
user=> (hash-map 1 2 1 3)
{1 3}

It seems it would be most consistent for literals constructed by the reader syntax to do the same.

I can see the argument that a literal representation is not a 'request to construct' but rather an attempt to simulate the printed representation of a literal data object. From that perspective, disallowing 'illegal' printed representations seems reasonable. Unfortunately, the possibility of evaluated forms inside literal vectors, sets, and maps (since lists are evaluated at read time) already breaks this theory. That is, the printed representation of such collections is not an accurately readable form, so read-time duplicate checking still cannot prevent seeming inconsistencies in print/read representations:

user=> '#{(+ 1 1) 2}
#{(+ 1 1) 2}
user=> #{(+ 1 1) 2}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

Given that the problem cannot be completely avoided at all, it seems simplest and most consistent to treat reader literal constructors like their run-time counterparts, as syntax quote would in the absence of the spurious duplicate check.



 Comments   
Comment by Alex Miller [ 09/Oct/14 8:04 AM ]

Also see CLJ-1555

Comment by Nicola Mometto [ 09/Oct/14 8:09 AM ]

Potentially related: http://dev.clojure.org/jira/browse/CLJ-1425





[CLJ-1532] pr-str captures stdout from printing side-effects of lazily evaluated expressions. Created: 23/Sep/14  Updated: 19/Jul/15

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

Type: Defect Priority: Minor
Reporter: Silas Davis Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: print
Environment:

Linux



 Description   

Because clojure.core/pr-str uses with-out-str to capture the output of pr (and pr cannot be parsed a writable thing - just uses out).

If you pr-str the result of something lazy you can get side-effects written to stdout with println interspersed with the output. For example in my case I was extracting benchmarks from the library criterium and trying to print the data structure to the file. The solution would be to provide an overload of pr/pr-str that takes a writer. I note that pr-on provides some of the functionality but it is private.

This is an ugly bug when you're trying to persist program output in EDN, because the randomly interspersed stdout messages make it invalid for read-string. We shouldn't need our functions to be pure for pr-str to work as expected.

I've omitted a patch because although I think a fix is straight-forward I'm not sure quite where it should go (e.g. make pr-on public, change pr, change pr-str)



 Comments   
Comment by Stuart Halloway [ 19/Jul/15 7:48 AM ]

as a workound for this, use print-dup or print-method





[CLJ-1526] clojure.core/> inconsistent behavior wrt to documentation. Created: 17/Sep/14  Updated: 22/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math


 Description   

The > function is inconsistent wrt to their behaviour for 0 arity.

user> (doc >)
-------------------------
clojure.core/>
([x] [x y] [x y & more])
  Returns non-nil if nums are in monotonically decreasing order,
  otherwise false.
nil
user> (> 3 2)
true
user> (> 3)
true
user> (>)
ArityException Wrong number of args (0) passed to: core/>  clojure.lang.AFn.throwArity (AFn.java:429)

This is mostly likely to become problematic when using > via apply where

(or (= 0 (count l))
    (apply > l))

It seems that the documentation should be updated, 0-arg case should return true, or the 1-arg case should also throw an exception.

This affects the other comparators also.



 Comments   
Comment by Robert Tweed [ 17/Sep/14 9:48 AM ]

As per my original post on this (here: https://groups.google.com/d/msg/clojure/8zkpO9FBN64/u2LAQsR93IgJ), while the question of whether an empty set has monotonic order perhaps has more than one answer in theory, from a purely pragmatic engineering perspective, it makes the most sense to evaluate to true here.

This /should/ not be a breaking change. Therefore it is fairly safe to introduce into a minor revision. It's a also a trivial fix. But it is possible (though highly unlikely) that someone could have code that depends on the exception being raised at runtime (as it does now) to handle empty lists in some special way. Such code is horrible and ought to be rewritten, so should not be seen as justification for retaining the current behaviour, which limits the general usefulness of these functions and may be responsible for subtle bugs in existing production code.

However such a change should probably not be backported to existing 1.6.x branches, just to be 100% safe, since it is not a security issue. My suggestion therefore would be to add a note to the docs in existing maintenance branches (any future 1.6.x) and evaluate to true in future versions (1.7+).





[CLJ-1516] Throw an exception if def name contains a dot Created: 29/Aug/14  Updated: 29/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-throw-an-exception-on-def-names-containing-dots.patch    
Patch: Code
Approval: Triaged

 Description   

In this comment: http://dev.clojure.org/jira/browse/CLJ-1100?focusedCommentId=35510&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-35510 Rich said that Vars whose name contains a dot are not supported, but the current implementation allows their definition.
This patch makes `(def foo.bar)` throw a compile-time exception



 Comments   
Comment by Alex Miller [ 29/Aug/14 10:41 AM ]

I'm curious whether this breaks existing code in the wild.

Comment by Nicola Mometto [ 29/Aug/14 10:45 AM ]

I find this hard to believe given the current behaviour:

user=> (def a.b 1)
#'user/a.b
user=> a.b
CompilerException java.lang.ClassNotFoundException: a.b, compiling:(NO_SOURCE_PATH:0:0)

one would need to go out of his way and refer to the var namespace qualified everywhere to make it work

Comment by Nicola Mometto [ 29/Aug/14 11:03 AM ]

After a brief conversation on #clojure, I updated the patch to only throw on non-macro defs so that macros like clojure.core/.. and clojure.core.incubator/.?. will work fine





[CLJ-1514] Use qualified class names for return type hints of standard Clojure functions Created: 28/Aug/14  Updated: 28/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, interop, patch, typehints

Attachments: Text File 0001-Use-fully-qualified-class-names-for-return-type-hint.patch    
Patch: Code

 Description   

The attached patch converts all function return type hints to spell out the class name fully qualified. There are two reasons for doing this:

1. Simple names in return type hints cause the issue described in http://dev.clojure.org/jira/browse/CLJ-1232. That's usually not a problem with return type hints referring to java.lang-classes because those are always imported. However, using `ns-unmap` you can remove them. For example, after `(ns-unmap ns 'String)` in my namespace, `(.length (format "foo = %s") 1)` throws an IllegalArgumentException: Unable to resolve classname: String. By using fully-qualified class names, that problem goes away.

2. tools.analyzer (used by the Clojure lint tool Eastwood) crashes when encountering such a simple-named return type hint. So currently, I cannot lint parts of my project because there's code that calls `clojure.core/format`.



 Comments   
Comment by Alex Miller [ 28/Aug/14 9:34 AM ]

1. that seems like a pretty weird thing to do
2. sounds like an issue with tools.analyzer, not with Clojure?

Comment by Nicola Mometto [ 28/Aug/14 10:46 AM ]

Just to clarify, tools.analyzer(.jvm) can analyze just fine forms in the form (defn x ^Class []) as long as Class is resolvable, whereas it will throw an exception if that function is then used in a namespace where that class is no longer resolvable, which is similar to what Clojure already does, except tools.analyzer.jvm will throw an exception even if the type hint is not used.

Since version 0.5.1 there's an handler that can be provided to change that behaviour, see https://github.com/clojure/tools.analyzer.jvm/blob/master/src/main/clojure/clojure/tools/analyzer/passes/jvm/validate.clj#L232

Comment by Nicola Mometto [ 28/Aug/14 11:02 AM ]

Now a comment regarding this ticket: the patch in this ticket is just a work-around for the issue exposed in http://dev.clojure.org/jira/browse/CLJ-1232, IMHO the correct move would be to actually recognize that issue as a bug rather than as an accepted "limitation" as Rich's comment seems to suggest so that a fix might be commited.

Comment by Tassilo Horn [ 28/Aug/14 1:29 PM ]

@Alex: 1. is not as weird as it sounds at first. For example, consider you have macros that generate complete APIs for something into some new namespace. Then it can make sense to use a real vanilla namespace, i.e., without referring clojure.core and importing java.lang. With 2. I side with Nicola and consider CLJ-1232 a bug.

@Nicola: Today I've used Eastwood (0.1.4) to lint my project. It crashed when it encountered this definition:

(defmacro error
  "Throws an exception with the given message and cause."
  ([msg]
     `(error ~msg nil))
  ([msg cause]
     `(throw (java.lang.Exception. ~msg ~cause))))

(defmacro errorf
  "Throws an exception with the given `msg` and `objs` passed to `format`.
  `msg` is a format string."
  [msg & objs]
  `(error (format ~msg ~@objs)))  ;; This is line 112 where the crash occurs

The message was:

Exception thrown during phase :analyze+eval of linting namespace funnyqt.tg-test
A function, macro, protocol method, var, etc. named clojure.core/format has been used here:
{:file "funnyqt/utils.clj",
 :end-column 19,
 :column 12,
 :line 112,
 :end-line 112}
Wherever it is defined, or where it is called, it has a type of String
This appears to be a Java class name with no package path.
Library tools.analyzer, on which Eastwood relies, cannot analyze such files.
If this definition is easy for you to change, we recommend you prepend it with
a full package path name, e.g. java.net.URI
Otherwise import the class by adding a line like this to your ns statement:
    (:import (java.net URI))

An exception was thrown while analyzing namespace funnyqt.tg-test 
Lint results may be incomplete.  If there are compilation errors in
your code, try fixing those.  If not, check above for info on the
exception.

So it seems it crashes because `format` has a `^String` return type hint. The namespace containing the `errorf` macro above has no modified ns-imports, i.e., all java.lang classes are imported there, too.

Comment by Nicola Mometto [ 28/Aug/14 1:46 PM ]

Tassilo, since `errorf` is a macro, that error is probably caused at the expansion point of that macro in a namespace that unmaps 'String.
If that's not the case, please open a ticket in the eastwood repo

Comment by Tassilo Horn [ 28/Aug/14 2:16 PM ]

Nicola, you are correct. As I've explained above to Alex, I generate APIs in fresh namespaces that don't refer clojure.core and also ns-unmap all java.lang classes, and the generated code also contains `errorf`-forms.

Well, since `ns-unmap` is there, I think it's legit to use it. So that makes CLJ-1232 even more important. But until that gets fixed which requires a common agreement that it is indeed a bug, I'd be very happy if this patch could be accepted. I mean, when it cannot do any harm and doesn't obscure anything but helps at least one person, then why not do it?





[CLJ-1509] Some clojure namespaces not AOT-compiled and included in the clojure jar Created: 20/Aug/14  Updated: 20/Aug/14

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build

Patch: Code
Approval: Triaged

 Description   

There is a list of namespaces to AOT in build.xml and several namespaces are missing from that list, thus no .class files for those namespaces are created or included in the standard clojure jar file as part of the build.

Missing namespaces include:

  • clojure.core.reducers
  • clojure.instant
  • clojure.parallel
  • clojure.uuid

Proposal: Attached patch sorts the ns list alphabetically (for easier maintenance) and adds clojure.instant and clojure.uuid to the compiled namespaces. clojure.parallel is deprecated and requires the JSR-166 jar so was not included (perhaps it's a separate ticket to remove this). clojure.core.reducers uses a compile-time check to choose the fork/join packages to use so cannot be compiled early.

Patch: clj-1509.diff

Screened by:



 Comments   
Comment by Alex Miller [ 20/Aug/14 1:06 PM ]

Looking at this a bit further, clojure.core.reducers uses the compile-if macro to determine what version of fork/join is available so AOT-compiling this namespace would fix that decision at build time rather than runtime, so it cannot be included.





[CLJ-1508] Supplied-p parameter in clojure Created: 18/Aug/14  Updated: 18/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring
Environment:

Mac OSX 10.9.4

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File supplied_p.diff    
Patch: Code and Test

 Description   

As see in https://groups.google.com/forum/?hl=en#!topic/clojure/jWc51JOkvsA

I think we can add a ? option for destructure ,then we can write a test like :

(deftest supplied-p-in-destructuring
  (let [{:keys [a b c d] :p? {a a-p? b b-p? c c-p? d d-p?} :or {a 1}} {:b 2 :c 3 }]
    (is (= a 1))
    (is (false? a-p?))
    (is (= 2 b))
    (is (true? b-p?))
    (is (= 3 c))
    (is (true? c-p?))
    (is (nil? d))
    (is (false? d-p?))))

Even if the a var has a default value 1 by :or option,but the a-p? is still false.
Just like the supplied-p-parameter in Commons LISP.

The patch is attached with code and test.



 Comments   
Comment by Steve Miner [ 18/Aug/14 8:24 AM ]

As mentioned on the mailing list, you could use {:as arg} destructuring to get same information. Here's a slightly modified example that works in the current Clojure:

(deftest supplied-p-in-destructuring
  ;; (let [{:keys [a b c d] :p? {a a-p? b b-p? c c-p? d d-p?} :or {a 1}} {:b 2 :c 3 }]
  (let [{:keys [a b c d] :or {a 1} :as argmap} {:b 2 :c 3 }
        supplied? (partial contains? argmap)
        a-p? (supplied? :a)
        b-p? (supplied? :b)
        c-p? (supplied? :c)
        d-p? (supplied? :d)]
    (is (= a 1))
    (is (false? a-p?))
    (is (= 2 b))
    (is (true? b-p?))
    (is (= 3 c))
    (is (true? c-p?))
    (is (nil? d))
    (is (false? d-p?))))




[CLJ-1507] Throw NPE in eval reader Created: 16/Aug/14  Updated: 16/Aug/14

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

Type: Defect Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: eval-reader
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fix_npe_eval_reader.diff    
Patch: Code

 Description   
Clojure 1.7.0-master-SNAPSHOT
user=> #=(var a)
NullPointerException   clojure.lang.Symbol.hashCode (Symbol.java:84)
user=> (.printStackTrace *e)
clojure.lang.LispReader$ReaderException: clojure.lang.LispReader$ReaderException: java.lang.NullPointerException
	at clojure.lang.LispReader.read(LispReader.java:218)
	at clojure.core$read.invoke(core.clj:3580)
	at clojure.core$read.invoke(core.clj:3578)
	at clojure.core$read.invoke(core.clj:3576)
	at clojure.core$read.invoke(core.clj:3574)
	at clojure.main$repl_read.invoke(main.clj:139)
	at clojure.main$repl$read_eval_print__6807$fn__6808.invoke(main.clj:237)
	at clojure.main$repl$read_eval_print__6807.invoke(main.clj:237)
	at clojure.main$repl$fn__6816.invoke(main.clj:257)
	at clojure.main$repl.doInvoke(main.clj:257)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.main$repl_opt.invoke(main.clj:323)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.LispReader$ReaderException: java.lang.NullPointerException
	at clojure.lang.LispReader.read(LispReader.java:218)
	at clojure.lang.LispReader$CtorReader.invoke(LispReader.java:1164)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:609)
	at clojure.lang.LispReader.read(LispReader.java:183)
	... 17 more
Caused by: java.lang.NullPointerException
	at clojure.lang.Symbol.hashCode(Symbol.java:84)
	at java.util.concurrent.ConcurrentHashMap.hash(ConcurrentHashMap.java:332)
	at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:987)
	at clojure.lang.Namespace.findOrCreate(Namespace.java:173)
	at clojure.lang.RT.var(RT.java:341)
	at clojure.lang.LispReader$EvalReader.invoke(LispReader.java:1042)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:616)
	at clojure.lang.LispReader.read(LispReader.java:183)
	... 20 more

If the var symbol doesn't contains namespace ,it will throw the NPE exception in above code.Instead,i think it should use Compiler.currentNS() when doesn't find the var's namespace.

The patch is attached, after patched:

Clojure 1.7.0-master-SNAPSHOT
user=> #=(var a)
#'user/a





[CLJ-1506] A little improvement when reading syntax quote form Created: 16/Aug/14  Updated: 30/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: syntax-quote
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fast_syntax_quote_reader.diff    
Patch: Code

 Description   

When reading syntax quote on keyword,string or number etc,it returns the form as result directly. Read it in:
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LispReader.java#L844-847

else if(form instanceof Keyword
       || form instanceof Number
       || form instanceof Character
       || form instanceof String)
   ret = form;

But missing check if it is a nil,regular pattern or boolean constants.
After patched:

else if(form == null
       || form instanceof Keyword
       || form instanceof Number
       || form instanceof Character
       || form instanceof Pattern
       || form instanceof Boolean
       || form instanceof String)
    ret = form;

It's a little patch, i am not sure if it is worth a try.






[CLJ-1502] Clojure Inspector navigation error Created: 12/Aug/14  Updated: 15/Aug/14

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

Type: Defect Priority: Minor
Reporter: Dan Campbell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, inspector, navigation
Environment:

Windows 7 and 8, Java 7, Clojure repl


Attachments: Text File clj-1502-v1.patch    
Patch: Code

 Description   

With Clojure 1.6.0 on some platforms (details below), if you create an object such as

(def nst (vec '((3 7 22) 99 (123 18 225 437))))

and then you inspect the tree representing the object

(inspect-tree nst)

Most of the navigation with the keyboard proceeds fine. However, when you point to an individual value - e.g. the 99 or the 437 - and press the right arrow key, there is an error

Exception in thread "AWT-EventQueue-0" java.lang.UnsupportedOperationException: count not supported on this type: Long
	at clojure.lang.RT.countFrom(RT.java:556)
	at clojure.lang.RT.count(RT.java:530)
	at clojure.inspector$fn__6907.invoke(inspector.clj:40)
	at clojure.lang.MultiFn.invoke(MultiFn.java:227)
	at clojure.inspector$tree_model$fn__6929.invoke(inspector.clj:63)
	at clojure.inspector.proxy$java.lang.Object$TreeModel$775afa87.getChildCount(Unknown Source)
	at javax.swing.plaf.basic.BasicTreeUI$Actions.traverse(BasicTreeUI.java:4395)
	at javax.swing.plaf.basic.BasicTreeUI$Actions.actionPerformed(BasicTreeUI.java:4052)
	at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1662)
	at javax.swing.JComponent.processKeyBinding(JComponent.java:2878)
	at javax.swing.JComponent.processKeyBindings(JComponent.java:2925)
	at javax.swing.JComponent.processKeyEvent(JComponent.java:2841)
	at java.awt.Component.processEvent(Component.java:6282)
	at java.awt.Container.processEvent(Container.java:2229)
	at java.awt.Component.dispatchEventImpl(Component.java:4861)
	at java.awt.Container.dispatchEventImpl(Container.java:2287)
	at java.awt.Component.dispatchEvent(Component.java:4687)
	at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1895)
	at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:762)
	at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:1027)
	at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:899)
	at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:727)
	at java.awt.Component.dispatchEventImpl(Component.java:4731)
	at java.awt.Container.dispatchEventImpl(Container.java:2287)
	at java.awt.Window.dispatchEventImpl(Window.java:2719)
	at java.awt.Component.dispatchEvent(Component.java:4687)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:735)
	at java.awt.EventQueue.access$200(EventQueue.java:103)
	at java.awt.EventQueue$3.run(EventQueue.java:694)
	at java.awt.EventQueue$3.run(EventQueue.java:692)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
	at java.awt.EventQueue$4.run(EventQueue.java:708)
	at java.awt.EventQueue$4.run(EventQueue.java:706)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:705)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)

Environments where this has been reproduced:
+ Windows 7 Enterprise, SP1, Oracle JDK 1.7.0_51, Clojure 1.6.0
+ Ubuntu Linux 14.04.1, Oracle JDK 1.7.0_65, Clojure 1.6.0

Environments where the same sequence of events does not cause an exception:
+ Mac OS X 10.8.5, Oracle JDK 1.7.0_51, Clojure 1.6.0



 Comments   
Comment by Andy Fingerhut [ 13/Aug/14 6:08 PM ]

Patch clj-1502-v1.patch avoids the exception in the situation reported. Tested manually on OS X, Linux, and Windows 7 versions mentioned in the patch comment. I suspect it is not worth the effort to write an automated test for this.

Comment by Dan Campbell [ 15/Aug/14 6:40 PM ]

Thanks, Andy

  • DC




[CLJ-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 11/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ex-info, exceptions
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.9.4


Attachments: File ex_info_arity.diff    
Patch: Code

 Description   

We often use 'ex-info' to throw a custom exception.But ex-info at least accepts two arguments: a string message and a data map.
In most cases,but we don't need to throw a exception that taken a data map.
So i think we can add a new arity to ex-info:

(ex-info "the exception message")

That created a ExceptionInfo instance carries empty data.

I am not sure it's useful for other people,but it's really useful for our developers.

The patch is attached.






[CLJ-1492] PersistentQueue objects are improperly eval'd and compiled Created: 06/Aug/14  Updated: 07/Aug/14

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

Type: Defect Priority: Minor
Reporter: Jon Distad Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

OS X 10.9.4
java version "1.7.0_60"
Java(TM) SE Runtime Environment (build 1.7.0_60-b19)
Java HotSpot(TM) 64-Bit Server VM (build 24.60-b09, mixed mode)


Attachments: Text File 0001-Exclude-PersistentQueue-from-IPersistentList-eval-co.patch    
Patch: Code and Test
Approval: Triaged

 Description   

PersistentQueue objects do not follow the correct evaluation path in the Compiler.

The simplest case:

user=> (def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
#'user/q
user=> q
#<PersistentQueue clojure.lang.PersistentQueue@7861>
user=> (eval q)
CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:4:1)

And you get the same exception when embedding a PersistentQueue:

user=> (eval `(fn [] ~q))
CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:2:1)

Instead of the expected:

CompilerException java.lang.RuntimeException: Can't embed unreadable object in code: #<PersistentQueue clojure.lang.PersistentQueue@7861>, compiling:(NO_SOURCE_PATH:3:1)

Since PersistentQueue implements IPersistentCollection and IPersistentList, and is not called out explicitly in the compiler, it is falling into the same compile path as a list. The exception comes from the call to emitValue inside the emitConstants portion of the FnExpr emit path. PersistentQueue does not implement java.util.List and thus the cast in emitListAsObjectArray (Compiler.java:4479) throws. Implementing List would NOT, however, resolve this issue, but would mask it by causing all eval'd PersistedQueues to be compiled as PersistentLists.

The first case is resolved by adding `&& !(form instanceof PersistentQueue)` to the IPersistentCollection branch of Compiler.eval() (Compiler.java:6695-8), allowing the PersistentQueue to fall through to the ConstantExpr case in analyze (Compiler.java:6459). The embedding case is resolved by adding `&& !(value instanceof PersistentQueue)` to the IPersistentList branch in ObjExpr's emitValue (Compiler.java:4639).

This bug also precludes definition of data-readers for PersistentQueue as the read object throws an exception when it is passed to the Compiler.

The attached patch includes the two changes mentioned above, and tests for each case that illustrates the bug.

Clojure-dev thread: https://groups.google.com/forum/#!topic/clojure-dev/LDUQfqjFg9w






[CLJ-1490] Exception on protocol implementation after protocol reloaded could be improved Created: 04/Aug/14  Updated: 07/Nov/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: errormsgs, protocols

Attachments: Text File CLJ-1490.1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

In a situation where you define a protocol, and then define a class that extends that protocol (e.g., reify, defrecord, deftype) and then later, re-define the protocol (typically, by reloading the namespace that defines the protocol), then the existing instances are no longer valid.

However, the exception that gets generated can be confusing:

     java.lang.IllegalArgumentException: No implementation of method: :injections of protocol: #'fan.microservice/MicroService found for class: fan.auth.AuthService
                                           clojure.core/-cache-protocol-fn                  core_deftype.clj:  544
                                           fan.microservice/eval23300/fn/G                  microservice.clj:   12
                                                       clojure.core/map/fn                          core.clj: 2559
                                                 clojure.lang.LazySeq.sval                      LazySeq.java:   40
                                                  clojure.lang.LazySeq.seq                      LazySeq.java:   49
                                                    clojure.lang.Cons.next                         Cons.java:   39
                                             clojure.lang.RT.boundedLength                           RT.java: 1654
                                               clojure.lang.RestFn.applyTo                       RestFn.java:  130
                                                        clojure.core/apply                          core.clj:  626
                 fan.microservice.StandardContainer/construct-ring-handler                  microservice.clj:   51

The confusing part is that (in the above example) AuthService does extend MicroService, just not the correct version of it.

The exception message should be extended to identify that this is "possibly because the protocol was reloaded since the class was defined."

A patch will be ready shortly.



 Comments   
Comment by Howard Lewis Ship [ 04/Aug/14 12:15 PM ]

Patch with tests





[CLJ-1486] Make fnil var-arg Created: 31/Jul/14  Updated: 18/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-make-fnil-vararg.patch    
Patch: Code and Test

 Description   

Currently fnil is defined only for 1 to 3 args, this patch makes it var-arg






[CLJ-1475] :post condition causes compiler error with recur Created: 25/Jul/14  Updated: 29/Jul/14

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: File clj-1475.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Michael O'Keefe <michael.p.okeefe@gmail.com> posted on the mailing list an example of code that causes a compiler error only if a :post condition is added. Here's my slightly modified version:

(defn g
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (if (seq xs)
     (recur (next xs) (+ (first xs) acc))
     acc))

CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position

The work-around is to wrap the body in a loop that simply rebinds the original args.



 Comments   
Comment by Steve Miner [ 25/Jul/14 9:53 AM ]

A macro expansion shows that body is placed in a let form to capture the result for later testing with the post condition, but the recur no longer has a proper target. The work-around of using a loop form is easy once you understand what's happening but it's a surprising limitation.

Comment by Steve Miner [ 25/Jul/14 9:55 AM ]

Use a local fn* around the body and call it with the original args so that the recur has a proper target. Update: not good enough for handling destructuring. Patch withdrawn.

Comment by Michael Patrick O'Keefe [ 25/Jul/14 10:37 AM ]

Link to the original topic discussion: https://groups.google.com/d/topic/clojure/Wb1Nub6wVUw/discussion

Comment by Steve Miner [ 25/Jul/14 1:42 PM ]

Patch withdrawn because it breaks on destructured args.

Comment by Steve Miner [ 25/Jul/14 5:27 PM ]

While working on a patch, I came up against a related issue: Should the :pre conditions apply to every recur "call". Originally, I thought the :pre conditions should be checked just once on the initial function call and never during a recur. People on the mailing list pointed out that the recur is semantically like calling the function again so the :pre checks are part of the contract. But no one seemed to want the :post check on every recursion, so the :post would happen only at the end.

That means automatically wrapping a loop (or nested fn* call) around the body is not going to work for the :pre conditions. A fix would have to bring the :pre conditions inside the loop.

Comment by Steve Miner [ 26/Jul/14 8:54 AM ]

I'm giving up on this bug. My approach was adding too much complexity to handle an edge case. I recommend the "loop" work-around to anyone who runs into this problem.

(defn g2
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (loop [xs xs acc acc]
    (if (seq xs)
       (recur (next xs) (+ (first xs) acc))
       acc)))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 10:29 AM ]

Add patch that handles rest arguments and destructuring.

Comment by Michael Patrick O'Keefe [ 26/Jul/14 10:57 AM ]

With regard to Steve's question on interpreting :pre, to me I would expect g to act like the case g3 below which uses explicit recursion (which does work and does appear to check the :pre conditions each time and :post condition once):

(defn g3
  [xs acc]
  {:pre [(or (sequential? xs) (nil? xs)) (number? acc)]
   :post [(number? %)]}
  (if (seq xs)
    (g3 (next xs) (+ (first xs) acc))
    acc))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 11:42 AM ]

Patch clj-1475.diff handles destructuring, preconditions and rest arguments

Comment by Steve Miner [ 26/Jul/14 4:04 PM ]

The clj-1475.diff patch looks good to me.

Comment by Alex Miller [ 27/Jul/14 7:18 AM ]

Please don't use "patch" as a label - that is the purpose of the Patch field. There is a list of good and bad labels at http://dev.clojure.org/display/community/Creating+Tickets

Comment by Steve Miner [ 27/Jul/14 11:32 AM ]

More knowledgeable commenters might take a look at CLJ-701 just in case that's applicable to the proposed patch.

Comment by Kevin Downey [ 29/Jul/14 1:35 AM ]

re clj-701

it is tricky to express loop expression semantics in jvm byte code, so the compiler sort of punts, hoisting expression loops in to anonymous functions that are immediately invoked, closing over whatever is in scope that is required by the loop, this has some problems like those seen in CLJ-701, losing type data which the clojure compiler doesn't track across functions, the additional allocation of function objects (the jit may deal with that pretty well, I am not sure) etc.

where the world of clj-701 and this ticket collide is the patch on this ticket lifts the function body out as a loop expression, which without the patch in clj-701 will have the issues I listed above, but we already have those issues anywhere something that is difficult to express in bytecode as an expression (try and loop) is used as an expression, maybe it doesn't matter, or maybe clj-701 will get fixed in some way to alleviate those issues.

general musings

it seems like one feature people like from asserts is the ability to disable them in production (I have never actually seen someone do that with clojure), assert and :pre/:post have some ability to do that (it may only work at macroexpansion time, I don't recall) since the hoisting of the loop could impact performance it might be nice to have some mechanism to disable it (maybe using the same flag assert does?).





[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 30/Jul/15

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch    
Patch: Code
Approval: Triaged

 Description   

Android ART runs compile time verification on bytecode and was failing on any usage of the locking macro. Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470

Having the monitor-enter inside the try block seems wrong to me, as surely if the lock fails to be acquired, it shouldn't be released with monitor-exit. Moving the monitor enter outside the try block seems to have resolved the issue and android no longer complains about usages of locking and all clojure tests still pass.

Java's generated code goes further and catches any exceptions generated by the monitor-exit itself and retries indefinitely (I believe the logic is that then at least your deadlock is in the right place, and not next time something else attempts to acquire a lock on the same object). I don't think that this can be replicated in clojure without getting down to the bytecode emitting level though and it doesn't seem to be an issue for the ART verifier.



 Comments   
Comment by Adam Clements [ 24/Jul/14 11:17 AM ]

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Comment by Andy Fingerhut [ 01/Aug/14 9:08 PM ]

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing

Comment by Adam Clements [ 04/Aug/14 4:24 PM ]

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.

Comment by Adam Clements [ 25/Nov/14 8:31 AM ]

Have just tested with Lollipop, and this patch might no longer be sufficient.

Getting in touch with the ART guys to see if they can shed a little more light and verify whether it will work on the current master branch of AOSP

Comment by Adam Clements [ 25/Nov/14 9:49 AM ]

Bug filed with AOSP project, hopefully they can shed some light on whether it is our problem and if so how we can fix it.

https://code.google.com/p/android/issues/detail?id=80823

Comment by Adam Clements [ 28/Nov/14 11:03 AM ]

I have uploaded an alternative implementation of the locking macro (0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch) which cheats a little - the synchronized block is actually implemented in Java and so guarantees compatibility. This is at the cost of a little extra indirection and the naming/location could probably be better.

But it does fix the bug and work on all versions of android, android + art and the jvm. Would this approach be acceptable?

Comment by Kevin Downey [ 08/Dec/14 1:12 PM ]

I have yet to see any evidence that the bytecode clojure is generating in some way violates the jvm spec, so I suspect the issue is clojure requires a jvm to run, and android doesn't provide a jvm, just something that looks like one if you don't tread outside the beaten path.

Comment by Kevin Downey [ 08/Dec/14 1:27 PM ]

given the structured locking verbiage in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10, `(locking nil)` may generate bytecode whose runtime behavior violates structured locking. the first patch on this issue can cause the compiler to emit monitorenter/exit instructions in different methods, which definitely violates structured locking

Comment by Adam Clements [ 09/Dec/14 10:45 AM ]

Yes, the first patch was definitely wrong, I left it for some context to the conversation, but it's probably best to just remove it for clarity.

For anyone following this conversation who doesn't want to decompile and observe the bytecode, here's a gist with the difference between a java synchronized block and clojure locking https://gist.github.com/AdamClements/2ae6c4919964b71eb470

I'm finding it hard to work out where the deviation from the spec occurs too, though I can see the difference with the Java version, if anything, the Clojure version looks closer to what's described in the spec than the Java one!

If someone with more knowledge than me on the subject could engage on the AOSP bug https://code.google.com/p/android/issues/detail?id=80823 then perhaps we could settle this as an android bug which is too focussed on the java implementation rather than the JVM spec, or perhaps they'll find something that's wrong with the Clojure implementation. I have uploaded the original clojure behaviour and asked them for some more explanation on why it is failing.

Comment by Adam Clements [ 09/Dec/14 11:09 AM ]

The response from the ART guys about what they think we're violating is:

The section on "Structured locking" contains the following:

"[...] implementations [...] are permitted but not required to enforce
both of the following two rules guaranteeing structured locking. [...]"

ART currently enforces both rules at verification time, including

"At no point during a method invocation may the number of monitor exits
performed by T on M since the method invocation exceed the number of
monitor entries performed by T on M since the method invocation."

Comment by Adam Clements [ 09/Dec/14 11:32 AM ]

If for example instruction https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24 or the monitor-enter itself on the next line were to fail, couldn't it could end up in the finally clause and attempt to release the lock even though it has never been captured?

I think this violates the structured locking rules in the jvm spec you linked to.

Comment by Kevin Downey [ 09/Dec/14 1:15 PM ]

an interesting question about structured locking, is does the specification refer to the static bytecode or the runtime behavior of the bytecode. given the bytecode linked(https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24), the static bytecode has the same number of entries and exits, but the dynamic behavior may be different. I wonder which one the art guys claim to be enforcing at verification time (it seems like it would have to be the static bytecode, not the dynamic properties, but then they shouldn't be failing to verify this). looking at the google code issue, the comment https://code.google.com/p/android/issues/detail?id=80823#c6 was made by the same dev as https://code.google.com/p/android/issues/detail?id=80823#c3, so I sort of suspect there is some miscommunication going on. It is not clear in what context the dev is replying in, since in the previous comment you mention splitting monitor-enter and exit across methods. I think things would be much clearer if all patches, specialized clojure android builds, etc, were gotten rid of, then with a vanilla clojure jar you get a javap dump of what fails to verify, then just take that over to the android issue tracker and ask "hey, this fails to verify, why?"

Comment by Adam Clements [ 11/Dec/14 9:15 AM ]

Yeah, I shouldn't have confused it with the patched versions. The gist and the currently uploaded version use the vanilla clojure version of the locking macro now though.

I think the issue comes from the exception table and the instructions that covers. If line 24 can throw for example, you would end up at runtime with a monitor-exit, having never encountered a monitor-enter.





[CLJ-1471] Option to print type info Created: 21/Jul/14  Updated: 21/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: pprint


 Description   

I've had an issue with defrecord-types being converted into ordinary maps somewhere, which was relatively hard to track down inside a deep structure since they are pprinted as the same thing by default.
The following code patches into the pprint dispatch and prints the type around values; it turned out to be quite useful, but feels hackish.
Maybe something like that would be useful to integrate into clojure.pprint directly (there are a number of cosmetic options already), i.e. into clojure.pprint/write-out.

Only printing (type) may not be enough in some cases; so an option to print all metadata would be nice.
Maybe something like :metadata nil as default, :metadata :type to print types (but also for non-IMetas, using (type) and :metadata true to print metadata for IMetas using (meta).

(defn pprint-with-type
  ([object] (pprint object *out*))
  ([object writer]
   ; keep original dispatch.
   ; calling it directly will print only that object,
   ; but return to our dispatch for subobjects.
   (let [dispatch clojure.pprint/*print-pprint-dispatch*]
     (binding [clojure.pprint/*print-pprint-dispatch*
               (fn [obj]
                 (if (instance? clojure.lang.IMeta obj)
                   (do (print "^{:type ")
                       (dispatch (type obj))
                       (print "} ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj))
                   (do (print "(^:type ")
                       (dispatch (type obj))
                       (print " ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj)
                       (print ")"))))]
       (clojure.pprint/pprint object writer)))))





[CLJ-1470] Make Atom and ARef easy to subclass Created: 20/Jul/14  Updated: 23/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Aaron Craelius Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1470-v1.patch    
Patch: Code

 Description   

Atom is currently defined as final and ARef.validate() is package-private. This makes it impossible to define a subclass of an Atom and difficult subclass ARef (if validate() needs to be called).

I propose removing the final modifier from Atom, making ARef.validate() protected and also making Atom.state protected (it is currently package-private).

I'm not sure if there is a specific reason why Atom is final - if this is for performance reasons or to prevent someone from doing strange things with Atom's, but I can see a use case for sub-classing it.

One use-case is to create reactive Atom that allows derefs to be tracked (as in reagent). I have some Clojure (not Clojurescript) code where I'm trying to play with this idea and I've had to copy the entire Atom class (because it's sealed) and place it in the clojure.lang package (because ARef.validate() is package-private): https://github.com/aaronc/freactive/blob/master/src/java/clojure/lang/ReactiveAtom.java. In addition, I need to copy the defns for swap! and reset! into my own namespace. This seems a bit inconvenient.



 Comments   
Comment by Kevin Downey [ 23/Jul/14 12:55 AM ]

related to http://dev.clojure.org/jira/browse/CLJ-803





[CLJ-1463] Providing own ClassLoader for eval is broken Created: 10/Jul/14  Updated: 21/Jul/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.5, Release 1.6
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

Clojure 1.6.0



 Description   

clojure.lang.Compiler has a method with the signature

public static Object eval(Object form, boolean freshLoader)

but the freshLoader argument is ignored since https://github.com/clojure/clojure/commit/2c2ed386ed0f6f875342721bdaace908e298c7f3

Is there a good reason this still needs to be "hotfixed" like this?

We would like to provide our own ClassLoader for eval to manage the lifecycle of the generated classes.



 Comments   
Comment by Stuart Halloway [ 21/Jul/15 8:04 AM ]

This is not part of the public API of Clojure. We would need to understand more about the use case.





[CLJ-1462] cl-format throws ClassCastException: Writer cannot be cast to Future/IDeref Created: 07/Jul/14  Updated: 09/Jul/14

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

Type: Defect Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print


 Description   

Using ~I and ~_ etc fails in many situations, the most trivial one being:

Clojure 1.6.0 and 1.5.1:

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Clojure 1.4.0

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.OutputStreamWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)

These work in other implementations, i.e. clisp, creating empty output in these trivial cases:

> (format t "~I")
NIL
> (format nil "~I")
""
> (format nil "~_")
""


 Comments   
Comment by Andy Fingerhut [ 07/Jul/14 11:01 AM ]

The tilde-underscore sequence is for "conditional newline", according to the CLHS here: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cea.htm

Tilde-capital-letter-I is for indent: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cec.htm

Comment by Pascal Germroth [ 07/Jul/14 12:09 PM ]

Ah, didn't think to try that. It fails without cl-format as well:

user=> (clojure.pprint/pprint-newline :linear)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/pprint-indent :block 0)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Manually creating a pretty writer does work though:

user=> (binding [*out* (clojure.pprint/get-pretty-writer *out*)] (clojure.pprint/pprint-newline :linear))
nil

In the get-pretty-writer doc it says:

Generally, it is unnecessary to call this function, since pprint,
write, and cl-format all call it if they need to.

Which appears to not be true for cl-format, and it would be nice if it would be applied automatically for all functions that need a pretty writer.

Comment by Pascal Germroth [ 09/Jul/14 6:37 PM ]

More bad news!
Manually creating a pretty-writer doesn't do the trick either, because it is not being properly flushed:

user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world~%"))
hello world
nil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world"))
hellonil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world") (.ppflush *out*))
hello worldnil

The ~% inserts an unconditional newline like \n, which also works as expected.

Insert ~_ before and it only prints up to that one. But I've also managed to get it to abort at other ~_ s, maybe because other commands flushed it.

Manually flushing it, like the inexplicably private with-pretty-writer macro does works though.
I don't understand why get-pretty-writer is exposed but not the macro that is needed to use it properly. Also all functions using pretty-writer facilities should use with-pretty-writer, that's what it appears to be specifically designed for. Then there's no need to expose it (or get-pretty-writer).





[CLJ-1455] Postcondition in defrecord: Compiler unable to resolve symbol % Created: 28/Jun/14  Updated: 29/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord


 Description   

Clojure's postconditions[1] are a splendiferous, notationally
idiot-proof way to scrutinize a function's return value without
inadvertently causing it to return something else.

Functions (implementing protocols) for a record type may be defined in
its defrecord or with extend-type. In functions defined in
extend-type, postconditions work as expected. Therefore, it is a
surprise that functions defined in defrecord cannot use
postconditions.

Actually it appears defrecord sees a pre/postcondition map as ordinary
code, so the postcondition runs at the beginning of the function (not
the end) and the symbol % (for return value) is not bound.

The code below shows a protocol and two record types that implement
it. Type "One" has an in-the-defrecord function definition where the
postcondition does not compile. Type "Two" uses extend-type and the
postcondition works as expected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defprotocol ITimesThree
  (x3 [a]))

;; defrecord with functions inside cannot use postconditions.
(defrecord One
    []
  ITimesThree
  (x3 [a]
    {:pre [(do (println "One x3 pre") 1)] ;; (works fine)
     :post [(do (println "One x3 post, %=" %) 1)]
     ;; Unable to resolve symbol: % in this context.
     ;; With % removed, it compiles but runs at start, not end.
     }
    (* 1 3)))

;; extend-type can add functions with postconditions to a record.
(defrecord Two
    [])
(extend-type Two
  ITimesThree
  (x3 [a]
    {:pre [(do (println "Two x3 pre") 1)] ;; (works fine)
     :post [(do (println "Two x3 post, %=" %) 1)] ;; (works fine)
     }
    (* 2 3)))

(defn -main
  "Main"
  []
  (println (x3 (->One)))
  (println (x3 (->Two))))

[1] http://clojure.org/special_forms, in the fn section.






[CLJ-1447] Make proxy work with protocols directly (like reify does) Created: 18/Jun/14  Updated: 18/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently Proxy only supports interfaces and abstract classes. While protocols are supported via the protocol's interface, this means that the method names must be java mangled. E.g. the method name for set-value! becomes set_value_BANG_. However, the only possible way to subclass abstract classes in Clojure is currently via gen-class (doesn't work from the REPL) or proxy.






[CLJ-1446] (def v) with no init supplied destroys #'v metadata Created: 13/Jun/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Nahuel Greco Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

(def a) destroys #'a metadata, check this:

(def ^:mykey a 1)

(meta #'a)              ;; ok, :mykey is present

(let [v (def a)]
   [(meta v)            ;; NO :mykey present, metadata destroyed
    (identical? v #'a)  ;; true, we are talking of the same var
   ])

(meta #'a)              ;; NO :mykey present

If this is not a bug but a "feature", then we have at least two problems:

1- The def special form documentation doesn't state this behaviour at all, it needs to be clarified. With the current documentation it seems as doing a def with no init supplied will not make any side-effect at all, and this is not true for the var metadata.

2- defmulti uses this form to lookup the var and check if it already binds to a MultiFn, if that is the case then defmulti does nothing... but it really does something, defmulti will destroy the original var metadata in the (supposedly non-destructive) check. This is the involved defmulti fragment:

(let [v# (def ~mm-name)]
  (when-not (and (.hasRoot v#) (instance? clojure.lang.MultiFn (deref v#)))
   ...


 Comments   
Comment by Alex Miller [ 13/Jun/14 4:14 PM ]

I think this is mostly a dupe of CLJ-1148 but I'll leave it as it states the specific problem more precisely.

Comment by Nahuel Greco [ 13/Jun/14 7:35 PM ]

Alex Miller: It seems CLJ-1148 is an special case where this problem shows, but the patches in CLJ-1148 only fixes the issues for defonce, not generally for def, not for defmulti and not clarifies this behaviour in the def special form documentation.

Comment by Stuart Halloway [ 31/Jul/15 2:49 PM ]

I am pretty sure we have been here before, and decided that def is working as desired. (If anybody can find the thread/ticket please add a link.) I think this should be a doc enhancement.

If the behavior of defmethod is a separate bug, please make a separate ticket for that, showing an example problem.

Comment by Andy Fingerhut [ 31/Jul/15 3:00 PM ]

CLJ-1213 might be related, but it doesn't mention metadata, only (def foo) without init value given.





[CLJ-1445] pprint prints some metadata when *print-meta* bound to true, but not all Created: 13/Jun/14  Updated: 13/Jun/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: File clj-1445-workaround-v2.clj    

 Description   

Short example illustrating the behavior:

user=> *clojure-version*
{:major 1, :minor 6, :incremental 0, :qualifier nil}

user=> (def f1 '(defn foo [^Integer x] ^{:bar 8} (inc x)))
#'user/f1

;; pr shows all metadata, as expected

user=> (binding [*print-meta* true] (pr f1))
^{:line 2, :column 10} (defn foo [^Integer x] ^{:bar 8, :line 2, :column 33} (inc x))nil

;; pprint shows some metadata, but not all

user=> (binding [*print-meta* true] (clojure.pprint/pprint f1))
(defn foo [^Integer x] (inc x))
nil

I have not dug into the details yet, but it appears that this may be because pprint uses pr to show symbols, but not to show collections. Thus pprint shows metadata on symbols, but not collections.

It would be nice if pprint could instead show all metadata, as pr does, when print-meta is bound to true.



 Comments   
Comment by Andy Fingerhut [ 13/Jun/14 11:30 AM ]

Attached file clj-1445-workaround-v1.clj is a function that pprints with more metadata than clojure.pprint does. As noted in the comments, it may not show metadata on other metadata. Please update with an enhanced version if you create one.

Comment by Andy Fingerhut [ 13/Jun/14 12:26 PM ]

Attached file clj-1445-workaround-v2.clj supersedes the earlier one, which I will delete.

The included function pprint-meta appears to be a correct way to pprint values with all metadata, even if the metadata maps themselves have metadata on them.





[CLJ-1444] Fix unquote splicing for empty seqs Created: 11/Jun/14  Updated: 12/Nov/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader, syntax-quote

Attachments: Text File 0001-Fix-unquote-splicing-for-empty-seqs-This-required-ma.patch    
Patch: Code and Test

 Description   

Current behaviour:

user=> `(~@())
nil
user=> `[~@()]
[]

Expected behaviour:

user=> `(~@())
()
user=> `[~@()]
[]


 Comments   
Comment by Andy Fingerhut [ 06/Aug/14 2:21 PM ]

Patch 0001-Fix-unquote-splicing-for-empty-seqs.patch dated Jun 11 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether this patch is straightforward to update.

Comment by Nicola Mometto [ 06/Aug/14 2:31 PM ]

Updated patch to apply to HEAD

Comment by Nicola Mometto [ 12/Nov/14 10:07 AM ]

This patch requires the patch at http://dev.clojure.org/jira/browse/CLJ-1586 to be applied first otherwise some compile-time metdata might get lost.





[CLJ-1443] reduce docstring partly incorrect with reducers. Created: 10/Jun/14  Updated: 10/Jun/14

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, reducers


 Description   

The docstring for reduce includes this: "If val is not supplied, returns the result of applying f to the first 2 items in coll". This is true if coll is a sequence, but not if it is a reducer. For example:

user=> (->> (range 0 10 2) (reduce (fn[x y] (+ x y))))
20
user=> (->> (range 0 10 2) (r/map #(/ % 2)) (reduce (fn[x y] (+ x y))))
ArityException Wrong number of args (0)

The docstring should be updated to make it clear that reducers (used without an initial seed value) require the reducing function to support a 0 arity overload returning the identity value for the reduction operation.






[CLJ-1441] Provide docs on how to reference imports that conflict with default ns class imports Created: 07/Jun/14  Updated: 07/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation


 Description   

This is related to CLJ-1440; a name clash on class "Compiler" between clojure.lang and another package.

The documentation does not address how to handle this cleanly; specifically, refer would appear to allow a way to exclude clojure.lang.Compiler, but does not.



 Comments   
Comment by Alex Miller [ 07/Jun/14 7:56 PM ]

refer is all about symbols that refer to Var. refer's docstring seems pretty clear on that to me.

Your conflict is on symbols that refer to a Class, which is the domain of import and has no exclusion facilities. The set of default imports is defined in RT.DEFAULT_IMPORTS and includes clojure.lang.Compiler along with everything in java.lang.*.

You can always fully-qualify any class you want to use in your ns, so that is one workaround available. Another is what Nicola suggested in CLJ-1440 - post-modify the ns after load.

Either ns or import could theoretically document more explicitly the list of auto-imports and recommend a solution to this problem. I'm not sure whether this is worth doing or would be accepted given the infrequency of the use case and availability of workarounds.

I tweaked http://clojure.org/namespaces to mention this.





[CLJ-1438] bit-* functions don't check for overflow Created: 05/Jun/14  Updated: 24/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, math


 Description   

The bit* functions, in contrast to the other numerical functions, don't appear to check for overflow, i.e. (bit-test 13 200000) returns true.

It would be nice if the behaviour would fit the other numerical operators, i.e. throw on overflow and provide a variant that doesn't, and one that works with arbitrary precision, also not currently supported:
(bit-test (bigint 13) 20000), (bit-test (biginteger 13) 20000) throw IllegalArgumentException.






[CLJ-1433] proxy-super calls generally use reflection Created: 28/May/14  Updated: 28/May/14

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints


 Description   

For example:

user=> (proxy [java.util.BitSet] []
  (flip [bitIndex]
    (proxy-super flip bitIndex)))
Reflection warning, NO_SOURCE_PATH:73:5 - call to method flip can't be resolved (target class is unknown).

I believe this issue might be fixed by simply adding type-hint metadata to the 'this symbol emitted by the proxy macro. I have not tried this change, but this macro seems to indicate it should work:

(defmacro proxy-super-cls [cls meth & args]
  (let [thissym (with-meta (gensym) {:tag cls})]
    `(let [~thissym ~'this]
      (proxy-call-with-super (fn [] (. ~thissym ~meth ~@args)) ~thissym ~(name meth))
    )))
;;;;;;;;;;;;;;;;;;;;;;
user=> (proxy [java.util.BitSet] []
  (flip [bitIndex]
    (proxy-super-cls java.util.BitSet flip bitIndex)))
#<BitSet$ff19274a {}>





[CLJ-1432] NullPointerException on function with primitive result declaration Created: 26/May/14  Updated: 30/May/14

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

Type: Enhancement Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: primitives, typehints


 Description   

The following minimal example shows the error:

(defn f ^double [])
(f)
=> NullPointerException

When decompiling the function `f` I found the following return expression:

return null.doubleValue();

This happened in a Java interop scenario where the called Java method had no return value but was in the return position of the primitive Clojure function.
The compiler should check for `null` on compilation.

Another example - calling a method with void return as the last expression fails in a similar way:

(defn f ^double [^SomeClassToAvoidRuntimeReflection obj, x, y]
  (.someMethod obj, x, y))
(f obj, x, y)
=> NullPointerException


 Comments   
Comment by Alex Miller [ 26/May/14 11:19 PM ]

What do you expect to happen in this case? You declared a function as returning a double but didn't return one.

Comment by Gunnar Völkel [ 27/May/14 8:48 AM ]

Since this is only the minimal example the error is relatively easy to spot.
Consider the following small example with Java interop:

(defn f ^double [^SomeClassToAvoidRuntimeReflection obj, x, y]
  (.someMethod obj, x, y))
(f obj, x, y)
=> NullPointerException

In this example it is much harder to find the reason for the NPE because you'd first suspect `obj` to be `null`.

I expect a check in the compiler at the point where "return null.doubleValue();" is emitted, followed by an error message, e.g. "Primitive return value of type 'double' expected, but no value is returned.".

Comment by Jozef Wagner [ 28/May/14 2:15 AM ]

Your second example seems perfectly OK to me, compiler should not report any error and NPE check must be at runtime.

Comment by Gunnar Völkel [ 28/May/14 2:46 AM ]

@Jozef: No, you are wrong. The compiler infers via reflection at compile time that the called method does not return a value and emits "return null.doubleValue()". So this can and should be reported as explicit error at compile time. I added a typehint to make it clear that there is no runtime reflection involved.
You would be right, if the compiler emitted something like "return somevar.doubleValue();" because then at compile time there is no knowledge about a possible "null" value.

Comment by Andy Fingerhut [ 28/May/14 10:00 AM ]

Gunnar, in your example, is the method 'someMethod' declared to return void, or something else? Adding that info to your example might help clarify it.

Comment by Jozef Wagner [ 29/May/14 2:26 AM ]

Gunnar, the second example was ambiguous and strayed away the discussion. Anyway, whether returning wrong type is through the native method or not, it is a user error in the first place. Right now it is reported at runtime. For me this ticket should be a minor enhancement instead of defect.

Comment by Gunnar Völkel [ 30/May/14 4:40 AM ]

Yes, the reason is a user error. But one that is harder to debug than necessary.
Also, it is clearly a defect since emitting 'null.doubleValue()' can not be considered as a valid compilation.

Andy, yes 'someMethod' is declared to return void. I'd edit the original ticket text to add the example and the java method return value information, but it seems jira does not let me.

Comment by Alex Miller [ 30/May/14 8:35 AM ]

I added the second example (with clarifying void comment) to the description.





[CLJ-1428] restart-agent is ignored inside an fn passed to set-error-handler. Created: 19/May/14  Updated: 19/May/14

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

Type: Defect Priority: Minor
Reporter: Rafik NACCACHE Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: agents
Environment:

Linux, jdk 1.7, emacs / cider



 Description   

If I pass a function containing start-agent to set-error-handler of an agent, if an exception occurs the restart-agent is ignored.
for example:

(def a (agent 0))

(set-error-handler! a (fn [the-agent the-exception] (restart-agent the-agent)) )

If I now issue : (send! a #(/ 1 0)), I still have a failed agent. It did not restart.

I know I can set the error-mode to the agent to :continue to have my agent up after a crash, but I wished I could fix the conditions that caused the exception in the first-place then restart the agent programmatically in the set-error-handler.

Maybe it is a known beahviour, but then it is not documented ?






[CLJ-1425] Defer literal map construction of syntax-quoted maps to allow for semantically valid unquote splicing Created: 16/May/14  Updated: 15/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Jon Distad Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader
Environment:

Any


Attachments: Text File 0001-Fix-map-unquote-splicing.patch    
Patch: Code and Test

 Description   

At present one cannot unquote-splice into a map literal unless the map contains an even number of literal forms, even if one of them is a null unquote (~@[]).

E.g.: `{~@[1 2]} ;=> RuntimeException Map literal must contain an even number of forms clojure.lang.Util.runtimeException (Util.java:219)

However, within the context of a syntax-quote, it is not essential that the map literal be represented internally as a map since the syntax-quote emits code to build the map and not the map itself. The syntaxQuote method on SyntaxQuoteReader does not even operate the map, but rather a flattened sequence of interleaved keys and values.

With the aid of metadata and a LispReader-global Var, we can track that a collection of elements within a syntax quote will become a map, and emit the proper code forms from the SyntaxQuoteReader. There is a small edge case in metadata literals, but an with additional piece of metadata containing the proto-map we can still generate the appropriate (with-meta ...) form at syntax-quote emission time.

Importantly, none of the hand-waving involved ever escapes the reader, and the eval/compile environment is none the wiser.

This allows the following:

`{~@[1 2]} ;=> after eval: {1 2}
`^{~@[:foo :bar]} sym ;=> metadata of 'sym after eval: {:foo :bar}

But not:
`~{1} ;=> RuntimeException ...

Or:{1} ;=> RuntimeException ...

And `{~@[1]} has the same semantics as the currently required `{~@[1] ~@[]}
;=> IllegalArgumentException No value supplied for key: 1 clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

The changes in my patch pass all existing tests and include an additional test for the newly-supported map unquote-splicing form.



 Comments   
Comment by Jon Distad [ 16/May/14 5:57 PM ]

Modified from this morning- more tests, plus bugfix for the new cases caught.

Comment by Jon Distad [ 17/May/14 10:47 AM ]

Updated patch.

Now uses two distinct paths for adding metadata. Old version potentially stacked with-meta calls, which could result in lost keys.

Comment by Kevin Downey [ 14/Oct/14 1:36 PM ]

It seems like this is a bad idea, it sort of makes sense from purely a macro writing perspective, but syntax quote is used outside of macros, in which case this just becomes a circumvention of the duplicate key checks that were added, I think some time around 1.3 maybe 1.4

http://dev.clojure.org/display/design/Allow+duplicate+map+keys+and+set+elements

Comment by Jon Distad [ 14/Oct/14 5:55 PM ]

Actually, unquote-splicing already circumvents the duplicate key check because it expands to an (apply hash-map ...) call.

In Clojure 1.7.0-alpha2

user> `{~@[:foo :bar :foo :bar] ~@[]}
;=> {:foo :bar}
user> '`{~@[:foo :bar :foo :bar] ~@[]}
;=> (clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat [:foo :bar :foo :bar] [])))

Comment by Kevin Downey [ 14/Oct/14 7:15 PM ]

yeah, sorry, I was confusing this implementation with a related issue that was closed. do you have a motivating example for this? I write a fair bit of clojure and have not found it to be an issue in practice, and I am leery of relaxing these sort of constraints. if we allow this behavior, then syntax quote can definitely never be pulled out of the reader(there may be other behavior that already makes this hard to impossible, I am not sure), effectively syntax quote would have to operate on data before it makes out of the reader, were as if maps used in syntax quote are "well formed" in may be possible to move syntax quote (a source of a lot of complexity in the reader) out of the reader and have it operate on data that has already been read in.

I am almost 100% sure making syntax quote a post reader macro is not a priority in any shape or form, but I just mention it as the sort of follow on thing that could have the door shut on it due to these kind of changes, I've general begun to think of basically anything related to syntax quote as adding syntax above beyond just data, which seems a negative.

So anyway, I don't feel much pain from this behavior and it seems like the "fix" could have some follow on consequences, so a solid motivating example would be good.

just to warn you away from spending time coming up with a motivating example, every feature I have railed against has been committed, so if you just ignore me there is a real chance you'll make it in

Comment by Jon Distad [ 15/Oct/14 8:16 AM ]

To be honest, I had forgotten I submitted this. I suppose it boils down to prioritizing principles- do the literal semantics of a map take precedence over the conceptual semantics? At this point I'm against my former position and I think the literal semantics of the should take precedence, as they currently do. Especially since this is in the reader.





[CLJ-1423] Applying a var to an infinite arglist consumes all available memory Created: 15/May/14  Updated: 15/May/14

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

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None

Attachments: Text File apply-var.patch    
Patch: Code and Test

 Description   

It is possible to apply a function to an infinite argument list: for example, (apply distinct? (repeat 1)) immediately returns false, after realizing just a few elements of the infinite sequence (repeat 1). However, (apply #'distinct? (repeat 1)) attempts to realize all of (repeat 1) into memory at once.

This happens because Var.applyTo delegates to AFn.applyToHelper to decide which arity of Var.invoke to dispatch to; but AFn doesn't expect infinite arglists (mostly those use RestFn). So it uses RT.seqToArray, which doesn't work well in this case.

Instead, Var.applyTo(args) can just dispatch to deref().applyTo(args), and let the function being stored figure out what to do with the arglist.

I've changed Var.applyTo to do this, and added a test (which fails before my patch is applied, and passes afterwards).






[CLJ-1422] Recur around try boxes primitives Created: 14/May/14  Updated: 28/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, performance, typehints


 Description   

Primitive function and recur variables can't pass through a (try) cleanly; they're boxed to Object instead. This causes reflection warnings for fns or loops that use primitive types.

user=> (set! *warn-on-reflection* true)
true
 
user=> (fn [] (loop [t 0] (recur t)))
#<user$eval676$fn__677 user$eval676$fn__677@3d80023a>
 
user=> (fn [] (loop [t 0] (recur (try t))))
NO_SOURCE_FILE:1 recur arg for primitive local: t is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: t
#<user$eval680$fn__681 user$eval680$fn__681@5419323a>

user=> (fn [^long x] (recur (try x)))
NO_SOURCE_FILE:1 recur arg for primitive local: x is not matching primitive, had: Object, needed: long

CompilerException java.lang.IllegalArgumentException:  recur arg for primitive local: x is not matching primitive, had: Object, needed: long, compiling:(NO_SOURCE_PATH:1:1)


 Comments   
Comment by David James [ 15/Jun/14 10:27 PM ]

Without commenting on the most desirable behavior, the following code does not cause reflection warnings:

user=> (set! *warn-on-reflection* true)
true
user=> (fn [] (loop [t 0] (recur (long (try t)))))
#<user$eval673$fn__674 user$eval673$fn__674@4e56c411>
Comment by Nicola Mometto [ 16/Jun/14 6:33 AM ]

Similar ticket http://dev.clojure.org/jira/browse/CLJ-701

Comment by Kevin Downey [ 21/Jul/14 6:59 PM ]

try/catch in the compiler only implements Expr, not MaybePrimitiveExpr, looking at extending TryExpr with MaybePrimitiveExpr it seems simple enough, but it turns out recur analyzes it's arguments in the statement context, which causes (try ...) to essentially wrap itself in a function like ((fn [] (try ...))), at which point it is an invokeexpr which is much harder to add maybeprimitiveexpr too and it reduces to the same case as CLJ-701

Comment by Kevin Downey [ 22/Jul/14 9:27 PM ]

http://dev.clojure.org/jira/browse/CLJ-701 has a patch that I think solves this

Comment by Alex Miller [ 28/Jul/14 1:56 PM ]

Should I dupe this to CLJ-701?

Comment by Kevin Downey [ 28/Jul/14 5:22 PM ]

if you want the fixes for try out of the return context to be part of CLJ-701 then yes it is a dupe, if you are unsure or would prefer 701 to stay more focused (my patch may not be acceptable, or may be too large and doing too much) then no it wouldn't be a dupe. I sort of took it on myself to solve both in the patch on CLJ-701 because I came to CLJ-701 via Nicola's comment here, and the same compiler machinery can be used for both.

I think the status is pending on the status of CLJ-701.





[CLJ-1420] ThreadLocalRandom instead of Math/random Created: 11/May/14  Updated: 29/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math, performance
Environment:

Requires Java >=1.7!


Attachments: Text File 0001-rand-using-ThreadLocalRandom-and-tests-for-random.patch    
Patch: Code
Approval: Vetted

 Description   

The standard Math.random() is thread-safe through being declared as a synchronized static method.

The patch uses java.util.concurrent.ThreadLocalRandom which actually seems to be two times faster than the ordinary Math.random() in a simple single threaded criterium.core/bench:

The reason I investigated the function at all was to be sure random-number generation was not a bottleneck when performance testing multithreaded load generation.

If necessary, one could of course make a conditional declaration (like in fj-reducers) based on the existence of the class java.util.concurrent.ThreadLocalRandom, if Clojure 1.7 is to be compatible with Java versions < 1.7



 Comments   
Comment by Linus Ericsson [ 11/May/14 11:05 AM ]

Benchmark on current rand (clojure 1.6.0), $ java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.4) (7u51-2.4.4-0ubuntu0.13.10.1)
OpenJDK 64-Bit Server VM (build 24.45-b08, mixed mode)

:jvm-opts ^:replace [] (ie no arguments to the JVM)

(bench (rand 10))
Evaluation count : 1281673680 in 60 samples of 21361228 calls.
Execution time mean : 43.630075 ns
Execution time std-deviation : 0.420801 ns
Execution time lower quantile : 42.823363 ns ( 2.5%)
Execution time upper quantile : 44.456267 ns (97.5%)
Overhead used : 3.194591 ns

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

Clojure 1.7.0-master-SNAPSHOT.

(bench (rand 10))
Evaluation count : 2622694860 in 60 samples of 43711581 calls.
Execution time mean : 20.474605 ns
Execution time std-deviation : 0.248034 ns
Execution time lower quantile : 20.129894 ns ( 2.5%)
Execution time upper quantile : 21.009303 ns (97.5%)
Overhead used : 2.827337 ns

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

I had similar results on Clojure 1.6.0, and ran several different tests with similar results. java.util.Random.nextInt is suprisingly bad. The ThreadLocalRandom version of .nextInt is better, but rand-int can take negative integers, which would lead to some argument conversion for (.nextInt (ThreadLocalRandom/current) n) since it need upper and lower bounds instead of a simple multiplication of a random number [0,1).

CHANGE:

The (.nextDouble (ThreadLocalRandom/current) argument) is very quick, but cannot handle negative arguments. The speed given a plain multiplication is about 30 ns.

Comment by Linus Ericsson [ 11/May/14 12:44 PM ]

Added some simplistic tests to be sure that rand and rand-int accepts ratios, doubles and negative numbers of various kinds. A real test would likely include repeated generative testing, these tests are mostly for knowing that various arguments works etc.

Comment by Linus Ericsson [ 11/May/14 1:38 PM ]

0001-rand-using-ThreadLocalRandom-and-tests-for-random.patch contains the changed (rand) AND the test cases.

Comment by Alex Miller [ 11/May/14 5:45 PM ]

Clojure requires Java 1.6.0 so this will need to be reconsidered at a later date. We do not currently have any plans to bump the minimum required JDK in Clojure 1.7 although that could change of course.

Comment by Gary Fredericks [ 11/May/14 5:54 PM ]

I've always thought that the randomness features in general are of limited utility due to the inability to seed the PRNG, and that a clojure.core/rand dynamic var would be a reasonable way to do that.

Maybe both of these problems could be partially solved with a standard library? I started one at https://github.com/fredericksgary/four, but presumably a contrib library would be easier for everybody to standardize on.

Comment by Linus Ericsson [ 12/May/14 2:17 AM ]

Gary, I'm all for creating some well-thought out random-library, which could be a candidate for some library clojure.core.random if that would be useful.

Please have a look at http://code.google.com/p/javarng/ since that seems to do what you library four does (and more). Probably we could salvage either APIs, algorithms or both from this library.

I'll contact you via mail!

Comment by Gary Fredericks [ 20/Jun/14 10:21 AM ]

Come to think of it, a rand var in clojure.core shouldn't be a breaking change, so I'll just make a ticket for that to see how it goes. That should at the very least allow solving the concurrency issue with binding. The only objection I can think of is perf issues with dynamic vars?

Comment by Gary Fredericks [ 20/Jun/14 10:42 AM ]

New issue is at CLJ-1452.

Comment by Andy Fingerhut [ 29/Aug/14 4:50 PM ]

Patch 0001-rand-using-ThreadLocalRandom-and-tests-for-random.patch dated May 11 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches





[CLJ-1416] Support transients in gvec Created: 06/May/14  Updated: 02/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: collections, transient

Attachments: Text File 0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch     Text File 0002-CLJ-1416-transients-hash-caching-interop-improvement.patch     Text File 0003-CLJ-1416-transients-hash-caching-interop-improvement.patch     Text File 0004-CLJ-1416-transients-hash-caching-interop-improvement.patch    
Patch: Code
Approval: Triaged

 Description   

Vectors of primitives produced by vector-of do not support transients.

core.rrb-vector implements transient support for vectors of primitives. Such transient-enabled vectors of primitives can be obtained in a number of ways: (1) using a gvec instance as an argument to fv/catvec (if RRB concatenation happens, which is not guaranteed) or fv/subvec; (2) passing a gvec instance to fv/vec, which as of core.rrb-vector 0.0.11 will simply rewrap the gvec tree in an RRB wrapper; (3) using fv/vector-of instead of clojure.core/vector-of. Native support in gvec would still be useful as part of an effort to make supported functionality consistent across vector flavours (see CLJ-787 in this connection); gvec is also simpler and still has (and is likely to maintain) a performance edge.

A port of core.rrb-vector's transient support to gvec is available here:

https://github.com/michalmarczyk/clojure/tree/transient-gvec

I'll bring it up to date with current master shortly.

See the clojure-dev thread for some benchmarks:

https://groups.google.com/d/msg/clojure-dev/9ozYI1e5SCM/BAIazVOkUmcJ



 Comments   
Comment by Michał Marczyk [ 13/May/14 5:32 AM ]

Here's the current version of the patch (0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch). It includes a few additional changes – here's the commit message:

CLJ-1416: transients, hash caching for gvec, Object methods for gvec seqs

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs

https://github.com/michalmarczyk/clojure/tree/transient-gvec-1.6

Comment by Michał Marczyk [ 05/Jul/14 2:59 AM ]

Here's an updated patch with some additional interop-related improvements.

The new commit message:

CLJ-1416: transients, hash caching, interop improvements for gvec

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs
  • Correctly implements iterator-related methods for gvec and gvec seqs
  • Introduces throw-unsupported and caching-hash (both marked private)
Comment by Andy Fingerhut [ 29/Aug/14 4:48 PM ]

Patch 0002-CLJ-1416-transients-hash-caching-interop-improvement.patch dated Jul 5 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Michał Marczyk [ 29/Aug/14 5:07 PM ]

Patch updated to apply cleanly to master.

Comment by Brandon Bloom [ 02/Oct/14 12:28 PM ]

Maybe this should be another ticket, but it would affect this patch, so I'll mention it here:

The ArrayManager interface is an incomplete abstraction. The original gvec code plus the new transients codepaths rely on System/arraycopy, rather than .arraycopy on the manager object. This means that it's impossible to create gvecs backed by non-JVM arrays. Or, in my case, to create a gvec of nibbles backed by an array of longs. See https://gist.github.com/brandonbloom/441a4b5712729dec7467

Comment by Brandon Bloom [ 02/Oct/14 1:34 PM ]

The current patch has a bug on line 762:

(let [node ^clojure.core.VecNode (.ensureEditable this node)

There is no such signature, only these:

(ensureEditable [this]
(ensureEditable [this node shift]

I discovered this problem using https://github.com/ztellman/collection-check

Comment by Michał Marczyk [ 02/Oct/14 2:46 PM ]

Thanks for the catch! Fixed patch attached. (There was in fact one more bug in editableArrayFor, also fixed in this version.)

Comment by Michał Marczyk [ 02/Oct/14 2:57 PM ]

As for gvecs of nibbles, could that be a separate ticket with patches building on top of this one?

On a separate note, core.rrb-vector could support vectors of nibbles as an extra feature (and adopt built-in gvec's representation if indeed the built-in gvec comes to support this feature at some point). Do you think that'd be useful?

Comment by Michał Marczyk [ 02/Oct/14 3:01 PM ]

Of course vectors of nibbles could be implemented today with a separate vector type wrapping a gvec of longs, but the implementation would be more involved. I wonder what kind of performance difference there would be between the wrapper approach and the "nibble AM" approach…





[CLJ-1412] Add 2-arity version of `cycle` that takes the numer of times to "repeat" the coll Created: 28/Apr/14  Updated: 18/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Add-2-arity-version-of-cycle-that-takes-the-number-o.patch    
Patch: Code

 Description   

There are already similar arities for repeat/repeatedly and similar functions, this patch adds a 2-arity version of cycle that behaves like this:

user> (cycle 0 '(1 2))
()
user> (cycle -1 '(1 2))
()
user> (cycle 3 '(1 2))
(1 2 1 2 1 2)
user> (cycle 1 '(1 2))
(1 2)


 Comments   
Comment by Andy Fingerhut [ 06/Aug/14 2:19 PM ]

Patch 0001-Add-2-arity-version-of-cycle-that-takes-the-number-o.patch dated Apr 28 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. This appears trivial to update, as it is likely only a couple of lines of diff context that have changed.

Comment by Nicola Mometto [ 06/Aug/14 2:36 PM ]

Updated patch to apply to HEAD





[CLJ-1411] Special symbols can be shadowed inconsistently Created: 28/Apr/14  Updated: 29/Apr/14

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

Type: Defect Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler


 Description   

The compiler does not complain about let binding (or def-ing) special symbols, but the binding only works if not used at the beginning of a list:

These work:

(let [try :a]
  try)
=> :a
(let [try (constantly :a)]
  (apply try :b))
=> :a

This doesn't work:

(let [try (constantly :a)]
  (try :b))
=> :b

This is true for all special symbols, not just publicly exposed ones like try and new, but also internal ones like fn*.

I would expect consistent behaviour: either the compiler does not permit shadowing special symbols at all, or shadowing them works in all cases.



 Comments   
Comment by Nicola Mometto [ 28/Apr/14 10:01 AM ]

I don't think that shadowing special symbols is a good idea, but probably having all the special symbols namespace qualified (clojure.core/import* is the only one ns-qualified atm) along with checking for the symbol in the locals env first and fallbacking to the special symbols map after that, would probably help in those scenarios

Comment by Volkert Oakley Jurgens [ 29/Apr/14 12:48 AM ]

I think that shadowing special symbols is a bad idea. If that was possible, we'd have to change most macros in clojure.core to make them safe (i.e. explicitly add a namespace to each special symbol usage). And how would we handle special symbols that are not just implementation specific, like try and new? Every 3rd party macro that uses those might become unsafe.

My personal preference would be to prohibit the shadowing of special symbols.

Comment by Nicola Mometto [ 29/Apr/14 5:37 AM ]

That won't be the case since what I'm proposing includes making syntax-quote aware of the namespaced special symbols.
`def would expand to 'clojure.core/def for example.

Comment by Volkert Oakley Jurgens [ 29/Apr/14 5:58 AM ]

That's true, but macros don't have to use the syntax quote. See for example the definition of when.





[CLJ-1409] Add support for marking gen-class methods as native Created: 21/Apr/14  Updated: 21/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: gen-class, interop


 Description   

As far as I know, there is no support for creating a Java instance in Clojure with native methods. Everything else needed exists, but there is no way to get the right annotation on the method right now (similar to static).

Here's an example (http://benchmarksgame.alioth.debian.org/u64q/program.php?test=pidigits&lang=clojure&id=4) from Alioth perf tests where ASM is being used directly to generate a class with native methods where gen-class would have been perfectly adequate with this enhancement. (Equivalent Java: http://benchmarksgame.alioth.debian.org/u64q/program.php?test=pidigits&lang=java&id=2).

Suggested implementation is to mark ^{:native true} on a method and omit the body.






[CLJ-1407] Recur mismatch might cause multiple evaluation Created: 17/Apr/14  Updated: 17/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, macro


 Description   

Since mismatching recurs cause the loop body to be re-analyzed, macroexpansion in the loop body might happen more than once, causing any side effects that happen during macroexpansion to be evaluated potentially multiple times

Clojure 1.7.0-master-SNAPSHOT
user=> (defmacro x [] (println "foo"))
#'user/x
user=> (fn [] (loop [y 1] (x) (recur (Integer. 1))))
foo
foo
#<user$eval6$fn__7 user$eval6$fn__7@71687585>


 Comments   
Comment by Andy Fingerhut [ 17/Apr/14 6:59 PM ]

This is not a question about whether the behavior in the description is a bug or not, but rather a curiosity about how often people write macros that have side effects at macroexpansion time. I think the following in Clojure itself do, but there may be others:

  • gen-class, and also ns because it uses gen-class
  • gen-interface, and also definterface because it uses gen-interface
  • clojure.core/compile-if (private) calls eval on its expr arg, but as used now doesn't cause macroexpansion-time side effects
  • doc seems to have one case that prints at macroexpansion time
  • I am not sure whether defprotocol or deftype have macroexpansion time side effects, or whether they are limited to run time
Comment by Nicola Mometto [ 17/Apr/14 9:20 PM ]

Andy, I don't think there are that many macros that side-effect at macroexpansion time and I haven't discovered this bug in real code but while thinking about how loop locals invalidation was implemented in Compiler.java.

Because there are a really a small number of side-effecting macros, this is unlikely to cause problems in real code, so I changed the priority to minor.





[CLJ-1402] sort-by calls keyfn more times than is necessary Created: 11/Apr/14  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Steve Kim Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJ-1402-v1.patch     Text File CLJ-1402-v2.patch    
Patch: Code

 Description   

clojure.core/sort-by evaluates keyfn for every pairwise comparison. This is wasteful when keyfn is expensive to compute.

user=> (def keyfn-calls (atom 0))
#'user/keyfn-calls
user=> (defn keyfn [x] (do (swap! keyfn-calls inc) x))
#'user/keyfn
user=> @keyfn-calls
0
user=> (sort-by keyfn (repeatedly 10 rand))
(0.1647483850582695 0.2836687590331822 0.3222305842748623 0.3850390922996001 0.41965440953966326 0.4777580378736771 0.6051704988802923 0.659376178201709 0.8459820304223701 0.938863131161208)
user=> @keyfn-calls
44


 Comments   
Comment by Steve Kim [ 11/Apr/14 11:46 AM ]

CLJ-99 is a similar issue

Comment by Michael Blume [ 09/Feb/15 3:03 PM ]

Avoid using for before it's defined

Comment by Andy Fingerhut [ 25/May/15 5:13 PM ]

Michael, does your patch CLJ-1402-v2.patch intentionally modify the doc string of sort-by, because the sentence you are removing is now obsolete? If so, that would be good to mention explicitly in the comments here.

Comment by Michael Blume [ 26/May/15 2:41 PM ]

Yep, the patch changes sort-by so that it maps over the collection and then performs a sort on the resulting seq. This means arrays will be unmodified and a new seq created instead.





[CLJ-1401] CompilerException / IllegalStateException when overriding vars Created: 10/Apr/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs

Approval: Triaged

 Description   
=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:4:1)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6745)
	clojure.lang.Compiler.analyze (Compiler.java:6529)
	clojure.lang.Compiler.analyze (Compiler.java:6490)
	clojure.lang.Compiler.eval (Compiler.java:6801)
	clojure.lang.Compiler.eval (Compiler.java:6760)
	clojure.core/eval (core.clj:3079)
	clojure.main/repl/read-eval-print--7095/fn--7098 (main.clj:240)
	clojure.main/repl/read-eval-print--7095 (main.clj:240)
	clojure.main/repl/fn--7104 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)
	clojure.main/main (main.clj:422)
Caused by:
IllegalStateException a already refers to: #'foo/a in namespace: bar
	clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
	clojure.lang.Namespace.intern (Namespace.java:72)
	clojure.lang.Compiler$DefExpr$Parser.parse (Compiler.java:534)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6738)
	clojure.lang.Compiler.analyze (Compiler.java:6529)

I would expect (at worst) a similar warning to the initial namespace loading, rather than an exception here.



 Comments   
Comment by Alex Miller [ 11/Apr/14 8:26 AM ]

Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.

Comment by Andy Fingerhut [ 11/Apr/14 10:19 AM ]

I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:

(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))

Then start a REPL with 'lein repl', and I see this behavior:

user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil

Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports.

It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.

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

Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.

Comment by Mike Anderson [ 12/Apr/14 12:41 AM ]

To reproduce:

(ns op)
(defn * [a b] (clojure.core/* a b)) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives error!

I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed.

The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op

Comment by Alex Miller [ 03/Nov/14 10:24 AM ]

Duped in CLJ-1578.

Comment by Mike Anderson [ 09/Jun/15 3:54 AM ]

This is still affecting me, and causing breakage with the latest versions of core.matrix. I don't know if this is a regression or not, but it certainly happens in 1.7.0-RC1

Any chance we can get a fix for 1.7? It is really annoying to have code fail because of this and force of refactoring of user code (my use case is adding a new var to clojure.core.matrix namespace, compiler error in user code that previously defined a var with the same name).

Comment by Mike Anderson [ 09/Jun/15 3:59 AM ]

Closing because I think this is better handled in the related issue

Comment by Mike Anderson [ 09/Jun/15 5:05 AM ]

Reopening because CLJ-1578 apparently does not resolve this specific issue, it only covers vars in clojure.core.

I'd still like to see this fixed for all namespaces, not just clojure.core.

Comment by Mike Anderson [ 09/Jun/15 5:08 AM ]

Reproduction:

=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:1:1)
=> clojure-version
{:major 1, :minor 7, :incremental 0, :qualifier "RC1"}

Stack trace:

CompilerException java.lang.RuntimeException: Unable to resolve symbol: pst in this context, compiling:(NO_SOURCE_PATH:1:1)
clojure.lang.Compiler.analyze (Compiler.java:6543)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3737)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6735)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5861)
clojure.lang.Compiler$FnMethod.parse (Compiler.java:5296)
clojure.lang.Compiler$FnExpr.parse (Compiler.java:3925)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6731)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.eval (Compiler.java:6789)
Caused by:
RuntimeException Unable to resolve symbol: pst in this context
clojure.lang.Util.runtimeException (Util.java:221)
clojure.lang.Compiler.resolveIn (Compiler.java:7029)
clojure.lang.Compiler.resolve (Compiler.java:6973)
clojure.lang.Compiler.analyzeSymbol (Compiler.java:6934)
clojure.lang.Compiler.analyze (Compiler.java:6506)
clojure.lang.Compiler.analyze (Compiler.java:6485)

Comment by Nicola Mometto [ 09/Jun/15 5:16 AM ]

As I already commented in CLJ-1578, I don't think this is a bug and I think this ticket should be declined.

Overriding non clojure.core vars has always (since 1.2 at least) caused an exception to be thrown.

Comment by Nicola Mometto [ 09/Jun/15 5:23 AM ]

Mike, maybe it would make sense to bring this issue up in the clojure-dev ml to get some opinions?

Comment by Mike Anderson [ 09/Jun/15 5:42 AM ]

Re-classify it as a feature request, if you prefer.

I still regard it as a defect because I expect :refer :all to work sanely.

Either way, this issue keeps breaking user code in my area (data science / exploratory statistics / data management). The ability to use / refer all is very useful for setting up a convenient namespace for exploratory work, so I don't accept that forcing users to explicitly require every single var used (as Nicola suggests in CLJ-1578) is a practical workaround.

I've also had it cause problems when working at the REPL and reloading namespaces.

If the Clojure core team really wants to keep this annoying behaviour, can we at least have some way to turn it off at the library level? Perhaps some namespace metadata that I can add to the clojure.core.matrix namespace to stop this from triggering?

Comment by Nicola Mometto [ 09/Jun/15 6:23 AM ]

Mike, this is just my personal opinion, I'm not a part of the core team and I don't speak for them, this is why I suggested you wrote on the clojure-dev ml.

Also to clarify, this issue you're reporting cannot manifest itself while reloading namespaces as the exception is thrown as soon as the redefinition happens.

Comment by Alex Miller [ 09/Jun/15 8:47 AM ]

You could use :exclude for this

(ns bar (:require [foo :refer :all :exclude (a)]))
Comment by Mike Anderson [ 09/Jun/15 9:30 AM ]

Hi Alex, that works as a fix when the problem occurs, but doesn't solve the problem of future user code breakage, unless the user accurately anticipates what symbols might get added to "bar" in the future. Which again seems like an unreasonable burden on the user.

What I'm arguing for, I guess, is a default presumption that if the user defines a var in their own namespace, they are happy to replace a similarly named var in any namespaces that they have previously use'd / refer-all'd.

If the user is genuinely concerned about overriding things by accident, I'd be happy with a warn-on-replace which does something analogous to warn-on-reflection. I proposed something similar in CLJ-1257 a while back, even wrote a patch that solves the whole problem in this way. Can we get that patch or something similar in 1.7?

Comment by Nicola Mometto [ 09/Jun/15 10:22 AM ]

CLJ-1746 is relevant and I like that proposal much better than CLJ-1257

Comment by Mike Anderson [ 09/Jun/15 10:43 AM ]

Hi Nicola, CLJ-1746 looks like a reasonable suggestion but still doesn't address the problem here, for the same reason that :exclude doesn't (see my comment to Alex)

The fundamental issue is that the current behaviour causes user code to break when libraries are upgraded to add new vars and requires changes to user code to fix / work around it. I consider that broken behaviour, or at least very bad design.

This is especially since we are encouraged to choose good names: I quote from the library coding standards(http://dev.clojure.org/display/community/Library+Coding+Standards)

"Use good names, and don't be afraid to collide with names in other namespaces. That's what the flexible namespace support is there for."

It isn't very flexible when user code keep breaking.....

Comment by Nicola Mometto [ 09/Jun/15 10:53 AM ]

From the same page, just a two lines below:
"Be explicit and minimalist about dependencies on other packages. (Prefer :require :refer in 1.4+ or :use :only in 1.0-1.3)."

If users carelessly import a whole package, I'd say that's their fault. Since clojure >1.3 the popular consensus has been that :use/:refer :all are not a good idea and people have been moving away from that, preferring :require :refer or :require :as instead.

The few that still use :use/:refer :all do it mostly for backward compatibility or in tests (I myself do it in tools.reader for the former reason).

I really don't think this is such a bad design choice as you seem to think, and I think that most people in the community would agree that the current behaviour and drive towards using :require :refer/require :as in lieu of :use/:refer :all is way more beneficial than harmful

Comment by Mike Anderson [ 09/Jun/15 11:42 AM ]

Hi Nicola, I have no issue with people using explicit :refer / :require, and I agree it is normal practice. It's good to be explicit and I generally do that myself (except in test / demo code).

However we aren't talking about that case : this issue is most relevant in those situations where users (for their own legitimate reasons) have decided to import a whole namespace. This is often convenient (e.g. REPL usage), sometimes it is valuable for specific purposes (e.g. testing).

I personally care about this mostly from the perspective of a library author: I want users to be able to use the library in whatever way is most convenient (which may include importing all vars), and I don't want user code to break randomly when I make a new release. Note that this also means I don't have control over user code so any solution that involves explicit requires, excludes, or some other manual workarounds is not a practical solution.

You can say "that's their fault" but this is currently supposed to be supported in Clojure so I think it ought to work sanely.

Comment by Alex Miller [ 09/Jun/15 12:00 PM ]

I think the library / evolution argument is a good one and I like that it reduces breakage due to evolution of 3rd party libraries. (I feel very strongly about this in clojure.core itself which is auto-referred, less strongly otherwise.)

On the flip side, if we remove this error, we should be explicit about what we are giving up to fully consider it. Presumably we are at least giving up a helpful error that occurs in the case of an accidental override. Are there other impacts?

I do not expect that we're going to change anything in 1.7.

Comment by Stuart Halloway [ 31/Jul/15 3:10 PM ]

Recategorizing as a feature request, as the current behavior was the original intent.

Could a namespace-reflecting macro provide for this use case without requiring a change to core?





[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 07/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

  • Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
  • the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
  • In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.
Comment by Scott Bale [ 11/Sep/14 11:19 PM ]

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

  • I used tabs in Compiler.java
  • After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
  • (Test is unchanged)
Comment by Scott Bale [ 11/Sep/14 11:22 PM ]

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.

Comment by Alex Miller [ 07/Oct/14 12:34 PM ]

I'll follow up on this patch later - Rich thought it was making too many assumptions. I think we validated many of those but need to double-check those.





[CLJ-1389] Re-loading a namespace ignores metadata specified for the namespace Created: 20/Mar/14  Updated: 20/Mar/14

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels:<