<< Back to previous view

[CLJ-2054] generator for `any?` occasionally generates `Double/NaN` for which equality semantics don't apply, and that is a problem for the :ret spec of many functions. Created: 07/Nov/16  Updated: 11/Nov/16

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

Type: Enhancement Priority: Critical
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec
Environment:

Ubuntu 16.10 - Oracle Java 8


Approval: Triaged

 Description   

The generator for `any?` will occasionally give back Double/NaN value(s). Since NaNs & equality (via `=`) don't get along, :ret spec'ing a fn which transforms/processes a collection according to a predicate, becomes rather problematic. That's because the most obvious thing to check under :ret (the case where the predicate didn't return true for any value, and so the output coll should be equal to the input coll because nothing was transformed/processed), cannot be expressed trivially.

The workaround I've come up with in my own specs is to spec the elements of the collection with `(s/and any? (complement double-NaN?))` instead of just `any?`, and it works. However, even though I can live without NaNs in the tests, I must admit it still feels sort of hacky.

Ideas:

1) The generator for `any?` could be hardcoded to never return Double/NaN. Sounds rather invasive.
2) The generator for `any?` could be reworked to somehow be configurable wrt allowing/prohibiting Double/NaNs. Then perhaps a dynamic-var and/or a macro (e.g. `without-NaNs`) could expose this (just brainstorming here).
3) The generator for `any? could stay as is, but a new equality operator could be added (e.g. `clojure.spec/===`), which somehow ignores NaNs (a naive implementation for instance might walk the data-structures and replace all NaNs with keywords, and only then perform a regular comparison).



 Comments   
Comment by Alex Miller [ 08/Nov/16 10:29 AM ]

Should consider whether this change is more appropriate in test.check or in the spec generator for any?.

Comment by Dimitrios Piliouras [ 11/Nov/16 12:31 PM ]

It turns out that my workaround does not fully work. I literally just stumbled in the following case:

{nil {[] {NaN 0}}}

which is a conforming value for:

(s/def ::persistent-map
(s/map-of ::anything-but-NaN ::anything-but-NaN)) ;; (s/and any? (complement double-NaN?))

So basically, the inner collections can still have NaNs. So far I've got 4 specs that I've written and faced this problem on all of them.





[CLJ-1891] New socket server startup proactively loads too much code, slowing boot time Created: 09/Feb/16  Updated: 09/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: server

Attachments: Text File clj-1891.patch    
Patch: Code
Approval: Prescreened

 Description   

In the new socket server code, clojure.core.server is proactively loaded (regardless of whether servers are in the config), which will also load clojure.edn and clojure.string.

Approach: Delay loading of this code until the first server config is found. This improves startup time when not using the socket server about 0.05 s.

Patch: clj-1891.patch






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

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

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

Approval: Vetted

 Description   

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

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

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

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

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

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



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

Probably similar to CLJ-700.





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

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

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

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

 Description   

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

With:

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

Before patch:

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

After patch:

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

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



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

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

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

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

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

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

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

is it possible that AOT is involved?

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

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

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

I don't see where AOT would be involved?

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

Michael Blume The updated patch should fix the issue you reported

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

Cool, thanks =)

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

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

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

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

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

Comment by Alex Miller [ 07/Jun/16 11:42 AM ]

Bumping priority as this is used in new inst? predicate - see https://github.com/clojure/clojure/commit/58227c5de080110cb2ce5bc9f987d995a911b13e





[CLJ-1793] Reducer instances hold onto the head of seqs Created: 05/Aug/15  Updated: 21/Sep/16

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 4
Labels: compiler
Environment:

1.8.0-alpha2 - 1.8.0-alpha4


Attachments: Text File 0001-Clear-this-before-calls-in-tail-position.patch     Text File clj-1793-2.patch     Text File clj-1793-3.patch    
Patch: Code and Test
Approval: Screened

 Description   

(This ticket started life as CLJ-1250, was committed in 1.8.0-alpha2, pulled out after alpha4, and this is the new version that fixes the logic about whether in a tail call as well as addresses direct linking added in 1.8.0-alpha3.)

Problem: Original example was with reducers holding onto the head of a lazy seq:

(time (reduce + 0 (map identity (range 1e8))))    ;; works
(time (reduce + 0 (r/map identity (range 1e8))))  ;; oome from holding head of range

Trickier example from CLJ-1250 that doesn't clear `this` in nested loop:

(let [done (atom false)
      f (future-call
          (fn inner []
            (while (not @done)
              (loop [found []]
                (println (conj found 1))))))]
  (doseq [elem [:a :b :c :done]]
    (println "queue write " elem))
  (reset! done true)
  @f)

Problem: #'reducer closes over a collection in order to reify CollReduce, and the closed-over collection reference is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Approach: When invoking a method in a tail call, clear 'this' prior to invoking.

The criteria for when a tail call is a safe point to clear 'this':

1) Must be in return position
2) Not in a try block (might need 'this' during catch/finally)
3) Not direct linked

Return position (#1) isn't simply (context == C.RETURN) because loop bodies are always parsed in C.RETURN context

A new dynvar METHOD_RETURN_CONTEXT tracks whether an InvokeExpr in tail position can directly leave the body of the compiled java method. It is set to RT.T in the outermost parsing of a method body and invalidated (set to null) when a loop body is being parsed where the context for the loop expression is not RETURN parsed. Added clear in StaticInvokeExpr as that is now a thing with direct linking again.

Removes calls to emitClearLocals(), which were a no-op.

Patch: clj-1793-3.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 05/Aug/15 12:16 PM ]

The this ref is cleared prior to the println, but the next time through the while loop it needs the this ref to look up the closed over done field (via getfield).

Adding an additional check to the inTailCall() method to not include tail call in a loop addresses this case:

static boolean inTailCall(C context) {
-    return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null);
+    return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null) && (LOOP_LOCALS.deref() == null);
}

But want to check some more things before concluding that's all that's needed.

Comment by Alex Miller [ 05/Aug/15 1:36 PM ]

This change undoes the desired behavior in the original CLJ-1250 (new tests don't pass). For now, we are reverting the CLJ-1250 patch in master.

Comment by Ghadi Shayban [ 05/Aug/15 3:12 PM ]

Loop exit edges are erroneously being identified as places to clear 'this'. Only exits in the function itself or the outermost loop are safe places to clear.

Comment by Ghadi Shayban [ 05/Aug/15 8:43 PM ]

Patch addresses this bug and the regression in CLJ-1250.

See the commit message for an extensive-ish comment.

Comment by Alex Miller [ 18/Aug/15 12:33 PM ]

New patch is same as old, just adds jira id to beginning of commit message.

Comment by Rich Hickey [ 24/Aug/15 10:00 AM ]

Not doing this for 1.8, more thought needs to go into whether this is the right solution to the problem. And, what is the problem? This title of this patch is just something to do.

Comment by Alex Miller [ 24/Aug/15 10:21 AM ]

changing to vetted so this is at a valid place in the jira workflow

Comment by Ghadi Shayban [ 24/Aug/15 10:45 AM ]

Rich the original context is in CLJ-1250 which was a defect/problem. It was merged and revert because of a problem in the impl. This ticket is the continuation of the previous one, but unfortunately the title lost the context and became approach-oriented and not problem-oriented. Blame Alex. (I kid, it's an artifact of the mutable approach to issue management.)

Comment by Nicola Mometto [ 23/Mar/16 7:34 AM ]

Just a note that the original ticket for this issue had 10 votes

Comment by Nicola Mometto [ 30/Mar/16 8:50 AM ]

The following code currently eventually causes an OOM to happen, the patch in this ticket correctly helps not holding onto the collection and doesn't cause memory to run infinitely

Before patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
OutOfMemoryError Java heap space  clojure.lang.RT.cons (RT.java:660)

After patch:

user=> (defn range* [x] (cons x (lazy-seq (range* (inc x)))))
#'user/range*
user=> (reduce + 0 (eduction (range* 0)))
;; runs infinitely without causing OOM
Comment by Alex Miller [ 08/Sep/16 5:14 PM ]

Refreshed patch to apply to master. No semantic changes, attribution retained.





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

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

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

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

 Description   

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

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

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

Patch: clj-1743-2.patch

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

Demo.java
package clojure_static_initialization;

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

unknown.

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

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





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

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

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

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

 Description   

Whereas

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

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

and

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

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

[1] The matter was discussed on Google Groups:

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

with a reference to an earlier thread

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

[2] CLJ-82 and the 2009 message thread



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

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

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

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

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

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

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

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

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

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





[CLJ-1544] AOT bug involving namespaces loaded before AOT compilation started Created: 01/Oct/14  Updated: 27/Oct/16

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

Type: Defect Priority: Critical
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 14
Labels: aot

Attachments: Text File 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co.patch     Text File 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v2.patch     Text File 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v3.patch     Text File 0001-CLJ-1641-disallow-circular-dependencies-even-if-the-.patch    
Patch: Code
Approval: Incomplete

 Description   

If namespace "a" that is being AOT compiled requires a namespace "b" that has been loaded but not AOT compiled, the classfile for that namespace will never be emitted on disk, causing errors when compiling uberjars or in other cases.

A minimal reproducible case is described in the following comment: http://dev.clojure.org/jira/browse/CLJ-1544?focusedCommentId=36734&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-36734

Other examples of the bug:
https://github.com/arohner/clj-aot-repro
https://github.com/methylene/class-not-found

A real issue triggered by this bug: https://github.com/cemerick/austin/issues/23

Related ticket: CLJ-1641 contains descriptions and comments about some potentially unwanted consequences of applying proposed patch 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v3.patch

Approach: The approach taken by the attached patch is to force reloading of namespaces during AOT compilation if no matching classfile is found in the compile-path or in the classpath

Patch: 0001-CLJ-1544-force-reloading-of-namespaces-during-AOT-co-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 04/Dec/14 12:45 PM ]

Possibly related: CLJ-1457

Comment by Nicola Mometto [ 05/Dec/14 4:51 AM ]

Has anyone been able to reproduce this bug from a bare clojure repl? I have been trying to take lein out of the equation for an hour but I don't seem to be able to reproduce it – this makes me think that it's possible that this is a lein/classlojure/nrepl issue rather than a compiler/classloader bug

Comment by Nicola Mometto [ 06/Dec/14 4:20 PM ]

I was actually able to reproduce and understand this bug thanks to a minimal example reduced from a testcase for CLJ-1413.

>cat error.sh
#!/bin/sh

rm -rf target && mkdir target

java -cp src:clojure.jar clojure.main - <<EOF
(require 'myrecord)
(set! *compile-path* "target")
(compile 'core)
EOF

java -cp target:clojure.jar clojure.main -e "(use 'core)"

> cat src/core.clj
(in-ns 'core)
(clojure.core/require 'myrecord)
(clojure.core/import myrecord.somerecord)

>cat src/myrecord.clj
(in-ns 'myrecord)
(clojure.core/defrecord somerecord [])

> ./error.sh
Exception in thread "main" java.lang.ExceptionInInitializerError
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.classForName(RT.java:2113)
	at clojure.lang.RT.classForName(RT.java:2122)
	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__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:630)
	at clojure.core$use.doInvoke(core.clj:5785)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval212.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6730)
	at clojure.core$eval.invoke(core.clj:3076)
	at clojure.main$eval_opt.invoke(main.clj:288)
	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.io.FileNotFoundException: Could not locate myrecord__init.class or myrecord.clj on classpath.
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5774)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at core__init.load(Unknown Source)
	at core__init.<clinit>(Unknown Source)
	... 33 more

This bug also has also affected Austin: https://github.com/cemerick/austin/issues/23

Essentially this bug manifests itself when a namespace defining a protocol or a type/record has been JIT loaded and a namespace that needs the protocol/type/record class is being AOT compiled later. Since the namespace defining the class has already been loaded the class is never emitted on disk.

Comment by Nicola Mometto [ 06/Dec/14 6:51 PM ]

I've attached a tentative patch fixing the issue in the only way I found reasonable: forcing the reloading of namespaces during AOT compilation if the compiled classfile is not found in the compile-path or in the classpath

Comment by Nicola Mometto [ 06/Dec/14 7:30 PM ]

Updated patch forces reloading of the namespace even if a classfile exists in the compile-path but the source file is newer, mimicking the logic of clojure.lang.RT/load

Comment by Nicola Mometto [ 06/Dec/14 7:39 PM ]

Further testing demonstrated that this bug is not only scoped to deftypes/defprotocols but can manifest itself in the general case of a namespace "a" requiring a namespace "b" already loaded, and AOT compiling the namespace "a"

Comment by Tassilo Horn [ 08/Dec/14 4:46 AM ]

I'm also affected by this bug. Is there some workaround I can apply in the meantime, e.g., by dictating the order in which namespaces are going to be loaded/compiled in project.clj?

Comment by Nicola Mometto [ 15/Dec/14 10:58 AM ]

Tassilo, if you don't have control over whether or not a namespace that an AOT namespace depends on has already been loaded before compilation starts, requiring those namespaces with :reload-all should be enough to work around this issue

Comment by Tassilo Horn [ 15/Dec/14 11:36 AM ]

Nicola, thanks! But in the meantime I've switched to using clojure.java.api and omit AOT-compilation. That works just fine, too.

Comment by Michael Blume [ 15/Dec/14 5:05 PM ]

Tassilo, that's often a good solution, another is to use a shim clojure class

(ns myproject.main-shim (:gen-class))

(defn -main [& args]
  (require 'myproject.main)
  ((resolve 'myproject.main) args))

then your shim namespace is AOT-compiled but nothing else in your project is.

Comment by Tassilo Horn [ 16/Dec/14 1:07 AM ]

Thanks Michael, that's a very good suggestion. In fact, I've always used AOT only as a means to export some functions to Java-land. Basically, I did as you suggest but required the to-be-exported fn's namespace in the ns-form which then causes AOT-compilation of that namespace and its own deps recursively. So your approach seems to be as convenient from the Java side (no need to clojure.java.require `require` in order to require the namespace with the fn I wanna call ) while still omitting AOT. Awesome!

Comment by Nicola Mometto [ 06/Jan/15 6:07 PM ]

I'm marking this as incomplete to prevent further screening until the bug reported here: http://dev.clojure.org/jira/browse/CLJ-1620?focusedCommentId=37232&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-37232 is figured out

Comment by Nicola Mometto [ 07/Jan/15 4:43 AM ]

Fixed the patch, I'm re marking the tickets as Vetted as it was before.

Comment by Alex Miller [ 16/Jan/15 12:54 PM ]

This patch is being rolled back for 1.7.0-alpha6 pending further investigation into underlying problems and possible solutions.

Comment by Colin Fleming [ 19/Jan/15 4:41 AM ]

I'm not 100% sure, but this looks a lot like Cursive issue 369. It had a case that I could reproduce with JDK 7 but not JDK 8, has the same mysterious missing namespace class symptom, and involves mixed AOT/non-AOT namespaces. However it's happening at runtime, not at compile time, which doesn't seem consistent.

Comment by Alex Miller [ 19/Jan/15 7:29 AM ]

My error report above was incorrectly tied to this issue (see CLJ-1636). I will delete the comment.

Comment by Nicola Mometto [ 29/Jan/15 12:23 PM ]

Since ticket CLJ-1641 has been closed, I'll repost here a comment I posted in that ticket + the patch I proposed, arguing why I think the patch I proposed for this ticket should not have been reverted:

Zach, I agree that having different behaviour between AOT and JIT is wrong.

But I also don't agree that having clojure error out on circular dependencies should be considered a bug, I would argue that the way manifold used to implement the circular dependency between manifold.stream and manifold.stream.graph was a just a hack around lack of validation in require.

My proposal to fix this disparity between AOT and JIT is by making require/use check for circular dependencies before checking for already-loaded namespaces.

This way, both under JIT and AOT code like

(ns foo.a (:require foo.b))
(ns foo.b)
(require 'foo.a)

will fail with a circular depdenency error.

This is what the patch I just attached (0001-CLJ-1641disallow-circular-dependencies-even-if-the.patch) does.

Comment by Stuart Halloway [ 15/Aug/16 1:35 PM ]

This is actually a runtime problem, not a compilation problem.

AOTed Clojure code cannot see JITed Clojure classes (vars are fine), because of the rules of Java classloading. JITed code is loaded by a DynamicClassLoader, which delegates up to the classpath loader for AOTed code.

The person who runs a particular Clojure app can solve this problem by making sure their own consumption of AOT compilation is "infectious", i.e. if you want to AOT-compile library A which uses library B, then you need to AOT-compile library B as well.

I think that attempts to have the Clojure compiler magically implement the "infectious" rule above will cause more problems than they solve, and that we should close this ticket and provide good guidance for tools like lein and boot.

Comment by Michael Sperber [ 15/Aug/16 2:24 PM ]

This problem occurs within the compilation of a single library/project, so I don't think this can be solved by simple usages of Leiningen or Boot.

Comment by John Szakmeister [ 27/Oct/16 4:54 AM ]

I just spent quite a bit of time tracking down what I thought might be a variant of this problem. I've been trying to use Colin Fleming's new gradle-clojure plugin and was running into issues with the resulting shadow jar (the equivalent of an uberjar). At the time I believed it to be a problem in the gradle-clojure plugin, but it turned out to be a different issue. The shadow plugin was not preserving the last modified time on files extracted from dependencies, and it resulted in some source files looking newer than the class files. I suspect that Clojure was then recompiling the class, thinking it was out-of-date. This was nasty to track down and quite unexpected, but I can see the sense in the behavior now that I know what's going on. I'm not sure if anything should be done on the Clojure side, but it points to a problem with including the source and AOT files together--you really need to make sure the timestamps are kept intact and I can see that fact being easily overlooked. In this case, it was a plugin completely unrelated to Clojure that had to be fixed. I should also add that taking the infectious approach Stuart mentions is probably an issue in the Gradle and (possibly?) Maven environments, since there are separate plugins for packaging the uberjar.

FWIW, I have a fair amount of information in the ticket for gradle-clojure about the failure mode and the steps I went through to try and track down the problem: https://github.com/cursive-ide/gradle-clojure/issues/8. Also, I've put a pull request in on the shadow plugin to help keep the timestamps intact: https://github.com/johnrengelman/shadow/pull/260. There is also an issue in the shadow plugin describing the problem too: https://github.com/johnrengelman/shadow/issues/259.





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

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

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

Approval: Triaged

 Description   

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

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

References:



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I've commented on the design page.

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

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

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

Related to CLJ-1530

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

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

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

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

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

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

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

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

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

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

Comment by Steven Yi [ 08/Nov/16 11:16 AM ]

Coming to this late, I had mentioned on the user mailing list in:

https://groups.google.com/forum/#!topic/clojure/CwZHu1Eszbk

that # is currently allowed as part of a symbol name, such that:

(let a# 4 b#a 3 (println a# b#a))

will print "4 3".

  1. is also employed in auto-gensyms and discussed in http://clojure.org/reference/reader#syntax-quote as part of a symbol's name. From the mailing list thread, # was noted as "may be allowed now, but could be changed later". I would appreciate if it is more clearly described as a special case/reserved, and would ask that its use be restricted in the reader to prevent users from using it now and potentially have code break later.




[CLJ-1522] Enhance multimethods metadata Created: 08/Sep/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 18
Labels: metadata

Approval: Triaged

 Description   

I think that multimethod metadata can be extended a bit with some property indicating the var in question is referring to a multimethod (we have something similar for macros) and some default arglists property.

I'm raising this issue because as a tool writer (CIDER) I'm having hard time determining if something is a multimethod (I have to resort to code like (instance? clojure.lang.MultiFn obj) which is acceptable, but not ideal I think (compared to macros and special forms)). There's also the problem that I cannot provide the users with eldoc (function signature) as it's not available in the metadata (this issue was raised on the mailing list as well https://groups.google.com/forum/#!topic/clojure/crje_RLTWdk).

I feel that we really have a problem with the missing arglist and we should solve it somehow. I'm not sure I'm suggesting the best solution and I'll certainly take any solution.



 Comments   
Comment by Bozhidar Batsov [ 09/Sep/14 4:24 AM ]

Btw, I failed to mention this as I thought it was obvious, but I think we should use the dispatch function's arglist in the multimethod metadata.





[CLJ-1517] Unrolled small vectors Created: 01/Sep/14  Updated: 18/May/16

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

Type: Enhancement Priority: Critical
Reporter: Zach Tellman Assignee: Unassigned
Resolution: Unresolved Votes: 22
Labels: collections, performance

Attachments: File unrolled-collections-2.diff     File unrolled-collections.diff     Text File unrolled-vector-2.patch     Text File unrolled-vector.patch    
Patch: Code
Approval: Incomplete

 Description   

As discussed on the mailing list [1], this patch has two unrolled variants of vectors and maps, with special inner classes for each cardinality. Currently both grow to six elements before spilling over into the general versions of the data structures, which is based on rough testing but can be easily changed. At Rich's request, I haven't included any integration into the rest of the code, and there are top-level static create() methods for each.

The sole reason for this patch is performance, both in terms of creating data structures and performing operations on them. This can be seen as a more verbose version of the trick currently played with PersistentArrayMap spilling over into PersistentHashMap. Based on the benchmarks, which can be run by cloning cambrian-collections [2] and running 'lein test :benchmark', this should supplant PersistentArrayMap. Performance is at least on par with PAM, and often much faster. Especially noteworthy is the creation time, which is 5x faster for maps of all sizes (lein test :only cambrian-collections.map-test/benchmark-construction), and on par for 3-vectors, but 20x faster for 5-vectors. There are similar benefits for hash and equality calculations, as well as calls to reduce().

This is a big patch (over 5k lines), and will be kind of a pain to review. My assumption of correctness is based on the use of collection-check, and the fact that the underlying approach is very simple. I'm happy to provide a high-level description of the approach taken, though, if that will help the review process.

I'm hoping to get this into 1.7, so please let me know if there's anything I can do to help accomplish that.

[1] https://groups.google.com/forum/#!topic/clojure-dev/pDhYoELjrcs
[2] https://github.com/ztellman/cambrian-collections

Patch: unrolled-vector-2.patch

Screener Notes: The approach is clear and understandable. Given the volume of generated code, I believe that best way to improve confidence in this code is to get people using it asap, and add collection-test [3] to the Clojure test suite. I would also like to get the generator [4] included in the Clojure repo. We don't need to necessarily automate running it, but would be nice to have it nearby if we want to tweak something later.

[3] https://github.com/ztellman/collection-check/blob/master/src/collection_check.clj
[4] https://github.com/ztellman/cambrian-collections/blob/master/generate/cambrian_collections/vector.clj



 Comments   
Comment by Zach Tellman [ 01/Sep/14 10:13 PM ]

Oh, I forgot to mention that I didn't make a PersistentUnrolledSet, since the existing wrappers can use the unrolled map implementation. However, it would be moderately faster and more memory efficient to have one, so let me know if it seems worthwhile.

Comment by Nicola Mometto [ 02/Sep/14 5:23 AM ]

Zach, the patch you added isn't in the correct format, they need to be created using `git format-patch`

Comment by Nicola Mometto [ 02/Sep/14 5:31 AM ]

Also, I'm not sure if this is on-scope with the ticket but those patches break with *print-dup*, as it expects a static create(x) method for each inner class.

I'd suggest adding a create(Map x) static method for the inner PersistentUnrolledMap classes and a create(ISeq x) one for the inner PersistentUnrolledVector classes

Comment by Alex Miller [ 02/Sep/14 8:14 AM ]

Re making patches, see: http://dev.clojure.org/display/community/Developing+Patches

Comment by Jozef Wagner [ 02/Sep/14 9:16 AM ]

I wonder what is the overhead of having meta and 2 hash fields in the class. Have you considered a version where the hash is computed on the fly and where you have two sets of collections, one with meta field and one without, using former when the actual metadata is attached to the collection?

Comment by Zach Tellman [ 02/Sep/14 12:13 PM ]

I've attached a patch using the proper method. Somehow I missed the detailed explanation for how to do this, sorry. I know the guidelines say not to delete previous patches, but since the first one isn't useful I've deleted it to minimize confusion.

I did the print-dup friendly create methods, and then realized that once these are properly integrated, 'pr' will just emit these as vectors. I'm fairly sure the create methods aren't necessary, so I've commented them out, but I'm happy to add them back in if they're useful for some reason I can't see.

I haven't given a lot of thought to memory efficiency, but I think caching the hashes are worthwhile. I can see an argument for creating a "with-meta" version of each collection, but since that would double the size of an already enormous patch, I think that should probably wait.

Comment by Zach Tellman [ 03/Sep/14 4:31 PM ]

I found a bug! Like PersistentArrayMap, I have a special code path for comparing keywords, but my generators for collection-check were previously using only integer keys. There was an off-by-one error in the transient map implementation [1], which was not present for non-keyword lookups.

I've taken a close look for other gaps in my test coverage, and can't find any. I don't think this substantively changes the risk of this patch (an updated version of which has been uploaded as 'unrolled-collections-2.diff'), but obviously where there's one bug, there may be others.

[1] https://github.com/ztellman/cambrian-collections/commit/eb7dfe6d12e6774512dbab22a148202052442c6d#diff-4bf78dbf5b453f84ed59795a3bffe5fcR559

Comment by Zach Tellman [ 03/Oct/14 2:34 PM ]

As an additional data point, I swapped out the data structures in the Cheshire JSON library. On the "no keyword-fn decode" benchmark, the current implementation takes 6us, with the unrolled data structures takes 4us, and with no data structures (just lexing the JSON via Jackson) takes 2us. Other benchmarks had similar results. So at least in this scenario, it halves the overhead.

Benchmarks can be run by cloning https://github.com/dakrone/cheshire, unrolled collections can be tested by using the 'unrolled-collections' branch. The pure lexing benchmark can be reproduced by messing around with the cheshire.parse namespace a bit.

Comment by Zach Tellman [ 06/Oct/14 1:31 PM ]

Is there no way to get this into 1.7? It's an awfully big win to push off for another year.

Comment by Alex Miller [ 07/Oct/14 2:08 PM ]

Hey Zach, it's definitely considered important but we have decided to drop almost everything not fully done for 1.7. Timeframe for following release is unknown, but certainly expected to be significantly less than a year.

Comment by John Szakmeister [ 30/Oct/14 2:53 PM ]

You are all free to determine the time table, but I thought I'd point out that Zach is not entirely off-base. Clojure 1.4.0 was released April 5th, 2012. Clojure 1.5.0 was released March 1st, 2013 with 1.6.0 showing up March 25th, 2014. So it appears that the current cadence is around a year.

Comment by Alex Miller [ 30/Oct/14 3:40 PM ]

John, there is no point to comments like this. Let's please keep issue comments focused on the issue.

Comment by Zach Tellman [ 13/Nov/14 12:23 PM ]

I did a small write-up on this patch which should help in the eventual code review: http://blog.factual.com/using-clojure-to-generate-java-to-reimplement-clojure

Comment by Zach Tellman [ 07/Dec/14 10:34 PM ]

Per my conversation with Alex at the Conj, here's a patch that only contains the unrolled vectors, and uses the more efficient constructor for PersistentVector when spilling over.

Comment by Alex Miller [ 08/Dec/14 1:10 PM ]

Zach, I created a new placeholder for the map work at http://dev.clojure.org/jira/browse/CLJ-1610.

Comment by Jean Niklas L'orange [ 09/Dec/14 1:52 PM ]

It should probably be noted that core.rrb-vector will break for small vectors by this patch, as it peeks into the underlying structure. This will also break other libraries which peeks into the vector implementation internals, although I'm not aware of any other – certainly not any other contrib library.

Also, two comments on unrolled-vector.patch:

private transient boolean edit = true;
in the Transient class should probably be
private volatile boolean edit = true;
as transient means something entirely different in Java.

conj in the Transient implementation could invalidate itself without any problems (edit = false;) if it is converted into a TransientVector (i.e. spills over) – unless it has a notable overhead. The invalidation can prevent some subtle bugs related to erroneous transient usage.

Comment by Alex Miller [ 09/Dec/14 1:58 PM ]

Jean - understanding the scope of the impact will certainly be part of the integration process for this patch. I appreciate the heads-up. While we try to minimize breakage for things like this, it may be unavoidable for libraries that rely on implementation internals.

Comment by Michał Marczyk [ 09/Dec/14 2:03 PM ]

I'll add support for unrolled vectors to core.rrb-vector the moment they land on master. (Probably with some conditional compilation so as not to break compatibility with earlier versions of Clojure – we'll see when the time comes.)

Comment by Michał Marczyk [ 09/Dec/14 2:06 PM ]

I should say that it'd be possible to add generic support for any "vector lookalikes" by pouring them into regular vectors in linear time. At first glance it seems to me that that'd be out of line with the basic promise of the library, but I'll give it some more thought before the changes actually land.

Comment by Zach Tellman [ 09/Dec/14 5:43 PM ]

Somewhat predictably, the day after I cut the previous patch, someone found an issue [1]. In short, my use of the ArrayChunk wrapper applied the offset twice.

This was not caught by collection-check, which has been updated to catch this particular failure. It was, however, uncovered by Michael Blume's attempts to merge the change into Clojure, which tripped a bunch of alarms in Clojure's test suite. My own attempt to do the same to "prove" that it worked was before I added in the chunked seq functionality, hence this issue persisting until now.

As always, there may be more issues lurking. I hope we can get as many eyeballs on the code between now and 1.8 as possible.

[1] https://github.com/ztellman/cambrian-collections/commit/2e70bbd14640b312db77590d8224e6ed0f535b43
[2] https://github.com/MichaelBlume/clojure/tree/test-vector

Comment by Zach Tellman [ 10/Jul/15 1:54 PM ]

As a companion to the performance analysis in the unrolled map issue, I've run the benchmarks and posted the results at https://gist.github.com/ztellman/10e8959501fb666dc35e. Some notable results:

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

Stu: I do not think this patch should be marked "screened" until the actual integration and build work (if the generator is integrated) has been completed.

Comment by Alex Miller [ 14/Jul/15 4:33 PM ]

FYI, we have "reset" all big features for 1.8 for the moment (except the socket repl work). We may still include it - that determination will be made later.

Comment by Zach Tellman [ 14/Jul/15 4:43 PM ]

Okay, any idea when the determination will be made? I was excited that we seemed to be finally moving forward on this.

Comment by Alex Miller [ 14/Jul/15 4:51 PM ]

No, but it is now on my work list.

Comment by Rich Hickey [ 15/Jul/15 8:17 AM ]

I wonder if all of the overriding of APersistentVector yields important benefits - e.g. iterator, hashcode etc.

Comment by Zach Tellman [ 15/Jul/15 11:51 AM ]

In the case of hashcode, definitely: https://gist.github.com/ztellman/10e8959501fb666dc35e#file-gistfile1-txt-L1013-L1076. This was actually one of the original things I wanted to speed up.

In the case of the iterator, probably not. I'd be fine removing that.

Comment by Zach Tellman [ 16/Jul/15 5:17 PM ]

So am I to infer from https://github.com/clojure/clojure/commit/36d665793b43f62cfd22354aced4c6892088abd6 that this issue is defunct? If so, I think there's a lot of improvements being left on the table for no particular reason.

Comment by Rich Hickey [ 16/Jul/15 6:34 PM ]

Yes, that commit covers this functionality. It takes a different approach from the patch in building up from a small core, and maximizing improvements to the bases rather than having a lot of redundant definitions per class. That also allowed for immediate integration without as much concern for correctness, as there is little new code. It also emphasizes the use case for tuples, e.g. small vectors used as values that won't be changed, thus de-emphasizing the 'mutable' functions. I disagree that many necessary improvements are being left out. The patch 'optimized' many things that don't matter. Further, there are not big improvements to the pervasive inlining. In addition, the commit includes the integration work at a fraction of the size of the patch. In all, it would have taken much more back and forth to get the patch to conform with this approach than to just get it all done, but I appreciate the inspiration and instigation - thanks!

Comment by Rich Hickey [ 16/Jul/15 6:46 PM ]

That said, this commit need not be the last word - it can serve as a baseline for further optimization. But I'd rather that be driven by need. Clojure could become 10x as large optimizing things that don't matter.

Comment by Zach Tellman [ 19/Jul/15 1:36 PM ]

What is our reference for "relevant" performance? I (or anyone else) can provide microbenchmarks for calculating hashes or whatever else, but that doesn't prove that it's an important improvement. I previously provided benchmarks for JSON decoding in Cheshire, but that's just one of many possible real-world benchmarks. It might be useful to have an agreed-upon list of benchmarks that we can use when debating what is and isn't useful.

Comment by Mike Anderson [ 19/Jul/15 11:14 PM ]

I was interested in this implementation so created a branch that integrates Zach's unrolled vectors on top of clojure 1.8.0-alpha2. I also simplified some of the code (I don't think the metadata handling or unrolled seqs are worthwhile, for example)

Github branch: https://github.com/mikera/clojure/tree/clj-1517

Then I ran a set of micro-benchmarks created by Peter Taoussanis

Results: https://gist.github.com/mikera/72a739c84dd52fa3b6d6

My findings from this testing:

  • Performance is comparable (within +/- 20%) on the majority of tests
  • Zach's approach is noticeably faster (by 70-150%) for 4 operations (reduce, mapv, into, equality)

My view is that these additional optimisations are worthwhile. In particular, I think that reduce and into are very important operations. I also care about mapv quite a lot for core.matrix (It's fundamental to many numerical operations on arrays implemented using Clojure vectors).

Happy to create a patch along these lines if it would be acceptable.

Comment by Zach Tellman [ 19/Jul/15 11:45 PM ]

The `reduce` improvements are likely due to the unrolled reduce and kvreduce impls, but the others are probably because of the unrolled transient implementation. The extra code required to add these would be pretty modest.

Comment by Mike Anderson [ 20/Jul/15 9:20 PM ]

I actually condensed the code down to a single implementation for `Transient` and `TupleSeq`. I don't think these really need to be fully unrolled for each Tuple type. That helps by making the code even smaller (and probably also helps performance, given JVM inline caching etc.)

Comment by Peter Taoussanis [ 21/Jul/15 11:46 AM ]

Hey folks,

Silly question: is there actually a particular set of reference benchmarks that everyone's currently using to test the work on tuples? It took me a while to notice how bad the variance was with my own set of micro benchmarks.

Bumping all the run counts up till the noise starts ~dying down, I'm actually seeing numbers now that don't seem to agree with others here .

Google Docs link: https://docs.google.com/spreadsheets/d/1QHY3lehVF-aKrlOwDQfyDO5SLkGeb_uaj85NZ7tnuL0/edit?usp=sharing
gist with the benchmarks: https://gist.github.com/ptaoussanis/0a294809bc9075b6b02d

Thanks, cheers!

Comment by Zach Tellman [ 21/Jul/15 6:52 PM ]

Hey Peter, I can't reproduce your results, and some of them are so far off what I'd expect that I have to think there was some data gathering error. For instance, the assoc operation being slower is kind of inexplicable, considering the unrolled version doesn't do any copying, etc. Also, all of your numbers are significantly slower than the ones on my 4 year old laptop, which is also a bit strange.

Just to make sure that we're comparing apples to apples, I've adapted your benchmarks into something that pretty-prints the mean runtime and variance for 1.7, 1.8-alpha2, and Mike's 1517 fork. It can be found at https://github.com/ztellman/tuple-benchmark, and the results of a run at https://gist.github.com/ztellman/3701d965228fb9eda084.

Comment by Mike Anderson [ 22/Jul/15 2:24 AM ]

Hey Zach just looked at your benchmarks and they are definitely more consistent with what I am seeing. The overall nanosecond timings look about right from my experience with similar code (e.g. working with small vectors in vectorz-clj).

Comment by Peter Taoussanis [ 22/Jul/15 2:41 AM ]

Hi Zach, thanks for that!

Have updated the results -
Gist: https://gist.github.com/ptaoussanis/0a294809bc9075b6b02d
Google docs: https://goo.gl/khgT83

Note that I've added an extra sheet/tab to the Google doc for your own numbers at https://gist.github.com/ztellman/3701d965228fb9eda084.

Am still struggling to produce results that show any consistent+significant post-JIT benefit to either of the tuple implementations against the micro benchmarks and one larger small-vec-heavy system I had handy.

It's looking to me like it's maybe possible that the JIT's actually optimising away most of the non-tuple inefficiencies in practice?

Of course it's very possible that my results are off, or my expectations wrong. The numbers have been difficult to pin down.

It definitely helped to have a standardised reference micro benchmark to work against (https://github.com/ztellman/tuple-benchmark). Could I perhaps suggest a similar reference macro benchmark (maybe something from core.matrix, Mike?)

Might also be a good idea to define a worthwhile target performance delta for ops like these that run in the nanosecond range (or for the larger reference macro benchmark)?

Just some thoughts from someone passing through in case they're at all useful; know many of you have been deeply involved in this for some time so please feel free to ignore any input that's not helpful

Appreciate all the efforts, cheers!

Comment by Rich Hickey [ 22/Jul/15 9:24 AM ]

I think everyone should back off on their enthusiasm for this approach. After much analysis, I am seeing definite negative impacts to tuples, especially the multiple class approach proposed by Zach. What happens in real code is that the many tuple classes cause call sites that see different sized vectors to become megamorphic, and nothing gets adequately optimized. In particular, call sites that will see tuple-sized and large vectors (i.e. a lot of library code) will optimize differently depending upon which they see often first. So, if you run your first tight loop on vector code that sees tuples, that code could later do much worse (50-100%) on large vectors than before the tuples patch was in place. Being much slower on large collections is a poor tradeoff for being slightly faster on small ones.

Certainly different tuple classes for arity 0-6 is a dead idea. You get as good or better optimization (at some cost in size) from a single class e.g. one with four fields, covering sizes 0-4. I have a working version of this in a local branch. It is better in that sites that include pvectors are only bi-morphic, but I am still somewhat skittish given what I've seen.

The other takeaway is that the micro benchmarks are nearly worthless for detecting these issues.

Comment by Zach Tellman [ 22/Jul/15 11:07 AM ]

I'm pretty sure that all of my real-world applications of the tuples (via clj-tuple) have been fixed cardinality, and wouldn't have surfaced any such issue. Thanks for putting the idea through its paces.

Comment by Mike Anderson [ 22/Jul/15 10:37 PM ]

Rich these are good insights - do you have a benchmark that you are using as representative of real world code?

I agree that it is great if we can avoid call sites becoming megamorphic, though I also believe the ship has sailed on that one already when you consider the multiple types of IPersistentVector that already exist (MapEntry, PersistentVector, SubVector plus any library-defined IPersistentVector instances such as clojure.core.rrb-vector). As a consequence, the JVM is usually not going to be able to prove that a specific IPersistentVector interface invocation is monomorphic, which is when the really big optimisations happen.

In most of the real world code that I've been working with, the same size/type of vector gets used repeatedly (Examples: iterating over map entries, working with a sequence of size-N vectors), so in such cases we should be able to rely on the polymorphic inline cache doing the right thing.

The idea of a single Tuple class for sizes 0-4 is interesting, though I can't help thinking that a lot of the performance gain from this may stem from the fact that a lot of code does stuff like (reduce conj [] .....) or the transient equivalent which is a particularly bad use case for Tuples, at least from the call site caching perspective. There may be a better way to optimise such cases rather than simply trying to make Tuples faster.... e.g. calling asTransient() on a Tuple0 could perhaps switch straight into the PersistentVector implementation.

Comment by Colin Fleming [ 18/May/16 8:32 PM ]

Here's a relevant issue from Google's Guava project, where they also found serious performance degradation with fixed-size collections: https://github.com/google/guava/issues/1268. It has a lot of interesting detail about how the performance breaks down.





[CLJ-1458] Enhance the performance of map merges Created: 04/Jul/14  Updated: 28/Sep/16

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

Type: Enhancement Priority: Critical
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: performance

Attachments: Text File 0001-very-simple-test-of-the-merge-function.patch     Text File clj-1458-4.patch     Text File CLJ-1458-5.patch     Text File CLJ-1458-6.patch     Text File CLJ-1458-7.patch     Text File CLJ-1458-transient-merge2.patch     Text File CLJ-1458-transient-merge3.patch     Text File CLJ-1458-transient-merge.patch     Text File merge-test-2.patch     File transient-merge.diff    
Patch: Code and Test
Approval: Triaged

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-7.patch

Approach
Migrate c.c/merge later in core after transients & reduce. Leave older version as merge1 for use in cases the precede the newer definition. Make APersistentMap/conj & ATransientMap/cons aware of IKVReduce.

The attached patch preserves two existing behaviors of merge

  • metadata propagation
  • the right hand side of the merges can be a Map.Entry, an IPersistentVector where size=2, and regular maps.

Screened by:



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.

Comment by Ghadi Shayban [ 28/Dec/14 10:07 PM ]

alternate approach attached delaying merge until after protocols load, and then using transducers.

Comment by Michael Blume [ 28/Dec/14 11:50 PM ]

Looks like you're doing (get m k) twice – shouldn't that be thrown in a local?

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

um, put, in a local, I mean, 'throw' was a bad choice of word.

Comment by Ghadi Shayban [ 29/Dec/14 2:14 PM ]

Yeah there's that – won't be using get anyways after CLJ-700 gets committed.

We should add performance tests too. merging two maps, three, many maps, also varying the sizes of the maps, and for merge-with, varying the % of collisions.

Need to go back to the (some identity) logic, otherwise metadata is propagated from maps other than the first provided. I'll fix later.

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

I don't know if this is supposed to be allowed, but this breaks

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

https://github.com/metosin/compojure-api/blob/0.16.6/src/compojure/api/meta.clj#L198

Comment by Michael Blume [ 29/Dec/14 2:54 PM ]

Ghadi, contains? uses get under the covers, so it's still two gets, right? It seems like it'd be more performant to stick with the ::none trick.

Comment by Nicola Mometto [ 29/Dec/14 5:36 PM ]

This calls for if-let + find.

Comment by Ghadi Shayban [ 29/Dec/14 10:37 PM ]

new patch addressing concerns so far

Comment by Ghadi Shayban [ 29/Dec/14 10:48 PM ]

CLJ-1458-transient-merge3.patch removes silly inlining macro, uses singleton fns instead.

Comment by Michael Blume [ 29/Dec/14 11:14 PM ]

Nice =)

This should come with tests. If we want to preserve the ability to merge with a MapEntry, we should test it. This isn't so much a weakness of the patch as of the existing tests. I see merge and merge-with being used a few times in the test suite, but I see no test whose purpose is to test their behavior.

Comment by Michael Blume [ 29/Dec/14 11:17 PM ]

Extremely simple merge test, we need more than this, but this is a start

Comment by Alex Miller [ 22/Jun/15 10:11 AM ]

clj-1458-4.patch refreshed to apply to master, no changes.

Comment by Ghadi Shayban [ 09/Jan/16 5:09 PM ]

I'd like to reevaluate the scope of this ticket. Can we address 'merge' only and defer 'merge-with'? It's by far the more common function. I've attached a new simplified patch.

Comment by Ghadi Shayban [ 09/Jan/16 9:50 PM ]

CLJ-1458-6.patch is yet another cleaner approach

Comment by Alex Miller [ 01/Feb/16 5:17 AM ]

Can you update the ticket approach section to discuss the APersistentMap.cons / ASSOC changes. Also, can you add a before / after perf test for one or more common cases?

Comment by Michael Blume [ 28/Sep/16 1:55 PM ]

Updated patch to handle use of merge in core_print before it's defined in core

Comment by Ghadi Shayban [ 28/Sep/16 2:22 PM ]

If anyone wants to take stewardship of this, go ahead. I had trouble getting consistent performance improvements on this. Obviously this needs fresh benchmarks.

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

Yes, this needs a benchmark showing demonstrable improvement. The whole goal here is improved perf - if we can't prove it's consistently faster, then there is no point in even reviewing it.





[CLJ-1454] Add deref-swap! and deref-reset! (swap! and reset! that return prior value) Created: 28/Jun/14  Updated: 10/Mar/16

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

Type: Enhancement Priority: Critical
Reporter: Philip Potter Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: atom

Attachments: File deref-swap-deref-reset-extending-IAtom.diff     File deref-swap-deref-reset-with-IAtomDeref-and-tests.diff    
Patch: Code and Test
Approval: Prescreened

 Description   

Sometimes, when swapping or resetting an atom, it's desirable to know the value before the update. The existing swap! and reset! functions return the new value instead. Currently, the only option is to roll your own using a loop and compare-and-set!

Example use cases:

  • When an atom contains a PersistentQueue and you want to atomically remove the head of the queue and process it: if you run (swap! q pop), you have lost the reference to the old head of the list so you can't process it.
  • Want to check if an operation has occurred before by using atom as a flag (this can be achieved with compare-and-set! but reads a little easier this way).
    (def has-run-once (atom false))
    ...
    (when-not (get-and-set! has-run-once true)
    (do something))
  • Want to use an atom similarly to a java.util.concurrent.LinkedTransferQueue, for the case of pairing up adds by writers and drainTo y readers:
    Thread 1: (swap! atm conj item1)
    Thread 2: (swap! atm conj item2)
    Thread 3: (let [new-vals (get-and-set! atm [])] 
    (do-something new-vals))

Approach:

  • Extends clojure.lang.IAtom with derefReset and derefSwap with the exact same arities as their original variants, .reset and .swap. The deref-methods returns the old (deref-ed) value of the atom after a successful mutation of the atom.
  • Added clojure.core/deref-reset! - just like reset! it unconditionally changes values of atoms but returns
    the old atom value. Based on Steven Yi's get-and-set! in ticket CLJ-1599, patch named get-and-set.diff
  • Added clojure.core/deref-swap! - just like swap! but returns the old atom value.

Patch: deref-swap-deref-reset-with-IAtomDeref-and-tests.diff



 Comments   
Comment by Philip Potter [ 28/Jun/14 4:00 PM ]

Overtone already defines functions like this in overtone.helpers.ref, which get used by overtone.libs.event. These return both the old and the new value, although in all existing use cases only the old value gets used.

flatland/useful defines a trade! fn which returns the old value, although the implementation is less clean than a compare-and-set! based solution would be.

Comment by Philip Potter [ 29/Jun/14 6:23 AM ]

Chris Ford suggested "swap-out!" as a name for this function. I definitely think "swap-returning-old!" isn't the ideal name.

Comment by Jozef Wagner [ 30/Jun/14 1:33 AM ]

I propose a switch! name. The verb switch is defined as "substitute (two items) for each other; exchange.", and as you get the old value back, it evokes slightly the exchange of items.

Comment by Philip Potter [ 30/Jun/14 3:03 AM ]

Medley also has a deref-swap! which does the same thing.

Comment by Alex Miller [ 30/Jun/14 8:20 AM ]

I think deref-swap! seems like a morally equivalent name to Java's AtomicReference.getAndSet() which is the same idea.

Comment by Philip Potter [ 30/Jun/14 1:19 PM ]

Funny you say that Alex, because prismatic/plumbing defines a get-and-set! (also defined by other projects), equivalent to deref-reset! in medley. Plumbing also defines swap-pair! which returns both old and new values, like the overtone fn, although once again the only usage I can find only uses the old value.

Comment by Alex Miller [ 30/Jun/14 3:37 PM ]

I think it's important to retain the notion that you are not switching/exchanging values but applying the update model of applying a function to the old value to produce the new value. I don't even particularly like "swap!" as I think that aspect is lost in the name (alter and alter-var-root are better). I like that "deref-swap!" combines two words with existing connotations and orders them appropriately.

Comment by Timothy Baldridge [ 30/Jun/14 3:43 PM ]

except that that naming doesn't fit well compared to functions like nfirst which are defined as (comp next first). This function is not (comp deref swap!).

Comment by Linus Ericsson [ 29/Feb/16 11:03 AM ]

The patch deref-swap-deref-reset-extending-IAtom.diff addresses CLJ-1454 and CLJ-1599 "Add deref-reset! (reset! that returns old value)".

The patch extends IAtom with .derefSwap and .derefReset methods with identical signatures as to .swap and .reset but are intended to return the derefed (old) value of the atom upon successful mutation of the atom.

The commit message (could be used as an updated description of this ticket, or a new one)

Extends clojure.lang.IAtom with derefReset and derefSwap with the
exact same arities as their original variants, .reset and .swap.

The deref-methods returns the old (derefed) value of the atom after
a successful mutation of the atom.

Added functions:

clojure.core/deref-reset!
Just like reset! it unconditionally changes values of atoms but returns
the old atom value.

clojure.core/deref-swap!
Just like swap! but returns the old atom value.

The deref-reset is based on Steven Yi's get-and-set! in ticket CLJ-1599,
patch named get-and-set.diff

Comment by Alex Miller [ 01/Mar/16 8:46 AM ]

Review:

  • remove ":static true" in the new fns, this is dead
  • as mentioned on the mailing list, I think we need to consider that we are introducing a breaking change here. Rather than modifying IAtom we should add the methods in a new interface IAtomDeref that Atom implements. The Clojure functions should then type-hint to IAtomDeref instead.
Comment by Alex Miller [ 01/Mar/16 8:47 AM ]

Oh, and needs tests!

Comment by Linus Ericsson [ 02/Mar/16 2:14 PM ]

Alex, in deref-swap-deref-reset-with-IAtomDeref-and-tests.diff you'll find a patch with the differences

  • removed ":static true"
  • added an IAtomDeref interface instead of extending IAtom
  • 2 test cases for deref-reset!
  • 1 test case for deref-swap!

Please especially have a look at the test-cases. I tried to catch the various arities of deref-swap! but I don't know if binding warn-on-reflection true actually affects the test cases in any way. Suggestions on how to test wether "arity works" - if even worth the effort to test - are most welcome.

Given the nature of atoms, a suite of generative tests would probably be the best idea. Until then, simple functional tests for the other atom functions should be added, but that's for another ticket.

Comment by Linus Ericsson [ 02/Mar/16 2:34 PM ]

(the second patch is independent from the first one)

Comment by Alex Miller [ 10/Mar/16 9:13 AM ]

Looks good to me, marking prescreened.





[CLJ-1289] aset-* and aget perform poorly on multi-dimensional arrays even with type hints. Created: 01/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium


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

 Description   

Here's a transcript of the behavior. I don't know for sure that reflection is being done, but the performance penalty (about 1300x) suggests it.

user=> (use 'criterium.core)
nil
user=> (def b (make-array Double/TYPE 1000 1000))
#'user/b
user=> (quick-bench (aget ^"[[D" b 304 175))
WARNING: Final GC required 3.5198021166354323 % of runtime
WARNING: Final GC required 29.172288684474303 % of runtime
Evaluation count : 63558 in 6 samples of 10593 calls.
             Execution time mean : 9.457308 µs
    Execution time std-deviation : 126.220954 ns
   Execution time lower quantile : 9.344450 µs ( 2.5%)
   Execution time upper quantile : 9.629202 µs (97.5%)
                   Overhead used : 2.477107 ns

One workaround is to use multiple agets.

user=> (quick-bench (aget ^"[D" (aget ^"[[D" b 304) 175))
WARNING: Final GC required 40.59820310542545 % of runtime
Evaluation count : 62135436 in 6 samples of 10355906 calls.
             Execution time mean : 6.999273 ns
    Execution time std-deviation : 0.112703 ns
   Execution time lower quantile : 6.817782 ns ( 2.5%)
   Execution time upper quantile : 7.113845 ns (97.5%)
                   Overhead used : 2.477107 ns

Cause: The inlined version only applies to arity 2, and otherwise it reflects.



 Comments   
Comment by Gary Fredericks [ 08/Dec/13 9:28 PM ]

A glance at the source makes it obvious that the hypothesis is correct – the inlined version only applies to arity 2, and otherwise it reflects.

I thought this would be as simple as converting the inline function to be variadic (using reduce), but after trying it I realized this is tricky as you have to generate the correct type hints for each step. E.g., given ^"[[D" the inline function needs to type-hint the intermediate result with ^"[D". This isn't difficult if we're just dealing with strings that begin with square brackets, but I don't know for sure that those are the only possibilities.

Comment by Yaron Peleg [ 13/Feb/14 4:44 AM ]

Bump. I just got bitten bad by this.

There are two seperate issues here:
1) (aget 2d-array-doubles 0 0 ) doesn't emit a reflection warning.
2) It seems like the compiler has enough information to avoid the reflective call here.

Note this gets exp. worse as number of dimensions grows, i.e (get doubles3d 0 0 0)
will be 1M slower, etc' Not true, unless you iterate over all elements. it's
simply n_dims*1000x per lookup.

Nasty surprise, especially considering you often go to primitive arrays for speed,
and a common use case is an inner loop(s) that iterate over arrays.

Comment by Gary Fredericks [ 13/Feb/14 7:08 AM ]

I can probably take a stab at this.

Comment by Gary Fredericks [ 13/Feb/14 8:34 PM ]

I think the reflection warning problem is pretty much impossible to solve without changing code elsewhere in the compiler, because the reflection done in aget is a different kind than normal clojure reflection – it's explicitly in the function body rather than emitted by the compiler. Since the compiler isn't emitting it, it doesn't reasonably know it's even there. So even if aget is fixed for other arities, you still won't get the warning when it's not inlined.

I can imagine some sort of metadata that you could put on a function telling the compiler that it will reflect if not inlined. Or maybe a more generic not-inlined warning?

The global scope of adding another compiler flag seems about balanced by the seriousness of array functions not being able to warn on reflection.

Comment by Gary Fredericks [ 13/Feb/14 8:52 PM ]

Attached CLJ-1289-p1.patch which simply inlines variadic calls to aget. It assumes that if it sees a :tag on the array arg that is a string beginning with [, it can assume that the return value from one call to aget can be tagged with the same string with the leading [ stripped off.

I'm not a jvm expert, but having read through the spec a little bit I think this is a reasonable assumption.

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

I think this probably is actually true, but a more official way to ask that question would be to get the array class and ask for Class.getComponentType() (and less janky than string munging).

Comment by Gary Fredericks [ 14/Feb/14 3:40 PM ]

How would you get the array class based on the :tag type hint?

Comment by Gary Fredericks [ 14/Feb/14 7:05 PM ]

I see (-> s (Class/forName) (.getComponentType) (.getName)) does the same thing – is that route preferred, or is there another one?





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 7
Labels: walk

Attachments: Text File 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch     Text File 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch    
Patch: Code
Approval: Triaged

 Description   

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the walk protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, commit 730eb75 in clojure.walk2



 Comments   
Comment by Vjeran Marcinko [ 19/Oct/13 12:32 PM ]

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

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

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

Comment by Alex Miller [ 23/Nov/13 1:52 AM ]

This patch and ticket are still candidates for future release.

Comment by Stuart Sierra [ 20/Dec/13 9:14 AM ]

Added new patch that applies on latest master after CLJ-1105.

Comment by Chouser [ 27/Feb/14 10:26 AM ]

The way this patch behaves can be surprising compared to regular maps:

(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}

Note how the [:a 1] entry is removed from the map, but not from the record.

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then walk could take advantage of that.

Comment by Alex Miller [ 27/Feb/14 2:11 PM ]

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

Comment by Alex Miller [ 27/Feb/14 3:54 PM ]

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.

Comment by Nicola Mometto [ 27/Feb/14 5:17 PM ]

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 14/May/15

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

Type: Enhancement Priority: Critical
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: clojure.test

Attachments: Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

(use 'clojure.test)
(deftest ex-test (throw (ex-info "err" {:some :data})))
(ex-test)

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:2)
    clojure.test$test_var$fn__7666.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    ...

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

After:

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
{:some :data}
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:3)
    clojure.test$test_var$fn__7667.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output

Comment by Stuart Sierra [ 05/Sep/14 3:24 PM ]

As pointed out on IRC, there's a possible risk of trying to print an infinite lazy sequence that happened to be included in ex-data.

To mitigate, consider binding *print-length* and *print-level* to small numbers around the call to pr.

Comment by Stephen C. Gilardi [ 13/May/15 2:39 PM ]

http://dev.clojure.org/jira/browse/CLJ-1716 may cover this well enough that this issue can be closed.

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

I don't think 1716 covers it at all as clojure.test/clojure.stacktrace don't use the new throwable printing. But they could! And that might be a better solution than the patch here.

For example, the existing patch does not consider what to do about nested exceptions, some of which might have ex-data. The new printer handles all that in a consistent way.





[CLJ-706] make use of deprecated namespaces/vars easier to spot Created: 05/Jan/11  Updated: 11/Mar/16

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

Type: Enhancement Priority: Critical
Reporter: Stuart Halloway Assignee: Cezary Kosko
Resolution: Unresolved Votes: 19
Labels: errormsgs

Attachments: File 706-deprecated-ns-var-warnings-tested-2.diff     File 706-deprecated-ns-var-warnings-tested-3.diff     File 706-deprecated-ns-var-warnings-tested.diff     File 706-deprecated-var-warning.diff     Text File 706-deprecated-var-warning-patch-v2.txt     File 706-fix-deprecation-warnings-agents.diff     File 706-fix-deprecation-warnings-on-replicate.diff     File 706-fix-deprecation-warning-test-junit.diff     File 706-warning-on-deprecated-ns.diff    
Patch: Code and Test
Approval: Prescreened

 Description   

From the mailing list http://groups.google.com/group/clojure/msg/c41d909bd58e4534. It is easy to use deprecated namespaces or vars without knowing you are doing so. The documentation warnings are small, and there is no compiler warning.

Proposed:

  • Add new *warn-on-deprecated* dynamic var, defaulted to false
  • Warn to stderr when {:deprecated true} namespace is loaded.
  • Warn to stderr when {:deprecated true} var is analyzed.
  • Warn to stderr when {:deprecated true} macro is expanded.
  • New system property clojure.compiler.warn-on-deprecated
  • Compile Clojure itself with clojure.compiler.warn-on-deprecated
  • Fix deprecation warnings inside Clojure (replicate, clear-agent-errors)
  • Mark clojure.parallel as deprecated with :deprecation tag

Examples:

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

;; use of a deprecated var (on compile)
(defn ^:deprecated f [x] x)
(f 5)
;;=> Deprecation warning, NO_SOURCE_PATH:7:1 : var #'user/f is deprecated

;; use of a deprecated macro (on macro expansion)
(defmacro ^:deprecated m [x] x)
(m 5)
;;=> Deprecation warning, NO_SOURCE_PATH:7:1 : macro #'user/m is deprecated

;; use of a deprecated namespace (on load)
(ns foo {:deprecated "1.1"})
(ns bar (:require foo))
;;=> Deprecation warning: loading deprecated namespace `foo` from namespace `bar`

Patch: 706-deprecated-ns-var-warnings-tested-3.diff

Questions: Should default for deprecation warnings be true instead? People upgrading are likely to see new warnings which might be surprising.

  • Should default be to warn or not warn on deprecated?


 Comments   
Comment by Rich Hickey [ 07/Jan/11 9:38 AM ]

I don't mind warning to stderr

Comment by Luke VanderHart [ 26/Oct/12 1:37 PM ]

706-deprecated-var-warning.diff adds warnings when using deprecated vars. The other three patches clean up deprecation warnings.

Comment by Andy Fingerhut [ 26/Oct/12 2:23 PM ]

Great stuff. I looked through the first patch, and didn't see anything in there that lets someone disable deprecation warnings from the command line, the way that warn-on-reflection can today be set to true with a command line option.

Is that something important to have for deprecation warnings?

Comment by Andy Fingerhut [ 28/Oct/12 4:57 PM ]

I was hoping it would be quick and easy to add source file, line, and column info to the deprecation warning messages. It isn't as easy as adding them to the format() call, because the method analyzeSymbol doesn't receive these values as args. Is this deprecation check being done in a place where it is not easy to relate it to the source file, line, and column? Could it be done in a place where that info is easily available?

Comment by Ghadi Shayban [ 29/Oct/12 1:02 AM ]

Another patch - this time to warn on loading deprecated namespaces, instead of vars. This patch requires the first one.

Re: line/column, I'll figure out how to thread the compile context through if it's desired.

Re: Compile flag. I have a patch for this also, but I'm still verifying how to invoke. How is warn-on-reflection set by command line?

Comment by Andy Fingerhut [ 29/Oct/12 1:43 AM ]

Re: Compile flag. Don't hold off on implementing a flag if you want to, but it might be worth hearing from others whether such a command line option is even desired. I was asking in hopes of eliciting such a response.

For the way that it is handled in the Clojure compiler, search for REFLECTION_WARNING_PROP and related code in Compile.java. If you invoke the Clojure compiler directly via a Java command line, use -Dclojure.compile.warn-on-reflection=true (default is false). See the recent email thread sent to Clojure Dev Google group if you want to know how to do it via ant or Maven. Link: https://mail.google.com/mail/?shva=1#label/clojure-dev/13aa0e34530196c3

There is also a separate command-line flag called compiler-options (see Compile.java) that is implemented as a map inside the compiler. It was added more recently than warn-on-reflection, and might be the preferred method to add more such options, to avoid having to continue to add more arguments to the pushThreadBindings calls done in several places.

Comment by Ghadi Shayban [ 29/Oct/12 10:36 AM ]

Thanks, Andy.

Alternately for the last ns patch, it is equivalent to call (print-method msg err), rather than binding out to err, may be more readable. I'll be glad to send that in if it's preferable.

Comment by Andy Fingerhut [ 13/Feb/13 12:38 AM ]

706-deprecated-var-warning-patch-v2.txt dated Feb 12 2013 is identical to 706-deprecated-var-warning.diff dated Oct 26 2012, except it applies cleanly to latest master.

Comment by Andy Fingerhut [ 23/Feb/15 8:21 PM ]

For the information of anyone examining this ticket wishing for this feature, the Eastwood lint tool reports calls to deprecated Clojure functions, and also to deprecated Java methods. https://github.com/jonase/eastwood

Comment by Alex Miller [ 25/Jan/16 12:32 PM ]

I'm interested in considering this for Clojure 1.9 but I need some help getting it ready. Some comments I have on the current state: - Ticket needs to have more details about the current approach

  • I prefer *warn-on-deprecated* over *warn-on-deprecation* because it echoes the keyword you use to mark deprecated vars
  • The warning message does not tell you a location, which is grr - should be similar to the reflection messages
  • Needs tests - see test/clojure/test_clojure/compilation.clj and test/clojure/test_helper.clj (should-not-reflect) for examples
  • clojure itself has some instances of deprecated usage - it would be nice to clean those up in the patch too. That may need to be in a separate patch, depends if they are easy to fix or not. If there are cases in test/ that are actually good to leave, can set *warn-on-deprecated* to false in that namespace.
  • Current default is true - should probably be false instead to match the reflection warning default.
Comment by Vijay Kiran [ 26/Jan/16 3:10 AM ]

Alex Miller I can give this a shot.

Comment by Alex Miller [ 26/Jan/16 8:51 AM ]

Hey Vijay, Andrew Rosa assigned it to himself so please coordinate with him as he was starting to work on it.

Comment by Bozhidar Batsov [ 26/Jan/16 10:52 AM ]

Just one small remark - isn't it more common to have deprecation warnings enabled by default? One could argue they are way more important than reflection warnings, as your code might get broken in the future because you didn't notice you were using deprecated stuff.

Comment by Alex Miller [ 26/Jan/16 2:01 PM ]

Bozhidar Batsov I'm on the fence. My main hesitation in making it the default is that people will suddenly have a bunch of new warnings (which could be either good or bad I suppose). Depends how strongly we want people to care about deprecations I guess.

Comment by Phill Wolf [ 26/Jan/16 9:33 PM ]

A deprecation warning that is off by default does not address the first and primary problem given in this ticket: "It is easy to use deprecated namespaces without knowing you are doing so."

It's unlike the reflection warning. You may focus on speed at any time, at your leisure. But the eventual removal of at-risk features will be a sudden, unpleasant surprise; a warning would be helpful.

But - Suppose I wrote 300 lines of Clojure and use a million lines that come from jars. Will any deprecation problems in my own code be buried in a tsunami of warnings about those jars? Worse, the tsunami will likely linger for weeks or months, until the libraries' respective authors catch up. Inasmuch as the jars are covered (much more expediently) by 'lein ancient' and similar, I would prefer to be able to limit deprecation warnings to just my stuff, perhaps by namespace prefix if from-a-jar-or-not is inconvenient from the compiler's point of view.

Comment by Alex Miller [ 26/Jan/16 10:35 PM ]

There is a middle ground here to turn it off by default in the compiler, but to turn it on by default in the tools (like lein). But there's a reasonable chance that whatever I prefer, Rich will have a preference that overrules it when it gets to him.

I think creating more complexity around namespace prefixes is unlikely to help this ticket move forward.

Comment by Cezary Kosko [ 07/Mar/16 9:18 PM ]

Uploaded a patch that coalesces var/ns-related patches by Luke & adds tests.
The patch does not, however, warn the user about deprecated macros, I assume I should adjust it, then?

Also, I'm not able adjust the description, so how do I take care of Alex's list's first bullet?

Comment by Andy Fingerhut [ 08/Mar/16 1:05 AM ]

Cezary, I have bumped up your permissions on JIRA so you should be able to edit tickets now. Please reload the page and try again.

Comment by Alex Miller [ 10/Mar/16 10:54 AM ]
  • The if in the first change in core.clj should be a when instead.
  • Can namespace deprecation warning include more about where this is occurring?
  • I'm having a hard time reproducing the deprecated ns warning in a manual test (see below). There seems to be something weird about the binding+printf as the conditions appear to be satisfied. I'm thinking it has something to do with flushing *err*? Seems like (println "Warning: loading deprecated ns" lib) would be better there.
(set! *warn-on-deprecated* true)
(ns foo {:deprecated true})
(ns bar (:require foo))
  • src/jvm/clojure/lang/Compile.java needs added support for clojure.compile.warn-on-deprecated RT flag
  • I think we should turn on warn-on-deprecated in the Clojure build itself (in build.xml)
  • If you do that, the following deprecation warnings exist in the Clojure build itself and we should fix those:

[java] Deprecation warning, clojure/core_proxy.clj:112:75 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/genclass.clj:149:41 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/genclass.clj:235:65 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/test/junit.clj:118:22 : var #'clojure.test/file-position is tagged as deprecated

  • Mark clojure.parallel as deprecated in the ns meta
Comment by Cezary Kosko [ 11/Mar/16 5:40 AM ]

Uploaded a new diff addressing the comments & added warning on macroexpansion.

As far as the namespace deprecation warning goes, though, the code's only printing the current namespace, did not know whether there was a decent way to get a file/line combo.

Comment by Alex Miller [ 11/Mar/16 8:53 AM ]

One more (hopefully final) round and then I think we're good:

  • The docstring for warn-on-deprecated should be updated now that we've expanded scope to cover ns too.
  • In the deprecated ns warning message, can we make it: "Deprecation warning: loading deprecated namespace `foo` from namespace `bar`."
  • In the macro and var warnings can we change "is tagged as deprecated" to just "is deprecated"?
  • Clean up the hanging parens in test/clojure/test_clojure/compilation/deprecated.clj

Thanks for the work on this!!

Comment by Alex Miller [ 11/Mar/16 1:49 PM ]

Marking pre-screened for Rich to look at.





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 31/Aug/16

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

Type: Defect Priority: Critical
Reporter: Herwig Hochleitner Assignee: Rich Hickey
Resolution: Unresolved Votes: 20
Labels: transient

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700-8.diff     Text File clj-700-9.patch     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt     Text File clj-700-rt.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Cause: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

Approach: Expand the types that RT.getFrom(), RT.contains(), and RT.find() can handle to cover the additional transient interfaces.

Alternative: Other older patches (prob best exemplified by clj-700-8.diff) restructure the collections type hierarchy. That is a much bigger change than the one taken here but is perhaps a better long-term path. That patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()). With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience.

Screening Notes

  • the extra conditions in RT add branches to some key functions. get already has a getFrom optimization, but there is no similar containsFrom or findFrom. Is it worth measuring the possible impact of these?
  • I believe the interface refactoring approach (not taken here) is worth separate consideration as an enhancement. If this is done, I think leveraging valAt would be simpler, e.g. allowing HashMap and ArrayMap to share code
  • it is not evident (to me anyway) why some API fns consume ILookup and others do not, among e.g. contains?, get, and find. Possible doc enhancement?
  • there is test code already in place (data_structures.clj) that could easily be expanded to cover transients. It would be nice to do this, or better yet get some test.check tests in place

Patch: clj-700-9.patch



 Comments   
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by Alexander Redington [ 07/Jan/11 2:07 PM ]

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Comment by Stuart Halloway [ 28/Jan/11 10:35 AM ]

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Comment by Alexander Redington [ 25/Mar/11 7:44 AM ]

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Comment by Stuart Sierra [ 17/Feb/12 2:59 PM ]

Latest patch does not apply as of f5bcf647

Comment by Andy Fingerhut [ 17/Feb/12 5:59 PM ]

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Comment by Andy Fingerhut [ 07/Mar/12 3:23 AM ]

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

Comment by Andy Fingerhut [ 23/Mar/12 6:34 PM ]

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

Comment by Stuart Halloway [ 08/Jun/12 12:44 PM ]

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Comment by Andy Fingerhut [ 08/Jun/12 12:52 PM ]

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

Comment by Andy Fingerhut [ 18/Jun/12 3:06 PM ]

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Comment by Andy Fingerhut [ 19/Aug/12 4:47 AM ]

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Comment by Stuart Sierra [ 24/Aug/12 1:08 PM ]

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Comment by Andy Fingerhut [ 24/Aug/12 1:53 PM ]

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 28/Aug/12 5:48 PM ]

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Comment by Alex Miller [ 19/Aug/13 12:26 PM ]

I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all the way up through screening. Should also keep an eye on CLJ-787 as it may have some collisions with this one.

Comment by Andy Fingerhut [ 08/Nov/13 10:14 AM ]

clj-700-7.diff is identical to clj-700-patch6.txt, except it applies cleanly to latest master. Only some lines of context in a test file have changed.

When I say "applies cleanly", I mean that there is one warning when using the proper "git am" command from the dev wiki page. This is because one line replaced in Associative.java has a CR/LF at the end of the line, because all lines in that file do.

Comment by Herwig Hochleitner [ 17/Feb/14 9:54 AM ]

Since clojure 1.5, contains? throws an IllegalArgumentException on transients.
In 1.6.0-beta1, transients are no longer marked as alpha.

Does this mean, that we won't be able to distinguish between a nil value and no value on a transient?

Comment by Stuart Halloway [ 27/Jun/14 10:20 AM ]

Request for someone to (1) update patch to apply cleanly, and (2) summarize approach so I don't have to read through the comment history.

Comment by Andy Fingerhut [ 27/Jun/14 11:02 AM ]

The latest patch is clj-700-7.diff dated Nov 8, 2013. I believe it is impossible to create a patch that applies any more cleanly using git for source files that have carriage returns in them, which at least one modified source file does. Here is the command I used on latest Clojure master as of today (Jun 27 2014), which is the same as that of March 25 2014:

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

If you want a patch that doesn't have the 'trailing whitespace' warning in it, I think someone would have to commit a change that removed the carriage returns from file Associative.java. If you want such a patch, let me know and we can remove all of them from every source file and be done with this annoyance.

Comment by Andy Fingerhut [ 27/Jun/14 11:19 AM ]

Updated description to contain a copy of only those comments that seemed 'interesting'. Most comments have simply been "attached an updated patch that applies cleanly", or "changed the state of this ticket for reason X".

Comment by Alex Miller [ 27/Jun/14 1:19 PM ]

Looks like Andy did as requested, moving back to Screenable.

Comment by Andy Fingerhut [ 29/Aug/14 4:27 PM ]

Patch clj-700-7.diff dated Nov 8 2013 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Comment by Andy Fingerhut [ 01/Sep/14 3:59 AM ]

Patch clj-700-8.diff dated Sep 1 2014 is identical to clj-700-7.diff, except that it applies "cleanly" to latest master, by which I mean it applies as cleanly as I think it is possible to apply for a git patch to a file with carriage return/line feed line endings, as one of the modified files still does.

Comment by Alex Miller [ 17/Dec/14 3:12 PM ]

Added new patch with alternate approach that just makes RT know about transients instead of refactoring the class hierarchy.

clj-700-rt.patch

In some ways I think the class hierarchy refactoring is due, but I'm not totally on board with all the changes in those patches and it has impacts on collections outside Clojure itself that are hard to reason about.

Comment by Rich Hickey [ 31/Jul/15 11:35 AM ]

I'd like to look at the type hierarchy myself

Comment by Tim McCormack [ 14/Dec/15 1:09 PM ]

Workaround: Use ternary get with an acceptable sentinal value:

(get (transient {:x 5}) :y :no)
:no




[CLJ-440] java method calls cannot omit varargs Created: 27/Sep/10  Updated: 03/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Alexander Taggart Assignee: Ragnar Dahlen
Resolution: Unresolved Votes: 10
Labels: interop

Attachments: Text File 0001-CLJ-440-Allow-calling-vararg-Java-methods-without-va.patch    
Approval: Triaged

 Description   

Problem

Clojure calls to Java vararg methods require creating an object array for the final arg. This is a frequent source of confusion when doing interop.

E.g., trying to call java.util.Collections.addAll(Collection c, T... elements):

user=> (Collections/addAll [] (object-array 0))
false
user=> (Collections/addAll [])
IllegalArgumentException No matching method: addAll  clojure.lang.Compiler$StaticMethodExpr.<init> (Compiler.java:1401)

The Method class provides an isVarArg() method, which could be used to inform the compiler to process things differently.

From http://groups.google.com/group/clojure/browse_thread/thread/7d0d6cb32656a621

Latest patch: Removed because incomplete and goal not clear

Varargs in Java

As currently stated, the scope of this ticket is only to omit varargs, but this is only one case where Clojures handling of varargs differs from Java. For completeness, here is a brief survey of how Java handles vararg methods, which could hopefully inform a discussion for how Clojure could do things differently, and what the goal of this ticket should be.

Given the following setup:

VarArgs.java
public class VarArgs {

    public static class SingleVarargMethod {
        public static void m(String arg1, String... args) {}
    }

    public static class MultipleVarargMethods {
        public static void m(String... args) {}
        public static void m(String arg1) {}
        public static void m(String arg1, String... args) {}
    }
}
Java Possible clojure equivalent? Comments
VarArgs.SingleVarargMethod.m("a"); (SingleVarargMethod/m "a")  
VarArgs.SingleVarargMethod.m("a", "b"); (SingleVarargMethod/m "a" "b")  
VarArgs.SingleVarargMethod.m("a", "b", "c"); (SingleVarargMethod/m "a" "b" "c")  
VarArgs.SingleVarargMethod.m("a", new String[]{"b", "c"}); (SingleVarargMethod/m "a" (object-array ["b" "c"]))  
VarArgs.MultipleVarargMethods.m(); (MultipleVarargMethods/m)  
VarArgs.MultipleVarargMethods.m((String) null); (MultipleVarargMethods/m nil) Use type hints to disambiguate?
VarArgs.MultipleVarargMethods.m((String[]) null); (MultipleVarargMethods/m nil) Use type hints to disambiguate?
VarArgs.MultipleVarargMethods.m("a", null); (MultipleVarargMethods/m "a" nil)  
VarArgs.MultipleVarargMethods.m("a", new String[]{}); (MultipleVarargMethods/m "a" (object-array 0))  
VarArgs.MultipleVarargMethods.m(new String[]{"a"}); (MultipleVarargMethods/m (object-array ["a"]))  
VarArgs.MultipleVarargMethods.m("a", new String[]{"b", "c"}); (MultipleVarargMethods/m "a" (object-array ["b" "c"]))  


 Comments   
Comment by Assembla Importer [ 27/Sep/10 8:19 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/440

Comment by Alexander Taggart [ 01/Apr/11 11:16 PM ]

Patch adds support for varargs. Builds on top of patch in CLJ-445.

Comment by Alexander Taggart [ 05/Apr/11 5:45 PM ]

Patch updated to current CLJ-445 patch.

Comment by Nick Klauer [ 29/Oct/12 8:12 AM ]

Is this ticket on hold? I find myself typing (.someCall arg1 arg2 (into-array SomeType nil)) alot just to get the right method to be called. This ticket sounds like it would address that extraneous into-array arg that I use alot.

Comment by Andy Fingerhut [ 29/Oct/12 10:45 AM ]

fixbug445.diff uploaded on Oct 29 2012 was written Oct 23 2010 by Alexander Taggart. I am simply copying it from the old Assembla ticket tracking system to here to make it more easily accessible. Not surprisingy, it doesn't apply cleanly to latest master. I don't know how much effort it would be to update it, but only a few hunks do not apply cleanly according to 'patch'. See the "Updating stale patches" section on the JIRA workflow page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 29/Oct/12 10:56 AM ]

Ugh. Deleted the attachment because it was for CLJ-445, or at least it was named that way. CLJ-445 definitely has a long comment history, so if one or more of its patches address this issue, then you can read the discussion there to see the history.

I don't know of any "on hold" status for tickets, except for one or two where Rich Hickey has explicitly said in a comment that he wants to wait a while before making the change. There are just tickets that contributors choose to work on and ones that screeners choose to screen.

Comment by Alex Miller [ 02/Feb/16 11:47 AM ]

I would love to see an updated patch on this ticket that specifically addressed the varargs issue without building on the other mentioned ticket and patch (which is of lower priority).

Comment by Ragnar Dahlen [ 03/Feb/16 8:01 AM ]

I had a stab at this, have attached an initial patch, parts of which I'm not too sure/happy about so feedback would be appreciated.

The patch takes the following approach:

  1. Teach Reflector/getMethods how to find matching vararg methods. In addition to the current constraints, a method can also match if it is a varargs method, and the arity of the method is one more than the requested arity. That means it's a varargs method we could call, but the user hasn't provided the varargs argument.
  2. In MethodExpr/emitTypedArgs we handle the case were there is one more argument in the method being called than there were arguments provided. The only case were that should happen is when it is a varargs method and the last argument was not provided. In that case we push a new empty object array to the stack.

I'm not to sure about my implementation of the second part. It could open up for some hard to understand bugs in the future. One option would be to be more defensive, and make sure it's really the last argument for instance, or even pass along the Method object (or a varargs flag) so we know what we can expect and need to do.

Comment by Ragnar Dahlen [ 03/Feb/16 8:49 AM ]

I realised my patch is missing two important cases; the interface handling in Reflector and handling multiple matching methods. I'll look into that too, but would still appreciate feedback on the approach in MethodExpr/emitTypedArgs.

Comment by Alex Miller [ 03/Feb/16 9:00 AM ]

I am in favor of using isVarArg() to explicitly handle this case rather than guessing if we're in this situation. We should check the behavior (and add tests where it seems needed) for calling a var args method with too few args, too many args, etc. And also double-check that non vararg cases have not changed behavior.

Also, keep in mind that as a general rule, existing AOT compiled code may rely on calling into public Reflector methods, so if you change the signatures of public Reflector methods, you should leave a version with the old arity that has some default behavior for backwards compatibility.





[CLJ-308] protocol-ize with-open Created: 21/Apr/10  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: io

Attachments: Text File 0001-Added-ClosableResource-protocol-for-with-open.patch    
Patch: Code
Approval: Triaged

 Description   

Good use (and documentation example) of protocols: make with-open aware of a Closable protocol for APIs that use a different close convention. See http://groups.google.com/group/clojure/browse_thread/thread/86c87e1fc4b1347c



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:39 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/308

Comment by Tassilo Horn [ 23/Dec/11 5:11 AM ]

Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open.

All tests pass.

Comment by Tassilo Horn [ 23/Dec/11 7:14 AM ]

Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).

Comment by Tassilo Horn [ 08/Mar/12 3:59 AM ]

Updated patch.

Comment by Andy Fingerhut [ 02/Apr/12 12:11 PM ]

Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.

Comment by Tassilo Horn [ 13/Apr/12 11:23 AM ]

Updated patch to apply cleanly against master again.

Comment by Brandon Bloom [ 22/Jul/14 9:00 PM ]

I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable

Some feedback on the patch:

1) This is a breaking change for anyone relying on the close method to be duck-typed.

2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous.

3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive:

(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))

That would perform much better than the existing patch.

Comment by Tassilo Horn [ 23/Jul/14 7:12 AM ]

Hi Brandon, two questions:

Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found?

Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?





[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: 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: 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: Vetted

 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.






[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: 22/Oct/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: 3
Labels: keywords, namespace, portability, spec

Attachments: Text File clj-2030-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)

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





[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