<< Back to previous view

[CLJ-2182] s/& does not check preds if regex matches empty collection Created: 08/Jun/17  Updated: 29/Jun/17

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

Type: Defect Priority: Critical
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

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

 Description   

In the following example, the ::x key is required in the map but it will conform without error for an empty collection:

(s/def ::x integer?)
(s/conform (s/keys* :req-un [::x]) [])
;; nil, expected error
(s/explain (s/keys* :req-un [::x]) [])
;; Success!, expected explanation

Cause: At the moment, (s/keys* :req-un [::x]) is expanded to a form equivalent to the following one:

(s/& (s/* (s/cat ::s/k keyword? ::s/v))
     ::s/kvs->map
     (s/keys :req-un [::x]))

The issue seems to be in the implementation of s/&, specifically the ::amp branch of accept-nil? which expects that either the conformed regex returns empty or that the preds are not invalid. This seems like a false assumption that an empty conformed regex ret does not require the s/& preds to be valid. In this case we are using a conformer to transform the input and do another check, so it's certainly not valid here.

Proposed: Modify s/& to always validate the preds when accepting nil.

user=> (s/conform (s/keys* :req-un [::x]) [])
:clojure.spec.alpha/invalid

user=> (-> (s/explain-data (s/keys* :req-un [::x]) []) ::s/problems first :pred)
(clojure.core/fn [%] (clojure.core/contains? % :x))

Patch: clj-2182-2.patch



 Comments   
Comment by Alex Miller [ 29/Jun/17 4:20 PM ]

-2 patch includes a test





[CLJ-2098] autodoc fails to load clojure/spec.clj Created: 12/Jan/17  Updated: 07/Apr/17

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

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

Attachments: Text File clj-2098.patch    
Approval: Vetted

 Description   

Reported by Tom F for the autodoc process. The following (essentially) is what autodoc is doing that currently blows up:

tom@renoir:~/src/clj/autodoc-work-area/clojure/src$ java -cp clojure.jar clojure.main
Clojure 1.9.0-master-SNAPSHOT
user=> (load-file "src/clj/clojure/spec.clj")
CompilerException java.lang.IllegalArgumentException: No implementation of method: :conform* \
of protocol: #'clojure.spec/Spec found for class: clojure.spec$regex_spec_impl$reify__14279, \
compiling:(/home/tom/src/clj/autodoc-work-area/clojure/src/src/clj/clojure/spec.clj:684:1)

Cause: There is code in Compiler.macroexpand() with the intention to suspend spec checking inside clojure.spec. However, it is currently doing an exact path match on SOURCE_PATH. In the load-file call above, this ends up being an absolute path.

Approach: Check for a path suffix rather than an exact match in Compiler.

Patch: clj-2098.patch



 Comments   
Comment by Alex Miller [ 07/Apr/17 3:38 PM ]

Removing "code" tag for patch as this is not good to screen yet. While the patch kind of works, it would be good if there were a more reliable way to omit spec checking in the scope of the spec namespace. Rather than checking the source path, it would be much better to instead check the current ns. This works EXCEPT during the macro expansion of ns (which has a spec) in spec itself. At that point you have not yet evaluated the (in-ns 'clojure.spec) (since that's what the ns macro is expanding to). Really, all of this is a problem at exactly one point - compiling clojure.spec itself. Another option might be some way to suspend macro spec checking altogether (which might be a generally useful feature).





[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: 18/Sep/17

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: 10
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     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v4.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v5.patch     Text File CLJ-1814-v6.patch    
Patch: Code and Test
Approval: Vetted

 Description   

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

With:

(defprotocol p (f [_]))
(deftype x [])
(deftype y [])
(extend-type x p (f [_]))

Before patch:

(let [s "abc"] (bench (instance? CharSequence s))) ;; Execution time mean : 1.358360 ns
(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 112.649568 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 2.605426 µs

Cause: `satisfies?` calls `find-protocol-impl` to see whether an object implements a protocol, which checks for whether x is an instance of the protocol interface or whether x's class is one of the protocol implementations (or if its in an inheritance chain that would make this true). This check is fairly expensive and not cached.

Proposed: Extend the protocol's method impl cache to also handle (and cache) instance checks (including negative results).

After patch:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 79.321426 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 77.410858 ns

Patch: CLJ-1814-v6.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

Comment by Alex Miller [ 29/Jun/17 2:31 PM ]

I ran the before and after tests with the v3 patch. Before times matched pretty closely but I could not replicate the after results. I got this which is actually much worse in the not found case:

(let [x (x.)] (bench (satisfies? p x))) ;; Execution time mean : 76.833504 ns
(let [y (y.)] (bench (satisfies? p y))) ;; Execution time mean : 20.570007 µs
Comment by Nicola Mometto [ 29/Jun/17 4:09 PM ]

v4 patch fixes the regression on the not-found case, not sure how that happened, apologies.
Here are the timings I'm getting now:

clojure master:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 604961580 in 60 samples of 10082693 calls.
             Execution time mean : 112.649568 ns
    Execution time std-deviation : 12.216782 ns
   Execution time lower quantile : 99.299203 ns ( 2.5%)
   Execution time upper quantile : 144.265205 ns (97.5%)
                   Overhead used : 1.898271 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 2 (3.3333 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 73.7545 % Variance is severely inflated by outliers
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 22676100 in 60 samples of 377935 calls.
             Execution time mean : 2.605426 µs
    Execution time std-deviation : 141.100070 ns
   Execution time lower quantile : 2.487234 µs ( 2.5%)
   Execution time upper quantile : 2.873045 µs (97.5%)
                   Overhead used : 1.898271 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 40.1251 % Variance is moderately inflated by outliers
nil

master + v4:

user=> (let [x (x.)] (bench (satisfies? p x)))
Evaluation count : 731759100 in 60 samples of 12195985 calls.
             Execution time mean : 79.321426 ns
    Execution time std-deviation : 3.959245 ns
   Execution time lower quantile : 75.365187 ns ( 2.5%)
   Execution time upper quantile : 87.986479 ns (97.5%)
                   Overhead used : 1.905711 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 35.2614 % Variance is moderately inflated by outliers
nil
user=> (let [y (y.)] (bench (satisfies? p y)))
Evaluation count : 771220140 in 60 samples of 12853669 calls.
             Execution time mean : 77.410858 ns
    Execution time std-deviation : 1.407926 ns
   Execution time lower quantile : 75.852530 ns ( 2.5%)
   Execution time upper quantile : 80.759226 ns (97.5%)
                   Overhead used : 1.897646 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 3 (5.0000 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 7.7866 % Variance is slightly inflated by outliers

So to summarize:
master found = 112ns
master not-found = 2.6us

master+v4 found = 79ns
master+v4 not-found = 77ns

Comment by Michael Blume [ 14/Jul/17 5:12 PM ]

For records that have been declared with an implementation of a particular protocol, and which therefore implement the corresponding interface, would it make sense to use an (instance?) check against that interface as a fast path?

Comment by Alex Miller [ 04/Aug/17 11:28 AM ]

Michael - that check is already in there

Nicola - I have a few comments/questions:

1. I don't get what the purpose of the NIL stuff is - could you explain that?
2. In the case where x is an instance of the interface, the old code returned x and the new code in find-protocol-impl* returns interface. Why the change?
3. This: (alter-var-root (:var protocol) assoc :impl-cache (expand-method-impl-cache cache c impl)) is not thread-safe afaict - I think simultaneous misses in two different threads for different impls would cause the cache to only have one of them. This is probably unlikely, and probably not a big deal since the cache will just be updated on the next call (not give a wrong answer), but wanted to mention it. I don't see any easy way to avoid it without a lot of changes.

Comment by Nicola Mometto [ 04/Aug/17 6:14 PM ]

Alex, thanks for looking at this,
1- The NIL object is a placeholder in the method impl cache for nil, as `find-and-cache-protocol-impl` tests for `nil?` to know if the dispatch has been cached or not

2- The change is purely a consistency one, making find-and-cache-protocol-impl always return a class/interface rather than either a class/interface or a concrete instance. Behaviourally, it doesn't change anything since the two consumers of `find-protocol-impl`, namely `find-protocol-method` and `satisfies?` don't care what that value is in that case

3- Yes you are correct that it is not thread safe, however I think it's a decent tradeoff as it doesn't cause any incorrect behaviour and at worse would cause an extra cache miss, making it thread safe would mean an extra performance penalty in every cache hit/miss

Comment by Michael Blume [ 16/Aug/17 1:44 PM ]

Nicola Mometto I'm seeing a change in behavior from this patch

(defprotocol BoolProtocol
  (proto-fn [this]))

(extend-protocol BoolProtocol
  Object
  (proto-fn [x] "Object impl")

  nil
  (proto-fn [x] "Nil impl"))

(proto-fn false)

returns "Object impl" with Clojure master and "Nil impl" with this patch

Comment by Nicola Mometto [ 17/Aug/17 3:08 AM ]

That's not good, I'll take a look later today, thanks

Comment by Nicola Mometto [ 18/Aug/17 3:29 AM ]

This was an issue with how `nil` was cached, I decided to special-case `nil` to skip the method cache, removing the need for all the `NIL` funny business and fixing this bad interaction between `false` and `nil`.

Comment by Michael Blume [ 18/Aug/17 1:17 PM ]

Not sure if it's in scope for this ticket, but given that this wasn't caught, there should probably be more protocol dispatch tests

Comment by Alex Miller [ 20/Aug/17 5:00 PM ]

yes, should definitely add

Comment by Michael Blume [ 21/Aug/17 12:52 PM ]

Patch with test

Comment by Michael Blume [ 21/Aug/17 12:56 PM ]

Verified that test fails with v4 patch:

[java] Testing clojure.test-clojure.protocols
     [java]
     [java] FAIL in (test-default-dispatch) (protocols.clj:695)
     [java] expected: (= "Object impl" (test-dispatch false))
     [java]   actual: (not (= "Object impl" "Nil impl"))
     [java]
     [java] FAIL in (test-default-dispatch) (protocols.clj:695)
     [java] expected: (= "Object impl" (test-dispatch true))
     [java]   actual: (not (= "Object impl" "Nil impl"))
Comment by Michael Blume [ 18/Sep/17 2:19 PM ]

Has this patch missed the 1.9 train? There was a fix we were hoping to make in HoneySQL that I'd hesitate to make with satisfies? as slow as it is.

Comment by Alex Miller [ 18/Sep/17 2:48 PM ]

Not necessarily. We don't add features after 1.9 but perf stuff like this is still possible. It's been vetted by Rich. It's in my list of stuff to screen.





[CLJ-1743] Avoid compile-time static initialization of classes when using inheritance Created: 02/Jun/15  Updated: 25/Sep/17

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     Text File clj-1743-3.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.

Comment by Michael Blume [ 13/Mar/17 10:00 PM ]

Refreshing patch so it applies to master, no changes, keeping attribution.

Comment by Alex Miller [ 27/Jun/17 5:22 PM ]

I am confused by the patch making changes in RT.loadClassForName() but the changes in Compiler are calls to RT.classForNameNonLoading()? Is this patch drift or what's up?

Comment by Michael Schwager [ 25/Sep/17 5:46 PM ]

Thank for you posting this patch. The issue with static initializers has been making it difficult to do JavaFX development with both AOT and interactive development. I cloned the Clojure 1.9.0-master source today and applied the patch, but the example Clojure project still shows "Running static initializers!" I verified this is the case with an actual use case of mine. The error goes away if I start a JFXPanel first. Is there a workaround as of Sept. 2017, eg another way of defining a proxy or deferring until runtime? Thank you.

$ lein clean;lein repl
Compiling 1 source files to C:\dev\clojure\clojure-static-initialization\target\classes
Compiling clojure-static-initialization.gen-class-demo
Compiling clojure-static-initialization.proxy-demo
Running static initializers!
Clojure 1.9.0-master-SNAPSHOT

user=> (def lcp (proxy [javafx.scene.control.ListCell] []))

CompilerException java.lang.ExceptionInInitializerError, compiling:(C:\dev\clojure\clojure-static-initialization\target\f31ee90298a1be447b450330204c3c0806c08b96-init.clj:1:10)

$ lein clean;lein repl
Compiling 1 source files to C:\dev\clojure\clojure-static-initialization\target\classes
Compiling clojure-static-initialization.gen-class-demo
Compiling clojure-static-initialization.proxy-demo
Running static initializers!
Clojure 1.9.0-master-SNAPSHOT

user=> (def jfxpanel (javafx.embed.swing.JFXPanel.))
#'user/jfxpanel
user=> (def lcp (proxy [javafx.scene.control.ListCell] []))
#'user/lcp





[CLJ-1611] clojure.java.io/pushback-reader Created: 08/Dec/14  Updated: 15/May/17

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

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

Attachments: Text File drupp-clj-1611-2.patch     Text File drupp-clj-1611.patch    
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-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-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-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 22/Dec/16

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 0001-use-new-printing-method.patch     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.

Comment by Ed Bowler [ 22/Dec/16 11:35 AM ]

I think http://dev.clojure.org/jira/secure/attachment/16361/0001-use-new-printing-method.patch fixes the printing of the Exceptions.





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

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

Type: Feature Priority: Critical
Reporter: Stuart Halloway Assignee: Cezary Kosko
Resolution: Unresolved Votes: 21
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    
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-440] java method calls cannot omit varargs Created: 27/Sep/10  Updated: 15/May/17

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

Type: Feature Priority: Critical
Reporter: Alexander Taggart Assignee: Ragnar Dahlen
Resolution: Unresolved Votes: 14
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.

Comment by Ghadi Shayban [ 21/Apr/17 11:31 AM ]

Any extra logic that the compiler implements will need to distinguish between:

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

which I don't think is possible generically, without breaking code.

Rather than omitting varargs, how about handling them without tedious array construction. An alternative is to introduce new explicit sugar that you have to opt-in to:

(Whatever/varargs a b c ... x y z)

where ... or similar would be understood by StaticMethodExpr or InstanceMethodExpr in the compiler, and could be type-hinted to resolve ambiguity. This would not be a breaking change.

Comment by Phill Wolf [ 27/Apr/17 6:58 PM ]

If javac can tell the methods apart without the caller providing a "..." partition, so should Clojure. (But in the specific "MultipleVarargMethods" class, javac can't distinguish:

public class MultipleVarargMethods {
    public static void m(String... args) {}
    public static void m(String arg1, String... args) {}
    public static void main(String [] args) {
        m("bankruptcy");
        m("bankruptcy","in","progress");
    }
}


MultipleVarargMethods.java:5: error: reference to m is ambiguous
        m("bankruptcy");
        ^
  both method m(String...) in MultipleVarargMethods and method m(String,String...) in MultipleVarargMethods match
MultipleVarargMethods.java:6: error: reference to m is ambiguous
        m("bankruptcy","in","progress");
        ^
  both method m(String...) in MultipleVarargMethods and method m(String,String...) in MultipleVarargMethods match
2 errors

)





[CLJ-2251] Support Coercion for clojure.spec Created: 11/Oct/17  Updated: 11/Oct/17

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

Type: Enhancement Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

[org.clojure/spec.alpha "0.1.134"]



 Description   

Problem

To do runtime coercion, specs need to be walked twice to strip away the branching information: s/conform + s/unform. This introduced extra latency (see the sample below).

Proposal

New versatile s/walk* to support generic spec walking.

Current status

Still, when running s/conform + s/unform, we walk the specs twice - which is performance-wise suboptimal. Below is a sample, with Late 2013 MacBook Pro with 2,5 GHz i7, with JVM running as -server.

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

(s/def ::id int?)
(s/def ::name string?)
(s/def ::languages (s/coll-of #{:clj :cljs} :into #{}))
(s/def ::street string?)
(s/def ::zip string?)
(s/def ::number int?)

(s/def ::address (s/keys
                   :req-un [::street ::zip ::number]))

(s/def ::user (s/keys
                :req [::id]
                :req-un [::name ::address]
                :opt-un [::languages]))

(def value {::id 1
            :name "Liisa"
            :languages #{:clj :cljs}
            :address {:street "Hämeenkatu"
                      :number 24
                      :zip "33200"}})

; 2.0 µs
(cc/quick-bench
  (s/conform ::user value))

; 6.2 µs
(cc/quick-bench
  (s/unform ::user (s/conform ::user value)))

Despite s/conform is relatively fast, we triple the latency in the sample when running also s/unform. As we know already that we are not interested in the branching info, we could just not emit those.

Suggestion

s/walk* to replace both s/confrom* and s/unform*, maybe even s/explain*. It would take extra mode argument, which would be a Keyword of one of the following:

  • :validate - return false on first failing spec
  • :conform - like the current s/conform*, maybe also return s/explain results?
  • :unform - like the current s/unform*
  • :coerce - s/conform* + s/unform*, could be optimized (e.g. if no branching info, just return the value)

The public apis could be remain the same (+ optional extra argument with CLJ-2116), and a new s/coerce to call the s/walk* with :coerce.

Results

Single sweep validation & coercion. Happy runtime.






[CLJ-2250] Avoid initializing Class when using Class as a value Created: 10/Oct/17  Updated: 10/Oct/17

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

Type: Defect Priority: Major
Reporter: Ragnar Dahlen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-2250-avoid-initializing-class-when-used-as-value.patch    

 Description   

Problem:
When an imported class is used as a value, the emitted bytecode uses RT.classForName to obtain the Class object, causing the class to be loaded and static initializers to be executed. This is different from when Java calls static initializers and makes it more difficult to use clojure with code that depend on the Java semantics.

Motivation
Some code has static initializers that can only execute in a certain environment. A prime example is JavaFX, where many JavaFX classes require the JavaFX platform to be started before any static initializers can run.

Consider this code:

example.clj
(import 'javafx.scene.control.Cell)

(defn f [] Cell)

It currently can't be compiled and executed (for example with "clj example.clj" using clojure-1.9.0beta2) failing with a CompilerException java.lang.ExceptionInInitializerError, with the root cause "Toolkit not initialized". This use of Cell as the return value of f causes the class to be loaded and initialised.

Approach
Modify ObjExpr.emitValue to emit a call to RT.classForNameNonLoading instead of RT.classForName when the value being emitted is a Class.

Patch
https://dev.clojure.org/jira/secure/attachment/17426/CLJ-2250-avoid-initializing-class-when-used-as-value.patch

Prior art
The import form previously was changed to similarly not load the class (CLJ-1315) and CLJ-1743 attempts to address similar issues where Clojure differs from the Java semantics.



 Comments   
Comment by Ragnar Dahlen [ 10/Oct/17 5:05 PM ]

Added patch.





[CLJ-2248] Collections' toString implementations are affected by *print-readably* Created: 04/Oct/17  Updated: 05/Oct/17

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

Type: Defect Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-2248-binding-proof-toString-in-collection-classe.patch    
Patch: Code and Test

 Description   

Collection classes implement toString by delegating to RT.printString, which in turn is affected by the value of *print-readably*:

Clojure 1.9.0-beta1
user=> (binding [*print-readably* false] (str ["foo"]))
"[foo]"
user=> (binding [*print-readably* true] (str ["foo"]))
"[\"foo\"]"

The attached patch fixes this by replacing the calls to RT.printString in collection toString implementations with calls to a new RT.prString method that explicitly binds *print-readably*.

See https://groups.google.com/d/msg/clojure/S13swxLy1ng/FKLYdY9HAgAJ for the original report of interactions between lazy-seq, print and str that are ultimately caused by the above issue.



 Comments   
Comment by Alex Miller [ 04/Oct/17 9:58 PM ]

It's not clear to me why or whether the current behavior is wrong?

Comment by Michał Marczyk [ 05/Oct/17 2:38 AM ]

I would expect str / toString to be stable when applied to persistent collections of immutable items. In other words, when applied to immutable inputs, I would expect it to be a pure function of the arguments explicitly passed in.

There is certainly no way for users to expect any dependency on any dynamic Vars here, or indeed anything other than the argument – which is a pure value.

The original example in the Google group thread is of a lazy seq that is ostensibly a value – (lazy-seq [(str ["a string"])]) – which returns a different pr-str (and print-method) representation depending on whether or not one passes it to print in between creating it and calling pr-str on it:

Clojure 1.9.0-beta1
user=> (let [x (lazy-seq [(str ["a string"])])] (print x) (pr-str x))
([a string])"(\"[a string]\")"
user=> (let [x (lazy-seq [(str ["a string"])])] (pr-str x))
"(\"[\\\"a string\\\"]\")"

Here the user might expect the pr-str call to be independent of the print call, since the former only takes place once the latter returns, and yet there is a spooky interaction.

The patch fixes this:

Clojure 1.9.0-master-SNAPSHOT
user=> (let [x (lazy-seq [(str ["a string"])])] (print x) (pr-str x))
(["a string"])"(\"[\\\"a string\\\"]\")"
user=> (let [x (lazy-seq [(str ["a string"])])] (pr-str x))
"(\"[\\\"a string\\\"]\")"
Comment by Alex Miller [ 05/Oct/17 9:04 AM ]

This is perhaps a philosophical argument and I don't really have a definitive answer, but happy to kick it back and forth.

Agreed that the persistent collection of immutable values is a value. However, there are many ways to build a string representing a view of that immutable data - we have several built into the Clojure print system (pprint, pr, print) + a variety of knobs like collection size limits, etc. I don't see any principle that leads me to believe that the toString has to be independent from the knobs choosing that view.

In other words, I don't necessarily see this as a problem to be solved.

Comment by Didier A. [ 05/Oct/17 10:57 AM ]

I think it's true this is a little philosophical. However, to add some weight toward the side of the patch, I think toString and thus str is generally expected by programmers to be stable. So I would say the current behaviour breaks the principle of least surprise. I'd vote for making str/toString stable, that said, there is a very small possibility this chamge would be a breaking change to someone.

Comment by Alex Miller [ 05/Oct/17 11:49 AM ]

As a grizzled Java veteran, I have 0 expectations about toStrings. Usually in Java they are built on mutable fields and are wildly *un*stable, so I certainly don't share your expectation.

str inherently involves "printing" (creating a string view of a value). I think str is "stable", just not solely a function of its explicit inputs (boo hidden state). To make an analogy, there are many ways to create a string from a date object and toString of a java.util.Date will format the string using your timezone, which is external hidden state.

I'm still not necessarily opposed to the changes here. I just don't find it to be obviously the right thing to do.

From a general perspective, I think the "principle of least surprise" implies much greater commonality in what people find surprising than actually exists, so I put little stock in that. I put a lot more weight in an argument that follows from some stated principles. I think this area is underdocumented/underspecified though.

Comment by Phill Wolf [ 05/Oct/17 5:24 PM ]

The original issue starts with toString. A more vivid problem is that the issue demonstrates that pr is undependable to emit EDN with.

Absence or presence of the intervening print-str (which the caller of pr might have nothing to do with) makes a material difference:

 
user> (clojure.edn/read-string
        (first
          (clojure.edn/read-string
            (let [mk-str (fn [] (lazy-seq [(str ["ZiZi"])]))
                  a (mk-str)]
              ;(print-str a)
              (pr-str a)))))
["ZiZi"]

user> (clojure.edn/read-string
        (first
          (clojure.edn/read-string
            (let [mk-str (fn [] (lazy-seq [(str ["ZiZi"])]))
                  a (mk-str)]
              (print-str a)
              (pr-str a)))))
[ZiZi]




[CLJ-2246] spec.test/check returns the wrong value of :failure for failing tests Created: 02/Oct/17  Updated: 04/Oct/17

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

Type: Defect Priority: Major
Reporter: Khalid Jebbari Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

[org.clojure/clojure "1.9.0-beta1"]
[org.clojure/spec.alpha "0.1.123"]
[org.clojure/test.check "0.10.0-alpha2" :scope "test"]


Approval: Triaged

 Description   

When trying to manually wire clojure.test and spec for fdef'ed functions, I realized that spec.test/check returns a map with [:failure false] for failing tests. I'm (almost) sure it should return true, because spec.test/abbrev-result receives as argument the return of spec.test/check and tests the value of :failure. I couldn't produce a case where spec.test/check returned [:failure true] for failing tests. For information it returns a map without the key :failure for passing test.

Here's a simple reproduction case in the REPL :

(defn foo-fn [x] "bar")

(s/fdef foo-fn
:args (s/cat :x any?)
:ret string?)

(stest/check `foo-fn) ;; => no error, normal small map printed, no :failure entry

(defn foo-fn [x] 1)

(stest/check `foo-fn) ;; => full error map, with :failure equals false. :failure shown below

(-> (stest/check `foo-fn) first :failure) ;; => false



 Comments   
Comment by Gary Fredericks [ 02/Oct/17 7:34 PM ]

This is not a bug, though it is confusing.

stest/check is passing through (at least part of) the return value from clojure.test.check/quickcheck, which returns the value returned by the property, which is evaluated for thruthiness to determine pass vs fail. Since a non-truthy value can only be false or nil, you don't learn very much by looking at it (though it's possible that the :failure key could also have an exception under it, which would be more informative).

The next release of test.check should have a more useful and less confusing way of extracting information from a test, but I don't know for sure how that would look when using stest/check.

Comment by Khalid Jebbari [ 03/Oct/17 3:43 AM ]

I still don't understand why you don't consider it a bug. The docstring of `stest/check` says ":failure optional test failure". I expect to have a `:failure` key only when the tests fail (hence the wording optional), with some valuable info.

I'm using the `stest/abbrev-result` to display the output of `stest/check`, which expects `:failure` to be truthy to display all the valuable failure information. If `stest/check` returns `false`, there's a mismatch I consider a bug. The docstring of `stest/abbrev-result` explicitly says it receives as argument the result of check, so I'm not forcing a square peg into a round hole.

Thank you for answering me so fast and for your time.





[CLJ-2235] Add named loop + recur-to facility for nested loops Created: 10/Sep/17  Updated: 17/Sep/17

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

Type: Feature Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler

Attachments: Text File 0001-CLJ-2235-implement-named-loop-recur-to.patch     Text File 0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch     Text File 0001-CLJ-2235-recur-to-keyword-loop-names.patch     Text File 0002-CLJ-2235-implement-recur-recur-to-as-separate-specia.patch     Text File 0002-CLJ-2235-use-keywords-as-loop-names.patch    
Approval: Triaged

 Description   

Copied from the proposal email to the Clojure/dev Google group:

https://groups.google.com/d/msg/clojure-dev/zlMGmv60MVA/beyIRTrhAgAJ

Hi,

Currently loop/recur is limited to "single-layered" loops: loop forms can occur inside other loop forms, but there is no facility for "recuring to an outer loop".

A few years ago I posted a proposal to add support for nested loops to Clojure with a proof-of-concept patch to ClojureScript with syntax and semantics that I think suffice to make nested loops feel natural while remaining a natural extension of the core loop/recur model, with the same explicit tail recursion benefits:

(loop foo [...]    ; named loop
  ...
  (loop [...]      ; intervening loop
    ...
    (loop [...]    ; innermost loop
      ...
      (recur-to foo ...)))) ; recur to the named loop;
                            ; NB. this must be in tail position
                            ; w.r.t. *all* three loops

https://groups.google.com/d/msg/clojure-dev/imtbY1uqpIc/8DWLw8Ymf4IJ
https://dev.clojure.org/display/design/Named+loops+with+recur-to
https://github.com/michalmarczyk/clojurescript/tree/recur-to
https://github.com/michalmarczyk/clojurescript/commit/feba0a078138da08d584a67e671415fc403fa093

I have now implemented a complete patch enabling the proposed feature in Clojure (the first link is to a branch based on current master, that is, the "prepare for next development iteration" commit after 1.9.0-alpha17; the second is to the current tip of this branch for future reference):

https://github.com/michalmarczyk/clojure/tree/recur-to

https://github.com/michalmarczyk/clojure/commit/212ea06d21d3b336ac35480c99170e81defaf956

I also opened a ticket in JIRA so as to have a place to post the above in the form of a patch file:

https://dev.clojure.org/jira/browse/CLJ-2235

The remainder of this email sets out the proposal in more detail, states its key properties in a somewhat rigorous form, briefly summarizes the implementation approach and discusses certain design choices made in the patch.

Overview
========

The idea is that one could write e.g.

(let [m (two-dimensional-matrix)]
  (loop iloop [i 0]           ; named loop
    (if (< i i-lim)
      (loop [j 0]             ; nested anonymous loop
        (if (< j j-lim)
          (do
            (work)
            (recur (inc j)))  ; recur to the innermost loop
          (recur-to iloop (inc i)))) ; recur to iloop
      (done))))

and, provided that each recur-to form is in tail position with respect to all its enclosing loop forms up to and including its target and the number of arguments passed to each recur-to form matches the number of loop locals of the target loop (plus one for the leading loop name argument), this should compile and behave much like nested loops in Java.

The proposed syntax is modelled on Scheme's named lets, although semantically
those are quite different - this proposal is strictly limited to expanding the loop/recur model to nested loops in a natural way. Of course named fn forms ought also to be valid recur-to targets.

Key properties of named loops and recur-to
==========================================

With the above-linked patch in place, the following rules are enforced at compilation time:

1. Each recur-to form must be in tail position with respect to all its enclosing loop forms, whether named or not, up to and including its target (which may be a named loop or fn form).

2. It is an error to specify a recur-to target which does not occur among the names of the recur-to form's enclosing loop or fn forms with respect to which it is in tail position.

3. It is not possible to recur-to across try.

4. The number of arguments passed to recur-to beyond the initial target/label argument must match the number of formal parameters of the target loop or fn form.

5. Shadowing loop names is permissible; recur-to can only target the innermost loop of the given name among those with respect to which it is in tail position. Loop locals introduced by a shadowed named loop remain visible within the shadowing loop (unless they are themselves shadowed by identically named locals).

NB. loop names are not locals. In particular, they neither shadow nor are shadowed by locals of the same name. This point merits a longer discussion; see the section on design choices at the end of this email.

The innermost loop or fn form can always be targeted using plain recur, whether it is named or not. Additionally (recur-to nil ...) is equivalent to (recur ...) (even when the innermost loop or fn form is actually named), and (loop nil [...] ...) is equivalent to (loop [...]).

Summary of the implementation approach
======================================

The patch modifies the handling of loop labels in the compiler and implements the few necessary tweaks to the loop macro.

It also introduces an optional name argument to the loop* special form. (It is optional primarily so as to avoid breaking any non-core macros that emit loop* directly.)

Finally, it renames the recur special form to recur*; recur and recur-to become macros defined in clojure.core. See the section on design choices below for alternative approaches.

Design choices
==============

1. During development, purely as a matter of convenience at that stage, I had a separate loop-as macro that accepted a name argument. I thought it reasonable to add the naming feature directly to loop, particularly since fn already takes an optional name. Still, loop-as is a valid alternative design.

2. Should it be desirable to avoid renaming the existing recur special form to recur* and reimplementing recur as a macro, a new recur-to special form could be added. (Alternatively, one could keep recur as it is while adding recur-to as a macro delegating to a new recur* special form.)

3. Should it be desirable to preserve the option of treating loop names as locals in the future, it would probably be preferable to make them shadow and be shadowed by locals now, as otherwise elevating them to the status of locals at a later point would be a breaking change. To give an example of why such a future change might be useful, if tail call elimination support arrives in a future JDK spec, one might consider whether it'd be useful to adopt a Scheme-like approach with the loop name treated as a local bound to a function with a single arglist corresponding to the loop locals of the named loop; the closure allocations this would entail could perhaps be optimized away if the local is never referenced.

It bears pointing out that if TCE support does materialize, it will enable a range of alternative designs. For example, Scheme-style named lets could then be introduced as a feature of the let macro. Thus it seems to me that it is reasonable to restrict loop/recur/recur-to to label+goto-style looping only, even in a hypothetical future with VM TCE support, and that there is no reason to afford local-like treatment to loop names; hence the current no-shadowing-interactions approach of the patch.

Cheers,
Michał



 Comments   
Comment by Alex Miller [ 11/Sep/17 1:38 PM ]

Thanks Michał, it looks like you've done a lot of good work here. I think you've just missed the window for looking at new feature stuff for 1.9 but would like to circle back in next release on this.

It seems undesirable for recur to change from special form to macro, so would probably be better to either extend the existing form or add a recur-to special form.

Did you consider other options for specifying a name, like a keyword? Keywords don't carry the expectation of lexical shadowing you have with locals so could side-step those issues maybe? Maybe they are undesirable for other reasons.

Comment by Michał Marczyk [ 17/Sep/17 8:01 PM ]

Cheers, that's good to know.

recur-to as a separate special form

That's fair enough re: changing recur. Here's a version of the patch that makes recur-to a new, separate special form. Note that it still shares the RecurExpr class in the compiler the way loop and let share LetExpr. This patch is meant to be applied on top of the previous one to make it clear what the delta is:

0002-CLJ-2235-implement-recur-recur-to-as-separate-specia.patch

https://github.com/michalmarczyk/clojure/tree/recur-to

I've also prepared a squashed version that takes the current master directly to the same state:

0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch

https://github.com/michalmarczyk/clojure/tree/recur-to-separate-special-form

Incidentally, in implementing this patch I had to revert a change that the original patch makes to clojure.pprint that I guess demonstrates why it's a better idea to go with a new, separate recur-to – I overlooked it when preparing the original posting, otherwise it probably would have tipped the scales for me. (The change is marked by a #_ comment that I forgot to remove in the original patch. The new squashed patch cleans it up automatically by not touching that file.)

Symbols vs keywords as loop names

I used symbols partly because fn expects the optional name argument to be a symbol if present, and so recur-to had to support symbolic names anyway (assuming here that we want to recur-to to use the same form of the recur target name that was used to introduce it, which seems reasonable); and partly just "by default", because static symbolic references generally use symbols. (clojure.core/extend uses keywords, but those aren't really static references.) Not sure if the ability to attach metadata to loop names is likely to be useful, but there's that as well.

The second part about existing usage may or may not be a major consideration, particularly since loop names are somewhat unique in that they can only be referred to by a single special form – recur-to – and are otherwise invisible in the source. This also distinguishes them from fn-introduced named recur targets which of course do double as locals.

So I suppose we could use keywords for loop names and symbols for fn names if we're happy to have metadata-less loop names. We could even allow fn forms to use keyword names if the intention is to establish a named recur target without providing a local reference to the fn instance. (That'd basically be sugar over fn + loop.)

I'll have to think some more about which approach I prefer. I still like the consistency of using symbols for recur targets everywhere (fn / loop / recur-to), but having the local/not-a-local difference be accompanied by a syntactic distinction is tempting.

In the way of some brainstorming-in-the-open, I find it interesting that by using keyword loop names now we'd keep the possibility open of adding support for symbols in the future – perhaps for those VM-TCE-based Scheme-like loop names that would provide local references to closures. Or we could make "symbol labels" a generic feature of "let-like" forms (the ones backed by LetExpr, i.e. let & loop) once TCE lands. Then we'd have consider whether it should be possible for recur-to to target such named lets… And what about plain let? Might be simpler to stick to label+goto looping in loop/recur/recur-to and Scheme-like lets supported through a separate facility (perhaps simply let), with fn something of a point of intersection of the two models (which it already is, since it does introduce recur targets even when unnamed).

As a final note, the one instance where I think the possibility of using keywords for something comparable was brought up was shorthand field access syntax – IIRC (. x :foo) / (.:foo x) was brought up as a possible syntax for what has ultimately become (. x -foo) / (.-foo x). Despite being a road not taken, I think it illustrates how one could plausibly get used to keywords/symbols in effect accessing two namespaces. (Well, in the longhand version; .:foo is technically a symbol. Using keywords for loop names would not involve affording special treatment to any class of "keyword-derived" symbols.)

In any case, I'll see about preparing a version of the patch using keywords for loop names.

Comment by Michał Marczyk [ 17/Sep/17 10:02 PM ]

Here's a first cut at a "keyword loop names" patch:

0002-CLJ-2235-use-keywords-as-loop-names.patch

This is to be applied on top of the squashed "separate special forms" patch:

0001-CLJ-2235-implement-named-loop-recur-to-separate-special-form.patch

Also attaching squashed patch for convenience:

0001-CLJ-2235-recur-to-keyword-loop-names.patch

Here's an example of the new scheme:

(let [m [[[1 2 3] [4 5 6] [7 8 9]]
                 [[10 11 12] [13 14 15] [16 17 18]]
                 [[19 20 21] [22 23 24] [25 26 27]]]]
          ((fn iloop [i ires]
             (if (== i (count m))
               ires
               (loop :jloop [j 0 jres 0]
                 (if (== j (count (get m i)))
                   (recur-to iloop (inc i) (+ ires jres))
                   (loop [k 0 kres 0]
                     (if (== k (count (get-in m [i j])))
                       (recur-to :jloop (inc j) (+ jres kres))
                       (recur (inc k) (+ kres (get-in m [i j k])))))))))
            0
            0))

Note that recur-to still uses symbols where the target is an fn form.

Also note that the approach taken in this patch has the side effect that a loop named :foo won't shadow an fn-introduced recur target named foo. If we wanted eventually to introduce non-fn-introduced recur target using symbols as names (recur-to-targetable Scheme-style let/loop forms as discussed in the previous comment), that would definitely be the way to go. If not, perhaps it's still less arbitrary than declaring that :foo shadows foo in this context?





[CLJ-2234] MultiFn.prefers() ignores the multimethod's internal hierarchy Created: 09/Sep/17  Updated: 12/Sep/17

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: John Alan McDonald Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: multimethods

Attachments: Text File prefersHierarchy.patch    
Patch: Code and Test
Approval: Triaged

 Description   

See https://groups.google.com/forum/#!topic/clojure/3nMn6TiBGOg, which hasn't had any response.

prefers(x,y) visits ancestors by calling parents.invoke() recursively.
This visits the parents in the global hierarchy, not the multimethod's hierarchy.
Is this the intended behavior? Why would that be?

On the assumption that prefer-method should behave the same for a local vs the global-hierarchy, below are 2 unit tests.
MultiFn-prefers-with-local-hierarchy fails with a "Multiple methods" IllegalArgumentException.
MultiFn-prefers-with-global-hierarchy succeeds.

(test/deftest MultiFn-prefers-with-local-hierarchy
  (def local-hierarchy 
    (let [h (make-hierarchy)
          h (derive h ::c0 ::b0)
          h (derive h ::d0 ::c0)
          h (derive h ::d0 ::a0)]
      h))
  (defmulti local identity :hierarchy #'local-hierarchy)
  (defmethod local ::a0 [x] [::a0 x]) 
  (defmethod local ::c0 [x] [::c0 x]) 
  (prefer-method local ::b0 ::a0)
  (test/is (= [::c0 ::d0] (local ::d0)))))

(test/deftest MultiFn-prefers-with-global-hierarchy
  (derive ::c1 ::b1)
  (derive ::d1 ::c1)
  (derive ::d1 ::a1)
  (defmulti global identity)
  (defmethod global ::a1 [x] [::a1 x]) 
  (defmethod global ::c1 [x] [::c1 x]) 
  (prefer-method global ::b1 ::a1)
  (test/is (= [::c1 ::d1] (global ::d1))))

If this is in fact wrong, the fix is pretty easy. I'll submit a patch once it's confirmed this is a real problem.



 Comments   
Comment by Alex Miller [ 11/Sep/17 1:41 PM ]

Patch welcome

Comment by John Alan McDonald [ 12/Sep/17 6:12 PM ]

I'm not sure about the change to preferMethod.
I've moved resetCache to the beginning so that
perfers() can be called with the current state of the hierarchy. Since we are in a write lock, I don't think it's necessary to call it again.





[CLJ-2231] Remove dep lib exclusions Created: 30/Aug/17  Updated: 30/Aug/17

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

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

Attachments: Text File remove-exclusions.patch    
Patch: Code
Approval: Vetted

 Description   

Originally, the spec.alpha and core.specs.alpha lib deps pulled in an older version of Clojure itself as dependencies and they were excluded by Clojure.

These libs have been altered to rely on Clojure, etc as provided scope dependencies instead, so Clojure no longer needs to exclude them (as they are no longer transitive deps).

Note: before applying this patch, the pom must be updated to rely on a version of spec.alpha with these changes (but we haven't released one yet).

Patch: remove-exclusions.patch






[CLJ-2227] s/form fails to unfn #(...) forms occurring in a nested spec Created: 24/Aug/17  Updated: 24/Aug/17

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

Type: Defect Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File CLJ-2227.patch    
Patch: Code

 Description   

s/form can unfn #(...) successfully in:

(s/form (s/and #(>= % 42)))
;=> (clojure.spec.alpha/and (clojure.core/fn [%] (clojure.core/>= % 42)))

But not in:

(s/form (s/and (s/and #(>= % 42))))
;=> (clojure.spec.alpha/and (clojure.spec.alpha/and (fn* [p1__1503#] (clojure.core/>= p1__1503# 42))))

; expected:
;  (clojure.spec.alpha/and (clojure.spec.alpha/and (clojure.core/fn [%] (clojure.core/>= % 42))))

The same goes for #(...) forms occurring in any nested specs.

Cause: clojure.spec.alpha/res calls unfn only when it's applied directly to a fn* form. So if a fn* form occurs in a nested spec form, c.s.a/res will do nothing and keep it as is.



 Comments   
Comment by Shogo Ohta [ 24/Aug/17 2:09 AM ]

Added a patch.





[CLJ-2223] Add extra map argument to clojure.core/assert and clojure.spec/assert (like with ex-info) Created: 13/Aug/17  Updated: 14/Aug/17

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

Type: Feature Priority: Major
Reporter: Vitalie Spinu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: assert, ex-info, spec

Attachments: Text File 0001-Add-extra-map-argument-to-clojure.spec-assert.patch     Text File 0001-Add-extra-map-argument-to-clojure.spec-assert.patch    
Approval: Triaged

 Description   

It's a common requirement to provide extra info about the context of the assertion. For instance one might want to include values of local or dynamic bindings into the assert report, or use `clojure.spec/assert` to leverage structured errors with specs outside of testing environments.

With clojure.core/assert one can splice extra data into formated message but with clojure.spec/assert there is no such feature. The message argument to clojure's assert was added in CLJ-774, and it was acknowledged there that the mechanism is not ideal. One notable benefit of passing reports as data is that editors can handle those gracefully in case of large data.

This proposal is related to CLJ-415 but has a broader scope. With an extra map one can pass any value (not just locals), and there is no danger of inadvertently flooding the REPL with large locals.



 Comments   
Comment by Alex Miller [ 13/Aug/17 12:41 PM ]

Can you start this with a use case for what you are trying to do where this would be useful? The title here is a solution but we find it is best to start with a problem, consider alternatives, and choose a solution.

Comment by Vitalie Spinu [ 13/Aug/17 1:44 PM ]

EDITED: Moved motivation into the issue's "Description".

Comment by Alex Miller [ 14/Aug/17 6:56 AM ]

Rewriting the ticket from this perspective would be a good start.

Comment by Vitalie Spinu [ 14/Aug/17 7:35 AM ]

I would be happy to rewrite, but Jira doesn't allow editing the description.

Comment by Alex Miller [ 14/Aug/17 7:58 AM ]

Sorry about that! I've given you edit rights.





[CLJ-2218] Improving consistency of explain-data for instrument/macroexpand-check Created: 06/Aug/17  Updated: 24/Aug/17

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

Type: Enhancement Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File CLJ-2218.patch    
Patch: Code

 Description   

Description

If you instrument a function, you may get a spec error like the following:

(defn f [x] (inc x))
(s/fdef f
  :args (s/cat :x (s/and integer? even?))
  :ret (s/and integer? odd?))

(t/instrument)

(f 3)
;; ExceptionInfo Call to #'user/f did not conform to spec:
;; In: [0] val: 3 fails at: [:args :x] predicate: even?
;; :clojure.spec.alpha/spec  #object[clojure.spec.alpha$regex_spec_impl$reify__1200
 0x19b3f9a "clojure.spec.alpha$regex_spec_impl$reify__1200@19b3f9a"]
;; :clojure.spec.alpha/value  (3)
;; :clojure.spec.alpha/args  (3)
;; :clojure.spec.alpha/failure  :instrument
;; :clojure.spec.test.alpha/caller  {:file "form-init3240393046310519022.clj", :lin
e 1, :var-scope user/eval1413}
;; clojure.core/ex-info (core.clj:4725)

(ex-data *e) 
;; {:clojure.spec.alpha/problems
;;   [{:path [:args :x],
;;     :pred clojure.core/even?,
;;     :val 3,
;;     :via [],
;;     :in [0]}],
;;  :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x19b3f9a "clojure.spec.alpha$regex_spec_impl$reify__1200@19b3f9a"],
;;  :clojure.spec.alpha/value (3),
;;  :clojure.spec.alpha/args (3),
;;  :clojure.spec.alpha/failure :instrument,
;;  :clojure.spec.test.alpha/caller {:file "form-init3240393046310519022.clj", :line 1, :var-scope user/eval1413}}

As you can see,

  • the explain-data has a regex (ie. the spec for the args of f) in it as ::s/spec
  • each problem contains :args in their :path

These facts can cause a confusion to spec error reporters because the spec for the args of f ((s/and integer? even?)) has no subspec corresponding to the key :args (I believe :path should only contains keys that is a clue to indicate which subspec to be chosen from a spec).

Possible resolutions

To resolve this confusing situation and improve the consistency of explain-data for instrument check, I think there are two options as follows:

  • Solution 1. removing :args from :path
  • Solution 2. modifying explain-data for instrument check so that they have fspec (rather than :args of it) as ::s/spec

Personally, I prefer Solution 2. since adding fspec in explain-data makes it possible to provide richer error information to *explain-out* implementors.

The same goes for macroexpand-check.



 Comments   
Comment by Alex Miller [ 06/Aug/17 11:14 PM ]

The fspec is the spec in question in here and it does have a component :args (the fspec instance supports key lookup via ILookup for :args as well). So while I would like to improve the error message and data here, I don't agree with removing :args from path. One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Comment by Shogo Ohta [ 07/Aug/17 12:20 AM ]

So while I would like to improve the error message and data here, I don't agree with removing :args from path.

Yes, so once we decide to go with Solution 2., then I think there is no need to remove :args from :path.

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call).

I totally agree that would be useful, though it sounds like it's somewhat beyond the scope of the ticket in terms of consistency improvement.

Comment by Shogo Ohta [ 24/Aug/17 5:05 AM ]

I've made a patch just to express what I meant. Any feedback will be appreciated.





[CLJ-2217] Disable fspec validation during instrumentation Created: 05/Aug/17  Updated: 08/Aug/17

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

JVM



 Description   

Problem statement: Enable instrumentation, and invoke a speced function with a lambda. To validate the lambda, spec tests it with generative testing. This results in the lambda being invoked multiple times. If the lambda launches a missile, many missiles are now launched by spec. There are many scenarios in which this is not acceptable because it can for example crash the environment.

Current solutions:

  • Don't spec the lambda. Disadvantage: Spec can't generate it in contexts where its spec is referred.
  • Set fspec-iterations to 0. Disadvantage: Disables all validation of all lambdas.
  • ???

Ideas:

  • An fspec flag to disable the generative testing of its validation.


 Comments   
Comment by Alex Miller [ 05/Aug/17 2:00 PM ]

Another option that has been proposed for this is to make the instrumented function also wrap the function arg in instrumentation according to its spec.

Comment by Leon Grapenthin [ 05/Aug/17 4:49 PM ]

@Alex Miller: Yes, I thought about this as well and believe it would be more consistent with how instrumentation works in regard to functions, i. e. they are checked at invocation time.

However I have not proposed it, because I don't see how we should do it. Spec would have to be able to generically replace all functions that are passed in any arg anywhere with instrumented ones, but also have to know which specs to use. How?

Comment by Leon Grapenthin [ 05/Aug/17 5:24 PM ]

One possible approach would be to implement "descriptive walking" in spec as internal or even public enhancement.
A spec-walk feature would work like prewalk/postwalk, but it takes a spec and a value and invokes the user provided function with both a (sub)value and its corresponding (sub)spec. Instrument wrapper could then replace values that are fspeced with instrumented fns generically. Every spec that composes other specs would have to implement walking over its children and their specs as a new interface method.

Comment by Alex Miller [ 06/Aug/17 11:03 PM ]

Yes, would need something like this (see CLJ-2208 for ticket re spec walking).

Comment by Leon Grapenthin [ 08/Aug/17 6:37 AM ]

@Alex Miller: CLJ-2208 won't do alone. We need to be able to generically walk/replace any given data structure using a spec describing its shape. Should I create a separate ticket and outline a few approaches to get things going? Or should that go here for now?





[CLJ-2213] Allow multiple bindings for if-let, when-let, if-some, and when-some Created: 29/Jul/17  Updated: 04/Aug/17

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

Type: Enhancement Priority: Major
Reporter: Justin Spedding Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clojure.core.patch     Text File clojure-core v2 8-3-2017.patch     Text File core.specs.alpha.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Description of issue:

Suppose I want to create multiple bindings with let and then execute a body. I can do that easily like so:

(let [a 1
      b (inc a)
      c (* b b)]
  [a b c])

But, if I want to do the same type of thing with if-let, I can only do so by nesting them because if-let only accepts one binding at a time.

(if-let [a 1]
  (if-let [b (inc a)]
    (if-let [c (* b b)]
      [a b c]
      "error")
    "error")
  "error")

This is very inelegant because:
1) It is not as simple to read as it would be if all of the bindings were next to each other on the same indentation
2) The else clause it duplicated multiple times.
3) The else clause is evaluated in a different context depending which binding failed. What if a was already bound to something? If the if-let shadows a, and b does not get bound, the else clause would be executed with a different value bound to a than if a was not shadowed in the first if-let. (see code below for example)

I want to be able to write this instead:

(if-let [a 1
         b (inc a)
         c (* b b)]
  [a b c]
  "error")
=> [1 2 4]

(let [a :original]
  (if-let [a :shadowed
           b false]
          a a))
=> :original

I also want to be able to do a similar thing with when-let, if-some, and when-some.

Proposed:

I re-wrote those macros to be able to handle multiple bindings. If supplied with just one binding, their behavior remains identical. If supplied with multiple bindings, they should only execute the body if every binding passed. In the case of some bindings passing and some failing in if-let or if-some, none of the bindings should leak into the else clause.

Patches:

  • clojure-core v2 8-3-2017.patch - Clojure patch with macro updates. For if-let and if-some, I had to add a bit of extra logic in order to prevent them from leaking bindings to the else clause in the case of some bindings passing and some failing. It also includes a few extra tests around each macro.
  • core.specs.alpha.patch - core.specs.alpha patch with equivalent updates to core specs


 Comments   
Comment by Alex Miller [ 29/Jul/17 4:40 PM ]

What's the relationship of this to CLJ-2007?

Comment by Justin Spedding [ 29/Jul/17 4:52 PM ]

I posted my solutions to that ticket as code in a comment. Then, you posted about the correct format of tickets and linked to the ticket creation guidelines. I figured that meant that you wanted a ticket to be made that followed the conventions.

Also, this ticket is about modifying the existing macros. CLJ-2007 was about creating 2 new macros: if-let* and when-let*.

Comment by Ghadi Shayban [ 03/Aug/17 3:30 PM ]

It is worth looking at what the JVM is intending on doing with test-and-destructure intrinsics. Brian Goetz covers this in a recent talk on pattern matching [1]

[1] https://www.youtube.com/watch?v=n3_8YcYKScw

Comment by Justin Spedding [ 03/Aug/17 9:44 PM ]

An updated patch that simplifies the generated code when 0 bindings are given to if-let and if-some





[CLJ-2208] Provide a means to ask a spec for its "child" specs Created: 15/Jul/17  Updated: 15/Jul/17

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

Type: Enhancement Priority: Major
Reporter: Eric Normand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

Some kinds of operations on specs are currently hard to implement as there is no uniform way to find what "child" specs are being composed by a spec. Examples:

  • Dependency analysis
  • Deep describe (show all specs used by a top-level spec)
  • Detection of missing or invalid spec names

For example, given:

(s/def ::user-id int?)
(s/def ::user (s/keys :req [::userid])) ;; note misspelling
(s/valid? ::user {::userid "Jim"}) ;; => true but expect false

And the means to determine the "child" specs of ::user, a linter could check whether all of the keys in s/keys are specs that have been defined.

Workarounds:

1. form can be used to get the original spec form, but that must then be further interpreted (and is missing the original lexical environment in which it was created). Example attempt: https://gist.github.com/ericnormand/6cfe6809beeeea3246679e904372cca0
2. Spec form specs (CLJ-2112) are not available yet, but could be used to get a parsed representation of specs, which would still require some processing but would at least have known forms.

Proposed:

Add a mechanism to get the "child" specs a spec is composed of. Each spec implementation could then choose how to implement this in the appropriate way.



 Comments   
Comment by Eric Normand [ 15/Jul/17 8:53 AM ]

I forgot to add this proposal:

Proposal

I propose that we add a children* method to the Spec protocol. It should return a collection of specs directly referred to. The specs in the collection should be a keyword (if it is referred to by name), an instance of Spec (for nested specs), or some other value valid as a spec (such as a fn).

Comment by Alex Miller [ 15/Jul/17 8:59 AM ]

Rewrote title and some of the description to be less dependent on implementation details (which may change) and more about the problem at hand.





[CLJ-2201] proxy-super is not threadsafe, it should be made safe or documented to be unsafe Created: 05/Jul/17  Updated: 05/Jul/17

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Coming from java you might expect proxy-super to be pretty innocuous, but proxy-super operates by mutating the proxy object then restoring it after the call to proxy-super is invoked. This can lead to very weird behavior. If you have a proxy with method M, which invokes proxy-super, then while that proxy-super is running all calls to M on that proxy object will immediately invoke the super M not the proxied M.

Actually making proxy-super safe (not just threadsafe, but also safe when invoked later on in the same callstack) seems like it might be really hard, but it would be nice. Alternatively some blinking hazard lights in the docstring might be a good idea.






[CLJ-2197] instrument :stub doesn't use :gen override Created: 30/Jun/17  Updated: 30/Jun/17

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

Type: Defect Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec


 Description   

`instrument` doesn't respect `:gen` override for `:stub`.

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.gen.alpha :as gen])
(require '[clojure.spec.test.alpha :as stest])

;; [org.clojure/spec.alpha "0.1.123"]

;; The goal is to stub functions which require some kind external
;; dependency, such as a service or other I/O.

(defprotocol Y
  (-do-y [r]))

(def y? (partial satisfies? Y))
(s/def ::y y?)

;; Protocol methods can't be spec'd, so wrap them in a function.

(defn do-y [r]
  (-do-y r))

(s/fdef do-y :args (s/cat :y-er ::y))

;; Example of the protocol implementation that we're going to stub.

(defrecord BadYer []
  Y
  (-do-y [_] (throw (Exception. "can't make me!"))))

;; Confirm BadYer instances are valid with respect to the protol spec.

(s/valid? ::y (->BadYer))
;; => true

;; And confirm BadYer instances will throw when called.

(try
  (do-y (->BadYer))
  (catch Exception e
    (.getMessage e)))
;; => "can't make me!"


(def y-gen (gen/return (->BadYer)))

;; Confirm generator works as expected:

(gen/sample y-gen 1)
;; => (#spec_ex.core.BadYer{})

;; We want to stub `do-y`, providing y-gen as a generator for `::y`

(try
  (stest/instrument `do-y {:stub #{`do-y}
                           :gen {::y (fn [] y-gen)}})
  (catch Exception e
    (ex-data e)))
;; => #:clojure.spec.alpha{:path [:y-er], :form :spec-ex.core/y, :failure :no-gen}

;; However, we *can* stub `do-y` if we replace its spec.

(stest/instrument `do-y
                  {:stub #{`do-y}
                   :spec {`do-y (s/fspec
                                  :args (s/cat :y-er (s/with-gen ::y
                                                       (fn [] y-gen))))}})
;; => [spec-ex.core/do-y]





[CLJ-2196] Allow string keys for `s/key` specs Created: 30/Jun/17  Updated: 30/Jun/17

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

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

Approval: Triaged

 Description   

JSON is a common data format, especially when interfacing with non-Clojure systems. All keys in JSON objects are strings (not keywords, as is common in Clojure).

It is desirable to be able to validate incoming JSON data and provide helpful error messages when data is poorly formed. Spec is an excellent tool for both, but `s/keys` only works with keyword keys.

It would be useful to be able to specify string keys, for instance, given some JSON data like

{"city" "Denver" "state" "CO"}

I would like to write a spec like:

(s/def :location/city string?)
(s/def :location/state string?)
(s/keys :req-str [:location/city :location/state])

where `:req-str` is like `:req` and `:req-un-str` would be like `:req-un`. The specs would still be fully-qualified keywords.

The current workaround:

1. Convert string keys to keyword keys using `clojure.walk/keywordize-keys`
2. Validate with spec
3. If there are problems, map over the problems and use `stringify-keys` on each val
4. Format the problems appropriately (basically, reproduce the formatting of `explain`).

This workaround is not particularly difficult, but since I suspect working with JSON is a common case, it may be useful to support this use case more directly.






[CLJ-2194] Spec metadata support Created: 30/Jun/17  Updated: 07/Jul/17

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

Type: Enhancement Priority: Major
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: spec


 Description   
  1. Spec metadata support

Problem: Currently there is no way to attach metadata to a spec

It would be nice to be able to add a docstring (the primary use case),
or potentially useful information about usage of that spec in different
contexts (static code analyser, custom conversion/coercion, how it
relates to a particular db schema, human readable error message
template, domain specific concerns or even clj.spec itself later, etc...).

In theory one can today create his own meta-data registry and handle
this at the library level, and that's what a few spec related project
already do, but it would be nicer to have a unified/standard way to do
this. By default it would make sense to add support docstrings for a
start. It could take the form of an extra argument of:

;; at least the following two
(s/def ::foo "Something that's a foo" any?)
(s/def ::foo string? {:doc "Something that's a foo" :cassandra-type :varchar})

;; potentially these depending on the implementation
(s/spec #() :gen ... :meta ...)
(with-meta (s/spec ...))

There are a few ways to implement this, with various pros/cons:

  • Implement the IMeta protocol:
    This seems like the clean approach, meta data would/could be supported
    at any Spec level (ex a non registered spec predicate, a Set spec and
    so on). The implementation would require a fair amount changes to the
    current code tho. Mostly adding a meta parameter to the various
    *-spec-impl macros and sugar at `s/def` and derivatives' level.
    A tricky part with that approach is that registered specs that reference
    another spec are just a "link" (a keyword), so we have nowhere to add
    metadata right now.
    They could be reified, return a "pointer" to their original spec and hold
    metadata at their own level.
  • a simple registry (similar to the spec registry, or shared in the main spec registry):
    Basically a map of spec-kw -> metadata if in a separate registry, or integrated into the
    main registry somehow.
    That's the easy approach, only registered spec would be supported, metadata is separated
    from the rest, would keep the Spec instances a bit lighter. Spec referencing other specs
    could have their own metadata.
    As mentioned this could be done in a separate registry or added to a spec value in the main spec
    registry.

It seems to be the IMeta is probably the better solution, we'd leverage the existing "meta" api.



 Comments   
Comment by Nicola Mometto [ 30/Jun/17 4:20 AM ]

(s/def ^{:doc "Something that's a foo" :cassandra-type :varchar} ::foo string?)
is not valid clojure, you can't add metadata to a keyword

Comment by Max Penet [ 30/Jun/17 4:26 AM ]

Changed example as per Nicolas' comment.

Comment by Andy Fingerhut [ 04/Jul/17 10:44 AM ]

Related to (if not a dup of) CLJ-1965

Comment by Alex Miller [ 04/Jul/17 11:40 AM ]

This is related to but not the same as CLJ-1965 - the scope here is larger to potentially support any meta.





[CLJ-2193] Override function spec within check Created: 28/Jun/17  Updated: 29/Jun/17

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

Type: Enhancement Priority: Major
Reporter: Michael Glaesemann Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

It's desirable to be able to override a function spec within the scope of a check.

For example:

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

(defn a [x])

(s/fdef a
        :args (s/cat :x int?)
        :fn (fn [_] true))

(s/fdef b
        :args (s/cat :x int?)
        :fn (fn [_] false))

;; should pass
(stest/check `a)

(stest/instrument `a {:spec {`a `b}})
;; should fail
(stest/check `a)

;; Similar cases which should fail:

(stest/instrument `a {:spec {`a (s/fspec :args (s/cat :x int?) :fn (fn [_] false))}})
(stest/check `a)

(stest/instrument `a {:spec {`a (s/get-spec `b)}})
(stest/check `a)





[CLJ-2192] When data fails to conform to `map-of` spec, `:in` path does not point to the invalid (inner) value Created: 27/Jun/17  Updated: 21/Sep/17

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

Type: Defect Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

org.clojure/clojure "1.9.0-alpha17", org.clojure/clojurescript "1.9.542", org.clojure/core.specs.alpha "0.1.10"



 Description   

Repro:

(require '[clojure.spec.alpha :as s]
(s/def :foo/user-map (s/map-of string? int?))
(s/explain-data :foo/user-map {"hi" "foo"})
;; Actual value:
;; #:cljs.spec.alpha{:problems
;;                 ({:path [1],
;;                   :pred int?,
;;                   :val "foo", 
;;                   :via [:foo/user-map], 
;;                   :in ["hi" 1]})}
;; Expected: the `:in` value to be ["hi"] ? 
(s/explain-data :foo/user-map {:hi 2})
;; Actual value: 
;; #:cljs.spec.alpha{:problems
;;                 ({:path [0],
;;                   :pred string?,
;;                   :val :hi, 
;;                   :via [:foo/user-map], 
;;                   :in [:hi 0]})}
;; Expected: I'm not sure, since a path can't "point to" a key

Motivation: given some top-level data (in this case, `{"hi" "foo"}`) and an `:in` path, I would like to be able to find the problematic data (in this case, `"foo"`).

In the case where the value of a map does not conform, the `:in` path is not compatible with functions like `get-in`, but it could be.

In the case where the key of a map does not conform, there is no way to "point to" a key using `get-in`, so I'm not sure what the right fix is.

I don't know that compatibility with `get-in` is a requirement: if spec provided a function that accomplished the same thing with "spec" paths (i.e. ones that could point to keys), that would be fine.



 Comments   
Comment by juan pedro monetta sanchez [ 12/Jul/17 9:58 AM ]

I think that more important than get-in compatible is a way of matching :clojure.spec.alpha/problems inside :clojure.spec.alpha/value independent of the spec that lead to the problem.
For this I think it's important to know if the problem is with the key or the value.

Currently s/map-of reports a path taking into account the map-entry vec, so 0 will be the key and 1 the value.

The problem with what I'm trying to implement is the :in is s/keys which only reports the key.

So when you see a problem in [::k 1] you don't know if it's a problem in the map value or the value is a seq and the problem is in the value at pos 1 in that seq.

Comment by Ben Brinckerhoff [ 21/Sep/17 8:56 AM ]

I agree with what is said above, and I'd like to expand on it after having more experience using the in path.

AIUI, clojure.spec is not intended to provide easy-to-read error messages. Rather, it is intended to provide a solid foundation so libraries can present errors in a variety of ways.

In my experience in trying to present errors in a different way (Expound), I can report that one of the biggest challenges in trying to interpret the in path.

There are two cases in particular that are challenging: integer indexes that indicate "key" or "value" failures in a map-of spec, and integer indexes that indicate positions in "key/value" vectors in coll-of specs.

I may be just failing to understand the way the 'in' path works, but anecdotally, this is a source of confusion for other library authors. https://groups.google.com/forum/#!topic/clojure/ppnWBJhz-R4 . Additionally, since this path is shown to users when using explain, a less ambiguous path would help all spec users better understand their errors.

Would it be possible to create unambiguous paths that do not require the reader to know anything about how the path is used? As noted above, the path [:key 1] can only be disambiguated in by knowing something about the failing data structure. This would likely require replacing the integer indexes with custom records. That would reduce conciseness and readability, but this might be able to be alleviated with new reader macros?





[CLJ-2186] ::s/map-bindings definition is underspecified Created: 22/Jun/17  Updated: 23/Jun/17

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

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core.specs, spec

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

 Description   

:clojure.core.specs.alpha/map-bindings` is less strict than expected. It specifies `:into {}`, but not `:kind map?` which means non-maps conform.

(s/conform ::clojure.core.specs.alpha/map-binding-form '[[foo bar]])
=>
[[foo bar]]

This is also an issue because of the next line:

(s/def ::map-binding-form (s/merge ::map-bindings ::map-special-binding))

s/merge takes two maps, and ::map-bindings is not required to be a map.

Patch: clj-2186.patch

Screened by: Alex Miller (apply to core.specs.alpha)



 Comments   
Comment by Sameer Rahmani [ 23/Jun/17 11:01 AM ]

On clojure 1.9.0-alpha17 :

user=> (s/conform ::clojure.core.specs.alpha/map-binding-form '[[foo bar]])
:clojure.spec.alpha/invalid




[CLJ-2183] `cat` specs should verify value is sequential Created: 08/Jun/17  Updated: 30/Jun/17

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

Type: Defect Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

[org.clojure/clojure "1.9.0-alpha17"]
[org.clojure/spec.alpha "0.1.109"]


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

 Description   

Regex op specs can currently pass with maps or sets (which are unordered) but may give confusing errors.

(require '[clojure.spec.alpha :as s])
(s/def ::kv (s/cat :k keyword? :v any?))
(s/explain ::kv {"foo" "bar"})
In: [0] val: ["foo" "bar"] fails spec: :user/kv at: [:k] predicate: keyword?

Cause: Conforming fails because the first value of the map (the tuple pair `["foo" "bar"]`) is not a keyword

Proposed: Regex op specs currently check `coll?`, which will pass unordered collections like sets or maps - this is unlikely to be useful for positional regex specs. Propose to narrow that check to `sequential?`. On failure, use an explain pred that describes the actual check (the current one just repeats the regex spec instead).

With the patch, the message actually tells you the actual predicate that is failing (the sequential? check):

val: {"foo" "bar"} fails spec: :user/kv predicate: (or (nil? %) (sequential? %))

Patch: clj-2183.patch

Screened by: Chouser - while making unordered colls invalid is the intent of the patch, a gray area is that of sorted colls (sorted sets, etc). These could have been matched with the prior impl, but will not be after the change. See comments for more examples.



 Comments   
Comment by Chouser [ 20/Jun/17 11:44 PM ]

Note, I believe this is a breaking change. Before the patch, this works:

(s/def ::dt (s/cat :datep (s/spec (s/cat :datek #{::date}, :datev number?))
                   :timep (s/spec (s/cat :timek #{::time}, :timev number?))))

(s/explain ::dt {::date 10, ::time 20})
;; Success!

After the patch, it fails:

(s/explain ::dt {::date 10, ::time 20})
;; val: #:user{:date 10, :time 20} fails spec: :user/dt predicate: (or (nil? %) (sequential? %))
;; :clojure.spec.alpha/spec  :user/dt
;; :clojure.spec.alpha/value  #:user{:date 10, :time 20}

However, even without the patch such specs will match only as long as the map (or set) entries are in the expected order. For hash-maps and hash-sets (or collections like array-maps that may be promoted to hash-maps), this is unpredictable and the patch arguably only "breaks" things that were at risk of breaking anyway.

Sorted collections are a little dicier. Without the patch, this arguably reasonable spec works reliably for sorted sets:

(s/def ::ab (s/cat :a (s/? #{"a"}) :b (s/? #{"b"})))
(s/explain ::ab (sorted-set "a" "b"))
;; Success!

With the patch, the sorted-set doesn't match because it's not a sequential collection, thus a breaking change.

Fortunately spec is still alpha so breaking changes are ok, and specs that intend to match sorted collections have several other options for doing so, especially the spec macros meant for non-sequential collections, like so:

(s/def ::ab (s/coll-of #{"a" "b"}))
Comment by Alex Miller [ 21/Jun/17 1:39 AM ]

Yes, the idea here is that this constrains the scope of what regex op specs support, so the "breaking" part of it is intentional.

Sorted colls is an interesting question, seems like a gray area. Personally, I'd be happy to rule it out, but maybe Rich would disagree. For something like this, it's best to note it as screener notes in the description as Rich doesn't necessarily read all the comments. I'll add it as an example.





[CLJ-2180] Enhancing :path info for s/merge & s/and & s/& to indicate which subspec raised spec error Created: 07/Jun/17  Updated: 07/Jun/17

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

Type: Enhancement Priority: Major
Reporter: Shogo Ohta Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

Description

Suppose we want to traverse a spec that caused some spec error, from the root spec embedded in the explain-data to a leaf of the pred that was the actual cause of the error.

We can usually use :path info in the explain-data for such a purpose:

user=> (s/explain-data (s/tuple integer? string?) [1 :a])
#:clojure.spec.alpha{:problems
                     ({:path [1], ;; <- indicates the 1st subspec, ie. integer?, was the cause of the error
                       :pred clojure.core/string?,
                       :val :a,
                       :via [],
                       :in [1]}),
                     :spec ...,
                     :value [1 :a]}
user=>

If we traverse the spec tree along the :path, we can eventually reach the leaf pred that raised the spec error.

In some cases, however, it doesn't hold since some specs such as s/merge, s/and and s/& don't put any clue into :path that tells which subspec actually raised the error:

user=> (s/explain-data (s/merge (s/map-of integer? string?)
                                (s/coll-of (fn [[k v]] (= (str k) v))))
                       {1 "2"})
#:clojure.spec.alpha{:problems
                     ({:path [], ;; <- doesn't tell us anything at all
                       :pred (fn [[k v]] (= (str k) v)), ;; <- we don't know which subspec this pred occurs in
                       :val [1 "2"],
                       :via [],
                       :in [0]}),
                     :spec ...,
                     :value {1 "2"}}
user=>

To achieve our purpose even in those cases, we have to make a nondeterministic choice: that is, choose a subspec arbitrarily and try traversing it down, and if something is wrong along the way, then backtrack to another subspec and so on.

From my experience that I implemented that backtracking algorithm in a library I'm working on (repo), I think it's much harder to implement correctly than necessary. In fact, my implementation is probably broken in some corner cases, and I don't even know if it's possible in theory to implement it completely correctly.

Proposal

To make it easier to implement the spec traversal, this ticket proposes adding the index into :path that indicates which subspec raised the spec error for s/merge, s/and and s/&, as follows:

user=> (s/explain-data (s/merge (s/map-of integer? string?)
                                (s/coll-of (fn [[k v]] (= (str k) v))))
                       {1 "2"})
#:clojure.spec.alpha{:problems
                     ({:path [1], ;; <- indicates the 1st subspec, ie. (s/coll-of (fn [[k v]] (= (str k) v))) has the actual cause of the error in it
                       :pred (fn [[k v]] (= (str k) v)),
                       :val [1 "2"],
                       :via [],
                       :in [0]}),
                     :spec ...,
                     :value {1 "2"}}
user=>

The enhancement, though it is indeed a breaking change, should reduce radically the effort needed to write the code traversing specs along the :path.






[CLJ-2179] s/inst-in and s/int-in generators should have uniform distribution not biased towards min value Created: 06/Jun/17  Updated: 06/Jun/17

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: gen-class, spec

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

 Description   

The s/inst-in and s/int-in generators are based on gen/large-integer* which grows from 0.

(require '[clojure.spec.alpha :as s] '[clojure.spec.gen.alpha :as gen])
(gen/sample (s/gen (s/int-in 0 100)))
;;=> (1 0 1 1 1 0 1 1 72 1)

(gen/sample (s/gen (s/inst-in #inst "2001-01-01" #inst "2001-12-31")))
;;=> (#inst "2001-01-01T00:00:00.000-00:00" #inst "2001-01-01T00:00:00.000-00:00" #inst "2001-01-01T00:00:00.001-00:00" #inst "2001-01-01T00:00:00.001-00:00" ...)

Proposed: Instead, s/inst-in should use a uniform distribution generator:

After on same:

(26 16 65 96 63 37 31 4 94 9)

(#inst "2001-03-03T04:51:43.702-00:00" 
 #inst "2001-07-25T07:13:03.224-00:00" 
 #inst "2001-03-31T18:28:41.625-00:00" 
 #inst "2001-04-17T19:33:14.176-00:00" 
 #inst "2001-01-14T07:03:08.521-00:00" 
 #inst "2001-06-06T09:52:03.421-00:00" ...)

Patch: clj-2179.patch






[CLJ-2178] s/& explain-data :pred problem Created: 05/Jun/17  Updated: 29/Jun/17

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

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

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

 Description   

As reported in CLJ-2175, s/& has some issues with reporting valid resolved preds in explain-data:

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

(-> (s/explain-data (s/& int? even?) []) ::s/problems first :pred)
;;=> #function[clojure.core/int?]
;;EXPECTED: clojure.core/int?

(-> (s/explain-data (s/& int? even?) [0 2]) ::s/problems first :pred)
;;=> (clojure.spec.alpha/& #function[clojure.core/int?] clojure.core/even?)
;;EXPECTED: (clojure.spec.alpha/& clojure.core/int? clojure.core/even?)

Problem: s/& was not capturing the re form. Added a new :amp key to carry the re form (similar to :maybe key in s/?) and used that for describe when necessary.

Patch: clj-2178.patch






[CLJ-2177] s/keys explain-data :pred problem Created: 05/Jun/17  Updated: 29/Jun/17

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

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

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

 Description   

As reported in CLJ-2175, s/keys has an issue with reporting a valid resolved pred in explain-data:

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

(-> (s/explain-data (s/keys :req [::x]) :a) ::s/problems first :pred)
;;=> map?   
;;EXPECTED: clojure.core/map?

Patch: clj-2177.patch



 Comments   
Comment by Shogo Ohta [ 05/Jun/17 6:31 PM ]

Missing the patch?





[CLJ-2176] s/tuple explain-data :pred problems Created: 05/Jun/17  Updated: 29/Jun/17

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

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

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

 Description   

As reported in CLJ-2175, s/tuple has some issues with reporting valid resolved preds in explain-data:

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

(-> (s/explain-data (s/tuple int?) :a) ::s/problems first :pred)
;;=> vector?   
;;EXPECTED: clojure.core/vector?

(-> (s/explain-data (s/tuple int?) []) ::s/problems first :pred)
;;=> (clojure.core/= (clojure.core/count %) 1)  
;;EXPECTED: (clojure.core/fn [%] (clojure.core/= (clojure.core/count %) 1))

Patch: clj-2176.patch






[CLJ-2174] Spec generated exceptions/error messages are a regression in terms of the out-of-the-box experience with Clojure. Created: 01/Jun/17  Updated: 01/Jun/17

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

Type: Defect Priority: Major
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, spec
Environment:

All


Approval: Triaged

 Description   

While it is clear that spec has a lot of advantages in terms of a uniform way to specify the shape of behavior, using spec to catch programming and API errors within Clojure itself has led to error messages that more verbose and less clear than what was there in previous versions. For example if I supply a bad name to defn, in version an earlier version of Clojure I got a clear, relatively English language messages back:

Clojure 1.7.0

user=> (defn 44 [x] x)
IllegalArgumentException First argument to defn must be a symbol clojure.core/defn--4156 (core.clj:281)

In the current 1.9 release, the same mistake generates the following:

Clojure 1.9.0-master-SNAPSHOT

user=> (defn 44 [x] x)
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
In: [0] val: 44 fails spec: :clojure.core.specs.alpha/defn-args at: [:args :name] predicate: simple-symbol?
:clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x3c0f93f1 "clojure.spec.alpha$regex_spec_impl$reify__1200@3c0f93f1"]
:clojure.spec.alpha/value (44 [x] x)
:clojure.spec.alpha/args (44 [x] x)
#:clojure.spec.alpha{:problems [{:path [:args :name], :pred clojure.core/simple-symbol?, :val 44, :via [:clojure.core.specs.alpha/defn-args :clojure.core.specs.alpha/defn-
args], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x3c0f93f1 "clojure.spec.alpha$regex_spec_impl$reify__1200@3c0f93f1"], :value (44 [x] x), :args
(44 [x] x)}, compiling:(NO_SOURCE_PATH:1:1)

There is a similar situation with let. Here is the behavior in earlier versions:

user-> (let (a 1) (println a))
IllegalArgumentException let requires a vector for its binding in user:1 clojure.core/let (core.clj:4309)

And in 1.9:

user=> (let (a 1) (println a))

CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/let did not conform to spec:
In: [0] val: (a 1) fails spec: :clojure.core.specs.alpha/bindings at: [:args :bindings] predicate: vector?
:clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x31dc339b "clojure.spec.alpha$regex_spec_impl$reify__1200@31dc339b"]
:clojure.spec.alpha/value ((a 1) (println a))
:clojure.spec.alpha/args ((a 1) (println a))
#:clojure.spec.alpha{:problems [{:path [:args :bindings], :pred clojure.core/vector?, :val (a 1), :via [:clojure.core.specs.alpha/bindings
:clojure.core.specs.alpha/bindings], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__1200 0x31dc339b
"clojure.spec.alpha$regex_spec_impl$reify__1200@31dc339b"], :value ((a 1) (println a)), :args ((a 1) (println a))}, compiling:(NO_SOURCE_PATH:1:1)

Yes all of the information – and more – is there in the 1.9 version.
But the spec error messages are likely to be incomprehensible to anyone relatively new to Clojure and adds to the cognitive load of even experienced Clojure programmers.



 Comments   
Comment by Russ Olsen [ 01/Jun/17 9:15 AM ]

Typos in that first sentence, should have read 'shape of DATA' not behavior, and 'error messages that ARE more verbose'.

Comment by Alex Miller [ 01/Jun/17 2:19 PM ]

Hey Russ, part of this is actually a bug that crept into alpha17 that is tracked with a patch here: https://dev.clojure.org/jira/browse/CLJ-2171

But also, I would like to overhaul the instrument exception reporting as I think it could be a lot clearer about the signature being invoked and how it is wrong.





[CLJ-2173] LispReader.java and EdnReader.java exception messages could be much more informative. Created: 31/May/17  Updated: 31/May/17

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

Type: Enhancement Priority: Major
Reporter: Russ Olsen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, reader
Environment:

Any


Approval: Triaged

 Description   

The messages in the exceptions thrown by the readers would be much more informative if they included readily available information. There are many instances of this, but to name a few specific instances (all from LispReader.java, though there in most cases there are corresponding problems in EdnReader.java):

  • If the RegexReader class hits an unexpected EOF, it reports "EOF while reading regex". It would be helpful if the message included the first few characters of the regex it was trying to read – available in sb – as a guide to the person trying to locate the problem.
  • The same logic applies to StringReader.
  • In NamespaceMapReader, the error thrown if the namespaced map is not in fact a map could include the namespace symbol.
  • Whenever an odd number of elements in a map is detected, the exception could at least report the number of elements that the bad map did include, something like: "Map literal cannot contain 7 forms. Map literals must contain an even number of forms." Even better would be the first few forms.
  • The "Metadata can only be applied to IMetas" exception in MetaReader is not nearly as helpful as it could be. At the very least it should report the class of the thing that is not an IMeta.
  • With an additional argument, readDelimitedList could report the kind of thing that it was reading in the event that it hit the EOF. Without the additional argument it still report that it hit an EOF while trying to read the first or 4th or 29 element of a collection.





[CLJ-2168] clojure.spec: :pred in explain for coll-of should have resolved symbols Created: 26/May/17  Updated: 01/Jun/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

0.1.123


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

 Description   

:pred should be resolved in explain problems like:

s/coll-of and s/every-kv should have resolved :pred functions if it's values aren't valid:

(::s/problems (s/explain-data (s/coll-of (fn [x] (pos? x))) [-1]))
({:path [], :pred (fn [x] (pos? x)), :val -1, :via [], :in [0]})

should be

(::s/problems (s/explain-data (s/coll-of (fn [x] (pos? x))) [-1]))
({:path [], :pred (clojure.core/fn [x] (clojure.core/pos? x)), :val -1, :via [], :in [0]})

Other examples:

;; same with every
(::s/problems (s/explain-data (s/every (fn [x] (pos? x))) [-1]))
({:path [], :pred (fn [x] (pos? x)), :val -1, :via [], :in [0]})

;; :distinct option pred is not resolved:
(::s/problems (s/explain-data (s/coll-of pos? :distinct true) [-1 -1]))
[{:path [], :pred distinct?, :val [-1 -1], :via [], :in []}]

map-of and every-kv do not have this issue. The :count, :min-count, :max-count, and :kind options do correctly produce resolved :preds.

Patch: clj-2168.patch



 Comments   
Comment by Shogo Ohta [ 31/May/17 2:19 AM ]

The same problem happens with s/every.

Comment by Shogo Ohta [ 01/Jun/17 9:02 PM ]

Oh, sorry. I meant s/every-kv, not s/every.

BTW, after looking into things around this, I found some other spec macros were putting inconsistent forms of :pred in their explain data.

For example,

(s/explain-data (s/tuple integer?) []) => (clojure.core/= (clojure.core/count %) 1)
(s/explain-data (s/& integer? even?) []) => #function[clojure.core/integer?] (not a symbol)

I'll file that as another ticket later.





[CLJ-2167] int-in-range? throws exception when val not an int Created: 26/May/17  Updated: 30/Jun/17

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 Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Clojure 1.9.0-alpha16
spec.alpha 0.1.123


Attachments: Text File int-in-range.patch    
Patch: Code
Approval: Screened

 Description   

The spec predicate int-in-range? throws an exception when not passed an int? value.

(require '[clojure.spec.alpha :as s])
(s/int-in-range? 0 10 :x)
ClassCastException clojure.lang.Keyword cannot be cast to java.lang.Number

Expected result: false

Cause: int-in-range? uses int? instead of (int? val), so that condition always evaluates true regardless of input.

Solution: Use (int? val) instead.

Patch: int-in-range.patch

Screened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 26/May/17 1:05 PM ]

It's totally a typo & you should sign the CA. Sooner the better because there seems to be some spec bug fixing activity today





[CLJ-2165] #clojure/var tag for transmitting var identity Created: 22/May/17  Updated: 08/Aug/17

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: print, reader, var

Attachments: Text File vartag2.patch    
Approval: Vetted

 Description   

Currently one can't send vars around in edn. #' is clojure reader specific. Objective is to transmit var identity and bind to same-named var on reading side (a la var serialization support).

Proposed: This is not generic enough to add to edn, so use #clojure/var for tag. Printing may print #clojure/var instead of #' (perhaps via a flag) - needs more assessment. #clojure/var tag reader should be installed in data readers.

Patch: vartag2.patch



 Comments   
Comment by Christophe Grand [ 11/Jul/17 10:14 AM ]

Should unnamed vars (eg created by with-local-vars) print to #clojure/var nil or throw an exception? (exception is the print-dup behavior)

Comment by Steven Yi [ 08/Aug/17 11:49 AM ]

I think the vartag2.patch has an issue in that the test for print-var-tagged in missing an assertion. I think it is supposed to have something like:

(is (and ...))

within the last let-binding.





[CLJ-2163] Add test for var serialization Created: 19/May/17  Updated: 20/Jun/17

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

Attachments: Text File var-test.patch    
Patch: Code and Test
Approval: Screened

 Description   

Add some tests for var serialization.

Screened by: Chouser






[CLJ-2158] multi-spec retag generator in conflict with user tag spec/gen Created: 22/Apr/17  Updated: 25/Apr/17

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

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

JVM


Approval: Triaged

 Description   

Problem: multi-spec does generate retag values on its own from values for which methods have been implemented. However a user can have purposefully speced the retag key differently and multi-spec will generate incompatible values (resulting in a such-that failure). An example is hierarchy dispatch where methods dispatch values aren't necessarily the valid tag values.

Proposed solution: When generating the retag value, multi-spec should first try the existing spec for that key and generate "such-that" it is a possible dispatch value for the multimethod, only generate direclty from the multimethod based mechanism iff there is no spec for the tag key.



 Comments   
Comment by Leon Grapenthin [ 25/Apr/17 3:00 AM ]

Improved proposed solution to cover both "user spec is a subset of dispatch values" and vice versa.





[CLJ-2157] multi-spec doesn't generate possible tags from hierarchy Created: 22/Apr/17  Updated: 22/Apr/17

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: 0
Labels: generator, spec


 Description   

Problem: Even though multi-spec supports hierarchy dispatch of multi-methods, its generator only generates tags that have direct method implementations.

Proposed solution: It should also generate from hierarchy.






[CLJ-2152] clojure.spec: s/& has a broken form Created: 12/Apr/17  Updated: 12/Apr/17

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

Type: Defect Priority: Major
Reporter: Tommi Reiman Assignee: Alex Miller
Resolution: Unresolved Votes: 3
Labels: spec
Environment:

1.9-alpha15


Approval: Vetted

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

(s/form (s/& integer?))
; (clojure.spec/& #object[clojure.core$integer_QMARK_ 0x5536db54 "clojure.core$integer_QMARK_@5536db54"])





[CLJ-2146] partition-by and partition-all transducers should ensure visibility of state changes Created: 09/Apr/17  Updated: 17/Apr/17

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: transducers

Approval: Vetted

 Description   

The partition-by and partition-all transducers use state stored in an ArrayList. This state should be protected (for example, by volatile) to ensure visibility if used in a transducing process that moves computations across threads.



 Comments   
Comment by Léo NOEL [ 13/Apr/17 1:00 PM ]

Discussion here : https://groups.google.com/forum/m/#!topic/clojure/VQj0E9TJWYY

Comment by Léo NOEL [ 16/Apr/17 9:47 AM ]

Note that following this logic, transients are as much broken, as they make use of plain arrays.
This paragraph https://clojure.org/reference/transients#_concurrent_use makes me believe this very problem has already been tackled before. Is the discussion available somewhere ?
In my opinion, documentation should be more precise about what is meant by thread isolation, and explain why it is OK to use unsynchronized mutable objects when they're owned by something that enforces sequential processing (agents, go blocks, channels, single-threading, etc).

Comment by Alex Miller [ 17/Apr/17 9:24 AM ]

Transients originally enforced thread isolation by recording and validating the originating thread. This was weakened to allow for transients passed around go blocks in Clojure 1.7 and has been through some rounds of fixes (like CLJ-1580). If there is an issue with them now, please file a separate ticket.





[CLJ-2145] locals closed over by a ^:once fn aren't cleared if the fn is in a branch Created: 08/Apr/17  Updated: 11/Apr/17

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: compiler, locals-clearing

Attachments: Text File 0001-CLJ-2145-fix-clearing-of-locals-closed-over-by-a-FNO.patch    
Patch: Code
Approval: Triaged

 Description   

Minimal case:

(fn foo [x]
  (if true
    (^:once fn* []
     ;; x is not cleared here
     x)))

This is a severe bug as it means that every local used inside a loop or try/catch expression that the clojure compiler internally hoists in a FNONCE, in a conditional branch, cannot be cleared at the moment.

As a concrete example reported in slack,

;; THIS OOMs
(defn test1 [x]
  (if true
    (do
      (try (doseq [_ x] _))
      1)
    0))

(test1 (take 1000000 (range)))

;; THIS DOESN'T OOM 
(defn test2 [x]
  (do
    (try (doseq [_ x] _))
    1))

(test2 (take 1000000 (range)))

Approach: don't set a new clearing frame if the fn is ^:once and there's an existing clearing frame
Patch: 0001-CLJ-2145-fix-clearing-of-locals-closed-over-by-a-FNO.patch






[CLJ-2144] clojure.walk/keywordize-keys wants ns support for clojure.spec utility Created: 08/Apr/17  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-2144-Add-namespace-arg-to-walk-keywordize-keys.patch    

 Description   

keywordize-keys currently takes a single argument, a nested structure presumably containing maps, turning all string keys into un-namespaced keys. I've found that I've needed to maintain my own modified version of keywordize-keys that allows me to pass a namespace so I can import JSON objects and use them with clojure.spec (which strongly prefers namespaced keys).

The addition of an additional 2-arity invocation with a namespace string argument is a non-breaking change. I'll attach a patch once I have a JIRA number.



 Comments   
Comment by Aaron Brooks [ 08/Apr/17 1:51 PM ]

This patch also includes a test but I accidentally selected "Code" when I opened the ticket. I don't think I can change that. Can a maintainer update the "Patch" field to "Code and test"?

Comment by Alex Miller [ 11/Apr/17 4:10 PM ]

Another route to go with this btw is to first do CLJ-1899 to build on.





[CLJ-2143] The result of s/form for s/keys* is different from the original form Created: 05/Apr/17  Updated: 05/Apr/17

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

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

Approval: Vetted

 Description   

If s/form is applied to s/keys*, it returns a value completely different from the original form:

user=> (s/form (s/keys* :req-un [::x ::y]))
(clojure.spec/& (clojure.spec/* (clojure.spec/cat :clojure.spec/k clojure.core/keyword? :clojure.spec/v clojure.core/any?)) :clojure.spec/kvs->map mspec__14270__auto__)
user=>


 Comments   
Comment by Alex Miller [ 05/Apr/17 8:57 AM ]

Thanks for logging - I've been working on an approach for this one but never got around to actually logging it.





[CLJ-2129] Enhance CompilerException to optionally return the invalid form Created: 16/Mar/17  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, error-reporting

Approval: Triaged

 Description   

Currently CompilerException wraps errors that occur at compile time and adds file/line/col info. Some tools could do more with the failing form, particularly if it includes useful meta.

Proposal is to add a CompilerException constructor that also takes the form (Object) and conveys it. Existing uses like CLI and REPL would do nothing different, but a tooling user of the Compiler could use that information.

Possible issue: if the form is lazy or large?



 Comments   
Comment by Thomas Heller [ 16/Mar/17 12:29 PM ]

I added the latest form to the CompilerException when available but that form contains basically no metadata.

user=> (defn x
  :foo)
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
...

user=> (binding [*print-meta* true] (prn (.-form *e)))
^{:line 17, :column 1} (defn x :foo)

That information is already present in the CompilerException. Having the form provides minimal benefit in this case since it has lost all other source information and we cannot tell how this look in the the source. To get a source mapping for tools so they can highlight the correct area it would still need to read the form itself (and probably not using the form from the Exception).

Comment by Thomas Heller [ 16/Mar/17 12:37 PM ]

Looking into LINE_BEFORE, COLUMN_BEFORE, LINE_AFTER, COLUMN_AFTER now to potentially provide the exact boundaries of the form if possible.

Comment by Thomas Heller [ 16/Mar/17 2:00 PM ]

I added the current reader location to the Compiler Exception [1].

user=> (load-file "/Users/zilence/code/shadow-devtools/src/dev/demo/defn_error.clj")
CompilerException clojure.lang.ExceptionInfo: Call to clojure.core/defn did not conform to spec:
...
user=> (.. *e -location -lineBefore)
4
user=> (.. *e -location -lineAfter)
6
;; column is 1 before/after, source is
(defn x
  :foo)

This would at least allow extracting the entire source string of the form easily. When using a reader however it could start at the location already provided by the CompilerException and it would find the end on its own. The bindings required for the reader location are also lost when working in a REPL, so it always points to 0/0-0/0. Not sure this is worth pursuing.

[1] https://github.com/clojure/clojure/compare/master...thheller:CLJ-2128





[CLJ-2126] Can set! to fields of a defrecord Created: 14/Mar/17  Updated: 14/Mar/17

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: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-CLJ-2126-don-t-assign-final-fields.patch    
Patch: Code
Approval: Triaged

 Description   

It is possible to set! to fields of a defrecord even though they are final.

(defprotocol SetA (seta [x a]))
=> SetA
(defrecord X [a]
  SetA
  (seta [this newa]
    ; Next line should error at compile time, does not.
    ; (However (set! a newa) does error correctly.)
    (set! (.a this) newa)))
=> user.X
(def x (->X 0))
=> #'user/x
x
=> #user.X{:a 0}
(seta x 1) ;; This should not run.
=> 1
x
=> #user.X{:a 1}

There are two issues here:

  1. The Clojure compiler does not detect that (set! (.a this) x) is assignment to a final field. This could be enhanced. Nicola Mometto has discovered why and believes he has a straightforward patch.
  2. The JVM bytecode verifier only performs the necessary final-assignment check on classfiles version 9 and above: https://bugs.openjdk.java.net/browse/JDK-8159215 (Clojure generates version 6 classfiles.) This is out of our hands.

Approach: make the compiler fail at compile time if trying to set! a field that's final

Patch: 0001-CLJ-2126-don-t-assign-final-fields.patch






[CLJ-2123] Lighter-weight aliasing for keywords Created: 10/Mar/17  Updated: 27/Sep/17

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 18
Labels: keywords, namespace

Approval: Vetted

 Description   

It is useful to make qualified keywords, and particularly so with spec. Using namespace aliases helps a lot in working with a lot of qualified keywords. However, currently creating an aliased namespace requires that the namespace actually exists.

This ticket is a placeholder to do something more with lighter-weight aliasing for keywords. Details TBD.



 Comments   
Comment by Herwig Hochleitner [ 26/Sep/17 8:01 PM ]

Hoping to get the conversation started on this in time for a 1.9-beta:

The rationale in my predecessor ticket, CLJ-2030, was to create a construct to allow for auto-aliasing in .cljc files, which I would like to submit as a requirement. The obvious place for cljs to declare an auto-alias is the ns clause.
So if we don't want to grow alias for that use case, I would propose to grow the ns clause with a declaration for keyword - aliases. Let's give it a working title of (:kwns-alias ..) for this comment.

:kwns-alias would be used to establish namespace aliases for ::qualified/keywords

One open question is, how :kwns-alias should interact with alias. i.e. whether the namespace of ::qualified/keyword should always expand to the same as the one of a qualified/symbol or if they should be allowed to differ. I'd argue they should always be the same, because of the rule of simplicity. That means, that

  • alias will need to check if the sym is already in :kwns-alias and throw, if so
  • :kwns-alias will need to also work on `qualified/keywords probably shouldn't have knws in its name any more

So what's a good name for :kwns-alias? :let maybe?

Comment by Alex Miller [ 26/Sep/17 8:23 PM ]

beta = feature complete, so this isn't happening in 1.9.

I think adding anything to ns is likely off the table, but until I learn more about what Rich has in mind, I can't really suggest anything more.

Comment by Herwig Hochleitner [ 27/Sep/17 7:05 AM ]

that's a pity. but maybe 1.10 will have a shorter release time ...

In any case, I'll be very interested in Rich's idea to do this outside of the ns clause and still have it fit well with clojurescript (or even clojure, for that matter, ...)
Do you have any rationale, why growing ns for this is a bad idea?

Comment by Alex Miller [ 27/Sep/17 7:07 AM ]

ns already does way too much stuff and we don't have any desire to make it do more.

Comment by Herwig Hochleitner [ 27/Sep/17 12:20 PM ]

Since ns is essential to how a namespace is set up in clojure, I'd imagine a fix for that problem (of ns doing too much) to imply some rather drastic changes to the state of the art. As we won't break ns, I'd imagine either some sort of ns2 or a way to do namespace setup outside of the ns clause. I'm curious either way.

Since I know, that Rich will not be hassled about his hammock time, I'll lay off this for now. I'll be sure to check back as the next round of alphas goes out. I also hope for some productive discussion in the meantime.





[CLJ-2116] Support for selective conforming with clojure.spec Created: 22/Feb/17  Updated: 04/Oct/17

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

Type: Feature Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 23
Labels: spec
Environment:

[org.clojure/clojure "1.9.0-alpha14"]


Attachments: Text File clj-2116.patch    

 Description   

Problem

using clojure.spec in runtime border validation supporting multiple exchange formats is hard.

Details

Currently in clojure.spec (alpha-14), conformers are attached to Spec instances at creation time and they are invoked on every conform. This is not very useful in system border validation, where conforming/coercion functions should be selected based on runtime data, e.g. the exchange format.

Examples:

  • a keyword? spec:
    • with EDN, no coercion should be done (it can present Keywords)
    • with JSON, String->Keyword coercion should be applied
    • with String-based formats (CSV, query-params, ...), String->Keyword coercion should be applied
  • a integer? spec:
    • with EDN, no coercion should be done (it can present numbers)
    • with JSON, no coercion should be done (it can present numbers)
    • with String-based formats (CSV, query-params, ...), String->Long coercion should be applied

Here is a more complete example:

(s/def ::id integer?)
(s/def ::name string?)
(s/def ::title keyword?)
(s/def ::person (s/keys :opt [::id], :req-un [::name ::title]))

;; this is how we see the data over different exchange formats
(def edn-person {::id 1, :name "Tiina", :title :boss})
(def json-person {::id 1, :name "Tiina", :title "boss"})
(def string-person {::id "1", :name "Tiina", :title "boss"})

;; here's what we want
(def conformed-person edn-person)

To use this today, one needs to manually create new border specs with different conformers for all different exchange formats. Non-qualified keywords could be mapped in s/keys to work (e.g. ::title => ::title$JSON), but this wont work if fully qualified keys are exposed over the border (like ::id in the example) - one can't register multiple, differently conforming version of the spec with same name.

Suggestion

Support selective conforming in the Spec Protocol with a new 3-arity conform* and clojure.spec/conform, both taking a extra user-provided callback/visitor function. If the callback is provided, it's called from within the Specs conform* with the current spec as argument and it will return either nil or a 2-arity conformer function that should be used for the actual confrom.

Actual conforming-matcher implementations can be maintained in 3rd party libraries, like spec-tools[1].

Using it would look like this:

;; edn
(assert (= conformed-person (s/conform ::person edn-person)))
(assert (= conformed-person (s/conform ::person edn-person nil)))

;; json
(assert (= conformed-person (s/conform ::person json-person json-conforming-matcher)))

;; string
(assert (= conformed-person (s/conform ::person string-person string-conforming-matcher)))

Alternative

Another option to support this would be to allow Specs to be extended with Protocols. 3rd party libs could have a new Conforming protocol with 3-arity conform and add implementations for it on all current specs. Currently this is not possible.

[1] https://github.com/metosin/spec-tools



 Comments   
Comment by Alex Miller [ 22/Feb/17 3:33 PM ]

I don't think we are interested in turning spec into a transformation engine via conformers, so I suspect we are probably not interested. However, I'll leave it for Rich to assess.

Comment by Tommi Reiman [ 23/Feb/17 1:26 AM ]

Currently, Plumatic Schema is the tool used at the borders. Now, people are starting to move to Spec and it would really bad for the Clojure Web Developement Story if one had to use two different modelling libraries for their apps. If Spec doesn't want to be a tranformation engine via conformers, I hope for the Alternative suggestion to allow 3rd parties to write this kind of extensions: exposing Specs as Records/Types instead of reified protocols would do the job?

Comment by ken restivo [ 28/Feb/17 9:43 PM ]

I could see why the Clojure core developers might not want Spec to support this kind of coercion, but the practical reality is that someone will have to. If it isn't in Spec itself, it'll have to be done libraries built upon it like Tommi's.

The use case here is: I have a conf file that is YAML. I'm parsing the YAML using a Clojure library, turning it into a map. Now I have to validate the map, but YAML doesn't support keywords, for example, and the settings structure goes directly into Component/Mount/etc as part of the app state, so it makes sense to run s/conform on it as the first step in app startup after reading configuration. Add to this the possibility of other methods of merging in configuration (env vars, .properties files, etc) and this coercion will be necessary somewhere.

Comment by Tommi Reiman [ 08/May/17 1:03 PM ]

Any news on assessing this? I would be happy to provide a patch or a link to a modified clojure.spec with samples on usage with the 3-arity conform in it. Some thinking aloud: http://www.metosin.fi/blog/clojure-spec-as-a-runtime-transformation-engine/

Comment by Alex Miller [ 09/May/17 10:10 AM ]

Rich hasn't looked at it yet. My guess is still that we're not interested in this change. While I think some interesting problems are described in the post, I don't agree with most of the approaches being taken there.

Comment by Simon Belak [ 10/May/17 4:39 AM ]

Why not just use s/or (or s/alt) and then dispatch on the tag. Something like:

(s/def ::id (s/and (s/or :int integer?
                         :str string?)
                   (s/conformer (fn [[tag x]]
                                  (case tag
                                    :int x
                                    :str (Integer/parseInt x))))))

I use that pattern quite a bit in https://github.com/sbelak/huri and with a bit of syntactic sugar it works quite well.

Comment by Imre Kószó [ 12/May/17 3:46 AM ]

Simon that will not work if you are trying to conform to specs from third parties though. One of the points of this suggestion is that third parties would be able to write their own conformers to existing specs without redefining those specs.

Comment by Tommi Reiman [ 08/Jun/17 1:40 AM ]

Thanks for the comments. I would be happy to provide a patch / sample repo with the changed needed for this, in hope that it would help to decide if this could end up in the spec or not. What do you think?

Below is a sample of initial spec-integration into ring/http libs, using spec-tools. For now, one needs to wrap specs into spec records to enable the 3-arity conforming. This is boilerplate I would like to see removed. With this change, it should work out-of-box for all (3rd party) specs.

(require '[compojure.api.sweet :refer :all])
(require '[clojure.spec.alpha :as s])
(require '[spec-tools.core :as st])

;; to enable 3-arity conforming
(defn enum [values]
  (st/spec (s/and (st/spec keyword?) values)))

(s/def ::id int?)
(s/def ::name string?)
(s/def ::description string?)
(s/def ::size (enum #{:L :M :S}))
(s/def ::country (st/spec keyword?) ;; to enable 3-arity conforming
(s/def ::city string?)
(s/def ::origin (s/keys :req-un [::country ::city]))
(s/def ::new-pizza (st/spec (s/keys :req-un [::name ::size ::origin] :opt-un [::description])))
(s/def ::pizza (st/spec (s/keys :req [::id] :req-un [::name ::size ::origin] :opt-un [::description])))

;; emits a ring-handler with input & output validation (& swagger-docs)
;; select conforming based on request content-type (e.g. json/edn) + strip-extra keys from maps
(context "/spec" []
  (resource
    {:coercion :spec
     :parameters {:body-params ::new-pizza}
     :responses {200 {:schema ::pizza}}
     :post {:handler (fn [{new-pizza :body-params}]
                       (ok (assoc new-pizza ::id 1))}}))
Comment by Tommi Reiman [ 21/Jul/17 4:13 AM ]

Intended to create internal PR in my fork of clojure.spec, but ended up doing a real DUMMY PR for the actual repo. Well, here it is anyway:

https://github.com/clojure/spec.alpha/pull/1

Happy to finalize & create a patch into Jira if this goes any further.

Comment by Tommi Reiman [ 21/Jul/17 4:14 AM ]

comments welcome. here's a sample test for it:

(deftest conforming-callback-test
  (let [string->int-conforming
        (fn [spec]
          (condp = spec
            int? (fn [_ x _]
                   (cond
                     (int? x) x
                     (string? x) (try
                                   (Long/parseLong x)
                                   (catch Exception _
                                     ::s/invalid))
                     :else ::s/invalid))
            :else nil))]

    (testing "no conforming callback"
      (is (= 1 (s/conform int? 1)))
      (is (= ::s/invalid (s/conform int? "1"))))

    (testing "with conforming callback"
      (is (= 1 (s/conform int? 1 string->int-conforming)))
      (is (= 1 (s/conform int? "1" string->int-conforming))))))
Comment by Tommi Reiman [ 22/Jul/17 2:12 AM ]

initial work as patch.

Comment by Tommi Reiman [ 04/Oct/17 1:02 AM ]

Any news on this?





[CLJ-2115] Support data conveying conform errors (alternative/complement to :clojure.spec/invalid) Created: 22/Feb/17  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: spec


 Description   

At the moment if a conform calls fails (returning :clojure.spec/invalid) there is no way to supply extra information about why it failed. We do have the possibility to get explain-data, but at best this would return a spec form, the original value and some metadata.

While this is fine in most cases, some conformer functions upon failure can provide extra data that'd be useful to the consumer. A practical example we had was a spec that can contain values for a String based DSL (think SQL like), that would conform these values to their parsed AST. When the conform wrapped function fails it would throw an ex-info with line/col info and more metadata about the failure. But all this data was lost since we can only return :clojure.spec/invalid. All this happened inside a rule engine schema, that can contain hundreds of these; re-parsing all the failing values for error reporting is something we wanted to avoid.

The proposal would be to to support a new return value that'd allow conveying data about the conform failure, or to support both this new value and :clojure.spec/invalid.
This could take the form of (explain-info {..}) potentially returned by conformer function for later consumption by explain, to match clojure semantics with exceptions (ex-info/ex-data, explain-info/explain-data).

A more naive implementation could just allow to throw inside the conformer function and have the error merged/assoced into the explain map (but that might be a bit too invasive in my opinion).






[CLJ-2112] Add specs for spec forms Created: 15/Feb/17  Updated: 06/Aug/17

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

Type: Feature Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 17
Labels: spec

Attachments: Text File spec-forms.patch    
Approval: Incomplete

 Description   

It would be useful to have specs that described spec forms, such that it was possible to go from a spec form like (s/keys :req [::a ::b] :opt [::c]) to a conformed version that allowed you to grab the parts without parsing the s-expression. This can be done by creating specs, thus allowing:

user=> (require '[clojure.spec :as s] '[clojure.spec.specs])
user=> (s/def ::aspec (s/keys :req [::a ::b] :opt [::c]))
user=> (def aspec-data (s/conform :clojure.spec.specs/spec (s/form ::aspec)))
user=> (pr aspec-data)
[:form {:s clojure.spec/keys, 
        :args {:req [[:key :clojure.spec.specs/a] [:key :clojure.spec.specs/b]], 
               :opt [:clojure.spec.specs/c]}}]
user=> (map val (-> aspec-data val :args :req))
(:clojure.spec.specs/a :clojure.spec.specs/b)

Patch: spec-forms.patch (a work in progress)



 Comments   
Comment by Saul Shanabrook [ 05/Aug/17 6:08 PM ]

Could I help out on this? Happy to work on it. It would be very helpful for me, trying parse the the specs from the :args and :ret in fspec.

Comment by Alex Miller [ 06/Aug/17 11:05 PM ]

As you can see, there is an existing patch here with substantial work on it already (and some early review from Rich and Stu). The most useful help on this at the moment would be feedback on the gaps/open questions in it.

Comment by Alex Miller [ 06/Aug/17 11:17 PM ]

Also, I should mention that I do not expect this to be finalized and included until we have finalized the spec forms themselves as they are still subject to change.





[CLJ-2111] Clarify s/every docstring for :kind Created: 14/Feb/17  Updated: 28/Jun/17

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

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

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

 Description   

Docstring for `s/every` in the `:kind` option says "a pred/spec that the collection type must satisfy, e.g. vector?" but using a spec for `:kind` will fail:

user=> (s/valid? (s/every number? :kind (s/or :vector vector? :list list?))
          [])
ClassCastException clojure.spec$or_spec_impl$reify__13891 cannot be cast to clojure.lang.IFn  dev/eval44499/fn--44501 (form-init3178965928127409998.clj:22)

user=> (pst *e)
ClassCastException clojure.spec$or_spec_impl$reify__13903 cannot be cast to clojure.lang.IFn
	user/eval20/fn--22 (NO_SOURCE_FILE:13)
	clojure.spec/every-impl/reify--14039 (spec.clj:1225)
	clojure.spec/valid? (spec.clj:744)
	clojure.spec/valid? (spec.clj:740)

Proposed: The intent here was just to support a predicate function. Change docstring to say just "pred" rather than "pred/spec".

Patch: clj-2111.patch



 Comments   
Comment by Alex Miller [ 14/Feb/17 5:06 PM ]

Certainly a function like this works (s/every number? :kind #(or (vector? %) (list? %))). The question is whether the s/every doc that states "pred/spec" means only a predicate function or "predicate function OR spec". I'm not sure what the intention was. Certainly the code in every seems to be wrapping the kind into a function and then invoking it in every-impl, so it's not written to accept a spec currently.

Comment by Alex Miller [ 14/Feb/17 5:06 PM ]

Marking vetted to either resolve, update docstring, or decline. Need more info from Rich.





[CLJ-2105] incorrect spec conform when an optional (?) is inside of a one or more (+) Created: 03/Feb/17  Updated: 03/Feb/17

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

Type: Defect Priority: Major
Reporter: Anthony D'Ambrosio Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

1.9.0-alpha14


Approval: Vetted

 Description   
(s/def ::thing (s/cat :a (s/? string?) :b (s/+ number?))) 
(s/def ::seq-of (s/+ ::thing))

(s/conform ::seq-of (list "foo" 1 "bar" 2 3 "qux" 4)) 
;=> [{:a "foo", :b [1]} [{:a "bar", :b [2 3]} {:a "qux", :b [4]}]]

;EXPECTED 
;=> [{:a "foo", :b [1]} {:a "bar", :b [2 3]} {:a "qux", :b [4]}]

;; ONLY 2nd thing matters? 
(s/conform ::seq-of (list "foo" 1 2 "bar" 3)) 
;=> [{:a "foo", :b [1 2]} {:a "bar", :b [2]}]

;; NO OPTIONAL 
(s/def ::thing (s/cat :a string? :b (s/+ number?))) 
(s/def ::seq-of (s/+ ::thing))
(s/conform ::seq-of (list "foo" 1 "bar" 2 3 "qux" 4)) 
;=> [{:a "foo", :b [1]} {:a "bar", :b [2 3]} {:a "qux", :b [4]}]

This also only shows up if there are 2+ numbers in the 2nd or later ::thing
AND the problem goes away if I make :a not optional...

Could be related to http://dev.clojure.org/jira/browse/CLJ-2003 ?






[CLJ-2102] Reduce collection generator default size from 20 Created: 25/Jan/17  Updated: 12/May/17

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: 3
Labels: generator, spec

Attachments: Text File clj-2102-2.patch     Text File clj-2102-3.patch     Text File clj-2102.patch    
Patch: Code
Approval: Incomplete

 Description   

In general I find that it is very easy (especially with nested or recursive collections) to have a check run OOME due to generating very large nested collections. Currently the default is 20 - I think we should change it to 3.

The attached patch just changes the default from 20 to 3. An alternate approach would be to change it to a dynvar setting.

Patch: clj-2102-3.patch



 Comments   
Comment by Alex Miller [ 07/Apr/17 3:45 PM ]

Updated patch to apply to master

Comment by Alex Miller [ 10/May/17 12:54 PM ]

Updated patch to apply to spec.alpha

Comment by Stuart Halloway [ 12/May/17 10:32 AM ]

I have definitely seen the pain here – nested collections can get big fast. OTOH for non-nested collections the larger generator is nice. Not sure moving the default is a help.

Comment by Alex Miller [ 12/May/17 3:23 PM ]

Use dynamic var and reduce default. Also consider ways to avoid this kind of problem in test.check itself (how does quickcheck deal with this?).





[CLJ-2097] Improve generation failure exception Created: 10/Jan/17  Updated: 11/Jan/17

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

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


 Description   

It's pretty easy to write a spec whose generator fails like this:

Couldn't satisfy such-that predicate after 100 tries.

This is of course expected in many ways, but it's a very unhelpful error. Some things that could make this better include:

  • Including the spec that failed in the exception. I only see one invocation of gen/such-that in spec.clj, and it appears to have the spec's form at hand. gen/such-that takes an exception constructor where this could be used.
  • Allow max-tries to be changed from the hardcoded value of 100. When dealing with an intermittent failure, it can be useful to crank down max-tries to a very small number, making the failure easier to reproduce.


 Comments   
Comment by Alex Miller [ 11/Jan/17 8:41 AM ]

These are reasonable suggestions and this area is likely to evolve in tandem with test.check to provide better info.





[CLJ-2092] deftype instances with mutable fields cannot be compiled Created: 24/Dec/16  Updated: 31/Dec/16

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype

Approval: Triaged

 Description   

When evaluating or compiling an implementer of clojure.lang.IType, the compiler tries to reflectively access its fields. This fails, when a field is marked mutable (hence private):

Clojure 1.9.0-master-SNAPSHOT
user=> (deftype T [^:unsynchronized-mutable t])
user.T
user=> (T. :t)
#object[user.T 0x2654635 "user.T@2654635"]
user=> (eval (T. :t))
CompilerException java.lang.IllegalArgumentException: No matching field found: t for class user.T
            Reflector.java:  271  clojure.lang.Reflector/getInstanceField
             Compiler.java: 4724  clojure.lang.Compiler$ObjExpr/emitValue
             Compiler.java: 4851  clojure.lang.Compiler$ObjExpr/emitConstants
             Compiler.java: 4529  clojure.lang.Compiler$ObjExpr/compile
             Compiler.java: 4049  clojure.lang.Compiler$FnExpr/parse
             Compiler.java: 6866  clojure.lang.Compiler/analyzeSeq
             Compiler.java: 6669  clojure.lang.Compiler/analyze
             Compiler.java: 6924  clojure.lang.Compiler/eval
             Compiler.java: 6890  clojure.lang.Compiler/eval
                  core.clj: 3105  clojure.core/eval
...

For classes that don't implement IType, no such problem exists.

user> (deftype* user/U user.U
        [^:unsynchronized-mutable u]
        :implements [])
nil
user> (eval (user.U. :u))
#object[user.U 0x34699051 "user.U@34699051"]

This problem commonly occurs, when implementing a tagged literal for a deftype with cached hash.



 Comments   
Comment by Alex Miller [ 31/Dec/16 12:01 PM ]

Yeah, this is interesting. The compiler compiles a deftype into a call to the constructor with the current values of the fields, but mutable fields are not accessible. One alternative would be to provide some standard method to "read" the field set rather than relying on reflection. (Another would be changing the access modifiers for mutable fields but I think that's probably a non-starter.)





[CLJ-2090] Improve clojure.core/distinct perf by using transient set Created: 23/Dec/16  Updated: 04/Jan/17

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: 0
Labels: performance, transducers, transient

Attachments: Text File clj-2090-use-transient-set-in-distinct-2.patch    
Patch: Code
Approval: Triaged

 Description   

Current implementation of clojure.core/distinct uses persistent set. This patch improves performance of lazy arity by ~25%-30% and transducer by ~40%-50% by using transient set instead.

10 elements
(doall (distinct coll)) 	 5.773439 µs => 4.179092 µs (-27%)
(into [] (distinct) coll) 	 3.238236 µs => 1.943254 µs (-39%)

100 elements
(doall (distinct coll)) 	 67.725764 µs => 42.129993 µs (-37%)
(into [] (distinct) coll) 	 35.702741 µs => 16.495947 µs (-53%)

1000 elements
(doall (distinct coll)) 	 540.652739 µs => 399.053873 µs (-26%)
(into [] (distinct) coll) 	 301.423077 µs => 164.025500 µs (-45%)

10000 elements
(doall (distinct coll)) 	 3.439137 ms => 3.058872 ms (-11%)
(into [] (distinct) coll) 	 1.437390 ms => 848.277178 µs (-40%)

Benchmarking code: https://gist.github.com/tonsky/97dfe1f9c48eccafc983a49c7042fb21



 Comments   
Comment by Alex Miller [ 23/Dec/16 8:52 AM ]

You can't remove the volatile - you still need that for safe publication in multi threaded transducing contexts.

Comment by Nikita Prokopov [ 23/Dec/16 11:50 AM ]

Alex Miller How do you mean?

  • I don’t update seen link because transient set can be mutated in-place
  • Are transducers meant to be used from multiple threads? Because even existing implementation clearly has race condition. I imagine fixing that would be costly (we’ll need a synchronized section), so maybe it should be a specialized transducer that you use only when needed?
Comment by Alex Miller [ 23/Dec/16 12:26 PM ]

Transient sets can NOT be mutated in place - you must use the return value.

Yes, transducers are used from multiple threads in (for example) transducer chans in core.async go blocks.

Comment by Alex Miller [ 23/Dec/16 12:28 PM ]

I should also say transducers are not expected to be used from more than one thread at a time, so there are no race problems. But being used from multiple threads over time requires proper safe publication.

Comment by Nikita Prokopov [ 24/Dec/16 3:07 AM ]

But being used from multiple threads over time requires proper safe publication.

Does that imply that no transients could be used in transducers (because underlying arrays on which transient impl is based are mutated in place, so different threads could potentially see different states of transient object)?

Does that also mean that partition-by and partition-all should be fixed (they use java.util.ArrayList which, being array of references, has no safe publication semantics)?

Transient sets can NOT be mutated in place - you must use the return value.

I was thinking that clojure/core.clj and clojure.lang.ATransientSet.java are both part of Clojure internals, colocated, so can share a little bit of internal knowledge about each other. It seems safe to do that, because that knowledge does not leak outside, and, if at any point impl of ATransientSet would change, core.clj could be updated accordingly in the same release. I wouldn’t do that in any third-party library, of course.

Comment by Alex Miller [ 24/Dec/16 9:13 AM ]

Does that imply that no transients could be used in transducers (because underlying arrays on which transient impl is based are mutated in place, so different threads could potentially see different states of transient object)?

Transients require only that they are asked by no more than a single thread at a time and so are safe to use in a transducer. However, they should guarantee safe publication. core.async channels already do this as an artifact of their implementation, but other transducing contexts may not.

Transients should NEVER be used as "mutate in place", regardless of concurrency. While they will appear to "work" in some circumstances, this is never correct (eventually an update operation will return a new instance and if you are mutating in place, your data will then be missing). This is discussed and correct examples are shown at http://clojure.org/reference/transients.

Does that also mean that partition-by and partition-all should be fixed (they use java.util.ArrayList which, being array of references, has no safe publication semantics)?

That's something Rich and I are discussing but, probably.

Comment by Nikita Prokopov [ 24/Dec/16 12:56 PM ]

Alex Miller Here’s quick test that shows that changes to transient set (which is nothing more but a wrapper around transient map) made in one thread are not always visible from another thread.

https://gist.github.com/tonsky/62a7ec6d539fc013186bee2df0812cf6

That means that if we try to use transients for e.g. distinct it will miss duplicate items

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

Removed transients from transducer arity of distincts because transducers might be accessed from multiple threads

Comment by Nikita Prokopov [ 24/Dec/16 1:12 PM ]

Maybe that doc http://clojure.org/reference/transients should be updated re: transients are not safe to use from multiple threads because changes made by one thread are not necessarily visible to another. Even if they don’t compete

Comment by Alex Miller [ 31/Dec/16 12:54 PM ]

I would say that test is demonstrating a bug in transient sets/maps and you should file a ticket for that as it's a lot more important than this enhancement.

distinct should be able to use transients in both the transducer and lazy seq impls. The issue with contains? not working on transients is actually a separate ticket - http://dev.clojure.org/jira/browse/CLJ-700 that will likely require some class hierarchy rearrangement. I don't think we would take this change until that is fixed (so that you can avoid relying on the class and Java method variants).

Comment by Nikita Prokopov [ 04/Jan/17 11:47 AM ]

I have to admit my test was demonstrating something else: there were no proper thread isolation. So it was a concurrency issue, not “safe publication” issue. My current understanding is this:

Transients require thread isolation. Use of a particular transient instance should be controlled either by using it in an single-threaded scope, or in a framework that enforces this.

That guarantee implicitly presumes that there’s happens-before relation between transient usage from multiple threads. There’s no other way to define “only one thread is in this section at a time”.

That, in turn, means that all writes that happened in thread 1 are visible in thread 2, regardless to volatility of the variables involved. In fact, we can remove all volatiles from transients implementation and probably make them faster, because, by asking “no more than one thread at a time” we enforce users to establish happens-before between sections, and that would give us all the safe publication guarantees we need.

Is my understanding correct? Am I missing something?

Comment by Nikita Prokopov [ 04/Jan/17 11:55 AM ]

Also, long-living transients (e.g. in a transducers associated with a queue, for example) will hold a reference to a thread that created them. Is that a bad thing? Should we switch to boolean flag instead?





[CLJ-2089] Sorted colls with default comparator don't check that first element is Comparable Created: 19/Dec/16  Updated: 12/May/17

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

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

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

 Description   

Sorted maps and sets use the default comparator. The default comparator requires elements to be Comparable. PersistentTreeMap will not actually invoke the comparator until the second element is added to the map/set, so it is possible to create an invalid single element sorted map/set that will blow up only on later use.

The following examples create invalid "timebomb" collections that will throw ClassCastException if another element is added or if they're used in other ways (because sets are not comparable):

(sorted-set #{1})
(conj (sorted-set) #{1})
(sorted-map #{} 1)
(assoc (sorted-map) #{} 1)

Example:

(def s (sorted-set #{1}))  ;; this doesn't fail
(conj s #{2})              ;; first conj triggers error
ClassCastException clojure.lang.PersistentHashSet cannot be cast to java.lang.Comparable  clojure.lang.Util.compare (Util.java:153)

Cause: In PersistentTreeMap.add(), in the case where the existing tree is null, the comparator is never invoked and so the default comparator will never throw the ClassCastException seen on later compares. Note that none of this applies for a custom comparator (sorted-set-by and sorted-map-by) - those comparators can do whatever.

Proposed: In add(), if the map is empty AND the comparator is the default comparator, then check whether the added key is Comparable and throw CCE if not. Note that PersistentTreeMap is also the impl used by PersistentTreeSet so this covers both. The error message is customized to give you a better hint about the problem (and written to be applicable for both maps and sets). In the patch some existing (bad) tests had to be adjusted.

user=> (def s (sorted-set #{1}))
ClassCastException Default comparator requires nil, Number, or Comparable: #{1}  clojure.lang.PersistentTreeMap.add (PersistentTreeMap.java:335)

One downside of the current patch is that does not catch the equivalent problem if using a custom comparator and a bad first element. You could maybe do this by calling comp.compare(k,k) (although many comparators, like the default comparator, have an early identity check that would not fail on this). For now, decided that if you supply a custom comparator, it's up to you to not use it with elements that don't work with that comparator.

Patch: clj-2089.patch

Screener Notes: The error is a ClassCastException in order to match the exception that would have come later, but is odd in that no class casting was done. Maybe IllegalArgumentException?






[CLJ-2083] spec for printable/readable/edn data Created: 12/Dec/16  Updated: 15/May/17

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

Type: Feature Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When spec'ing some things, I've used `any?` in a few cases where it is overly permissive. In particular, sometimes I need to specify a value must be printable/readable, such as when a value may wind up in an edn file. Similarly, I've needed to spec something must have non-generative value-identity, ie. ban closures, etc. Printable/readable or simply `edn?` would be a much better approximation than `any?`.



 Comments   
Comment by Brandon Bloom [ 12/Dec/16 1:08 PM ]

I realize that an edn? predicate would have O(N) runtime, vs an edn spec that could take advantage of every/every-kv etc for sampling conformance.

Comment by Herwig Hochleitner [ 12/Dec/16 11:12 PM ]

Related: CLJ-1527





[CLJ-2080] clojure.spec/every-kv does not work on vectors - improve docs/errors Created: 08/Dec/16  Updated: 12/May/17

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

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

alpha 14


Attachments: Text File clj-2080-2.patch     Text File clj-2080-3.patch     Text File clj-2080-4.patch     Text File clj-2080-5.patch     Text File clj-2080-6.patch     Text File clj-2080-7.patch     Text File clj-2080.patch    
Patch: Code
Approval: Screened

 Description   

The every-kv doc states "takes separate key and val preds and works on associative collections". Vectors return true for associative? but do not currently work:

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

Another similar problem:

(s/explain-data (s/every-kv int? int?) [{:a :b}])
UnsupportedOperationException nth not supported on this type: PersistentArrayMap  clojure.lang.RT.nthFrom (RT.java:903)

Cause: Vectors should not work with every-kv. The combination of every-kv and every-impl assume that the collection passed to every-kv can provide a seq of map entries. In the explain case, the ::kfn created by every-kv is used to create a better path segment using the key rather than the element index. The kfn assumes that an element of the collection can call `(nth entry 0)` on the element. In the explain failure above, the map {:a :b} will throw when invoked with nth.

Proposed: Do the following to be clearer about the requirement that the coll elements are map entries:

  • Modify the docstring to say "seqs to map entries" rather than "is a map"
  • Modify the kfn to add a check that the element is an entry and if so, use it's key. If not, use the index (of the element itself). In this case, when passed an entry it will report with the actual key but when passed something that's not an entry, it will report the collection based index of the non-entry.

After the patch, the explain-data call will provide a useful error rather than the exception above:

user=> (s/explain-data (s/every-kv int? int?) [{:a :b}])
#:clojure.spec.alpha{:problems ({:path [], :pred vector?, :val {:a :b}, :via [], :in [0]})}

Patch: clj-2080-7.patch



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

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

Comment by Alex Miller [ 31/Mar/17 9:07 AM ]

Updated to apply to master

Comment by Alex Miller [ 10/May/17 12:26 PM ]

Updated patch to apply to spec.alpha.

Comment by Stuart Halloway [ 12/May/17 10:08 AM ]

Can you please update this ticket with expected behavior for the examples shown in the description, plus test showing that expected behavior. I am still seeing vectors fail for conform.

(s/explain-data (s/every-kv integer? string?) ["x"])
=> #:clojure.spec.alpha{:problems ({:path [], :pred vector?, :val "x", :via [], :in [0]})}
Comment by Alex Miller [ 12/May/17 10:33 AM ]

Vectors should be failing - every-kv requires a seq of map entries. The patch clarifies this in the docstring and fixes the exception that can (incorrectly) be thrown when producing explain-data. I updated the title, the description to include the explain-data after behavior, and added a test to the patch.

Comment by Alex Miller [ 12/May/17 10:52 AM ]

Lost docstring change a ways back, re-added in -7 patch.





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

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

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

Approval: Vetted

 Description   

Generator overrides for spec aliases are not respected.

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

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

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

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


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

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

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

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

(require '[clojure.spec :as s])
(s/def ::original number?)
(s/def ::alias ::original)
(@#'clojure.spec/spec-name (s/get-spec ::alias))
;; => :user/original
Comment by Charles Despointes [ 20/Jun/17 1:19 PM ]

I've a somewhat similar issue. I think it is related.
I'm trying to do something like :

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.gen.alpha :as gen])
(s/def ::bar any?)
(s/def ::foo (s/with-gen any? (fn [] (s/gen ::bar))))
(gen/generate (s/gen ::foo {::bar (fn [] (s/gen int?))}))

I'm somewhat expecting it generates me an integer like it would have with a direct aliasing to ::bar in ::foo definition. But it doesn't and keep the with-gen binded generator.
Is that the same issue or is that an expected behaviour or should i fill a new issue ?





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

Status: Reopened
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: 10
Labels: None

Attachments: Text File clj-2075-add-three-arities-to-comparisons-3.patch    
Patch: Code
Approval: Triaged

 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

Comment by Nikita Prokopov [ 23/Dec/16 7:50 AM ]

I found a problem with previous patches that during defining = (equality), and is not yet defined. Replaced with if

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

Dupe of CLJ-1912

Comment by Nikita Prokopov [ 15/May/17 3:43 PM ]

Alex Miller It is a duplicate, but my patch is waaaaaaaaay faster. Just look at the numbers (70-80% improvement vs 5-10%). It’s because I introduced a real arity so that intermediate collection is not created and is not destructured in case of 3 arguments.

Comment by Jozef Wagner [ 15/May/17 11:55 PM ]

There's a quite serious bug in the supplied patch(es), that causes e.g. (= 3 3 2) to return true. Because of this the benchmarks are flawed too I guess.

Comment by Nikita Prokopov [ 16/May/17 3:13 PM ]

Jozef Wagner thanks for spotting this! Attaching an updated path. Benchmark wasn’t flawed too much because perf gain comes not from doing one less/one more comparison but from not having an overhead of calling a fn with unknown arity.





[CLJ-2074] ::keys spec conflicts with destructuring spec Created: 02/Dec/16  Updated: 11/Jan/17

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Alex Miller
Resolution: Unresolved Votes: 1
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.



 Comments   
Comment by Brandon Bloom [ 10/Jan/17 5:36 PM ]

I also just ran in to this problem. Just wanted to say that I'd like to see a fix, but I'm not quite sure about the proposed solution. Or, at least, the name "closed?" seems to imply a non-extensible map, when in reality the flag more or less means "not a map that participates in the global keys system", for which I do not have a better name suggestion.

Comment by Alex Miller [ 11/Jan/17 8:35 AM ]

The proposed patch is a non-starter. I have some ideas on how to address this, but just haven't gotten around to working on it yet.

Comment by Alex Miller [ 11/Jan/17 8:37 AM ]

Removed proposal and patch from the ticket as we will not be going this direction. Captured here for reference:

"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-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: 1
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 evaluated predicate yields :s/unknown Created: 23/Nov/16  Updated: 10/May/17

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: 1
Labels: spec

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

 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?

Cause: specize was falling through on these cases to Object and just returning unknown.

Proposed:
Special handling for 2 cases:
1. Sets - explictly catch IPersistentSet and use the set as the form.
2. Functions - demunge the function name and use the qualified function name symbol as the form. Add a special check for anonymous functions and revert to ::unknown for those (not much we can do with an eval'ed anonymous function).

Patch: clj-2068-3.patch



 Comments   
Comment by Alex Miller [ 13/Dec/16 6:52 PM ]

Simplified anon fn check and added a few basic tests.

Comment by Stuart Halloway [ 10/Mar/17 11:22 AM ]

Could the Specize protocol be extended to IFn, reducing the iffiness?

Comment by Alex Miller [ 10/Mar/17 11:39 AM ]

Yes, I think that would make sense.

Comment by Ghadi Shayban [ 10/Mar/17 12:57 PM ]

Extending Specize to IFn may incur the wrath of CLJ-1152

Comment by Alex Miller [ 10/May/17 12:07 PM ]

Updated patch to apply to spec.alpha instead.





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

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

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

Attachments: Text File tcrawley.CLJ-2066.2017-03-16.patch     Text File tcrawley.CLJ-2066.2017-09-07.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Due to changes in reflective access for the Jigsaw module system in Java 9, the Reflector will now fail on some cases that worked in previous Java versions.

(def fac (javax.xml.stream.XMLInputFactory/newInstance))
(.createXMLStreamReader fac (java.io.StringReader. ""))

Here fac will be an instance of com.sun.xml.internal.stream.XMLInputFactoryImpl, which is an implementation of javax.xml.stream.XMLInputFactory. In the new java.xml module, javax.xml.stream is an exported package, but the XMLInputFactoryImpl is an internal implementation of the public interface in that package. The invocation of createXMLStreamReader will be reflective and the Reflector will attempt to invoke the method based on the implementation class, which is not accessible outside the module, yielding:

IllegalAccessException class clojure.lang.Reflector cannot access class com.sun.xml.internal.stream.XMLInputFactoryImpl (in module java.xml) because module java.xml does not export com.sun.xml.internal.stream to unnamed module @4722ef0c
	jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:423)
	jdk.internal.reflect.Reflection.throwIllegalAccessException (Reflection.java:414)
	jdk.internal.reflect.Reflection.ensureMemberAccess (Reflection.java:112)
	java.lang.reflect.AccessibleObject.slowCheckMemberAccess (AccessibleObject.java:632)
	java.lang.reflect.AccessibleObject.checkAccess (AccessibleObject.java:624)
	java.lang.reflect.Method.invoke (Method.java:539)
	clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:93)
	clojure.lang.Reflector.invokeInstanceMethod (Reflector.java:28)

One workaround here is to avoid the reflective call by type-hinting to the public exported interface:

(.createXMLStreamReader ^javax.xml.stream.XMLInputFactory fac (java.io.StringReader. ""))

Another (undesirable) workaround is to export the private package from java.xml to the unnamed module (which is the module used when code is loaded from the classpath rather than from a module) 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 ...

Proposed: The attached patch will check for and catch IllegalAccessException in the Reflector. When it occurs, the Reflector will attempt to invoke the method on all super-classes and super-interfaces in case one of them can succeed.

Patch: tcrawley.CLJ-2066.2017-09-07.patch

More info:



 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

Comment by Toby Crawley [ 14/Feb/17 3:48 PM ]

I don't see any indication that Java 9 will change in our favor, so I'm attaching a patch to fix this in Clojure itself.

Comment by Alex Miller [ 07/Mar/17 4:04 PM ]

Screening comments:

I think by just inspecting the patch that the new code will attempt to invoke the method on all super classes and interfaces, many of which will not have the applicable method and will thus throw IllegalArgumentException, possibly before finding the correct one. Can we reduce the effort here by only attempting the call on the super types that actually have the method? It would be good to expand the tests to also include this case if possible if I'm just mis-reading things (I did not attempt to construct this scenario).

Also, is there a method we can call to determine whether the method is accessible before invoking and needing to catch IllegalAccessException? (Obviously, we would want something that is not new in Java 9 so that this call worked on older JDKs too.)

Comment by Herwig Hochleitner [ 07/Mar/17 5:37 PM ]

This may be a stupid question, but has there been any research, whether [1] and [2] could be utilized to handle this without catching exceptions?

[1] http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule--
[2] http://download.java.net/java/jdk9/docs/api/java/lang/reflect/Module.html#isExported-java.lang.String-java.lang.reflect.Module-

Comment by Alex Miller [ 07/Mar/17 6:27 PM ]

Nope, that's the kind of thing I was asking about in my comment. However, those are jdk 9 only methods and right now Clojure supports jdk 6+. That's not impossible to deal with but does increase the difficulty.

Comment by Toby Crawley [ 08/Mar/17 2:30 PM ]

Alex: you are correct, an intermediate ancestor that does not provide the method will cause the lookup to fail - I'll rework and provide a new patch with better tests.

Herwig: we could build Clojure as a multi-release jar in order to have code that can use Java 9 features, but that would definitely complicate the build. However, there may be other Java 9 issues that may require us to take that option (I'm investigating one potential issue now).

Comment by Alex Miller [ 08/Mar/17 3:28 PM ]

Yeah, I was looking at the multi release jar stuff yesterday. I would really like to avoid that if at all possible as it adds a bunch of work for build, ci, and test.

Comment by Toby Crawley [ 13/Mar/17 9:42 PM ]

I've attached a new patch (tcrawley.CLJ-2066.2017-03-13.patch) and removed the old one (tcrawley.CLJ-2066.2017-02-14.patch). The new patch checks for any matching methods before calling invokeMatchingMethod; this will prevent the lack of the method on an ancestor from throwing IllegalArgumentException and aborting the lookup process.

I wasn't able to provide a test for the case where the method was missing on an ancestor, since that requires trying to invoke the method through an interface that doesn't provide it (all subclasses will provide methods from parent classes, so you need an ancestor tree that pulls in an interface), and I couldn't find anything in Java's stdlib that did that and was a non-exposed class. To implement a proper test for this, I believe we'd need to construct a module ourselves, which would require changes to the build process to build that module (and run the test) only under Java 9. I did, however, confirm locally that the old code was broken in this regard, and that the new patch fixes it.

Comment by Toby Crawley [ 14/Mar/17 12:00 PM ]

After reviewing tcrawley.CLJ-2066.2017-03-13.patch, I can see a new issue with it: if the method being invoked exists only on the non-public class, then the patch will throw an IllegalArgumentException claiming that the method doesn't exist instead of the original, more accurate IllegalAccessException. I'll rework and provide another patch.

Comment by Toby Crawley [ 16/Mar/17 9:22 AM ]

New patch attached (tcrawley.CLJ-2066.2017-03-16.patch) that will properly throw the original IllegalAccessException when the method only exists on the non-public class. Patch tcrawley.CLJ-2066.2017-03-13.patch has been deleted.

Comment by Ghadi Shayban [ 05/Apr/17 6:56 PM ]

In JDK9 public classes housed in a non-exported package are now inaccessible. Previously public classes might not be public any more. Clojure should link to them neither non-reflectively nor reflectively – perhaps a slightly larger adjustment to the compiler/reflector method lookup?

Comment by Herwig Hochleitner [ 06/Apr/17 8:07 AM ]

Ghadi: does that mean, that we can't even get at the Class object to call http://download.java.net/java/jdk9/docs/api/java/lang/Class.html#getModule-- ?

Comment by Alex Miller [ 06/Apr/17 9:07 AM ]

My understanding (which may be faulty) is that the new module system is independent from the classloader setup (although the base classloader hierarchy has changed a bit) and that reflection is still largely the same with respect to examining classes and methods. But you may run into issues with invoking constructors or methods that are not allowed according to access restrictions. I'm not sure if the ability to load classes is affected (but I didn't think so).

Comment by Alex Miller [ 30/Aug/17 3:08 PM ]

Toby Crawley comments on the current patch:

  • make ancestors() and isAccessException() private
  • ancestors() is recursive and could blow the stack. should be iterative instead.
  • in invokeInstanceMethod/invokeNoArgInstanceMember, couldn't you have two catch blocks there, one for IllegalAccessException and one for Exception? Or does the type checker not like that?
  • seems like the logic in invokeInstanceMethod/invokeNoArgInstanceMember could get pushed into invokeMatchingMethod rather than have two copies?
Comment by Toby Crawley [ 07/Sep/17 7:52 AM ]

I've added a new patch (tcrawley.CLJ-2066.2017-09-07.patch), and left the old patch (tcrawley.CLJ-2066.2017-03-16.patch) attached for comparison.

make ancestors() and isAccessException() private

Done.

ancestors() is recursive and could blow the stack. should be iterative instead.

Done.

in invokeInstanceMethod/invokeNoArgInstanceMember, couldn't you have two catch blocks there, one for IllegalAccessException and one for Exception? Or does the type checker not like that?

Since the methods we're calling use Util.sneakyThrow(), and don't declare that they throw any exceptions, the compiler won't let us catch IllegalAccessException there. That's also why I had to add isAccessException().

seems like the logic in invokeInstanceMethod/invokeNoArgInstanceMember could get pushed into invokeMatchingMethod rather than have two copies?

I considered that, but I use invokeMatchingMethod inside invokeAncestorMethod, and was concerned about complicating invokeMatchingMethod further with managing the current state to know if it should call invokeAncestorMethod or not (since it shouldn't if it's currently within the stack of invokeAncestorMethod). Instead, I've moved some of the logic into maybeInvokeAncestorMethod (renamed from invokeAncestorMethod) to simplify invokeInstanceMethod/invokeNoArgInstanceMember a bit. Let me know if you want me to take another pass at that.

Interesting side note - the build output under Java 9 now gives the output that users will see when reflecting:

     [java] Testing clojure.test-clojure.compilation
     [java] WARNING: An illegal reflective access operation has occurred
     [java] WARNING: Illegal reflective access by clojure.lang.Reflector (file:/Users/tcrawley/oss/clojure/clojure/target/classes/) to method com.sun.xml.internal.stream.XMLInputFactoryImpl.getXMLReporter()
     [java] WARNING: Please consider reporting this to the maintainers of clojure.lang.Reflector
     [java] WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
     [java] WARNING: All illegal access operations will be denied in a future release
Comment by Toby Crawley [ 28/Sep/17 9:30 AM ]

Alex Miller any chance we can get this into 1.9? Let me know if there is anything I can do to help that happen.

Comment by Alex Miller [ 28/Sep/17 12:07 PM ]

Still hoping to!





[CLJ-2060] Add support for undefining a spec Created: 16/Nov/16  Updated: 30/Jun/17

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: Alex Miller
Resolution: Unresolved Votes: 1
Labels: spec

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

 Description   

Currently there is no 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: Make s/def take nil as a way to "clear" a spec:

user=> (s/def ::a string?)
:user/a
user=> (s/def ::a nil)
:user/a
user=> (s/get-spec ::a)
nil

Patch: clj-2060-4.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.

Comment by Alex Miller [ 10/May/17 1:03 PM ]

Updated patch to apply to spec.alpha

Comment by Alex Miller [ 29/Jun/17 4:25 PM ]

Update s/def docstring as well in -4 patch.





[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: 14/Dec/16

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

Type: Enhancement Priority: Major
Reporter: Dimitrios Piliouras Assignee: Unassigned
Resolution: Unresolved Votes: 2
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-2046] generate random subsets of or'd required keys in map specs Created: 17/Oct/16  Updated: 10/May/17

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: 2
Labels: generator, spec

Attachments: Text File clj-2046-2.patch     Text File clj-2046-3.patch     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: clj-2046-3.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 07/Apr/17 3:59 PM ]

Patch updated to apply to current master, no semantic changes, attribution retained.

Comment by Alex Miller [ 10/May/17 12:18 PM ]

Updated patch to apply to spec.alpha, no other changes, attribution retained.





[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-2040] Allow runtime modification of REPL exception handling Created: 11/Oct/16  Updated: 15/May/17

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

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

Attachments: Text File CLJ-2040-dynamic-repl-exceptions.patch    
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.



 Comments   
Comment by Alex Miller [ 15/Feb/17 9:39 AM ]

On the repl-caught var, it would be good to mention the signature of the handler, namely that it takes an exception, is expected to print or otherwise handle the exception, and that its return will be ignored.

Rather than the changes in repl, what if you added a dynamic-repl-caught and change that to be the default caught handler in repl? Or even just changed repl-caught itself.





[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: 2
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-2031] clojure.walk/postwalk does not preserve MapEntry type objects Created: 01/Oct/16  Updated: 27/Jul/17

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: 3
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.

Comment by Sean Corfield [ 27/Jul/17 3:21 PM ]

This keeps coming up on Slack – given there's a patch and this is prescreened, can it just get fixed as part of the next Clojure 1.9.0 Alpha/Beta build?

Comment by Alex Miller [ 27/Jul/17 3:39 PM ]

It's in a list for Rich to look at.





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

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-2.patch     Text File clj-2026.patch    
Patch: Code
Approval: Screened

 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-2.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.

Comment by Alex Miller [ 10/May/17 12:52 PM ]

Updated patch to apply to spec.alpha





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

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

Comment by Leon Grapenthin [ 28/Jan/17 4:49 PM ]

This can quickly be traced down to :clojure.core.specs/arg-list which is speced as a (s/and <regex> vector?). When unforming, it doesn't create a vector.

Thinking about it, a vcat would be nice for this and similar cases.





[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: 15/May/17

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

Type: Feature 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] Alternative s/cat options not error-reported Created: 24/Aug/16  Updated: 04/Oct/17

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:

alpha14


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

 Description   

This problem was detected in context of this discussion https://groups.google.com/d/msg/clojure/mIlKaOiujlo/tF71zZ2BCwAJ

A minimal version of how specs error reporting failed the users intuition there looks like this:

He used an invalid ns form

(ns foo (require [clojure.spec :as s])) ; should be :require

The error reported by spec:

In: [1] val: ((require [clojure.spec :as s])) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

While the error is technically true, it doesn't show the user /how/ each of the alternative options of the reported s/cat failed.

To get a better understanding why the users data is not correct, he should know precisely what spec tried and how it failed.

A good example of how this works is s/alt, where all failing alternatives are always reported to the user.

The problem has been investigated, first experimentally, then in specs code. Finally, a patch that brings error reporting like s/alts comes attached.

It has been observed that specs error reporting behavior for cat with optional branches is the following:

1. If the cat failed after one or many optional branches, the entire cat is reported as failing
2. If the cat failed after one or many optional branches /and/ a subsequent required branch, only the subsequent required branch is reported with no remarks to the alternative optional branches.

Rule 1 explains the ns example.
Rule 2 can fail the users intuition significantly worse:

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

gives

In: [0] val: "3" fails at: [:keyword] predicate: keyword?

The report clearly doesn't address the users intent of putting in a number. Instead he is made to believe that he should have entered a keyword.

Solution:

A simple patch has been programmed that changes op-explain to have the following behaviour:

  • All alternatives that have been tried in a s/cat are reported individually.

It improves the reported errors significantly because it makes clearly transparent how the users data failed the validation.

(ns foo (require [clojure.spec :as s])) ; should be :require

now gives

ExceptionInfo Call to clojure.core/ns did not conform to spec:
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :docstring] predicate: string?
In: [1] val: (require [clojure.spec :as s]) fails at: [:args :attr-map] predicate: map?
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer-clojure at: [:args :clauses :refer-clojure :clause] predicate: #{:refer-clojure}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-require at: [:args :clauses :require :clause] predicate: #{:require}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-import at: [:args :clauses :import :clause] predicate: #{:import}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-use at: [:args :clauses :use :clause] predicate: #{:use}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-refer at: [:args :clauses :refer :clause] predicate: #{:refer}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-load at: [:args :clauses :load :clause] predicate: #{:load}
In: [1 0] val: require fails spec: :clojure.core.specs/ns-gen-class at: [:args :clauses :gen-class :clause] predicate: #{:gen-class}
:clojure.spec/args  (foo (require [clojure.spec :as s]))
  clojure.core/ex-info (core.clj:4725)

It would be even better if explain-data sorted ::s/problems by length of their :path which would push the first two unintended options to the end.

(s/explain (s/cat :maybe-num (s/? number?)
                  :keyword keyword?)
           ["3"])

now gives

In: [0] val: "3" fails at: [:maybe-num] predicate: number?
In: [0] val: "3" fails at: [:keyword] predicate: keyword?

While examples can be made up where this reporting produces more noise (like defn) I believe its the right tradeoff for aforementioned reasons and:

  • We programmers always ask our users for the most specific information when something went wrong - It is correct to apply the same to specs error reporting
  • Custom error reporters (s/explain-out) get more data to generate narrow reports matching the users intent even better


 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,
...
Comment by Leon Grapenthin [ 29/Sep/17 9:07 AM ]

@alexmiller: How about deciding this before releasing 1.9. since it improves error messages in a major way. Since after the original discussion on the groups there have been more reports of unintuitive error messages that this would fix. Related to this is sorting explanations by length of path, as illustrated above (for which there is no ticket yet).
Please get some input from Rich.

Comment by Ghadi Shayban [ 29/Sep/17 9:15 AM ]

Work is ongoing with spec, and is happening in a separate git repo unimpacted by the Clojure 1.9 release. We'll be able to update the spec dependency independent of Clojure and receive an eventual fix.

Comment by Leon Grapenthin [ 29/Sep/17 10:18 AM ]

This affects compile time error messages of Clojure, not just the opt-in part of spec. This part of compiler error reporting is broken and shouldn't be shipped with a major release. In that regard it really doesn't matter whether spec is a separate dep or not.

Also, users who wait for a stable version of spec won't necessarily update their spec dependency.

Comment by Leon Grapenthin [ 30/Sep/17 11:36 AM ]

Also AFAIK better error reporting is the sole reason that Spec is a dependency of 1.9. - please reconsider.

Comment by Alex Miller [ 04/Oct/17 9:01 AM ]

I don't know whether this will be considered before or after 1.9. Seems reasonable based on what I read.

The sorted by path length does have a ticket at CLJ-2063, which was applied in May and released in spec.alpha 0.1.109.





[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-2003] Nesting cat inside ? causes unform to return nested result Created: 11/Aug/16  Updated: 30/Aug/17

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

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

Attachments: Text File CLJ-2003-corrected.patch     Text File CLJ-2003.patch    
Patch: Code and Test
Approval: Vetted

 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)

Patch: CLJ-2003-corrected.patch



 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)))

Comment by Tyler Tallman [ 09/Aug/17 9:41 PM ]

This seems to be all that is needed.

Comment by Francis Avila [ 28/Aug/17 9:24 PM ]

The problem is actually more universal than (? (cat ...)). s/unform of a s/? with any regex child op will introduce an extra level of nesting. When the child is a regex, we are consuming the same "level" of sequence so unform should not introduce an extra level. However in other cases (non-regex ops), we should still possibly produce a nested collection.

The previous patch was too aggressive: it unwrapped all sub-unforms of s/?. This patch CLJ-2003-corrected.patch only unwraps when the sub-op is a regex.

Unfortunately it is impossible to distinguish between a desired-but-optional nil and a non-match from s/?. Specifically, the following tests now hold:

(testing "s/? matching nil"
  (is (nil? (s/conform (s/? nil?) [nil])))
  (is (nil? (s/conform (s/? nil?) [])))
  (is (nil? (s/conform (s/? nil?) nil)))
  (is (= (s/unform (s/? nil?) nil) [])))

(I did not add these tests to the patch because I was unsure if they should be part of the contract of unform. However, they are pretty big gotchas.)

I also added tests for every possible subop of s/?, except ::s/accept, which I could not think of a test case for. (I'm not sure ::s/accept is actually reachable inside s/op-unform?)

Comment by Alex Miller [ 29/Aug/17 7:33 AM ]

Thanks for working on this - I will take a look when I get a chance.

Comment by Francis Avila [ 29/Aug/17 9:20 AM ]

I had to amend my patch slightly (same name): one of the test cases wasn't testing the correct thing.





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