<< Back to previous view

[CLJ-2080] clojure.spec/every-kv does not work correctly on vectors Created: 08/Dec/16  Updated: 09/Dec/16

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

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

alpha 14


Approval: Vetted

 Description   

I'd expect all of these to conform successfully:

user=> (s/conform (s/every-kv any? any?) [])
[]
user=> (s/conform (s/every-kv any? any?) [1 2 3])
:clojure.spec/invalid
user=> (s/conform (s/every-kv integer? string?) [])
[]
user=> (s/conform (s/every-kv integer? string?) ["x"])
:clojure.spec/invalid


 Comments   
Comment by Alex Miller [ 09/Dec/16 8:35 AM ]

At the moment, I'm inclined to say the doc in every-kv should be tightened to say "map" instead of "associative collection" but will check with Rich.





[CLJ-2079] Generator overrides for spec aliases are not respected Created: 08/Dec/16  Updated: 08/Dec/16

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

Type: Defect Priority: Major
Reporter: Nate Smith Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: generator, spec

Approval: Vetted

 Description   

Generator overrides for spec aliases are not respected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(require '[clojure.spec :as s])
(require '[clojure.spec.gen :as gen])
(s/def ::original number?)
(s/def ::alias ::original)

(every? double? (gen/sample (s/gen ::alias {::alias gen/double})))
;; => false

Providing a generator override for the original spec works as expected:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(every? double? (gen/sample (s/gen ::alias {::original gen/double})))
;; => true


 Comments   
Comment by Alex Miller [ 08/Dec/16 5:02 PM ]

Probably a missing delay in the alias case - there's another ticket that has the same cause.

Comment by Nate Smith [ 08/Dec/16 6:43 PM ]

Looks like it might be because gensub looks for matching overrides by calling spec-name, which returns the wrong value for spec aliases.

(require '[clojure.spec :as s])
(s/def ::original number?)
(s/def ::alias ::original)
(@#'clojure.spec/spec-name (s/get-spec ::alias))
;; => :user/original




[CLJ-2077] Clojure can't be loaded from the boot classpath under java 9 Created: 06/Dec/16  Updated: 09/Dec/16

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None


 Description   

As part of the changes for the jigsaw module system in Java 9, the
java packages available to the boot classloader are now a subset of
the the full java distribution. This means that classes loaded via the
boot classloader cannot access any classes outside of that subset.

The list of packages not visible to the boot classloader are:

java.activation
java.annotations.common
java.compact1
java.compact2
java.compact3
java.compiler
java.corba
java.scripting
java.se
java.se.ee
java.sql
java.sql.rowset
java.transaction
java.xml.bind
java.xml.ws
jdk.accessibility
jdk.charsets
jdk.crypto.ec
jdk.crypto.pkcs11
jdk.dynalink
jdk.jsobject
jdk.localedata
jdk.naming.dns
jdk.scripting.nashorn
jdk.xml.dom
jdk.zipfs
jdk.attach
jdk.compiler
jdk.hotspot.agent
jdk.internal.le
jdk.internal.opt
jdk.jartool
jdk.javadoc
jdk.jconsole
jdk.jdeps
jdk.jdi
jdk.jlink
jdk.jshell
jdk.jstatd
jdk.jvmstat

Clojure itself only uses one package on that list: java.sql. It is
used in clojure.instant to provide print-method and
print-dup implementations for java.sql.Timestamp, and in
clojure.core/resultset-seq.

This can be seen with (using Clojure 1.4.0 or higher, and a early-access build
of Java 9, most recently tested with 9-ea+147):

java -Xbootclasspath/a:clojure.jar clojure.main -e "(require 'clojure.instant)"

This affects any clojure-based tool that puts itself on the boot
classpath in order to gain a startup time boost (both lein
and boot are affected currently).



 Comments   
Comment by Toby Crawley [ 06/Dec/16 12:34 PM ]

More details on the underlying change that is triggering this are available at http://openjdk.java.net/jeps/261 (search for java.sql to find the relevant section).

Comment by Alex Miller [ 06/Dec/16 8:41 PM ]

Does this need to be a ticket here? Or is this really an issue for build tools?

Comment by Toby Crawley [ 08/Dec/16 4:30 PM ]

That depends on if we want using Clojure from the boot classpath to be an acceptable use case. If not, then I agree, it is just an issue for tooling.

Comment by Toby Crawley [ 09/Dec/16 2:21 PM ]

I realized today that this issue doesn't actually affect boot, since it doesn't use the bootclasspath. So lein is the only tooling I know of that is affected by this.





[CLJ-2076] s/coll-of and s/map-of do not unform their elements Created: 05/Dec/16  Updated: 09/Dec/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2076-2.patch     Text File clj-2076.patch    
Patch: Code and Test
Approval: Vetted

 Description   

s/coll-of and s/map-of unform with identity but should unform their elements:

(s/def ::o (s/coll-of (s/or :i integer? :s string?)))
(->> [1 2 "blah"] (s/conform ::o) (s/unform ::o))
=> [[:i 1] [:i 2] [:s "blah"]]

Expected: [1 2 "blah"]

Cause: every-impl unform* just returns x

Approach: Use the init/add/complete fns to generate an unformed value (when needed)

Patch: clj-2076-2.patch



 Comments   
Comment by Alex Miller [ 07/Dec/16 5:50 PM ]

This needs tests and a bunch of verification, but first pass at fixing.

Comment by Alex Miller [ 09/Dec/16 8:03 AM ]

Added tests, ready to screen





[CLJ-2075] Add three-arities to < <= > >= = == not= Created: 03/Dec/16  Updated: 03/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File clj-2075-add-three-arities-to-comparisons.patch     Text File clj-2075-over-clj-1912.patch    

 Description   

In my practice, using three-arities of less/greater operations is pretty common for e.g. checking a number is in range:

(< 0 temp 100)

The problem is, it is almost three times as slow compared to (and (< 0 temp) (< temp 100)).

This happens because three-arities are handled by the generic vararg arity branch:

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y & more]
    (if (< x y)
     (if (next more)
       (recur y (first more) (next more))
       (< y (first more)))
     false)

This patch adds special handling for three-arities to these fns: < <= > >= = == not=

(defn <
  "Returns non-nil if nums are in monotonically increasing order,
  otherwise false."
  {:inline (fn [x y] `(. clojure.lang.Numbers (lt ~x ~y)))
   :inline-arities #{2}
   :added "1.0"}
  ([x] true)
  ([x y] (. clojure.lang.Numbers (lt x y)))
  ([x y z] (and (. clojure.lang.Numbers (lt x y))
                (. clojure.lang.Numbers (lt y z))))
  ([x y z & more]
   (if (< x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (< y z (first more))))
     false)))

The performance gains are quite significant:

(= 5 5 5) 	 24.508635 ns => 4.802783 ns (-80%)
(not= 1 2 3) 	 122.085793 ns => 21.828776 ns (-82%)
(< 1 2 3) 	 30.842993 ns => 6.714757 ns (-78%)
(<= 1 2 2) 	 30.712399 ns => 6.011326 ns (-80%)
(> 3 2 1) 	 22.577751 ns => 6.893885 ns (-69%)
(>= 3 2 2) 	 21.593219 ns => 6.233540 ns (-71%)
(== 5 5 5) 	 19.700540 ns => 6.066265 ns (-69%)

Higher arities also become faster, mainly because there's one less iteration now:

(= 5 5 5 5) 	 50.264580 ns => 31.361655 ns (-37%)
(< 1 2 3 4) 	 68.059758 ns => 43.684409 ns (-35%)
(<= 1 2 2 4) 	 65.653826 ns => 45.194730 ns (-31%)
(> 3 2 1 0) 	 119.239733 ns => 44.305519 ns (-62%)
(>= 3 2 2 0) 	 65.738453 ns => 44.037442 ns (-33%)
(== 5 5 5 5) 	 50.773521 ns => 33.725097 ns (-33%)

This patch also changes vararg artity of not= to use next/recur instead of apply:

(defn not=
  "Same as (not (= obj1 obj2))"
  {:tag Boolean
   :added "1.0"
   :static true}
  ([x] false)
  ([x y] (not (= x y)))
  ([x y z] (not (= x y z)))
  ([x y z & more]
   (if (= x y)
     (let [nmore (next more)]
       (if nmore
         (recur y z (first more) nmore)
         (not= y z (first more))))
     true)))

Results are good:

(not= 1 2 3 4) 	 130.517439 ns => 29.675640 ns (-77%)

I'm also doing what Jozef Wagner did in CLJ-1912 (calculating (next more) just once), although perf gains from that alone are not that big.

My point here is that optimizing three-arities makes sence because they appear in the real code quite often. Higher arities (4 and more) are much less widespread.



 Comments   
Comment by Nikita Prokopov [ 03/Dec/16 2:32 AM ]

Benchmark code here https://gist.github.com/tonsky/442eda3ba6aa4a71fd67883bb3f61d99

Comment by Alex Miller [ 03/Dec/16 8:24 AM ]

It might make more sense to combine this with CLJ-1912, otherwise these patches will fight.

Comment by Nikita Prokopov [ 03/Dec/16 1:02 PM ]

Use this patch if CLJ-1912 would be applied first





[CLJ-2074] ::keys spec conflicts with destructuring spec Created: 02/Dec/16  Updated: 02/Dec/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: destructuring, spec

Attachments: File close-destructuring-keys-specs.diff    
Patch: Code

 Description   

As a consequence of the destructuring specs being implemented in terms of `s/keys`, defining a spec for `::keys` or `::strs` is problematic at the moment, because it will conflict with trying to use `::keys` for destructuring:

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::keys nil?)
:user/keys
user=> (let [{::keys [a]} {::a 1}] a)
ExceptionInfo Call to clojure.core/let did not conform to spec:
In: [0 0] val: #:user{:keys [a]} fails spec: :clojure.core.specs/local-name at: [:args :bindings :binding :sym] predicate: simple-symbol?
In: [0 0 0] val: ([:user/keys [a]]) fails spec: :clojure.core.specs/seq-binding-form at: [:args :bindings :binding :seq] predicate: (cat :elems (* :clojure.core.specs/binding-form) :rest (? (cat :amp #{(quote &)} :form :clojure.core.specs/binding-form)) :as (? (cat :as #{:as} :sym :clojure.core.specs/local-name))),  Extra input
In: [0 0 :user/keys] val: [a] fails spec: :user/keys at: [:args :bindings :binding :map :user/keys] predicate: nil?
:clojure.spec/args  ([#:user{:keys [a]} #:user{:a 1}] a)
  clojure.core/ex-info (core.clj:4725)

This feels like an implementation detail leak.

The attached patch implements a proposed solution to this issue, by adding a `:closed?` option to `s/keys` and using it for the destructuring spec. If `s/keys` is used with `:closed?` set to true, `conform` will only validate declared specs as opposed to the default behaviour of `s/keys` of validating all namespaced keywords with existing specs.

After this patch, the above example runs fine and usages of `s/keys` without `:closed?` set to true will validate against `::keys` as per current behaviour.

Patch: close-destructuring-keys-specs.diff






[CLJ-2073] AOT compilation can result in spurious ClassCastException during compile Created: 02/Dec/16  Updated: 02/Dec/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: Paul Mooser Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, compiler
Environment:

java version "1.8.0_112"
Java(TM) SE Runtime Environment (build 1.8.0_112-b16)
Java HotSpot(TM) 64-Bit Server VM (build 25.112-b16, mixed mode)


Attachments: File consumer.clj     File implementer.clj     File protocol.clj    

 Description   

If you try to compile the attached files as follows (assuming they are in "src"):

java -Dclojure.compile.path=out -cp "./clojure-1.8.0.jar:out:src" clojure.lang.Compile implementer protocol consumer

an exception will be thrown:

Exception in thread "main" java.lang.ClassCastException: implementer.Obj cannot be cast to protocol.Dependent, compiling:(consumer.clj:5:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3657)
	at clojure.lang.Compiler.compile1(Compiler.java:7474)
	at clojure.lang.Compiler.compile(Compiler.java:7541)
	at clojure.lang.RT.compile(RT.java:406)
	at clojure.lang.RT.load(RT.java:451)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$compile$fn__5682.invoke(core.clj:5903)
	at clojure.core$compile.invokeStatic(core.clj:5903)
	at clojure.core$compile.invoke(core.clj:5895)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.Compile.main(Compile.java:67)
Caused by: java.lang.ClassCastException: implementer.Obj cannot be cast to protocol.Dependent
	at protocol$fn__12$G__8__14.invoke(protocol.clj:3)
	at protocol$fn__12$G__7__17.invoke(protocol.clj:3)
	at protocol$expand_deps.invokeStatic(protocol.clj:8)
	at protocol$expand_deps.invoke(protocol.clj:6)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3652)
	... 15 more
  • This does not occur with 1.6 or earlier versions
  • This does not occur if you do not try to invoke AOT
  • This may not occur for some orderings of the arguments

This appears to be related to the class being loaded by two different class loaders, and also may result in the namespace being compiled more than once. This issue has popped up for us multiple times in our production build, but it took a while to realize it was a compiler issue and to find a minimal example.






[CLJ-2070] Faster clojure.core/delay implementation Created: 29/Nov/16  Updated: 30/Nov/16

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

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

macOS Sierra
intel Core i7 2.5G Hz
Memory 16GB

java version "1.8.0_66"
Java(TM) SE Runtime Environment (build 1.8.0_66-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.66-b17, mixed mode)


Attachments: Text File fast_delay_with_volatile_fn2.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

clojure.lang.Delay uses a synchronized lock to protect the deref method, because it must make sure the Delay object is realized exactly once.

As we known synchronized lock plays worse performance under heavy concurrency. Instead, using volatile and double-checking lock in this situation improves the performance. The benchmark code is at test-delay.clj. The benchmark-delay function accepts thread count number as an argument, and it will start as many threads as the argument to access delay object concurrently as in (time (benchmark-delay 1)).

threads 1.9.0-alpha14 + patch % better
1 0.570196 ms 0.499905 ms 12
10 19.66194 ms 1.313828 ms 93
20 40.740032 ms 2.149794 ms 95
100 184.041421 ms 8.317295 ms 95

Patch: fast_delay_with_volatile_fn2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 29/Nov/16 8:52 AM ]

A faster version of delay would be helpful - these are used extensively inside spec so would actually help the average case spec performance.

These whitespace errors need to be cleaned up...

$ git apply ~/Downloads/fast_delay.patch
/Users/alex/Downloads/fast_delay.patch:67: trailing whitespace.
	                try
/Users/alex/Downloads/fast_delay.patch:105: trailing whitespace.

/Users/alex/Downloads/fast_delay.patch:115: space before tab in indent.
        	    (fn []
/Users/alex/Downloads/fast_delay.patch:116: space before tab in indent.
          		    (.await barrier)
/Users/alex/Downloads/fast_delay.patch:117: space before tab in indent.
          		    (dotimes [_ 10000]
warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.

More importantly, the double-check is on fn, so it's critical that fn is marked as volatile. You should re-run the perf test afterwards.

Comment by dennis zhuang [ 29/Nov/16 9:13 AM ]

Sorry, white spaces errors should be fixed before my attached.

But the fn doesn't need to be marked as volatile, because it's protected by synchronized in all blocks. And writing it to be null is fine here.

fn=null;

It's not like double-checking in singleton example, there is no reordering here.

Comment by Alex Miller [ 29/Nov/16 9:25 AM ]

fn is read at the top before the synchronized block - it needs to be volatile or one thread may not see the write inside the synchronized block from another thread.

Comment by dennis zhuang [ 29/Nov/16 9:41 AM ]

Yep ,but it's fine here.
If one thread can't see the writing null for fn at the top, it will enter the locking block.
The double-checking on fn!=null will make sure the fn is called at most once, and if the fn was called by another thread and was set to be null ,then current thread will fail on second checking on fn!=null and exit the locking to go on reading value or exception.

So, in the worst situation, maybe two or more threads would enter the locking block,but they all will fail on second checking on fn!=null except one thread of them success.

I don't want to declare fn to be volatile, because volatile also has a cost on reading. The fn variable may be flushed into main memory too late, but it's acceptable and safe here, and we avoid the cost of volatile reading.

Comment by Alex Miller [ 29/Nov/16 9:45 AM ]

I think you're wrong, and I'm not screening it without it being marked as volatile.

Comment by dennis zhuang [ 29/Nov/16 9:54 AM ]

The patch which mark fn volatile.

Comment by dennis zhuang [ 29/Nov/16 9:54 AM ]

The patch which does't mark fn volatile.

Comment by dennis zhuang [ 29/Nov/16 9:59 AM ]

Hi, alex.

I understand your opinion here. Though i still insist that fn doesn't need to be marked as volatile, but it's not a critical concern here.

I uploaded two patches, one is marked fn volatile, the other is not. All of them fixed the whitespace errors and update the benchmark result in ticket description.

Thanks.

Comment by dennis zhuang [ 29/Nov/16 10:15 AM ]

Rebase master.

Comment by Nicola Mometto [ 30/Nov/16 11:53 AM ]

dennis, here's an article describing why fn needs to be volatile: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

Comment by dennis zhuang [ 30/Nov/16 6:01 PM ]

@Nicola

I knew the double-checking issue in old JVM memory model, but it is different here.
There is no instance constructed, it's only assigning fn to be null, so it doesn't have instruments reordering. And we don't have a partial constructed instance that escapes.

But it's not critical concern here, it seems that volatile doesn't compact performance of this patch.

Thanks.





[CLJ-2069] lazy seq that encounters an exception has differing behavior on repeated use Created: 27/Nov/16  Updated: 29/Nov/16

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

Type: Defect Priority: Major
Reporter: TianJun Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: lazy
Environment:

OS X EI Capitan, Java HotSpot(TM) 64-Bit Server VM 1.8.0_101-b13


Attachments: Text File 0001-CLJ-2069-cache-exceptions-thrown-during-lazy-seq-rea.patch    
Patch: Code
Approval: Prescreened

 Description   

It seems the below does not compile with 1.8.0 and 1.9.0-alpha14, the same errors appear in both versions.

user=> (def fibonacci-1
  ((fn fib  [a b]
    (lazy-seq  (cons a  (fib b  (+ a b)))))
    0 1))

user=> (filter #(< % 100) fibonacci-1)

ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)

user=> (filter #(< % 100) fibonacci-1)

NullPointerException   clojure.lang.Numbers.ops (Numbers.java:1013)

user=> (def fibonacci-2
         (lazy-cat [0 1] (map + (rest fibonacci-2) fibonacci-2)))

user=> (filter #(< % 100) fibonacci-2)

ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)

user=> (filter #(< % 100) fibonacci-2)
(0 1 1 2 3 5 8 13 21 34 55 89)

Patch: 0001-CLJ-2069-cache-exceptions-thrown-during-lazy-seq-rea.patch

Proposal: Cache exceptions thrown during lazy-seq realization, to avoid re-running bodyfn which is declared as `^:once`

Prescreened by: Alex Miller



 Comments   
Comment by TianJun [ 27/Nov/16 10:42 AM ]

Maybe I should use take-while instead of filter.

However, can anyone explain why I get ArithmeticException while running

(filter #(< % 100) fibonacci-2)

for the first time and get the right result at the second time?

Comment by Nicola Mometto [ 28/Nov/16 3:27 AM ]

The NPE is caused by the interaction between:

  • lazy-seq throwing an exception while realizing part of the sequence
  • lazy-seq internally using ^:once for locals clearing

lazy-seq expects the bodyfn to be run exactly once and then the result to be cached, but if an exception gets thrown during the execution of bodyfn, the function will get run again when the sequence tries to be realized a second time. However if locals clearing has already happened (even partially) this means some locals in bodyfn will now be nil rather than holding their actual value.

Comment by Nicola Mometto [ 28/Nov/16 3:36 AM ]

Attached patch that fixes this issue





[CLJ-2068] s/explain of non-registered predicate yields :s/unknown Created: 23/Nov/16  Updated: 23/Nov/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Vetted

 Description   

Got:

(s/explain #{1 2 3} 4)
val: 4 fails predicate: :clojure.spec/unknown

(s/explain odd? 10)
val: 10 fails predicate: :clojure.spec/unknown

Expected to receive a description of the failing predicate as in:

(s/def ::s #{1 2 3})
(s/explain ::s 4)
;; val: 4 fails spec: :user/s predicate: #{1 3 2}

(s/def ::o odd?)
(s/explain ::o 10)
val: 10 fails spec: :user/o predicate: odd?





[CLJ-2066] Reflection on internal classes fails under Java 9 Created: 22/Nov/16  Updated: 22/Nov/16

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, reflection

Approval: Triaged

 Description   

With the module system (jigsaw) as it is currently implemented in Java 9 early access builds (9-ea+144), calling a method via reflection is only allowed if the Method was retrieved for a class/interface in a package that is exported by its containing module. Reflector.java currently uses only target.getClass() to locate the Method, so reflective method invocation on a non-exported class will fail even if the method is provided by an exported parent interface or superclass.

The current workaround is to export the package to the unnamed module (where an application that doesn't explicitly use the module system runs) when invoking java/javac:

java --add-exports=java.xml/com.sun.xml.internal.stream=ALL-UNNAMED --add-exports=java.xml/com.sun.xml.internal.stream.writers=ALL-UNNAMED --add-exports=java.xml/com.sun.org.apache.xerces.internal.impl=ALL-UNNAMED ...

It's possible that this will be addressed in jigsaw before the release of Java 9. If not, Reflector.java could be modified to walk the ancestor chain if the initial invocation fails. Note that even with that change, accessing methods that are only defined on the non-exported class (i.e. methods that don't override a method from an exported superclass/interface) will require an --add-exports option.

For more details, see https://groups.google.com/d/msg/clojure-dev/Tp_WuEEcdWI/LMMQVAUYBwAJ



 Comments   
Comment by Toby Crawley [ 22/Nov/16 10:02 AM ]

This is the root cause of http://dev.clojure.org/jira/browse/DXML-32





[CLJ-2063] Show longest path explain error first Created: 17/Nov/16  Updated: 17/Nov/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, spec

Attachments: Text File longest-explain.patch    
Patch: Code
Approval: Vetted

 Description   

It is observed that the explain problem with the longest path is most likely the one that parsed the furthest and is thus the closest to the user's actual intent.

Proposed: Sort the explain problems with longest path first.

Patch: longest-explain.patch






[CLJ-2062] Spec import and refer-clojure macros Created: 17/Nov/16  Updated: 17/Nov/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core.specs, spec

Attachments: Text File import-referclj-2.patch    
Patch: Code
Approval: Vetted

 Description   

Add specs for import and refer-clojure.

Patch:

  • Fixes some indentation of previous specs
  • Factors out ::filters spec from ::ns-refer-clojure
  • Factors out ::import-list from ::ns-import
  • Reuses ::filters in ::ns-refer
  • Reuses ::filters in ::use-prefix-list
  • Removes :ret any? in ::ns-use (no need for it)
  • Adds clojure.core/import spec
  • Adds clojure.core/refer-clojure spec

Patch: import-referclj-2.patch






[CLJ-2061] Better error message when exercise-fn called on fn without :args spec Created: 17/Nov/16  Updated: 17/Nov/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, spec

Attachments: Text File clj-2061.patch    
Patch: Code
Approval: Vetted

 Description   
;; no spec
user=> (s/exercise-fn str)
IllegalArgumentException No implementation of method: :specize* of protocol: #'clojure.spec/Specize found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:583)

;; no :args spec
user=> (s/fdef str :ret string?)
user=> (s/exercise-fn str)
IllegalArgumentException No implementation of method: :specize* of protocol: #'clojure.spec/Specize found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:583)

Proposed: Check for missing :args spec and throw better error

user=> (s/exercise-fn str)
Exception Unable to resolve args spec  clojure.spec/exercise-fn (spec.clj:1811)

user=> (s/fdef str :ret string?)
user=> (s/exercise-fn str)
Exception Unable to resolve args spec  clojure.spec/exercise-fn (spec.clj:1811)

Patch: clj-2061.patch






[CLJ-2060] Add undef to remove a spec Created: 16/Nov/16  Updated: 16/Nov/16

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

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

Attachments: Text File clj-2060.patch    
Patch: Code
Approval: Vetted

 Description   

Currently there is no provided way to remove a spec from the registry. During interactive development, particularly when working on complicated or recursive specs, it would be useful to have this ability.

Proposed: Add s/undef that removes a spec from the registry. In the patch it returns the updated registry, although that may be more harmful than helpful at the repl (where receiving nil would probably be less noise). Another option would be to return true/false indicating whether the key was in the registry. However, as the registry is held in an atom, this has a race and would be more involved to implement (so I didn't).

Patch: clj-2060.patch



 Comments   
Comment by Alex Miller [ 16/Nov/16 11:55 AM ]

Moving to 1.9 so it will get looked at, may not be added.





[CLJ-2059] explain-data problems don't provide resolved symbols under :pred Created: 15/Nov/16  Updated: 16/Nov/16

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, spec

Attachments: Text File clj-2059.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Currently, explain-data returns unresolved preds. This is a problem when trying to write a custom explain print function that chooses what to do based on the predicate as it does not have enough information.

(require '[clojure.spec :as s])

(defn password-valid-length? [pass]
  (> (count pass) 12))

(s/def ::password (s/and string? password-valid-length?))

(-> (s/explain-data ::password "foobar")
  ::s/problems first :pred) 
;;=> password-valid-length?  
;;expected: user/password-valid-length?

Cause: Currently, explain* returns preds in the abbrev form for all spec impls.

Proposed: Have explain* return resolved preds. In cases where the abbreviated form should be used (anything for human consumption at either the repl or an error message), convert to it. For example, explain-printer should (and already does) do this.

Patch: clj-2059.patch

  • Changes all spec impls to avoid calling abbrev on preds when building explain-data
  • Undoes op-describe change for s/? in CLJ-2042 and fixes this at a higher level by calling res on the incoming pred (this is a better fix)
  • Changes the expected test output for spec tests to expect the resolved pred





[CLJ-2057] Function spec missing :ret can yield wrong answer for valid? Created: 14/Nov/16  Updated: 16/Nov/16

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

Type: Defect Priority: Major
Reporter: James Gatannah Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File clj-2057.patch    
Patch: Code
Approval: Vetted

 Description   

Create a spec on a function, leaving off the :ret.

user> (s/fdef ::foo :args (s/cat :n integer?))
=> :user:foo
user> (defn f [n] (* n 2))
=> #'user/f
;; Need org.clojure/test.check on classpath
user> (s/valid? ::foo f)
=> false
user> (s/explain-data ::foo f)
=> nil

Cause: Originally, :ret spec was required. We loosened that requirement, but parts of the implementation still assume that the :ret spec exists (valid-fn, etc). Here, s/valid? is incorrectly returning false because the returned value does not match the non-existent :ret spec, even though f should be fine. explain-data is doing the right thing (it's not failing).

Proposed: Patch in any? as the default :ret spec if it's missing. Another way to go would be to verify that all of the existing fspec conform and explain code worked as intended when :ret spec is missing - it seems like we would effectively be swapping in an any? spec in all of those cases though, so the proposed path seemed easier.

Patch: clj-2057.patch



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

fyi, fdef should take a qualified symbol, not a qualified keyword. To do what you're doing here, I would do:

(s/def ::foo (s/fspec :args (s/cat :n integer?)))
(defn f [n] (* n 2))
(s/valid? ::foo f)
(s/explain-data ::foo f)

Not that you will get a different result, but that's really the intent of the api. You're leaning a bit too much on implementation details that may change (namely that fdef is effectively def of an fspec - this didn't used to be the case and may not be the case in the future).





[CLJ-2056] Efficient shortcut for (first (filter pred coll)) idiom Created: 11/Nov/16  Updated: 14/Nov/16

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 25
Labels: None

Attachments: Text File clj-2056-clojure-core-seek-2.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

It's a common task to look up for an item in a collection based on predicate. Currently Clojure has no direct support for that in clojure.core. Instead, our options are:

1. (first (filter pred coll)) will create intermediate lazy sequence and might evaluate pred up to 31 extra times in case of chunked sequence

2. (some #(when (pred %) %) coll) will short-circuit on first match, but won't catch false value in something like (some #(when false? %) [true false true])

Additionally, both of these workarounds a) obscure the purpose of the code, and b) do not handle custom not-found values.

Attached is a patch that makes use of efficiency of reduce-able collections, handles edge cases like looking for false? or nil?, and supports optional not-found value.

Examples:

(seek odd? (range)) => 1
(seek pos? [-1 1]) => 1
(seek pos? [-1 -2] ::not-found) => ::not-found
(seek nil? [1 2 nil 3] ::not-found) => nil

Patch: clj-2056-clojure-core-seek-2.patch

Prescreening notes: I think the general approach is good. Is it necessary to support nil? and false? preds? Or would a transduce formulation like the one in comments be sufficient.



 Comments   
Comment by Alex Miller [ 11/Nov/16 8:54 AM ]

Just as an interesting aside, the new halt-when transducer could actually be used to create something like this too (if you set aside the desire to support nil? and false? preds).

(transduce (comp (filter pred) (halt-when any?)) identity nil coll)

Patch has some trailing whitespace in the test code - could you clean that up?

Comment by Nikita Prokopov [ 12/Nov/16 3:46 AM ]

Attaching patch with trailing whitespace cleaned

Comment by Nikita Prokopov [ 12/Nov/16 3:46 AM ]

Thanks Alex! Attached new patch with whitespace cleaned





[CLJ-2055] binding-form spec parses symbol-only maps incorrectly Created: 08/Nov/16  Updated: 09/Dec/16

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

Type: Defect Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha14


Attachments: Text File CLJ-2055-01.patch    
Patch: Code
Approval: Screened

 Description   

The :clojure.core.specs/binding-form spec incorrectly treats some maps as sequential bindings.

Actual:

user=> (s/conform :clojure.core.specs/binding-form '{x y})
[:seq {:elems [[:seq {:elems [[:sym x] [:sym y]]}]]}]

Expected:

user=> (s/conform :clojure.core.specs/binding-form '{x y})
[:map {x y}]

Cause:

When there is no :keys, :strs, or :syms from :clojure.core.specs/map-special-binding, then :clojure.core.specs/seq-binding-form treats a map as sequential.

Proposed fix:

Include an (s/and vector? ...) check. See patch.

Patch: CLJ-2055-01.patch
Screened by: Alex Miller






[CLJ-2048] java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Ljava.lang.StackTraceElement Created: 21/Oct/16  Updated: 21/Oct/16

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

Type: Defect Priority: Major
Reporter: Gerrit Jansen van Vuuren Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-2048-b.patch    
Patch: Code
Approval: Prescreened

 Description   

clojure.core/throw-if creates an array to call Exception.setStracktrace() without specifying the array type. This works fine when passed at least one StackTraceElement, but in the case where passed no stack trace elements (all are filtered or stack traces are being elided by the JVM), this will be an Object[] which results in a ClassCastException:

Exception in thread "main" java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Ljava.lang.StackTraceElement;
        at clojure.core$throw_if.invokeStatic(core.clj:5649)
        at clojure.core$load_one.invokeStatic(core.clj:5698)
        at clojure.core$compile$fn__5682.invoke(core.clj:5903)
        at clojure.core$compile.invokeStatic(core.clj:5903)
        at clojure.core$compile.invoke(core.clj:5895)
        at clojure.lang.Var.invoke(Var.java:379)
        at clojure.lang.Compile.main(Compile.java:67)

This is tricky to reproduce because it involves stack trace filtering so there is no reproducing case here.

Patch: CLJ-2048-b.patch
Prescreened by: Alex Miller



 Comments   
Comment by Gerrit Jansen van Vuuren [ 21/Oct/16 4:18 AM ]

patch calls into-array with StackTraceElement type

Comment by Alex Miller [ 21/Oct/16 8:01 AM ]

How do you cause this to occur?

Comment by Alex Miller [ 21/Oct/16 8:11 AM ]

into-array will create a typed array based on the first element of the seq it is passed, so generally you should see a StackTraceElement[] here. I think the only time this would fail is if it was passed no stack trace elements.

Comment by Alex Miller [ 21/Oct/16 8:19 AM ]

I'd be happy to move this through screening, but the patch needs to be in the proper format (see http://dev.clojure.org/display/community/Developing+Patches).

Comment by Gerrit Jansen van Vuuren [ 21/Oct/16 8:42 AM ]

I'm trying to reproduce this in a way that can be presented here, but I got the compile error just after doing some serious package renaming, and can't reproduce it outside of the project itself.

Comment by Gerrit Jansen van Vuuren [ 21/Oct/16 8:45 AM ]

ok, I'll reformat the patch after reading (http://dev.clojure.org/display/community/Developing+Patches)

Comment by Gerrit Jansen van Vuuren [ 21/Oct/16 9:15 AM ]

I've created a new patch based on the guidelines, attached as file: CLJ-2048-b.patch.

Just to summarise:
The into-array returns the correct type if its provided with a none empty sequence, but if the sequence is empty it cannot know the type and then returns an object array. Because we set the array here to a java method Exception::setStackTrace passing it an object array causes a ClassCastException. One fix is to check for an empty sequence, but a less verbose way is just to pass the type which is known as part of the call to into-array, this is what is done in the patch CLJ-2048-b.patch.





[CLJ-2046] generate random subsets of or'd required keys in map specs Created: 17/Oct/16  Updated: 17/Oct/16

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

Type: Enhancement Priority: Major
Reporter: David Chelimsky Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: generator, spec

Attachments: Text File map-spec-gen-enhancements.patch    
Patch: Code and Test
Approval: Screened

 Description   

(s/keys :req [(or ::x ::y)]) always generates maps with both ::x and ::y but it should also generate maps with either ::x or ::y.

The attached patch supports arbitrarily deeply nested or and and expressions within the values of :req and :req-un in map specs. It also uses the same 'or' mechanism for :opt and :opt-un keys, thereby replacing the use of clojure.core/shuffle with clojure.test.check.generators/shuffle, ensuring repeatability of the generators.

Patch: map-spec-gen-enhancements.patch

Screened by: Alex Miller






[CLJ-2044] Four functions in clojure.instant have incomplete documentation Created: 15/Oct/16  Updated: 24/Oct/16

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

Type: Enhancement Priority: Major
Reporter: Bruce Adams Assignee: Bruce Adams
Resolution: Unresolved Votes: 0
Labels: docstring, instant

Attachments: Text File defns-for-instant-def-timestamp.patch     Text File defns-for-instant.patch    
Patch: Code
Approval: Prescreened

 Description   

Of the five public functions defined in clojure.instant, these four:

  • parse-timestamp
  • read-instant-calendar
  • read-instant-date
  • read-instant-timestamp

are each declared as a Var without any arglists metadata. This means documentation does not contain function calling information.

In http://clojure.github.io/clojure/clojure.instant-api.html, each of these functions is described as a var and there is no Usage: ... information given.

The output of doc does not include argument list information. For example:

user=> (doc clojure.instant/read-instant-date)
-------------------------
clojure.instant/read-instant-date
  To read an instant as a java.util.Date, bind *data-readers* to a map with
this var as the value for the 'inst key. The timezone offset will be used
to convert into UTC.

A related problem is that stack traces show anonymous functions instead of the names for any of these functions. For example:

user=> (clojure.instant/read-instant-timestamp "123")
RuntimeException Unrecognized date/time syntax: 123  clojure.instant/fn--6879/fn--6880 (instant.clj:107)

Proposed: Refactor the code into defn functions which makes the code clearer and addresses the documentation issue. An alternate approach would be to apply :arglists metadata to the vars.

Patch: defns-for-instant-def-timestamp.patch
Prescreened: Alex Miller



 Comments   
Comment by Bruce Adams [ 15/Oct/16 12:40 PM ]

Proposed solution: refactor the definitions of the four problematic functions to be defined using defn.

Comment by Bruce Adams [ 16/Oct/16 5:24 PM ]

Some of my thinking leading to the solution I propose.

Initially, when I realized that I didn't know what arguments parse-timestamp required, I assumed the appropriate fix was to enhance the docstring. Then I noticed that the on-line documentation for functions was formatted quite differently from the text output by doc. Any decent fix was going to have to feed function information into different variants of documentation formatting code.

I can guess, from other examples such as first, that :arglists metadata is what indicates that a var is a function. One possible solution would be to add :arglists to each of the four functions. It felt cleaner to refactor the code into simple defn functions. Refactoring code just for the side effect of documentation seems a bit odd, but the code itself strikes me as more legible after my refactoring.

Comment by Alex Miller [ 17/Oct/16 9:53 AM ]

This seems reasonable to me. I would move the timestamp regex into a separate (private) var instead of creating it in parse-timestamp.

It's possible the way these functions were defined was designed to minimize startup time or something like that, but I don't have any background on that.

Comment by Bruce Adams [ 23/Oct/16 4:06 PM ]

Updated patch based on Alex's great suggestion. This adds a separate, private, def for the timestamp regex.





[CLJ-2043] s/form of conformer is broken Created: 14/Oct/16  Updated: 14/Oct/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec

Attachments: Text File clj-2043.patch    
Patch: Code
Approval: Screened

 Description   

s/form of s/conformer is wrong:

(s/form (s/conformer str))
=> str

Proposed: Fix the form for conformer to match the conformer call.

Patch: clj-2043.patch






[CLJ-2040] Allow runtime modification of REPL exception handling Created: 11/Oct/16  Updated: 11/Oct/16

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File CLJ-2040-dynamic-repl-exceptions.patch    
Patch: Code
Approval: Triaged

 Description   

Problem Statement

Clojure's REPL is capable of paramterizing almost every aspect of its functionality, including how uncaught exceptions are printed. In the current implementation, these customization hooks are passed in as arguments and closed over, meaning that they cannot be changed once the REPL is started.

Many development tools want to override how the REPL handles uncaught errors. Examples of useful customizations include (but are not limited to):

  • Formatted exception messages (including whitespace and ANSI coloring)
  • Alternative representations for certain types of exceptions (e.g, Spec errors)
  • Dropping into a graphical interaction mode to better inspect ex-data.

Currently, this type of customization must be applied before a REPL is started, meaning that changing how a REPL displays errors requires support from (or plugins to) a third-party tool such as Boot or Leiningen.

Alternatives


1. Take no action.

Third-party tool support is required to create customized exception handling in the REPL. Tools have different techniques for doing this:

  • nREPL can intercept the exception on the wire and passes it through middleware
  • Leiningen plugins alter the root binding of clojure.main/repl-caught.
  • Boot allows users to build a task to invoke clojure.main/repl with the desired arguments.

Users will continue to select one of these according to their tooling preferences.

Benefits:
1. No effort or changes to the existing code.

Tradeoffs:
1. Tools will continue to implement their own diverse, sometimes hacky techniques for printing custom exceptions.
2. Any library intended to provide alternative exception handling will be tied to a specific launcher tool.

2. Make the REPL exception handler dynamically rebindable

If the REPL exception handler were a dynamic, thread-local var, users and libraries could change the behavior of the currently running REPL.

Benefits:
1. Users and libraries can freely override how exceptions are printed, regardless of how Clojure was launched.
2. Fully backwards compatible with existing tools.

Tradeoffs:
1. It will be possible for library authors to provide "bad" or poorly reasoned error printers. This is still possible with launch tools, but the barrier of entry is even lower with libraries.

The attached patch implements this option.

3. Encourage users to start new REPLs instead

In many Clojure environments, it's possible to explicitly launch a REPL from within another REPL. This sub-REPL could have the desired :caught hook.

Benefits:
1. No effort or changes to the existing code.
2. "Functionally pure", and in alignment with the evident design of the current REPL.

Tradeoffs:
1. There is a non-trivial subset of Clojure developers who do not know exactly how REPLs work. They are likely to be confused or subject to increased cognitive load. Insofar as this set of beginner/intermediate developers are precisely who enhanced error messages are meant to help in the first place, this solution is counterproductive.
2. For better or for worse, many existing and widely used tools do not support this. This does not work at all in nREPL, for example. However, even the simplest command-line REPLs behavior would change for the worse; sending a EOF (accidentally or otherwise) would always kill the sub-REPL with no feedback as to what just happened.






[CLJ-2037] specs in registry lack :file metadata despite having :line, :column Created: 08/Oct/16  Updated: 01/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Felix Andrews Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

As of 1.9.0-alpha13, specs in the registry lack :file metadata despite having :line, :column

user=> (require '[clojure.spec :as s])
user=> (-> (s/registry) (get :clojure.core.specs/arg-list) (meta))
{:line 1118, :column 5, :clojure.spec/name :clojure.core.specs/arg-list}
user=> (-> (s/registry) (get 'clojure.core/let) (meta))
{:line 1675, :column 5, :clojure.spec/name clojure.core/let}

This would be useful because:

  • we could list all the specs defined in a project, by filtering the registry.
  • we could read the source of a spec, like clojure.repl/source, for pretty formatting.

(specifically, for use in Codox https://github.com/weavejester/codox/pull/134 )

I had a quick look but couldn't see where the metadata is set.
Cheers



 Comments   
Comment by Alex Miller [ 08/Oct/16 11:12 AM ]

You can use s/describe or s/form to grab the source of a spec now, btw.

Comment by Felix Andrews [ 12/Oct/16 11:29 PM ]

The following works in my tests. (For testing I used in-ns, @#'registry-ref, #'ns-qualify)).

The approach is to set the registry item metadata after a def. It is not enough to set metadata on the def'd value because it is subsequently altered inside def.

(ns clojure.spec)
(alias 'c 'clojure.core)

(defmacro def
  [k spec-form]
  (let [k (if (symbol? k) (ns-qualify k) k)
        m (assoc (meta &form) :file *file*)]
    `(do
       (def-impl '~k '~(res spec-form) ~spec-form)
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

(defmacro fdef
  [fn-sym & specs]
  (let [k (ns-qualify fn-sym)
        m (assoc (meta &form) :file *file*)]
    `(do
       (clojure.spec/def ~fn-sym (clojure.spec/fspec ~@specs))
       (swap! registry-ref update '~k vary-meta c/merge ~m)
       '~k)))

You can use s/describe or s/form to grab the source of a spec now, btw.

Yes, that's nice except for longer specs when line wrapping and indentation would help.

Comment by Jozef Wagner [ 01/Dec/16 12:31 PM ]

Note that current :line and :column meta are not pointing to the place where the spec was defined but to the clojure/spec.clj file, e.g. second example (c.c/let) points to fspec-impl





[CLJ-2036] Generators for some? and any? only return collections or nil Created: 07/Oct/16  Updated: 11/Oct/16

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

Type: Defect Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec

Approval: Vetted

 Description   

Both of these should generate a better variety of values (strings, keywords, symbols, numbers) in addition to collections and nil.



 Comments   
Comment by Sean Corfield [ 07/Oct/16 11:55 AM ]

See also http://dev.clojure.org/jira/browse/TCHECK-111 which may solve this directly?

Comment by Gary Fredericks [ 09/Oct/16 2:08 PM ]

This is probably http://dev.clojure.org/jira/browse/TCHECK-83, which is fixed on test.check master.

Comment by Alex Miller [ 11/Oct/16 12:14 PM ]

I'm going to leave this open pending a new release and dependency update to test.check, which I presume will address it.





[CLJ-2035] Bad s/form for collection specs Created: 07/Oct/16  Updated: 14/Oct/16

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

Type: Defect Priority: Major
Reporter: Johan Gall Assignee: Alex Miller
Resolution: Unresolved Votes: 4
Labels: spec

Attachments: Text File clj-2035-2.patch     Text File clj-2035.patch    
Patch: Code and Test
Approval: Screened

 Description   

There are several problems with s/form for collection specs (coll-of,map-of,every,every-kv):

1. coll spec forms expose implementation details of building on every:

(s/form (s/coll-of int?))
=> (clojure.spec/every int? :clojure.spec/cpred #object[user$eval16$fn__18 0xd506900 "user$eval16$fn__18@d506900"] :clojure.spec/kind-form nil :clojure.spec/conform-all true)

2. form does not resolve nested spec preds:

(s/def ::a (s/every (s/tuple ::b)))

(s/form ::a)
=> (clojure.spec/every (*s/tuple* :user/b) [ ... ])

(which impacts map-of and coll-of).

3. :kind fn is not resolved

(s/form (s/coll-of int? :kind vector?))
=> (clojure.spec/every int? :clojure.spec/cpred #object[user$eval4$fn__6 0x8fc095 "user$eval4$fn__6@8fc095"] :clojure.spec/kind-form clojure.core/vector? :kind #object[clojure.core$vector_QMARK___6428 0x6596f6ef "clojure.core$vector_QMARK___6428@6596f6ef"] :clojure.spec/conform-all true)

Ignoring the rest of the problems from #1, the :kind should be here but should be the resolved form (clojure.core/vector?).

Patch: clj-2035-2.patch






[CLJ-2031] clojure.walk/postwalk does not preserve MapEntry type objects Created: 01/Oct/16  Updated: 27/Oct/16

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

Type: Defect Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: walk

Attachments: File clj-2031-w-test.diff     File clj-2031-w-test-v2.diff    
Patch: Code and Test
Approval: Prescreened

 Description   

This came up on Slack. A naïve implementation of "lispify" to turn vectors into lists used this code:

(defn lispify [s]
  (w/postwalk (fn [e] (if (vector? e) (apply list e) e)) s))

But when called like this:

(lispify [:html {:a "b"} ""])

It produces this error: java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.util.Map$Entry

My initial reaction was to change the condition to (and (vector? e) (not (map-entry? e))) but that still failed, because while walking the hash map, the MapEntry [:a "b"] was turned into a PersistentVector.

At this point, we can switch to using prewalk and it works as expected:

(defn lispify [s]
  (w/prewalk (fn [e] (if (and (vector? e) (not (map-entry? e))) (apply list e) e)) s))

Now we get the expected result:

boot.user> (lispify [:html {:a "b"} ""])
(:html {:a "b"} "")

This seems unintuitive at best and feels like a bug: postwalk should preserve the MapEntry type rather than converting it to a PersistentVector.

The problem seems to be this line https://github.com/clojure/clojure/blob/master/src/clj/clojure/walk.clj#L45:

(instance? clojure.lang.IMapEntry form) (outer (vec (map inner form)))

Would it be reasonable for this to become:

(instance? clojure.lang.IMapEntry form) (outer (clojure.lang.MapEntry/create (inner (first form)) (inner (second form)))))

This would preserve the type of the subelement.

Patch: clj-2031-w-test-v2.diff
Prescreened by: Alex Miller



 Comments   
Comment by Alex Miller [ 11/Oct/16 12:19 PM ]

seems reasonable

Comment by Jozef Wagner [ 27/Oct/16 8:19 AM ]

Added patch with test

Comment by Alex Miller [ 27/Oct/16 8:34 AM ]

Instead of the calls to .key and .val you should just call key and val.

Comment by Jozef Wagner [ 27/Oct/16 8:42 AM ]

Good catch, thanks! Added patch clj-2031-w-test-v2.diff that uses key and val instead.





[CLJ-2030] Auto-create alias namespaces Created: 28/Sep/16  Updated: 06/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: keywords, namespace, portability, spec

Attachments: Text File clj-2030-2.patch     Text File clj-2030-3-1.patch     Text File clj-2030-3-2.patch     Text File clj-2030.patch    
Patch: Code and Test
Approval: Screened

 Description   

It is useful to name keywords in namespaces, without creating or requiring those namespaces. When wanting to do that via an ::alias/keyword, the aliased namespace has to actually exist, in order to be aliased.
Currently, in Clojure, this can be achieved dynamically, through a combination of create-ns and alias, Clojurescript requires a dummy file and a :require :as.

Proposals:

1. Extend clojure.core/alias to auto-create missing namespaces
2. Extend clojure.core/alias to accept varargs & {:as kvs}
3. Extend ns to accept (:alias ...) clauses

Patch:

  • clj-2030.patch does 1 (but not 2 or 3), and was screened by SDH
  • clj-2030-2.patch does 1+2 (but not 3)
  • clj-2030-3-1.patch does 1+3 (but not 2)
  • clj-2030-3-2.patch does 1+2+3

Before:

user=> (alias 'parts 'company.domain.parts)
java.lang.Exception: No namespace: company.domain.parts found

After:

user=> (alias 'parts 'company.domain.parts)
nil
user=> ::parts/widget
:company.domain.parts/widget


 Comments   
Comment by Alex Miller [ 28/Sep/16 10:04 PM ]

From original description:

My use case is a simplification of data.xml, which would benefit greatly from a uniform way to alias + auto-create namespaces within the ns clause.

I would like to support the syntax:

(ns foo.bar
  (:alias xh  #xml/ns "http://..<xhtml>.."
          svg #xml/ns "http://..<svg>.."))

{:tag ::xh/div
 :content [{:tag ::svg/g}]}

see https://github.com/bendlas/data.xml/commit/22cbe21181175d302c884b4ec9162bd5ebf336d7

Comment by Alex Miller [ 28/Sep/16 10:08 PM ]

Thanks for filing this, it is something we've looked at a bit already. I simplified the description a bit and moved the use case and syntax to comments. I don't really understand the ns :alias example given in your syntax proposal but I think it's very unlikely we would go that far.

Comment by Herwig Hochleitner [ 29/Sep/16 3:55 AM ]

My syntax example could already be implemented, if alias had the proposed behavior and was available in an ns clause. In the linked commit, I implemented a scheme to encode xml namespaces in clojure namespaces, by using percent-encoding. I could easily provide that reader tag, if clojure and clojurescript provided the proposed extensions to alias and ns.

Comment by Alex Miller [ 29/Sep/16 8:53 AM ]

Yeah, I get it now (sleep!). I think the particular example is distracting to understand the enhancement request though.

Comment by Alex Miller [ 18/Oct/16 9:13 AM ]

moving this to vetted just so we don't lose track of it, but Rich has not actually ok'ed this for 1.9 yet

Comment by Herwig Hochleitner [ 06/Dec/16 10:50 PM ]

added patches for 3





[CLJ-2026] Transient exceptions thrown in clojure.spec.test/check Created: 21/Sep/16  Updated: 14/Oct/16

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

Type: Defect Priority: Major
Reporter: Coda Hale Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Java Virtual Machine 1.8
Clojure 1.9.0-alpha12
test.check 0.9.0


Attachments: Text File clj-2026.patch    
Patch: Code
Approval: Vetted

 Description   

So far I've seen two transient exceptions from running stest/check against some very simple functions:

First, while checking this spec:

(s/fdef str->long
        :args (s/cat :s (s/or :s string? :nil nil?))
        :ret (s/or :int int? :nil nil?)))

This exception was raised:

#error {
 :cause "Assert failed: Arg to one-of must be a collection of generators\n(every? generator? generators)"
 :via
 [{:type java.lang.AssertionError
   :message "Assert failed: Arg to one-of must be a collection of generators\n(every? generator? generators)"
   :at [clojure.test.check.generators$one_of invokeStatic "generators.cljc" 275]}]
 :trace
 [[clojure.test.check.generators$one_of invokeStatic "generators.cljc" 275]
  [clojure.test.check.generators$one_of invoke "generators.cljc" 264]
  [clojure.lang.AFn applyToHelper "AFn.java" 154]
  [clojure.lang.AFn applyTo "AFn.java" 144]
  [clojure.core$apply invokeStatic "core.clj" 657]
  [clojure.spec.gen$fn__13064$one_of__13067 doInvoke "gen.clj" 92]
  [clojure.lang.RestFn invoke "RestFn.java" 408]
  [clojure.spec$or_spec_impl$reify__13853 gen_STAR_ "spec.clj" 1060]
  [clojure.spec$gensub invokeStatic "spec.clj" 269]
  [clojure.spec$re_gen invokeStatic "spec.clj" 1565]
  [clojure.spec$re_gen$ggens__14185$gen__14186 invoke "spec.clj" 1554]
  [clojure.core$map$fn__6863 invoke "core.clj" 2739]
  [clojure.lang.LazySeq sval "LazySeq.java" 40]
  [clojure.lang.LazySeq seq "LazySeq.java" 49]
  [clojure.lang.RT seq "RT.java" 525]
  [clojure.core$seq__6397 invokeStatic "core.clj" 137]
  [clojure.core$every_QMARK_ invokeStatic "core.clj" 2652]
  [clojure.spec$re_gen invokeStatic "spec.clj" 1573]
  [clojure.spec$regex_spec_impl$reify__14229 gen_STAR_ "spec.clj" 1643]
  [clojure.spec$gensub invokeStatic "spec.clj" 269]
  [clojure.spec$gen invokeStatic "spec.clj" 275]
  [clojure.spec.test$quick_check$fn__13374 invoke "test.clj" 305]
  [clojure.spec.test$quick_check invokeStatic "test.clj" 305]
  [clojure.spec.test$check_1 invokeStatic "test.clj" 333]
  [clojure.spec.test$check$fn__13395 invoke "test.clj" 409]
  [clojure.core$pmap$fn__9360$fn__9361 invoke "core.clj" 6895]
  [clojure.core$binding_conveyor_fn$fn__6747 invoke "core.clj" 2020]
  [clojure.lang.AFn call "AFn.java" 18]
  [java.util.concurrent.FutureTask run "FutureTask.java" 266]
  [java.util.concurrent.ThreadPoolExecutor runWorker "ThreadPoolExecutor.java" 1142]
  [java.util.concurrent.ThreadPoolExecutor$Worker run "ThreadPoolExecutor.java" 617]
  [java.lang.Thread run "Thread.java" 745]]}

Second, while checking this spec:

(s/fdef percentage
        :args (s/cat :dividend nat-int? :divisor (s/and nat-int? pos?))
        :ret nat-int?)

This exception was thrown:

#error {
 :cause "Can't take value of a macro: #'clojure.test.check.random/bxoubsr"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.RuntimeException: Can't take value of a macro: #'clojure.test.check.random/bxoubsr, compiling:(clojure/test/check/random.clj:135:25)"
   :at [clojure.lang.Compiler analyze "Compiler.java" 6719]}
  {:type java.lang.RuntimeException
   :message "Can't take value of a macro: #'clojure.test.check.random/bxoubsr"
   :at [clojure.lang.Util runtimeException "Util.java" 221]}]
 :trace
 [[clojure.lang.Util runtimeException "Util.java" 221]
  [clojure.lang.Compiler analyzeSymbol "Compiler.java" 7124]
  [clojure.lang.Compiler analyze "Compiler.java" 6679]
  [clojure.lang.Compiler analyze "Compiler.java" 6656]
  [clojure.lang.Compiler$InvokeExpr parse "Compiler.java" 3766]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6920]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6906]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6906]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6906]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler analyze "Compiler.java" 6656]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 6029]
  [clojure.lang.Compiler$NewInstanceMethod parse "Compiler.java" 8345]
  [clojure.lang.Compiler$NewInstanceExpr build "Compiler.java" 7852]
  [clojure.lang.Compiler$NewInstanceExpr$DeftypeParser parse "Compiler.java" 7728]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6918]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler analyze "Compiler.java" 6656]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 6029]
  [clojure.lang.Compiler$LetExpr$Parser parse "Compiler.java" 6347]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6918]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler analyze "Compiler.java" 6656]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 6029]
  [clojure.lang.Compiler$FnMethod parse "Compiler.java" 5406]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 3972]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6916]
  [clojure.lang.Compiler analyze "Compiler.java" 6700]
  [clojure.lang.Compiler eval "Compiler.java" 6974]
  [clojure.lang.Compiler load "Compiler.java" 7429]
  [clojure.lang.RT loadResourceScript "RT.java" 374]
  [clojure.lang.RT loadResourceScript "RT.java" 365]
  [clojure.lang.RT load "RT.java" 455]
  [clojure.lang.RT load "RT.java" 421]
  [clojure.core$load$fn__7821 invoke "core.clj" 6008]
  [clojure.core$load invokeStatic "core.clj" 6007]
  [clojure.core$load doInvoke "core.clj" 5991]
  [clojure.lang.RestFn invoke "RestFn.java" 408]
  [clojure.core$load_one invokeStatic "core.clj" 5812]
  [clojure.core$load_one invoke "core.clj" 5807]
  [clojure.core$load_lib$fn__7766 invoke "core.clj" 5852]
  [clojure.core$load_lib invokeStatic "core.clj" 5851]
  [clojure.core$load_lib doInvoke "core.clj" 5832]
  [clojure.lang.RestFn applyTo "RestFn.java" 142]
  [clojure.core$apply invokeStatic "core.clj" 659]
  [clojure.core$load_libs invokeStatic "core.clj" 5889]
  [clojure.core$load_libs doInvoke "core.clj" 5873]
  [clojure.lang.RestFn applyTo "RestFn.java" 137]
  [clojure.core$apply invokeStatic "core.clj" 659]
  [clojure.core$require invokeStatic "core.clj" 5911]
  [clojure.core$require doInvoke "core.clj" 5911]
  [clojure.lang.RestFn invoke "RestFn.java" 436]
  [clojure.test.check.generators$eval40270$loading__7707__auto____40271 invoke "generators.cljc" 10]
  [clojure.test.check.generators$eval40270 invokeStatic "generators.cljc" 10]
  [clojure.test.check.generators$eval40270 invoke "generators.cljc" 10]
  [clojure.lang.Compiler eval "Compiler.java" 6977]
  [clojure.lang.Compiler eval "Compiler.java" 6966]
  [clojure.lang.Compiler load "Compiler.java" 7429]
  [clojure.lang.RT loadResourceScript "RT.java" 374]
  [clojure.lang.RT loadResourceScript "RT.java" 365]
  [clojure.lang.RT load "RT.java" 455]
  [clojure.lang.RT load "RT.java" 421]
  [clojure.core$load$fn__7821 invoke "core.clj" 6008]
  [clojure.core$load invokeStatic "core.clj" 6007]
  [clojure.core$load doInvoke "core.clj" 5991]
  [clojure.lang.RestFn invoke "RestFn.java" 408]
  [clojure.core$load_one invokeStatic "core.clj" 5812]
  [clojure.core$load_one invoke "core.clj" 5807]
  [clojure.core$load_lib$fn__7766 invoke "core.clj" 5852]
  [clojure.core$load_lib invokeStatic "core.clj" 5851]
  [clojure.core$load_lib doInvoke "core.clj" 5832]
  [clojure.lang.RestFn applyTo "RestFn.java" 142]
  [clojure.core$apply invokeStatic "core.clj" 659]
  [clojure.core$load_libs invokeStatic "core.clj" 5889]
  [clojure.core$load_libs doInvoke "core.clj" 5873]
  [clojure.lang.RestFn applyTo "RestFn.java" 137]
  [clojure.core$apply invokeStatic "core.clj" 659]
  [clojure.core$require invokeStatic "core.clj" 5911]
  [clojure.spec.gen$dynaload invokeStatic "gen.clj" 18]
  [clojure.spec.gen$fn__13223$fn__13224 invoke "gen.clj" 115]
  [clojure.lang.Delay deref "Delay.java" 37]
  [clojure.core$deref invokeStatic "core.clj" 2310]
  [clojure.spec.gen$fn__13223$simple_type_printable__13226 doInvoke "gen.clj" 115]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.spec.gen$fn__13280 invokeStatic "gen.clj" 131]
  [clojure.spec.gen$fn__13280 invoke "gen.clj" 130]
  [clojure.lang.Delay deref "Delay.java" 37]
  [clojure.core$deref invokeStatic "core.clj" 2310]
  [clojure.spec.gen$gen_for_pred invokeStatic "gen.clj" 191]
  [clojure.spec$spec_impl$reify__13794 gen_STAR_ "spec.clj" 877]
  [clojure.spec$gensub invokeStatic "spec.clj" 269]
  [clojure.spec$re_gen invokeStatic "spec.clj" 1565]
  [clojure.spec$re_gen$ggens__14185$gen__14186 invoke "spec.clj" 1554]
  [clojure.core$map$fn__6863 invoke "core.clj" 2739]
  [clojure.lang.LazySeq sval "LazySeq.java" 40]
  [clojure.lang.LazySeq seq "LazySeq.java" 49]
  [clojure.lang.RT seq "RT.java" 525]
  [clojure.core$seq__6397 invokeStatic "core.clj" 137]
  [clojure.core$every_QMARK_ invokeStatic "core.clj" 2652]
  [clojure.spec$re_gen invokeStatic "spec.clj" 1573]
  [clojure.spec$regex_spec_impl$reify__14229 gen_STAR_ "spec.clj" 1643]
  [clojure.spec$gensub invokeStatic "spec.clj" 269]
  [clojure.spec$gen invokeStatic "spec.clj" 275]
  [clojure.spec.test$quick_check$fn__13374 invoke "test.clj" 305]
  [clojure.spec.test$quick_check invokeStatic "test.clj" 305]
  [clojure.spec.test$check_1 invokeStatic "test.clj" 333]
  [clojure.spec.test$check$fn__13395 invoke "test.clj" 409]
  [clojure.core$pmap$fn__9360$fn__9361 invoke "core.clj" 6895]
  [clojure.core$binding_conveyor_fn$fn__6747 invoke "core.clj" 2020]
  [clojure.lang.AFn call "AFn.java" 18]
  [java.util.concurrent.FutureTask run "FutureTask.java" 266]
  [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 was unable to reproduce either exception during consequent runs.

Cause: See further investigation in the comments - this appears to be caused by the pmap in check triggering concurrent requires of the test.check.generators namespace.

Approach: Add locking to prevent concurrent loads in dynaload.

Patch: clj-2026.patch



 Comments   
Comment by Alex Miller [ 21/Sep/16 1:27 PM ]

On the first one, you should use this instead:

(s/fdef str->long
        :args (s/cat :s (s/nilable string?))
        :ret (s/nilable int?))

s/nilable is performance optimized, works better as a generator, and is shorter.

I'll look into the failures though.

Comment by Gary Fredericks [ 21/Sep/16 9:18 PM ]

The second error seemed crazy spooky, and the only thing I could imagine was that it was a problem that would manifest itself any time compiling clojure.test.check.random, but only occasionally.

So I decided to just continually compile that namespace in a loop and see what would happen. After ~30 minutes I got this, which is not obviously related as far as I can tell:

Exception in thread "main" java.lang.ExceptionInInitializerError, compiling:(clojure/test/check/random.clj:16:1)
        at clojure.lang.Compiler.load(Compiler.java:7441)
        at clojure.lang.RT.loadResourceScript(RT.java:374)
        at clojure.lang.RT.loadResourceScript(RT.java:365)
        at clojure.lang.RT.load(RT.java:455)
        at clojure.lang.RT.load(RT.java:421)
        at clojure.core$load$fn__7821.invoke(core.clj:6008)
        at clojure.core$load.invokeStatic(core.clj:6007)
        at clojure.core$load.doInvoke(core.clj:5991)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at user$eval5.invokeStatic(NO_SOURCE_FILE:1)
        at user$eval5.invoke(NO_SOURCE_FILE:1)
        at clojure.lang.Compiler.eval(Compiler.java:6977)
        at clojure.lang.Compiler.eval(Compiler.java:6940)
        at clojure.core$eval.invokeStatic(core.clj:3187)
        at clojure.main$eval_opt.invokeStatic(main.clj:290)
        at clojure.main$eval_opt.invoke(main.clj:284)
        at clojure.main$initialize.invokeStatic(main.clj:310)
        at clojure.main$null_opt.invokeStatic(main.clj:344)
        at clojure.main$null_opt.invoke(main.clj:341)
        at clojure.main$main.invokeStatic(main.clj:423)
        at clojure.main$main.doInvoke(main.clj:386)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
Caused by: java.lang.ExceptionInInitializerError
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:526)
        at java.lang.Class.newInstance(Class.java:383)
        at clojure.lang.Compiler$ObjExpr.eval(Compiler.java:4939)
        at clojure.lang.Compiler.eval(Compiler.java:6976)
        at clojure.lang.Compiler.eval(Compiler.java:6966)
        at clojure.lang.Compiler.load(Compiler.java:7429)
        ... 23 more
Caused by: java.lang.ClassNotFoundException: clojure.test.check.random.IRandom
        at java.net.URLClassLoader$1.run(URLClassLoader.java:366)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:355)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:354)
        at clojure.lang.DynamicClassLoader.findClass(DynamicClassLoader.java:69)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:425)
        at clojure.lang.DynamicClassLoader.loadClass(DynamicClassLoader.java:77)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:358)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:278)
        at clojure.lang.RT.classForName(RT.java:2183)
        at clojure.lang.RT.classForName(RT.java:2192)
        at clojure.test.check.random$eval1059936.<clinit>(random.clj:16)
        ... 32 more
Comment by Kevin Downey [ 22/Sep/16 12:49 AM ]

The Random thing seems like it might be the issue that was fixed here (https://github.com/clojure/clojure/commit/2ac93197e356af3c826ca895b5a538ad08c5715) for other constructs, creating a class and having it get gc'ed before it can be used.

Comment by Colin Jones [ 22/Sep/16 7:56 AM ]

Here's a fairly small repro case that I got to throw the same error as that second one (once), with some comments in the test file noting various ways in which the failures seem to go away: https://github.com/trptcolin/spec-race-repro

I've seen all of the following errors on a `lein test` of the linked project:

  • Wrong number of args (0) passed to: dynaloadable/asdf
  • Var spec-race.dynaloadable/asdf-consumer is not on the classpath
  • Can't take value of a macro: #'spec-race.dynaloadable/asdf-consumer

This last one was by far the rarest - only seen once, over many minutes of running. But both the first and last are errors related to confusing whether `asdf` is a function or a macro.

I'm reasonably confident it comes down to dynaload / require'ing the same file concurrently. Locking the dynaload require, eager loading all to-be-dynaloaded nses before adding concurrency, and just avoiding concurrency all appear work without issues. In the interest of keeping things flexible & letting consumers do what they want, I'd personally lean towards the locking approach (maybe striping per-file), but hopefully this repro case at least helps us study the issue more.

Comment by Alex Miller [ 22/Sep/16 8:39 AM ]

Just a note of thanks for those that have looked at this so far - thanks! Certainly concurrent requires during dynaload sounds like a reasonable candidate. The only source of concurrency that I'm aware of is the pmap inside `check` (presuming there is not something concurrent in the original testing environment).

Comment by Stuart Halloway [ 14/Oct/16 12:41 PM ]

If we are going to add locking, why not add it to ns and require in core? Wouldn't all users of require want these better semantics, or is that too expensive for general use?

See also http://dev.clojure.org/jira/browse/CLJ-1406.

Comment by Stuart Halloway [ 14/Oct/16 12:41 PM ]

If we are going to add locking, why not add it to ns and require in core? Wouldn't all users of require want these better semantics, or is that too expensive for general use?

See also http://dev.clojure.org/jira/browse/CLJ-1406.





[CLJ-2021] case where spec/conform -> spec/unform -> spec/conform gives invalid result Created: 12/Sep/16  Updated: 12/Sep/16

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

Type: Defect Priority: Major
Reporter: Jeroen van Dijk Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec
Environment:

clojure 1.9, mac osx, java 1.8



 Description   

The example belows shows a case where a conform-ed form, does not conform any after an unform. It would be my expectation that you can repeat conform -> unform -> conform endlessly and get the same result.

(require '[clojure.core.specs])
(require '[clojure.spec :as s])

(s/def ::defn-macro (s/cat :type #

Unknown macro: {'defn}
:definition :clojure.core.specs/defn-args))

(let [form '(defn foo "bar" ([a & b] a a c) ([a b] a))]

(-> form
(->> (s/conform ::defn-macro))) ;;=> {:type defn, :definition {:name foo, :docstring "bar", :bs [:arity-n {:bodies [{:args {:args [[:sym a]], :varargs {:amp &, :form [:sym b]}}, :body [a a c]} {:args {:args [[:sym a] [:sym b]]}, :body [a]}]}]}}

;; Unforming returns the function definition, but with the args in a list instead of a vector:
(->> form
(s/conform ::defn-macro)
(s/unform ::defn-macro)) ;;=> (defn foo "bar" ((a (& b)) a a c) ((a b) a)))

;; Conforming after unforming doesn't work anymore
(->> form
(s/conform ::defn-macro)
(s/unform ::defn-macro)
(s/conform ::defn-macro)) ;;=> :clojure.spec/invalid

)



 Comments   
Comment by Jeroen van Dijk [ 12/Sep/16 8:22 AM ]

This gist shows the above code with better formatting https://gist.github.com/jeroenvandijk/28c6cdd867dbc9889565dca92673a531





[CLJ-2019] Loosen constraint between key name and spec name in clojure.spec/keys Created: 04/Sep/16  Updated: 19/Sep/16

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

Type: Enhancement Priority: Major
Reporter: Peter Schuck Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec


 Description   

According to the Clojure Spec Rationale

Most systems for specifying structures conflate the specification of the key set (e.g. of keys in a map, fields in an object) with the specification of the values designated by those keys. I.e. in such approaches the schema for a map might say :a-key's type is x-type and :b-key's type is y-type. This is a major source of rigidity and redundancy.

Currently clojure.spec/keys does exactly the complecting the rationale says is a major source of rigidity and redundancy. clojure.spec/keys requires that any keys in it's key set have the name the have in the spec registry. For example:

(ns my.ns
  (:require
    [clojure.spec :as spec]))

(spec/def ::x-type integer?)
(spec/def ::y-type bool?)

;;The only map that can be checked for is {::x-type <x-type> ::y-type <y-type>}
(spec/def ::my-map (spec/keys :req [::x-type ::y-type]))

;;To validate a map like {::a-key <x-type> ::b-key <y-type>} You need to do this
(spec/def ::a-key ::x-type)
(spec/def ::b-key ::y-type)
(spec/def ::my-map (spec/keys :req [::a-key ::b-key]))

What clojure.spec/keys should allow you to do is this

(ns my.ns
  (:require
    [clojure.spec :as spec]))

(spec/def ::x-type integer?)
(spec/def ::y-type bool?)

;;The key set is now independent of the spec's names. You can validate
;;a map like {::a-key <x-type> ::b-key <y-type>}
(spec/def ::my-map (spec/keys :req {::a-key ::x-type ::b-key ::y-type}))

The exact implementation may vary from what I show here but the end result should be allowing users to check for x-type under ::a-key with out having to do the redundant step of (clojure.spec/def ::a-key (clojure.spec/spec <x-type>).



 Comments   
Comment by Laszlo Török [ 19/Sep/16 4:34 PM ]

Spec advocates to use namespaced keys to convey contextual semantics of the value.

Relevant quote from the Spec guide:

"These maps represent various sets, subsets, intersections and unions of the same keys, and in general ought to have the same semantic for the same key wherever it is used."

There may be different pieces of information that end up having the same representation (e.g. both are of type integer).

The ::x-type vs ::a-key nomenclature above is misleading. One should rather look a keyword-value pair in a map as an attribute-value pair, where you can specify the valid values of that attribute using a spec.





[CLJ-2017] with-gen should specify if the generator should return conformed or unformed data Created: 03/Sep/16  Updated: 04/Sep/16

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

Type: Enhancement Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, spec


 Description   

I think the answer is "unformed", but this isn't very clear from the docstring.



 Comments   
Comment by Alex Miller [ 03/Sep/16 6:46 PM ]

The answer is definitely unconformed. Conforming only happens when you call conform. This doesn't seem confusing to me, but maybe it should be clearer. I suspect it would be better to clarify this in a reference documentation page though.

Comment by lvh [ 04/Sep/16 10:29 AM ]

I agree that a reference documentation change would be most helpful.

I rely heavily on my environment showing me docstrings, so a small point (maybe just unconformed/unformed/whatever in parens) in the docstring would still be helpful.





[CLJ-2015] with-instrument Created: 29/Aug/16  Updated: 30/Aug/16

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

Type: Enhancement Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec

Approval: Triaged

 Description   

Right now, instrument and unstrument are great for unconditional instrumentation for tests and for development. I also want to run instrument for just a particular piece of code. For example, I want a test with some stubs or some overrides. Right now I need to instrument and unstrument; I'd prefer to have a with-instrument macro that does the obvious try/finally block for me.



 Comments   
Comment by Alex Miller [ 30/Aug/16 2:30 PM ]

So (like most things), obvious things aren't.

There are several ways to call instrument:

  • (instrument)
  • (instrument sym)
  • (instrument [syms])
  • (instrument sym opts)
  • (instrument [syms] opts)

The number there is variable. Similarly, a "body" is typically also variadic in other with-style macros. Parsing those two variadic things is ambiguous.

You mentioned the opts map, so I'm assuming you'd want that as an option. So you could narrow the args to: [sym-or-syms opts & body]. Not sure whether you've then introduced things you don't need in common cases and ruined the usefulness of the macro.

(with-instrument `my-fun {my-opts ...} (test-something))

would expand to

(do
  (instrument user/my-fun)
  (try
    (test-something)
    (finally
      (unstrument user/my-fun))))

There are maybe interesting things to think about with how much you take into account what's already instrumented. Do you unstrument what you instrument, or do you try to return the instrumentation to what it was before (where some stuff may already have been instrumented)?

Comment by Daniel Solano Gómez [ 30/Aug/16 3:24 PM ]

So, here's the implementation I have been using, which isn't necessarily the one to use, but I think it helps with some of the ambiguity with respect to arguments:

(defmacro with-instrumentation
  [args & body]
  `(let [[arg1# arg2#] ~args
         [sym-or-syms# opts#] (cond
                                (nil? arg1#) [(stest/instrumentable-syms) arg2#]
                                (map? arg1#) [(stest/instrumentable-syms) arg1#]
                                :default     [arg1# arg2#])]
     (try
       (stest/instrument sym-or-syms# opts#)
       ~@body
       (finally
         (stest/unstrument sym-or-syms#)))))

It's not perfect, but it has served me well enough.

The question of what happens at the end is a very good one. Ideally, with-instrumentation would have stack-like semantics where instrumentation would return to its previous state. Is that something that can be done with spec?





[CLJ-2013] spec doesn't explain failing path of a s/cat with purely optional branches Created: 24/Aug/16  Updated: 12/Oct/16

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

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, spec
Environment:

alpha11


Approval: Triaged

 Description   

In an s/cat with two optional regex branches, e. g. via s/? or s/*, spec doesn't explain their individual problems, but the whole spec as failed.

(s/explain (s/cat :begin (s/? (s/cat :num number?))
                  :end (s/* #{:foo}))
           [:bar])

In: [0] val: (:bar) fails predicate: (cat :begin (? (cat :num number?)) :end (* #{:foo})),  Extra input

Spec does not explain the full optional paths that failed, but instead explains that the s/cat spec failed as a whole.

If one forces spec down into one branch, it explains the error at the deepest possible path and explains the failing predicate

(s/explain (s/cat :begin (s/? (s/cat :num number?))
               ;; :end (s/* #{:foo})
                  )
           [:bar])
In: [0] val: :bar fails at: [:begin :num] predicate: number?

An interesting case is if one makes the second branch non-optional

(s/explain (s/cat :begin (s/? (s/cat :num number?))
                  :end #{:foo})
           [:bar])
In: [0] val: :bar fails at: [:end] predicate: #{:foo}

It does not explain why the first branch has failed as a potential option, but only the second. This makes sense from the perspective that it successfully parses the :begin branch as legally non-existent and then explains a detailed failure on the second one. However it omits valuable information in real world use cases as shown in https://groups.google.com/d/msg/clojure/mIlKaOiujlo/tF71zZ2BCwAJ .

Desired behavior would be at least that if all branches are optional in a cat and all fail they are all reported.

At most that if a cat fails but an optional branch was parsed as non-existent it is retried without being allowed to be parsed as non-existent.



 Comments   
Comment by Nuttanart Pornprasitsakul [ 12/Oct/16 8:43 AM ]

I'll put a somewhat different issue here because I think it has the same root cause.

It should specifically say :ret of fspec is missing but it says failing at :args.

(require '[clojure.spec :as s])
(require '[clojure.spec.test :as st])

(defn x [f] (f 1))

(s/fdef x
  :args (s/cat :f (s/fspec :args (s/cat :i int?))))

(st/instrument `x)

(x (fn [a] a))
Exception in thread "main" clojure.lang.ExceptionInfo: Call to #'user/x did not conform to spec:
In: [0] val: (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]) fails at: [:args] predicate: (cat :f (fspec :args (cat :i int?))),  Extra input
:clojure.spec/args  (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "debug.clj", :line 16, :var-scope user/eval20}
 {:clojure.spec/problems [{:path [:args], :reason "Extra input", :pred (cat :f (fspec :args (cat :i int?))), :val (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :via [], :in [0]}], :clojure.spec/args (#object[user$eval20$fn__21 0x3e521715 "user$eval20$fn__21@3e521715"]), :clojure.spec/failure :instrument,
...




[CLJ-2011] clojure.walk.macroexpand-all will not properly expand macros that depend on &env Created: 23/Aug/16  Updated: 23/Aug/16

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

Type: Defect Priority: Major
Reporter: Collin Bell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: macro, walk
Environment:

MacOSX, Clojure 1.9.0-alpha10, Java 1.8.0_45, CIDER 0.13.0snapshot (package: 20160602.809), nREPL 0.2.12



 Description   

(clojure.walk/macroexpand-all '(defn foo [a] (go [] a)))

Unhandled clojure.lang.ExceptionInfo
Could not resolve var: a
{:var a}

This is because go depends on &env and macroexpand-all does not handle &env.

The reason this issue is important is because it breaks the cider debugger for async.






[CLJ-2007] if-let* & when-let* with multiple bindings implementation Created: 21/Aug/16  Updated: 21/Aug/16

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

Type: Enhancement Priority: Major
Reporter: Ertuğrul Çetin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Patch: Code

 Description   

I think Clojure programs will be more elegant if we use let versions of if & when with multiple bindings.

;; if-let* imp.

(defmacro if-let*
  "Multiple binding version of if-let"
  ([bindings then]
   `(if-let* ~bindings ~then nil))
  ([bindings then else]
   (when (seq bindings)
     (assert-args
       (vector? bindings) "a vector for its binding"
       (even? (count bindings)) "exactly even forms in binding vector"))
   (if (seq bindings)
     `(if-let [~(first bindings) ~(second bindings)]
        (if-let* ~(vec (drop 2 bindings)) ~then ~else)
        ~(if-not (second bindings) else))
     then)))

;;Example if-let*

(if-let* [a 1
          b (+ a 1) ]
          b)

;;=> 2

(if-let* [a 1
          b (+ a 1)
          c false] ;;false or nil - does not matter
          b
          a)

;;=> 1

;; when-let* imp.

(defmacro when-let*
  "Multiple binding version of when-let"
  [bindings & body]
  (when (seq bindings)
    (assert-args
      (vector? bindings) "a vector for its binding"
      (even? (count bindings)) "exactly even forms in binding vector"))
  (if (seq bindings)
    `(when-let [~(first bindings) ~(second bindings)]
       (when-let* ~(vec (drop 2 bindings)) ~@body))
    `(do ~@body)))

;;Example when-let*

  (when-let* [a 1 
             b 2 
             c (+ a b)]
             (println "yeah!")
             c)
  ;;=>yeah!
  ;;=>3

  (when-let* [a 1 
             b nil 
             c 3]
             (println "damn! b is nil")
             a)
  ;;=>nil





[CLJ-2003] Nesting cat inside ? causes unform to return nested result Created: 11/Aug/16  Updated: 08/Nov/16

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

Type: Defect Priority: Major
Reporter: Sam Estep Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec

Approval: Triaged

 Description   

Calling conform and then unform with a spec that consists of some cat nested inside of some ? creates an extra level of nesting in the result:

(require '[clojure.spec :as s])

(let [spec (s/? (s/cat :foo #{:foo}))
      initial [:foo]
      conformed (s/conform spec initial)
      unformed (s/unform spec conformed)]
  [initial conformed unformed])
;;=> [[:foo] {:foo :foo} [(:foo)]]

This behavior does not occur with just ? or cat alone:

(let [spec (s/? #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> [:foo]

(let [spec (s/cat :foo #{:foo})]
  (s/unform spec (s/conform spec [:foo])))
;;=> (:foo)


 Comments   
Comment by Phil Brown [ 14/Aug/16 9:55 PM ]

I came across another case of extra nesting, when repeating one or more sequences with an optional element at the beginning or end, where that element's predicate also matches the element at the other end:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} [{:k :b, :v 2}]]

where I expected

[{:k :a, :v 1} {:k :b, :v 2}]

The following give expected results:

user=> (s/conform (s/+ (s/cat :k any? :v (s/? any?))) [:a 1 :b])
[{:k :a, :v 1} {:k :b}]
user=> (s/conform (s/+ (s/cat :k keyword? :v (s/? int?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
user=> (s/conform (s/* (s/cat :k any? :v (s/? any?))) [:a 1 :b 2])
[{:k :a, :v 1} {:k :b, :v 2}]
Comment by Alex Miller [ 01/Sep/16 11:06 AM ]

Phil, I think your example is a different issue and you should file a new jira for that.

Comment by Alex Miller [ 01/Sep/16 3:05 PM ]

Well, maybe I take that back, they may be related.

Comment by Brandon Bloom [ 08/Nov/16 6:10 PM ]

I just ran in to this trying to make sense of some defn forms. Here's an example:

user=> (s/unform :clojure.core.specs/defn-args (s/conform :clojure.core.specs/defn-args '(f [& xs])))
(f ((& xs)))





[CLJ-2002] StackOverflowError in clojure.spec Created: 11/Aug/16  Updated: 26/Aug/16

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

Type: Defect Priority: Major
Reporter: Michiel Borkent Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

In this example a non-conforming value is passed to conform, which should return ::s/invalid but instead throws StackOverflow.

(s/conform (s/* (s/alt :n (s/* number?) :s (s/* string?))) [[1 2 3]])

CompilerException java.lang.StackOverflowError, compiling:(/Users/alex/code/clojure.spec/src/spec/examples/tree.clj:44:1)
	clojure.lang.Compiler.load (Compiler.java:7415)
	user/eval2674 (form-init3668332544888233146.clj:1)
	user/eval2674 (form-init3668332544888233146.clj:1)
	clojure.lang.Compiler.eval (Compiler.java:6951)
	clojure.lang.Compiler.eval (Compiler.java:6914)
	clojure.core/eval (core.clj:3187)
	clojure.core/eval (core.clj:3183)
	clojure.main/repl/read-eval-print--9692/fn--9695 (main.clj:241)
	clojure.main/repl/read-eval-print--9692 (main.clj:241)
	clojure.main/repl/fn--9701 (main.clj:259)
	clojure.main/repl (main.clj:259)
	clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--675 (interruptible_eval.clj:69)
Caused by:
StackOverflowError 
	clojure.spec/deriv (spec.clj:1296)
	clojure.spec/deriv (spec.clj:1311)
	clojure.spec/deriv/fn--13794 (spec.clj:1312)
	clojure.core/map/fn--6680 (core.clj:2728)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:525)
	clojure.core/seq--6221 (core.clj:137)
	clojure.core/map/fn--6687 (core.clj:2736)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:525)


 Comments   
Comment by Phil Brown [ 14/Aug/16 9:50 PM ]

While the following isn't super useful, it causes one too:

user=> (s/conform (s/+ (s/? any?)) [:a])

StackOverflowError   clojure.lang.RT.first (RT.java:683)




[CLJ-2001] Invalid conversion from BigDecimal to long using clojure.core/long Created: 09/Aug/16  Updated: 09/Aug/16

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

Type: Defect Priority: Major
Reporter: Eugene Aksenov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math
Environment:

Ubuntu Linux 15


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

 Description   

Trying to convert from BigDecimal to long

(long 201608081812113241M)
=> 201608081812113248                  ;; not really our number

let's just use BigDecimal.longValue()

(.longValue 201608081812113241M)
=> 201608081812113241                  ;; ok, correct value

looking into clojure.lang.RT and suspecting incorrect conversion chain

(.longValue (.doubleValue 201608081812113241M))
=> 201608081812113248                  ;; yep, incorrect

Cause: long cast from BigDecimal will use Number.longValue(), which in this case produces an incorrect value even though the conversion is possible. The javadoc indicates that this call is equivalent to a double to long conversion and is potentially lossy in several ways.

Approach: add explicit case in long cast to handle BigDecimal and instead call longValueExact(). Patch adds additional cast tests for some BigInteger and BigDecimal values. The unchecked-long cast does not seem to be affected (returned the proper value with no changes).

Questions: while it may be confusing, the incorrect result may actually be the one that is consistent with Java. unchecked-long would give the expected result and may be the better choice for the example here. So it's possible that we should NOT apply this patch and instead do nothing. If we do move forward with the patch, we may want to also apply an equivalent change to call byteValueExact(), shortValueExact(), intValueExact(), and toBigIntegerExact() in the appropriate places as well.

Patch: clj-2001.patch



 Comments   
Comment by Alex Miller [ 09/Aug/16 8:14 AM ]

Yeah, RT.longCast() doesn't seem to explicitly handle BigDecimal.

Comment by Ghadi Shayban [ 09/Aug/16 10:07 AM ]

Patch seems like it may negatively affect inlining

Comment by Alex Miller [ 09/Aug/16 7:36 PM ]

Indeed that's a possibility, although I think it's probably rare in this case.





[CLJ-1997] Macros cannot reliably detect usage of locals Created: 02/Aug/16  Updated: 02/Aug/16

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: macro


 Description   

Problem

The motivating problem is the implementation of gen/let in test.check (see also TCHECK-98).

A common usage of gen/let might look something like this:

(gen/let [a gen-a
          b gen-b]
  (f a b))

The crucial characteristic of this code is that the generator for b does not depend on the value a (though in general it could). Because of this independence, the ideal expansion is:

(gen/fmap 
  (fn [[a b]] (f a b)) 
  (gen/tuple gen-a gen-b))

However, because gen/let cannot, in general, tell whether or not the expression for the generator for b depends on a, it needs to fallback to a more general expansion:

(gen/fmap
  (fn [[a b]] (f a b))
  (gen/bind 
    gen-a
    (fn [a]
      (gen/tuple (gen/return a) gen-b))))

Using gen/bind greatly reduces shrinking power, and so it's best to avoid it when possible.

A knowledgeable user could get around this by using gen/tuple explicitly, e.g.:

(gen/let [[a b] (gen/tuple gen-a gen-b)]
  (f a b))

But I think most users would prefer not to have to think about these things.

Possible Solutions

tools.analyzer

tools.analyzer is probably adequate, but is a large dependency for a library.

a subset of tools.analyzer

Nicola has mentioned the idea of carving out some subset of the analyzer that would be sufficient for this case, and that might be the best option.

a mechanism for macroexpanding a macro body

I believe if there were a robust mechanism for a macro to fully macroexpand an expression that this problem would be easier (clojure.core/macroexpand and friends have a few known incorrectnesses) – a simple tree-seq over the expanded expression could prove that a local is not used (though a naive approach might falsely conclude that a local *is* used, which might be an acceptable compromise for the test.check case, and otherwise a robust code walker should not be difficult to implement on expanded code).

I believe zach's riddley library does something like this, and depending on riddley would probably be the best option for a non-contrib library, but is not an acceptable dependency for a contrib library.






[CLJ-1996] clojure.spec stubs don't cooperate with clojure.spec.test/check Created: 31/Jul/16  Updated: 31/Jul/16

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

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


 Description   

This is just like CLJ-1949, but for stubs instead of higher-order-function arguments.

The solution is more difficult, though, since cst/check and cst/instrument can be called/used seperately.

My only idea is to have a dynamic var where the two can coordinate. Stubs would use gen/generate when not called during testing, but in the context of a call to cst/check the dynamic var would contain an alternate implementation that works similarly to the patch in CLJ-1949.

I'd be happy to prepare a patch with that implementation (or any other) if desired.






[CLJ-1986] Suppress printing namespace map literal syntax when only one namespaced key Created: 21/Jul/16  Updated: 21/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: maps, print

Attachments: Text File clj-1986.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

Really an aesthetic choice, but right now maps with only a single namespaced key are printed in namespace map literal syntax:

user=> {:my.ns/b 1}
#:my.ns{:b 1}

And that seems unnecessarily complicated (and longer).

Proposal: Only print namespace map literal syntax when >1 key is using the same namespace.

Patch: clj-1986.patch






[CLJ-1980] Unable to construct gen in indirectly recursive specs with s/every and derivations Created: 12/Jul/16  Updated: 26/Aug/16

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

alpha-10


Approval: Triaged

 Description   

Problem statement: Some spec implementations return no generator but nil, in their gen* implementation when their recursion-limit has been reached (e. g. s/or). Specs that implement composition of other specs sometimes respect getting no generator from other specs gen* and adjust behavior of their own gen* accordingly, sometimes to the extent of returning nothing themselves (e. g. s/or's gen* returns nil if of all of its branches specs also don't have a gen and otherwise uses only those gens it got). However, there are various specs that don't respect getting no generator from gen* (like s/every, s/map-of) and they are essential building blocks in many real world recursive specifications. They then end up throwing an exception "Unable to construct gen ...".

Here is a minimal example (not real world usecase illustration) of the problem with actual specs:

;; A ::B is an s/or with branches going through ::B recursively
(s/def ::B (s/or :A ::A))

;; An ::A is a map of keywords to ::Bs (or it is empty as recursive termination)

(s/def ::A (s/map-of keyword? ::B
                     :gen-max 3))

(gen/sample (s/gen ::A))

ExceptionInfo Unable to construct gen at: [1 :A 1 :A 1 :A 1 :A 1] for: :spec.examples.tree/B  clojure.core/ex-info (core.clj:4725)

Valid values for the spec above (I can mail you a real usecase that enforces above pattern in which we parse an internal query DSL) are: {}, {:a {}}, {:foo {:bar {}}} etc.

The problem why the current implementation of spec fails to generate values for above spec is that ::A's map-of doesn't generate an empty map when ::B's gen* returns nil, but instead throws an exception. s/every and all derived specs are affected by this and there might be others.

Proposed fix: A spec's gen* impl must always respect other spec's gen* returning nil not by throwing but by either adjusting the returned gen or by returning nil itself so that the not-returning-gen behavior propagates back to the caller where an exception should be thrown instead.






[CLJ-1978] recursion-limit not respected Created: 08/Jul/16  Updated: 19/Oct/16

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

Type: Defect Priority: Major
Reporter: Maarten Truyens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   

(Also see closed http://dev.clojure.org/jira/browse/CLJ-1964)

(require '[clojure.spec :as s])
(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)))
(s/exercise ::map-tree)

hangs on my machine.

Another example from https://groups.google.com/forum/#!topic/clojure/IvKJc8dEhts, which immediately results in a StackOverflowError on my machine:

(require '[clojure.spec.gen :as gen])

(defrecord Tree [name children])
(defrecord Leaf [name])

(s/def ::name string?)
(s/def ::children (s/coll-of (s/or :tree ::Tree, :leaf ::Leaf)))

(s/def ::Leaf (s/with-gen
                (s/keys :req-un [::name])
                #(gen/fmap (fn [name] (->Leaf name)) (s/gen ::name))))

(s/def ::Tree (s/with-gen
                (s/keys :req-un [::name ::children])
                #(gen/fmap
                   (fn [[name children]] (->Tree name children))
                   (s/gen (s/tuple ::name ::children)))))

;; occasionally generates but usually StackOverflow
(binding [s/*recursion-limit* 1]
    (gen/generate (s/gen ::Tree)))

StackOverflowError 
	clojure.lang.RT.seqFrom (RT.java:533)
	clojure.lang.RT.seq (RT.java:527)
	clojure.core/seq--6221 (core.clj:137)
	clojure.core/map/fn--6687 (core.clj:2736)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:525)
	clojure.core/seq--6221 (core.clj:137)
	clojure.core/every? (core.clj:2652)
	clojure.spec/tuple-impl/reify--13509 (spec.clj:905)
	clojure.spec/gensub (spec.clj:228)
	clojure.spec/gen (spec.clj:234)


 Comments   
Comment by Leon Grapenthin [ 12/Jul/16 1:03 PM ]

As the author of CLJ-1964 I can't confirm this.

(binding [s/*recursion-limit* 1]
  (s/exercise ::map-tree))

... immediately generates.

Using the new :gen-max argument spec can also generate with a higher recursion limit in reasonable time

(s/def ::map-tree (s/map-of keyword? (s/or :tree ::map-tree :leaf nil?)
                            :gen-max 3))
(time (s/exercise ::map-tree))
"Elapsed time: 0.135683 msecs"

Note that :gen-max defaults to 20, so with 4 recursion steps this quickly ends up generating 20^5 3.2 million values

Comment by Alex Miller [ 26/Aug/16 11:31 AM ]

I tried this again today and the first example still works just fine for me. I'm using Java 1.8 with default settings in a basic Clojure repl (not lein).

Comment by Maarten Truyens [ 19/Oct/16 4:32 AM ]

With the :gen-max option, everything works now. Thanks for the suggestion!





[CLJ-1975] clojure.spec attempts to make `empty` records Created: 05/Jul/16  Updated: 23/Sep/16

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   
user> (require '[clojure.spec :as s])
nil
user> (defrecord Box [a])
user.Box
user> 
user> (s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer?))
        [(Box. 0) [5]])
UnsupportedOperationException Can't create empty: user.Box  user.Box (form-init8049111656025227309.clj:1)
user> (clojure.repl/pst *e)
UnsupportedOperationException Can't create empty: user.Box
       	user.Box (NO_SOURCE_FILE:2)
	clojure.core/empty (core.clj:5151)
	clojure.spec/every-impl/cfns--14008/fn--14014 (spec.clj:1215)
	clojure.spec/every-impl/reify--14027 (spec.clj:1229)
	clojure.spec/conform (spec.clj:150)
	clojure.spec/dt (spec.clj:731)
	clojure.spec/dt (spec.clj:727)
	clojure.spec/deriv (spec.clj:1456)
	clojure.spec/deriv (spec.clj:1463)
	clojure.spec/deriv (spec.clj:1467)
	clojure.spec/re-conform (spec.clj:1589)
	clojure.spec/regex-spec-impl/reify--14267 (spec.clj:1633)

This is a regression from -alpha7; the same sort of spec (modulo the default-value arg to `coll-of`) works as expected there.






[CLJ-1968] clojure.test/report :error does not flush *out* when the test fails with an exception Created: 23/Jun/16  Updated: 23/Jun/16

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

Type: Defect Priority: Major
Reporter: Sam Roberton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Approval: Triaged

 Description   

Minimal reproduction:

(require 'clojure.test)

(clojure.test/deftest foo-test
  (throw (ex-info "I fail" {})))

(clojure.test/deftest bar-test
  (.println System/out "bar"))

(clojure.test/test-vars [#'foo-test #'bar-test])

Result:

ERROR in (foo-test) (core.clj:4617)
Uncaught exception, not in assertion.
expected: nil
bar
  actual: clojure.lang.ExceptionInfo: I fail
 at clojure.core$ex_info.invokeStatic (core.clj:4617)
...

Note "bar" appearing in the output in the middle of the error report for foo-test.

Analysis:

(clojure.test/report {:type :error, :actual some-exception}) calls stack/print-cause-trace. Unlike other clojure.test/report callpaths, this does not flush on newline. Thus, when tests fail with exceptions and there is anything writing directly to Java's System.out, there can be a large gap between the first part of the error report and the exception trace.

(To explain why this is annoying: we're running Selenium tests via clj-webdriver, and our system under test is logging with log4j via clojure.tools.logging. We invariably see dozens or even hundreds of lines between "expected: ..." and the subsequent "actual: ..." exception trace. This makes it very easy to come to completely the wrong conclusion about when failures occurred with respect to the other events that appear interleaved in the log.)

It would be preferable (in my opinion) if clojure.test/report always constructed the output from each individual invocation into a single string which got written to *out* all at once – that way there could be no way for output to be interleaved from other threads. Absent that, it would at least help a lot if the :error implementation called (flush).






[CLJ-1966] :clojure.spec/invalid is not a valid :clojure.spec/any value Created: 21/Jun/16  Updated: 12/Sep/16

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

Type: Defect Priority: Major
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

(clojure.spec/valid? :clojure.spec/any :clojure.spec/invalid) returns false

This issue gets serious, if one likes to write specs for core functions like = which are used by spec itself. I observed this bug as I wrote a spec for assoc.

A possible solution could be to use an (Object.) sentinel internally and :clojure.spec/invalid only at the API boundary. But I have not thought deeply about this.



 Comments   
Comment by Alexander Kiel [ 24/Jun/16 9:48 AM ]

I have another example were the described issue arises. It's not possible to test the return value of a predicate suitable for conformer, because it should return :clojure.spec/invalid itself.

(ns coerce
  (:require [clojure.spec :as s]))

(s/fdef parse-long
  :args (s/cat :s (s/nilable string?))
  :ret (s/or :val int? :err #{::s/invalid}))

(defn parse-long [s]
  (try
    (Long/parseLong s)
    (catch Exception _
      ::s/invalid)))
Comment by Alexander Kiel [ 12/Jul/16 10:01 AM ]

No change in alpha 10 with the removal of :clojure.spec/any and introduction of any?.

Comment by Sean Corfield [ 12/Sep/16 4:06 PM ]

Another example from Slack, related to this:

(if-let [a 1]
  ::s/invalid)

Fails compilation (macroexpansion) because ::s/invalid causes the spec for if-let to think the then form is non-conforming.

Workaround:

(if-let [a 1]
  '::s/invalid)




[CLJ-1959] adding functions `map-vals` and `map-keys` Created: 14/Jun/16  Updated: 21/Jul/16

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

Type: Enhancement Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File map-mapper.patch     Text File map-mapper-v2.patch     Text File map-mapper-v3.patch    
Patch: Code
Approval: Triaged

 Description   

Many people have been writing a function to map values in HashMap:

Proposal: Add `map-keys` and `map-values` which: maps keys in HashMap, and map values in HashMap. They return HashMap as a result.

Workaround: Using function `reduce-kv` or ordinary `map` and `into` is a common solution, but they are confusing and types change, which makes it tricky and tedious.

Discussions: https://groups.google.com/forum/#!topic/clojure-dev/kkPYIl5qj0o



 Comments   
Comment by Hiroyuki Fudaba [ 14/Jun/16 11:22 AM ]

code and test for map-keys and map-vals

Comment by Nicola Mometto [ 14/Jun/16 1:05 PM ]

I propose those functions being called `update-vals` and `update-keys` rather than `map-vals` and `map-keys`

Comment by Alex Miller [ 14/Jun/16 2:03 PM ]

It's not worth bike-shedding names on this - Rich will have his own opinion regardless.

On the patch:

  • remove the :static metadata, that's not used anymore
  • needs docstrings, which should be written in the style of other Clojure docstrings. map is probably a good place to draw from.
  • rather than declare into, defer the definition of these till whatever it needs has been defined. There is no reason to add more declares for this.

There are other potential implementations - these should be implemented and compared for performance across a range of input sizes. In addition to the current approach, I would investigate:

  • reduce-kv with construction into a transient map. This allows the map to reduce itself (no seq caching needed) and avoid creating entries only to tear them apart again.
  • transducers with (into {} (map ...) m)

Also should consider

  • whether to build a k/v vector and convert to a map, or build a map directly (the former may be faster, not sure)
  • if building the map, how to construct the map entries (vector vs creating a mapentry object directly)
  • in map-keys, is there any open question when map generates new overlapping keys?
  • are there places in existing core code where map-keys/map-vals could be used (I am pretty certain there are)
Comment by Hiroyuki Fudaba [ 15/Jun/16 11:01 AM ]

Thanks for comments

> I propose those functions being called `update-vals` and `update-keys` rather than `map-vals` and `map-keys`
Maybe. But I name it `map-*` just for now, we can choose it later

about potential implementations:
I have tried several implementations, and seems to be the current implementation is the fastest.
You can see it here: https://github.com/delihiros/performance

about considerings:
> whether to build a k/v vector and convert to a map, or build a map directly (the former may be faster, not sure)
> are there places in existing core code where map-keys/map-vals could be used (I am pretty certain there are)
> if building the map, how to construct the map entries (vector vs creating a mapentry object directly)
I'll check which them as soon as possible. I haven't done it yet.

> in map-keys, is there any open question when map generates new overlapping keys?
I believe it should be overwritten by latter applied key and value.

Comment by Nathan Marz [ 15/Jun/16 11:35 AM ]

I've done quite a bit of investigation into this through building Specter. Here are some benchmarks of numerous ways of doing map-vals, including using Specter.

Code: https://github.com/nathanmarz/specter/blob/4778500e0370fb211f47ebf4d69ca64366117b6c/scripts/benchmarks.clj#L87
Results: https://gist.github.com/nathanmarz/bf571c9ed86bfad09816e17b9b6e59e3

A few comments:

  • Implementations that build and tear apart MapEntry's perform much worse.
  • Transients should be used for large maps but not for small ones.
  • This benchmark shows that the property of maintaining the type of the map in the output can be achieved without sacrificing performance (the test cases using Specter or "empty" have this property).
Comment by Hiroyuki Fudaba [ 11/Jul/16 3:27 AM ]

I've modified the implementation. It should be faster than before.

Comment by Steve Miner [ 20/Jul/16 10:46 AM ]

Implementations that call reduce-kv are not lazy so the documentation should be clarified in the proposed patch (map-mapper-v3.patch). Also, it's probably better to say "map" (as the noun) rather than to specify a particular concrete type "hash map".

Comment by Nicola Mometto [ 21/Jul/16 4:30 AM ]

map->map operations can't be lazy either way. Even if one implementation used lazy operations to iterate over the original map, the `into {}` would realize it later.





[CLJ-1954] clojure.set/intersection mishandles vectors Created: 09/Jun/16  Updated: 09/Jun/16

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

Type: Defect Priority: Major
Reporter: Ashton Kemerling Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: set


 Description   

clojure.set/intersection appears to use the indexes of vectors as values. This results in very strange behavior if you accidentally end up passing a vector in as one of the arguments.

ti.repl-init=> (clojure.set/intersection #{0 1} [2 2 2 2 2])
#{0 1}
ti.repl-init=> (clojure.set/intersection [2 2 2 2] #{0 1})
#{0 1}
ti.repl-init=> (clojure.set/intersection [0 1] [2 2 2 2])
[0 1]
ti.repl-init=> (clojure.set/intersection [2 2 2 2] [2 2 2 2])
[2 2 2 2]
ti.repl-init=> (clojure.set/intersection [3 3 3 ] [2 2 2 2])
[3 3 3]
ti.repl-init=> (clojure.set/intersection [55] [2 2 2 2])

ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1476)

If any of the arguments are lists, you get a ClassCastException which is maybe a bit less clear than one would hope.

ti.repl-init=> (clojure.set/intersection #{0 1} (list 2 2 2 2))

IllegalArgumentException contains? not supported on type: clojure.lang.PersistentList  clojure.lang.RT.contains (RT.java:814)

The same also happens if all arguments are lists:



 Comments   
Comment by Ashton Kemerling [ 09/Jun/16 9:44 AM ]

More odd side effects.

ti.repl-init=> (clojure.set/intersection #{:foo} {:foo 1})
#{:foo}
ti.repl-init=> (clojure.set/intersection #{:foo} {})
{}
ti.repl-init=> (clojure.set/intersection #{:foo} [:foo])
#{}
ti.repl-init=> (clojure.set/intersection [:foo] [:foo])

ClassCastException clojure.lang.PersistentVector cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1476)
ti.repl-init=> (clojure.set/intersection [0] [:foo])
[0]
Comment by Alex Miller [ 09/Jun/16 9:54 AM ]

See comments on CLJ-1953





[CLJ-1953] clojure.set should check or throw on non-set inputs Created: 09/Jun/16  Updated: 09/Jun/16

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

Type: Defect Priority: Major
Reporter: Ashton Kemerling Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: set
Environment:

Not Relevant



 Description   

clojure.set/union is very sensitive to the types of its inputs. It does not attempt to check or fix the input types, raise an error, or even document this behavior.

If all inputs are sets, it works.

ti.repl-init=> (clojure.set/union #{1 2 3} #{1 2 3 4})
#{1 4 3 2}

If the arguments are both vectors or sequences, it returns the same type with duplicates.

ti.repl-init=> (clojure.set/union [1 2 3] [1 2 3])
[1 2 3 1 2 3]
ti.repl-init=> (clojure.set/union (list 1 2 3) (list 1 2 3))
(3 2 1 1 2 3)

If the arguments are mixed, the correct result is returned only if the longest input argument is a set.

ti.repl-init=> (clojure.set/union #{1 2 3} [2 3])
#{1 3 2}
ti.repl-init=> (clojure.set/union [1 2 3] #{2 3})
[1 2 3 3 2]
ti.repl-init=> (clojure.set/union [2 3] #{1 2 3})
#{1 3 2}
ti.repl-init=> (clojure.set/union #{2 3} [1 2 3])
[1 2 3 3 2]


 Comments   
Comment by Alex Miller [ 09/Jun/16 9:40 AM ]

This has been raised a number of times. See CLJ-1682, CLJ-810.

Comment by Ashton Kemerling [ 09/Jun/16 9:52 AM ]

I do not see set/union being covered in the tickets you mentioned.

Furthermore, this issue differs from the intersection bugs in a few ways important ways:

  1. It silently returns data that is the wrong type, and which contains the wrong values.
  2. It never raises an exception.

But it does share the following bugs with the intersection problem:

  1. This behavior is not only type dependent, but data dependent. It will happen to work depending on the lengths of the given sets.
  2. It isn't even documented that this function expects sets.
  3. It runs directly contrary to the definition of the mathematical function it purports to represent.

I only caught this bug in my own code because I hand inspected the result. I had just assumed that set/union would do the right thing, and was deeply surprised when against both definition and documentation it did not.

Comment by Andy Fingerhut [ 09/Jun/16 11:07 AM ]

I am sympathetic to your desires, Ashton, but have no new arguments that might convince those who decide what changes are made to Clojure that it would be a good enough idea to do so.

I would point out an answer to one of your comments: "It isn't even documented that this function expects sets." It seems to me from past comments that the point of view of the Clojure core team is that this is documented, e.g. "Return a set that is the union of the input sets" tells you what clojure.set/union does when you give it sets as arguments. It specifies nothing about what it does when you give it non-set arguments, so it is free to do anything at all in those cases, including what it currently does.





[CLJ-1949] Generator for fspec is not deterministic & ignores sizing Created: 05/Jun/16  Updated: 26/Aug/16

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

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec

Attachments: Text File CLJ-1949-impure.patch     Text File CLJ-1949-pure.patch    
Patch: Code
Approval: Triaged

 Description   

Problem

One of the goals of test.check is for users to be able to write arbitrarily rich generators while maintaining determinism, which has obvious benefits for reproducing failures.

Currently the fspec generator generates a function which itself generates random return values by calling clojure.test.check.generators/generate, which is a function intended only for development use as it circumvents test.check's controlled source of psuedorandomness. It also circumvents test.check's sizing mechanism, since the generate function always uses a size of 30.

Possible Solutions

I see two reasonable solutions to this, depending on whether the generated function ought to be a pure function (which it currently isn't, since it ignores its arguments and randomly generates a return value).

Pure Function

We can generate a non-empty vector of possible return values and use that to create a function that selects one of the possible return values using the hash of the arguments.

Impure Function

We can generate a non-empty collection of possible return values and use that to create a function with internal state that cycles through the possible return values.



 Comments   
Comment by Gary Fredericks [ 05/Jun/16 5:44 PM ]

Added a patch for each of the approaches listed. Would be happy to add tests too if feedback is given about either approach being preferred.





[CLJ-1942] Add predicate for sequential search in a collection Created: 02/Jun/16  Updated: 05/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File has-predicate.patch    
Approval: Triaged

 Description   

Many people have been writing a predicate of their own to find whether a sequence contains an item or not.

Proposal: Add a predicate (similar to `clojure.string/includes?`) that checks whether a sequential collection contains a value by doing a sequential search.

Workaround: Using function `some` is a common solution, but is confusing for beginners and can be tricky if searching for nil or false. Using .contains or other methods directly is another solution but in that case, we need to think about the class of sequence.

Discussions: https://groups.google.com/forum/#!topic/clojure-dev/dIO-Ee9XOZY






[CLJ-1929] Can't typehint literal collection to avoid reflection on Java interop call Created: 16/May/16  Updated: 18/May/16

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

Type: Defect Priority: Major
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, reflection, typehints
Environment:

OS X 10.11.4


Attachments: Text File 0001-CLJ-1929-preserve-type-hints-in-literals.patch    
Patch: Code
Approval: Triaged

 Description   

There is a reflection warning when passing a Clojure collection to a method that has a parameter of a collections interface type like java.util.Map.

Example calling java.time.format.DateTimeFormatterBuilder.appendText(java.time.temporal.TemporalField, java.util.Map):

(import 'java.time.format.DateTimeFormatterBuilder
        'java.time.format.TextStyle
        'java.time.temporal.ChronoField)

(set! *warn-on-reflection* true)

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR {}))
; Reflection warning, NO_SOURCE_PATH:6:3 - call to method appendText on java.time.format.DateTimeFormatterBuilder can't be resolved (argument types: java.time.temporal.ChronoField, clojure.lang.IPersistentMap).

The map literal cannot be hinted:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR ^java.util.Map {}))
; Reflection warning, NO_SOURCE_PATH:8:3 - call to method appendText on java.time.format.DateTimeFormatterBuilder can't be resolved (argument types: java.time.temporal.ChronoField, clojure.lang.IPersistentMap).

The warning does not appear when the map is not empty:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR {1 "a"}))

Nor does it appear on similar methods where there is no overloaded method with the same arity:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendZoneText builder TextStyle/FULL #{}))

Workaround is to not use a literal:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR ^java.util.Map (array-map)))

It should be possible to infer in these cases like elsewhere that {} implements java.util.Map.

If that is not viable a type hint on {} should be honored.

Approach: preserve user hints in literal collections
Patch: 0001-CLJ-1929-preserve-type-hints-in-literals.patch






[CLJ-1921] Wrong numeric result from Math/abs on Java 8 Created: 09/May/16  Updated: 10/May/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math, reflection
Environment:

does not seem specific to Clojure version
occurs only in Java 1.8



 Description   

This is with Java 1.8 (Oracle or Open JDK):

weird-abs.core=> (Math/abs -10000000000000)
10000000000000
weird-abs.core=> (def a    -10000000000000)
#'weird-abs.core/a
weird-abs.core=> (Math/abs a)
1316134912

In Java 1.7, the expected results are returned instead (10000000000000).

Cause: It appears that Math.abs(int) is being invoked. Both the int and long versions are considered by the reflector but Java 1.7 and 1.8 return these signatures in different orders and the first one found is picked.

Workaround: Use hint or cast to inform the reflector which one to pick.



 Comments   
Comment by Alex Miller [ 09/May/16 9:03 AM ]

In the first case, -10000000000000 is a long and the compiler unambiguously finds Math.abs(long).

In the second case, a is an Object and all abs signatures are considered (this is in Reflector.invokeMatchingMethod). In both Java 1.7 and 1.8, the long and int signatures are found "congruent".

In Java 1.7, the long version is found first and treated as a match, then int is checked and Compiler.subsumes([int], [long]) returns false, leading to the long method being kept as the match.

In Java 1.8, the int version is found first and treated as a match, then long is checked and Compiler.subsumes([long], [int]) returns false, leading to the int method being kept as the match.

Both of these return false on both JDKs:

(Compiler/subsumes (into-array [Long/TYPE]) (into-array [Integer/TYPE]))
(Compiler/subsumes (into-array [Integer/TYPE]) (into-array [Long/TYPE]))

so the real difference is just the ordering that is considered, which is JDK-specific.

The considered signatures could be sorted in some canonical way making this behavior consistent, or could maybe express a preference between the two signatures somehow.

In any case, getting rid of the reflection here by hinting or casting a resolves the problem - it should be considered only luck not intention that the correct results comes out with Java 7 in this case, I think.

Comment by Nicola Mometto [ 10/May/16 7:58 AM ]

It seems to me that the non-deterministic behaviour of clojure's reflector of randomly picking one overload when more than one is available is both highly counterintuitive and undesirable.

IMHO the only sane approach would be to:

  • pick the most specific type if possible (e.g. if what's available is [Object, CharSequence, String] and the reflected type is a StringBuffer, we use CharSequence rather than Object.
  • pick the widest primitive type if possible (e.g. in this case we'd use long rather than int)
  • Fail with a `More than one matching method found` exception if conflicts can't be resolved (this already happens in some cases)

(I'm still scarred from previous experiences of reading/patching the complex beast that is Reflector.java and the reflective bits of Compiler.java, so I propose this with no intention of ever working on this myself )

Comment by Alex Miller [ 10/May/16 8:03 AM ]

I think the subsumes check is effectively trying to do your option #1 already - this is a case where the types of the arguments in the two cases have no hierarchical relationship. Probably #2 would make more sense - expressing a preference, although there are certainly cases where "widest" has no meaning, so not sure what the general form of this is.

Comment by Nicola Mometto [ 10/May/16 8:05 AM ]

To clarify, that wasn't a list of different options, it was a list of steps to take.
i.e. if it's possible to pick the most specific type from a hierarchy, do that, ELSE if the types are primitive, pick the widest ELSE fail





[CLJ-1920] Create an easy way to gracefully shutdown agents Created: 03/May/16  Updated: 03/May/16

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

Type: Enhancement Priority: Major
Reporter: Ruslan Al-Fakikh Assignee: Ruslan Al-Fakikh
Resolution: Unresolved Votes: 1
Labels: agents


 Description   

Currently there is no easy way to shutdown agents while making sure all the submitted actions are completed and no new actions are sent.

Here is the naive approach:

(shutdown-agents)

There are two problems with that:
1) It will discard all the actions that have already been submitted, but haven't been started.
2) It won't prohibit from sending further actions to agents (no explicit error will be thrown, just silent ignoring).

Here is the proof:

(def my-agent (agent 1))

(defn sleep-and-inc [number]
  (Thread/sleep 3000)
  (println "action number" number "complete")
  (inc number))

(println "sending off 2 times")
(send-off my-agent sleep-and-inc)
(send-off my-agent sleep-and-inc)
(println "sending off complete")

(shutdown-agents)
(println "shutdown requested")

(println "sending off a 3rd time")
(send-off my-agent sleep-and-inc)
(println "sending off complete")

Here is the output:

sending off 2 times
sending off complete
shutdown requested
sending off a 3rd time
sending off complete
action number 1 complete

As you can see - the 2nd action got discarded, the 3rd action got ignored.

And btw, the shutdown-agents' docstring is misleading (not clear):
"...Running actions will complete, but no new actions will be accepted"
1) It doesn't say anything about already submitted actions
2) "no new actions will be accepted" sounds like there should be an error, but it's silently ignored.
So, the better docstring should be "...Running actions will complete, waiting actions will be discarded and new actions will be ignored"

A similar naive approach works perfectly well in Java:

ExecutorService executor = Executors.newSingleThreadExecutor();

        executor.submit(new Runnable() {
            @Override
            public void run() {
                try {
                    Thread.sleep(3000);
                } catch (InterruptedException e) {
                    throw new RuntimeException(e);
                }
                System.out.println("Action 1 complete");
            }
        });
        executor.submit(new Runnable() {
            @Override
            public void run() {
                try {
                    Thread.sleep(3000);
                } catch (InterruptedException e) {
                    throw new RuntimeException(e);
                }
                System.out.println("Action 2 complete");
            }
        });

        executor.shutdown();
        System.out.println("Shutdown requested");

//        //will throw RejectedExecutionException
//        executor.submit(new Runnable() {
//            @Override
//            public void run() {
//                try {
//                    Thread.sleep(3000);
//                } catch (InterruptedException e) {
//                    throw new RuntimeException(e);
//                }
//                System.out.println("Action 3 complete");
//            }
//        });

Output:

Shutdown requested
Action 1 complete
Action 2 complete

By "perfectly well" I mean:
1) It will complete all the waiting tasks (not just running)
2) It will throw an error on a new task after "shutdown" was called.

So, back to Clojure - currently we are only left with this idiom (not trivial!):

(await my-agent)
(shutdown-agents)

It is not a trivial and straightforward idiom, because:
1) You need to keep track of all the agents in the system. Becomes close to impossible if you are dealing with third-party code that uses agents.
2) Still doesn't even throw an exception if you happen to send another action while waiting or shutting down.

Proposal
(inspired by Java):
1) Create a new function called "shutdown-agents-gracefully" which will do 2 additional things:
1.1) Put the agents system to "shutting down" state
1.2) Completes the running actions as well as the waiting actions
2) Modify "send" and "send-off" so that they throw an error in case the agent system is in "shutting down" state.
3) Fix the docstring of "shutdown-agents" (see above)

I'll start developing a patch when this jira ticket is validated.






[CLJ-1903] Provide a transducer for reductions Created: 17/Mar/16  Updated: 08/Sep/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: 2
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.

Comment by Pierre-Yves Ritschard [ 24/May/16 6:40 AM ]

Is there anything I can help to move this forward?
@alexmiller any comments on the code itself?

Comment by Alex Miller [ 24/May/16 7:31 AM ]

Haven't had a chance to look at it yet, sorry.

Comment by Pierre-Yves Ritschard [ 24/May/16 7:36 AM ]

@alexmiller, if the upshot is getting clojure.spec, I'll take this taking a bit of time to review

Comment by Steve Miner [ 25/May/16 3:21 PM ]

For testing, I suggest you compare the output from the transducer version to the output from a simliar call to the sequence reductions. For example,

(is (= (reductions + 3 (range 20)) (sequence (reductions-with + 3) (range 20)))

I would like to see that equality hold. The 0002 patch doesn't handle the init the same way the current Clojure reductions does.

Comment by Pierre-Yves Ritschard [ 07/Sep/16 4:29 PM ]

@alexmiller I'm tempting one more nudge to at least get an idea on the patch and the reductions-with variant since 1.9 seems to be getting closer to a release.

Comment by Alex Miller [ 08/Sep/16 10:43 AM ]

Sorry, don't know that I'll get to it soon or that it will be considered for 1.9. I also don't know that it won't, just ... don't know.

Comment by Pierre-Yves Ritschard [ 08/Sep/16 10:48 AM ]

@alexmiller, Thanks for the prompt reply. I'm trying to make sure i'll be around when feedback comes to be able to act quickly on it. Cheers!





[CLJ-1901] amap calls `alength` at every iteration step Created: 13/Mar/16  Updated: 24/Apr/16

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

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

JVM


Attachments: Text File fix_amap.patch    
Patch: Code
Approval: Prescreened

 Description   

During the 1.7 => 1.8 upgrade `areduce` was fixed to not call `alength` on the same thing, at every single iteration step. However, `amap` which suffers from the same issue was not fixed (even though exactly the same fix applies).

Example:

(def an-array (long-array 100000 0))
(dotimes [_ 50]
  (time (amap ^longs an-array idx ret (+ 1 (aget ^longs an-array idx)))))

Before (last time): 0.3930 ms
After (last time): 0.3459 ms

Patch: fix_amap.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 13/Mar/16 4:39 PM ]

Thanks!

Comment by Dimitrios Piliouras [ 24/Apr/16 1:33 PM ]

Not a problem. I actually noticed a very similar thing in the `internal-reduce` implementation for StringSeq [1]. The `.length()` method is called on the same String on every single iteration step, even though it is a constant. Is that easy enough to be sorted without me submitting another trivial patch? Thanks in advance...

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

Comment by Alex Miller [ 24/Apr/16 1:48 PM ]

Separate ticket would be preferred, thanks.

Comment by Dimitrios Piliouras [ 24/Apr/16 2:32 PM ]

Sure thing, I'll create it now.





[CLJ-1899] Add function transform-keys to clojure.walk Created: 08/Mar/16  Updated: 14/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Rafal Szalanski Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: walk
Environment:

OS X, Java 8, Clojure 1.8


Attachments: Text File clj1899.patch     Text File clj1899-review1.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

In CLJ-1894 I proposed a patch to change clojure.walk/stringify-keys to include namespace if keywords use namespaces. I made a wrong assumption about backwards compatibility of that change, however I still think the behaviour is not exactly what it should be.

Interesting thing Alex Miller pointed out in his comment to CLJ-1894 is that stringify-keys and keywordize-keys are essentially the same function with a different transformation. I think having one function which does a deep transformation of map keys using a transformation supplied by user is a good idea and it could be used to simplify some Clojure libraries.

Proposal:

  • add clojure.walk/transform-keys to walk a map and transform all keys
  • use transform-keys in clojure.walk/stringify-keys & clojure.walk/keywordize-keys

Patch: clj1899-review1.patch

Screened by: Alex Miller



 Comments   
Comment by Rafal Szalanski [ 08/Mar/16 6:37 AM ]

CLJ-1899 patch

Comment by Alex Miller [ 10/Mar/16 9:20 AM ]

In the patch, transform-keys should take the arguments in the reverse order [m f] - generally for any function that is collection -> collection, the collection should be the first arg.

Comment by Rafal Szalanski [ 14/Mar/16 9:34 AM ]

CLJ-1899 patch addressing issues pointed by Alex miller.





[CLJ-1898] Inconsistent duplicate check in set/map literals with quoted/unquoted equal constants Created: 06/Mar/16  Updated: 01/Dec/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: 1
Labels: collections, compiler

Attachments: Text File clj-1898.patch    
Patch: Code and Test
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



 Comments   
Comment by Jozef Wagner [ 01/Dec/16 3:38 PM ]

Attached patch with tests.





[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: 1
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-1895] Remove loading of clojure.string in clojure.java.io Created: 22/Feb/16  Updated: 22/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: performance

Attachments: Text File clj-1895.patch    
Patch: Code
Approval: Prescreened

 Description   

clojure.core loads clojure.java.io to define slurp and spit. clojure.java.io loads clojure.string, solely for a single call to replace. This slows down Clojure core startup for no reason.

Approach: Replace clojure.string/replace call with a Java interop call to .replace. This saves about 18 ms during Clojure core startup.

Patch: clj-1895.patch






[CLJ-1888] AReference#meta() is synchronized Created: 26/Jan/16  Updated: 16/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Roger Kapsi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: PNG File aref-meta-after.png     PNG File aref-meta.png     Text File clj-1888-2.patch     Text File clj-1888.patch    
Patch: Code
Approval: Prescreened

 Description   

We use Clojure for a "rules engine". Each function represents a rule and metadata describes the rule and provides some static configuration for the rule itself. The system is immutable and concurrent.

If two or more Threads invoke the same Var concurrently they end up blocking each other because AReference#meta() is synchronized (see attached screenshot, the red dots).

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Approach: Replace synchronized block with a rwlock for greater read concurrency. This approach removes meta read contention (see real world example in comments). However, it comes with the downsides of:

  • extra field for every AReference (all namespaces, vars, atoms, refs, and agents)
  • adds construction of lock into construction of AReference (affects perf and startup time)

Patch: clj-1888-2.patch replaces synchronized with a rwlock for greater read concurrency

Alternatives:

  • Use volatile for _meta and synchronized for alter/reset. Allow read of _meta just under the volatile - would this be safe enough?
  • Extend AReference from ReentrantReadWriteLock instead of holding one - this is pretty weird but would have a different (potentially better) footprint for memory/construction.


 Comments   
Comment by Alex Miller [ 26/Jan/16 10:19 PM ]

A volatile is not sufficient in alterMeta as you need to read/update/write atomically.

You could however use a ReadWriteLock instead of synchronized. I've attached a patch that does this - if you have a reproducible case I'd be interested to see how it affects what you see in the profiler.

There are potential issues that would need to be looked at - this will increase memory per reference (the lock instance) and slow down construction (lock construction) at the benefit of more concurrent reads.

Comment by Roger Kapsi [ 27/Jan/16 8:34 AM ]

Hey Alex,

I do have a reproducible case. The blocking has certainly disappeared after applying your patch (see attached picture). The remaining blocking code on these "WorkerThreads" is sun.nio.ch.SelectorImpl.select(long) (i.e. not clojure related).

You can repro it yourself by executing something like the code below concurrently in an infinite loop.

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Suggestions for the patch: Make the meta lock a final field and maybe pull the read/write locks into local variables to avoid the double methods calls.

alterMeta(...)
  Lock w = _metaLock.writeLock();
  w.lock();
  try {
    // ...
  } finally {
    w.unlock();
  }
}
Comment by Alex Miller [ 16/Mar/16 3:02 PM ]

Marking pre-screened,





[CLJ-1887] clojure.core.Vec does not fully implement clojure.lang.IPersistentVector Created: 26/Jan/16  Updated: 26/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: Steffen Dienst Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections
Environment:

Windows 7, Ubuntu Linux 14.04


Attachments: Text File CLJ-1887.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The implementation of `vector-of` in gvec.clj implements the interface clojure.lang.IPersistentVector, but skips the method `int length()`(see https://github.com/clojure/clojure/blob/bc186508ab98514780efbbddb002bf6fd2938aee/src/clj/clojure/gvec.clj#L240).

user=> (.length [1 2 3])
3
user=> (.length (vector-of :long 1 2 3))
AbstractMethodError Method clojure/core/Vec.length()I is abstract  clojure.core.Vec (gvec.clj:-1)

This was encountered while trying to use core.matrix -https://github.com/mikera/core.matrix/issues/266

Approach: Implement length in gvec

Patch: CLJ-1887.patch

Screened by: Alex Miller



 Comments   
Comment by Steffen Dienst [ 26/Jan/16 3:47 AM ]

The attached patch adds a .length method for primitive type vectors. Now it fully satisfies the interface clojure.lang.IPersistentVector

Comment by Alex Miller [ 26/Jan/16 8:50 AM ]

Good find and good fix.





[CLJ-1882] Use transients in merge-with Created: 11/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: performance, transient


 Description   

This ticket has been broken away from CLJ-1458 for tracking.






[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: 19/Aug/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: Vetted

 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-1876] calling require from java is not thread safe Created: 07/Jan/16  Updated: 17/Feb/16

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

Type: Defect Priority: Major
Reporter: Robert (Bobby) Evans Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Crappy Linux VM running RHEL6

java version "1.8.0_60"
Java(TM) SE Runtime Environment (build 1.8.0_60-b27)
Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode)



 Description   

As a part of Apache Storm we have some code that can load a clojure function from java using the following code.

public static IFn loadClojureFn(String namespace, String name) {
        try {
            clojure.lang.Compiler.eval(RT.readString("(require '" + namespace + ")"));
        } catch (Exception e) {
            //if playing from the repl and defining functions, file won't exist
        }
        return (IFn) RT.var(namespace, name).deref();
    }

If this function is called from multiple different threads at the same time, trying to import the same namespace, I will occasionally get some very odd errors. NOTE: I had to modify the catch to actually print out the error message it was getting (We should not be eating exceptions either way).

{verbatim}
2016-01-07 16:26:09.305 b.s.u.Utils [WARN] Loading namespace failed
clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context, compiling:(storm/starter/clj/word_count.clj:21:1)
at clojure.lang.Compiler.analyze(Compiler.java:6543) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6725) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6524) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6786) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.load(Compiler.java:7227) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:371) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:362) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:446) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:412) ~[clojure-1.7.0.jar:?]
at clojure.core$load$fn__5448.invoke(core.clj:5866) ~[clojure-1.7.0.jar:?]
at clojure.core$load.doInvoke(core.clj:5865) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$load_one.invoke(core.clj:5671) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib$fn__5397.invoke(core.clj:5711) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib.doInvoke(core.clj:5710) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:142) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$load_libs.doInvoke(core.clj:5749) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:137) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$require.doInvoke(core.clj:5832) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$eval114.invoke(NO_SOURCE_FILE:0) ~[?:?]
at clojure.lang.Compiler.eval(Compiler.java:6782) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6745) ~[clojure-1.7.0.jar:?]
at backtype.storm.utils.Utils.loadClojureFn(Utils.java:602) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.clojure.ClojureBolt.prepare(ClojureBolt.java:57) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.daemon.executor$fn_8297$fn_8310.invoke(executor.clj:785) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.util$async_loop$fn__556.invoke(util.clj:482) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at clojure.lang.AFn.run(AFn.java:22) [clojure-1.7.0.jar:?]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_60]
Caused by: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context
at clojure.lang.Util.runtimeException(Util.java:221) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolveIn(Compiler.java:7019) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolve(Compiler.java:6963) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6924) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6506) ~[clojure-1.7.0.jar:?]
... 33 more{verbatim}

If I make the static java function synchronized the issue goes away. It always seems to blow up when parsing a few specific macros getting confused that a specific symbol cannot be resolved.

The namespace trying to be loaded.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/examples/storm-starter/src/clj/storm/starter/clj/word_count.clj

The macros that we seem to get exceptions on.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/storm-core/src/clj/backtype/storm/clojure.clj#L77-L138

And like I said it look like it is a threading issue of some sort. When I added the synchronized keyword everything works.



 Comments   
Comment by Kevin Downey [ 17/Feb/16 10:19 AM ]

calling require from clojure isn't thread safe either, no different from this





[CLJ-1874] Var redefinition breaks in AOT-compiled code Created: 05/Jan/16  Updated: 18/Feb/16

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

Type: Defect Priority: Major
Reporter: William Parker Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1874-ensure-vars-get-interned-when-AOT-compiled.patch    
Patch: Code

 Description   

This is basically a copy of my post from https://groups.google.com/forum/#!topic/clojure/Ozt5HQyM36I on the mailing list. Based on the replies there I'm not sure whether this should be logged as an enhancement or a defect. Please change the designation to whatever is appropriate.

I have found what appears to be a bug in AOT-compiled Clojure when ns-unmap is used; the root cause of this probably impacts other code that redefines Vars as well. I have the following reduced case:

(ns unmap-test.core)

(def a :test-1)

(ns-unmap 'unmap-test.core 'a)

(def a :test-2)

It turns out that a is not resolvable when this namespace is loaded. When I looked at the compiled bytecode,
it appears that the following operations occur:

1. A call to RT.var withe 'unmap-test.core and 'a returns a Var, which is bound to a constant.
This var is added to the namespaces's mapping during this call.
2. Same as 1.
3. The var from 1 is bound to :test-1.
4. ns-unmap is called.
5. The var from 2 is bound to :test-2.

Disclaimer: This is the first time I have had occasion to look directly at bytecode and I could be missing something.

The basic problem here is that the var is accessible from the load method, but when step 5 executes the var is no longer
accessible from the namespace mappings. Thus, the root of the Var is set to :test-2, but that Var is not mapped from the namespace.
This works when there is no AOT compilation, as well as when I use

(ns unmap-test.core)

(def a :test-1)

(ns-unmap 'unmap-test.core 'a)

(intern 'unmap-test.core 'a :test-2)

I realize that creating defs, unmapping them, and then recreating them is generally poor practice in Clojure.
We have an odd case in that we need to have an interface and a Var of the same name in the same namespace. Simply
doing definterface and then def causes a compilation failure:

user=> (definterface abc)
user.abc
user=> (def abc 1)
CompilerException java.lang.RuntimeException: Expecting var, but abc is mapped to interface user.abc, compiling/private/var/folders/3m/tvc28b5d7p50v5_8q5ntj0pmflbdh9/T/form-init4734176956271823921.clj:1:1)

Without going into too much detail, this is basically a hack to allow us to refactor our internal framework code
without immediately changing a very large amount of downstream consumer code. We get rid of the usage of the interface during macroexpansion,
but it still needs to exist on the classpath so it can be imported by downstream namespaces.
There are a number of other ways to accomplish this, so it isn't a particularly big problem for us, but I thought the issue was worth raising.
This was just the first thing I tried and I was surprised when it didn't work.

Note that I used the 1.8.0 RC4 version of Clojure in my sample project, but I had the same behavior on 1.7.0.

Relevant links:

1. Bytecode for the load method of the init class: https://gist.github.com/WilliamParker/d8ef4c0555a30135f35a
2. Bytecode for the init0 method: https://gist.github.com/WilliamParker/dc606ad086670915efd9
3. Decompiled Java code for the init class. Note that this does not completely line up with the bytecode as far as I can tell,
but it is a quicker way to get a general idea of what is happening than the bytecode.
https://gist.github.com/WilliamParker/4cc47939f613d4595d94
4. A simple project containing the code above: https://github.com/WilliamParker/unmap-test
Note that if you try it without AOT compilation the target folder with any previously compiled classes should be removed.



 Comments   
Comment by Nicola Mometto [ 05/Jan/16 9:44 AM ]

The issue is similar to the one in CLJ-1604, the proposed patch extends that fix to all vars rather than just for clojure.core ones.

Comment by Kevin Downey [ 17/Feb/16 10:21 AM ]

was this fixed by clj-1604?

Comment by Nicola Mometto [ 18/Feb/16 8:38 AM ]

No, CLJ-1604 only deals with clojure.core Vars, the patch attached to this ticket is an extension on top of the patch committed for CLJ-1604 that deals with other namespaces





[CLJ-1865] Direct linking doesn't work on recursive calls Created: 08/Dec/15  Updated: 11/Dec/15

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

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


 Description   

It looks like self-recursive calls aren't optimized by direct linking, but if we redefine the same function twice, the Compiler is tricked into thinking that the call is not recursive and (rightfully) optimizes it into an invokeStatic.

I haven't investigated the cause but I suspect (and I might be wrong) it has to do with :arglist metadata potentially having different values when the Var is undefined vs when it's already bound.

[~]> cat test.clj
(ns test)

(defn a [x]
  (a x))
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (compile 'test)
test
user=> ^D
[~]> cd classes
[~/classes]> javap -c test\$a
Compiled from "test.clj"
public final class test$a extends clojure.lang.AFunction {
  public static final clojure.lang.Var const__0;

  public static {};
    Code:
       0: ldc           #11                 // String test
       2: ldc           #13                 // String a
       4: invokestatic  #19                 // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #21                 // class clojure/lang/Var
      10: putstatic     #23                 // Field const__0:Lclojure/lang/Var;
      13: return

  public test$a();
    Code:
       0: aload_0
       1: invokespecial #26                 // Method clojure/lang/AFunction."<init>":()V
       4: return

  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: getstatic     #23                 // Field const__0:Lclojure/lang/Var;
       3: invokevirtual #32                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6: checkcast     #34                 // class clojure/lang/IFn
       9: aload_0
      10: aconst_null
      11: astore_0
      12: invokeinterface #37,  2           // InterfaceMethod clojure/lang/IFn.invoke:(Ljava/lang/Object;)Ljava/lang/Object;
      17: areturn

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #41                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn
}

Redefining the same function twice makes it work.

[~]> cat test.clj
(ns test)

(defn a [x]
  (a x))

(defn a [x]
  (a x))
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (compile 'test)
test
user=> ^D
[~]> cd classes
[~/classes]> javap -c test\$a
Compiled from "test.clj"
public final class test$a extends clojure.lang.AFunction {
  public static final clojure.lang.Var const__0;

  public static {};
    Code:
       0: ldc           #11                 // String test
       2: ldc           #13                 // String a
       4: invokestatic  #19                 // Method clojure/lang/RT.var:(Ljava/lang/String;Ljava/lang/String;)Lclojure/lang/Var;
       7: checkcast     #21                 // class clojure/lang/Var
      10: putstatic     #23                 // Field const__0:Lclojure/lang/Var;
      13: return

  public test$a();
    Code:
       0: aload_0
       1: invokespecial #26                 // Method clojure/lang/AFunction."<init>":()V
       4: return

  public static java.lang.Object invokeStatic(java.lang.Object);
    Code:
       0: aload_0
       1: aconst_null
       2: astore_0
       3: invokestatic  #30                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn

  public java.lang.Object invoke(java.lang.Object);
    Code:
       0: aload_1
       1: aconst_null
       2: astore_1
       3: invokestatic  #30                 // Method invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
       6: areturn
}





[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-1863] Bad type hints on a defn cause the compiler to throw a NPE Created: 04/Dec/15  Updated: 18/Dec/15

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

Type: Defect Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

After CLJ-1232 was committed to master, it is possible for the Clojure compiler to throw a NPE if a defn is type hinted with a invalid type. This surfaces in CLJS where the defn macro is re-used by the ClojureScript compiler, but I think it raises the question: "Should a bad type hint result in a compiler exception?"

The offending line can be found here on GitHub: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L247



 Comments   
Comment by Alex Miller [ 18/Dec/15 8:12 AM ]

This is basically the same as CLJ-1868, but I think what you are asking here is whether bad type hints should be ignored or throw any exception, right?

(Whereas CLJ-1868 is about which exception/message is thrown)

Comment by Timothy Baldridge [ 18/Dec/15 8:22 AM ]

Agreed. I think another possible solution would be to update CLJS to not use the CLJ defn, but I still think that a bad type hint should just be ignored.

Comment by Nicola Mometto [ 18/Dec/15 8:29 AM ]

I don't agree that we shoud ignore bad type hints.
If the compiler knows that something is wrong, it should tell the user immediately rather than silently ignoring and potentially failing at runtime later





[CLJ-1862] Release both a direct linked and a non direct linked clojure Created: 02/Dec/15  Updated: 25/Feb/16

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: 3
Labels: release


 Description   

Currently all new clojure releases will have the core library direct linked.
We should distribute both a direct linked and non direct linked alternatives, using a different classifier for the release.






[CLJ-1832] unchecked-* functions have different behavior on primitive longs vs boxed Longs Created: 26/Oct/15  Updated: 24/Feb/16

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

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

Attachments: Text File clj-1832.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The behavior of unchecked-* functions such as unchecked-add, unchecked-subtract, and unchecked-multiply give different results for primitive longs (expected) and boxed longs (can get overflow exceptions). For example:

user=> *clojure-version*
{:major 1, :minor 7, :incremental 0, :qualifier nil}
user=> (doc unchecked-multiply)
-------------------------
clojure.core/unchecked-multiply
([x y])
  Returns the product of x and y, both long.
  Note - uses a primitive operator subject to overflow.
nil
user=> (unchecked-multiply 2432902008176640000 21)
-4249290049419214848
user=> (unchecked-multiply 2432902008176640000 (Long. 21))

ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)

Normally no one would use explicit boxed Long arguments like in the example above, but these can easily occur, unintentionally, if the arguments to the unchecked functions are not explicitly type hinted as primitive long values.

Approach: clj-1832.patch

Prescreened: Alex Miller



 Comments   
Comment by Alex Miller [ 26/Oct/15 9:03 AM ]

I think this is a reasonable complaint. The trickiness of handling is of course doing it without affecting performance for the non-error case.

Comment by Gary Fredericks [ 26/Oct/15 8:04 PM ]

My first thought was that there shouldn't be any perf concern because it's just a matter of modifying lines such as this one, where the type dispatching has already been done. But maybe you're thinking that that line has to be more complex since the arguments could be of various different numeric types, not just Long and Long?

Comment by Alex Miller [ 27/Oct/15 7:19 AM ]

That was a general comment. I haven't actually looked at the code changes necessary.

Comment by Alexander Kiel [ 11/Feb/16 4:38 AM ]

This costs me an hour today.

I'm with Gary as I see no performance issue. But I see a code amount issue, because the whole tree of add, multiply ... methods has to be repeated.

I would opt for a doc amendment which explains that the unchecked-* functions only work with primitive types. User which see a need for using unchecked math certainly have no problem doing a cast if necessary.

Comment by Gary Fredericks [ 11/Feb/16 11:33 AM ]

It's probably also worth mentioning that speed is not the only use case for unchecked operations – sometimes, e.g. with crypto algorithms, you actually want the weirder kind of arithmetic, and might not want to bother with primitives at first.

Comment by Alexander Kiel [ 12/Feb/16 2:38 PM ]

After a suggestion from Alex Miller, I started with the implementation route here: https://github.com/alexanderkiel/clojure/tree/clj-1832 But it's still work in progress.

Comment by Alexander Kiel [ 13/Feb/16 4:14 AM ]

This patch correctly implements all unchecked-* functions. It assumes that the issue exists only for longs because doubles are unchecked anyway and ratios have bigints.

Comment by Alex Miller [ 24/Feb/16 3:01 PM ]

This looks good to me but I do not see a Contributor Agreement on file for you Alexander. Can you sign one per here: http://clojure.org/community/contributing





[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: 5
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-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-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-1787] Add executables to distribution (suggested changes included) Created: 27/Jul/15  Updated: 27/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Morten Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Linix, OS X and Windows


Patch: Code

 Description   

Being new to clojure I was surprised that there was not a executable included in the download. It makes Clojure appear less proff then it is and is unnecessary complex to remember the exact java cmd line to use so I suggest that shell executable files are added to the distibution for the next version of Clojure. They could look as suggest below for Windows and OS X:

1) Linux and OS X:
#!/bin/bash
java -jar `dirname "$0"`/clojure-1.8.0.jar $@

2) Windows:
java -jar %~dp0\clojure-1.8.0.jar %1 %2 %3 %4 %5 %6 %7 %8






[CLJ-1784] Reflector.getMethods should be cached Created: 21/Jul/15  Updated: 11/Dec/15

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

Type: Enhancement Priority: Major
Reporter: Vladimir Sitnikov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1784.patch    

 Description   

Currently Reflector.getMethods performs expensive logic that includes java.lang.reflect.Method copying.
See: https://github.com/clojure/clojure/blob/b8607d5870202034679cda50ec390426827ff692/src/jvm/clojure/lang/Reflector.java#L373

In our application I see the following back-traces:

at Reflector.copyMethods
at Reflector.invokeInstanceMethod
at ...

These kind of backtraces are second top consumers of all the heap allocation.

JDK cannot cache Methods / Fields since they are mutable (e.g. user can call setAccessible here and there).
However, for the purposes of Clojure, I believe it should be fine to cache Methods and Fields.

What do you think?
E.g. WeakHashMap<Class, WeakReference<List<Method>>> or more sophisticated structure to account String name.



 Comments   
Comment by Alex Miller [ 23/Jul/15 8:19 AM ]

If you are seeing Reflector as a hot spot in your application, you should probably turn on warn-on-reflection and use type hints to avoid reflection.

Comment by Vladimir Sitnikov [ 28/Jul/15 6:10 AM ]

Do you mean there is absolutely no reason to use reflection in Clojure ever?
I do understand that if developer gives enough type hints the reflection would go away.

However:
1) I just do not know if it is easily doable (in other words, if it is possible at all, maintainable, etc)
2) I'm not sure if "always use type hints" is considered a best practice. For instance, warn-on-reflection documentation page says nothing like "always use type hints"
3) Caching copyMethods seems to be a low-hanging fruit here, so it would shave cpu cycles for those who omitted type hints

PS. I'm a java performance engineer, not a Clojure engineer (as in "my Clojure knowledge is somewhere near (+ x y)"), so I kindly beg on your forgiveness for me not doing RTFM.

Comment by Alex Miller [ 28/Jul/15 7:44 AM ]

No, I'm saying that if reflection is a hotspot in your application, usually it's worth investing a few minutes to add type hints in those hotspot areas and this is common advice for Clojure apps. Once that minimal work is done, few Clojure apps are bound by reflection.

Caching seems like an easy solution until you consider all of the management aspects. How does the cache get cleaned? Are the instances mutable and able to be reused? Are there cases where class loaders or code reloading create unexpected side effects? What are the concurrency effects of putting a shared resource in the invocation path? What is the memory impact of a cache and is it configurable?

Those are all things that would need to be investigated, meaning that this is not low-hanging fruit.

Comment by Mike Anderson [ 08/Dec/15 8:39 PM ]

Patch for simple caching of Reflector.getMethods calls for small arities

Comment by Mike Anderson [ 08/Dec/15 8:46 PM ]

I created a small patch to add very simple (fixed size, 1 element for each arity) caching for Reflector.getMethods calls. The aim is to keep this super simple to avoid issues like concurrency effects and having a variable-sized cache.

This helps a small amount in my tests (about 15-20%) on reflection calling the same method in a loop, which is probably the common case where people actually care about reflection performance.

Performance could certainly be improved further due to the fact that I think most of the overhead is actually is the `invokeMatchingMethod`, but that is an orthogonal issue. This patch opens the way for further performance optimisation in that area.

;; clojure 1.8.0-RC3
user=> (let [v (identity 1)] (time (dotimes [i 1000] (.doubleValue v))))
"Elapsed time: 1.598779 msecs"

;; with cached arities
user=> (let [v (identity 1)] (time (dotimes [i 1000] (.doubleValue v))))
"Elapsed time: 1.359888 msecs"

Comment by Vladimir Sitnikov [ 09/Dec/15 2:24 AM ]

Mike Anderson, I wonder if your patch results in a performance regression for concurrent workload.

You've created a single point of contention as lots of threads will try to update private static InstanceMethodCache[] instanceMethodCache entries, so it will hit both "true sharing" and "false sharing" problems.

Should instanceMethodCache be final and written in capital letters?

Comment by Alex Miller [ 09/Dec/15 10:33 AM ]

This ticket needs a better problem definition. That is: "I am doing ____" (with an example) that shows Reflector.getMethods() as the bottleneck.

If I guess at what the problem is, I remain unconvinced that this is the best solution.

A ThreadLocal is likely the cache solution with the lowest concurrency impact.

Comment by Mike Anderson [ 09/Dec/15 9:00 PM ]

This shouldn't have any noticeable concurrency impact: no locking is required for this very simple approach. Most of the time it is simply an unlocked read from an array on the heap, the Java memory model is enough to guarantee correct behaviour. That's cheaper than even a threadlocal, e.g. there's some evidence here that this is 10-20x faster: http://stackoverflow.com/questions/609826/performance-of-threadlocal-variable

At the very least, any concurrency impact is so tiny it will be dwarfed by the benefits of often avoiding the getMethods calls, which are expensive. The cost of array access is a few nanoseconds compared to the cost of getMethods which appears from the benchmark above to be a few hundred nanoseconds.

The worst concurrency case I can think of is the case where two different threads are calling getMethods on different methods at a high rate and these calls are perfectly interleaved so that they always invalidate the cache. But even in that case, it's probably not measurably worse than the current code.

@Vladimir yes, insntanceMethodCache could be final. Might help the JVM very marginally, I guess.

@Alex, I proposed this patch because it is an improvement over what is currently there, I certainly don't think it will be the "best possible solution". In the spirit of open source and making incremental progress, I'd like you to consider accepting it, even if this issue stays open for future consideration. This is also linked to clj-1866, I'm trying to make the "fast path" for reflection better in a few different ways. If you'd rather have a single large patch with a whole bunch of improvements I can certainly do that, I has under the impression that smaller, more "obvious" patches would be easier for you to review but happy either way.

Comment by Vladimir Sitnikov [ 10/Dec/15 1:18 AM ]

Mike Anderson, you are missing the point.

Please check here: http://shipilev.net/blog/2014/jmm-pragmatics/#_benchmarks, slide 77/100 "SC-DRF: Writes"

Alexey Shipilev: This reinforces the idea that data sharing is what you should avoid in the first place, not volatiles

Having ThreadLocal cache would eliminate "shared update" problem.

This ticket needs a better problem definition. That is: "I am doing ____" (with an example) that shows Reflector.getMethods() as the bottleneck

That is true.
My particular case was somehow resolved by development team.
I just thought some basic cache would make Clojure do the right thing by default and require less type specialization written manually.

Comment by Alex Miller [ 10/Dec/15 7:56 AM ]

I see a lot of "should" type statements in there. The whole point is that no change like this is going to go in until we know that there aren't impacts. But more importantly I'm not even going to mark it triaged until it's a good ticket that starts with a problem statement.

Comment by Mike Anderson [ 10/Dec/15 7:22 PM ]

Alex, what do you mean by "know there aren't impacts"? That seems an absurd position on the face of it, it is a perfectly acceptable trade-off to allow minor regressions in rare corner cases if you are improving the fast path / common case significantly.

Also, this definitely isn't a standard that is universally applied for changes to Clojure. Plenty of changes go into Clojure which cause performance regressions in other areas, you only need to look at Andy Fingerhut's excellent benchmarking efforts here to see that: https://jafingerhut.github.io/clojure-benchmarks-results/Clojure-expression-benchmark-graphs.html

Problem statement is IMHO obvious for anything performance related: "Performance of X is sub-optimal, which hurts users who are doing X." If you want a new ticket / changed description that says something like that then I'm happy to do it, but that seriously just feels like bureaucratic box ticking. Please consider this as constructive feedback on your contribution process.

What exactly (i.e. which benchmarks) do you need to see as a valid demonstration of improvement in performance-related issues like this?

Comment by Alex Miller [ 11/Dec/15 10:33 AM ]

Similar to my comments in CLJ-1866, the title of this ticket is "Reflector.getMethods should be cached". That is again a solution, not a problem. What I'm looking for is a title like "Repeated reflection in a loop is slow" and a description that starts with some example code demonstrating the problem. Without a good problem statement, I cannot triage this ticket. I may still consider the priority of the problem to be low enough that it's not worth triaging at this time - I'll withhold judgement though until the ticket is improved.

The fact that prior changes have had unexpected performance impacts only lends additional credence to my suggestion that this (performance-oriented) ticket should validate its claims. You have added code, which makes the "miss" path of this code slower than it was before. How much slower? It should make the "hit" path faster - how much faster? In typical code, how often do we encounter hit vs miss paths? My presumption is that the example will demonstrate a case where the hit path is common. These are the kinds of questions I, as a screener, must ask to evaluate any proposed solution.

Additionally, you are introducing concurrency concerns and some additional work is required to verify both correctness (the current patch has visibility issues) and that you have not introduced contention or memory issues. These are typical problems for any caching-related optimization and I could point you to any number of prior tickets that have wrestled with them.

Comment by Mike Anderson [ 11/Dec/15 9:13 PM ]

Thanks Alex for explaining your concerns.

I agree that a problem-oriented approach to patches is better, so I suggest the following:

  • We close both this issue and clj-1866
  • I create a separate problem-focused ticket for reflection performance
  • I'll benchmark the changes as whole for a number of different cases
  • You'll triage the patch on the assumption that we can demonstrate noticeable improvement in the common cases, all tests pass as before, and no major regressions occur in the corner cases (concurrent access, frequent cache misses etc.)

If you want a problem-oriented issue then I don't think it makes much sense to create separate tickets / patches for each "solution" (although some OSS projects choose to do it that way, they usually have have a much more streamlined process for minor changes / optimisations which probably doesn't suit the Clojure dev process)

Agreed?

Comment by Alex Miller [ 11/Dec/15 9:45 PM ]

As I said, we would prefer small focused tickets and patches, rather than one big patch.

I will reiterate that I'm not convinced doing any of this work makes sense if the scenario is one where a type hint would solve the problem.





[CLJ-1779] Optimise compiler usage of getMethod calls Created: 17/Jul/15  Updated: 17/Jul/15

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

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


 Description   

Currently the Clojure compiler makes multiple redundant calls to Method.getMethod(...) while emitting code, e.g.

gen.invokeStatic(Type.getType(Long.class), Method.getMethod("Long valueOf(long)"));

It seems to be the case that that:

a) These getMethod calls are effectively returning equivalent, immutable constant values
b) getMethod is moderately expensive (performs string analysis and quite a few object allocations)
c) These calls are very common during compilation of typical Clojure code

The proposed enhancement is to replace all of these getMethod calls with constant static values. This should improve compilation performance noticeably with no effect on behaviour.






[CLJ-1777] Add function version of vswap! Created: 13/Jul/15  Updated: 19/Mar/16

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-CLJ-1777-Change-vswap-from-a-macro-to-an-inlineable-.patch     Text File 0001-CLJ-1777-Change-vswap-from-a-macro-to-an-inlineable-v2.patch    
Patch: Code

 Description   

Volatiles and vswap! were added in 1.7 as performant mechanisms to achieve uncoordinated mutation to the language.
given the fact that their addition was a performance-centric one, vswap! was implemented as a macro rather than a normal function to avoid runtime dereference of the function var and the optional apply overhead in case of multiple args.

However this:
-is not necessary
-breaks the api parallelism between volatile/atom swap!/vswap! reset!/vreset!
-makes impossible certain use cases (vswaps! in update-in forms)
-is potentially confusing given that swap! is a function

Infact the macro can be replaced with a function with :inline metadata.
This is a strictly additive change that will make so that for all the current valid usages of vswap! nothing will change, it will still be macroexpanded by the inliner and additionally since it is now a function it can be used in HOF contexts where it's not unusual to see swap! used.



 Comments   
Comment by Alex Miller [ 13/Jul/15 8:07 PM ]

Nicola, please don't set the fix version on tickets.

Comment by Nicola Mometto [ 13/Jul/15 8:10 PM ]

Sorry, I mixed the fix version with the affected field

Comment by Greg Chapman [ 09/Mar/16 11:41 AM ]

An additional drawback to the current macro is that it will result in double evaluation of the vol expression:

user=> (macroexpand '(vswap! (return-a-vol) inc))
(. (return-a-vol) reset (inc (.deref (return-a-vol))))
Comment by Alex Miller [ 10/Mar/16 9:22 AM ]

I don't think Rich is interested in expanding the use of inline.

Re Greg's comment, that would be considered if you want to make a second ticket.

Comment by Nicola Mometto [ 10/Mar/16 12:31 PM ]

Not really sure I understand the adversion to using inline – it works perfectly fine and this is the exact use-case for it: a function that we want to inline for performance reason.

I really fail to see the reasoning behind making what should be a function, a macro, just for the sake of not using inline.

Comment by Phill Wolf [ 19/Mar/16 8:59 AM ]

Adding :inline to existing functions may be deplored as an unprincpled, sloppy, desperate, tragic compromise, and a slippery slope. But vswap! is the opposite case: The macro is what already exists, somewhat idiosyncratically. Pairing it with a function would make it more wholesome, not less.





[CLJ-1774] Field access on typed record does not preserve type Created: 02/Jul/15  Updated: 03/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord, reflection, typehints


 Description   
(ns field-test.core
  (:import [java.util UUID]))

(defrecord UUIDWrapper [^UUID uuid])

(defn unwrap [^UUIDWrapper w]
  (.-uuid w)) ; <- No reflection

(defn get-lower-bits [^UUIDWrapper w]
  (-> w .-uuid .getLeastSignificantBits)) ; <- Reflection :(

The compiler seems to have all the information it needs, but lein check prints

Reflection warning, field_test/core.clj:10:3 - reference to field getLeastSignificantBits on java.lang.Object can't be resolved.

(test case also at https://github.com/MichaelBlume/field-test)



 Comments   
Comment by Alex Miller [ 02/Jul/15 4:31 PM ]

afaik, that ^UUID type hint on the record field doesn't do anything. The record field will be of type Object (only ^double and ^long affect the actual field type).

Perhaps more importantly, it is bad form to use Java interop to retrieve the field values of a record. Keyword access for that is preferred.

Comment by Nicola Mometto [ 03/Jul/15 4:48 AM ]

The same issue applies for deftypes where keyword access is not an option

Comment by Alex Miller [ 03/Jul/15 12:17 PM ]

Per http://clojure.org/datatypes: "You should always program to protocols or interfaces -
datatypes cannot expose methods not in their protocols or interfaces"

Along those lines, usually for deftypes, I gen an interface with the proper types if necessary, then have the deftype implement the interface to expose the field.

Also per http://clojure.org/datatypes:

"note that currently a type hint of a non-primitive type will not be used to constrain the field type nor the constructor arg, but will be used to optimize its use in the class methods" - that is, inside a method implemented on the record/type, referring to the field should have the right hint. So in the example above, if unwrap was an interface or protocol implementation method on the record, and you referred to the field, you should expect the hint to be utilized in that scenario.

So, my contention would be that all of the behavior described in this ticket should be expected based on the design, which is why I've reclassified this as an enhancement, not a defect.





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

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

Comment by Yehonathan Sharvit [ 19/Aug/16 10:19 AM ]

for the sake of symmetry with `assoc` I'd love to see this ticket fixed

Comment by Alex Miller [ 27/Nov/16 10:33 PM ]

Do you need the "if kvs" check?

Should have tests.





[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-1758] xf overload for vec and set Created: 17/Jun/15  Updated: 17/Jun/15

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

Type: Enhancement Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Having (vec xf coll) and (set xf coll) overloads seem useful as opposed to writing (into [] ...).

One might also consider these as variadic overloads, like the sequence function has. I am unsure about that since into doesn't have one and I know too little about multiple input transducers.






[CLJ-1754] Destructuring with :merge Created: 16/Jun/15  Updated: 11/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Henrik Heine Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring


 Description   

Destructuring with :or {...} does not add those defaults to :as binding. Destructuring with :merge adds this.
Usefull when you're wrapping calls to functions and want to add defaults that your callers need not pass in.

(defn foo [& {:merge {:c "C" :d "D"}
:as opt-args}]
opt-args)

should behave like:

(defn foo [& {:keys [c d]
:or {c "C" d "D"}
:as opt-args}]
(let [opt-args (merge {:c c :d d} opt-args)]
opt-args))

Options:
(a) the bindings for c and d in the example may be usefull or not. For the :merge example above they are not needed.
(b) :merge could use keywords or symbols. keywords make it look like the (merge) and symbols make it look like :keys/:or.

Suggestion: using symbols will build a binding for those names and using keywords will not. So users can get the bindings if they need them.

see also https://groups.google.com/forum/#!folder/Clojure$20Stuff/clojure/gG6Tzssn9Nw



 Comments   
Comment by Alex Miller [ 16/Jun/15 7:14 AM ]

Destructuring is about extracting parts of a composite input value. This seems to go a step beyond that into transformation of the input value. Can't say I am a fan of that but I will leave it open.





[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: 4
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-1741] deftype class literals and instances loaded from different classloaders when recompiling namespace Created: 30/May/15  Updated: 19/Oct/15

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

Type: Defect Priority: Major
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, classloader, compiler

Attachments: Text File 0001-CLJ-1714-Don-t-load-AOT-class-when-compiling-already.patch    
Patch: Code
Approval: Incomplete

 Description   

Scenario: Given two files:

src/dispatch/core.clj:

(ns dispatch.core (:require [dispatch.dispatch]))

src/dispatch/dispatch.clj:

(ns dispatch.dispatch)
(deftype T [])
(def t (->T))
(println "T = (class t):" (= T (class t)))

Compile first core, then dispatch:

java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main
user=> (compile 'dispatch.core)
T = (class t): true
dispatch.core
user=> (compile 'dispatch.dispatch)
T = (class t): false     ;; expected true
dispatch.dispatch

This scenario more commonly occurs in a leiningen project with :aot :all. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch.

Cause:

(compile 'dispatch.core)

  • transitively compiles dispatch.dispatch
  • writes .class files to compile-path (which is on the classpath)
  • assertion passes

(compile 'dispatch.dispatch)

  • due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader
  • ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk
  • however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version

In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in CLJ-979 to support other redefinition and AOT mixing usages.

Approaches:

1) Compile in reverse dependency order to avoid compiling twice.

Either swap the order of compilation in the first example or specify the order in project.clj:

:aot [dispatch.dispatch dispatch.core]

This is a short-term workaround.

2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround.

3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init)

(set! *compile-path* "foo")
(compile 'dispatch.core)
(compile 'dispatch.dispatch)

This is not easy to set up via Leiningen currently.

4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals.

Probably too annoying to actually do right now in Leiningen or otherwise.

5) Make compilation non-transitive. This is in the ballpark of CLJ-322, which is another can of worms. Also possibly where we should be headed though.

Screening: I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity (CLJ-322) (#5), but I think we should do this after 1.7. - Alex

See also: CLJ-1650



 Comments   
Comment by Alex Miller [ 30/May/15 8:50 PM ]

Pulling into 1.7 for consideration.

Comment by Stephen Nelson [ 30/May/15 8:55 PM ]

I've added a debug flag to my example that causes type instance hashcodes and their class-loaders to be printed.

Compiling dispatch.core
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
dispatch:  :pass
Compiling dispatch.dispatch
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 760357227 (sun.misc.Launcher$AppClassLoader@42a57993)
dispatch:  :fail
Comment by Nicola Mometto [ 01/Jun/15 7:23 AM ]

The compiler has weird loading rules when using `compile` and both a clj file and a class file are present in the classpath.

This bug happens because RT.load will load the AOT class file rebinding the ->Ctor to use the AOT deftype instance.

A fix for this would be making load "loaded libs" aware to avoid unnecessary/harmful reloadings.

Comment by Nicola Mometto [ 01/Jun/15 10:55 AM ]

The attached patch fixes this bug by keeping track of what has already been loaded and loading the AOT class only if necessary

Comment by Alex Miller [ 16/Jun/15 2:24 PM ]

Original description (since replaced):

Type-dispatching multimethods are defined using the wrong type instance

When using a multimethod that dispatches on types, such as print-dup/print-method, the type reference passed to addMethod in the presence of aot is incorrect on the second load of the namespace. This means that if the namespace has already been loaded as a dependency of another file, the second load when the namespace is loaded for aot compilation will produce a multimethod that fails to dispatch correctly.

I've created an example repository:
https://github.com/sfnelson/clj-mm-dispatch

To reproduce independently, create a namespace that contains a deftype and a multimethod dispatching on the type, and a second namespace that requires the first and sorts alphabetically before the first. Aot-compile both namespaces. When the type-defining namespace is loaded via require it produces a class file for the deftype. When it is loaded the second time for aot-compilation, the type corresponding to the existing class file is given to the defmethod, instead of the new class constructed by loading the namespace. This causes the multimethod it fail to dispatch correctly.

To me this issue seems similar to CLJ-979: the type passed to the multimethod is retrieved using the wrong classloader. This suggests that it might have wider implications than AOT and multimethod dispatch.

Comment by Nicola Mometto [ 18/Jun/15 11:09 AM ]

I just realized this ticket is a duplicate of CLJ-1650





[CLJ-1730] Improve `refer` performance Created: 13/May/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, performance

Attachments: Text File refer-perf.patch    
Patch: Code
Approval: Prescreened

 Description   

refer underlies require, use, and refer-clojure use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

Approach: Some improvements that can be made:

  • Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
  • Use transients to build map of references to refer
  • Instead of cas'ing each new reference individually, build map of all changes, then cas
  • For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

Performance:

expr in a new repl 1.7.0-beta3 1.7.0-beta3+patch
(in-ns 'foo) (clojure.core/refer 'clojure.core) 2.65 ms 0.994 ms
(in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) 1.04 ms 0.113 ms
(use 'criterium.core) 0.877 ms 0.762 ms
(require '[clojure.core.async :refer (>!! <!! chan close!)]) 3408 ms 3302 ms

Patch: refer-perf.patch






[CLJ-1729] Make Counted and count() return long instead of integer Created: 12/May/15  Updated: 08/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

Currently count() returns an int - should bump that up to a long.

On long overflow, count() should throw ArithmeticException. Also see CLJ-1229.



 Comments   
Comment by Erik Assum [ 07/Jul/15 9:24 AM ]

Looking at this, there are some problems like in
clojure.lang.RT#toArray line 1658
where you create a new Object array based on the count of a collection.
It seems as if new Object[] takes an int as a param, so one would have to downcast the long to an int for this to work.

Comment by Alex Miller [ 07/Jul/15 9:39 AM ]

If you're creating an Object[] greater than 2147483647, you may have other problems.

But yes, this ticket definitely needs a more thorough analysis as to what is affected. In this case, I think if the count is <= Integer/MAX_VALUE, then it should proceed and otherwise should throw an exception.

Comment by Erik Assum [ 08/Jul/15 8:19 AM ]

hmmm, this also causes problems wrt java.util.Collection size:
clojure.lang.APersistentSet#size line 164
Where size is specified by

http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#size()





[CLJ-1721] Enable test case for char? Created: 03/May/15  Updated: 03/May/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: 1
Labels: patch

Attachments: Text File CLJ-1721_v01.patch    
Patch: Code and Test

 Description   

clojure.core/char? already exists, but there was no test for it (despite a comment suggesting one).






[CLJ-1720] Add clojure.core/pattern? predicate Created: 03/May/15  Updated: 03/May/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: 0
Labels: patch

Attachments: Text File CLJ-1720_v01.patch     Text File CLJ-1720_v02.patch    
Patch: Code and Test

 Description   

Just like http://dev.clojure.org/jira/browse/CLJ-1719 , this helps with clj/cljs compatibility.



 Comments   
Comment by Brandon Bloom [ 03/May/15 2:37 PM ]

See also http://dev.clojure.org/jira/browse/CLJS-1242

Comment by Brandon Bloom [ 03/May/15 2:48 PM ]

Whoops, uploaded wrong patch. Tests actually pass in this v02 patch.





[CLJ-1707] conditional form is not consumed when :read-allow is falsey Created: 15/Apr/15  Updated: 15/Apr/15

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


 Description   
user=> (def a (java.io.PushbackReader. (java.io.StringReader. "#?(:clj [1 2])")))
#'user/a
user=> (read a)
RuntimeException Conditional read not allowed  clojure.lang.Util.runtimeException (Util.java:221)
user=> (read a)
(:clj [1 2])

the expected result would be an EOF exception on the second read.






[CLJ-1701] Serialization of protocol methods broken: java.io.NotSerializableException: clojure.lang.MethodImplCache Created: 13/Apr/15  Updated: 20/Apr/15

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

Type: Defect Priority: Major
Reporter: Dr. Christian Betz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

With this test, you can see that we cannot serialize methods from protocols (i.e. time-from-tweet), as this results in a java.io.NotSerializableException: clojure.lang.MethodImplCache
at java.io.ObjectOutputStream.writeObject0 (ObjectOutputStream.java:1183)
java.io.ObjectOutputStream.defaultWriteFields (ObjectOutputStream.java:1547)
java.io.ObjectOutputStream.writeSerialData (ObjectOutputStream.java:1508)
java.io.ObjectOutputStream.writeOrdinaryObject (ObjectOutputStream.java:1431)
java.io.ObjectOutputStream.writeObject0 (ObjectOutputStream.java:1177)
java.io.ObjectOutputStream.writeObject (ObjectOutputStream.java:347)
sparkling.protocol_test$serialize.invoke (protocol_test.clj:11)

This is the actual test:

(ns sparkling.protocol-test
(:require [clojure.test :refer :all])
(:import [java.io ObjectInputStream ByteArrayInputStream ObjectOutputStream ByteArrayOutputStream]))

(defn- serialize
"Serializes a single object, returning a byte array."
[v]
(with-open [bout (ByteArrayOutputStream.)
oos (ObjectOutputStream. bout)]
(.writeObject oos v)
(.flush oos)
(.toByteArray bout)))

(defn- deserialize
"Deserializes and returns a single object from the given byte array."
[bytes]
(with-open [ois (-> bytes ByteArrayInputStream. ObjectInputStream.)]
(.readObject ois)))

(defprotocol timestamped
(time-from-tweet [item]))

(defrecord tweet [username tweet timestamp]
timestamped
(time-from-tweet [_]
timestamp
))

(deftest sequable-serialization
(testing "Serialization of function"
(let [item identity]
(is item (-> item serialize deserialize))))

(testing "Serialization of protocol method"
(let [item time-from-tweet]
(is item (-> item serialize deserialize)))))



 Comments   
Comment by Dr. Christian Betz [ 13/Apr/15 6:15 AM ]

BTW: Same is true for multimethods, here the exception is java.io.NotSerializableException: clojure.lang.MultiFn

Comment by Alex Miller [ 13/Apr/15 9:35 AM ]

I don't think we expect functions to be serializable in this way. Both protocols and multimethods effectively have runtime state based on what implementations have extended them. What would it mean to serialize these functions? Would you serialize them with whatever implementations have been loaded at that point? Or with none? Both seem problematic to me. Regular functions are closures and can capture the state of their environment. I think better answers are either AOT or for regular functions, something like the serializable-fn library.

Comment by Dr. Christian Betz [ 13/Apr/15 1:13 PM ]

Hi,

thanks for the comments. First, something to the background: I'm developing Sparkling, a Clojure API to Apache Spark. For distributing code in the cluster it depends on AOT compiled functions, so yes, you cannot simply serialize any function around, it needs to be AOT'd. Serializiation provides us with support for the current bindings etc, and everything works as expected. So, AFunction is serializable for a reason and so are other implementations of AFn/IFn, everything works well.

Regarding the state of protocols and multimethods - I think it's conceptually the same as the state of functions (which function definition, the var might be bound multiple times, etc.), and the closures given in bindings. There's no reason for me as the user of a protocol to believe that the method from the protocol differs from a function. In fact (ifn? protocol-method) also returns true.

serializable-fn, not being intended for over-the-wire serialization in the first place, has problems with collections of functions in bindings of the serializble function, together with an issue with PermGen pollution by creating classes for the same function over and over again in the context of Spark.

I think I'm fine for the moment, as I can wrap the protocol method in a function, but I still believe, that this is a bug.

Regards

Chris

Comment by Dr. Christian Betz [ 20/Apr/15 3:10 AM ]

actually, this is the code snippet from clojure.lang.AFunction causing the pain:

clojure.lang.AFunction.java
public abstract class AFunction extends AFn implements IObj, Comparator, Fn, Serializable {

public volatile MethodImplCache __methodImplCache;

AFunction is serializable, but MethodImplCache is not. I'm not sure if it's enough to mark it as transient, because I did not check where initialization happens.

Comment by Dr. Christian Betz [ 20/Apr/15 3:29 AM ]

My comment per mail got lost in SMTP-nirvana: There's an easy workaround. Wrap the protocol method in a function, that will do the trick at the cost of uglifying your code





[CLJ-1690] Make Range, Repeat and Cycle implement Indexed Created: 31/Mar/15  Updated: 01/Apr/15

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

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

Attachments: Text File clj-1690.patch    

 Description   

Currently, Cycle, Range and Repeat do not implement Indexed, which means that "nth" is O( n ) on average.

The proposed change is to implement Indexed for these classes, so that "nth" becomes an O( 1 ) operation.



 Comments   
Comment by Alex Miller [ 31/Mar/15 9:37 PM ]

This is an expansion of capabilities and commitments beyond what these functions have done in the past. We've already committed to more than we really wanted to with them, so I'm not sure we want to add yet more commitments. In any case, we won't do it for 1.7.

FYI, that Range you're patching is not currently used for anything - the current impl uses a chunked seq definition in core.clj. CLJ-1515 will (likely) replace the Range class with an all new impl. In any case, patching Range here probably isn't useful until CLJ-1515 is resolved.

Comment by Mike Anderson [ 31/Mar/15 9:50 PM ]

Understood re 1.7. Though I personally think this is small enough that you could squeeze it in. Your call.

However I still think this approach is useful though: nth is a very common operation and I note you aren't benchmarking it yet in CLJ-1515. Whatever new Range implementation is used will benefit from implementing Indexed.

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

None of these functions currently promises to return something Indexed. If we add that and people come to rely on it, we can never change the implementation in a way that removes it. So I'm not sure that's a promise we want to make.

Comment by Mike Anderson [ 01/Apr/15 1:39 AM ]

I am not proposing that we make any "promise" of an indexed return value, simply that such classes implement the interface as an implementation detail (where it makes sense).

This then causes the fast path in functions like RT.nth to be taken, so we get O(1) instead of O( n) for the most common indexed lookup cases.

TBH, my assumption was that this was the main purpose of the "Indexed" interface, i.e. to allow concrete collection types to participate in Clojure's indexed access functions with an efficient implementation.

Comment by Ghadi Shayban [ 01/Apr/15 10:25 AM ]

People regularly rely on implementation details, promise or no promise. If clojure were to add Indexed then remove it, people's code would either break or get slower.

Implementation behavior (whether overt or implicit) is necessarily treated as future constraints (shackles), so it is considered carefully.





[CLJ-1679] Add fast path in seq comparison for structurally sharing seqs Created: 21/Mar/15  Updated: 23/Mar/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: 0
Labels: collections, seq

Attachments: Text File 0001-CLJ-1679-do-pointer-checks-in-seq-equality.patch     Text File CLJ-1679-v2.patch     Text File CLJ-1679-v3.patch    
Patch: Code and Test

 Description   

Currently comparing two non identical seqs requires iterating through both seqs comparing value by value, ignoring the possibility of seq `a` and `b` having the same (pointer-equal) rest.

The proposed patch adds a pointer equality check on the seq tails that can make the equality short-circuit if the test returns true, which is helpful when comparing large (or possibly infinite) seqs that share a common subseq.

After this patch, comparisons like

(let [x (range)] (= x (cons 0 (rest x))))
which currently don't terminate, return true in constant time.

Patch: CLJ-1679-v3.patch



 Comments   
Comment by Michael Blume [ 23/Mar/15 12:52 PM ]

When this test fails (it fails on my master, but I've got a bunch of other development patches, I'm still figuring out where the conflict is), it fails by hanging forever. Maybe it'd be better to check equality in a future and time out the future?

Comment by Michael Blume [ 23/Mar/15 1:01 PM ]

like so =)

Comment by Nicola Mometto [ 23/Mar/15 1:11 PM ]

Makes sense, thanks for the updated patch

Comment by Michael Blume [ 23/Mar/15 1:24 PM ]

Hm, previous patch had a problem where the reporting logic still tries to force the sequence and OOMs, this patch prevents that.

Comment by Michael Blume [ 23/Mar/15 1:41 PM ]

ok, looks like CLJ-1515, CLJ-1603, and this patch, all combine to fail together, though any two of them work fine.

Comment by Michael Blume [ 23/Mar/15 1:43 PM ]

(And really there's nothing wrong with the source of this patch, it still works nicely to short-circuit = where there's structural sharing, it's just that the other two patches break structural sharing for ranges, so the test fails)

Comment by Nicola Mometto [ 23/Mar/15 2:02 PM ]

I see, I guess we'll have to change the test if the patches for those tickets get applied.





[CLJ-1674] Boolean return type-hint confusing the compiler Created: 12/Mar/15  Updated: 12/Mar/15

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

Type: Defect Priority: Major
Reporter: Mikkel Kamstrup Erlandsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints
Environment:

OSX, Clojure 1.6.0



 Description   

Saving the below snippet and running it like

java -jar clojure-1.6.0.jar snippet.clj

Produces

$ java -jar clojure-1.6.0.jar snippet.clj 
Exception in thread "main" java.lang.IllegalArgumentException: Unable to resolve classname: clojure.core$boolean@1356d4d4, compiling:(/Users/kamstrup/tmp/snippet.clj:15:1)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6651)
	at clojure.lang.Compiler.analyze(Compiler.java:6445)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6632)
	at clojure.lang.Compiler.analyze(Compiler.java:6445)
	at clojure.lang.Compiler.access$100(Compiler.java:38)
	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:538)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6644)
	at clojure.lang.Compiler.analyze(Compiler.java:6445)
	at clojure.lang.Compiler.analyze(Compiler.java:6406)
	at clojure.lang.Compiler.eval(Compiler.java:6707)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$script_opt.invoke(main.clj:336)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Unable to resolve classname: clojure.core$boolean@1356d4d4
	at clojure.lang.Compiler$HostExpr.tagToClass(Compiler.java:1069)
	at clojure.lang.Compiler$InvokeExpr.getJavaClass(Compiler.java:3659)
	at clojure.lang.Compiler$LocalBinding.hasJavaClass(Compiler.java:5657)
	at clojure.lang.Compiler$LocalBindingExpr.hasJavaClass(Compiler.java:5751)
	at clojure.lang.Compiler.maybePrimitiveType(Compiler.java:1283)
	at clojure.lang.Compiler$IfExpr.doEmit(Compiler.java:2631)
	at clojure.lang.Compiler$IfExpr.emit(Compiler.java:2613)
	at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:5826)
	at clojure.lang.Compiler$LetExpr.doEmit(Compiler.java:6180)
	at clojure.lang.Compiler$LetExpr.emit(Compiler.java:6133)
	at clojure.lang.Compiler$BodyExpr.emit(Compiler.java:5826)
	at clojure.lang.Compiler$FnMethod.doEmit(Compiler.java:5374)
	at clojure.lang.Compiler$FnMethod.emit(Compiler.java:5232)
	at clojure.lang.Compiler$FnExpr.emitMethods(Compiler.java:3771)
	at clojure.lang.Compiler$ObjExpr.compile(Compiler.java:4410)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3904)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6642)
	... 19 more

The snippet:

snippet.clj
;; Bug in the Clojure compiler (1.6.0): If we annotate the return here with ^boolean we get:
;; 'IllegalArgumentException: Unable to resolve classname: clojure.core$boolean' from the compiler.
;; Removing it, everything is as expected
(defn ^boolean foo-bar?
  [node]
  (= node "foo-bar"))

;; Check it out, we can have ^boolean here, but not on foo-bar? !! :-)
(defn ^boolean bar-foo?
  [node]
  (= node "bar-foo"))

;; Instead of removing the ^boolean return on foo-bar? we can also remove this function
;; to have all work as expected
(defn ^boolean interesting?
  [node]
  (or (foo-bar? node) (bar-foo? node)))

(println "Foo-Bar?" (foo-bar? "baz"))


 Comments   
Comment by Mikkel Kamstrup Erlandsen [ 12/Mar/15 5:25 AM ]

Typo in comment 2 in the snippet: s/xtc-scenario?/foo-bar?/

Comment by Nicola Mometto [ 12/Mar/15 6:01 AM ]

Metadata on def's symbol is evaluated as per the doc (http://clojure.org/special_forms), evaluating `boolean` results in the clojure.core/boolean function which is not a valid type hint.

As a rule of thumb, attach the return tag in the argvec rather than on the def symbol, in this case you should write

(defn foo-bar?
   ^boolean [node]
  (= node "foo-bar"))

I understand why the fact that

(defn ^boolean foo [] true)

and

(defn foo ^boolean [] true)

behave differently and the fact that the compiler will throw iff the type hint is used rather than throwing at the function definition time is confusing (and I've complained about this and the lack of documentation/specification regarding type hints for a while) but this is not a bug

Comment by Mikkel Kamstrup Erlandsen [ 12/Mar/15 6:36 AM ]

Thanks for clarifying Nicola, you are indeed correct.

Putting return type annotations before the method name seems to be common practice in a lot of Clojure code I've read online. Perpetuated by some online tutorials, and the clojure.org docs them selves (fx.

(defn ^:private ^String my-fn ...)
is found in http://clojure.org/cheatsheet)

Comment by Andy Fingerhut [ 12/Mar/15 8:36 AM ]

Mikkel: If the type tags are Java classes, not primitives, then ^Classname is a correct type tag. If you use Eastwood, it can warn about these incorrect type tags, and has some documentation on what works and what does not here: https://github.com/jonase/eastwood#wrong-tag

Also here: https://github.com/jonase/eastwood#unused-meta-on-macro





[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-1662] folding over hash-map nested hash-map throws exception Created: 17/Feb/15  Updated: 17/Feb/15

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers
Environment:

JVM 1.7.0_76



 Description   

I got a baffling exception in a recursive function that folds. REPL transcript below:

nREPL server started on port 57818 on host 127.0.0.1 - nrepl://127.0.0.1:57818
REPL-y 0.3.5, nREPL 0.2.6
Clojure 1.7.0-alpha5
Java HotSpot(TM) 64-Bit Server VM 1.7.0_76-b13
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
 Javadoc: (javadoc java-object-or-class-here)
    Exit: Control+D or (exit) or (quit)
 Results: Stored in vars *1, *2, *3, an exception in *e

user=> (use 'foldtest.core)
nil
user=> (source leafs)
(defn leafs [xs]
  (->> (r/mapcat (fn [k v]
                   (if (map? v)
                     (leafs v)
                     [[k v]])) xs)
       (r/foldcat)))
nil
user=> (leafs (hash-map :a (hash-map :b 1 :c 2)))

ClassCastException clojure.lang.PersistentHashMap$1 cannot be cast to clojure.lang.IFn  clojure.core.reducers/fjinvoke (reducers.clj:48)
user=> (pst)
ClassCastException clojure.lang.PersistentHashMap$1 cannot be cast to clojure.lang.IFn
	clojure.core.reducers/fjinvoke (reducers.clj:48)
	clojure.lang.PersistentHashMap.fold (PersistentHashMap.java:207)
	clojure.core.reducers/eval1347/fn--1348 (reducers.clj:367)
	clojure.core.reducers/eval1220/fn--1221/G--1211--1232 (reducers.clj:81)
	clojure.core.reducers/folder/reify--1247 (reducers.clj:130)
	clojure.core.reducers/fold (reducers.clj:98)
	clojure.core.reducers/fold (reducers.clj:96)
	clojure.core.reducers/foldcat (reducers.clj:318)
	foldtest.core/leafs (core.clj:5)
	foldtest.core/leafs/fn--1367 (core.clj:7)
	clojure.core.reducers/mapcat/fn--1277/fn--1280 (reducers.clj:185)
	clojure.lang.PersistentHashMap$NodeSeq.kvreduce (PersistentHashMap.java:1127)
nil
user=>

Note that it must be a hash-map nested in a hash-map. Other combinations of array and hash maps seem fine:

user=> (leafs (array-map :a (hash-map :b 1 :c 2)))
[[:c 2] [:b 1]]
user=> (leafs (hash-map :a (array-map :b 1 :c 2)))
[[:b 1] [:c 2]]
user=> (leafs (hash-map :a (hash-map :b 1 :c 2)))

ClassCastException clojure.lang.PersistentHashMap$1 cannot be cast to clojure.lang.IFn  clojure.core.reducers/fjinvoke (reducers.clj:48)
user=> (leafs (array-map :a (array-map :b 1 :c 2)))
[[:b 1] [:c 2]]
user=>

Possibly related: CLJCLR-63

It took me a while to discover this because of this inconsistency (which I am not sure is a bug):

user=> (def a {:a 1})
#'user/a
user=> (type a)
clojure.lang.PersistentHashMap
user=> (let [a {:a 1}] (type a))
clojure.lang.PersistentArrayMap
user=> (type {:a 1})
clojure.lang.PersistentArrayMap
user=>

(I had put test input in a def, but using the defed var always failed but literals always worked!)






[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: 28/Sep/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: 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     Text File CLJ-1656-v8.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?

Comment by Michael Blume [ 28/Sep/16 2:14 PM ]

Updated patch to apply to master





[CLJ-1620] Constants are leaked in case of a reentrant eval Created: 18/Dec/14  Updated: 12/Jul/16

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

Type: Defect Priority: Major
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v2.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v3.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch     Text File clj-1620-v5.patch     Text File eval-bindings.patch    
Patch: Code
Approval: Incomplete

 Description   

Compiling a function that references a non loaded (or uninitialized) class triggers its init static. When the init static loads clojure code, some constants (source code I think) are leaked into the constants pool of the function under compilation.

It prevented CCW from working in some environments (Rational) because the static init of the resulting function was over 64K.

Steps to reproduce:

Load the leak.main ns and run the code in comments: the first function has 15 extra fields despite being identical to the second one.

(ns leak.main)

(defn first-to-load []
  leak.Klass/foo)

(defn second-to-load []
  leak.Klass/foo)

(comment
=> (map (comp count #(.getFields %) class) [first-to-load second-to-load])
(16 1)
)
package leak;
 
import clojure.lang.IFn;
import clojure.lang.RT;
import clojure.lang.Symbol;
 
public class Klass {
  static {
    RT.var("clojure.core", "require").invoke(Symbol.intern("leak.leaky"));
  }
  public static IFn foo = RT.var("leak.leaky", "foo");
}
(ns leak.leaky)

(defn foo
  "Some doc"
  []
  "hello")

(def unrelated 42)

https://gist.github.com/cgrand/5dcb6fe5b269aecc6a5b#file-main-clj-L10

Patch: clj-1620-v5.patch



 Comments   
Comment by Christophe Grand [ 18/Dec/14 3:56 PM ]

Patch from Nicola Mometto

Comment by Nicola Mometto [ 18/Dec/14 4:01 PM ]

Attached the same patch with a more informative better commit message

Comment by Laurent Petit [ 18/Dec/14 4:03 PM ]

I'd like to thank Christophe and Alex for their invaluable help in understanding what was happening, formulating the right hypothesis and then finding a fix.

I would also mention that even if non IBM rational environments where not affected by the bug to the point were CCW would not work, they were still affected. For instance the class for a one-liner function wrapping an interop call weighs 700bytes once the patch is applied, when it weighed 90kbytes with current 1.6 or 1.7.

Comment by Laurent Petit [ 18/Dec/14 5:07 PM ]

In CCW for the initial problematic function, the -v2 patch produces exactly the same bytecode as if the referenced class does not load any namespace in its static initializers.
That is, the patch is valid. I will test it live in the IBM Rational environment ASAP.

Comment by Laurent Petit [ 19/Dec/14 12:10 AM ]

I confirm the patch fixes the issue detected initially in the IBM Rational environment

Comment by Michael Blume [ 06/Jan/15 4:03 PM ]

I have absolutely no idea why, but if I apply this patch, and the patch for CLJ-1544 to master, and then try to build a war from this test project https://github.com/pdenhaan/extend-test I get a scary-looking traceback:

$ lein do clean, war!
Exception in thread "main" java.lang.NoSuchFieldError: __thunk__0__, compiling:(route.clj:1:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3606)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at extend_test.core.handler$loading__5301__auto____66.invoke(handler.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at extend_test.core.servlet$loading__5301__auto____7.invoke(servlet.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$compile$fn__5420.invoke(core.clj:5834)
	at clojure.core$compile.invoke(core.clj:5833)
	at user$eval5.invoke(form-init180441230737245034.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6776)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.eval(Compiler.java:6766)
	at clojure.lang.Compiler.load(Compiler.java:7203)
	at clojure.lang.Compiler.loadFile(Compiler.java:7159)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NoSuchFieldError: __thunk__0__
	at instaparse.core__init.load(Unknown Source)
	at instaparse.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clout.core$loading__5301__auto____273.invoke(core.clj:1)
	at clout.core__init.load(Unknown Source)
	at clout.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:482)
	at compojure.core$loading__5301__auto____68.invoke(core.clj:1)
	at compojure.core__init.load(Unknown Source)
	at compojure.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.route$loading__5301__auto____1508.invoke(route.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	... 75 more
Subprocess failed
Comment by Michael Blume [ 06/Jan/15 4:06 PM ]

https://github.com/MichaelBlume/clojure/tree/no-field
https://github.com/MichaelBlume/extend-test/tree/no-field

mvn clean install in the one, lein ring uberwar in the other.

Comment by Nicola Mometto [ 06/Jan/15 6:09 PM ]

Michael, thanks for the report, I've tried investigating this a bit but the big amount of moving parts involved make it really hard to figure out why the combination of the two patches causes this issue.

A helpful minimal case would require no lein and no external dependencies, I'd appreciate some help in debugging this issue if anybody has time.

Comment by Michael Blume [ 06/Jan/15 10:56 PM ]

Ok, looks like the minimal case is

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

(ns bar (:require [foo]))

and then attempt to AOT-compile both foo and bar.

I don't yet know what's special about instaparse.core.

Comment by Michael Blume [ 06/Jan/15 11:30 PM ]

Well, not a minimal case, of course, but one without lein, at least.

Comment by Michael Blume [ 06/Jan/15 11:51 PM ]

ok, problem is instaparse's defclone macro, I've extracted it to a test repo

https://github.com/MichaelBlume/thunk-fail

lein do clean, compile will get you a failure, but the repo has no dependencies so I'm sure there's a way to do that without lein.

Comment by Ghadi Shayban [ 06/Jan/15 11:56 PM ]

Sorry for the barrage of questions, but these classloader bugs are subtle (and close to being solved I hope). Your report is immensely valuable, and yet it will help to be even more specific. There are a cluster of these bugs – and keeping them laser-focused is key.

The minimal case to which you refer is the NoSuchFieldError?
How are is this being invoked this without lein?
What are you calling to AOT? (compile 'bar) ?
What is the classpath? When you invoke originally, is ./target/classes empty?
Does the problem go away with CLJ-979-7 applied?

Comment by Michael Blume [ 07/Jan/15 12:16 AM ]

I have tried and failed to replicate without leiningen. When I just run

java -Dclojure.compile.path=target -cp src:../clojure/target/clojure-1.7.0-aot-SNAPSHOT.jar clojure.lang.Compile thunk-fail.first thunk-fail.second

everything works fine.

Comment by Ghadi Shayban [ 07/Jan/15 12:30 AM ]

The NoSuchFieldError is related to the keyword lookup sites.

Replacing defclone's body with
`(do (:foo {})) is enough to trigger it, with the same ns structure.

Comment by Nicola Mometto [ 07/Jan/15 4:47 AM ]

I have updated the patch for CLJ-1544, now the combination of the new patch + the patch from this ticket should not cause any exception.

That said, a bug in this patch still exists since while the patch for CLJ-1544 had a bug, it was causing a perfectly valid (albeit hardly reproducible) compilation scenario so we should keep debugging this patch with the help of the bugged patch for CLJ-1544.

I guess the first thing to do is figure out what lein compile is doing differently than clojure.Compile

Comment by Nicola Mometto [ 07/Jan/15 4:49 AM ]

Also Ghadi is right, infact replacing the whole body of thunk-fail.core with (:foo {}) is enough.

It would seem like the issue is with AOT (re)compiling top-level keyword lookup sites, my guess is that for some reason this patch is preventing correct generation of the __init static initializer.

Comment by Nicola Mometto [ 07/Jan/15 5:35 AM ]

I still have absolutely no idea what lein compile is doing but I figured out the issue.
The updated patch binds (in eval) the appropriate vars only when already bounded.

Comment by Alex Miller [ 07/Jan/15 9:00 AM ]

Would it be worth using transients on the bindings map now?

Comment by Nicola Mometto [ 07/Jan/15 9:11 AM ]

Makes sense, updated the patch to use a transient map

Comment by Michael Blume [ 07/Jan/15 12:25 PM ]

Is there a test we can add that'll fail in the presence of the v2 patch? preferably independent of the CLJ-1544 patch? I can try to write one myself, but I don't have a lot of familiarity with the Clojure compiler internals.

Comment by Nicola Mometto [ 07/Jan/15 12:32 PM ]

I'll have to think about a way to reproduce that bug, it's not a simple scenario to reproduce.
It involves compiling a namespace from an evaluated context.

Comment by Laurent Petit [ 15/Apr/15 11:14 AM ]

Hello, is there any chance left that this issue will make it to 1.7 ?

Comment by Alex Miller [ 15/Apr/15 11:18 AM ]

Wasn't planning on it - what's the impact for you?

Comment by Laurent Petit [ 29/Apr/15 2:14 PM ]

The impact is that I need to use a patched version of Clojure for CCW.
While it's currently not that hard to follow clojure's main branch and regularly rebase on it or reapply the patch, it's still a waste of time.

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

I will check with Rich whether it can be screened for 1.7 before we get to RC.

Comment by Alex Miller [ 29/Apr/15 3:49 PM ]

same as v4 patch, but just has more diff context

Comment by Laurent Petit [ 01/May/15 7:25 AM ]

the file mentioned in the patch field is not the right one IMHO

Comment by Alex Miller [ 01/May/15 8:42 AM ]

which one is?

Comment by Laurent Petit [ 01/May/15 8:58 AM ]

I think you previous comment relates to clj-1620-v5.patch, but at the end of the description there's the following line:

Patch: 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch

Comment by Alex Miller [ 01/May/15 9:30 AM ]

Those patches are equivalent with respect to the change they introduce; they just differ in how much diff context they have.

Comment by Alex Miller [ 18/May/15 2:25 PM ]

Rich has ok'ed screening this one for 1.7 but I do not feel that I can mark it screened without understanding it much better than I do. The description, code, and cause information here is not sufficient for me to understand what the problem actually is or why the fix is the right one. The fix seems to address the symptom but I worry that it is just a symptom and that a better understanding of the actual cause would lead to a different or better fix.

The evolution of the patches was driven by bugs in CLJ-1544 (a patch which has been pulled out for being suspect for other reasons). Starting fresh, were those modifications necessary and correct?

Why does this set of vars need to push clean impls into the bindings? Why not some of the other vars (like those pushed in load())? The set chosen here seems to match that from the ReifyParser - why? Why should they only be pushed if they are bound (that is, why is "not bound" not the same as "bound but empty")? Are we affecting performance?

Popping all the way out, is the thing being done by CCW even a thing that should be doable? The description says "Compiling a function that references a non loaded (or uninitialized) class triggers its init static" - should this load even happen? Can we get an example that actually demonstrates what CCW was doing originally?

Comment by Laurent Petit [ 19/May/15 7:12 AM ]

Alex, the question of "should what CCW is doing be doable" can be answered if you answer it on the given example, I think.

The question "should the initialization of the class occur when it could just be loaded" is a good one. Several reports have been made on the Clojure list about this problem, and I guess there is at least one CLJ issue about changing some more classForName into classForNameNonLoading here and there in Clojure.
For instance, it prevents referencing java classes which have code in their static initializers as soon as the code does some supposition about the runtime it is initialized in. This is a problem with Eclipse / SWT, this a problem with Cursive as I remember Colin mentioning a similar issue. And will probably is a problem that can appear each time one tries to AOT compile clojure code interoperating with java classes who happen to have, somewhere within static initializers triggered by the compilation (and this is transitive), assumptions that they are initialized in the proper target runtime environment.

What I don't know is if preventing the initialization to occur in the first place would be sufficient to get rid of the class of problems this bug and the proposed patch tried to solve. I do not claim to totally what is happening either (Christophe and Nicolas were of great help to analyze the issue and create the patch), but as I understand it, it's a kind of "Inception-the-movie-like" bug. Compiling a fn which triggers compiling another fn (here through the loading of clojure namespaces via a java initializer).

If preventing the initialization of class static methods when they are referenced (through interop calls - constructor, field, method, static field, static method-) is the last remaining bit that could cause such "compilation during compilation" scenario, then yes, protecting the compilation process like Nicolas tried to do may not be necessary, and just fixing the undesired loading may be enough.





[CLJ-1612] clojure.core.reducers/mapcat can call f1 with undefined arity of 0 arguments? Created: 10/Dec/14  Updated: 10/Dec/14

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

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers


 Description   

I have not run across this with running code, so perhaps it is impossible for reasons I have not understood. Also not sure whether fixing issues with reducers is of any importance, given transducers. This was found while testing the Eastwood lint tool on some Clojure namespaces, including clojure.core.reducers.

(defcurried mapcat
  "Applies f to every value in the reduction of coll, concatenating the result
  colls of (f val). Foldable."
  {:added "1.5"}
  [f coll]
  (folder coll
   (fn [f1]
     (let [f1 (fn
                ([ret v]
                  (let [x (f1 ret v)] (if (reduced? x) (reduced x) x)))
                ([ret k v]
                  (let [x (f1 ret k v)] (if (reduced? x) (reduced x) x))))]
       (rfn [f1 k]
            ([ret k v]
               (reduce f1 ret (f k v))))))))

The definition of macro rfn expands to a (fn ...) that can call f1 with no arguments, which is not a defined arity for f1.






[CLJ-1610] Unrolled small maps Created: 08/Dec/14  Updated: 22/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Zach Tellman
Resolution: Unresolved Votes: 10
Labels: collections


 Description   

Placeholder for unrolled small maps enhancement (companion for vectors at CLJ-1517).



 Comments   
Comment by Andy Fingerhut [ 09/Jul/15 10:59 PM ]

Is there an expectation that these would perform better that PersistentArrayMap?

Comment by Zach Tellman [ 09/Jul/15 11:53 PM ]

Yes, in some cases significantly so, for three reasons (in rough order of importance):

  • positional constructors, without any need for array instantiation/population
  • short-circuiting equality checks using hash comparisons
  • no iteration on any operation

There are a series of benchmarks at https://github.com/ztellman/cambrian-collections/blob/master/test/cambrian_collections/map_test.clj#L64-L148, which compare operations against maps with both keywords (which don't benefit from the hash comparisons) and symbols (which do). The 7-entry map cases cause the unrolled maps to overflow, so they only exist to test the overflow mechanism.

I've run the benchmark suite on my laptop, and the results are at https://gist.github.com/ztellman/961001e1a77e4f76ee1d. Some notable results:

The rest of the benchmarks are marginally faster due to unrolling, but most of the performance benefits are from the above behaviors. In a less synthetic benchmark, I found that Cheshire JSON decoding (which is 33% JSON lexing and 66% data structure building) was sped up roughly 30-40%.





[CLJ-1583] Apply forces the evaluation of one element more than necessary Created: 07/Nov/14  Updated: 09/Nov/14

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

Attachments: Text File 0001-make-RT.boundedLength-lazier.patch    
Patch: Code

 Description   

Given a function with one fixed argument and a vararg, it should be sufficient to force evaluation of 2 elements for apply to know which arity it should select, however it currently forces 3:

user=> (defn x ([a & b]))
#'user/x
user=> (apply x (map println (iterate inc 0)))
0
1
2
nil

This makes lazy functions that use apply (for example mapcat) less lazy than they could be.
The proposed patch makes RT.boundedLength short-circuit immediately after the seq count is greater than the max fixed arity:

user=> (defn x ([a & b]))
#'user/x
user=> (apply x (map println (iterate inc 0)))
0
1
nil


 Comments   
Comment by Nicola Mometto [ 09/Nov/14 3:37 PM ]

The patch in this ticket slightly improves the issue reported at http://dev.clojure.org/jira/browse/CLJ-1218





[CLJ-1582] Overriding in-ns and ns is problematic Created: 07/Nov/14  Updated: 07/Nov/14

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: 1
Labels: None

Attachments: Text File 0001-Allow-overriding-of-clojure.core-in-ns-and-clojure.c.patch    
Patch: Code

 Description   

Currently it is possible to override clojure.core/in-ns and clojure.core/ns, but it is not possible to refer to the namespace-specific vars without fully qualifying them:

user=> (ns foo (:refer-clojure :exclude [in-ns]))
nil
foo=> (def in-ns 1)
#'foo/in-ns
foo=> in-ns
#<clojure.lang.RT$1@76b5e4c5>

After this patch, overriding in-ns and ns works like for every other clojure.core var:

user=> (ns foo (:refer-clojure :exclude [in-ns]))
nil
foo=> (def in-ns 1)
#'foo/in-ns
foo=> in-ns
1


 Comments   
Comment by Reid McKenzie [ 07/Nov/14 11:46 AM ]

This is motivated by https://github.com/jonase/eastwood/issues/100





[CLJ-1553] Parallel transduce Created: 07/Oct/14  Updated: 22/Jan/16

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

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


 Description   

Consider how to create a parallel path for transducers, similar to reducers fold.






[CLJ-1552] Consider kv support for transducers (similar to reducers fold) Created: 07/Oct/14  Updated: 22/Jan/16

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

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


 Description   

In reducers, fold over a map has special support for kv. Consider whether/how to add this for transducers.



 Comments   
Comment by Marshall T. Vandegrift [ 16/Dec/14 11:13 AM ]

We don't have a JIRA "unvote" feature, but I'd like to register my vote against this proposed enhancement. As a heavy user of clojure.core.reducers, I consider the switch to k-v semantics when reducing a map to be a significant mis-feature. As only an initial transformation function applied directly to a map is able to receive the k-v semantics (a limitation I can’t see how would not carry over to transducers), this behavior crops up most frequently when re-ordering operations and discovering that an intermediate map has now caused an airity error somewhere in the middle of a chain of threaded transformations. I’ve never found cause to invoke it intentionally.

Comment by Ghadi Shayban [ 21/Jan/16 9:17 PM ]

Marshall, there really isn't a proposed enhancement, yet. So there's nothing to be against! Your input is valuable. (Regarding c.c.reducers, that is a separate problem – yes that behavior is surprising)

Considering kv-support for transducers:
Is it useful to have some functions that transform reduce-kv style reducing functions (fn [result k v])?

Ignore naming:
map-key
map-val
map-keyval
filter-*

These could be mechanically generated. You wouldn't have to have a kv-version for every transducer currently in core. Some like map or filter could specifically apply to the key and ignore the val, or v.v.

Some things like map's transducer would be arity-incompatible (map's transducer has a varargs arity).





[CLJ-1551] Consider transducer support for primitives Created: 07/Oct/14  Updated: 12/Jul/15

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

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

Approval: Vetted

 Description   

Need to consider how we can support primitives for transducers. In particular it may be that IFn needs overloading for L/D in addition to O.






[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