<< Back to previous view

[CLJ-1917] internal-reduce extended on StringSeq calls `.length` on every iteration step Created: 24/Apr/16  Updated: 25/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

n/a


Approval: Triaged

 Description   

internal-reduce extended on StringSeq calls `.length` (on the same String object) upon every iteration step [1]. There is absolutely no need for this as the length of a String cannot change. Therefore, it can be bound once (in the `let` a couple of lines up) and used thereafter.

[1]: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L151






[CLJ-1915] Tests for clojure.core/atom Created: 18/Apr/16  Updated: 18/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: atom

Attachments: File 0001-atom-unit-tests.clj    
Patch: Code and Test
Approval: Triaged

 Description   

As per discussion with Alex Miller Mars 3rd 2016 on clojure-dev, Alex suggested we should add tests to clojure.core/atom functionality, of which there is none today.

I proposed tests for

  • the various ways to instatiate atoms (with and without validator and metadata)
  • that validators throws correctly
  • adding and removing watchers and that they trig as one would expected.
  • various ways of changing values (no aim for finding high-load concurrency issues or patological cases or similar).
  • that the arities of the interface in IAtom .swap works as expected - ie no reflection warnings (help/pointers for these type of cases needed!)
  • generative, tests trying to find the glitches while using atoms (the things excluded above).

Alex suggested generative testing, but no performance tests.

The patch "0001-atom-unit-tests.clj" (attached) contains unit tests for

  • creating "bare" atom
  • creating atom with validator
  • that validate-fn triggers and that the atom is unchanged
  • that deref (@) reader macro creates correct '(clojure.core/deref a)
  • that CAS works for ordinary values (no validator-triggering etc).

There are plenty of combinations not covered with these tests, but this is a start.

To cover all cases (like cas-ing with invalid values and other strange things) generative testing is indeed a must.






[CLJ-1911] min-key and max-key should return NaN if any of the argument is NaN Created: 08/Apr/16  Updated: 29/Apr/16

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

Type: Defect Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None
Environment:

Likely All. Including older version of Clojure.


Attachments: Text File CLJ-1911-contagious-NaN-and-tests.patch     Text File CLJ-1911-contagious-NaN.patch    
Patch: Code
Approval: Triaged

 Description   

It appears that min-key and max-key behave incorrectly (following Java that follows IEEE floating point convention):

(apply max-key last [[:a 10000] [:b (/ 0. 0)] [:c 0]])
[:c 0]

Not sure how this should then propagate forward, but definitely not silently. Options:

1. [:b NaN] (the first item to generate the NaN)
2. NaN (this is changing the expected type)
3. ArithmeticException Operation with at least one NaN operand.

If this was to be patched the same as it was for min/max (http://dev.clojure.org/jira/browse/CLJ-868) it will probably result in option 1.



 Comments   
Comment by Nicholas Antonov [ 14/Apr/16 9:36 PM ]

This implements the first solution of a contagious NaN in the same style as CLJ 868

Comment by Alex Miller [ 15/Apr/16 12:03 AM ]

Patch should have tests...

Comment by Nicholas Antonov [ 15/Apr/16 1:07 AM ]

This latest patch adds tests for min-key and max-key with and without NaN results, as there were none before.

Comment by Alex Miller [ 29/Apr/16 10:06 AM ]

This overlaps with CLJ-99, which has already been prescreened. I would like to base whatever changes this patch requires over the top of that ticket. To build this, apply the CLJ-99 patch, then branch, make you changes, and then create a patch relative to the clj-99 branch. Sorry that's a pain - usually patches don't collide at this level of conflict.





[CLJ-1907] Document non-caching behaviour of `iterate` when used as generator Created: 31/Mar/16  Updated: 31/Mar/16

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: docstring

Approval: Triaged

 Description   

The non-caching behaviour of `iterate` when used as a generator is not documented and counter-intuitive. It should be documented, just like it's documented for e.g. `eduction`.

Even though the docstring for `iterate` requires `f` to be side-effect free, `f` might take a long time to compute, in which case users should be wary that the computation might happen more than once.






[CLJ-1903] Provide a transducer for reductions Created: 17/Mar/16  Updated: 18/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: transducers

Attachments: Text File 0001-clojure.core-add-reductions-stateful-transducer.patch     Text File 0002-clojure.core-add-reductions-with-for-init-passing-va.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Reductions does not currently provide a transducer when called with a 1-arity.

Proposed:

  • A reductions transducer
  • Similar to seequence reductions, initial state is not included in reductions
(assert (= (sequence (reductions +) nil) []))
(assert (= (sequence (reductions +) [1 2 3 4 5]) [1 3 6 10 15]))

A second patch proposes a variant which allows explicit initialization values: reductions-with

(assert (= (sequence (reductions-with + 0) [1 2 3 4 5]) [1 3 6 10 15])))

Patch: 0001-clojure.core-add-reductions-stateful-transducer.patch
Patch: 0002-clojure.core-add-reductions-with-for-init-passing-va.patch



 Comments   
Comment by Steve Miner [ 17/Mar/16 3:47 PM ]

The suggested patch gets the "init" value for the reductions by calling the function with no args. I would like a "reductions" transducer that took an explicit "init" rather than relying on a nullary (f).

If I remember correctly, Rich has expressed some regrets about supporting reduce without an init (ala Common Lisp). My understanding is that an explicit init is preferred for new Clojure code.

Unfortunately, an explicit init arg for the transducer would conflict with the standard "no-init" reductions [f coll]. In my own code, I've used the name "accumulations" for this transducer. Another possible name might be "reductions-with".

Comment by Pierre-Yves Ritschard [ 17/Mar/16 4:38 PM ]

Hi Steve,

I'd much prefer for init values to be explicit as well, unfortunately, short of testing the 2nd argument in the 2-arity variant - which would probably be even more confusing, there's no way to do that with plain "reductions".

I like the idea of providing a "reductions-with" variant that forced the init value and I'm happy to augment the patch with that if needed.

Comment by Pierre-Yves Ritschard [ 18/Mar/16 3:35 AM ]

@Steve Miner I added a variant with reductions-with.





[CLJ-1898] Inconsistent duplicate check in set/map literals with quoted/unquoted equal constants Created: 06/Mar/16  Updated: 06/Mar/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, compiler

Approval: Triaged

 Description   

Set and map literals containing the same constant quoted and unquoted, will throw a duplicate key exception in some cases (the correct behaviour), while silently ignore the duplicate in some others.

user=> #{'1 1}
#{1}
user=> #{'[] []}
IllegalArgumentException Duplicate key: []  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

This happens because the compiler assumes that literals that have distinct elements at read-time, will have distinct elements at runtime. This is not true for self-evaluating elements where (quote x) is equal to x






[CLJ-1896] Support transducers in vec and set fns Created: 24/Feb/16  Updated: 24/Feb/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transducers

Approval: Triaged

 Description   

Rather than

(into [] (map inc) [1 2 3])
vec (and set) could support the transducer directly:

(vec (map inc) [1 2 3])
(set (map inc) #{1 2 3})

Depending how far we wanted to take this, the implementation could be somewhat clever for vec in building the initial set of results in an array and then creating the vector with it directly as is already done in some other cases.






[CLJ-1889] Add optional predicate to string trim functions that determines if a character should be trimmed Created: 27/Jan/16  Updated: 28/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Tamas Szabo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: string

Attachments: Text File CLJ-1889-trim-enhancement.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The proposal is that the trim functions (trim, triml, and trimr) would get a second arity with a function trim?:

[trim? ^CharSequence s]

trim? comes first to support partial.

New doc string would be:

"Removes characters from both ends of string. 
 If trim? is omitted white space is removed. When supplied it accepts 
 a character and returns true if the character should be removed."

Example test:

(deftest t-trim 
  (is (= "foo" (s/trim "  foo  \r\n"))) 
  (is (= "bar" (s/trim "\u2000bar\t \u2002"))) 

  ;; Additional test 
  (is (= "bar" (s/trim "$%#\u2000bar\t \u2002%$#" 
                       #(or (Character/isWhitespace %) ((set "$#%") %))))))

Similar to Python's strip - https://docs.python.org/2/library/stdtypes.html#str.strip

Approach: The proposed solution isn't very DRY but it follows the design guidelines at the top of the file, more exactly point 3:

"3. Functions take advantage of String implementation details to
write high-performing loop/recurs instead of using higher-order
functions. (This is not idiomatic in general-purpose application
code.)"

First I had a solution in which I replaced Character/isWhitespace from the current implementation by calling pred. pred was defaulted to an is-whitespace? function.
That code is of course nicer, even trim-newline could just call into trimr, removing a lot of duplication, but it adds the overhead of always calling a function, instead of calling Character/isWhitespace directly.

The only way I can see to have optimised and DRYer code is to use macros, but I don't think that it will necessary lead to nicer code.

Given the existing design style of the other functions in string.clj I felt that the best solution would be to just simply duplicate in favour of optimised code.



 Comments   
Comment by Tamas Szabo [ 27/Jan/16 2:44 PM ]

Proposed solution. Code + tests.

Comment by Tamas Szabo [ 27/Jan/16 3:42 PM ]

Added new patch that renames pred to trim?

Comment by Andy Fingerhut [ 27/Jan/16 6:27 PM ]

Note that Java, and thus Clojure/Java, uses UTF-16 encoding for strings in memory. Thus if you wanted to trim a set of Unicode code points from the beginning and/or end of a string, the API of trim? taking a single 16-bit Java character is not enough information to determine whether it should be trimmed or not.

If you want to handle that generality, it would require a more complex implementation, which checks whether the first/last character is one half of a code point that is encoded as 2 16-bit Java characters, and pass a 32-bit int to trim?, or something similar to that.

I have no objections if these API enhancements are made without enabling testing against an arbitrary Unicode code point. In the past, similar suggestions have been rejected in Clojure's built-in lib, e.g. CLJ-945

Comment by Tamas Szabo [ 28/Jan/16 1:56 AM ]

Yes, the UTF-16 encoding and Character representing either a codepoint or a half-codepoint is a bit of a mess, isn't it?

In the Java String and Character API's the methods that accept char, handle only characters in the Basic Multilingual Plane.
trim? accepts a character, so following the same behavior it will work only for removing characters in the Basic Multilingual Plane.

I think even this would be fine, but additionally because the high/low surrogates and the BMP characters are disjoint, you could actually use the same implementation to remove Unicode code points that aren't in the BMP. You can just say that both the high and low code unit of the codepoint are "unwanted".

Ex:
𝄞 is "\uD834\uDD1E"

user=> (trimr (set " \uD834\uDD1E") "example string  𝄞  ")
"example string"
Comment by Andy Fingerhut [ 28/Jan/16 5:11 AM ]

Agreed, but probably better to anti-recommend such an implementation of trimr for removing such things, because it would also remove only one UTF-16 Java character out of 2 high/low surrogates if it matched a member of the set, even if the other surrogate didn't match anything in the set, which would leave behind a malformed UTF-16 string.

Again, probably best to either not include this in the implementation at all, and at most warn about it in the docs, or to handle it in the implementation by checking for high/low surrogates in the loop(s).

Comment by Tamas Szabo [ 28/Jan/16 6:00 AM ]

Yes, you're right. That solution won't work in all cases, so it can't be recommended.

I am slightly inclined towards having trim? accept chars and work only for removing BMP characters. This will arguably be enough for the majority of the use cases.
The other solution can be used for all use cases, but then trim? will have to accept int, or 2 chars, or a string, so trim? would be less intuitive (although closer to the real world ), and writing those trim? functions would be less user friendly.

That being said, I am happy to change the implementation to do that if it is required.

Currently, I'm not even sure if the enhancement will be accepted or rejected or what the process for that is.





[CLJ-1885] data/diff does not return a tuple when comparing different maps Created: 16/Jan/16  Updated: 16/Jan/16

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

Type: Defect Priority: Minor
Reporter: Eric Dvorsak Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

all


Attachments: Text File CLJ-1885.patch     Text File CLJ-1885-tests.patch    
Approval: Triaged

 Description   

Problem: clojure.data/diff inconsistently returns a lazy seq when comparing different maps, but a vector otherwise.

user> (data/diff {:a 1 :b 2} {:a 1})
({:b 2} nil {:a 1})

This is inconsistent with doc and normal behavior :

user> (data/diff {:a 1 :b 2} {:a 1 :b 2})
[nil nil {:a 1, :b 2}]
user> (data/diff #{1 2 3} #{1 2 3})
[nil nil #{1 3 2}]
user> (data/diff #{1 2 3} #{1 2})
[#{3} nil #{1 2}]

The docstring states: "Recursively compares a and b, returning a tuple of [things-only-in-a things-only-in-b things-in-both]", implying that it should always return a vector.



 Comments   
Comment by Eric Dvorsak [ 16/Jan/16 10:02 AM ]

Fixing it just requires to vectorize diff-associative output like this :

(defn- diff-associative
  "Diff associative things a and b, comparing only keys in ks."
  [a b ks]
  (vec (reduce
   (fn [diff1 diff2]
     (doall (map merge diff1 diff2)))
   [nil nil nil]
   (map
    (partial diff-associative-key a b)
    ks))))
Comment by Alex Miller [ 16/Jan/16 10:10 AM ]

There are other potential ways to address this, such as by using transducers instead. Not sure if that's worth doing, but seems reasonable to consider while we're making changes.

Comment by Eric Dvorsak [ 16/Jan/16 10:15 AM ]

Maybe this could be done as an improvement and proposed in an other ticket.

Vec is already used to vectorize the lists in diff-sequential. I would suggest to just fix the bug and add the test cases that should have screen it.

Comment by Eric Dvorsak [ 16/Jan/16 10:20 AM ]

There is a test case that should already fail :

[{:a #{2}} {:a #{4}} {:a #{3}}] {:a #{2 3}} {:a #{3 4}}

I get

({:a #{2}} {:a #{4}} {:a #{3}})
Comment by Alex Miller [ 16/Jan/16 10:33 AM ]

The test may need to be made more strict, checking not just for sequential equality but also for a returned vector.

Just curious - was this issue causing a problem in your code or did you just notice it and find it surprising?

Comment by Eric Dvorsak [ 16/Jan/16 11:05 AM ]

Simple patch that just does for maps what is done for lists : Creates a new vector with the vec function.

Comment by Eric Dvorsak [ 16/Jan/16 11:08 AM ]

@Alex Miller : I noticed a bug in my program behavior and traced it down to a (get diff 2) instead of (nth diff 2), but I realized that it was only buggy in some cases so I looked further and found out if was coming from diff.

Comment by Eric Dvorsak [ 16/Jan/16 11:27 AM ]

More strict tests checking for a returned vector.





[CLJ-1881] Can :or destructuring refer to previous sequential bindings? Created: 11/Jan/16  Updated: 11/Jan/16

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: destructuring, docs

Approval: Triaged

 Description   

The following code works, but it is unspecified in the docs whether `(inc a)` can rely on `a` being bound.

user=> (defn foo [a {:keys [b] :or {b (inc a)}}]
  [a b])
user=> (foo 1 {:b 99})
[1 99] ;; :or not needed
user=> (foo 1 {})
[1 2]  ;; :or binds b to (inc a)

In sequential destructuring, are bindings bound in order such that subsequent :or value expressions can rely on prior sequential bindings?

This is true based on the current implementation of destructure, but looking for a statement to this effect in the docs and/or tests.






[CLJ-1880] IKVReduce impl for records Created: 09/Jan/16  Updated: 11/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

Attachments: Text File CLJ-1880.patch    
Approval: Triaged

 Description   

Records don't implement IKVReduce, which could help with efficient merging (CLJ-1458)



 Comments   
Comment by Ghadi Shayban [ 11/Jan/16 2:49 PM ]

simple implementation attached





[CLJ-1879] reduce-kv on a PHMs doesn't consistently execute the intended fastpath Created: 09/Jan/16  Updated: 11/Jan/16

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

Type: Defect Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File CLJ-1879.patch    
Approval: Triaged

 Description   

https://github.com/clojure/clojure/blob/010864f/src/clj/clojure/core.clj#L6553-L6562

Because PHMs implement clojure.lang.IKVReduce and IPersistentMap, they have nondeterministic dispatch through the protocol that backs reduce-kv (clojure.core.protocols/IKVReduce).

A potential way to solve this is to add an instance check for clojure.lang.IKVReduce inside `reduce-kv` (This is similar to how reduce checks for IReduceInit)



 Comments   
Comment by Nicola Mometto [ 11/Jan/16 9:23 AM ]

CLJ-1807 offers a generic solution for this class of problems





[CLJ-1875] Parameter names in docstring for `into` Created: 06/Jan/16  Updated: 20/Jan/16

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

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

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

 Description   

The docstring for into does not have the correct names for the parameters. The parameter names in the arglist are to, from and xform, but the doctring refers to them as to-coll from-coll, and the docstring does not refer to the optional transducer, xform by name.

Approach: update docstring to reflect param names
Patch: CLJ-1875.patch
Screened by:



 Comments   
Comment by Alex Miller [ 20/Jan/16 3:58 PM ]

The additional phrase at the end doesn't make sense to me. How about instead:

"A transducer, xform, may be supplied as an optional second argument."





[CLJ-1872] empty? is broken for transient collections Created: 26/Dec/15  Updated: 12/Jan/16

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

Type: Defect Priority: Critical
Reporter: Leonid Bogdanov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Triaged

 Description   

Couldn't find whether it was brought up earlier, but it seems that empty? predicate is broken for transient collections

user=> (empty? (transient []))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient ()))
ClassCastException clojure.lang.PersistentList$EmptyList cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:3209)

user=> (empty? (transient {}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient #{}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.seqFrom (RT.java:528)

The workaround is to use (zero? (count (transient ...))) check instead.



 Comments   
Comment by Alex Miller [ 26/Dec/15 9:58 PM ]

Probably similar to CLJ-700.





[CLJ-1867] with-redefs used on a macro permanently changes it to a function Created: 10/Dec/15  Updated: 10/Dec/15

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

Type: Defect Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

If you use with-redefs to redefine a macro (which is likely a mistake), the macro loses its macro status after the with-redefs call completes.

Presumably the fix depends on whether we think there is a valid use of with-redefs on a macro (which would only work if you're calling eval or equivalent in the body, and would require knowing enough about what you're doing to add the two extra macro args to your function) – if so, we would keep it from losing the macro status; if not, we might also have it throw an exception if you accidentally use it on a macro.

Demonstration of the effect:

user> (defmacro kwote [arg] `(quote ~arg))
#'user/kwote
user> (kwote hello)
hello
user> kwote
CompilerException java.lang.RuntimeException: Can't take value of a macro: #'user/kwote, compiling:(/tmp/form-init6222001939841513290.clj:1:18983)

;; Everything above is as expected

user> (with-redefs [kwote (constantly :in-with-redefs)] (kwote with-redefs-body))
with-redefs-body
user> (kwote hello)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: hello in this context, compiling:(/tmp/form-init6222001939841513290.clj:1:1) 
user> (kwote :arg-1)
ArityException Wrong number of args (1) passed to: user/kwote  clojure.lang.AFn.throwArity (AFn.java:429)
user> (kwote :arg-1 :arg-2 :arg-3)
(quote :arg-3)
user> kwote
#object[user$kwote 0x37e32ff6 "user$kwote@37e32ff6"]


 Comments   
Comment by Gary Fredericks [ 10/Dec/15 12:04 PM ]

Looks like the root cause is that with-redefs uses Var#bindRoot which intentionally clears the macro flag: https://github.com/clojure/clojure/blob/5cfe5111ccb5afec4f9c73b46bba29ecab6a5899/src/jvm/clojure/lang/Var.java#L270





[CLJ-1864] clojure.core/proxy does not work when reloading namespaces Created: 06/Dec/15  Updated: 08/Dec/15

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

Type: Defect Priority: Major
Reporter: Ralf Schmitt Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: protocols, proxy
Environment:

tested on 64 bit linux, oracle jdk 1.8


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

 Description   

clojure.core/proxy does not work when one reloads namespace containing defprotocol.

E.g. one can't reload the following file without triggering an error:

(ns foo.baz)

(defprotocol Hello
  (hello [this]))

(def hello-proxy
  (proxy [foo.baz.Hello] []
    (hello []
      (println "hello world"))))

(hello hello-proxy)

Saving the above as foo/baz.clj, I get the following error:

$ rlwrap java -cp target/clojure-1.8.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.8.0-master-SNAPSHOT
user=> (require 'foo.baz :reload)
hello world
nil
user=> (require 'foo.baz :reload)
CompilerException java.lang.IllegalArgumentException: No implementation of method: :hello of protocol: #'foo.baz/Hello found for class: foo.baz.proxy$java.lang.Object$Hello$6f95b989, compiling:(foo/baz.clj:11:1) 

I'm using the current git master (commit 5cfe5111ccb5afec4f9c73), but clojure 1.7 has the same problem.

The problem is that proxy-name only uses the interface names as a key. These names do not change when reloading the namespace, but the interfaces themself are new.

I'm going to attach a short patch which fixes that issue for me.



 Comments   
Comment by Ralf Schmitt [ 06/Dec/15 11:45 AM ]

I'm not sure how this interacts with AOT compilation.





[CLJ-1859] Update parameter name to reflect docstring Created: 30/Nov/15  Updated: 30/Nov/15

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

Type: Enhancement Priority: Trivial
Reporter: Matthew Boston Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJ-1859-Update-parameter-name-to-reflect-docstring.patch    
Approval: Triaged

 Description   

The docstrings for `zero?`, `pos?`, and `neg?` reference `num` but the parameter is named `x`. This issue is to update the name of the parameter to `num` to reflect the docstring.



 Comments   
Comment by Alex Miller [ 30/Nov/15 1:14 PM ]

The inline fns should be updated too.

Comment by Matthew Boston [ 30/Nov/15 1:22 PM ]

Thanks, Alex. I was trying to follow the existing pattern that the inline functions have shorter parameter names. New patch attached.





[CLJ-1843] Add =to function exposing Util/equivPred Created: 06/Nov/15  Updated: 18/Dec/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: 1
Labels: None

Attachments: Text File 0001-CLJ-1843-add-to-for-faster-equality-check-against-kn.patch    
Patch: Code
Approval: Triaged

 Description   

Description:
It is sometimes useful to compare a collection of values against one value, clojure internally defines a predicate for this exact purpose which has some nice performance improvements over just a partial applied =.

Prior discussion with Rich: https://groups.google.com/forum/#!topic/clojure-dev/0c-VNhEKVkI

Example usage:

;;before:
(map (partial = 3) coll)
;;after:
(map (=to 3) coll)

Benchmarks:

test (partial = 'foo) #(= 'foo %) (=to 'foo)
small homogeneous coll 217ns 165ns 39ns
small eterogeneous coll, 192ns 167ns 41ns
big homogeneous coll 66us 52us 8us
big eterogeneous coll 82us 66us 27us

Full benchmarks output:

(use 'criterium.core)

(defn benchmark-f [f]
  (let [colls [['foo 'foo 'foo]
               [1 :foo 'foo]
               (doall (repeat 1e3 'foo))
               (doall (take 1e3 (cycle [1 :foo 'foo])))]]
    (doseq [c colls]
      (quick-bench (run! f c)))))

(benchmark-f (partial = 'foo))
ARNING: Final GC required 1.405293826432628 % of runtime
WARNING: Final GC required 10.202923149112559 % of runtime
Evaluation count : 3116130 in 6 samples of 519355 calls.
Execution time mean : 217.723199 ns
Execution time std-deviation : 29.425291 ns
Execution time lower quantile : 189.944710 ns ( 2.5%)
Execution time upper quantile : 261.717351 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 4.2579397621583315 % of runtime
Evaluation count : 3138636 in 6 samples of 523106 calls.
Execution time mean : 198.985418 ns
Execution time std-deviation : 12.691848 ns
Execution time lower quantile : 182.441245 ns ( 2.5%)
Execution time upper quantile : 207.839280 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 6.631646134523004 % of runtime
Evaluation count : 10038 in 6 samples of 1673 calls.
Execution time mean : 66.977712 µs
Execution time std-deviation : 10.411821 µs
Execution time lower quantile : 59.620690 µs ( 2.5%)
Execution time upper quantile : 84.483254 µs (97.5%)
Overhead used : 1.863362 ns

Found 1 outliers in 6 samples (16.6667 %)
low-severe  1 (16.6667 %)
Variance from outliers : 47.3059 % Variance is moderately inflated by outliers
WARNING: Final GC required 5.272721959665122 % of runtime
Evaluation count : 7908 in 6 samples of 1318 calls.
Execution time mean : 82.588512 µs
Execution time std-deviation : 5.215537 µs
Execution time lower quantile : 75.977936 µs ( 2.5%)
Execution time upper quantile : 86.849982 µs (97.5%)
Overhead used : 1.863362 ns


(benchmark-f #(= 'foo %))
WARNING: Final GC required 1.284421364203217 % of runtime
WARNING: Final GC required 10.04376144830405 % of runtime
Evaluation count : 3643032 in 6 samples of 607172 calls.
             Execution time mean : 165.393131 ns
    Execution time std-deviation : 1.041355 ns
   Execution time lower quantile : 164.277060 ns ( 2.5%)
   Execution time upper quantile : 166.849951 ns (97.5%)
                   Overhead used : 1.605524 ns
WARNING: Final GC required 6.258680973295933 % of runtime
Evaluation count : 3584574 in 6 samples of 597429 calls.
             Execution time mean : 167.659014 ns
    Execution time std-deviation : 3.821817 ns
   Execution time lower quantile : 164.175156 ns ( 2.5%)
   Execution time upper quantile : 173.210781 ns (97.5%)
                   Overhead used : 1.605524 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
WARNING: Final GC required 6.914389197005716 % of runtime
Evaluation count : 11196 in 6 samples of 1866 calls.
             Execution time mean : 52.593759 µs
    Execution time std-deviation : 834.220092 ns



(benchmark-f (=to 'foo))
WARNING: Final GC required 7.40391654943877 % of runtime
Evaluation count : 15169068 in 6 samples of 2528178 calls.
Execution time mean : 39.937424 ns
Execution time std-deviation : 2.782661 ns
Execution time lower quantile : 37.393937 ns ( 2.5%)
Execution time upper quantile : 42.780432 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.986859953402835 % of runtime
Evaluation count : 15199992 in 6 samples of 2533332 calls.
Execution time mean : 41.229082 ns
Execution time std-deviation : 2.815533 ns
Execution time lower quantile : 37.371527 ns ( 2.5%)
Execution time upper quantile : 43.208673 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.039484046472016 % of runtime
Evaluation count : 69462 in 6 samples of 11577 calls.
Execution time mean : 8.976972 µs
Execution time std-deviation : 587.089991 ns
Execution time lower quantile : 8.505317 µs ( 2.5%)
Execution time upper quantile : 9.744296 µs (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.773010947849351 % of runtime
Evaluation count : 23352 in 6 samples of 3892 calls.
Execution time mean : 27.277376 µs
Execution time std-deviation : 2.115666 µs
Execution time lower quantile : 25.719322 µs ( 2.5%)
Execution time upper quantile : 30.123547 µs (97.5%)
Overhead used : 1.863362 ns

Patch: 0001-CLJ-1843-add-to-for-faster-equality-check-against-kn.patch



 Comments   
Comment by Alex Miller [ 06/Nov/15 9:15 AM ]

Did you look at (apply = 3 coll) ? Just curious.

Comment by Nicola Mometto [ 06/Nov/15 9:19 AM ]

The advantage of Util/equivPred over Util/equiv is that it can assume the type of the provided argument, avoiding the runtime cost of doing the multiple instance checks that Util/equiv has to do to figure out what comparator to use internally

Comment by Alex Miller [ 06/Nov/15 10:08 AM ]

Could you quantify the difference between these approaches on 2-3 collection sizes?

Comment by Nicola Mometto [ 06/Nov/15 2:53 PM ]

With the following setup:

(use 'criterium.core)

(defn =to [x]
  (let [pred (clojure.lang.Util/equivPred x)]
    (fn [y]
      (.equiv pred x y))))

(defn benchmark-f [f]
  (let [colls [['foo 'foo 'foo]
               [1 :foo 'foo]
               (doall (repeat 1e3 'foo))
               (doall (take 1e3 (cycle [1 :foo 'foo])))]]
    (doseq [c colls]
      (quick-bench (run! f c)))))

The results for (benchmark-f (partial = 'foo) are:

WARNING: Final GC required 1.405293826432628 % of runtime
WARNING: Final GC required 10.202923149112559 % of runtime
Evaluation count : 3116130 in 6 samples of 519355 calls.
Execution time mean : 217.723199 ns
Execution time std-deviation : 29.425291 ns
Execution time lower quantile : 189.944710 ns ( 2.5%)
Execution time upper quantile : 261.717351 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 4.2579397621583315 % of runtime
Evaluation count : 3138636 in 6 samples of 523106 calls.
Execution time mean : 198.985418 ns
Execution time std-deviation : 12.691848 ns
Execution time lower quantile : 182.441245 ns ( 2.5%)
Execution time upper quantile : 207.839280 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 6.631646134523004 % of runtime
Evaluation count : 10038 in 6 samples of 1673 calls.
Execution time mean : 66.977712 µs
Execution time std-deviation : 10.411821 µs
Execution time lower quantile : 59.620690 µs ( 2.5%)
Execution time upper quantile : 84.483254 µs (97.5%)
Overhead used : 1.863362 ns

Found 1 outliers in 6 samples (16.6667 %)
low-severe  1 (16.6667 %)
Variance from outliers : 47.3059 % Variance is moderately inflated by outliers
WARNING: Final GC required 5.272721959665122 % of runtime
Evaluation count : 7908 in 6 samples of 1318 calls.
Execution time mean : 82.588512 µs
Execution time std-deviation : 5.215537 µs
Execution time lower quantile : 75.977936 µs ( 2.5%)
Execution time upper quantile : 86.849982 µs (97.5%)
Overhead used : 1.863362 ns

The results fore (benchmark-f (=to 'foo)) are:

WARNING: Final GC required 7.40391654943877 % of runtime
Evaluation count : 15169068 in 6 samples of 2528178 calls.
Execution time mean : 39.937424 ns
Execution time std-deviation : 2.782661 ns
Execution time lower quantile : 37.393937 ns ( 2.5%)
Execution time upper quantile : 42.780432 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.986859953402835 % of runtime
Evaluation count : 15199992 in 6 samples of 2533332 calls.
Execution time mean : 41.229082 ns
Execution time std-deviation : 2.815533 ns
Execution time lower quantile : 37.371527 ns ( 2.5%)
Execution time upper quantile : 43.208673 ns (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.039484046472016 % of runtime
Evaluation count : 69462 in 6 samples of 11577 calls.
Execution time mean : 8.976972 µs
Execution time std-deviation : 587.089991 ns
Execution time lower quantile : 8.505317 µs ( 2.5%)
Execution time upper quantile : 9.744296 µs (97.5%)
Overhead used : 1.863362 ns
WARNING: Final GC required 5.773010947849351 % of runtime
Evaluation count : 23352 in 6 samples of 3892 calls.
Execution time mean : 27.277376 µs
Execution time std-deviation : 2.115666 µs
Execution time lower quantile : 25.719322 µs ( 2.5%)
Execution time upper quantile : 30.123547 µs (97.5%)
Overhead used : 1.863362 ns
Comment by Nicola Mometto [ 06/Nov/15 3:07 PM ]

Using #(= 'foo %) rather than (partial = 'foo) allows for inlining of = and makes performance a bit better, but =to still wins noticeably

WARNING: Final GC required 1.284421364203217 % of runtime
WARNING: Final GC required 10.04376144830405 % of runtime
Evaluation count : 3643032 in 6 samples of 607172 calls.
             Execution time mean : 165.393131 ns
    Execution time std-deviation : 1.041355 ns
   Execution time lower quantile : 164.277060 ns ( 2.5%)
   Execution time upper quantile : 166.849951 ns (97.5%)
                   Overhead used : 1.605524 ns
WARNING: Final GC required 6.258680973295933 % of runtime
Evaluation count : 3584574 in 6 samples of 597429 calls.
             Execution time mean : 167.659014 ns
    Execution time std-deviation : 3.821817 ns
   Execution time lower quantile : 164.175156 ns ( 2.5%)
   Execution time upper quantile : 173.210781 ns (97.5%)
                   Overhead used : 1.605524 ns

Found 1 outliers in 6 samples (16.6667 %)
	low-severe	 1 (16.6667 %)
 Variance from outliers : 13.8889 % Variance is moderately inflated by outliers
WARNING: Final GC required 6.914389197005716 % of runtime
Evaluation count : 11196 in 6 samples of 1866 calls.
             Execution time mean : 52.593759 µs
    Execution time std-deviation : 834.220092 ns
   Execution time lower quantile : 51.510161 µs ( 2.5%)
   Execution time upper quantile : 53.367649 µs (97.5%)
                   Overhead used : 1.605524 ns
WARNING: Final GC required 6.179040224498723 % of runtime
Evaluation count : 9162 in 6 samples of 1527 calls.
             Execution time mean : 66.527357 µs
    Execution time std-deviation : 2.119652 µs
   Execution time lower quantile : 65.308835 µs ( 2.5%)
   Execution time upper quantile : 70.201570 µs (97.5%)
                   Overhead used : 1.605524 ns

small homogeneous coll, (partial = 'foo): 217ns, #(= 'foo %): 165ns, (=to 'foo): 39ns
small eterogeneous coll, (partial = 'foo): 192ns, #(= 'foo %): 167ns, (=to 'foo): 41ns
big homogeneous coll, (partial = 'foo): 66us, #(= 'foo %): 52us, (=to 'foo): 8us
big eterogeneous coll, (partial = 'foo: 82us, #(= 'foo %): 66us, (=to 'foo): 27us

Comment by Nicola Mometto [ 17/Dec/15 5:13 PM ]

Apparently this was something that was already discussed a couple of years ago and Rich seemed ok with this https://groups.google.com/forum/#!topic/clojure-dev/0c-VNhEKVkI





[CLJ-1836] Expose clojure.repl/doc as a function call Created: 28/Oct/15  Updated: 21/Nov/15

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

Type: Enhancement Priority: Minor
Reporter: Terje Dahl Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File doc-fn-1.patch    
Patch: Code
Approval: Triaged

 Description   

Restructure the printing function clojure.repl/doc so that it calls a fuction clojure.repl/doc-fn for its data - in the same way as dir calls dir-fn. Make doc-fn public so that it can be called directly and allow developers to parse and display the data as needed.

Use case: I am making a namespace inspector (using JavaFX) (somewhat like the Swing-based tree-inspector in Clojure), and when getting a function, I would like to display the same meta-information as "doc" prints in the REPL - including the special forms data coded in a private var/map in Clojure.

Patch: doc-fn.patch



 Comments   
Comment by Alex Miller [ 28/Oct/15 12:34 PM ]

A few comments:

1) Patch authors must sign the contributor's agreement, see http://clojure.org/contributing

2) The patch is not in the correct format - see http://dev.clojure.org/display/community/Developing+Patches for more info.

3) Patch should include a test for the new doc-fn.

Comment by Terje Dahl [ 21/Nov/15 6:29 AM ]

1. Agreement signed.

2. Tests added.
(That was useful! I had to fix a couple of things.)

3. Patch created as per instructions and attached:
"doc-fn-1.patch"





[CLJ-1818] cl-format does not respect aesthetic ~A with a keyword Created: 26/Sep/15  Updated: 12/Jan/16

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

Type: Defect Priority: Trivial
Reporter: Jong-won Choi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Approval: Triaged

 Description   

In Common Lisp, (format nil "~a" :A) returns "A". However, in Clojure, it returns with the colon:

(clojure.pprint/cl-format false "~a" :A)
=> ":A"


 Comments   
Comment by Jong-won Choi [ 28/Sep/15 6:26 AM ]

Found another problem of cl-format:

(clojure.pprint/cl-format false "SELECT * from RateSchedules ~@[WHERE ~{~A=?~^ ~}~]" '())
=> "SELECT * from RateSchedules WHERE" ;; instead of "SELECT * from RateSchedules"

I guess the problem is () or [] has to be treated as falsey but not.

Comment by Alex Miller [ 28/Sep/15 9:58 AM ]

:a is a keyword and I would expect it's ascii format to be :a. I'm not sure what case sensitivity has to do with it.

Comment by Andy Fingerhut [ 28/Sep/15 10:08 AM ]

Alex, case is a side issue. Common Lisp's (format nil "~a" :A) returns "A", not ":A". It is the presence of the colon in the output that is the issue, not the case of the string.

Comment by Jong-won Choi [ 28/Sep/15 4:41 PM ]

For a record, what Alex described is for ~S - standard. See http://www.lispworks.com/documentation/lw50/CLHS/Body/22_cd.htm





[CLJ-1817] Allow AssertionError messages for function :pre and :post conditions to be specified. Created: 23/Sep/15  Updated: 03/Apr/16

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

Type: Enhancement Priority: Major
Reporter: Tristan Strange Assignee: Colin Taylor
Resolution: Unresolved Votes: 4
Labels: error-reporting
Environment:

All Clojure platforms


Attachments: Text File CLJ-1817.patch    
Patch: Code and Test
Approval: Triaged

 Description   

A failing in a predicate in a list of :pre or :post conditions currently causes messages similar to one below to be displayed:

(defn must-be-a-map [m] {:pre [(map? m)]} m)
(must-be-a-map [])
;;=> AssertionError Assert failed: (map? m)  user/must-be-a-map (form-init.....clj:1)

These exception messages could be made significantly more descriptive by allowing specific messages strings to be associated with each predicate in :pre and :post conditions.

Predicate functions and there associated messages strings could be specified as a pair of values in a map:

(defn must-be-a-map 
  [m]
  {:pre [{(map? m) "m must be a map due to some domain specific reason."}]}
  m)

The following would then produce an error message as follows:

(must-be-a-map 10)
AssertionError Assert failed: m must be a map due to some domain specific reason.
(map? m) user/must-be-a-map (form-init.....clj:1)

This would allow predicates without messages to specified alongside pairs of associated predicate message pairs as follows:

(defn n-and-m [n m] {:pre [(number? n) {(map? m) "You must provide a map!"}]})

This change would not break any existing functionality and still allow for predicates to be predefined elsewhere in code.

As a result pre and post conditions could provide a natural means of further documenting the ins and outs of a function, simplify the process of providing meaningful output when developing libraries and perhaps make the language better suited to teaching environments[1]

[1] http://wiki.science.ru.nl/tfpie/images/2/22/TFPIE2013_Steps_Towards_Teaching_Clojure.pdf



 Comments   
Comment by Colin Taylor [ 03/Apr/16 5:26 PM ]

Attached approach differs from that advocated for in the description by not requiring a map. The existing spec of :

{:pre [pre-expr*]
 :post [post-expr*]}

in effect becoming :

{:pre [(pre-expr assert-msg?)*]
 :post [(pre-expr assert-msg?)*]}

where assert-msg is a String. Note this means a (presumably erroneous) second String after an expression would be treated as a truthy pre-expr.

Contrived example :

(defn print-if-alphas-and-nums [arg] {:pre [(hasAlpha arg) "No alphas"
                                            (hasNum arg) "No numbers"
                                            (canPrint arg)]}
  (println arg))

user=> (print-if-alphas-and-nums "a5%")
a5%
nil
user=> (print-if-alphas-and-nums "$$%")
AssertionError Assert failed: No alphas
(hasAlpha arg)  user/print-if-alphas-and-nums (NO_SOURCE_FILE:19)

I have considered extending the spec further to (pre-expr assert-msg? data-map)* perhaps supported by ex-info, ex-data analogues in assert-info, assert-data to convey diagnostic info (locals?). A map could contain a :msg key or perhaps the map is additional to the message string. I thought I'd wait for input though at this point.

I also considered allowing % substitution for the fn return value in the message as in :post conds, but how to escape?

Comment by Colin Taylor [ 03/Apr/16 6:17 PM ]

I should point out that the tests include the currently uncovered existing functionality too.





[CLJ-1814] Make `satisfies?` as fast as a protocol method call Created: 11/Sep/15  Updated: 02/Nov/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: performance, protocols

Attachments: Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v2.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v3.patch    
Patch: Code
Approval: Triaged

 Description   

Currently `satisfies?` doesn't use the same impl cache used by protocol methods, making it too slow for real world usage.

With:

user=> (defprotocol p (f [_]))
p
user=> (deftype x [])
user.x
user=> (deftype y [])
user.y
user=> (extend-type x p (f [_]))
nil

Before patch:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 548182380 in 60 samples of 9136373 calls.
             Execution time mean : 108.856460 ns
    Execution time std-deviation : 4.151711 ns
   Execution time lower quantile : 103.306368 ns ( 2.5%)
   Execution time upper quantile : 117.597299 ns (97.5%)
                   Overhead used : 1.681820 ns
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 20220420 in 60 samples of 337007 calls.
             Execution time mean : 3.325396 µs
    Execution time std-deviation : 277.917798 ns
   Execution time lower quantile : 3.035664 µs ( 2.5%)
   Execution time upper quantile : 3.915870 µs (97.5%)
                   Overhead used : 1.681820 ns
nil

After patch:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 3091276560 in 60 samples of 51521276 calls.
             Execution time mean : 19.048289 ns
    Execution time std-deviation : 0.724232 ns
   Execution time lower quantile : 17.558597 ns ( 2.5%)
   Execution time upper quantile : 20.067082 ns (97.5%)
                   Overhead used : 1.639685 ns
niluser=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 2699888040 in 60 samples of 44998134 calls.
             Execution time mean : 20.968108 ns
    Execution time std-deviation : 0.658803 ns
   Execution time lower quantile : 20.336564 ns ( 2.5%)
   Execution time upper quantile : 22.508062 ns (97.5%)
                   Overhead used : 1.639685 ns
nil

Patch: 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v3.patch



 Comments   
Comment by Michael Blume [ 11/Sep/15 4:17 PM ]

Nice. Honeysql used to spend 80-90% of its time in satisfies? calls before we refactored them out.

Comment by Michael Blume [ 24/Sep/15 3:55 PM ]

I realize this is a deeply annoying bug to reproduce, but if I clone core.match, point its Clojure dependency to 1.8.0-master-SNAPSHOT, start a REPL, connect to the REPL from vim, and reload clojure.core.match, I get

|| java.lang.Exception: namespace 'clojure.tools.analyzer.jvm.utils' not found, compiling:(clojure/tools/analyzer/jvm.clj:9:1)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5647| clojure.core$throw_if.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5733| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:703)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.analyzer.jvm/0.6.5/tools.analyzer.jvm-0.6.5.jar::clojure/tools/analyzer/jvm.clj|9| clojure.tools.analyzer.jvm$eval4968$loading__5561__auto____4969.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.analyzer.jvm/0.6.5/tools.analyzer.jvm-0.6.5.jar::clojure/tools/analyzer/jvm.clj|9| clojure.tools.analyzer.jvm$eval4968.invokeStatic
|| clojure.tools.analyzer.jvm$eval4968.invoke(jvm.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6923)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:457)
src/main/clojure/clojure/core/match.clj|1| clojure.core.match$eval4960$loading__5561__auto____4961.invoke
src/main/clojure/clojure/core/match.clj|1| clojure.core.match$eval4960.invokeStatic
|| clojure.core.match$eval4960.invoke(match.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6923)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:421)
|| clojure.core.match$eval4949.invokeStatic(form-init2494799382238714928.clj:1)
|| clojure.core.match$eval4949.invoke(form-init2494799382238714928.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6897)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|3096| clojure.core$eval.invokeStatic
|| clojure.core$eval.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404$fn__7407.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl$fn__7413.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl.invokeStatic
|| clojure.main$repl.doInvoke(main.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:1523)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|58| clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__637.invoke
|| clojure.lang.AFn.applyToHelper(AFn.java:152)
|| clojure.lang.AFn.applyTo(AFn.java:144)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|645| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|1874| clojure.core$with_bindings_STAR_.invokeStatic
|| clojure.core$with_bindings_STAR_.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:425)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|56| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic
|| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|191| clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__679$fn__682.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|159| clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__674.invoke
|| clojure.lang.AFn.run(AFn.java:22)
|| java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
|| java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
|| java.lang.Thread.run(Thread.java:745)

Same thing with reloading a namespace in my own project which depends on clojure.core.match

Comment by Nicola Mometto [ 24/Sep/15 3:59 PM ]

is it possible that AOT is involved?

Comment by Michael Blume [ 24/Sep/15 5:31 PM ]

Narrowed it down a little, if I check out tools.analyzer.jvm, open a REPL, and do (require 'clojure.tools.analyzer.jvm.utils) I get

|| java.lang.ClassCastException: java.lang.Class cannot be cast to clojure.asm.Type, compiling:(utils.clj:260:13)
|| clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3642)
|| clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3636)
|| clojure.lang.Compiler$DefExpr.eval(Compiler.java:450)
|| clojure.lang.Compiler.eval(Compiler.java:6939)
|| clojure.lang.Compiler.load(Compiler.java:7381)
|| clojure.lang.RT.loadResourceScript(RT.java:372)
|| clojure.lang.RT.loadResourceScript(RT.java:363)
|| clojure.lang.RT.load(RT.java:453)
|| clojure.lang.RT.load(RT.java:419)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5883| clojure.core$load$fn__5669.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5882| clojure.core$load.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5683| clojure.core$load_one.invokeStatic
|| clojure.core$load_one.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5728| clojure.core$load_lib$fn__5618.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5727| clojure.core$load_lib.invokeStatic
|| clojure.core$load_lib.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:142)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5765| clojure.core$load_libs.invokeStatic
|| clojure.core$load_libs.doInvoke(core.clj)
|| clojure.lang.RestFn.applyTo(RestFn.java:137)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|647| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|5787| clojure.core$require.invokeStatic
|| clojure.core$require.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:421)
|| clojure.tools.analyzer.jvm.utils$eval4392.invokeStatic(form-init8663423518975891793.clj:1)
|| clojure.tools.analyzer.jvm.utils$eval4392.invoke(form-init8663423518975891793.clj)
|| clojure.lang.Compiler.eval(Compiler.java:6934)
|| clojure.lang.Compiler.eval(Compiler.java:6897)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|3096| clojure.core$eval.invokeStatic
|| clojure.core$eval.invoke(core.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404$fn__7407.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|240| clojure.main$repl$read_eval_print__7404.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl$fn__7413.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/main.clj|258| clojure.main$repl.invokeStatic
|| clojure.main$repl.doInvoke(main.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:1523)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|58| clojure.tools.nrepl.middleware.interruptible_eval$evaluate$fn__637.invoke
|| clojure.lang.AFn.applyToHelper(AFn.java:152)
|| clojure.lang.AFn.applyTo(AFn.java:144)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|645| clojure.core$apply.invokeStatic
zipfile:/Users/michael.blume/.m2/repository/org/clojure/clojure/1.8.0-master-SNAPSHOT/clojure-1.8.0-master-SNAPSHOT.jar::clojure/core.clj|1874| clojure.core$with_bindings_STAR_.invokeStatic
|| clojure.core$with_bindings_STAR_.doInvoke(core.clj)
|| clojure.lang.RestFn.invoke(RestFn.java:425)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|56| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invokeStatic
|| clojure.tools.nrepl.middleware.interruptible_eval$evaluate.invoke(interruptible_eval.clj)
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|191| clojure.tools.nrepl.middleware.interruptible_eval$interruptible_eval$fn__679$fn__682.invoke
zipfile:/Users/michael.blume/.m2/repository/org/clojure/tools.nrepl/0.2.10/tools.nrepl-0.2.10.jar::clojure/tools/nrepl/middleware/interruptible_eval.clj|159| clojure.tools.nrepl.middleware.interruptible_eval$run_next$fn__674.invoke
|| clojure.lang.AFn.run(AFn.java:22)
|| java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
|| java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
|| java.lang.Thread.run(Thread.java:745)

I don't see where AOT would be involved?

Comment by Nicola Mometto [ 27/Sep/15 2:28 PM ]

Michael Blume The updated patch should fix the issue you reported

Comment by Michael Blume [ 28/Sep/15 12:39 PM ]

Cool, thanks =)

New patch no longer deletes MethodImplCache, which is not used – is that deliberate?

Comment by Alex Miller [ 02/Nov/15 3:08 PM ]

It would be cool if there was a bulleted list of the things changed in the patch in the description. For example: "Renamed MethodImplCache to ImplCache", etc. That helps makes it easier to review.

Comment by Nicola Mometto [ 02/Nov/15 3:35 PM ]

Attached is an updated patch that doesn't replace MethodImplCache with ImplCache but simply reuses MethodImplCache, reducing the impact of this patch and making it easier (and safer) to review.





[CLJ-1811] test line reporting doesn't always report test's file & line number Created: 02/Sep/15  Updated: 03/Dec/15

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

Type: Enhancement Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test

Attachments: Text File clj-1811.patch     File error_reporting.clj     Text File LINE_REPORING_1.patch     Text File LINE_REPORING_2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If an Exception is thrown during test execution, the filename and line number are frequently not helpful for finding the problem. For instance, this code:

error_reporting.clj
(require '[clojure.test :refer [deftest test-var]])

(deftest foo
  (meta))

(test-var #'foo)

Will output an error at AFn.java:429.

ERROR in (foo) (AFn.java:429)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (0) passed to: core/meta--4144
 at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:28)
    user/fn (error_reporting.clj:4)
    clojure.test$test_var$fn__7670.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    user$eval6.invoke (error_reporting.clj:6)
    clojure.lang.Compiler.eval (Compiler.java:6782)
    ...etc

Rich's Comment 24016 on CLJ-377 says that he thinks the message should report the test file line rather than where the exception was thrown.

Approach: Filter the stacktrace class prefix clojure.lang.AFn from the top of error stacktraces.

After applying the patch, the above example outputs error_reporting.clj:4:

ERROR in (foo) (error_reporting.clj:4)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ArityException: Wrong number of args (0) passed to: core/meta--4141
 at clojure.lang.AFn.throwArity (AFn.java:429)
    clojure.lang.AFn.invoke (AFn.java:28)
    user$fn__3.invokeStatic (error_reporting.clj:4)
    user/fn (error_reporting.clj:3)
    clojure.test$test_var$fn__114.invoke (test.clj:705)
    clojure.test$test_var.invokeStatic (test.clj:705)
    clojure.test$test_var.invoke (test.clj:-1)
    user$eval6.invokeStatic (error_reporting.clj:6)
    user$eval6.invoke (error_reporting.clj:-1)
    clojure.lang.Compiler.eval (Compiler.java:6939)
    ...etc

Patch: clj-1811.patch



 Comments   
Comment by Ryan Fowler [ 02/Sep/15 5:36 PM ]

example file from description

Comment by Alex Miller [ 04/Sep/15 10:04 AM ]

A quick search on Github shows many cases where people call into the (admittedly private) file-and-line function. These users would be broken by the patch. Perhaps it would be better to create a new function or a new arity rather than removing the existing arity.

Just eyeballing it, but I suspect you've introduced reflection in a couple places in the new code, particularly these might need another type hint:

1. (.getName (.getClass (:test (meta test-var))))
2. #(= (.getClassName %) test-var-class-name)

I need to look at more of the code to make a judgement on everything else. Seeing testing-vars in there means that this function is now dependent on external state, so need to think carefully to be sure that every calling context has that global state (or won't fail in bad ways if it doesn't). It would be helpful to see a discussion of your thinking about that in the "approach" section of the ticket description.

Comment by Ryan Fowler [ 30/Sep/15 3:41 PM ]

Second attempt at a patch to resolve this issue. Corrects issues Alex pointed out

Comment by Ryan Fowler [ 30/Sep/15 3:53 PM ]

I've filled in some detail in the approach section.

I also added a new patch LINE_REPORTING_2.patch that addresses reflection warnings, restores the old arity of file-and-line and adds protection from people calling file-and-line from outside a testing context.

Comment by Ryan Fowler [ 30/Sep/15 6:20 PM ]

While discussing an issue with my coworker James, I realized that this fix helps with shared functions calling (is). Notice how the run of this sample code reports line 7 with LINE_REPORTING_2.patch applied. This test line is generally much more useful than the shared function line.

example_2
ryans-mbp:~/oss/clojure% cat -n error_reporting.clj
     1  (require '[clojure.test :refer [deftest test-var is]])
     2
     3  (defn shared-code [arg]
     4    (is arg))
     5
     6  (deftest test-shared-code
     7    (shared-code false))
     8
     9  (test-var #'test-shared-code)
ryans-mbp:~/oss/clojure% java -jar ~/.m2/repository/org/clojure/clojure/1.7.0/clojure-1.7.0.jar ./error_reporting.clj

FAIL in (test-shared-code) (error_reporting.clj:4)
expected: arg
  actual: false
ryans-mbp:~/oss/clojure% java -jar target/clojure-1.8.0-master-SNAPSHOT.jar ./error_reporting.clj

FAIL in (test-shared-code) (error_reporting.clj:7)
expected: arg
  actual: false
Comment by Michael Blume [ 02/Dec/15 5:55 PM ]

Patch doesn't apply anymore, but maybe this has been sorted by CLJ-1856?

Comment by Alex Miller [ 03/Dec/15 9:57 AM ]

This is not fixed by CLJ-1856, but could be if clojure.lang.AFn was filtered out of error stacktraces when finding the location. This is pretty specific - it might make sense to broaden to other calling paths too, not sure.

Attached a new patch that applies this filtering.





[CLJ-1807] Add prefer-proto, like prefer-method but for protocols Created: 30/Aug/15  Updated: 04/Sep/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: protocols

Attachments: Text File 0001-CLJ-1807-add-prefer-proto.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently it's possible to extend a protocol to multiple interfaces but there's no mechanism like prefer-method for multimethods to prefer one implementation over another, as a result, if multiple interfaces match, a random one is picked.

One particular example where this is a problem, is trying to handle generically records and maps (this come up in tools.analyzer): when extending a protocol to both IRecord and IPersistentMap there's no way to make the IRecord implementation be chosen over the IPersistentMap one and thus protocols can't be used.

The attached patch adds a prefer-proto function that's like prefer-method but for protocols.

No performance penalty is paid if prefer-proto is never used, if it's used there will be a penalty during the first protocol method dispatch to lookup the perference table but the protocol method cache will remove that penalty for further calls.

Example:

user=> (defprotocol p (f [_]))
p
user=> (extend-protocol p clojure.lang.Counted (f [_] 1) clojure.lang.IObj (f [_] 2))
nil
user=> (f [1])
2
user=> (prefer-proto p clojure.lang.Counted clojure.lang.IObj)
nil
user=> (f [1])
1

Patch: 0001-CLJ-1807-add-prefer-proto.patch






[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-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-1794] Sorting vector yields non-indexed ArraySeq Created: 05/Aug/15  Updated: 10/Aug/15

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

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

Attachments: Text File 0001-CLJ-1794-Make-ArraySeqs-implement-Indexed.patch    
Approval: Triaged

 Description   

Sorting a vector gives back an ArraySeq with O(n) gets instead of O(log N) gets. This means it can be more efficient to take a vector, sort, then turn it back into a vector.

Cause: sort works by copying the collection to be sorted into an array, calls Arrays/sort to sort it, and then returns a seq on the sorted array. The seq returned is an ArraySeq and doesn't implement Indexed.

Alternatives:

1. Make ArraySeq (and primitive specializations thereof) implement Indexed, providing constant time lookup by index.
2. Specialize sorting for different collection types
3. ???



 Comments   
Comment by Ragnar Dahlen [ 06/Aug/15 2:28 AM ]

Update description, attach patch.

Comment by Ragnar Dahlen [ 06/Aug/15 2:31 AM ]

Added link to current patch.

Comment by Alex Miller [ 06/Aug/15 6:50 AM ]

Another alternative to consider here is to have sort do something smarter.

Comment by Ragnar Dahlen [ 06/Aug/15 7:44 AM ]

Having thought a bit more about the approach and implications of this I'm not sure this patch is a good idea at all. It makes a little bit sense for the particular case of sorting a vector, but on the other hand sort only promises to return a sorted sequence of given coll. Implementing Indexed for a sequence type just because the underlying data structure supports efficient lookup by index feels wrong. Like you suggest, effort is maybe better spent thinking about making sort smarter, which is a different issue, or just using sorted collections instead.

Comment by Kevin Downey [ 06/Aug/15 12:49 PM ]

It seems like the best thing here would be to change sort to return a vector. Usages of sort in the middle of sequence pipelines will continue to work, but a sort followed by conj will break (I cannot recall an instance of this off hand, but I am sure they exist). Sorting seems to imply a fully realized collection, and vectors are the "strongest" realized collections that can be returned here.

Given the conservative nature of core, and the issue with conj ordering above, the next best thing might be to add a sortv similar to the existing mapv.

Another option might be to remove the call to seq, so sort returns the sorted array. This would actually be really useful because you can use Arrays.binarySearch. Calls to conj after a sort would then fail with an exception instead of conj to the "wrong" place.





[CLJ-1790] Error extending protocols to Java arrays Created: 29/Jul/15  Updated: 26/Jan/16

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, protocols

Attachments: Text File 0001-CLJ-1790-emit-a-cast-to-the-interface-during-procol-.patch    
Patch: Code
Approval: Triaged

 Description   

Root cause appears to be related to how protocols are being handled when used with Java arrays:

e.g. for the protocol implementation:

(extend-protocol mp/PImplementation
  (Class/forName "[Ljava.lang.Object;")
    (implementation-key [m] :object-array)
    (meta-info [m]
      {:doc "Clojure.core.matrix implementation for Java Object arrays"})
    (new-vector [m length] (construct-object-vector (long length)))
    (new-matrix [m rows columns]
      (let [columns (long columns)
            m (object-array rows)]
        (dotimes [i rows]
          (aset m i (construct-object-vector columns)))
        m))
    (new-matrix-nd [m shape]
      (construct-nd shape))
    (construct-matrix [m data]
      (construct-object-array data))
    (supports-dimensionality? [m dims]
      (>= dims 1)))

When called as:

(clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

Gives exception:

VerifyError (class: clojure/core/matrix$eval10586, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Also see: CLJ-1381

Patch: 0001-CLJ-1790emit-a-cast-to-the-interface-during-procol.patch



 Comments   
Comment by Nicola Mometto [ 06/Nov/15 3:53 PM ]

Mike Anderson does 1.8.0-beta2 fix this issue?
Alex Miller if core.matrix is still affected this must be fixed before 1.8.0 as it'd mean that direct linking is still broken

Comment by Nicola Mometto [ 06/Nov/15 6:26 PM ]

I could reproduce the bug with 1.8.0-beta2 btw

Comment by Nicola Mometto [ 06/Nov/15 7:27 PM ]

Apparently this is not a 1.8 regression.

At least 1.6 and 1.7 both manifest the same issue:

Clojure 1.6.0
user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Comment by Michael Blume [ 06/Nov/15 8:24 PM ]

Do we know why core.matrix works with Clojure 1.6/1.7 then?

Comment by Nicola Mometto [ 06/Nov/15 9:09 PM ]

It doesn't.

Clojure 1.7.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27

user=> (require 'clojure.core.matrix.protocols)
nil
user=> (clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

VerifyError (class: user$eval6935, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)
user=>

I attached a patch that fixes this issue.
It's caused by the jvm verifier understanding that the object on the stack is an array and thus can never be an instance of the protcol interface, but not being able to understant that the code path leading to the direct protocol interface method invocation can never be reached because of a branch guided by an instance check for that interface on the target

Comment by Mike Anderson [ 06/Nov/15 10:10 PM ]

Apologies, it is possible I just hadn't tested this code path thoroughly before.

It only seems to get triggered in certain circumstances, the following behaviour is interesting:

=> (let [o (identity (object-array 1))]
     (clojure.core.matrix.protocols/dimensionality o))
1
=> (let [o (object-array 1)]
     (clojure.core.matrix.protocols/dimensionality o))
VerifyError (class: clojure/core/matrix$eval17775, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Perhaps it only happens when the callsite has type information about the protocol parameter?

Comment by Nicola Mometto [ 07/Nov/15 4:53 AM ]

Correct, apparently the jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

here's an explaination of exactly our case in pseudo bytecode:

..
 load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return




[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 23/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

All


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

 Description   

It would be nice if assoc-in supported multiple key(s)-to-value pairs (and threw an error when there were an even number of arguments, just like assoc):

user=> (assoc-in {} [:a :b] 1 [:c :d] 2)
{:a {:b 1}, :c {:d 2}}
user=> (assoc-in {} [:a :b] 1 [:c :d])
IllegalArgumentException assoc-in expects even number of arguments after map/vector, found odd number


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

Simple patch attached. I did not find any existing tests for assoc-in but I could add them if wanted.





[CLJ-1770] atom watchers are not atomic with respect to reset! Created: 29/Jun/15  Updated: 31/Jul/15

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

Type: Defect Priority: Major
Reporter: Eric Normand Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: atom

Attachments: Text File atom-reset-atomic-watch-2015-06-30.patch     File timingtest.clj    
Patch: Code and Test
Approval: Triaged

 Description   

It is possible that two threads calling `reset!` on an atom can interleave, causing the corresponding watches to be called with the same old value but different new values. This contradicts the guarantee that atoms update atomically.

(defn reset-test []
  (let [my-atom (atom :start
                      :validator (fn [x] (Thread/sleep 100) true))
        watch-results (atom [])]
    (add-watch my-atom :watcher (fn [k a o n] (swap! watch-results conj [o n])))
  
    (future (reset! my-atom :next))
    (future (reset! my-atom :next))
    (Thread/sleep 500)
    @watch-results))

(reset-test)

Yields [[:start :next] [:start :next]]. Similar behavior can be observed when mixing reset! and swap!.

Expected behavior

Under atomic circumstances, (reset-test) should yield [[:start :next] [:next :next]]. This would "serialize" the resets and give more accurate information to the watches. This is the same behavior one would achieve by using (swap! my-atom (constantly :next)).

(defn swap-test []
  (let [my-atom (atom :start
                      :validator (fn [x] (Thread/sleep 100) true))
        watch-results (atom [])]
    (add-watch my-atom :watcher (fn [k a o n] (swap! watch-results conj [o n])))
  
    (future (swap! my-atom (constantly :next)))
    (future (swap! my-atom (constantly :next)))
    (Thread/sleep 500)
    @watch-results))

(swap-test)

Yields [[:start :next] [:next :next]]. The principle of least surprise suggests that these two functions should yield similar output.

Alternative expected behavior

It could be that atoms and reset! do not guarantee serialized updates with respect to calls to watches. In this case, it would be prudent to note this in the docstring for atom.

Analysis

The code for Atom.reset non-atomically reads and sets the internal AtomicReference. This allows for multiple threads to interleave the gets and sets, resulting in holding a stale value when notifying watches. Note that this should not affect the new value, just the old value.

Approach

Inside Atom.reset(), validation should happen first, then a loop calling compareAndSet on the internal state (similar to how it is implemented in swap()) should run until compareAndSet returns true. Note that this is still faster than the swap! constantly pattern shown above, since it only validates once and the tighter loop should have fewer interleavings. But it has the same watch behavior.

public Object reset(Object newval){
    validate(newval);
    for(;;)
        {
            Object oldval = state.get();
            if(state.compareAndSet(oldval, newval))
                {
                    notifyWatches(oldval, newval);
                    return newval;
                }
        }
}


 Comments   
Comment by Eric Normand [ 30/Jun/15 9:24 AM ]

I've made a test just to back up the timing claims I made above. If you run the file timingtest.clj, it will run code with both reset! and swap! constantly, with a validator that sleeps for 10ms. In both cases, it will print out the number of uniques (should be equal to number of reset!s, in this case 1000) and the time (using clojure.core/time). The timing numbers are relative to the machine, so should not be taken as absolutes. Instead, the ratio between them is what's important.

Run with: java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main timingtest.clj

Results

Existing implementation:

"Elapsed time: 1265.228 msecs"
Uniques with reset!: 140
"Elapsed time: 11609.686 msecs"
Uniques with swap!: 1000
"Elapsed time: 7010.132 msecs"
Uniques with swap! and reset!: 628

Note that the behaviors differ: swap! serializes the watchers, reset! does not (# of uniques).

Suggested implementation:

"Elapsed time: 1268.778 msecs"
Uniques with reset!: 1000
"Elapsed time: 11716.678 msecs"
Uniques with swap!: 1000
"Elapsed time: 7015.994 msecs"
Uniques with swap! and reset!: 1000

Same tests being run. This time, they both serialize watchers. Also, the timing has not changed significantly.

Comment by Eric Normand [ 30/Jun/15 10:16 AM ]

Adding atom-reset-atomic-watch-2015-06-30.patch. Includes test and implementation.





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

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.



 Comments   
Comment by Tim Engler [ 26/Apr/16 4:17 AM ]

Still exists in clojure 1.8





[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: 1
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-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-1750] There should be a way to observe platform features at runtime Created: 08/Jun/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: reader

Approval: Triaged

 Description   

Reader conditionals let the reader emit code conditionally based upon a set of platform features.

This is a closed set - however, currently it is baked in as an implementation detail of the reader. Runtime code cannot access the current platform feature set.

This is problematic when writing a macro that needs to emit code conditionally based upon the platform of the code being compiled. Reader conditionals themselves won't work since macros are always themselves read in Clojure.

We should enable some mechanism for retrieving the current platform at runtime, or at least at macro expansion time.

For example, this is the kind of thing it should be possible to do:

(defmacro mymacro []
    (if (*platforms* :clj)
      `(some-clojure-thing)
      `(some-cljs-thing)))


 Comments   
Comment by Micah Martin [ 19/Jun/15 1:46 PM ]

+1 - Would very much like to see this in 1.7. Currently I have to use an ugly hack.

(def ^:private ^:no-doc cljs? (boolean (find-ns 'cljs.analyzer)))





[CLJ-1743] Avoid compile-time static initialization of classes when using inheritance Created: 02/Jun/15  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Abe Fettig Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: aot, compiler, interop

Attachments: Text File 0001-Avoid-compile-time-class-initialization-when-using-g.patch     Text File clj-1743-2.patch    
Patch: Code
Approval: Triaged

 Description   

I'm working on a project using Clojure and RoboVM. We use AOT compilation to compile Clojure to JVM classes, and then use RoboVM to compile the JVM classes to native code. In our Clojure code, we call Java APIs provided by RoboVM, which wrap the native iOS APIs.

But we've found an issue with inheritance and class-level static initialization code. Many iOS APIs require inheriting from a base object and then overriding certain methods. Currently, Clojure runs a superclass's static initialization code at compile time, whether using ":gen-class" or "proxy" to create the subclass. However, RoboVM's base "ObjCObject" class [1], which most iOS-specific classes inherit from, requires the iOS runtime to initialize, and throws an error at compile time since the code isn't running on a device.

CLJ-1315 addressed a similar issue by modifying "import" to load classes without running static initialization code. I've written my own patch which extends this behavior to work in ":gen-class" and "proxy" as well. The unit tests pass, and we're using this code successfully in our iOS app.

Patch: clj-1743-2.patch

Here's some sample code that can be used to demonstrate the current behavior (Full demo project at https://github.com/figly/clojure-static-initialization):

Demo.java
package clojure_static_initialization;

public class Demo {
  static {
    System.out.println("Running static initializers!");
  }
  public Demo () {
  }
}
gen_class_demo.clj
(ns clojure-static-initialization.gen-class-demo
  (:gen-class :extends clojure_static_initialization.Demo))
proxy_demo.clj
(ns clojure-static-initialization.proxy-demo)

(defn make-proxy []
  (proxy [clojure_static_initialization.Demo] []))

[1] https://github.com/robovm/robovm/blob/master/objc/src/main/java/org/robovm/objc/ObjCObject.java



 Comments   
Comment by Alex Miller [ 18/Jun/15 3:01 PM ]

No changes from previous, just updated to apply to master as of 1.7.0-RC2.

Comment by Alex Miller [ 18/Jun/15 3:03 PM ]

If you had a sketch to test this with proxy and gen-class, that would be helpful.

Comment by Abe Fettig [ 22/Jun/15 8:31 AM ]

Sure, what form would you like for the sketch code? A small standalone project? Unit tests?

Comment by Alex Miller [ 22/Jun/15 8:40 AM ]

Just a few lines of Java (a class with static initializer that printed) and Clojure code (for gen-class and proxy extending it) here in the test description that could be used to demonstrate the problem. Should not have any dependency on iOS or other external dependencies.

Comment by Abe Fettig [ 01/Jul/15 8:49 PM ]

Sample code added, let me know if I can add anything else!

Comment by Abe Fettig [ 27/Jul/15 2:21 PM ]

Just out of curiosity, what are the odds this could make it into 1.8?

Comment by Alex Miller [ 27/Jul/15 6:06 PM ]

unknown.

Comment by Didier A. [ 20/Nov/15 7:11 PM ]

I'm affected by this bug too. A function in a namespace calls a static Java variable which is initialized in place. Another namespace which is genclassed calls that function. Now at compile time, the static java is initialized and it makes building fail, because that static java initialization needs resources which don't exist on the build machine.





[CLJ-1737] Omit java exception class from CompilerException message Created: 23/May/15  Updated: 14/Dec/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
Approval: Triaged

 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-1733] print-dup form unreadable for sorted sets and maps Created: 19/May/15  Updated: 12/Jan/16

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: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.6.0
Clojure 1.7.0-alpha5
Clojure 1.7.0-beta3

java version "1.8.0"
Java(TM) SE Runtime Environment (build 1.8.0-b132)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b70, mixed mode)


Attachments: Text File clj-1733-tagged-literals-throw-on-sorted-set.patch    
Patch: Code and Test
Approval: Triaged

 Description   

print-dup for sorted sets and maps presume a nonexistent static create method that takes an IPersistentCollection

Printing

user=> (print-dup (sorted-set 1) *out*)
#=(clojure.lang.PersistentTreeSet/create [1])

Can't read back

(read-string "#=(clojure.lang.PersistentTreeSet/create [1])")
ClassCastException Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq  java.lang.Class.cast (Class.java:3356)

Possible Fixes

  • add create methods taking IPersistentVector to collections
  • emit something different from print-dup


 Comments   
Comment by Alex Miller [ 19/May/15 4:55 PM ]

It's trying to invoke PersistentTreeSet.create(ISeq) with ["123"]. It's not clear to me where the vector comes from?

Comment by Nikita Prokopov [ 19/May/15 5:04 PM ]

It’s a particular case of CLJ-1461. Vector comes from reading output of print-dup:

(defrecord Rec [f])

(binding [*print-dup* true]
  (prn (Rec. (sorted-set 1))))
;; => #tonsky.Rec[#=(clojure.lang.PersistentTreeSet/create [1])]

I already have a patch for PersistentTreeSet (attached here). Can look into CLJ-1461 later.





[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-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-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-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: 12/Jan/16

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

Type: Enhancement Priority: Trivial
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-1676] map destructuring: prevent evaluation of values in :or when they are not used/needed Created: 14/Mar/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Trivial
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring

Approval: Triaged

 Description   

The name :or implies this should behave as "or" and be "lazy" but it's not the case currently.
The following gist shows the issue. :x is present in the map but we eval the default value:

(defn foo
  [{:keys [x]
    :or {x (println :set-default)}}] 
  x)
 
 
 
user> (foo {:x 1})
:set-default
1


 Comments   
Comment by Ghadi Shayban [ 15/Mar/15 2:40 PM ]

1.2 - current all behave this way, doesn't seem like a recent change.

Comment by Max Penet [ 15/Mar/15 2:55 PM ]

Right, I thought it might have been a regression, but wasn't sure at all.
It seems it would be safe to change the current behavior, I doubt it would break any ones code.





[CLJ-1665] take-nth transducer could be faster without rem Created: 20/Feb/15  Updated: 20/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

Mac OS X 10.10.2, JDK 1.8.0_31


Attachments: Text File CLJ-1665-faster-take-nth-transducer-without-rem.patch    
Patch: Code
Approval: Triaged

 Description   

The take-nth transducer is calling rem on each index, which is relatively expensive compared to a zero? test. It could just count down from N instead as the step size is fixed.



 Comments   
Comment by Steve Miner [ 20/Feb/15 12:34 PM ]

Patch attached. It's about 25% faster on a simple test like:

(time (transduce (take-nth 13) + (range 1e7)))
Comment by Steve Miner [ 20/Feb/15 12:41 PM ]

I didn't worry about (take-nth 0) case, but my patch does give a different result. The current implementation gets a divide by zero error (from rem). My patched version returns just the first element once. The regular collection version returns an infinite sequence of the first element. I doubt anyone expects a sensible answer from the 0 case so I didn't try to do anything special with it.

Comment by Michael Blume [ 20/Feb/15 12:55 PM ]

Nice =)

I would say that the transducer version really ought to match the collection version as closely as possible, but I don't think there's actually a way to write a transducer that transforms a finite sequence into an infinite sequence, so no luck there.

Maybe while we're at it we should change both the transducer and the collection arities to throw on zero?





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

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

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

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

 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-1656] Unroll assoc and assoc! for small numbers of arguments Created: 06/Feb/15  Updated: 29/Apr/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: Major
Reporter: Tom Crayford Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: performance

Attachments: File assoc.diff     Text File assoc-gen-test.patch     Text File CLJ-1656-v1.patch     Text File CLJ-1656-v2.patch     Text File CLJ-1656-v3.patch     Text File CLJ-1656-v4.patch     Text File CLJ-1656-v5.patch     Text File CLJ-1656-v6.patch     Text File CLJ-1656-v7.patch     File cpuinfo     File javaversion     File output     File uname    
Patch: Code and Test
Approval: Triaged

 Description   

Whilst doing performance work recently, I discovered that unrolling to single assoc calls were significantly faster than using multiple keys (~10% for my particular application). Zachary Tellman then pointed out that clojure.core doesn't unroll assoc at all, even for the case of relatively low numbers of keys.

We already unroll other performance critical functions that call things via `apply`, e.g. `update` https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L5914, but `assoc` (which is, I think in the critical path for quite a bunch of applications and libraries), would likely benefit from this.

I have not yet developed patches for this, but I did some standalone benchmarking work:

https://github.com/yeller/unrolling-assoc-benchmarks

benchmark results:

code: https://github.com/yeller/unrolling-assoc-benchmarks/blob/master/src/bench_assoc_unrolling.clj

  1 2 3 4
empty array map (not unrolled) 23ns 93ns 156ns 224ns
empty array map (unrolled assoc) N/A 51ns 80ns 110ns
         
20 element persistent hashmap (not unrolled) 190ns 313ns 551ns 651ns
20 element persistent hashmap (unrolled assoc) N/A 250ns 433ns 524ns
         
record (not unrolled) 12ns 72ns 105ns 182ns
record (unrolled assoc) N/A 21ns 28ns 41ns

Each measurement was made in a separate JVM, to avoid JIT path dependence.

Benchmarks were ran on a commodity server (8 cpus, 32gb ram), with ubuntu 12.04 and a recent release of Java 8. Attached are `cpuinfo`, `uname` and `java -version` output.

Relatively standard JVM production flags were enabled, and care was taken to disable leiningen's startup time optimizations (which disable many of the JIT optimizations).

Benchmarks can be run by cloning the repository, and running `script/bench`

There's one outstanding question for this patch: How far should we unroll these calls? `update` (which is unrolled in the 1.7 alphas) is unrolled to 3 arguments. Adding more unrolling isn't difficult, but it does impact the readability of assoc.

Patch: CLJ-1656-v5.patch



 Comments   
Comment by Tom Crayford [ 09/Feb/15 12:01 PM ]

Ok, attached `assoc.diff`, which unrolls this to a single level more than the current code (so supporting two key/value pairs without recursion). The code's going to get pretty complex in the case with more than the unrolled number of keys if we continue on this way, so I'm unsure if this is a good approach, but the performance benefits seem very compelling.

Comment by Michael Blume [ 09/Feb/15 3:35 PM ]

Since the unroll comes out kind of hairy, why not have a macro write it for us?

Comment by Michael Blume [ 09/Feb/15 4:03 PM ]

Patch v2 includes assoc!

Comment by Tom Crayford [ 09/Feb/15 5:01 PM ]

I benchmarked conj with similar unrolling, across a relatively wide range of datatypes from core (lists, sets, vectors, each one empty and then again with 20 elements):

  1 2 3 4
empty vector (not unrolled) 19ns 57ns 114ns 126ns
empty vector (unrolled conj) N/A 44ns 67ns 91ns
         
20 element vector (not unrolled) 27.35ns 69ns 111ns 107ns
20 element vector (unrolled conj) N/A 54ns 79ns 104ns
         
empty list (not unrolled) 7ns 28ns 53ns 51ns
empty list (unrolled conj) N/A 15ns 20ns 26ns
         
twenty element list (not unrolled) 8.9ns 26ns 49ns 49ns
twenty element list (unrolled) N/A 15ns 19ns 30ns
         
empty set (not unrolled) 64ns 170ns 286ns 290ns
empty set (unrolled) N/A 154ns 249ns 350ns
         
twenty element set (not unrolled) 33ns 81ns 132ns 130ns
twenty element set (unrolled) N/A 69ns 108ns 139ns

Benchmarks were run on the same machine as before. There's a less clear benefit here, except for lists, where the overhead of creating seqs and recursing seems to be clearly dominating the cost of actually doing the conj (which makes sense - conj on any element list should be a very cheap operation). Raw benchmark output is here: https://gist.github.com/tcrayford/51a3cd24b8b0a8b7fd74

Comment by Tom Crayford [ 09/Feb/15 5:04 PM ]

Michael Blume: I like those patches! They read far nicer to me than my original patch. Did you check if any of those macro generated methods blew Hotspot's hot code inlining limit? (it's 235 bytecodes). That'd be my only worry with using macros here - it's easy to generate code that defeats the inliner.

Comment by Michael Blume [ 09/Feb/15 5:57 PM ]

Thanks! This is new for me, so I might be doing the wrong thing, but I just ran nodisassemble over both definitions and the "instruction numbers" next to each line go up to 219 for the varargs arity assoc and up to 251 for assoc!, so, assuming I'm looking at the right thing, maybe that one needs to have an arity taken off? If I remove the highest arity I get 232 for varargs which is just under the line.

I guess alternatively we could call assoc! instead of assoc!* in the varargs arity, which removes a lot of code – in that case it's 176 for varargs and 149 for six pairs.

Comment by Michael Blume [ 09/Feb/15 6:01 PM ]

Gah, I forgot to include coll in the varargs call to assoc!

which reminds me that this patch needs tests.

Comment by Michael Blume [ 09/Feb/15 10:27 PM ]

OK, this has some fixes I made after examining the disassembled output. There's a change to the assoc!* macro to be sure it type-hints correctly – I'm honestly not sure why it didn't type-hint properly before, but it does now. Also, I changed the call to assoc! rolling up the first six entries at the top of the varargs version from a macro call to a function call so it'd fit within the 251 inlineable bytecodes. (This, again, is assuming I'm reading the output correctly).

Comment by Tom Crayford [ 10/Feb/15 6:38 AM ]

Michael: Wanna push a branch with these patches to clojars or something? Then I can rerun the benchmarks with the exact code in the patches.

Comment by Michael Blume [ 10/Feb/15 2:36 PM ]

Hmm, not sure I know how to do that – here's a branch on github though https://github.com/MichaelBlume/clojure/tree/unroll-assoc

Comment by Michael Blume [ 12/Feb/15 1:12 PM ]

v5 marks the helper macros private.

Comment by Tom Crayford [ 13/Feb/15 4:11 AM ]

Michael: was that branch just based off clojure/clojure master? I tried running benchmarks off it, but ran into undefined var errors when building this code (which doesn't happen with alpha5):

(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.pom from clojars)
(Retrieving com/yellerapp/clojure-unrolled-assoc/1.7.0-unrollassoc-SNAPSHOT/clojure-unrolled-assoc-1.7.0-unrollassoc-20150213.092242-1.jar from clojars)
(Retrieving org/clojure/clojure/1.3.0/clojure-1.3.0.jar from central)
Exception in thread "main" java.lang.RuntimeException: Unable to resolve symbol: bench in this context, compiling:(bench_assoc_unrolling.clj:5)
at clojure.lang.Compiler.analyze(Compiler.java:6235)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3452)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6411)
at clojure.lang.Compiler.analyze(Compiler.java:6216)
at clojure.lang.Compiler.analyze(Compiler.java:6177)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5572)
at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5008)

Comment by Michael Blume [ 23/Feb/15 5:08 PM ]

Ok, how are you building? Why the strange clojure group?

Comment by Michael Blume [ 23/Feb/15 5:09 PM ]

The existing version of assoc does runtime checking that an even number of varargs are passed in, but assoc! does not. Do we want to preserve this behavior or do checks in both?

Comment by Michael Blume [ 23/Feb/15 6:00 PM ]

Also, I'm curious how relevant inlining is here – does HotSpot inlining actually work with Var invocation when there's a getRootBinding step in the way?

Comment by Alex Miller [ 23/Feb/15 7:59 PM ]

Yes, inlining works through var invocation.

Comment by Tom Crayford [ 16/Mar/15 7:05 AM ]

Michael,

That group is just an uploaded version of clojure master with your patches applied, built in just the same way as before (you should be able to check out the repo and replicate).

Comment by Alex Miller [ 29/Apr/15 1:44 PM ]

The patch CLJ-1656-v5.patch doesn't seem to do anything with the old version of assoc (in core.clj around line 179)?

The new one needs to have the arglists and other stuff like that. I'm not sure about the macro/private vars in there either. Did you try leveraging RT.assocN() with a vector?

Are there existing tests in the test suite for assoc with N pairs?

Comment by Michael Blume [ 29/Apr/15 8:46 PM ]

The dependencies in clojure.core were such that assoc needed to be defined well before syntax-quoting, so I just let it be defined twice, once slower, once faster. I'll put up a patch with arglists. Does it need an arglist for every new arity, or are the existing arglists enough? (I'm afraid I'm not 100% solid on what the arglists metadata does) There is an annoying lack of existing tests of assoc. I have a generative test in my tree because that seemed more fun than writing cases for all the different arities. I can post it if it seems useful, it might be overkill though.

Comment by Michael Blume [ 29/Apr/15 9:50 PM ]

Here's the test patch I mentioned, it's even more overkill than I remembered

Comment by Michael Blume [ 29/Apr/15 10:01 PM ]

There, code and test.

This also checks that assoc! is passed an even number of kvs in the varargs case, which is the behavior of assoc. The test verifies that both assoc and assoc! throw for odd arg count.

Comment by Alex Miller [ 29/Apr/15 11:10 PM ]

The existing arglist is fine - it just overrides the generated one for doc purposes.

Did you try any of the RT.assocN() stuff?

I guess another question I have is whether people actually do this enough that it matters?





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

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

Approval: Triaged

 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-1643] Generative test for sequence implementations Created: 15/Jan/15  Updated: 12/Jan/16

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: generative-test, test

Attachments: Text File clj-1643-gen-seq-test-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

This is an attempt to write a minimal-foreknowledge failing test for CLJ-1633. By minimal-foreknowledge, I mean a test that fails in the presence of the bug, but which one could imagine writing without intimate knowledge of the details of the bug. I suspect that looking for tests like this is a good way to find gaps in test coverage, and produce tests that will uncover novel regressions later on.

Approach: Generate a single list of operations that could be performed on a sequence, changing that sequence. Make two copies of that operation list, and insert what should be identity-preserving operations into each. Run the two lists of operations and verify that the final results are the same.

With CLJ-1633 unfixed, we get this output:

[java] Testing clojure.test-clojure.sequences
     [java]
     [java] FAIL in (seq-gentest) (sequences.clj:135)
     [java] {:acts1 (->> nil (cons :foo) (cons :foo) into-array next (apply list)),
     [java]  :acts2 (->> nil (cons :foo) (cons :foo) next),
     [java]  :result1 (:foo :foo),
     [java]  :result2 (:foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false





[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 12/Jan/16

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: destructuring, errormsgs

Attachments: Text File CLJ-1630-v2.patch     Text File no-multiple-rest-params-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

(let [[foo & bar & baz] []]) compiles and probably shouldn't.



 Comments   
Comment by Alex Miller [ 01/Jan/15 10:17 AM ]

I see:

user=> (defn foo [bar & baz & qux])

CompilerException java.lang.RuntimeException: Invalid parameter list, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init3743582784321941885.clj:1:1)

?

Comment by Michael Blume [ 01/Jan/15 12:27 PM ]

Sorry, I was working on memory rather than actually typing the thing I put in the description into a REPL, which was dumb.

user=> (let [[foo & bar & baz] []])
nil





[CLJ-1629] Improve error message when defn form omits parameter declaration Created: 29/Dec/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

When defn form is malformed, Clojure compiler will report meaningless error and in combination with function body, can cause really bad experience. Here is the sample:

(defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))

It will report:

IllegalArgumentException Parameter declaration "let" should be a vector  clojure.core/assert-valid-fdecl (core.clj:7123)

However, if is written:

(defn foo "bla")

error report makes more sense:

IllegalArgumentException Parameter declaration missing  clojure.core/assert-valid-fdecl (core.clj:7107)


 Comments   
Comment by Michael Blume [ 29/Dec/14 1:39 PM ]

I don't think this is really meaningless – if you replace the symbol let with a vector, say, [i], you get a perfectly valid function definition

(defn foo
  "This is docstring."
  ([i] [i 1]
    (+ i 1)))
Comment by Sanel Zukan [ 29/Dec/14 2:41 PM ]

Yes and maybe make sense for this case. But in general, the report is misleading for common defn forms (how often you will see function definitions written this way, unless you want multi-arity function) and should have the same report as for second sample; in both cases it is the same cause.





[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-1613] :or defaults should refer to enclosing scope in map destructuring Created: 12/Dec/14  Updated: 11/Jan/16

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: 2
Labels: destructuring

Attachments: Text File 0001-CLJ-1613-evaluate-or-defaults-in-enclosing-scope-in-.patch     Text File CLJ-1613-v2.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.

Comment by Michael Blume [ 11/Sep/15 1:00 PM ]

Updating this patch

Comment by Michał Marczyk [ 11/Sep/15 1:41 PM ]

@Stuart:

To substantiate what was said above, here is the same code snippet evaluated at a Clojure 1.6 REPL and then again at a Clojure 1.7 REPL, both REPLs freshly started, with different results:

Clojure 1.6.0
(let [foo 1 bar 2
      {:keys [foo bar]
       :or {foo 3 bar (inc foo)}} {}]
  [foo bar])
[3 2]

Clojure 1.7.0
(let [foo 1 bar 2
      {:keys [foo bar]
       :or {foo 3 bar (inc foo)}} {}]
  [foo bar])
[3 4]

It is certainly not promised in the docs that there will be no surprising interactions between :or and :keys, but as demonstrated above, any existing code that depended on 1.6 behaviour has already been broken by 1.7. Specifying some behaviour and sticking to it in the future would prevent such surprises going forward.

I also think that the current behaviour is "random" in the sense that there is no principled reason why one might expect it – hence the proposal to make :or defaults refer to the enclosing scope that I've implemented in the patch.





[CLJ-1611] clojure.java.io/pushback-reader Created: 08/Dec/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: io, reader

Attachments: Text File drupp-clj-1611-2.patch     Text File drupp-clj-1611.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Whereas

  • clojure.core/read and clojure.edn/read require a PushbackReader;
  • clojure.java.io/reader produces a BufferedReader, which isn't compatible;
  • the hazard has tripped folks up for years[1];
  • clojure.java.io is pure sugar anyway (and would not be damaged by the addition of a little bit more);
  • clojure.java.io's very existence suggests suitability and fitness for use (wherein by the absence of a read-compatible pushback-reader it falls short);

i.e., in the total absence of clojure.java.io it would not seem "hard" to use clojure.edn, but in the presence of clojure.java.io and its "reader" function, amidst so much else in the API that does fit together, one keeps thinking one is doing it wrong;

and

  • revising the "read" functions to make their own Pushback was considered but rejected [2];

Therefore let it be suggested to add clojure.java.io/pushback-reader, returning something consumable by clojure.core/read and clojure.edn/read.

[1] The matter was discussed on Google Groups:

(2014, "clojure.edn won't accept clojure.java.io/reader?") https://groups.google.com/forum/#!topic/clojure/3HSoA12v5nc

with a reference to an earlier thread

(2009, "Reading... from a reader") https://groups.google.com/forum/#!topic/clojure/_tuypjr2M_A

[2] CLJ-82 and the 2009 message thread



 Comments   
Comment by David Rupp [ 10/Jan/15 4:05 PM ]

Attached patch drupp-clj-1611.patch implements clojure.java.io/pushback-reader as requested.

Comment by David Rupp [ 10/Jan/15 4:07 PM ]

Note that you can always import java.io.PushbackReader and do something like (PushbackReader. (reader my-thing)) yourself; that's really all the patch does.

Comment by Phill Wolf [ 11/Jan/15 7:54 AM ]

clojure.java.io/reader is idempotent, while the patch of 10-Jan-2015 re-wraps an existing PushbackReader twice: first with a new BufferedReader, then with a new PushbackReader.

Leaving a given PushbackReader alone would be more in keeping with the pattern of clojure.java.io.

It also needs a docstring. If pushback-reader were idempotent, the docstring's opening phrase could echo clojure.java.io/reader's, e.g.: Attempts to coerce its argument to java.io.PushbackReader; failing that, (bla bla bla).

Comment by David Rupp [ 11/Jan/15 11:14 AM ]

Adding drupp-clj-1611-2.patch to address previous comments.





[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-1579] source-fn can fail due to reading namespace-aliased keywords while in another namespace context Created: 05/Nov/14  Updated: 20/Nov/15

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.



 Comments   
Comment by Reid McKenzie [ 20/Nov/15 2:29 PM ]

Patch no longer applied to master, updated.





[CLJ-1545] Add unchecked-divide, unchecked-remainder Created: 02/Oct/14  Updated: 06/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Colin Taylor
Resolution: Unresolved Votes: 0
Labels: math, newbie

Attachments: File CLJ-1545-2.diff     File CLJ-1545.diff    
Patch: Code and Test
Approval: Triaged

 Description   

This appears like it might be an oversight that these are missing. There are unchecked-divide-int and unchecked-remainder-int functions, but not equivalents for longs, even though there are equivalents for longs for every other unchecked operation. The JVM has bytecodes for long division and remainder.

The Clojure documentation in the section "Support for Java Primitives" on page http://clojure.org/java_interop has links for unchecked-divide and unchecked-remainder, but since they don't exist in Clojure, the API link targets don't exist.

It seems like a good idea to either add these to Clojure, or remove them from the documentation.



 Comments   
Comment by Colin Taylor [ 03/Oct/14 6:17 PM ]

Having a go at this.

Comment by Colin Taylor [ 04/Oct/14 6:02 AM ]
  • Added tests for unchecked-divide-int and unchecked-remainder-int too.
  • Unchecked fns only support binary arity and will throw CompilerException(ArityException)s where checked will not.
  • Is there any value to (int,long) (long,int) overrides for java interop cases e.g. using java collections from Clojure in high perf code?
Comment by Alex Miller [ 04/Oct/14 9:13 AM ]

Thanks for taking this on Colin!

1) When I apply the patch (git apply CLJ-1545.diff), I get a bunch of whitespace errors which will need to be cleaned up but also the patch seems to fail to apply at all on the changes in test/clojure/test_clojure/numbers.clj. It looks like perhaps the diff is just not the right diff format. You might want to check out the instructions at http://dev.clojure.org/display/community/Developing+Patches about using git format-patch.

2) If you could put a more useful git commit message, that would be helpful. Something like "CLJ-1545 Adds missing unchecked-divide and unchecked-remainder for primitive longs."

Thanks!

Comment by Colin Taylor [ 04/Oct/14 4:47 PM ]

Uggh, sorry Alex.

New patch with better commit message.

Comment by Alex Miller [ 04/Oct/14 7:24 PM ]

The patch format looks better. Pulling out farther to the ticket itself, afaict Clojure will already use the right byteocode for checked or unchecked so this may not even be needed?

If I compile (without the patch):

(defn foo-div ^long [^long a ^long b]
  (quot a b))

then the bytecode for that fn is:

public final long invokePrim(long, long);
    Code:
       0: lload_1
       1: lload_3
       2: ldiv
       3: lreturn

similarly, quot of two longs yields the same code but with lrem. I think patch has no net effect on the resulting bytecode?

Comment by Andy Fingerhut [ 04/Oct/14 7:42 PM ]

Alex, did you do the testing in your previous comment with *unchecked-math* true or false? If false, then I would think that if CLJ-1254 is judged a bug, then the behavior you saw is a bug, too, that misses the same corner case.

Comment by Alex Miller [ 04/Oct/14 10:19 PM ]

The current results are the same with either unchecked-math setting, but I see your point.

Refreshing my memory of the (/ Long/MIN_VALUE -1) case, I think you're right. The (new) unchecked-divide / remainder should do what the current (checked) forms do and the regular division and remainder cases should be making the overflow check. I think CLJ-1254 should cover the quot changes.

Comment by Colin Taylor [ 04/Oct/14 10:19 PM ]

user=> (dotimes [_ 6] (time (dotimes [_ 50000000] (unchecked-divide 4 (System/currentTimeMillis)))))
"Elapsed time: 1806.942 msecs"
"Elapsed time: 1808.747 msecs"
"Elapsed time: 1865.074 msecs"
"Elapsed time: 1802.777 msecs"
"Elapsed time: 1839.468 msecs"
"Elapsed time: 1830.61 msecs"
nil
user=> (dotimes [_ 6] (time (dotimes [_ 50000000] (/ 4 (System/currentTimeMillis)))))
"Elapsed time: 5003.598 msecs"
"Elapsed time: 4998.182 msecs"
"Elapsed time: 4941.237 msecs"
"Elapsed time: 5036.517 msecs"
"Elapsed time: 4965.867 msecs"
"Elapsed time: 4982.693 msecs"





[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-1530] Make foo/bar/baz unreadable Created: 22/Sep/14  Updated: 31/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader

Attachments: Text File 0001-fix-LispReader-and-EdnReader-so-that-foo-bar-baz-is-.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently keywords and symbols containing more than one slash are disallowed by the spec, but allowed by the readers.
This trivial patch makes them unreadable by the readers too.

Pre:

user=> :foo/bar/baz
:foo/bar/baz

Post:

user=> :foo/bar/baz
RuntimeException Invalid token: :foo/bar/baz  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Andy Fingerhut [ 22/Sep/14 12:14 PM ]

Perhaps overlap with CLJ-1527 ?

Comment by Thomas Engelschmidt [ 28/Oct/14 4:36 AM ]

Please notice that keywords with more than one slash has a different hashcode across clojure version 1.5 and 1.6

This creates a problem when using a datomic version that works with clojure 1.5 under clojure 1.6 and the schema have one or more keys with more than one slash.

Comment by Chris Zheng [ 30/Mar/16 8:41 AM ]

Please reconsider this `fix` for the following reasons:

Please see discussion on this topic below:

dm3 [5:04 PM]
Is there a reason why `(read-string "a/b/c")` is OK, while `(clojure.tools.reader/read-string "a/b/c")` fails with `Invalid token`?

hiredman [5:04 PM]
there is a ticket to fix read-string

dm3 [5:05 PM]
so the correct behaviour is to fail?

hiredman [5:05 PM]
http://dev.clojure.org/jira/browse/CLJ-1530

dm3 [5:06 PM]
thanks, seems like a breaking change :simple_smile:

hiredman [5:07 PM]
the docs have precluded symbols like a/b/c for some time, and the behavior of how those unspecified symbols were read apparently changed at some point

lucas [5:09 PM]
joined #clojure

zcaudate [5:16 PM]
@hiredman: seriously. I’m really peeved about that because I’ve been using `:a/b/c` keywords for a while… even wrote a whole freaking library to deal with that http://docs.caudate.me/hara/hara-string.html#api---path (edited)

[5:16]
and now they are taking it out

[5:18]
i’m of the opinion that the `::foo/baz` keywords should be taken out first

[5:20]
```user=> (require '[clojure.walk :as walk])
nil
user=> ::walk/hello
:clojure.walk/hello
```

[5:20]
that causes way more problems

[5:20]
especially with analysers

[5:22]
https://github.com/jonase/kibit/issues/14

GitHub
Kibit breaks on namespace qualified keywords · Issue #14 · jonase/kibit · GitHub
If the code contains namespace qualified keywords with aliases, Kibit errors out with a Invalid token exception. The following code demonstrates the problem - ;;; foo.clj (ns foo) ;;; bar.clj (n...

[5:25]
@dm3 if there’s really a problem, you can patch it:

[5:25]
https://github.com/helpshift/hydrox/blob/master/lein/src/leiningen/hydrox/setup.clj

GitHub
helpshift/hydrox
hydrox - Dive deeper into your code

dm3 [5:31 PM]
yeah, would have to patch cljs.tools.reader too unfortunately :confused:

[5:31]
think I'll just work around that

bronsa [5:36 PM]
@zcaudate: `::foo/bar` style keywords are in by design and not going anywhere, `:foo/bar/baz` have always been invalid by the spec and undefined behaviour (edited)

[5:37]
@dm3: is changing undefined behaviour a breaking change? :simple_smile:

dm3 [5:37 PM]
breaking as in breaking people's code :simple_smile:

[5:37]
e.g. zcaudate

bronsa [5:38 PM]
that code is already broken if it's using invalid clojure. It's just accidentaly working

dm3 [5:39 PM]
I'm looking from a pragmatic perspective. Theoretically you're right :simple_smile:

[5:40]
and I'm not judging either

bronsa [5:40 PM]
pragmatically, `:foo/bar/baz` is a bug waiting to happen. what does `(namespace :foo/bar/baz)` return?

dm3 [5:40 PM]
whatever it returns currently?

[5:41]
I mean it's kind of defined by the implementation

sveri [5:41 PM]
@dm3 @bronsa I wouldnt even agree that you are theoretically right. As soon as enough people adapted the broken code it falls under something like a common law that was accepted by both parties for a long enough time.

bronsa [5:42 PM]
what about `namespace` on `(keyword "foo/bar" "baz")` and `(keyword "foo" "bar/baz")`

dm3 [5:43 PM]
I agree that currently implemented semantics are messy

[5:43]
but my point was that it's still a breaking change

[5:43]
not that it's a "bad" change

[5:43]
that's a judgement

bronsa [5:43 PM]
@sveri: I would agree with you, as long as the undefined behaviour we accept as defined doesn't cause impossible to fix semantics.

[5:44]
that's why for example, the patch that made symbols starting with numbers illegal was rolled back

[5:45]
it broke existing code, it didn't cause weird semantics so it was rolled back. not the case with `:foo/bar/baz`

[5:45]
@dm3: you could make the point that fixing any bug is a breaking change then – people might be relying on that bug.

dm3 [5:46 PM]
yep, I guess what matters is how obvious the incorrect behaviour is and how many people rely on it

bronsa [5:46 PM]
if the doc explicitely says "you can use ​one​ `/` inside a symbol", then if you're using more than one you're writing invalid clojure and you should expect it to maybe break (edited)

dm3 [5:47 PM]
I really haven't even thought about multiple slashes (nor noticed the docs) in a symbol in ~3 years of using Clojure

[5:47]
my initial thought (today) was that it was permitted

[5:47]
and the namespace would be the first segment before the first slash (edited)

bronsa [5:51 PM]
well, that doesn't make much sense though. `/` in clojure means `namespace separator`. if I see `FOO/BAR`, no matter what `FOO` and `BAR` are, I know that `FOO` is the namespace, and `BAR` is the name. if you want to express paths with keywords as in @zcaudate's lib, you should use a different separator in your keywords that doesn't have a special meaning in clojure, like `.` (i.e. `:foo/bar/baz` -> `:foo.bar.baz` or `:foo/bar.baz`) (edited)

dm3 [5:51 PM]
I do not want to argue semantics. Just sharing one point of view

bronsa [5:52 PM]
and my point is that pragmatic point of views (especially when they go against the current doc) should only be considered if the semantics they imply are clear and unambiguous

sveri [5:53 PM]
@bronsa: Nice explanation, thank you :simple_smile:

dm3 [5:53 PM]
yep :simple_smile:

[5:54]
I agree to that, as you have to make decisions in the end

zcaudate [9:07 PM]
@bronsa: written form of communication has a way of making things more serious than they seem

[9:08]
honestly… i knew it was coming since 1.6 when the edn reader started breaking my code

[9:09]
it’s probably more my fault for not communicating this earlier but oh well.. we all have to roll with the times

bronsa [9:09 PM]
@zcaudate: yeah no worries, I was just using your lib as an example since you brought that up

zcaudate [9:10 PM]
having said that, you can imagine my disappointment because I had designed an entire query semantic based on the keyword `:foo/bar/baz` feature (now bug) (edited)

[9:11]
http://docs.caudate.me/adi/adi-walkthrough.html#querying

[9:11]
you noticed I didn’t use `(adi/select ds {:student/classes/teacher/name "Mr. Blair"}})`

[9:12]
in my docs because the tests started breaking

jstew [9:12 PM]
@zcaudate: You put out so much quality stuff that I wonder if you ever sleep!
1

bronsa [9:12 PM]
@zcaudate: luckly the fix should be easy :simple_smile: just replace `/` with `.`

zcaudate [9:12 PM]
no!

[9:13]
see the problem is… datomic has things like `account.type/user`

[9:13]
and so I would have to do the cljs thing `account.type$user`

bronsa [9:13 PM]
(that's some high quality documentation btw, good job)

zcaudate [9:14 PM]
@bronsa: hahaha thanks… so maybe you can push the fix to 1.10

[9:14]
that way I can get a few more months left

[9:15]
like it’s not a big deal… but I thought that there is a parallel between the path structure of the `/` calls and the nesting of maps

[9:16]
and so there is an equivalence to `{:student {:classes {:teacher {:name '(?fulltext "Blair")}`

[9:17]
and `{:student/classes/teacher/name "Mr. Blair"}`

[9:17]
which is prettier in my opinion

bronsa [9:17 PM]
@zcaudate: sorry if that wasn't clear, but I don't actually have any control over when or what gets into clojure or not, I'm just a contributor :simple_smile: so there are chances that the clojure/core team will take a different decision and actually decline that ticket (I would be really disappointed if that was the case though!). If that will happenI will obviously make a change to `tools.reader` to allow them aswell, (edited)

zcaudate [9:18 PM]
@bronsa: damn.

[9:19]
well… maybe you can highlight this fact

[9:19]
and also if the fix is made, a fix to `(keyword "foo/bar/baz")` will also be needed (edited)

bronsa [9:20 PM]
don't think that'll ever be done. validating inputs to `keyword`/`symbol` etc has been asked/discussed tons of times and repeatedly declined for performance reasons

zcaudate [9:20 PM]
so that’s a matter of consistency.

bronsa [9:20 PM]
(not that I agree with that decision, but it seems like Rich isn't going ot change his mind on that)

[9:21]
@zcaudate: there's a difference between what a symbol/keyword can be at runtime, and what a valid read-time symbol/keyword is

zcaudate [9:21 PM]
and also, it means I can setup a reader macro #k foo/bar/baz and get the same effect

[9:22]
like it’s stupidly ugly but i believe it will work

bronsa [9:23 PM]
but the ambiguity of what to do with `namespace` and `name` still remains so dunno

[9:23]
@zcaudate: that wouldn't work either way, if http://dev.clojure.org/jira/browse/CLJ-1530 gets accepted neither `:foo/bar/baz` ​nor​ `foo/bar/baz` will be valid anymore

[9:24]
@zcaudate: btw I'd suggest you log your issues with that ticket in a comment there if you feel strongly against it

new messages
[9:25]
I ​suspect​ that the response will be "you should use a delimiter that doesn't have a special meaning in clojure", but I might be completely wrong (I find the core team doesn't agree with my opinions quite frequently ) especially if you point out that your library will break. (edited)

zcaudate [9:30 PM]
@bronsa: thanks for the heads up. I’ll leave a comment and add a prayer for the bdfl

Comment by Alex Miller [ 30/Mar/16 8:55 AM ]

Chris -

  • (keyword "foo/bar/baz") will still be fine. Programmatic keywords can be created for any string - this is intentionally much broader than what the reader supports as a literal in code and it's a feature that's widely used. At some future point, there may be an escaping mechanism for symbols or keywords with characters outside the spec such that the reader could read them as well, but that's outside the scope of this.
  • Your api is using illegal keywords according to http://clojure.org/reference/reader and you should not expect them to work. I think you should change your library.
Comment by Chris Zheng [ 30/Mar/16 6:15 PM ]

Alex

  • I wouldn't necessarily call it `illegal` as the current behavior in the edn.reader was added in 1.6 without warning.
  • Also, if (keyword "foo/bar/baz") is allowed to exist, then there still would be the indeterminate namespace/name problem that @bronsa highlighted. I would argue for consistency and if :foo/bar/baz is illegal in the reader, then it should be illegal everywhere
  • My library should be fine... but users of the library may have to change their queries
Comment by Alex Miller [ 30/Mar/16 7:00 PM ]

The reader page clearly states "'/' has special meaning, it can be used once in the middle of a symbol to separate the namespace from the name" and keywords are "like symbols". This has been on the reader page since the oldest version I can find in the internet archive (July 2008). edn (despite its similarities) is a separate thing than Clojure, and irrelevant.

The 2-arity form of keyword can be used without ambiguity: (keyword nil "foo/bar/baz"). The 1-arity form will split based on the first / found (in this example into "foo" and "bar/baz"). I see no reason that would need to change.

Comment by Chris Zheng [ 31/Mar/16 8:58 AM ]

I disagree on the "specialness" of `/` in keywords, especially if it is allowed as a string.

This leads to another problem in the ambiguity of output:

user=> (keyword "foo" "bar/baz")
:foo/bar/baz

user=> (keyword nil "foo/bar/baz")
:foo/bar/baz

Unless the output is displayed as :foo//bar/baz, it is unclear where the namespace is and the concept of `code is data` will be diminished if the output cannot be read back as data.

There is also the case for symbol:
user=> (symbol "foo/bar/baz")
foo/bar/baz

well... at least all the examples listed are all consistently "illegal"

Anyways, even if the docs had not explicitly stated such, :foo/bar/baz has existed since the beginning of clojure and personally, it seems to be more string-like than symbol-like. Ironically, I'm pretty sure that I got the idea of using multiple slashes in keywords from reading the datomic documentation from back when I started work on adi.

Ultimately, the decision is not mine to make and I do value the guidance of clojure team over the development of the language. I do however, hope that my points for keeping things as they are can be recognised and be taken into consideration.

Comment by Chris Zheng [ 31/Mar/16 9:23 AM ]

I'll quote Rich https://www.youtube.com/watch?v=P76Vbsk_3J0 @ 5:10

"Many of the things you consider to be problems (with lisp) are features... down the line..."





[CLJ-1527] Clarify and align valid symbol and keyword rules for Clojure (and edn) Created: 18/Sep/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: reader

Approval: Triaged

 Description   

Known areas of under-specificity (http://clojure.org/reader#The%20Reader--Reader%20forms):

  • symbols (and keywords) description do not mention constituent characters that are currently in use by Clojure functions such as <, >, =, $ (for Java inner classes), & (&form and &env in macros), % (stated to be valid in edn spec)
  • keywords currently accept leading numeric characters which is at odds with the spec - see CLJ-1286

References:



 Comments   
Comment by Andy Fingerhut [ 17/Oct/14 2:13 AM ]

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.

Comment by Andy Fingerhut [ 01/Jun/15 12:22 AM ]

Alex, Rich made this comment on CLJ-17 in 2011: "Runtime validation off the table for perf reasons. cemerick's suggestion that arbitrary symbol support will render them valid is sound, but arbitrary symbol support is a different ticket/idea." I am not aware of any tickets that propose the enhancement of allowing arbitrary symbols to be supported by Clojure, e.g. via a syntax like

#|white space and arbitrary #$@)$~))@ chars here|

Do you think it is reasonable to create an enhancement ticket for supporting arbitrary characters in symbols and keywords?

Comment by Alex Miller [ 01/Jun/15 6:36 AM ]

Sure. I looked into this a bit as a digression off of feature expressions and #| has been reserved for this potential use. However, there are many tricky issues with it and I do not expect this to happen soon - more likely to be something we're pushed to do when necessary for some other reason.

Comment by Herwig Hochleitner [ 01/Jun/15 8:46 AM ]

Wrong ticket, but to anybody thinking about #|arbitrary symbols (or strings)|, please do consider making the delimiters configurable, as in mime multipart.

Comment by Andy Fingerhut [ 01/Jun/15 8:54 AM ]

I've created a design page for now. I'm sure it does not list many of the tricky issues you have found. I'd be happy to take a shot at documenting them if you have any notes you are willing to share.

http://dev.clojure.org/pages/viewpage.action?pageId=11862058

Comment by Andy Fingerhut [ 01/Jun/15 9:01 AM ]

Herwig, can you edit the design page linked in my previous comment, to add a reference or example to precisely how mime multipart allows delimiters to be configurable, and why you believe fixed delimeters would be a bad idea?

Comment by Herwig Hochleitner [ 01/Jun/15 9:46 AM ]

I've commented on the design page.

Comment by Alex Miller [ 13/Jul/15 12:44 PM ]

Removed a couple of issues that have been clarified on the reader page and are no longer issues.

Comment by Nicola Mometto [ 13/Jul/15 12:45 PM ]

Related to CLJ-1530

Comment by Adam Frey [ 15/Jul/15 11:55 AM ]

Related to this: The Clojure reader will not accept symbols and keywords that contain consecutive colons (See LispReader.java), although that is permitted by the current EDN spec. Here is a GitHub issue regarding consecutive colons. I would like to qualify why consecutive colons are disallowed, and sync up the Clojure Reader and the EDN spec on this.

Comment by Herwig Hochleitner [ 31/Jul/15 8:03 AM ]

The updated reader spec says that a symbol can contain a single / to separate the namespace. It also mentions a bare / to be the division function.
So what about clojure.core//? That still got to be a readable symbol right? So is that an exception to the 'single /' rule?
Will foo.bar// also be readable? What about foo//bar?

Comment by Francis Avila [ 10/Sep/15 9:26 AM ]

Another source of ambiguity I see is that it's unclear whether the first colon of a keyword is the first character of the keyword (and therefore of the symbol) or whether it is something special and the spec really describes what happens from the second character onward. This matters because the specification for a keyword is (in both edn and reader specs) given in terms of differences from symbols. I think many of the strange keyword edge cases (including legality of :1 vs :a/1) stem from this ambiguity, and different tickets/patches seem to choose one or the other underlying assumption. See this comment for more examples.

Possibly we can use tagged literals for keywords and symbols to create or print these forms when they are not readable and simplify the reader spec for their literal forms. E.g. instead of producing complicated parse rules to ensure clojure.core// or :1 are legal, just make the literal form simple and have users write something like #sym["clojure.core" "/"] or #kyw "1" (and have the printer print these) when they hit these edge cases.

Comment by Alex Miller [ 10/Sep/15 9:44 AM ]

I would say : (and : are syntactic markers and the spec describes the characters following it. But I agree it would be nice for this to be more explicit. The (incorrect) regex in LispReader does not help either.

The tagged literal idea is an interesting alternative to the | | syntax that has been reserved for possible future support for invalid characters in keywords and symbols. But I think the idea is out of scope for this ticket, which is really about clarifying the spec.





[CLJ-1523] Add 'doseq' like macro for transducers Created: 08/Sep/14  Updated: 22/Aug/15

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

Type: Enhancement Priority: Trivial
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: File doreduced2.diff     File doreduced.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Doseq is currently a good way to execute a lazy sequence and perform side-effects. It would be nice to have a matching macro for transducers.

Approach: The included patch simply calls transduce with the provided xform, collection, and a reducing function that throws away the accumulated value at each step. The value from each reducing step is bound to the provided symbol. A shorter arity is provided for those cases when no xform is desired, but fast doseq-like semantics are still wanted.

Patch: doreduced2.diff



 Comments   
Comment by Jozef Wagner [ 09/Sep/14 4:19 AM ]

How about making xform parameter optional? And you have a typo in docstring example, doseq -> doreduced.

Comment by Timothy Baldridge [ 09/Sep/14 7:52 AM ]

Good point, fixed typeo, added other arity.





[CLJ-1522] Enhance multimethods metadata Created: 08/Sep/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: metadata

Approval: Triaged

 Description   

I think that multimethod metadata can be extended a bit with some property indicating the var in question is referring to a multimethod (we have something similar for macros) and some default arglists property.

I'm raising this issue because as a tool writer (CIDER) I'm having hard time determining if something is a multimethod (I have to resort to code like (instance? clojure.lang.MultiFn obj) which is acceptable, but not ideal I think (compared to macros and special forms)). There's also the problem that I cannot provide the users with eldoc (function signature) as it's not available in the metadata (this issue was raised on the mailing list as well https://groups.google.com/forum/#!topic/clojure/crje_RLTWdk).

I feel that we really have a problem with the missing arglist and we should solve it somehow. I'm not sure I'm suggesting the best solution and I'll certainly take any solution.



 Comments   
Comment by Bozhidar Batsov [ 09/Sep/14 4:24 AM ]

Btw, I failed to mention this as I thought it was obvious, but I think we should use the dispatch function's arglist in the multimethod metadata.





[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-1493] Fast keyword intern Created: 06/Aug/14  Updated: 14/Aug/15

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

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: keywords, performance
Environment:

Mac OS X 10.9.4 / 2.6 GHz Intel Core i5 / 8 GB 1600 MHz DDR3
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_keyword_intern.diff    
Patch: Code
Approval: Triaged

 Description   

Keyword's intern(Symbol) method uses recursive invocation to get a valid keyword instance.I think it can be rewrite into a 'for loop'
to reduce method invocation cost.
So i developed this patch, and make some simple benchmark.Run the following command line three times after 'ant jar':

java -Xms64m -Xmx64m -cp test:clojure.jar clojure.main -e "(time (dotimes [n 10000000] (keyword (str n))))"

Before patched:

"Elapsed time: 27343.827 msecs"
"Elapsed time: 26172.653 msecs"
"Elapsed time: 25673.764 msecs"

After patched:

"Elapsed time: 24884.142 msecs"
"Elapsed time: 23933.423 msecs"
"Elapsed time: 25382.783 msecs"

It looks the patch make keyword's intern a little more fast.

The patch is attached and test.

Thanks.

P.S. I've signed the contributor agreement, and my email is killme2008@gmail.com .



 Comments   
Comment by Alex Miller [ 07/Aug/14 9:01 AM ]

Looks intriguing (and would be a nice change imo). I ran this on a json parsing benchmark I used for the keyword changes and saw ~3% improvement.

Comment by dennis zhuang [ 07/Aug/14 9:54 PM ]

Updated the patch, remove the 'k == null' clause in for loop,it's not necessary.

Comment by Andy Fingerhut [ 11/Aug/14 1:29 AM ]

Dennis, while JIRA can handle multiple patches with the same name, it can be confusing for people discussing the patches, and for some scripts I have to evaluate them. Please consider giving the patches different names (e.g. with version numbers in them), or removing older ones if they are obsolete.

Comment by dennis zhuang [ 11/Aug/14 9:19 AM ]

Hi,andy

Thank you for reminding me.I deleted the old patch.

Comment by dennis zhuang [ 11/Sep/14 10:34 AM ]

I am glad to see it is helpful.I benchmark the patch with current master branch,it's fine too.

Comment by dennis zhuang [ 14/Aug/15 9:12 AM ]

Is this patch can be merged? Or is it rejected?

Comment by Alex Miller [ 14/Aug/15 9:41 AM ]

As a minor enhancement, this patch has not yet been high enough priority to be considered yet.

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

All right.Hope to merge it.Thanks.





[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: 6
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-1488] Implement Named over Vars Created: 01/Aug/14  Updated: 28/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Implement-clojure.lang.Named-over-Vars.patch    
Patch: Code
Approval: Triaged

 Description   

Vars, while a general reference structure, are used to implement bindings and have special reader and printer notation reflecting this reality. Unlike Keywords and Symbols which share the "namespace/name" notation of Vars, Vars do not implement the clojure.lang.Named interface while they print as if they were Named.

The attached patch implements Named over Vars.

Example:

user=> (name :clojure.core/conj)
"conj"
user=> (namespace :clojure.core/conj)
"clojure.core"
user=> (name 'clojure.core/conj)
"conj"
user=> (namespace 'clojure.core/conj)
"clojure.core"
user=> (name #'clojure.core/conj)
"conj"
user=> (namespace #'clojure.core/conj)
"clojure.core"
user=> (with-local-vars [x 1] (name x))
"--unnamed--"
user=> (with-local-vars [x 1] (namespace x))
nil
user=> (with-local-vars [x 1] (println x))
#<Var: --unnamed-->

This is useful for applications such as the CinC project where Vars are often taken directly as values in which context they would ideally be interchangeable with the Symbols the bound values of which they represent.



 Comments   
Comment by Nicola Mometto [ 02/Aug/14 11:42 AM ]

With this patch calling `name` on a unnamed Var will cause a NPE, I don't think this is desiderable.

Comment by Reid McKenzie [ 02/Aug/14 1:39 PM ]

I agree, however this behavior seems to be standard in Core.

Clojure 1.6.0
user=> (name nil)
NullPointerException clojure.core/name (core.clj:1518)
user=> (namespace nil)
NullPointerException clojure.core/namespace (core.clj:1526)

I'm also not convinced that the "name" or "namespace" of an unbound var is meaningful, in which case a NPE is probably acceptable.

Comment by Nicola Mometto [ 02/Aug/14 1:45 PM ]

I was not talking about unbound Vars, but about anonymous Vars, I'm assuming you miswrote.

I'd agree with you that throwing an exception could be a reasonable behaviour, except I can test for nil before calling name on it while there's no way to test whether a var is named or not, except trying to access directly the .name field which is excatly what this ticket is for.

Comment by Nicola Mometto [ 02/Aug/14 2:27 PM ]

Me and Reid have been talking about this issue over IRC, here's what's come up:

  • Vars can be either unnamed (as are Vars returned by with-local-vars) or contain both a namespace and a name part( that's the case for interned Vars)
  • there's currently no way to test for the "internedness" of a Var, so accessing either the .name or the .namespace field of the Var testing for nil is the only way to do it currently

given the above, the current patch seems unsatisfactory, here some proposed solutions:

  • make Var Named, make namespace return nil for an unnamed Var and name return "--unnamed--"
  • keep Var not implementing Named, add a "var-symbol" function returning either a namespaced symbol matching the ns+name of the Var or nil for an unnamed Var

Personally, I'd rather have the second solution implemented as I don't feel Var should be Named given that they can be unnamed and that strikes me as a contradicion

Comment by Reid McKenzie [ 02/Aug/14 3:16 PM ]

Added patches explicitly handling the unnamed var cases.

Comment by Reid McKenzie [ 02/Aug/14 3:33 PM ]

Squashed all patches into a single diff and updated attachments.





[CLJ-1483] Clarify the usage of replace(-first) with a function Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, string

Attachments: Text File 0001-Clarify-the-usage-of-replace-first-with-pattern-func.patch    
Patch: Code
Approval: Triaged

 Description   

The documentation of replace and replace-first didn't feature any example usage of the pattern + function combo so I've added one.






[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-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 15/Nov/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch     Text File CLJ-1473_v02.patch     Text File CLJ-1473_v03.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)

Patch: CLJ-1473_v03.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 29/Apr/15 1:54 PM ]

Would be nice to include the bad condition in the error (maybe via ex-info?) and also have tests.

Comment by Brandon Bloom [ 03/May/15 12:11 PM ]

New patch includes tests. Unfortunately, can't call ex-info directly due to bootstrapping concerns. Instead, just calls ExceptionInfo constructor directly.

Comment by Alex Miller [ 04/May/15 9:41 AM ]

Bug in the reporting: {:post pre} should be {:post post}.

Test should be improved as it could have caught that.

Comment by Brandon Bloom [ 04/May/15 7:25 PM ]

Good catch with the pre/post copy/paste screw up. Didn't enhance the test though, since that would involve creating an ex-info friendly variant of fails-with-cause

Comment by Rich Hickey [ 09/Oct/15 7:32 AM ]

:pre and :post don't require vectors, just collections

Comment by Andy Fingerhut [ 15/Nov/15 2:39 PM ]

Eastwood 0.2.2, released on Nov 15 2015, will warn about several kinds of incorrect pre and postconditions. See https://github.com/jonase/eastwood#wrong-pre-post

The Eastwood documentation may be misleading right now, in that it says that :pre and :post should be vectors, which is at odds with Rich's comment of Oct 9 2015. Corrections to Eastwood's documentation here are welcome. I guess Rich's intent is that :pre and :post could be vectors, lists, or sets? Would a map ever make sense there?





[CLJ-1458] Enhance the performance of map merges Created: 04/Jul/14  Updated: 01/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: performance

Attachments: Text File 0001-very-simple-test-of-the-merge-function.patch     Text File clj-1458-4.patch     Text File CLJ-1458-5.patch     Text File CLJ-1458-6.patch     Text File CLJ-1458-transient-merge2.patch     Text File CLJ-1458-transient-merge3.patch     Text File CLJ-1458-transient-merge.patch     Text File merge-test-2.patch     File transient-merge.diff    
Patch: Code and Test
Approval: Triaged

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-6.patch

Approach
Migrate c.c/merge later in core after transients & reduce. Leave older version as merge1 for use in cases the precede the newer definition. Make APersistentMap/conj & ATransientMap/cons aware of IKVReduce.

The attached patch preserves two existing behaviors of merge

  • metadata propagation
  • the right hand side of the merges can be a Map.Entry, an IPersistentVector where size=2, and regular maps.

Screened by:



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.

Comment by Ghadi Shayban [ 28/Dec/14 10:07 PM ]

alternate approach attached delaying merge until after protocols load, and then using transducers.

Comment by Michael Blume [ 28/Dec/14 11:50 PM ]

Looks like you're doing (get m k) twice – shouldn't that be thrown in a local?

Comment by Michael Blume [ 29/Dec/14 1:41 PM ]

um, put, in a local, I mean, 'throw' was a bad choice of word.

Comment by Ghadi Shayban [ 29/Dec/14 2:14 PM ]

Yeah there's that – won't be using get anyways after CLJ-700 gets committed.

We should add performance tests too. merging two maps, three, many maps, also varying the sizes of the maps, and for merge-with, varying the % of collisions.

Need to go back to the (some identity) logic, otherwise metadata is propagated from maps other than the first provided. I'll fix later.

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

I don't know if this is supposed to be allowed, but this breaks

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

https://github.com/metosin/compojure-api/blob/0.16.6/src/compojure/api/meta.clj#L198

Comment by Michael Blume [ 29/Dec/14 2:54 PM ]

Ghadi, contains? uses get under the covers, so it's still two gets, right? It seems like it'd be more performant to stick with the ::none trick.

Comment by Nicola Mometto [ 29/Dec/14 5:36 PM ]

This calls for if-let + find.

Comment by Ghadi Shayban [ 29/Dec/14 10:37 PM ]

new patch addressing concerns so far

Comment by Ghadi Shayban [ 29/Dec/14 10:48 PM ]

CLJ-1458-transient-merge3.patch removes silly inlining macro, uses singleton fns instead.

Comment by Michael Blume [ 29/Dec/14 11:14 PM ]

Nice =)

This should come with tests. If we want to preserve the ability to merge with a MapEntry, we should test it. This isn't so much a weakness of the patch as of the existing tests. I see merge and merge-with being used a few times in the test suite, but I see no test whose purpose is to test their behavior.

Comment by Michael Blume [ 29/Dec/14 11:17 PM ]

Extremely simple merge test, we need more than this, but this is a start

Comment by Alex Miller [ 22/Jun/15 10:11 AM ]

clj-1458-4.patch refreshed to apply to master, no changes.

Comment by Ghadi Shayban [ 09/Jan/16 5:09 PM ]

I'd like to reevaluate the scope of this ticket. Can we address 'merge' only and defer 'merge-with'? It's by far the more common function. I've attached a new simplified patch.

Comment by Ghadi Shayban [ 09/Jan/16 9:50 PM ]

CLJ-1458-6.patch is yet another cleaner approach

Comment by Alex Miller [ 01/Feb/16 5:17 AM ]

Can you update the ticket approach section to discuss the APersistentMap.cons / ASSOC changes. Also, can you add a before / after perf test for one or more common cases?





[CLJ-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 14/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: math

Attachments: Text File CLJ-1452.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Clojure's random functions currently use Math.random and related features, which makes them impossible to seed. This seems like an appropriate use of a dynamic var (compared to extra arguments), since library code that wants to behave randomly could transparently support seeding without any extra effort.

I propose (def ^:dynamic *rand* (java.util.Random.)) in clojure.core, and that rand, rand-int, rand-nth, and shuffle be updated to use *rand*.

I think semantically this will not be a breaking change.

Criterium Benchmarks

I did some benchmarking to try to get an idea of the performance implications of using a dynamic var, as well as to measure the changes to concurrent access.

The code used is at https://github.com/gfredericks/clj-1452-tests; the raw output is in a comment.

rand is slightly slower, while shuffle is insignificantly faster. Using shuffle from 8 threads is insignificantly slower, but switching to a ThreadLocalRandom manually in the patched version results in a 2.5x speedup.

Running on my 8 core Linode VM:

Benchmark Clojure Runtime mean Runtime std dev
rand 1.6.0 61.3ns 7.06ns
rand 1.6.0 + *rand* 63.7ns 1.80ns
shuffle 1.6.0 12.9µs 251ns
shuffle 1.6.0 + *rand* 12.8µs 241ns
threaded-shuffling 1.6.0 151ms 2.31ms
threaded-shuffling 1.6.0 + *rand* 152ms 8.77ms
threaded-local-shuffling 1.6.0 N/A N/A
threaded-local-shuffling 1.6.0 + *rand* 64.5ms 1.41ms

Approach: create a dynamic var *rand* and update rand, rand-int, rand-nth, and shuffle to use *rand*

Patch: CLJ-1452.patch

Screened by:



 Comments   
Comment by Gary Fredericks [ 21/Jun/14 7:50 PM ]

Attached CLJ-1452.patch, with the same code used in the benchmarks.

Comment by Gary Fredericks [ 23/Jun/14 8:34 AM ]

Should we be trying to make Clojure's random functions thread-local by default while we're mucking with this stuff? We could have a custom subclass of Random that has ThreadLocal logic in it (avoiding ThreadLocalRandom because Java 6), and make that the default value of *rand*.

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

I think the ThreadLocal question is interesting, not sure re answer.

It would be nice if the description summarized the results of the tests in a table and the criterium output was in the comments instead.

Comment by Gary Fredericks [ 30/Dec/14 1:26 PM ]

Full output from the test repo (which is summarized in the table in the description):

$ echo "Clojure 1.6.0"; lein with-profile +clj-1.6 run; echo "Clojure 1.6.0 with *rand*"; lein with-profile +clj-1452 run
Clojure 1.6.0

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
WARNING: Final GC required 1.261632096547911 % of runtime
Evaluation count : 644646900 in 60 samples of 10744115 calls.
             Execution time mean : 61.297605 ns
    Execution time std-deviation : 7.057249 ns
   Execution time lower quantile : 56.872437 ns ( 2.5%)
   Execution time upper quantile : 84.483045 ns (97.5%)
                   Overhead used : 16.319772 ns

Found 6 outliers in 60 samples (10.0000 %)
    low-severe   1 (1.6667 %)
    low-mild     5 (8.3333 %)
 Variance from outliers : 75.5119 % Variance is severely inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4780800 in 60 samples of 79680 calls.
             Execution time mean : 12.873832 µs
    Execution time std-deviation : 251.388257 ns
   Execution time lower quantile : 12.526871 µs ( 2.5%)
   Execution time upper quantile : 13.417559 µs (97.5%)
                   Overhead used : 16.319772 ns

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

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 150.863290 ms
    Execution time std-deviation : 2.313755 ms
   Execution time lower quantile : 146.621548 ms ( 2.5%)
   Execution time upper quantile : 155.218897 ms (97.5%)
                   Overhead used : 16.319772 ns
Clojure 1.6.0 with *rand*

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
Evaluation count : 781707720 in 60 samples of 13028462 calls.
             Execution time mean : 63.679152 ns
    Execution time std-deviation : 1.798245 ns
   Execution time lower quantile : 61.414851 ns ( 2.5%)
   Execution time upper quantile : 67.412204 ns (97.5%)
                   Overhead used : 13.008428 ns

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

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4757940 in 60 samples of 79299 calls.
             Execution time mean : 12.780391 µs
    Execution time std-deviation : 240.542151 ns
   Execution time lower quantile : 12.450218 µs ( 2.5%)
   Execution time upper quantile : 13.286910 µs (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 7.8228 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 152.471310 ms
    Execution time std-deviation : 8.769236 ms
   Execution time lower quantile : 147.954789 ms ( 2.5%)
   Execution time upper quantile : 161.277200 ms (97.5%)
                   Overhead used : 13.008428 ns

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

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-local-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 960 in 60 samples of 16 calls.
             Execution time mean : 64.462853 ms
    Execution time std-deviation : 1.407808 ms
   Execution time lower quantile : 62.353265 ms ( 2.5%)
   Execution time upper quantile : 67.197368 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 9.4540 % Variance is slightly inflated by outliers
Comment by Gary Fredericks [ 30/Dec/14 1:28 PM ]

I think using a ThreadLocal is logically independent from adding *rand*, so it could be a separate ticket. I just suggested it here since it would for some uses mitigate any slowdown from *rand* but now that I'm looking at the benchmark results again the slowdown might be insignificant.

Comment by Gary Fredericks [ 30/Dec/14 5:44 PM ]

Also worth noting that (as I did in the benchmark code) with just the patch's changes (i.e., no ThreadLocal involved) users still gain the ability to do ThreadLocal manually, which is not currently possible.

Comment by Stuart Halloway [ 19/Jul/15 7:42 AM ]

workaround: data.generators provides seedable random

Comment by Ghadi Shayban [ 14/Jan/16 10:15 AM ]

Just noting, ThreadLocalRandom is >= JDK 7.





[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-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 01/Sep/15

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

Type: Enhancement Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: None

Approval: Triaged

 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

The auto-conversion to Longs is not really the problem in my mind. I'd like to see numerator return the original value when presented with a BigInt and denominator always return 1 when presented with a BigInt. It seems reasonable to request the same for Longs.

If desired, I'd be happy to produce a patch.



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

Comment by Gary Fredericks [ 27/Aug/15 10:38 AM ]

I've definitely written the helper functions Andy describes on several occasions.

Comment by Felipe Micaroni Lalli [ 01/Sep/15 4:58 PM ]

Related issue: https://stackoverflow.com/questions/25194809/how-to-convert-any-number-to-a-clojure-lang-ratio-type-in-clojure

A workaround to that is (numerator (clojure.lang.Numbers/toRatio (rationalize <put any type of number here>)))





[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-1402] sort-by calls keyfn more times than is necessary Created: 11/Apr/14  Updated: 29/Apr/16

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
Approval: Triaged

 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.

Comment by Alex Miller [ 29/Apr/16 10:10 AM ]

This patch seems like it could be slower, rather than faster, due to the extra allocation. Really needs perf tests before it can be seriously considered.





[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-1386] Add transient? predicate Created: 17/Mar/14  Updated: 20/Apr/15

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

Type: Enhancement Priority: Major
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0004-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.

Comment by Andy Fingerhut [ 06/Aug/14 2:16 PM ]

Patch 0002-Add-transient-predicate.patch dated Mar 22 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 Devin Walters [ 06/Aug/14 4:11 PM ]

I've updated the patch to 0003-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.

Comment by Andy Fingerhut [ 29/Aug/14 4:44 PM ]

Patch 0003-Add-transient-predicate.patch dated Aug 6 2013 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.

Comment by Devin Walters [ 31/Aug/14 12:01 AM ]

I've updated the patch to 0004-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.





[CLJ-1373] LazySeq should utilize cached hash from its underlying seq. Created: 09/Mar/14  Updated: 23/Jun/15

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

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

1.6.0 master SNAPSHOT


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

 Description   

Even if underlying seq contains a cached hash, LazySeq computes it every time.

user=> *clojure-version*
{:major 1, :minor 7, :incremental 0, :qualifier "master", :interim true}
user=> (def a (range 100000))
#'user/a
user=> (time (hash a))
"Elapsed time: 21.812 msecs"
375952610
user=> (time (hash a))        ;; hash is cached
"Elapsed time: 0.036 msecs"
375952610
user=> (def b (seq a))
#'user/b
user=> (time (hash b))
"Elapsed time: 0.042 msecs"   ;; uses cached hash
375952610
user=> (def c (lazy-seq b))
#'user/c
user=> (time (hash c))        ;; SHOULD use underlying hash
"Elapsed time: 27.758 msecs"
375952610
user=> (time (hash c))        ;; SHOULD use underlying hash
"Elapsed time: 17.846 msecs"
375952610

Approach: If seq produced by LazySeq implementing IHashEq, use it to calculate the hasheq().
Patch: clj-1373.diff



 Comments   
Comment by Jozef Wagner [ 09/Mar/14 9:20 AM ]

Added patch which checks if underlying seq implements IHashEq and if yes, uses that hash instead of recomputing.

Comment by Alex Miller [ 04/May/15 9:34 AM ]

In this patch, can you update the else case (the original code) to use s rather than this, so seq() is not re-called?

Comment by Jozef Wagner [ 04/May/15 12:30 PM ]

Added patch [^clj-1373-2.diff] that reuses s for else case.

Comment by Alex Miller [ 23/Jun/15 2:15 PM ]

The -2 patch doesn't compile so I guess that was a bad suggestion.





[CLJ-1317] clojure.zip/seq-zip returns spurious nils during traversal Created: 31/Dec/13  Updated: 10/Feb/16

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

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: zip

Attachments: Text File 0001-CLJ-1317-fix-seq-zip-to-avoid-spurious-nils.patch    
Patch: Code
Approval: Triaged

 Description   

Problem reported by Lee Spector on the mailing list:

https://groups.google.com/d/msg/clojure/8TL7IGmE7N0/u1xfgTOLDRgJ

Here's a quote from Lee's post describing the problem:

Here's an illustration, stepping through '(() 0) with next and printing the node at each step: 

(loop [z (zip/seq-zip '(() 0))] 
  (if (zip/end? z) 
    :done 
    (do (println (zip/node z)) 
      (recur (zip/next z))))) 

That produces: 

(() 0) 
() 
nil 
0 
:done 

I don't expect the nil to be there. 

The underlying cause is that seq-zip passes identity as the children argument to zipper. Applied to (), this returns (), which is truthy, leading zipper to descend into a non-existent subtree.

One natural solution would be to use seq in place of identity:

(defn seq-zip [root]
  (zipper seq?
          seq  ;; changed
          (fn [node children] (with-meta children (meta node)))
          root))

With this change, no nil is produced in the example above. Patch with this change forthcoming.



 Comments   
Comment by Michał Marczyk [ 31/Dec/13 5:52 PM ]

Note that the docstring of clojure.zip/zipper asks that the children argument return a seq of children. The rest of clojure.zip, however, expects nil to be returned when there are no children, as evidenced by this problem.

One could argue that this behaviour of the rest of clojure.zip should be fixed, but I think it makes sense and is convenient. Perhaps the docstring should be adjusted, though.

Comment by Alex Miller [ 08/Feb/16 4:36 PM ]

Michał, can I ask why you assigned this to yourself - was there something you planned to add?

Comment by Michał Marczyk [ 10/Feb/16 9:09 AM ]

Hey Alex, I was going to attach a separate patch with a proposal for a docstring adjustment along the lines suggested above (will do that tonight). No change to the code, though, and I guess not worth assigning the ticket – sorry about the unnecessary ping.

Comment by Alex Miller [ 10/Feb/16 9:38 AM ]

No worries, just wanted to know if something was still pending - I will wait to prescreen it.





[CLJ-1305] Add optional not-found argument when invoking vectors or sets as functions Created: 12/Dec/13  Updated: 30/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Dave Tenny Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-Added-tests-for-using-get-on-sets-refs-1305.patch     Text File 0001-add-not-found-to-sets-and-vecs-as-functions-refs-130.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Maps, keywords, and symbols when used as operators allow optional second arguments for 'default-not-found' values is if to 'get'.

({:a 1} :b 'b) => b

However sets don't support this behavior (though they do with 'get') and vectors don't allow the optional default-not-found in their pseudo 'nth' semantics.

user=> (#{:a  :b} :b 'notfound)
ArityException Wrong number of args (2) passed to: PersistentHashSet  clojure.lang.AFn.throwArity (AFn.java:437)


 Comments   
Comment by Pepijn de Vos [ 12/Nov/14 1:31 PM ]

I fixed the problem with Dirk at the Amsterdam Clojurians Hackathon.

Comment by Bozhidar Batsov [ 12/Nov/14 3:15 PM ]

Guess you can add a couple of unit tests as well.

Comment by Dirk Geurs [ 30/Dec/14 9:51 AM ]

I have added some tests for the changes made earlier.





[CLJ-1298] Add more type predicate fns to core Created: 21/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 18
Labels: None

Approval: Triaged

 Description   

Add more built-in type predicates:

1) Definitely missing: (atom? x), (ref? x), (deref? x), (named? x), (map-entry? x), (lazy-seq? x), (boolean?).
2) Very good to have: (throwable? x), (exception? x), (pattern? x).

The first group is especially important for writing cleaner code with core Clojure.



 Comments   
Comment by Alex Miller [ 21/Nov/13 8:42 AM ]

In general many of the existing predicates map to interfaces. I'm guessing these would map to checks on the following types:

atom? = Atom (final class)
ref? = IRef (interface)
deref? = IDeref (interface)
named? = Named (interface, despite no I prefix)
map-entry? = IMapEntry (interface)
lazy-seq? = LazySeq (final class)

throwable? = Throwable
exception? = Exception, but this seems less useful as it feels like the right answer when you likely actually want throwable?
pattern? = java.util.regex.Pattern

Comment by Alex Fowler [ 21/Nov/13 9:02 AM ]

Yes, they do, and sometimes the code has many checks like (instance? clojure.lang.Atom x). Ok, you can write a little function (atom? x) but it has either to be written in all relevant namespaces or required/referred there from some extra namespace. All this is just a burden. For example, we have predicates like (var? x) or (future? x) which too map to Java classes, but having them abbreviated often makes possible to write a cleaner code.

I feel the first group to be especially significant for it being about core Clojure concepts like atom and ref. Having to fall to manual Java classes check to work with them feels inorganic. Of course we can, but why then do we have (var? x), (fn? x) and other? Imagine, for example:

(cond
(var? x) (...)
(fn? x) (...)
(instance? clojure.lang.Atom x) (...)
(or (instance? clojure.lang.Named x) (instance? clojure.lang.LazySeq x)) (...))

vs

(cond
(var? x) (...)
(fn? x) (...)
(atom? x) (...)
(or (named? x) (lazy-seq? x)) (...))

The second group is too, essential since these concepts are fundamental for the platform (but you're right with the (exception? x) one).

Comment by Alex Fowler [ 22/Nov/13 6:35 AM ]

Also, obviously I missed the (boolean? x) predicate in the original post. Did not even guess it is absent too until I occasionally got into it today. Currently the most clean way we have is to do (or (true? x) (false? x)). Needles to say, it looks weird next to the present (integer? x) or (float? x).

Comment by Brandon Bloom [ 22/Jul/14 1:02 AM ]

Predicates for core types are also very useful for portability to CLJS.

Comment by Brandon Bloom [ 22/Jul/14 1:05 AM ]

I'd be happy to provide a patch for this, but I'd prefer universal interface support where possible. Therefore, this ticket, in my mind, is behind http://dev.clojure.org/jira/browse/CLJ-803 etc.

Comment by Alex Miller [ 22/Jul/14 6:12 AM ]

I don't think it's worth making a ticket for this until Rich has looked at it and determined which parts are wanted.

Comment by Alex Miller [ 02/Dec/14 4:33 PM ]

Someone asked about a boolean? predicate, so throwing this one on the list...

Comment by Reid McKenzie [ 02/Dec/14 4:51 PM ]

uuid? maybe. UUIDs have a bit of a strange position in that we have special printer handling for them built into core implying that they are intentionally part of Clojure, but there is no ->UUID constructor and no functions in core that operate on them so I could see this one being specifically declined.

Comment by Brandon Bloom [ 03/May/15 2:50 PM ]

This has been troubling me again with my first cljc project. So, I've added a whole bunch of tickets (with patches!) for individual predicates in both CLJ and CLJS.

Comment by Alex Miller [ 03/May/15 5:35 PM ]

As I said above, I don't want to mess with specific patches or tickets on this until Rich gets a look at this and we decide which stuff should and should not be included. So I'm going to ignore your other tickets for now...

Comment by Nicola Mometto [ 24/Nov/15 4:08 PM ]

map-entry? is included since 1.8





[CLJ-1293] Portable "catch-all" mechanism Created: 05/Nov/13  Updated: 27/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Text File CLJ-1293-v001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Design page: http://dev.clojure.org/display/design/Platform+Errors

CLJS ticket/patch: http://dev.clojure.org/jira/browse/CLJS-661

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.

Open Question: Catch Throwable (patch v001 does this) or Exception? Alternatively, a more carefully crafted list of "non-fatal" errors. See Scala's NonFatal pattern extractor: http://www.scala-lang.org/api/current/index.html#scala.util.control.NonFatal$



 Comments   
Comment by Brandon Bloom [ 28/Dec/14 11:33 AM ]

Noticed this switched from "Minor" to "Critical", so I figured I should mention that I later realized that we might want :default to catch Exception instead of Throwable, so as to avoid catching Error subclasses. Javadocs say: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch." If that's what we actually want, I can provide an updated patch.

Comment by Alex Miller [ 28/Dec/14 2:19 PM ]

Seems like an open question, might be best just to list it as such in the description.

I don't really expect to reach consensus on the ticket or patch right now, just trying to update priorities and raise visibility for discussion with Rich once we get to 1.8.





[CLJ-1290] clojure.xml parse docstring omits InputSource Created: 01/Nov/13  Updated: 12/Mar/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, newbie, xml

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

 Description   

The clojure.xml parse docstring mentions that parameter s "can be a File, InputStream or String naming a URI." But those choices do not cover a common case, parsing the value of a String. Actually, parse also allows InputSource, which solves the problem. The docstring should mention InputSource (or clarify its omission, if not inadvertent).

user> (use '[clojure.xml :as xml])
nil
user> (import '[java.io StringReader])
java.io.StringReader
user> (import '[org.xml.sax InputSource])
org.xml.sax.InputSource
user> (xml/parse (InputSource. (StringReader. "<egg>green</egg>")))
{:tag :egg, :attrs nil, :content ["green"]}

Approach: Update doc-string to reflect that s also can be an InputSource
Patch: CLJ-1290.patch
Screened by:



 Comments   
Comment by Édipo L Féderle [ 15/Sep/14 3:57 PM ]

You and mean that de (doc xml/parse) should include also "can be a xml String" ?
I don't know if I understand you right.
Thanks.

Comment by Phill Wolf [ 24/Sep/14 6:06 AM ]

InputSource is the use of xml/parse that is not encompassed by the docstring:

(xml/parse (InputSource. (StringReader. "<egg>green</egg>")))

Perhaps xml/parse aimed to hide InputSource by making specific provision for some of InputSource's capabilities. But reading a String is important, and xml/parse does not accept a StringReader, so InputSource remains important.





[CLJ-1289] aset-* and aget perform poorly on multi-dimensional arrays even with type hints. Created: 01/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium


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

 Description   

Here's a transcript of the behavior. I don't know for sure that reflection is being done, but the performance penalty (about 1300x) suggests it.

user=> (use 'criterium.core)
nil
user=> (def b (make-array Double/TYPE 1000 1000))
#'user/b
user=> (quick-bench (aget ^"[[D" b 304 175))
WARNING: Final GC required 3.5198021166354323 % of runtime
WARNING: Final GC required 29.172288684474303 % of runtime
Evaluation count : 63558 in 6 samples of 10593 calls.
             Execution time mean : 9.457308 µs
    Execution time std-deviation : 126.220954 ns
   Execution time lower quantile : 9.344450 µs ( 2.5%)
   Execution time upper quantile : 9.629202 µs (97.5%)
                   Overhead used : 2.477107 ns

One workaround is to use multiple agets.

user=> (quick-bench (aget ^"[D" (aget ^"[[D" b 304) 175))
WARNING: Final GC required 40.59820310542545 % of runtime
Evaluation count : 62135436 in 6 samples of 10355906 calls.
             Execution time mean : 6.999273 ns
    Execution time std-deviation : 0.112703 ns
   Execution time lower quantile : 6.817782 ns ( 2.5%)
   Execution time upper quantile : 7.113845 ns (97.5%)
                   Overhead used : 2.477107 ns

Cause: The inlined version only applies to arity 2, and otherwise it reflects.



 Comments   
Comment by Gary Fredericks [ 08/Dec/13 9:28 PM ]

A glance at the source makes it obvious that the hypothesis is correct – the inlined version only applies to arity 2, and otherwise it reflects.

I thought this would be as simple as converting the inline function to be variadic (using reduce), but after trying it I realized this is tricky as you have to generate the correct type hints for each step. E.g., given ^"[[D" the inline function needs to type-hint the intermediate result with ^"[D". This isn't difficult if we're just dealing with strings that begin with square brackets, but I don't know for sure that those are the only possibilities.

Comment by Yaron Peleg [ 13/Feb/14 4:44 AM ]

Bump. I just got bitten bad by this.

There are two seperate issues here:
1) (aget 2d-array-doubles 0 0 ) doesn't emit a reflection warning.
2) It seems like the compiler has enough information to avoid the reflective call here.

Note this gets exp. worse as number of dimensions grows, i.e (get doubles3d 0 0 0)
will be 1M slower, etc' Not true, unless you iterate over all elements. it's
simply n_dims*1000x per lookup.

Nasty surprise, especially considering you often go to primitive arrays for speed,
and a common use case is an inner loop(s) that iterate over arrays.

Comment by Gary Fredericks [ 13/Feb/14 7:08 AM ]

I can probably take a stab at this.

Comment by Gary Fredericks [ 13/Feb/14 8:34 PM ]

I think the reflection warning problem is pretty much impossible to solve without changing code elsewhere in the compiler, because the reflection done in aget is a different kind than normal clojure reflection – it's explicitly in the function body rather than emitted by the compiler. Since the compiler isn't emitting it, it doesn't reasonably know it's even there. So even if aget is fixed for other arities, you still won't get the warning when it's not inlined.

I can imagine some sort of metadata that you could put on a function telling the compiler that it will reflect if not inlined. Or maybe a more generic not-inlined warning?

The global scope of adding another compiler flag seems about balanced by the seriousness of array functions not being able to warn on reflection.

Comment by Gary Fredericks [ 13/Feb/14 8:52 PM ]

Attached CLJ-1289-p1.patch which simply inlines variadic calls to aget. It assumes that if it sees a :tag on the array arg that is a string beginning with [, it can assume that the return value from one call to aget can be tagged with the same string with the leading [ stripped off.

I'm not a jvm expert, but having read through the spec a little bit I think this is a reasonable assumption.

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

I think this probably is actually true, but a more official way to ask that question would be to get the array class and ask for Class.getComponentType() (and less janky than string munging).

Comment by Gary Fredericks [ 14/Feb/14 3:40 PM ]

How would you get the array class based on the :tag type hint?

Comment by Gary Fredericks [ 14/Feb/14 7:05 PM ]

I see (-> s (Class/forName) (.getComponentType) (.getName)) does the same thing – is that route preferred, or is there another one?





[CLJ-1279] Fix confusing macroexpand1 ArityException handling Created: 16/Oct/13  Updated: 24/Mar/16

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: Compiler, errormsgs, macro

Attachments: Text File 0001-Edit-macro-ArityException-in-AFn.patch     Text File 0001-Fix-macroexpand1-s-handling-of-ArityException.patch    
Patch: Code and Test
Approval: Triaged

 Description   

macros can give very confusing error messages when they execute a form which generates an ArityException. clojure.lang.Compiler.macroexpand1 assumes that any ArityException comes from the call to the macro itself, which need not be the case. For instance:

user> (do (defmacro f [] (assoc)) (f))
  ArityException Wrong number of args (-2) passed to: core$assoc  clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
  user> (use 'clojure.repl) (pst)
  nil
  ArityException Wrong number of args (-2) passed to: core$assoc
  	clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
  	clojure.lang.Compiler.macroexpand (Compiler.java:6544)
  	clojure.lang.Compiler.eval (Compiler.java:6618)
  	clojure.lang.Compiler.eval (Compiler.java:6624)
  	clojure.lang.Compiler.eval (Compiler.java:6597)
  	clojure.core/eval (core.clj:2864)
  	clojure.main/repl/read-eval-print--6596/fn--6599 (main.clj:260)
  	clojure.main/repl/read-eval-print--6596 (main.clj:260)
  	clojure.main/repl/fn--6605 (main.clj:278)
  	clojure.main/repl (main.clj:278)
  	clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--1251 (interruptible_eval.clj:56)
  	clojure.core/apply (core.clj:617)
  nil

Easy enough to see the source of the problem in this case, but because both the number of arguments actually passed is off by two, and the stacktrace element for the call to assoc has been dropped, this shortcut by macroexpand1 can get super confusing.

The attached patch corrects this behavior. E.g.

user=> (do (defmacro f [] (assoc)) (f))
  ArityException Wrong number of args (0) passed to: core$assoc  clojure.lang.AFn.throwArity (AFn.java:437)
  user=> (use 'clojure.repl) (pst)
  nil
  ArityException Wrong number of args (0) passed to: core$assoc
  	user/f (NO_SOURCE_FILE:1)
  	clojure.lang.Var.invoke (Var.java:419)
  	clojure.lang.Var.applyTo (Var.java:532)
  	clojure.lang.Compiler.macroexpand1 (Compiler.java:6507)
  	clojure.lang.Compiler.macroexpand (Compiler.java:6580)
  	clojure.lang.Compiler.eval (Compiler.java:6654)
  	clojure.lang.Compiler.eval (Compiler.java:6660)
  	clojure.lang.Compiler.eval (Compiler.java:6633)
  	clojure.core/eval (core.clj:2864)
  	clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
  	clojure.main/repl/read-eval-print--6594 (main.clj:260)
  	clojure.main/repl/fn--6603 (main.clj:278)
  nil


 Comments   
Comment by Alex Coventry [ 17/Oct/13 11:01 AM ]

Patch with test

Comment by Alex Coventry [ 23/Oct/13 11:42 PM ]

Amended patch to deal more gracefully with unexpected stack trace structure.

Comment by Alex Miller [ 24/Oct/13 12:09 AM ]

Also see CLJ-397 and CLJ-383.

Comment by Alex Coventry [ 24/Oct/13 2:46 PM ]

Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing?

Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace.

[1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090

Comment by Alex Miller [ 24/Oct/13 2:57 PM ]

I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.

Comment by Alex Coventry [ 24/Oct/13 10:26 PM ]

This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported.

I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn.

The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.

Comment by Andy Fingerhut [ 25/Oct/13 6:33 PM ]

I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.





[CLJ-1278] State function's unmunged full name in compiled function's toString() Created: 10/Oct/13  Updated: 17/May/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: errormsgs, interop

Attachments: Text File CLJ-1278-2.patch     Text File CLJ-1528--function-tostring.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently function instances print their toString() with the munged Java name:

user=> (ns proj.util-fns)
nil
proj.util-fns=> (defn a->b [a] (inc a))
#'proj.util-fns/a->b
proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x141ba1f1 "proj.util_fns$a__GT_b@141ba1f1"]

For debugging purposes, it would be useful to have the function toString() describe the Clojure-oriented fn name.

Approach: Store the original name in the function instance and use it in the toString() rather than returning the class name.

proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x47d1a507 "proj.util-fns/a->b(NO_SOURCE_FILE:2)"]

Tradeoffs: Increased function instance size for the function name.

Patch: CLJ-1278-2.patch



 Comments   
Comment by Howard Lewis Ship [ 10/Oct/13 8:39 PM ]

Contains changes and updated tests. I don't have any details on if this affects compiler performance or generated code size in any significant or even measurable way.

Comment by Andy Fingerhut [ 11/Oct/13 4:06 PM ]

Howard, sorry I do not have more useful comments on the changes you make in your patch. Right now I only have a couple of minor comments on its form. The preferred format for patches is that created using the instructions shown on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Also, there are several parts of your patch that appear to only make changes in the whitespace of lines. It would be best to leave such changes out of a proposed patch.

Comment by Howard Lewis Ship [ 11/Oct/13 5:00 PM ]

Yes, I didn't notice the whitespace changes until after; I must have hit reformat at some point, despite my best efforts. I'll put together a new patch shortly.

Comment by Howard Lewis Ship [ 11/Oct/13 6:26 PM ]

Clean patch

Comment by Howard Lewis Ship [ 25/Nov/14 6:00 PM ]

FYI, it's been a year. The correct file is CLJ-1278-2.patch.

Comment by Howard Lewis Ship [ 25/Nov/14 6:07 PM ]

... hm, something's changed in recent times.

     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (factory-function))) (str "clojure.test-clojure.fn/" "factory-function/fn"))
     [java]   actual: (not (= "clojure.test-clojure.fn/factory-function/fn__7565" "clojure.test-clojure.fn/factory-function/fn"))
     [java]
     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (named-factory-function))) (str "clojure.test-clojure.fn/" "named-factory-function/a-function-name"))
     [java]   actual: (not (= "clojure.test-clojure.fn/named-factory-function/a-function-name__7568" "clojure.test-clojure.fn/named-factory-function/a-function-name"))

I'd be willing to update my patch if there's any indication that it will ever be picked up. It's been over a year since last update.

Comment by Andy Fingerhut [ 26/Nov/14 10:30 AM ]

The change in behavior you are seeing is most likely due to a fix for ticket CLJ-1330.

And in case you were wondering, no, I am not the person who knows what tickets are of interest. I know that this one has gotten a fair number of votes, and by votes is one of the top ranked enhancement suggestions - look under "enhancements" on this report, or search for 1330: http://jafingerhut.github.io/clj-ticket-status/CLJ-top-tickets-by-weighted-vote.html

The features going into Clojure 1.7 are pretty well decided upon, and a fair number of other fixes and enhancements were delayed to 1.8. A longer than 1 year wait is not unusual, especially for enhancements.

Comment by Howard Lewis Ship [ 26/Nov/14 3:06 PM ]

Thanks for the info; don't want to come off as whiny but The Great Silence is off putting to someone who wants to help improve things.

I'll update my patch, and hope to see some motion for 1.8.

Comment by Alex Miller [ 26/Nov/14 3:43 PM ]

There are ~400 open tickets for Clojure. As a printing enhancement, this is generally considered lower priority than defects. Additionally, the proposal changes the compiler, bytecode generation code, and adds fields to generated objects, which has unassessed and potentially wide impacts. The combination of these things means it might be a while before we get around to looking at it.

Things that you could do to help:
1) Simplify the description. Someone coming to this ticket (screeners and ultimately Rich) want to look at the description and get the maximal understanding with the minimal effort. We have some guidelines on this at http://dev.clojure.org/display/community/Creating+Tickets if you haven't seen it. For an enhancement, a short (1-2 sentence) description of the problem and an example I can run in the repl is best. Then a proposal (again, as short as possible). Examples: CLJ-1529, CLJ-1325, CLJ-1378. For an enhancement like this, seeing (succinct) before/after versions where a user will see this is often the quickest way for a screener to understand the b