<< Back to previous view

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

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

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

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


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

 Description   

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

First, while checking this spec:

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

This exception was raised:

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

Second, while checking this spec:

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

This exception was thrown:

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

I was unable to reproduce either exception during consequent runs.

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

Approach: Add locking to prevent concurrent loads in dynaload.

Patch: clj-2026.patch



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

On the first one, you should use this instead:

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

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

I'll look into the failures though.

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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

Proposals:

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

Patch:

  • clj-2030.patch does 1 (but not 2 or 3)
  • clj-2030-2.patch does 1+2 (but not 3)

Before:

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

After:

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


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

From original description:

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

I would like to support the syntax:

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

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

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

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

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

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

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

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

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





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

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

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

Attachments: Text File clj-1714-4.patch     Text File CLJ-1714.patch     Text File CLJ-1714-v2.patch     Text File CLJ-1714-v3.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

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

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

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

Approach: Don't cause class to load merely from being in a type hint.

Patch: clj-1714-4.patch

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

Prescreened: Alex Miller



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

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

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

Patch won't apply to master for me

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

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

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

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

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

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

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

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

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

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

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

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

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

Hi Adam,

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

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

Added failing tests which now pass

Comment by Michael Blume [ 28/Sep/16 2:07 PM ]

Updated patch to apply to master

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

Added new patch that is semantically identical, just easier to read (with formatted using -U8). Attribution retained.





[CLJ-1613] :or defaults should refer to enclosing scope in map destructuring Created: 12/Dec/14  Updated: 28/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 3
Labels: destructuring

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

 Description   

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

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

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

Updating this patch

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

@Stuart:

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

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

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

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

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

Comment by Michael Blume [ 28/Sep/16 2:39 PM ]

Updated patch to apply to master





[CLJ-1523] Add 'doseq' like macro for transducers Created: 08/Sep/14  Updated: 28/Sep/16

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

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

Attachments: Text File CLJ-1523-1.patch     File doreduced2.diff     File doreduced.diff    
Patch: Code and Test
Approval: Triaged

 Description   

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

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

Patch: CLJ-1523-1.patch



 Comments   
Comment by Jozef Wagner [ 09/Sep/14 4:19 AM ]

How about making xform parameter optional? And you have a typo in docstring example, doseq -> doreduced.

Comment by Timothy Baldridge [ 09/Sep/14 7:52 AM ]

Good point, fixed typeo, added other arity.

Comment by Ghadi Shayban [ 29/Jun/16 9:03 PM ]

perhaps another arity on `run!`

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

Fixed patch to apply to master

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

I really don't like how this looks like bindings, except there's a transducer stuck in it. That seems unlikely to fly from a syntax perspective.





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

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

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

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

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-7.patch

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

The attached patch preserves two existing behaviors of merge

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

Screened by:



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

I will take a crack at a patch today.

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

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

Three potential issues:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

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

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

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

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

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

This calls for if-let + find.

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

new patch addressing concerns so far

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

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

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

Nice =)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1656] Unroll assoc and assoc! for small numbers of arguments Created: 06/Feb/15  Updated: 28/Sep/16

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

Type: Enhancement Priority: Major
Reporter: Tom Crayford Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: performance

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

 Description   

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

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

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

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

benchmark results:

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

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

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

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

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

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

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

Patch: CLJ-1656-v5.patch



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

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

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

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

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

Patch v2 includes assoc!

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

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

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

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

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

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

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

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

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

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

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

which reminds me that this patch needs tests.

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

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

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

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

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

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

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

v5 marks the helper macros private.

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

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

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

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

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

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

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

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

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

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

Yes, inlining works through var invocation.

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

Michael,

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

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

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

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

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

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

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

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

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

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

There, code and test.

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

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

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

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

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

Comment by Michael Blume [ 28/Sep/16 2:14 PM ]

Updated patch to apply to master





[CLJ-2029] (nth nil <anything>) does not throw an exception Created: 28/Sep/16  Updated: 28/Sep/16  Resolved: 28/Sep/16

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

Type: Defect Priority: Minor
Reporter: Hans Hübner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

If `nth` is being passed `nil` as `coll` argument, no exception is thrown for arbitrary indices. This does not match the expected behavior (all indices should throw an "Index out of bounds" exception) and also not documented.



 Comments   
Comment by Alex Miller [ 28/Sep/16 9:21 AM ]

It is by design that nth works on nil to return nil for any index and changing this would likely break many existing programs. An enhancement for the docstring would be considered.





[CLJ-1243] Cannot resolve public generic method from package-private base class Created: 01/Aug/13  Updated: 27/Sep/16

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: interop

Attachments: GZip Archive clj-1243-demo1.tar.gz     Text File invocation_target_selection.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The Clojure compiler cannot resolve a public generic method inherited from a package-private base class.

Instructions to reproduce:

  • In package P1
    • Define a package-private class A with generic type parameters
    • Define a public method M in A using generic types in either its arguments or return value
    • Define a public class B which extends A
  • In package P2
    • Construct an instance of B
    • Invoke B.M()

This is valid in Java. In Clojure, invoking B.M produces a reflection warning, followed by the error "java.lang.IllegalArgumentException: Can't call public method of non-public class." No amount of type-hinting prevents the warning or the error.

Attachment clj-1243-demo1.tar.gz contains sample code and script to demonstrate the problem.

Examples of Java projects which use public methods in package-private classes:



 Comments   
Comment by Stuart Sierra [ 01/Aug/13 5:11 PM ]

It is also not possible to call the method reflectively from Java.

This may be a bug in Java reflection: JDK-4283544

But why does it only happen on generic methods?

Comment by Stuart Sierra [ 08/Aug/13 11:59 AM ]

According to Rich Hickey, the presence of bridge methods is unspecified and inconsistent across JDK versions.

A possible solution is to use ASM to examine the bytecode of third-party Java classes, instead of the reflection API. That way the Clojure compiler would have access to the same information as the Java compiler.

Comment by Andy Fingerhut [ 17/Nov/13 11:01 PM ]

CLJ-1183 was closed as a duplicate of this one. Mentioning it here in case anyone working on this ticket wants to follow the link to it and read discussion or test cases described there.

Comment by Noam Ben Ari [ 21/Feb/15 4:55 AM ]

The current work around I use is to define a new Java class, add a static method that does what I need, and call that from Clojure.

Comment by Noam Ben Ari [ 21/Feb/15 9:28 AM ]

Also, I'm seeing this issue in 1.6 and 1.7(alpha5) but the issue mentions only up to 1.5 .

Comment by Adam Tait [ 03/Apr/16 5:32 PM ]

Just ran into this issue trying to use Google's Cloud APIs.
To use Google's Cloud Datastore, you need to access the .kind method on a protected generic subclass (BaseKey), to which KeyFactory extends.

Tested on both Clojure 1.7 & 1.8 at runtime, the following exception persists;

IllegalArgumentException Can't call public method of non-public class: public com.google.gcloud.datastore.BaseKey$Builder com.google.gcloud.datastore.BaseKey$Builder.kind(java.lang.String) clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

Comment by Kai Strempel [ 18/Jun/16 1:19 PM ]

I ran into the exact same issue with Google's Cloud API's.

Tested it with 1.8 and with 1.9.0-alpha7. Same Problem.

Comment by Kai Strempel [ 18/Jun/16 1:19 PM ]

I ran into the exact same issue with Google's Cloud API's.

Tested it with 1.8 and with 1.9.0-alpha7. Same Problem.

Comment by Michal Růžička [ 23/Sep/16 1:08 PM ]

I ran into the same issue. The attached patch fixes the problem for me.
All tests in the project still pass, but this desperately needs a review of someone knowledgeable.

Comment by Alex Miller [ 23/Sep/16 3:00 PM ]

Hey Michal,

Thanks for looking at it.

1. Please follow the instructions on how to create a patch in the proper format here: http://dev.clojure.org/display/community/Developing+Patches
2. If you can provide some explanation of the changes to aid in review that would be most helpful. Otherwise screeners have to re-engineer your thought processes from scratch.
3. Before getting screened, this change will also need some tests (admittedly not particularly fun to write, but I think it's necessary here)

Comment by Michal Růžička [ 27/Sep/16 8:56 AM ]

I've added tests and updated the patch according to the instructions.

Here is some reasoning behind it. Below is an excerpt from the src/jvm/clojure/lang/Compiler.java file:

src/jvm/clojure/lang/Compiler.java
1462:	if(target.hasJavaClass() && target.getJavaClass() != null)
1463:		{
1464:		List methods = Reflector.getMethods(target.getJavaClass(), args.count(), methodName, false);
1465:		if(methods.isEmpty())
1466:			{
1467:			method = null;
1468:			if(RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
1469:				{
1470:				RT.errPrintWriter()
1471:					.format("Reflection warning, %s:%d:%d - call to method %s on %s can't be resolved (no such method).\n",
1472:						SOURCE_PATH.deref(), line, column, methodName, target.getJavaClass().getName());
1473:				}
1474:			}
1475:		else
1476:			{
1477:			int methodidx = 0;
1478:			if(methods.size() > 1)
1479:				{
1480:				ArrayList<Class[]> params = new ArrayList();
1481:				ArrayList<Class> rets = new ArrayList();
1482:				for(int i = 0; i < methods.size(); i++)
1483:					{
1484:					java.lang.reflect.Method m = (java.lang.reflect.Method) methods.get(i);
1485:					params.add(m.getParameterTypes());
1486:					rets.add(m.getReturnType());
1487:					}
1488:				methodidx = getMatchingParams(methodName, params, args, rets);
1489:				}
1490:			java.lang.reflect.Method m =
1491:					(java.lang.reflect.Method) (methodidx >= 0 ? methods.get(methodidx) : null);
1492:			if(m != null && !Modifier.isPublic(m.getDeclaringClass().getModifiers()))
1493:				{
1494:				//public method of non-public class, try to find a public descendant
1495:				if((type=Reflector.getDeepestPublicDescendant(m.getDeclaringClass(), target.getJavaClass())) == null)
1496:					//if descendant not found, try to find an ancestor
1497:					m = Reflector.getAsMethodOfPublicBase(m.getDeclaringClass(), m);
1498:				}
1499:			method = m;
1500:			if(method == null && RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
1501:				{
1502:				RT.errPrintWriter()
1503:					.format("Reflection warning, %s:%d:%d - call to method %s on %s can't be resolved (argument types: %s).\n",
1504:						SOURCE_PATH.deref(), line, column, methodName, target.getJavaClass().getName(), getTypeStringForArgs(args));
1505:				}
1506:			}
1507:		}
  • the condition on line 1462 ensures that the type/class of the target is known
  • the clojure.lang.Reflector.getMethods() method called on line 1464 returns a list of all public methods of the given name defined for the target type
  • then the best method to call is selected on lines 1477-1491
  • if the declaring class of the selected method is not public then an attempt is made to find a public class which is both superclass of the target type and a subclass of the class declaring the selected method - this is implemented in the clojure.lang.Reflector.getDeepestPublicDescendant() method
  • if such a class is found than it is used instead of the method's declaring class when emitting the byte code for the method call
  • if no such class is found then an attempt is made to find a compatible method in the public ancestors of the class declaring the selected method

Note that the change may result in a different method being called than prior to the change as demonstrated by the selecting-method-on-nonpublic-interface test. This is IMO an acceptable change as it:

  • results in better matching (with respect to the argument types) method to be called
  • makes the method selection in clojure behave in a more similar way to that in java




[CLJ-2028] Docstring error in clojure.core/filter, remove, and take-while Created: 26/Sep/16  Updated: 26/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: Alan Thompson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring
Environment:

All


Approval: Triaged

 Description   

The docstring for filter could be clearer about responding to logical true values:

​​Returns a lazy sequence of the items in coll for which
(pred item) returns true. pred must be free of side-effects.
Returns a transducer when no collection is provided.

should be corrected to read:

​Returns a lazy sequence of the items in coll for which
(pred item)​ ​​​returns logical true​. pred must be free of side-effects.​
Returns a transducer when  o collection is provided.

Similar changes could be applied to remove and take-while.



 Comments   
Comment by Alex Miller [ 26/Sep/16 12:49 PM ]

"logical true" is the phrase used for this in other docstrings.





[CLJ-2027] bean printing regression from namespace map printing Created: 24/Sep/16  Updated: 26/Sep/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: Trey Sullivan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print, regression
Environment:

Clojure 1.9.0-alpha12


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

 Description   

The new namespace map printing is causing a failure in printing bean maps (which are proxies that don't support empty):

user=> (bean (java.util.Date.))
UnsupportedOperationException empty  clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.empty (:-1)

user=> (pst *e)
UnsupportedOperationException empty
	clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.empty (:-1)
	clojure.core/empty (core.clj:5151)
	clojure.core/lift-ns (core_print.clj:237)

Cause: The internal lift-ns function calls empty on the map too early (here it doesn't need to call it at all).

Proposed: Defer calling (empty m) until we know map has namespace keywords and namespace maps will be used for printing.

Patch: clj-2027.patch (note that into is not used in the change b/c it has not yet been defined at this point)






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

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

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

1.9.0-alpha11


Approval: Triaged

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

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






[CLJ-1910] Namespaced maps Created: 07/Apr/16  Updated: 23/Sep/16  Resolved: 23/Jun/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 1
Labels: print, reader

Attachments: Text File clj-1910-2.patch     Text File clj-1910.patch    
Patch: Code and Test
Approval: Ok

 Description   

A common usage of namespaced keywords and symbols is in providing attribute disambiguation in map contexts:

{:person/first "Han" :person/last "Solo" :person/ship 
  {:ship/name "Millenium Falcon" :ship/model "YT-1300f light freighter"}}

The namespaces provide value (disambiguation) but have the downside of being repetitive and verbose.

Namespaced maps are a reader (and printer) feature to specify a namespace context for a map.

  • Namespaced maps combine a default namespace with a map and yield a map.
  • Namespaced maps are reader macros starting with #: or #::, followed by a normal map definition.
    • #:sym indicates that sym is the default namespace for the map to follow.
    • #:: indicates that the default namespace auto-resolves to the current namespace.
    • #::sym indicates that sym should be auto-resolved using the current namespace's aliases OR any fully-qualified loaded namespace.
      • These rules match the rules for auto-resolved keywords.
  • A namespaced map is read with the following differences from normal maps:
    • A keyword or symbol key without a namespace is read with the default namespace as its namespace.
    • Keys that are not symbols or keywords are not affected.
    • Keys that specify an explicit namespace are not affected EXCEPT the special namespace _, which is read with NO namespace. This allows the specification of bare keys in a namespaced map.
    • Values are not affected.
    • Nested map keys are not affected.
  • The edn reader supports #: but not #:: with the same rules as above.
  • Maps will be printed in namespaced map form only when:
    • All map keys are keywords or symbols
    • All map keys are namespaced
    • All map keys have the same namespace

Examples:

;; same as above - notice you can nest #: maps and this is a case where the printer roundtrips
user=> #:person{:first "Han" :last "Solo" :ship #:ship{:name "Millenium Falcon" :model "YT-1300f light freighter"}}
#:person{:first "Han" :last "Solo" :ship #:ship{:name "Millenium Falcon" :model "YT-1300f light freighter"}}

;; effects on keywords with ns, without ns, with _ ns, and non-kw
user=> #:foo{:kw 1, :n/kw 2, :_/bare 3, 0 4}
{:foo/kw 1, :n/kw 2, :bare 3, 0 4}

;; auto-resolved namespaces (will use user as the ns)
user=> #::{:kw 1, :n/kw 2, :_/bare 3, 0 4}
:user/kw 1, :n/kw 2, :bare 3, 0 4}

;; auto-resolve alias s to clojure.string
user=> (require '[clojure.string :as s])
nil
user=> #::s{:kw 1, :n/kw 2, :_/bare 3, 0 4}
{:clojure.string/kw 1, :n/kw 2, :bare 3, 0 4}

;; to show symbol changes, we'll quote the whole thing to avoid evaluation
user=> '#::{a 1, n/b 2, _/c 3}
{user/a 1, n/b 2, c 3}

;; edn reader also supports (only) the #: syntax
user=> (clojure.edn/read-string "#:person{:first \"Han\" :last \"Solo\" :ship #:ship{:name \"Millenium Falcon\" :model \"YT-1300f light freighter\"}}")
#:person{:first "Han", :last "Solo", :ship #:ship{:name "Millenium Falcon", :model "YT-1300f light freighter"}}

Patch: clj-1910-2.patch

Screener notes:

  • Autoresolution supports fully-qualified loaded namespaces (like auto-resolved keywords)
  • TODO: pprint support for namespaced maps
  • TODO: printer flag to suppress printing namespaced maps


 Comments   
Comment by Nicola Mometto [ 08/Apr/16 3:57 AM ]

1- yes please. that's consistent with how tagged literals work.
2- no please. that would make the proposed syntax useless for e.g. Datomic schemas, for which I think this would be a good fit to reduce noise
3- yes please
4- yes please, consistency over print methods is important

Comment by Nicola Mometto [ 08/Apr/16 4:00 AM ]

Quoting from a post I wrote on the clojure-dev ML:

  • I really don't like the idea of special-casing `_` here, users are already confused about idioms like `[.. & _]` thinking that `_` is some special token/variable. Making it an actual special token in some occasion wouldn't help.
  • I also don't like how we're making the single `:` auto-qualify keywords when used within the context of a qualifying-map. Auto-qualifying keywords has always been the job of the double `::`, changing this would introduce (IMO) needless cognitive overhead.
  • The current impl treats `#:foo{'bar 1}` and `'#:foo{bar 1}` differently. I can see why is that, but the difference might be highly unintuitive to some.
  • The current proposal makes it feel like quote is auto-qualifying symbols , when that has always been the job of syntax-quote. I know that's not correct, but that's how it's perceived.

Here's an alternative syntax proposal that handles all those issues:

  • No #::, only #:foo or #::foo
  • No auto-resolution of symbols when the namespaced-map is quoted, only when syntax-quoted
  • No special-casing of `_`
  • No auto-resolution of single-colon keywords

Here's how the examples in the ticket description would look:

#:person{::first "Han", ::last "Solo", ::ship #:ship{::name "Millenium Falcon", ::model "YT-1300f light freighter"}}
;=> {:person/first "Han" :person/last "Solo" :person/ship {:ship/name "Millenium Falcon" :ship/model "YT-1300f light freighter"}}

#:foo{::kw 1, :n/kw 2, :bare 3, 0 4}
;=> {:foo/kw 1, :n/kw 2, :bare 3, 0 4}

{::kw 1, :n/kw 2, bare 3, 0 4}
;=> {:user/kw 1, :n/kw 2, :bare 3, 0 4}

Note in the previous example how we don't need `#::` at all – `::` already does that job for us

(require '[clojure.string :as s])
#::s{::kw 1, :n/kw 2, bare 3, 0 4}
;=> {:clojure.string/kw 1, :n/kw 2, :bare 3, 0 4}

`{a 1, n/b 2, ~'c 3}
;=> {user/a 1, n/b 2, c 3}

Again, no need for `#::` here, we can just rely on the existing auto-qualifying behaviour of `.

`#:foo{a 1, n/b 2}
;=> {foo/a 1, n/b 2}

I think this would be more consistent with the existing behaviour – it's basically just making `#:foo` or `#::foo` mean: in the top-level keys of the following map expression, resolve keywords/symbols as if ns was bound to `foo`, rather than introducing new resolution rules and special tokens.

I realize that this proposal wouldn't work with EDNReader as-is, given its lack of support for `::` and "`". I don't have a solution to that other than "let's just bite the bullet and implement them there too", but maybe that's not acceptable.

Comment by Alex Miller [ 08/Apr/16 8:45 AM ]

Nicola, thanks for the proposal, we talked through it. We share your dislike for :_/kw syntax and you should consider that a placeholder for this behavior for the moment - it may be removed or replaced before we get to a published release.

For the rest of it:

  • requiring syntax quote is a non-starter
  • supporting a mixture of default ns and the current ns is important and this is not possible with your proposal. Like #:foo{:bar 1 ::baz 2}.
  • there is a lot of value to changing the scope of a map without modifying the contents, which is an advantage of the syntax in the ticket
Comment by Christophe Grand [ 08/Apr/16 10:31 AM ]

Why restrict this feature to a single namespace? (this doesn't preclude a shorthand for the single mapping) I'd like to locally define aliases (and default ns).

Comment by Alex Miller [ 08/Apr/16 11:02 AM ]

We already have namespace level aliases. You can use :: in the map to leverage those aliases (independently from the default ns):

(ns app 
  (:require [my.domain :as d]
            [your.domain :as y]))

#::{:svc 1, ::d/name 2, ::y/name 3}

;;=> {:app/svc 1, :my.domain/name 2, :your.domain/y 3}
Comment by Christophe Grand [ 11/Apr/16 4:03 AM ]

Alex, if existing namespace level aliases are enough when there's more than one namespace used in the key set I fail to understand the real value of this proposal.

Okay I'm lying a little: there are no aliases in edn, so this would bring aliases to edn (and allows printers to factor/alias namespaces out). And for Clojure code you can't define an alias to a non-existing namespace – and I believe that this implementation wouldn't check namespace existence when resolving the default ns #:person{:name}.

Still my points hold for edn (and that's where the value of this proposal seems to be): why not allows local aliases too?

#:person #:employee/e {:name "John Smith", :e/eid "012345"}
;=> {:person/name "John Smith", :employee/eid "012345"}

I have another couple of questions:

  • should it apply to other datatypes?
  • should it be transitive?
Comment by Alex Miller [ 28/Apr/16 1:33 PM ]

New patch rev supports spaces between the namespace part #:foo and the map in both LispReader and EdnReader.

Comment by Tim Gilbert [ 23/Sep/16 1:12 AM ]

Any news on the flag to prevent this behavior (mentioned in the screener notes)?

I ask because (pr-str) in Clojure 1.9.0-alpha12 currently produces EDN that can't be consumed by (cljs.reader/read-string) in ClojureScript 1.9.229 (the ClojureScript consumption side is tracked in CLJS-1706).

It would be nice to have a way to disable this behavior until ClojureScript reaches parity, and a lot of fairly widely-used open-source libraries use plain (pr-str) to generate EDN output - for example, ring-middleware-format.

Comment by Alex Miller [ 23/Sep/16 8:37 AM ]

Yes, that was implemented in a separate ticket (CLJ-1993) in Clojure 1.9.0-alpha11. There is now a print-namespace-maps dynvar. It defaults to false, but is set to true in the standard REPL.

So at the REPL you can (set! print-namespace-maps false) to turn it off.

If you're doing stuff in your program outside a REPL, it will be off by default so you probably don't need to do anything as of alpha11.

Comment by Alex Miller [ 23/Sep/16 8:38 AM ]

that's *print-namespace-maps* above, sorry for the formatting





[CLJ-2025] When a generator fails to gen, state which spec/pred failed Created: 21/Sep/16  Updated: 22/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: David Collie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   

Given a generator using such-that that fails to find a value, the error does not give enough information to determine which spec or predicate was at fault:

(require '[clojure.spec :as s])
(s/exercise (s/and string? #{"hi"}))
ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

Another special case of this is when providing a custom generator that produces a valid that doesn't satisfy the spec (Clojure adds this filter internally):

(require '[clojure.spec :as s])
(s/exercise (s/with-gen int? #(s/gen #{:a})))

Proposal: Indicate in the error which spec failed to generate and possibly the path in the overall spec if feasible.

(Note: original description moved to comment)



 Comments   
Comment by Alex Miller [ 22/Sep/16 3:54 PM ]

[Original description from ticket:]

I created a generator that did not conform to the spec (doh!). The generator contained the such-that predicate. When I tried creating a sample from the generator I got this error:

ExceptionInfo Couldn't satisfy such-that predicate after 100 tries.  clojure.core/ex-info (core.clj:4725)

I assumed that it referred to my custom generator but that was a red herring because in fact spec must be using such-that to ensure that the generated value conforms to the spec, and it was this such-that that generated the failure, not the one in my custom generator.

Code (with the problem corrected but showing the such-that in my generator:

(defn mod11-checkdigit
  "Calculate the checkdigit see http://freagra.com/imthealth/mitNNC.html"
  [n]
  (let [x (->> (map #(Integer/parseInt (str %)) (take 9 n))
               (map * (range 10 1 -1))
               (reduce +))
        y (mod x 11)
        c (- 11 y)]
    (cond (== 10 c) nil
          (== 11 c) 0
          :else c)))

(def nhs-number-gen
  "Generates a valid NHS number"
  (gen/fmap #(str (+ (* 10 %) (mod11-checkdigit (str %))))
            (gen/such-that #(mod11-checkdigit (str %))
                           (gen/choose 100000000 999999999))))

(defn nhs-number?
  "Returns true if passed a valid nhs number else returns false"
  [n]
  (and (string? n) (= 10 (count n)) (= (str (mod11-checkdigit n)) (str (last n)))))

(s/def ::nhs-number (s/with-gen nhs-number?
                                (fn [] nhs-number-gen)))

It would be nicer if the error thrown due to the generated value being non-conformant with the spec stated this.

Comment by Alex Miller [ 22/Sep/16 4:07 PM ]

I'm not sure that this is possible right now based on what we give to and get back from test.check.





[CLJ-2024] Check should specize function specs before checking Created: 19/Sep/16  Updated: 22/Sep/16

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

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

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

 Description   

This code works fine in 1.9.0-alpha12:

(defn f [x] (+ x 1))
(s/def f (s/fspec :args (s/cat :x number?) :ret number?))
(stest/check `f)

But if we factor the fspec out into its own keyword:

(defn f [x] (+ x 1))
(s/def ::inc (s/fspec :args (s/cat :x number?) :ret number?))
(s/def f ::inc)
(stest/check `f)

The check fails with the exception:

({:failure #error {
 :cause "No :args spec"
 :data #:clojure.spec{:failure :no-args-spec}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "No :args spec"
   :data #:clojure.spec{:failure :no-args-spec}
   :at [...]}]
 :trace
 [...]}, :sym user/f, :spec :user/inc})

The check function doesn't seem to be resolving ::inc, when presumably it should.






[CLJ-1242] = on sorted collections with different key types incorrectly throws Created: 31/Jul/13  Updated: 22/Sep/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: collections

Attachments: Text File 0001-fix-for-CLJ-1242-tests.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Comparing a sorted-set with numbers to a set with keywords is not symmetric:

user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword  clojure.lang.Keyword.compareTo (Keyword.java:109)

The latter case should return false instead of throwing.

Cause: APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check.

Proposed: It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.

See also: CLJ-1983 (downstream example with clojure.data/diff)



 Comments   
Comment by OHTA Shogo [ 31/Jul/13 8:02 PM ]

PersistentVector also has the same problem.

user=> (compare [1] [:a])
java.lang.ClassCastException: clojure.lang.Keyword cannot be cast to java.lang.Number

The cause of this problem is that Util.compare() casts the second argument
to Number without checking its type when the first argument is a Number.

Comment by OHTA Shogo [ 31/Jul/13 8:26 PM ]

Umm, my brain was not working right.
Util.compare() should raise an Exception when the arguments' type are different.

Comment by François Rey [ 02/May/15 4:44 PM ]

Upvoting.
Here's a instance of this bug in codox:
https://github.com/weavejester/codox/issues/91

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

The behavior of get is consistent with Java collections, so I think changing that expectation should be considered a feature request and not a bug.

The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers.

Comment by Alex Miller [ 21/Jan/16 10:33 AM ]

I re-focused this ticket on just the equality aspect. The other request regarding `get` with a value of a different type is consistent with Java behavior and should be considered "as designed" - a separate enhancement ticket could be considered for that one.

user=> (def s (java.util.TreeSet.))
#'user/s
user=> (.add s 1)
true
user=> (.contains s "a")
ClassCastException java.lang.Long cannot be cast to java.lang.String  java.lang.String.compareTo (String.java:108)
Comment by Alex Miller [ 19/Jul/16 2:00 PM ]

oops, sorry for the close/reopen.

Comment by Rich Hickey [ 19/Aug/16 10:47 AM ]

Stu - link for "The fix for equals should be informed by the approach taken in the JDK, where the implementation of equals (not get) has exception catchers." ?

Comment by Alex Miller [ 22/Sep/16 1:45 PM ]

I don't think this change is good in its current location - should reconsider alternative impl based on the proposed suggestion.





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 22/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Alex Miller
Resolution: Unresolved Votes: 2
Labels: collections, docstring, ft

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch     Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    
Patch: Code
Approval: Incomplete

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", possibly suggesting the transient edit happens (always) in-place, `coll` being the first argument. However, this is not the case and the returned collection should always be the one that's used.

Approach: Replace "Returns coll." with "Returns an updated collection." in `conj!`, `assoc!`, `pop!` docstrings.

Patch: CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Comment by Alex Miller [ 05/Apr/14 11:55 AM ]

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).

Comment by Andy Fingerhut [ 06/Aug/14 2:14 PM ]

Patch CLJ-1385-reword-docstrings-on-transient-update-funct.patch dated Apr 6 2014 no longer applies to latest Clojure master cleanly, due to some changes committed earlier today. I suspect it should be straightforward to update the patch to apply cleanly, given that they are doc string changes, but there may have been doc string changes committed to master, too.

Comment by Gary Fredericks [ 06/Aug/14 3:04 PM ]

Attached a new patch.

Comment by Rich Hickey [ 09/Oct/15 8:04 AM ]

I think it could be clearer still, since we want people to know the original coll might have been affected and returned, and the return must be used for subsequent calls. I think some of the language from the transients page should make it into these docstrings.

Comment by Andy Fingerhut [ 24/Oct/15 2:25 PM ]

Would it be correct to say that the collection passed into pop! conj! assoc! etc. has undefined contents after the operation completes, and only the return value has defined contents?

That kind of strong wording may get people's attention.

Comment by Alex Miller [ 24/Oct/15 9:07 PM ]

I'm working on this.

Comment by Alex Miller [ 22/Sep/16 1:42 PM ]

incomplete and I (still) own this one





[CLJ-1406] Libs are blindly added into loaded-libs even if an error occurs during loading Created: 17/Apr/14  Updated: 22/Sep/16

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

Type: Defect Priority: Major
Reporter: OHTA Shogo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-modify-clojure.core-load-lib-so-that-it-removes-the-.patch    
Patch: Code
Approval: Incomplete

 Description   

Suppose you have a lib that causes some errors during loading, like the following:

(ns broken-lib)

(} ; this line will cause a reader error

And then, if you require the lib, it would be added into loaded-libs in spite of the reader error, which makes require succeed silently after that.

user=> (contains? (loaded-libs) 'broken-lib)
false
user=> (require 'broken-lib)
CompilerException java.lang.RuntimeException: Unmatched delimiter: }, compiling:(broken_lib.clj:3:3) 
user=> (contains? (loaded-libs) 'broken-lib)
true
user=> (require 'broken-lib)
nil
user=>

Things get worse if you have another namespace that requires a broken lib (say here broken-lib.core):

(ns broken-lib.core
  (:require [broken-lib :as lib]))

Although you'll get the actual error the first time you load the depending namespace, after that you'll find a wrong compiler exception thrown every time you try to reload it. The situation will last even after you actually do fix the code causing the original error.

user=> (require 'broken-lib.core)

CompilerException java.lang.RuntimeException: Unmatched delimiter: }, compiling:(broken_lib.clj:3:3) 

user=> (require 'broken-lib.core :reload)
CompilerException java.lang.Exception: namespace 'broken-lib' not found, compiling:(broken_lib/core.clj:1:1) 

user=> (require 'broken-lib.core :reload) ;; reload after fix the bug in broken-lib

CompilerException java.lang.Exception: namespace 'broken-lib' not found, compiling:(broken_lib/core.clj:1:1) 
user=>

Cause:
The patch for CLJ-1116 made the ns macro blindly add the lib being defined into loaded-libs even if an error occurs during loading.

Approach:
Modify clojure.core/load-lib so that it removes the lib from loaded-libs on error.



 Comments   
Comment by Alex Miller [ 17/Apr/14 9:07 AM ]

This patch seems somewhat removed from the cause - is there some way to instead prevent the lib from being added to loaded-libs in the first place?

Comment by OHTA Shogo [ 17/Apr/14 9:21 AM ]

To do so, I think we need to revert CLJ-1116.

Comment by Alex Miller [ 22/Sep/16 1:41 PM ]

I don't think this solution is good as is so moving to Incomplete.





[CLJ-787] transient blows up when passed a vector created by subvec Created: 03/May/11  Updated: 22/Sep/16

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

Type: Defect Priority: Major
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File CLJ-787-p1.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Subvectors created with subvec from a PersistentVector cannot be made transient:

user=> (transient (subvec [1 2 3 4] 2))
ClassCastException clojure.lang.APersistentVector$SubVector cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:2864)

Cause: APersistentVector$SubVector does not implement IEditableCollection

Patch: CLJ-787-p1.patch

Approach: Create a TransientSubVector based on an underlying TransientVector.

Two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable functionality to the underlying TransientVector (sometimes explicitly, sometimes implicitly) - calling ensureEditable explicitly also requires that the field for the underlying vector be the concrete TransientVector type rather than the ITransientVector interface.
  • When an operation that would throw an exception on a PersistentVector happens from the wrong thread (or after persistent!), we throw that exception rather than the IllegalAccessError that transients throw when accessed inappropriately.


 Comments   
Comment by Stuart Sierra [ 31/May/11 9:28 AM ]

Confirmed. APersistentVector$SubVector does not implement IEditableCollection.

The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.

Comment by Gary Fredericks [ 25/May/13 8:11 PM ]

We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?

Preparing a patch to that effect.

Comment by Gary Fredericks [ 25/May/13 10:58 PM ]

Text from the commit msg:

Made two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable
    functionality to the underlying TransientVector (sometimes
    explicitely, sometimes implicitely) – calling ensureEditable
    explicitely also requires that the field for the underlying vector
    be the concrete TransientVector type rather than the
    ITransientVector interface.
  • When an operation that would throw an exception on a
    PersistentVector happens from the wrong thread (or after
    persistent!), we throw that exception rather than the
    IllegalAccessError that transients throw when accessed
    inappropriately.
Comment by Alex Miller [ 11/Oct/13 4:17 PM ]

I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.

A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):

user=> (transient (subvec (first {:a 1}) 0 1))
ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IEditableCollection  clojure.lang.APersistentVector$TransientSubVector.<init> (APersistentVector.java:592)

PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too. TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.

I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).

Needs more thought.

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

Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.

Comment by Alex Miller [ 17/Jan/14 10:19 AM ]

No good solution to consider right now, removing from 1.6.

Comment by Alex Miller [ 22/Sep/16 1:40 PM ]

Rich moved this to vetted, but I think it should have been left as Incomplete as last comments were never addressed.





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

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

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

1.8.0-alpha2 - 1.8.0-alpha4


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

 Description   

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

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

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

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

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

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

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

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

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

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

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

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

Patch: clj-1793-3.patch

Screened by: Alex Miller



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

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

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

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

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

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

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

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

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

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

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

See the commit message for an extensive-ish comment.

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

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

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

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

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

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

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

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

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

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

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

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

Before patch:

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

After patch:

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

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





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

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

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


 Description   

According to the Clojure Spec Rationale

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

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

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

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

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

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

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

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

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

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

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



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

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

Relevant quote from the Spec guide:

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

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

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





[CLJ-1906] Clojure should make representing iterated api calls easier Created: 30/Mar/16  Updated: 18/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None

Attachments: Text File 0001-CLJ-1906-add-ingeminate-function.patch     Text File 0001-CLJ-1906-add-unfold-function.patch     Text File 0001-CLJ-1906-transducer-enabled-iterate.patch     File CLJ-1906-seqable-reducible.diff     Text File CLJ-1906-successions.patch    

 Description   

Many apis (elasticsearch, github, s3, etc) have parts of the api
which, in usage, end up being used in an interative way. You make an
api call, and you use the result to make another api call, and so
on. This most often shows up in apis have some concept of pages of
results that you page through, and is very prevalent in http apis.

This appears to be such a common pattern that it would be great if
Clojure had in built support for it.

You may think Clojure already does have support for it, after all,
Clojure has `iterate`. In fact the docstring for `iterate`
specifically says the function you give it must be free of side
effects.

I propose adding a function `unfold` to clojure.core to support this
use case. `unfold` would return an implementation of ReduceInit. The
name `unfold` matches what would be a similar Haskell function
(https://hackage.haskell.org/package/base-4.8.2.0/docs/Data-List.html#v:unfoldr)
and also matches the name for a similar function used in some existing
Clojure libraries
(https://github.com/amalloy/useful/blob/develop/src/flatland/useful/seq.clj#L128-L147).

`unfold` in some ways looks like a combination of `take-while` and
`iterate`, except for the fact that `iterate` requires a pure
function. Another possible solution would be a version of `iterate`
that doesn't require a pure function.

It seems like given the use case I envision for `unfold`, a
non-caching reducible would be perfect. But that would leave those
that prefer seqs high and dry, so maybe at least some consideration
should be given to seqs.

Mailing list discussion is here
(https://groups.google.com/forum/#!topic/clojure-dev/89RNvkLdYc4)

A sort of dummy api you might want to interact with would look something like

(import '(java.util UUID))

(def uuids (repeatedly 1000 #(UUID/randomUUID)))

(def uuid-index
  (loop [uuids uuids
         index  {}]
    (if (seq uuids)
      (recur (rest uuids) (assoc index (first uuids) (rest uuids)))
      index)))

(defn api
  "pages through uuids, 10 at a time. a list-from of :start starts the listing"
  [list-from]
  (let [page (take 10 (if (= :start list-from)
                        uuids
                        (get uuid-index list-from)))]
    {:page page
     :next (last page)}))

given the above api, if you had an implementation of `unfold` that took a predicate that decided when to continue unfolding, a producer which given a value in a sequence produced the next value, and an initial value, you could do something like this:

(= uuids (into [] (mapcat :page) (unfold :next (comp api :next) (api :start))))

and the result would be true.

The equivilant take-while + iterate would be something like:

;; the halting condition is not strictly the same
(= uuids (into [] (mapcat :page) (take-while (comp seq :page) (iterate (comp api :next) (api :start)))))


 Comments   
Comment by Kevin Downey [ 31/Mar/16 4:21 PM ]

I made two patches, one adds unfold as discussed above, one adds ingeminate which is like iterate but without the function purity restrictions, and doesn't return a seq.

Comment by Ghadi Shayban [ 11/Apr/16 10:46 AM ]

Though syntax is less important than the semantics, may I propose the name `progression` for this? Clojure's fold is called reduce, so unfold is too much like Haskell. Other names I was considering include evolve & derivations.

Comment by Alex Miller [ 11/Apr/16 11:23 AM ]

Another option would be `productions` (reminiscent of `reductions`).

Comment by Kevin Downey [ 11/Apr/16 9:32 PM ]

productions has a nice ring to it. emanate could work too, would sort near eduction

Comment by Ghadi Shayban [ 12/Apr/16 10:08 PM ]

Adding a patch with a generator impl that is clojure.lang.{Seqable,IReduceInit}.

Generative tests assert that the seq and reduce halves are equivalent.

Tests assert basic functionality, obeying reduced, and maximal laziness of the seq impl.

Docstring has been wordsmithed and the function named `productions`.

Comment by Kevin Downey [ 18/Apr/16 3:21 PM ]

apparently unfold is part of SRFI 1: List Library in scheme land http://srfi.schemers.org/srfi-1/srfi-1.html#FoldUnfoldMap

it looks like their unfold is take-while + iterate + map

Comment by Ghadi Shayban [ 23/Apr/16 11:06 PM ]

Main differences between Scheme's impl and this proposed one:
Predicate reversed (stop? vs continue?)
Scheme has a "mapping function" to produce a different value from the current seed, Clojure doesn't (but has transducers)
Scheme has an extra optional arg to build the tail of the list

Now I'm partial to the name successions.

Comment by Michał Marczyk [ 10/May/16 11:07 AM ]

I can confirm that I found unfold quite useful in my Scheme days.

In Clojure, this general pattern can be expressed using transducers at a modest cost in keystrokes:

(def numbers (doall (range 1000)))

(defn api [list-from]
  (if list-from
    (let [page (vec
                 (take 10 (if (= :start list-from)
                            numbers
                            (drop list-from numbers))))]
      {:page page
       :next (some-> (last page) inc)})))

(= numbers
   (sequence (comp (take-while some?)
                   (mapcat :page))
             (iterate (comp api :next)
                      (api :start))))
;= true

Maybe this could be simplified with an xform-enabled version of iterate?

(defn iterate*
  ([f seed]
   (iterate f seed))
  ([xform f seed]
   (sequence xform (iterate f seed))))

(= numbers
   (iterate*
     (comp (take-while some?) (mapcat :page))
     (comp api :next)
     (api :start)))
;= true

Admittedly this takes more characters, but is quite generic and a transducer-enabled overload in iterate feels pretty natural to me. Attaching a simple patch implementing this in clojure.core/iterate – I'll look at clojure.lang.Iterate to see if it's worth implementing direct support later, unless of course nobody wants this.

Comment by Michał Marczyk [ 10/May/16 11:08 AM ]

0001-CLJ-1906-transducer-enabled-iterate.patch adds a ternary overload to iterate that delegates to the binary overload and sequence.

Comment by Ghadi Shayban [ 10/May/16 12:56 PM ]

A few unsatisfactory things about overloading {iterate}
1) iterate's docstring says {f must be free of side-effects}
2) There is boilerplate and subtlety around the terminating item. In this case the final API call is made unconditionally, leading to an extra empty/marker item that is filtered by take-while. With the current proposal, the predicate controls iteration from the inside out

Comment by Ghadi Shayban [ 06/Jun/16 8:40 AM ]

updated patch to apply cleanly to core

Comment by Ghadi Shayban [ 18/Sep/16 11:40 PM ]

I'm not sure I'm sold on this anymore, and have suggested a different approach on the mailing list https://groups.google.com/d/msg/clojure-dev/89RNvkLdYc4/PAJh8gfmDAAJ





[CLJ-2023] clojure.spec edge case failure Created: 14/Sep/16  Updated: 14/Sep/16  Resolved: 14/Sep/16

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

Type: Defect Priority: Major
Reporter: Brian Noguchi Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Using clojure 1.9.0-alpha12



 Description   

I stumbled onto an odd edge case. The following is a minimal example you can run in the REPL:

```
(require '[clojure.spec :as s])
(s/def :tv/star (s/or :starring/ernie #{:char/ernie}
:starring/big-bird #{:char/big-bird}))

; The following behaves as expected.
(s/explain
(s/and (s/keys :req [:tv/star]
:opt [:tv/co-star])
(s/or
:ernie-and-bert (s/and #(do (println "!!!!!") (println %) (= (:tv/star %) :char/ernie))
#(= (:tv/co-star %) :char/bert))
:just-the-bird #(not (contains? % :tv/co-star))))
{:tv/star :char/big-bird})
; This outputs the following:
; !!!!!
; #:tv{:star [:starring/big-bird :char/big-bird]}
; Success!
; => nil

; The following is unexpected
(s/explain
(s/and (s/keys)
(s/or
:ernie-and-bert (s/and #(do (println "!!!!!") (println %) (= (:tv/star %) :char/ernie))
#(= (:tv/co-star %) :char/bert))
:just-the-bird #(not (contains? % :tv/co-star))))
{:tv/star :char/ernie :tv/co-star :char/bert})
; This outputs the following:
; !!!!!
; #:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert}
; !!!!!
; #:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert}
; !!!!!
; #:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert}
; !!!!!
; #:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert}
; val: #:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert} fails at: [:ernie-and-bert] predicate: (do (println "!!!!!") (println %) (= (:tv/star %) :char/ernie))
; val: #:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert} fails at: [:just-the-bird] predicate: (not (contains? % :tv/co-star))
; => nil
```

I would have expected the second `(s/explain ...)` to succeed, given my understanding of `clojure.spec` semantics. However, it seems as though the argument inside `#(do (println "!!!!!") (println %) (= (:tv/star %) :char/ernie))` does not resolve `%` to the original map `{:tv/star :char/ernie :tv/co-star :char/bert}` but rather the transformed map `#:tv{:star [:starring/ernie :char/ernie], :co-star :char/bert}` that seems to mix in the output of applying conform against the spec definition of `:tv/star`.

Miscellaneous edge case observations:

  • If I replace the sibling spec `(s/keys)` with a simple predicate like `some?`, then it succeeds.

```
(s/explain
(s/and some?
(s/or
:ernie-and-bert (s/and #(do (println "!!!!!") (println %) (= (:tv/star %) :char/ernie))
#(= (:tv/co-star %) :char/bert))
:just-the-bird #(not (contains? % :tv/co-star))))
{:tv/star :char/ernie :tv/co-star :char/bert})
```

  • If I elide the first, solitary registered clojure.spec definition `(s/def :tv/star ...)`, then it succeeds.

I haven't investigated a solution yet.



 Comments   
Comment by Brian Noguchi [ 14/Sep/16 4:20 AM ]

It looks like the observed behavior is the expected correct behavior. I just noticed that successive conformed values are supposed to propagate through the rest of the predicates.

https://github.com/clojure/clojure/blob/d920ada9fab7e9b8342d28d8295a600a814c1d8a/src/clj/clojure/spec.clj#L439-L440

Comment by Brian Noguchi [ 14/Sep/16 4:21 AM ]

This issue can be ignored and closed. Sorry!





[CLJ-2022] Add fixed arities for mapcat Created: 13/Sep/16  Updated: 13/Sep/16

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

Type: Enhancement Priority: Minor
Reporter: Robert C Faber Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

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

 Description   

Add fixed arities for mapcat, in the pattern of map.

Same change as CLJS-1776.



 Comments   
Comment by Alex Miller [ 13/Sep/16 10:29 AM ]

Presumably this is to improve performance. Please include a benchmark showing the difference.

Comment by Ghadi Shayban [ 13/Sep/16 12:47 PM ]

Please consider interactions with apply and laziness CLJ-1218





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

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

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


 Description   

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

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

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



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

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

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

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

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

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

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

Another example from Slack, related to this:

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

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

Workaround:

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




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

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

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

clojure 1.9, mac osx, java 1.8



 Description   

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

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

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

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

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

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

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

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

)



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

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





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

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

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

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

 Description   

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

Proposed:

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

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

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

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



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

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

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

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

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

Hi Steve,

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

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

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

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

Comment by Pierre-Yves Ritschard [ 24/May/16 6:40 AM ]

Is there anything I can help to move this forward?
@alexmiller any comments on the code itself?

Comment by Alex Miller [ 24/May/16 7:31 AM ]

Haven't had a chance to look at it yet, sorry.

Comment by Pierre-Yves Ritschard [ 24/May/16 7:36 AM ]

@alexmiller, if the upshot is getting clojure.spec, I'll take this taking a bit of time to review

Comment by Steve Miner [ 25/May/16 3:21 PM ]

For testing, I suggest you compare the output from the transducer version to the output from a simliar call to the sequence reductions. For example,

(is (= (reductions + 3 (range 20)) (sequence (reductions-with + 3) (range 20)))

I would like to see that equality hold. The 0002 patch doesn't handle the init the same way the current Clojure reductions does.

Comment by Pierre-Yves Ritschard [ 07/Sep/16 4:29 PM ]

@alexmiller I'm tempting one more nudge to at least get an idea on the patch and the reductions-with variant since 1.9 seems to be getting closer to a release.

Comment by Alex Miller [ 08/Sep/16 10:43 AM ]

Sorry, don't know that I'll get to it soon or that it will be considered for 1.9. I also don't know that it won't, just ... don't know.

Comment by Pierre-Yves Ritschard [ 08/Sep/16 10:48 AM ]

@alexmiller, Thanks for the prompt reply. I'm trying to make sure i'll be around when feedback comes to be able to act quickly on it. Cheers!





[CLJ-2020] New prohibited field names (__hash __hasheq) break existing software Created: 07/Sep/16  Updated: 08/Sep/16  Resolved: 08/Sep/16

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

Type: Defect Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: defrecord


 Description   

The most recent Clojure alpha (1.9.0-alpha12) contained a patch closing CLJ-1224. This had the unfortunate side effect of breaking some existing software, notably datascript. I've filed an issue upstream as well: https://github.com/tonsky/datascript/issues/176

It's not clear to me what the best resolution is; IIUC the behavior datascript was trying to accomplish is what records now do automagically, although I might have misunderstood. Ideally, datascript wouldn't have the serious performance regression on >=1.8.0, but it definitely should compile on 1.9.0, regardless of how that's resolved.



 Comments   
Comment by Alex Miller [ 07/Sep/16 9:02 PM ]

My initial reaction is that datascript should not be using fields starting with __ as that is at least implied to be "internal stuff" in the defrecord docstring. But, I reserve the right to think about it more.

Comment by Alex Miller [ 08/Sep/16 9:51 AM ]

After thinking about it more, I will double down to say that extending a defrecord to IHashEq is also bad form. Records are intended to have the hash semantics of maps and to be controlled by the language, not by a record extender. I did quite a bit of searching and have found no other project that does this.

Implementing IHashEq for deftypes is a normal and perfectly acceptable thing to do as deftypes are considered to be opaque from Clojure's point of view - you give it the semantics.

Comment by Alex Miller [ 08/Sep/16 9:58 AM ]

Rich concurs so I am declining. The lib code should be changed.

Comment by lvh [ 08/Sep/16 9:59 AM ]

IIUC the options for datascript are either moving to deftype, or removing the IHashEq impl (and living with the perf penalty). Is there an option that lets them keep the perf, conditionally implementing only on 1.9.0? (That may not be desirable at all.)

Comment by Alex Miller [ 08/Sep/16 10:41 AM ]

It's conceivable to conditionally load different versions of a namespace that defines the records based on Clojure version. I'm not sure whether that's worth doing or not (and how it plays with AOT). I don't know whether it even affects perf or by how much - seems like that's the first question to answer. If it doesn't matter, then just remove it.





[CLJ-1935] clojure.spec/multi-spec ignores the multimethod hierarchy Created: 28/May/16  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Defect Priority: Major
Reporter: Viktor Magyari Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: multimethods, spec

Attachments: Text File clj-1935-1.patch     Text File clj-1935.patch    
Patch: Code
Approval: Ok

 Description   

Minimal example:

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

(def h (derive (make-hierarchy) :a :b))

(defmulti spec-type identity :hierarchy #'h)

(defmethod spec-type :b [_]
  (s/spec (constantly true)))

(s/def ::multi (s/multi-spec spec-type identity))

(s/explain ::multi :b)
;; => Success!
;; as expected

(s/explain ::multi :a)
;; => val: :a fails at: [:a] predicate: spec-error/spec-type,  no method
;; fails even though :a derives from :b

Also fails with the default hierarchy. Worked fine in alpha1, broken in alpha2 and alpha3.

Cause: The implementation of multi-spec uses a predicate that invokes the dispatch function, then looks up the result in the method table. However this does not leverage the actual logic used in multimethods for hierarchy resolution.

Approach: Replace the lookup in the method table with a call to getMethod(), which will use the same lookup logic that multimethod uses.

Patch: clj-1935.patch



 Comments   
Comment by Rich Hickey [ 06/Sep/16 12:20 PM ]

dval should change similarly

Comment by Alex Miller [ 06/Sep/16 1:41 PM ]

dval was and is correct, I think? it's just the means of looking up a match for the dispatch value.





[CLJ-322] Enhance AOT compilation process to emit classfiles only for explicitly-specified namespaces Created: 29/Apr/10  Updated: 07/Sep/16

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 27
Labels: aot

Attachments: Text File 0322-limit-aot-resolved.patch     File CLJ-322.diff     Text File compile-interop-1.patch     GZip Archive write-classes-1.diff.gz    
Patch: Code and Test
Approval: Vetted
Waiting On: Chas Emerick

 Description   

Summary: still needs decision on implementation approach.

This was originally/erroneously reported by Howard Lewis Ship in the clojure-contrib assembla:

My build file specifies the namespaces to AOT compile but if I include another namespace
(even from a JAR dependency) that is not AOT compiled, the other namespace will be compiled as well.

In my case, I was using clojure-contrib's clojure.contrib.str-utils2 namespace, and I got a bunch of
clojure/contrib/str_utils2 classes in my output directory.

I think that the AOT compiler should NOT precompile any namespaces that are transitively reached,
only namespaces in the set specified by the command line are appropriate.

As currently coded, you will frequently find unwanted third-party dependencies in your output JARs;
further, if multiple parties depend on the same JARs, this could cause bloating and duplication in the
eventual runtime classpath.

Having the option of shipping either all AOT-compiled classfiles or mixed source/AOT depending upon one's distribution requirements would make that phase of work with a clojure codebase significantly easier and less error-prone. The only question in my mind is what the default should be. We're all used to the current behaviour, but I'd guess that any nontrivial project where the form of the distributable matters (i.e. the source/AOT mix), providing as much control as possible by default makes the most sense. Given the tooling that most people are using, it's trivial (and common practice, IIUC) to provide a comprehensive list of namespaces one wishes to compile, so making that the default shouldn't be a hurdle to anyone. If an escape hatch is desired, a --transitive switch to clojure.lang.Compile could be added.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/322
Attachments:
aot-transitivity-option-compat-322.diff - https://www.assembla.com/spaces/clojure/documents/aI7Eu-HeGr35ImeJe5cbLA/download/aI7Eu-HeGr35ImeJe5cbLA
aot-transitivity-option-322.diff - https://www.assembla.com/spaces/clojure/documents/aIWFiWHeGr35ImeJe5cbLA/download/aIWFiWHeGr35ImeJe5cbLA

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

hlship said: I'd like to reinforce this. I've been doing research on Clojure build tools for an upcoming talk and all of them (Maven, Leiningen, Gradle) have the same problem: the AOT compile extends from the desired namespaces (such as one containing a :gen-class) to every reached namespace. This is going to cause a real ugliness when application A uses libraries B and C that both depend on library D (such as clojure-contrib) and B and C are thus both bloated with duplicate, unwanted AOT compiled classes from the library D.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: This behaviour is an implementation detail of Clojure's AOT compilation process, and is orthogonal to any particular build tooling.

I am working on a patch that would provide a mechanism for such tooling to disable this default behaviour.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: A first cut of a change to address this issue is here (caution, work in progress!):

http://github.com/cemerick/clojure/commit/6f14e0790c0d283a7e44056adf1bb3f36bb16e0e

This makes available a new recognized system property, clojure.compiler.transitive, which defaults to true. When set/bound to false (i.e. -Dclojure.compiler.transitive=false when using clojure.lang.Compile), only the first loaded file (either the ns named in the call to compile or each of the namespaces named as arguments to clojure.lang.Compile) will have classfiles written to disk.

This means that this compilation invocation:

java -cp <your classpath> -Dclojure.compiler.transitive=false clojure.lang.Compile com.bar com.baz

will generate classfiles only for com.bar and com.baz, but not for any of the namespaces or other files they load, require, or use.


The only shortcoming of this WIP patch is that classfiles are still generated for proxy and gen-class classes defined outside of the explicitly-named namespaces. What I thought was a solution for this ended up breaking the loading of generated interfaces (as produced by defprotocol, etc).

I'll take a second look at this before the end of the week, but wanted to get this out there so as to get any comments people might have.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: Looks good, but I'm having trouble getting it to work. I tried compiling from master of Chas's fork on github, but I still got the all the .class files generated with -Dclojure.compiler.transitive=false. It could be a quirk of the way I'm using ant to fork off processes though. Is it possible to set it using System/setProperty, or must it be given as a property on the command-line?

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Bah, that's just bad documentation. :-/

The system property is only provided by clojure.lang.Compile; the value of it drives the binding of clojure.core/transitive-compile, which has a root binding of true.

You should be able to configure the transitivity the same way you configure compile-path (system prop to clojure.lang.Compile or a direct binding when at the REPL, etc).

If not, ping me in irc or elsewhere.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: I think, excluding parts 'load'ed is a little strong. I have some namespaces which load several parts from different files, but which belong to the same namespace. The most prominent example of such a case is clojure.core itself. I'm find with stopping require and use, but load is a bit too much, I'd say.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: Chas: Thanks; will give that a go.

Meikel: Do people actually use load outside of clojure.core? I thought it was only used there because clojure.core is a "special" namespace where you want more vars to be available than can reasonably fit in a single file. Splitting up a namespace into several files is quite unadvisable otherwise.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: I can confirm that this works for me modulo the proxy/gen-class issue that Chas mentioned. I would love to see this in Clojure 1.2; it would really clean up a lot of build-related issues.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: I used it several times and this is the first time, I hear that it is unadvisable to do so. Even with a lower number of Vars in the namespace (c.c is here certainly exceptional) and might be of use to split several "sections" of code which belong to the same namespace but have different functionality. Whether to use a big comment in the source to indicate the section or split things into subfiles is a matter of taste. But it's a perfectly reasonable thing todo.

Another use case, where I use this (and c.c.lazy-xml, IIRC) is to conditionally load code depending on whether a dependency is available or not. Eg. vimclojure uses c.c.pprint and c.c.stacktrace/clj-stacktrace transparently depending on their availability.

There are perfectly legal uses of load. I don't see any "unadvisable" here.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Thanks, Meikel; I had forgotten about that use case, as I don't use load directly myself at all. I probably wouldn't say it's inadvisable, just mostly unnecessary. In any case, that's a good catch. It complicates things a bit, but we'll see what happens. I'm going to take another whack at resolving the proxy/gen-class case and narrowing the impact of nontransitivity to use and require later tonight.

I agree wholeheartedly that this should be in 1.2, assuming the technical bits work out. This has been an irritant for quite a long time. I actually believe that nontransitivity should be the default – no one wants or expects to have classfiles show up for dependencies show up when a project is AOT-compiled. I think the only negative impact would be whoever still fiddles with compilation at the REPL, and doesn't use maven or lein – and even then, it's just a matter of binding another var.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: Then the var should be added to the default bindings in the clojure.main repl. Then it's set!-able like the other vars ��� warn-on-reflection and friends.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: This is looking pretty good (still WIP):

http://github.com/cemerick/clojure/commit/fedfb022ecef420a932b3d69c182ec7a8e5960a6

Thank you again for mentioning load, Meikel: it was very helpful in resolving the proxy/gen-class issue as well.

Just a single data point: the jar produced by the medium-sized project I've been using for testing the changes has shrunk from 1.8MB to less than 1MB. That's not the only reason this is a good change, but it's certainly a nice side-effect.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: [file:aIWFiWHeGr35ImeJe5cbLA]

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: [file:aI7Eu-HeGr35ImeJe5cbLA]

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Patched attached. The compat one retains the current default behaviour [*transitive-compile* true], the other changes the default so that transitivity is a non-default option. At least of those I've spoken to about this, the latter is preferred.

The user impact of changing the default would be:

  1. The result of compiling from the REPL will change. Getting back current behaviour would require adding a [*transitive-compile* true] binding to the existing bindings one must set when compiling from the REPL.
  2. The same as #1 goes for those scripting AOT compilation via clojure.lang.Compile as well (whether by shell scripts, ant, etc).
  3. Those using lein, clojure-maven-plugin, gradle, and others will likely have a new option provided by those tools, and perhaps a different default than the language's. I suspect those using such tools would much prefer a change from the default behaviour in any case.
Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

hlship said: Just had a brain-storm:

How about an option to support transitive compilation, but only if the Clojure source file being compiled as a file: URL (i.e., its a local file on the file system, not a file stored in a JAR). That would make it easier to use compilation on the local project without transitively compiling imported libraries, such as clojure-contrib.

So transitive-compile should be a keyword, not a boolean, with values :all (for 1.1 behavior), :none (to compile only the exact specified namespaces) or :local (to compile transitively, but only for local files, not source files from JARs).

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: (Crossposted to the clojure-dev list)

I thought about this some, and I don't think that's a good idea, at least for now. I'm uncomfortable with semantics changing depending upon where code is being loaded from – which, depending upon a tool's implementation, might be undefined. E.g. if the com.foo.bar ns is available in source form in one directory, but as classes from a jar, and classpaths aren't being constructed in a stable fashion, then the results of compilation will change.

If we decide that special treatment depending upon the source of code is warranted in the future, that's a fairly straightforward thing to do w.r.t. the API – we could have :all and :local as you suggest, with nil representing :none.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

stu said: Rich is not comfortable enough with the implementation complexity of this patch (e.g. the guard clause for proxies and gen-class) to slide this in as a minor fix under the wire for 1.2.

Better to live with the pain we know a little longer than ship something we don't have enough experience with to be confident.

Comment by Chas Emerick [ 19/Nov/10 9:28 PM ]

Updated patch to cleanly apply to HEAD and address issues raised by screening done by Cosmin Stejerean. Also includes proper tests.

Note: this patch's tests require the fix for CLJ-432!

Comment by Stuart Halloway [ 29/Nov/10 7:18 AM ]

the "-resolved" patch resolves a conflict in main.clj

Comment by Stuart Halloway [ 29/Nov/10 7:25 AM ]

Several questions:

  1. I am getting an ant build error: "/Users/stuart/repos/clojure/build.xml:137: java.io.FileNotFoundException: Could not locate clojure/test_clojure/aot/nontransitive__init.class or clojure/test_clojure/aot/nontransitive.clj on classpath:"
  2. It feels icky to have a method named writeClassFile that, under some circumstances, does not write a class file, but instead loads it via a dynamic loader. Maybe this is just a naming issue.
  3. Are there any other ways to accomplish the goals of load-level? Or, taking the other side, if we are going to have a load-level, are there other possible consumers who might have different needs?
  4. (Minor) Why the use :only idiom instead of just require?
Comment by Stuart Sierra [ 10/Dec/10 3:34 PM ]

An alternative approach: patch write-classes-1.diff.gz

From my forked branch

What this patch does:

  • Keeps 'compile' and 'compile-files' exactly the same
  • Adds 'compile-write-classes' to write .class files for specifically named classes
  • Minor compiler changes to support this

This approach was prompted by the following observations:

  • Java interop is the dominant reason for needing .class files
  • Things other than namespaces can generate classes for Java interop:
    • deftype/defrecord
    • defprotocol
    • gen-class/gen-interface
  • For library releases, we want to control which .class files are emitted on a per-class basis, not per-namespace
  • Some legitimate uses of AOT compilation will want transitive compilation
    • Pre-compiling an entire application before release
Comment by Chas Emerick [ 10/Dec/10 4:04 PM ]

S. Halloway: My apologies, I didn't know you had commented. I thought that, having assigned this issue to myself, I'd be notified of updates.

FWIW, I aim to review your comments and SS' approach over the weekend.

Comment by Chas Emerick [ 16/Dec/10 7:36 AM ]

S. Halloway:

1. Certainly shouldn't happen. AFAIK, others have screened the patch, presumably with a successful build.
2. Agreed; given the approach, I think it's just a bad name.
3. Yes, I think S. Sierra's is one. See my next comment.
4. Because the :use form was already there. I've actually been using that form of :use more and more; I've found that easier than occasionally having to shuffle around specs between :use and :require. I think I'm aping Chris Houser in that regard.

Comment by Chas Emerick [ 16/Dec/10 9:00 AM ]

I think S. Sierra's approach is fundamentally superior what I offered. I have two suggestions: one slight perspective change (which implies no change in the actual implementation), and an idea for an even simpler approach (at least from a user perspective), in that order.

While interop is the driving requirement behind AOT, I absolutely do not want to have to keep an updated enumeration of all of the classes resulting from each and every defrecord et al. usages in my pom.xml/project.clj (and I wouldn't wish the task of ferreting those usages and their resulting classnames on any build tool author).

Right now, *compile-write-classes* is documented to be a set of classname strings, but could just as easily be any other function. *compile-write-classes* should be documented to accept any predicate function (renamed to e.g. *compile-write-class?*?). There's no reason why it shouldn't be bound to, e.g. #(re-matches #"foo\.bar\.[\w_]+$" %) if I know that all my records are defined in the foo.bar namespace.

To go along with that, I think some package/classname-globbing utilities along with corresponding options to clojure.lang.Compile would be most welcome. Classname munging rules are not exactly obvious, and it'd be good to make things a little easier for users in this regard.


Another alternative

If there's a closed set of forms that generate classes that one might reasonably be interested in having in a build result (outside of use cases for pervasive AOT), then why not have a simple option that only those forms utilize? gen-class and gen-interface already do this, but reusing the all-or-nothing *compile-files* binding; if they keyed off of a binding that implied a diminished scope (e.g. *compile-interop-forms* – which would be true if *compile-files* were true), then they'd do exactly what we wanted. Extending this approach to deftype (and therefore defrecord) should be straightforward.

An implementation of this would probably be somewhat more complicated than S. Sierra's patch, though not as complex as my original stab at the problem (i.e. no *load-level*). On the plus side:

1. No additional configuration for users or implementation work for build tool authors, aside from the addition of the boolean diminished-scope AOT option
2. Class file generation would remain opaque from a build process standpoint
3. Future/other class-generating forms (there are a few people futzing with ASM independently, etc) can make local decisions about whether or not to participate in interop-centric classfile generation. This might be particularly helpful if a given form emits multiple classes, making the determination of a classname-based filter fn less straightforward.

I can see wanting to further restrict AOT to specific classnames in certain circumstances, in which case the above and S. Sierra's patch might be complimentary.

Comment by Stuart Sierra [ 16/Dec/10 11:49 AM ]

I like the idea of *compile-interop-forms*. But is it always possible to determine what an "interop form" is? I think it is, I'm just not sure.

Comment by Allen Rohner [ 09/Oct/11 12:50 PM ]

I'm also in favor of compile-interop-forms. As far as determining, how about sticking metadata on the var?

(defmacro ^{:interop-form true} deftype ...)

Comment by Stuart Sierra [ 21/Oct/11 8:38 AM ]

Summary and design discussion on wiki at http://dev.clojure.org/display/design/Transitive+AOT+Compilation

Comment by Stuart Sierra [ 29/Nov/11 6:54 PM ]

New attachment compile-interop-1.patch has new approach: Add a third possible value for *compile-files*. True and false keep their original meanings, but :interop causes only interop-related forms to be written out as .class files. "Interop forms" are gen-class, gen-interface, deftype, defrecord, defprotocol, and definterface.

Pros:

  • doesn't change existing behavior
  • handles common case for non-transitive AOT (interop)
  • minimal changes to the compiler

Cons:

  • not flexible
Comment by Stuart Sierra [ 02/Dec/11 8:12 AM ]

Just realized my patch doesn't solve the transitive compilation problem. If library A loads library B, then compiling interop forms in A will also emit interop .class files in B.

Comment by Paudi Moriarty [ 01/Jan/13 3:55 AM ]

It's disappointing to see an important issue like this still unresolved after 2.5 years. This is a real pain for us. We have a large closed source project where shipping source is not an option. This forces us to manage the AOT'ing of dependencies due to the hard dependency on protocol interfaces introduced by transitive AOT compilation (see https://groups.google.com/forum/?fromgroups=#!topic/clojure-dev/r3A1JOIiwVU).

Comment by Andy Fingerhut [ 01/Jan/13 4:27 PM ]

Paul, do you have a suggestion for which of the approaches described in comments here, or on the wiki page http://dev.clojure.org/display/design/Transitive+AOT+Compilation would be preferable solution for you? Or perhaps even a patch that implements your preferred approach?

Comment by Howard Lewis Ship [ 04/Jan/13 4:18 PM ]

Andy,

I'm now consulting with Paudi's organization, so I think I can speak for him (I'm now the default buildmeister).

I like Stuart's :interop idea, but that is somewhat orthogonal to our needs.

I return to what I would like; compilation would compile specific namespaces; dependencies of those namespaces would not be compiled.

To be honest, I'm still a little hung up on the interop forms: especially defprotocol and friends; from a cursory glance, it appears that todays AOT compilation will compile the protocol into a Java class, then compile the namespace that references the protocol with the assumption that the protocol's Java class is available. When we use build rules to only package our namespace's class files into the output JAR, the code fails with a NoClassDefFoundError because the protocol really needs to be recompiled, at runtime compilation, into an in-memory Java class.

Obviously, supporting this correctly will be a challenge; the compiled bytecode for our namespace would ideally:
1) check to see if the Java class already exists and use it if so
2) load the necessary namespaces so as to force the creation of the Java class

I can imagine any number of ways to juggle things to make this work, so I won't suggest a specific implementation.

In the meantime, our workaround is to create a "stub" module as part of our build; it simply requires in the necessary namespaces (for example, org.clojure:core.cache); this forces an AOT compile of the dependencies and we have a special rule to package such dependencies in the stub module's output JAR. This may not be a scalable problem, and it is expensive to identify what 3rd party dependencies require this treatment.

Comment by Stuart Halloway [ 24/May/13 1:25 PM ]

I am marking this incomplete because there does not yet seem to be plurality, much less consensus or unanimity, on approach.

Personally I am in favor of a solution based on a predicate that gets handed the class name and compiled bits, and then can choose whether to write the class. Pretty close to Stuart Sierra's compile-write-classes. Might be possible to flow more information than classname to the predicate.

Comment by Alex Miller [ 21/Oct/13 2:35 PM ]

Removed the 1.6 release from this and added to Release.Next list to make this a priority for the next release.

Comment by Kevin Downey [ 21/Oct/13 3:42 PM ]

Howard,

I don't exactly understand your write up, I am reading the compiler, the emit-protocol macro, and emit-method-builder to try and understand it.

You might check to see if you have a situation similar to the following:

(ns a.b)

(defprotocol P1
  (pm [a]))

then either

(ns a.c
  (:import (a.b P1))

(defrecord R []
  P1
  (pm [x] x))

or

(ns a.c)

(defrecord R []
  a.b.P1
  (pm [x] x))

in both examples defrecord is actually getting the class behind the protocol instead of the protocol, the correct thing to do is

(ns a.c
  (:require [a.b :refer [P1]]))

(defrecord R []
  P1
  (pm [x] x))

This is an extremely common mistake people make when using protocols, unfortunately the flexibility of using interfaces directly in defrecord forms, and protocols being backed by interfaces means it is very easy to unwittingly make such a mistake. Both of the mistake examples could result in missing classes/namespace problems.

Comment by Michael Blume [ 08/Jan/15 4:26 PM ]

The write-classes patch still applies cleanly, I just tried it out, made a one-line change to lein-ring and got a war-file for my app with two generated classes instead of hundreds. This seems like a good fix to me. What do we need to do to get it into release?

Comment by Alex Miller [ 08/Jan/15 4:30 PM ]

As Stu said above, we do not have consensus on whether this is the right way to go with this. I am planning to look at all outstanding AOT tickets (including this one) early in 1.8.

Comment by Stuart Halloway [ 07/Sep/16 3:15 PM ]

I believe that we should close this ticket. AOT ought to make all the classfiles as it does today to avoid classloader inversion problems (see e.g. http://dev.clojure.org/jira/browse/CLJ-1544), and tools can us e.g. boot's sift to remove classes not part of the current project from delivered jars.

We might want to make some tools that identify jars that erroneously includes compiled classes from other namespaces, i.e. resulting from not using sift.

Comment by Kevin Downey [ 07/Sep/16 3:44 PM ]

something like boot's sift is not a complete solution.

lein his included that functionality for sometime, and back in 2010 it defaulted to removing classes transitively generated. that turned out to break things so it was disabled. the functionality still exists but defaults to off.

the setting if someone wants to try it: https://github.com/technomancy/leiningen/blob/master/sample.project.clj#L330
the issue that lead to it being turned off by default: https://github.com/technomancy/leiningen/issues/141





[CLJ-2012] ns spec breaks gen-class forms that use strings as class names Created: 24/Aug/16  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: regression, spec

Attachments: Text File clj-2012-2.patch     Text File clj-2012.patch    
Patch: Code
Approval: Ok

 Description   

The following valid `ns` gen-class form is reported as invalid by spec:

(ns foo
  (:gen-class
     :name ^{org.apache.logging.log4j.core.config.plugins.Plugin
             {:name "LogstashConverter" :category "Converter"}
             org.apache.logging.log4j.core.pattern.ConverterKeys ["logstashJson"]} social.common.logging.LogstashPatternConverter
     :constructors {["[Ljava.lang.String;"] [String String]}
     :factory newInstance
     :init init
     :state state
     :extends org.apache.logging.log4j.core.pattern.LogEventPatternConverter))

This is because the ns spec assumes that all class names can be represented by simple-symbols, while in reality some can only be represented by strings.

Approach: Pull out spec for class identifiers which can be either a simple-symbol (class name) or a string and use that in signature (which is used for both gen-class constructors and methods.

Patch: clj-2012-2.patch



 Comments   
Comment by Stuart Halloway [ 26/Aug/16 7:23 AM ]

Nicola: Good catch, thanks for the report!

Alex: argtype is a pretty general name to grab at the top level spec ns. How about something like class-ident?





[CLJ-2008] clojure.spec.test/check without args tests fns from clojure.core, erroring out with #:clojure.spec{:failure :no-fn} Created: 21/Aug/16  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Defect Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec
Environment:

N/A


Attachments: Text File clj-2008.patch    
Patch: Code
Approval: Ok

 Description   

When running (clojure.spec.test/check (in CIDER, but I doubt that's relevant other than it changes the formatting of the following snippet):

0. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/when-let, :spec clojure.spec$fspec_impl$reify__13891@6ee9efaf }
  1. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/let, :spec clojure.spec$fspec_impl$reify__13891@4ff7da85 }
  2. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/if-let, :spec clojure.spec$fspec_impl$reify__13891@35daec9c }
  3. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/fn, :spec clojure.spec$fspec_impl$reify__13891@98fcf1f }
... (some elided)
  8. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/ns, :spec clojure.spec$fspec_impl$reify__13891@283add16 }
  9. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/defn, :spec clojure.spec$fspec_impl$reify__13891@59681966 }
... (some elided)
  12. { :failure clojure.lang.ExceptionInfo: No fn to spec #:clojure.spec{:failure :no-fn}, :sym clojure.core/defn-, :spec clojure.spec$fspec_impl$reify__13891@3b34b0db }

It doesn't seem appropriate to tell me that these macros have no fn to spec. Perhaps it should know about macros.

Problem: checkable-syms does not omit spec'ed macros so when they flow down to check-1, they throw errors. Since these can't be checked, they should be omitted from checkable-syms. I put the change in fn-spec-name?, which is also used (and has the same problem in) instrumentable-syms.

Approach: Skip spec'ed macros.

Patch: clj-2008.patch



 Comments   
Comment by Alex Miller [ 22/Aug/16 10:10 AM ]

That's interesting as there were changes in alpha11 designed to catch and omit macros from check. So, something still amiss there.

Comment by Stuart Halloway [ 26/Aug/16 7:31 AM ]

lvh: thanks for the report!

Not sure that we want to give up on generatively testing macros, but it certainly doesn't work atm, so this seems good until we take it on.





[CLJ-2006] clojure.spec/fdef mentions nonexistent clojure.spec.test/run-tests Created: 21/Aug/16  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Defect Priority: Minor
Reporter: lvh Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, spec
Environment:

N/A


Attachments: Text File clj-2006.patch    
Patch: Code
Approval: Ok

 Description   

Once registered, function specs are included in doc, checked by
instrument, tested by the runner clojure.spec.test/run-tests, and (if
a macro) used to explain errors during macroexpansion.

Should be: clojure.spec.test/check



 Comments   
Comment by lvh [ 21/Aug/16 6:55 AM ]

Crud, I messed up the title of this issue copy-pasting from the docstring, and I appear to lack the permissions to resolve it.

Comment by Alex Miller [ 21/Aug/16 3:34 PM ]

lvh I've added you to the groups with edit permission.

Comment by Stuart Halloway [ 26/Aug/16 7:34 AM ]

Thanks again lvh!





[CLJ-2004] s/multi-spec doesn't include :retag in `s/form` Created: 18/Aug/16  Updated: 07/Sep/16  Resolved: 07/Sep/16

Status: Closed
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: Completed Votes: 0
Labels: spec

Attachments: Text File clj-2004.patch    
Patch: Code
Approval: Ok

 Description   

`s/form` on a multispec doesn't include the :retag, so it's impossible to recover that information from an existing spec. This is important for tooling that interacts with specs.

(require '[clojure.spec :as s])
(defmulti mm :mm/type)
(s/def ::foo (s/multi-spec mm :mm/type))
(s/form ::foo)
;; (clojure.spec/multi-spec user/mm) ;; :mm/type is missing

Approach: Include retag in form

Patch: clj-2004.patch



 Comments   
Comment by Alex Miller [ 18/Aug/16 6:18 PM ]

Yeah, this is one of many known form issues.





[CLJ-1988] collection specs conform to reversed list when used on a sequence Created: 24/Jul/16  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Defect Priority: Major
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: spec

Attachments: Text File clj-1988.patch    
Patch: Code and Test
Approval: Ok

 Description   

Example

Clojure 1.9.0-alpha10
user=> (require '[clojure.spec :as s])
nil
user=> (s/conform (s/coll-of int?) (range 5))
(4 3 2 1 0)

Problem: Current code handles vectors, maps, and lists but falls through on sequences to the last case and adds to an empty sequence (at the head).

Approach: Add seqs to the list case.

Patch: clj-1988.patch



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

In case it helps, a workaround might be: (s/conform (s/coll-of int? :into ()) (range 5))





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

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

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

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

 Description   

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

After:

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

Patch: clj-1673-3.patch

Screened by: Alex Miller



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

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

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

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

Please add a test...

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

Updated patch, tweaked dir-fn, added test.

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

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

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

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

Comment by Michael Blume [ 12/Oct/15 5:54 PM ]

Applied this patch directly to master, test failure:

[java] ERROR in (test-dir) (core.clj:4023)
     [java] expected: (= (dir-fn (quote clojure.string)) (dir-fn (quote str)))
     [java]   actual: java.lang.Exception: No namespace: str found
     [java]  at clojure.core$the_ns.invokeStatic (core.clj:4023)
     [java]     clojure.repl$dir_fn.invokeStatic (repl.clj:186)
     [java]     clojure.repl$dir_fn.invoke (repl.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967$fn__21976.invoke (repl.clj:28)
     [java]     clojure.core$with_redefs_fn.invokeStatic (core.clj:7211)
     [java]     clojure.core$with_redefs_fn.invoke (core.clj:-1)
     [java]     clojure.test_clojure.repl$fn__21967.invokeStatic (repl.clj:27)
     [java]     clojure.test_clojure.repl/fn (repl.clj:-1)
     [java]     clojure.test$test_var$fn__7962.invoke (test.clj:703)
     [java]     clojure.test$test_var.invokeStatic (test.clj:703)
     [java]     clojure.test$test_var.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984$fn__7989.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars$fn__7984.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invokeStatic (test.clj:673)
     [java]     clojure.test$default_fixture.invoke (test.clj:-1)
     [java]     clojure.test$test_vars.invokeStatic (test.clj:717)
     [java]     clojure.test$test_all_vars.invokeStatic (test.clj:723)
     [java]     clojure.test$test_ns.invokeStatic (test.clj:744)
     [java]     clojure.test$test_ns.invoke (test.clj:-1)
     [java]     clojure.core$map$fn__4770.invoke (core.clj:2639)
     [java]     clojure.lang.LazySeq.sval (LazySeq.java:40)
     [java]     clojure.lang.LazySeq.seq (LazySeq.java:49)
     [java]     clojure.lang.Cons.next (Cons.java:39)
     [java]     clojure.lang.RT.next (RT.java:688)
     [java]     clojure.core$next__4327.invokeStatic (core.clj:64)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:924)
     [java]     clojure.core$reduce1.invokeStatic (core.clj:914)
     [java]     clojure.core$merge_with.invokeStatic (core.clj:2943)
     [java]     clojure.core$merge_with.doInvoke (core.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:139)
     [java]     clojure.core$apply.invokeStatic (core.clj:647)
     [java]     clojure.test$run_tests.invokeStatic (test.clj:754)
     [java]     clojure.test$run_tests.doInvoke (test.clj:-1)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:137)
     [java]     clojure.core$apply.invokeStatic (core.clj:645)
     [java]     clojure.core$apply.invoke (core.clj:-1)
     [java]     user$eval29133.invokeStatic (run_test.clj:8)
     [java]     user$eval29133.invoke (run_test.clj:-1)
     [java]     clojure.lang.Compiler.eval (Compiler.java:6934)
     [java]     clojure.lang.Compiler.load (Compiler.java:7381)
     [java]     clojure.lang.Compiler.loadFile (Compiler.java:7319)
     [java]     clojure.main$load_script.invokeStatic (main.clj:275)
     [java]     clojure.main$script_opt.invokeStatic (main.clj:335)
     [java]     clojure.main$script_opt.invoke (main.clj:-1)
     [java]     clojure.main$main.invokeStatic (main.clj:421)
     [java]     clojure.main$main.doInvoke (main.clj:-1)
     [java]     clojure.lang.RestFn.invoke (RestFn.java:408)
     [java]     clojure.lang.Var.invoke (Var.java:379)
     [java]     clojure.lang.AFn.applyToHelper (AFn.java:154)
     [java]     clojure.lang.Var.applyTo (Var.java:700)
     [java]     clojure.main.main (main.java:37)
Comment by Jason Whitlark [ 12/Oct/15 7:22 PM ]

After poking at it for a while, I discovered that the test in question isn't being executed in clojure.test-clojure.repl namespace, but in user. So replacing the failing test with:
(is (= (the-ns 'clojure.test-clojure.repl) ns))
also fails.

Which is very surprising to me. Is this the expected behavior? I could fix the test by doing the following:

(binding [*ns* (the-ns 'clojure.test-clojure.repl)]
(is (= (dir-fn 'clojure.string) (dir-fn 'str))))

But I don't want to mask an error, if this behavior is unexpected. Guidance please.

Comment by Alex Miller [ 12/Oct/15 7:57 PM ]

I don't want this to get applied as is so moving to incomplete for now.

Comment by Alex Miller [ 12/Oct/15 8:00 PM ]

You know this could be due to direct linking - calls inside the clojure code to other clojure code are now direct linked (static) calls as of 1.8.0-alpha3 and can't be redef'ed.

Comment by Alex Miller [ 12/Oct/15 8:03 PM ]

Yeah, if you turn direct linking off the test passes.

Comment by Jason Whitlark [ 13/Oct/15 1:06 PM ]

Fixed test to work with direct linking.

Comment by Jason Whitlark [ 10/Nov/15 12:58 AM ]

This is ready to go. Do I need to do anything to get it back in the queue?

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

We've missed the window for 1.8 but it's in the queue for 1.9.





[CLJ-1224] Records do not cache hash like normal maps Created: 24/Jun/13  Updated: 07/Sep/16  Resolved: 07/Sep/16

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

Type: Enhancement Priority: Critical
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 21
Labels: defrecord, performance

Attachments: Text File 0001-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-cache-hasheq-and-hashCode-for-records-v2.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records-v2.patch     Text File clj-1224-3.patch    
Patch: Code and Test
Approval: Ok

 Description   

Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

Approach: Cache hash values in record definitions, similar to maps.

Timings:

coll 1.9.0-master 1.9.0-master+patch
small record 90 ns 7 ns
big record 446 ns 8 ns
(defrecord R [a b])  ;; small
(def r  (R. (range 1e3) (range 1e3)))
(bench (hash r))
(defrecord R [a b c d e f g h i j])  ;; big
(def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
(bench (hash r))

Patch: clj-1224-3.patch

Screened by: Alex Miller

Also see: http://dev.clojure.org/jira/browse/CLJS-281



 Comments   
Comment by Nicola Mometto [ 14/Feb/14 5:46 PM ]

I want to point out that my patch breaks ABI compatibility.
A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.

Comment by Stuart Halloway [ 27/Jun/14 11:09 AM ]

The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:

  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.

I found these problems via the following reasoning:

  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Comment by Nicola Mometto [ 27/Jun/14 2:53 PM ]

Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor

Comment by Alex Miller [ 27/Jun/14 4:08 PM ]

Questions addressed, back to Vetted.

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

All patches dated Jun 7 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

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

Comment by Alex Miller [ 29/Aug/14 4:40 PM ]

Would be great to get this one updated as it's otherwise ready to screen.

Comment by Nicola Mometto [ 29/Aug/14 4:58 PM ]

Updated patch to apply to lastest master

Comment by Alex Miller [ 16/Jun/15 4:06 PM ]

1) hash and hasheq are stored as Objects, which seems kind of gross. It would be much better to store them as primitive longs and check whether they'd been calculated by comparing to 0. We still end up with a long -> int conversion but at least we'd avoid boxing.

2) assoc wrongly copies over the __hash and __hasheq to the new instance:

(defrecord R [a])
(def r (->R "abc"))
(hash r)                   ;; -1544829221
(hash (assoc r :a "def"))  ;; -1544829221

3) Needs some tests if they don't already exist:

  • with-meta on a record should not affect hashcode
  • modifying a record with assoc or dissoc should affect hashcode
  • maybe something else?

4) Needs some perf tests with a handful of example records (at least: 1 field, many fields, extmap, etc).

Nicola, I'm happy to let you continue to do dev on this patch with me doing the screening if you have time. Or if you don't have time, let me know and I (or someone else) can work on the dev parts. Would like to get this prepped and clean for 1.8.

Comment by Nicola Mometto [ 16/Jun/15 5:56 PM ]

Updated patch addresses the issues raised, will add some benchmarks later

Comment by Nicola Mometto [ 16/Jun/15 5:59 PM ]

Alex, regarding point 1, I stored __hash and __hasheq as ints rather than longs and compared to -1 rather than 0, to be consistent with how it's done elsewhere in the clojure impl

Comment by Alex Miller [ 17/Jun/15 11:39 AM ]

Looking at the bytecode for hashcode and hasheq, I had two questions:

1) the -1 there is a long and requires an upcast of the private field from int to long. I'm sure that's not a big deal, but wish there was a way to avoid it. I didn't try it but maybe (int -1) would help the compiler out?

2) I wonder whether something like this would perform better:

`(hasheq [this#] 
   (if (== -1 ~'__hasheq)
     (set! ~'__hasheq (int (bit-xor ~type-hash (clojure.lang.APersistentMap/mapHasheq this#)))))
   ~'__hasheq)

The common case will be a failed compare and then the field can be loaded and returned directly without any casting.

Comment by Nicola Mometto [ 17/Jun/15 11:54 AM ]

1- there's no Numbers.equiv(int, int) so even casting -1 to an int wouldn't solve this. a cast is always necessary. if we were to make hasheq a long, we'd need l2i in the return path, making hasheq an int we need an i2l in the comparison.
2- that doesn't remove any casting, it just replaces a load from the local variable stack with a field load:

;; current version
0: ldc2_w        #203                // long -1l
3: aload_0
4: getfield      #236                // Field __hasheq:I
7: i2l
8: lcmp
9: ifne          38
12: ldc2_w        #267                // long 1340417398l
15: aload_0
16: checkcast     #16                 // class clojure/lang/IPersistentMap
19: invokestatic  #274                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
22: i2l
23: lxor
24: invokestatic  #278                // Method clojure/lang/RT.intCast:(J)I
27: istore_1
28: aload_0
29: iload_1
30: putfield      #236                // Field __hasheq:I
33: iload_1
34: goto          42
37: pop
38: aload_0
39: getfield      #236                // Field __hasheq:I
42: ireturn
;; your version
0: ldc2_w        #203                // long -1l
3: aload_0
4: getfield      #236                // Field __hasheq:I
7: i2l
8: lcmp
9: ifne          35
12: aload_0
13: ldc2_w        #267                // long 1340417398l
16: aload_0
17: checkcast     #16                 // class clojure/lang/IPersistentMap
20: invokestatic  #274                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
23: i2l
24: lxor
25: invokestatic  #278                // Method clojure/lang/RT.intCast:(J)I
28: putfield      #236                // Field __hasheq:I
31: goto          37
34: pop
35: aconst_null
36: pop
37: aload_0
38: getfield      #236                // Field __hasheq:I
41: ireturn
Comment by Alex Miller [ 17/Jun/15 12:01 PM ]

Fair enough! Looks pretty good to me, still needs the perf numbers.

Comment by Nicola Mometto [ 17/Jun/15 1:00 PM ]
coll 1.7.0-RC2 1.7.0-RC2+patch
big record ~940ns ~10ns
small record ~150ns ~11ns
;; big record, 1.7.0-RC2
user=> (use 'criterium.core)
nil
user=> (defrecord R [a b c d e f g h i j])
user.R
user=> (def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.291182176566658 % of runtime
Evaluation count : 63385020 in 60 samples of 1056417 calls.
             Execution time mean : 943.320293 ns
    Execution time std-deviation : 44.001842 ns
   Execution time lower quantile : 891.919381 ns ( 2.5%)
   Execution time upper quantile : 1.033894 µs (97.5%)
                   Overhead used : 1.980453 ns

;; big record, 1.7.0-RC2 + patch
user=> (defrecord R [a b c d e f g h i j])
user.R
user=> (def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.0097162582088168 % of runtime
Evaluation count : 4820968380 in 60 samples of 80349473 calls.
             Execution time mean : 10.657581 ns
    Execution time std-deviation : 0.668011 ns
   Execution time lower quantile : 9.975656 ns ( 2.5%)
   Execution time upper quantile : 12.190471 ns (97.5%)
                   Overhead used : 2.235715 ns

;; small record 1.7.0-RC2
user=> (defrecord R [a b])
user.R
user=> (def r  (R. (range 1e3) (range 1e3)))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.456092401467115 % of runtime
Evaluation count : 423779160 in 60 samples of 7062986 calls.
             Execution time mean : 147.154359 ns
    Execution time std-deviation : 8.148340 ns
   Execution time lower quantile : 138.052054 ns ( 2.5%)
   Execution time upper quantile : 165.573489 ns (97.5%)
                   Overhead used : 1.629944 ns

;; small record 1.7.0-RC2+patch
user=> (defrecord R [a b])
user.R
user=> (def r  (R. (range 1e3) (range 1e3)))
#'user/r
user=>  (bench (hash r))
WARNING: Final GC required 1.720638384341818 % of runtime
Evaluation count : 4483195020 in 60 samples of 74719917 calls.
             Execution time mean : 11.696574 ns
    Execution time std-deviation : 0.506482 ns
   Execution time lower quantile : 10.982760 ns ( 2.5%)
   Execution time upper quantile : 12.836103 ns (97.5%)
                   Overhead used : 2.123801 ns
Comment by Alex Miller [ 17/Jun/15 3:36 PM ]

Screened for 1.8.

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

Note that using -1 for the uncomputed hash value can cause issues with transient lazily computed hash codes on serialization (CLJ-1766). In this case, the defrecord cached code is not transient so I don't think it's a problem, but something to be aware of. Using 0 would avoid this potential issue.

Comment by Rich Hickey [ 17/Jul/15 12:00 PM ]

So, why not use 0? You won't need initialization then either

Comment by Nicola Mometto [ 17/Jul/15 7:50 PM ]

Updated patch so that it applies on current master and changed the default hash value from -1 to 0.

Rich, we still need initialization since all the record ctors delegate to the ctor arity with explicit __hash and __hasheq, following the approach of the alt ctors for __extmap and __meta

Comment by Alex Miller [ 17/Jul/15 10:30 PM ]

Moving back to vetted for screening

Comment by Alex Miller [ 28/Jul/15 6:17 PM ]

Hey Nicola, two comments on the hasheq/hashcode impl:

1) I don't think there's any reason to use == in the check instead of =, and = seems better (I think the resulting bytecode is same either way though).

2) The generated bytecode in these cases will call getfield twice in the cached case (once for the check, and once for the return):

public int hasheq();
    Code:
       0: lconst_0      
       1: aload_0       
       2: getfield      #232                // Field __hasheq:I     ;; <-- HERE
       5: i2l           
       6: lcmp          
       7: ifne          36
      10: ldc2_w        #263                // long -989260517l
      13: aload_0       
      14: checkcast     #16                 // class clojure/lang/IPersistentMap
      17: invokestatic  #270                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
      20: i2l           
      21: lxor          
      22: invokestatic  #274                // Method clojure/lang/RT.intCast:(J)I
      25: istore_1      
      26: aload_0       
      27: iload_1       
      28: putfield      #232                // Field __hasheq:I
      31: iload_1       
      32: goto          40
      35: pop           
      36: aload_0       
      37: getfield      #232                // Field __hasheq:I   ;; <-- HERE
      40: ireturn

Letting a local will avoid that:

`(hasheq [this#] (let [hv# ~'__hasheq]     ;; ADDED
                                  (if (= 0 hv#)           ;; USED
                                    (let [h# (int (bit-xor ~type-hash (clojure.lang.APersistentMap/mapHasheq this#)))]
                                      (set! ~'__hasheq h#)
                                      h#)
                                    hv#)))                ;; USED

Output bytecode:

public int hasheq();
    Code:
       0: aload_0       
       1: getfield      #227                // Field __hasheq:I
       4: istore_1      
       5: lconst_0      
       6: iload_1       
       7: i2l           
       8: lcmp          
       9: ifne          38
      12: ldc2_w        #258                // long -989260517l
      15: aload_0       
      16: checkcast     #16                 // class clojure/lang/IPersistentMap
      19: invokestatic  #265                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
      22: i2l           
      23: lxor          
      24: invokestatic  #269                // Method clojure/lang/RT.intCast:(J)I
      27: istore_2      
      28: aload_0       
      29: iload_2       
      30: putfield      #227                // Field __hasheq:I
      33: iload_2       
      34: goto          39
      37: pop           
      38: iload_1       
      39: ireturn

For me, this was about 2% faster in bench too.

Comment by Alex Miller [ 28/Jul/15 6:18 PM ]

Equivalent change in hashCode too.

Comment by Nicola Mometto [ 29/Jul/15 4:06 AM ]

Updated patch takes into account Alex's last notes

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

Added clj-1224-3.patch - identical to prior but uses zero? rather than = 0 at Rich's request. Attribution retained.





[CLJ-1149] Unhelpful error message from :use (and use function) when arguments are malformed Created: 17/Jan/13  Updated: 06/Sep/16  Resolved: 06/Sep/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.4, Release 1.5
Fix Version/s: Release 1.9

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: errormsgs

Approval: Ok

 Description   

the following exception happens when you have something like this(bad):

(ns runtime.util-test
(:use [midje.sweet :reload-all]))

as opposed to any of these(correct):

(ns runtime.util-test
(:use midje.sweet :reload-all))

(ns runtime.util-test
(:use [midje.sweet] :reload-all))

and the exception is:
Exception in thread "main" java.lang.IllegalArgumentException: No value supplied for key: true
at clojure.lang.PersistentHashMap.create(PersistentHashMap.java:77)
at clojure.core$hash_map.doInvoke(core.clj:365)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:617)
at clojure.core$load_lib.doInvoke(core.clj:5352)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:619)
at clojure.core$load_libs.doInvoke(core.clj:5403)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:621)
at clojure.core$use.doInvoke(core.clj:5497)

Note that this is similar to the equally unhelpful message shown in http://dev.clojure.org/jira/browse/CLJ-1140 although that is a different root cause.

Probably best to enhance the `use` function to validate its arguments before trying to apply hash-map?



 Comments   
Comment by Gary Fredericks [ 26/May/13 3:17 PM ]

I believe this applies to require as well.

Comment by Alex Miller [ 19/Aug/16 2:25 PM ]

As of Clojure 1.9.0-alpha11, the spec for ns will catch this and fail at compile-time:

java.lang.IllegalArgumentException: Call to clojure.core/ns did not conform to spec:
In: [1] val: ((:use [midje.sweet :reload-all])) fails at: [:args] predicate: (cat :docstring (? string?) :attr-map (? map?) :clauses :clojure.core.specs/ns-clauses),  Extra input
:clojure.spec/args  (runtime.util-test (:use [midje.sweet :reload-all]))




[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 06/Sep/16  Resolved: 06/Sep/16

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: destructuring, errormsgs

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

 Description   

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



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

I see:

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

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

?

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

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

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

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

As of Clojure 1.9.0-alpha11, the `let` spec catches this and fails at compile time.





[CLJ-2018] clojure.spec.gen lazy-loaded fns should contain wrapped thing's docstring Created: 03/Sep/16  Updated: 06/Sep/16  Resolved: 06/Sep/16

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

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


 Description   

Right now, using clojure.spec.gen's lazily loaded fns is harder than using the test.check versions because the former doesn't have useful docstrings and the latter does.

clojure.spec.gen lazy-loaded fns should contain docstring of the thing they refer to, or at least what it was at compile time, or at least a universally-understood reference to that thing that IDEs can follow. I don't think the latter exists, so perhaps the docstring at compile time should be embedded into the lazy-loaded fn if possible.

(As a general note, and I don't know if my experience generalizes, but I find myself grabbing for stuff in e.g. test.chuck so often that the "no runtime dependency" thing doesn't really work out anyway.)



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

I don't think they can contain those docstrings without actually loading the remote var to get its meta, which defies the whole point of lazy loading. So while I sympathize with the request, I'm not sure that I understand how it is possible to achieve?

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

I was hoping a macro would be able to load the var, but only at compile time (e.g. when building the jar), not affecting a runtime consumer of the same ns; the same way that some of my ClojureScript code is built with macros that read resource files at compile time. (Perhaps that is not technologically possible right now in Clojure.)

Comment by Alex Miller [ 06/Sep/16 11:20 AM ]

I'm going to decline this as I'm not sure how it would be done while still satisfying the other goals of this code. But if someone finds a reasonable way to do it, would reconsider.





[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-2003] Nesting cat inside ? causes unform to return nested result Created: 11/Aug/16  Updated: 01/Sep/16

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

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

Approval: Triaged

 Description   

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

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

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

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

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

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


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

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

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

where I expected

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

The following give expected results:

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

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

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

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





[CLJ-1967] Enhanced namespaced map pprint support Created: 23/Jun/16  Updated: 31/Aug/16  Resolved: 31/Aug/16

Status: Closed
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: Completed Votes: 0
Labels: print

Attachments: Text File clj-1967.patch    
Patch: Code
Approval: Ok

 Description   

CLJ-1910 added namespaced map syntax for reader but did not include pprint support.

user=> (binding [clojure.pprint/*print-right-margin* 40]
         (pprint {:the.namespace/lajsdflkajsd 1 :the.namespace/aljsdfjasdf 2}))
#:the.namespace{:lajsdflkajsd 1,
                :aljsdfjasdf 2}
nil
user=> (binding [clojure.pprint/*print-right-margin* 40
                 *print-namespace-maps* false]
  (pprint {:the.namespace/lajsdflkajsd 1 :the.namespace/aljsdfjasdf 2}))
{:the.namespace/lajsdflkajsd 1,
 :the.namespace/aljsdfjasdf 2}

Patch: clj-1967.patch






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

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

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

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

 Description   

Behavior with Clojure 1.6.0:

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

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

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

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

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

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

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

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

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

Screening Notes

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

Patch: clj-700-9.patch



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

the same is also true for TransientVectors

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

false

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

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

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

nil

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

nil

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

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

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

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

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

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

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

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

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

Latest patch does not apply as of f5bcf647

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

clj-700-rt.patch

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

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

I'd like to look at the type hierarchy myself

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

Workaround: Use ternary get with an acceptable sentinal value:

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




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

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

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

Approval: Triaged

 Description   

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



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

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

There are several ways to call instrument:

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

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

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

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

would expand to

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

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

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

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

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

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

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





[CLJ-2016] function contains? return value error Created: 30/Aug/16  Updated: 30/Aug/16  Resolved: 30/Aug/16

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

Type: Defect Priority: Major
Reporter: wenjixiao Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

ubuntu 14.04



 Description   

hello.core=> (contains? [1 2 3] 1)
true
hello.core=> (contains? [1 2 3] 2)
true
hello.core=> (contains? [1 2 3] 3)
false

i am not sure, it's a bug or not, because it's so simple.
But, [1 2 3] should contains 3 right?!



 Comments   
Comment by Alex Miller [ 30/Aug/16 9:18 AM ]

This is a common question and this is the expected behavior.

contains? checks for containment of a key in an indexed collection. In a map, contains? works on keys. In a set, it works on the (hashed) elements. In a vector, it vector on the vector indexes (not the values).

So asking (contains? [1 2 3] 1) is asking whether there is an element at index 1 in [1 2 3], which there is (here it's 2). When you ask (contains? [1 2 3] 3) you are asking if [1 2 3] has index 3 (0-based), which it does not.

Hope that helps.

Comment by Alex Miller [ 30/Aug/16 9:27 AM ]

Also, more info here http://insideclojure.org/2015/01/27/clojure-tips-contains/





[CLJ-1941] Instrumentation of fns with primitive type hints fails Created: 01/Jun/16  Updated: 29/Aug/16

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

Type: Defect Priority: Minor
Reporter: Kenny Williams Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: spec
Environment:

Ubuntu 15.10
Using boot 2.6.0 on openjdk version "1.8.0_91"


Approval: Triaged

 Description   
(require '[clojure.spec :as s] '[clojure.spec.test :as st])
(defn foo [^double val] val)
(s/fdef foo :args (s/cat :val double?))
(st/instrument `foo)
(foo 5.2)

user=> (foo 5.2)
ClassCastException clojure.spec.test$spec_checking_fn$fn__13069 cannot be cast to clojure.lang.IFn$DO
       	user/eval6 (NO_SOURCE_FILE:5)
       	user/eval6 (NO_SOURCE_FILE:5)
       	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--9704/fn--9707 (main.clj:241)
       	clojure.main/repl/read-eval-print--9704 (main.clj:241)
       	clojure.main/repl/fn--9713 (main.clj:259)
       	clojure.main/repl (main.clj:259)
       	clojure.main/repl-opt (main.clj:323)
       	clojure.main/main (main.clj:422)

Cause: spec replaces var values with instrumented functions that will not work with primitive function interfaces

Approach: Take primitive interfaces into account and make them work, or document/fail that instrumentation will not work with these.



 Comments   
Comment by Kevin Downey [ 02/Jun/16 1:41 AM ]

spec replaces var values with instrumented functions, which works for the default linking case, var deref cast to ifn, invoke, but in the other cases (primitive functions, direct linking, others?) this won't work

Comment by Kenny Williams [ 02/Jun/16 3:39 PM ]

Hmm. Well this should be at least be documented. So, spec cannot be used on functions with a type hinted arg?

Comment by Sean Corfield [ 02/Jun/16 4:16 PM ]

Spec cannot be used on functions with primitive typed hinted arguments or returns – non-primitive type hints seem to be fine.

But documentation isn't enough here: instrumenting a namespace and then discovering it broke a function (that happened to have a primitive type hint) isn't acceptable. If the instrumentation isn't going to work, the function should be skipped (and a warning produced, hopefully).

Comment by Kevin Downey [ 02/Jun/16 8:10 PM ]

yeah, I was giving the root cause of the issue, not excusing the issue.

Understanding the root cause predicts other places where there will be issues: where ever some non-default function linking strategy is used.

One such place is direct linked functions, but I suspect for direct linked functions, Clojure/Core will just say you should only instrument code for testing, and you should only turn on direct liking for production.

Another case, which I am sort of surprised we haven't heard more about yet is protocol functions.

Comment by Sean Corfield [ 02/Jun/16 8:35 PM ]

Your comment about direct linking made me wonder about the validity of spec'ing and instrumenting clojure.core functions. The examples show clojure.core/symbol, but Clojure's core library is shipped as direct linked, as of 1.8.0 isn't it?

Comment by Kevin Downey [ 03/Jun/16 3:14 AM ]

what alters the calling convention isn't the function being compiled with direct linking on, but a caller of that function being compiled with direct linking on.

This code will throw a non-conforming error for the bogus symbol spec with direct linking off, and return the symbol foo with direct linking on

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

(s/fdef symbol
  :args string?
  :ret symbol?)

(defn foo
  []
  (symbol 'foo))

(s/instrument-all)

(foo)
Comment by Kevin Downey [ 03/Jun/16 3:26 AM ]

This code returns true because m is a protocol function, if you replace it with a regular function it throws a non-conforming error

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

(defprotocol P
  (m [_]))

(deftype T []
  P
  (m [_] true))

(s/fdef m
  :args (s/cat :p (constantly false))
  :ret string?)

(defn foo
  []
  (m (T.)))

(s/instrument-all)

(foo)
Comment by Alex Miller [ 13/Jun/16 3:53 PM ]

@Sean instrumenting core functions will work for calls from your code into core (which are presumably not direct-linked), but will not affect calls from one core function to another as they are direct-linked and do not go through the var. One thing we've considered for a long while is building a dev version of core that would not be direct-linked and could potentially turn on instrumentation or other helpful dev-time tooling.

Comment by Sean Corfield [ 14/Jun/16 5:48 PM ]

Thanks for that answer @alexmiller – We have dev set to non-direct-linking but QA / production set to direct linking, so I'm only concerned about possible issues in dev with (s/instrumental-all) and wanting to be sure "code won't break". If instrumentation won't affect existing (direct-linked) calls within core, that's good enough for me. I am concerned about primitive hinting and protocols (and whatever crawls out of the woodwork here) since you don't want to be forced to read the source of every library you include, just to see whether (s/instrument-all) is safe or whether it will bite you in some weird way while you're developing.





[CLJ-1982] Better explain reporting on a failed zero or one match with an embedded spec. Created: 18/Jul/16  Updated: 26/Aug/16

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

Type: Enhancement Priority: Minor
Reporter: Nick Jones Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

OSX, Java 8, Clojure 1.9.0-alpha10


Approval: Triaged

 Description   

Problem:

When attempting to validate a vector containing an optional map, the spec will validate correctly if the vector contains a valid map. If however the optional map does not satisfy the spec misleading error messages are produced. It would be nice if on a partial match of an optional map that some indication of this would be given to the user.

Example REPL session to illustrate problem:

The optional nested map (:optional-nested-map) below fails validation because :nested-element-b is a string instead of an int however the explain report says the spec fails at the parent predicate: :user/vector-schema at: [:element-value] predicate: string?.

It would be more helpful for the user in this case if spec reported that the optional nested map at :optional-nested-map had failed due to ::nested-element-b failing the int? predicate.

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::nested-element-a string?)
:user/nested-element-a
user=> (s/def ::nested-element-b int?)
:user/nested-element-b
user=> (s/def ::nested-element-schema
          (s/keys :opt-un [::nested-element-a ::nested-element-b]))
:user/nested-element-schema
user=> (s/def ::vector-schema
         (s/cat :tag-kw               #{:tag}
                :optional-nested-map  (s/? (s/spec ::nested-element-schema))
                :element-value        string?))
:user/vector-schema
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b 10} "Element"])
true
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
false
user=> (s/explain ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
In: [1] val: {:nested-element-a "bla", :nested-element-b ""} fails spec: :user/vector-schema at: [:element-value] predicate: string?
nil
user=>


 Comments   
Comment by Alex Miller [ 18/Jul/16 7:43 AM ]

Can you update this description with a self-contained example that demonstrates the problem? It's too hard to repro and understand this larger example.

Comment by Nick Jones [ 19/Jul/16 3:30 AM ]

Hi,

Sorry I don't seem to have access to edit the description of the ticket after creation. Here is a simplified sample that I hope will help illustrate the case better.

When the optional nested map below fails validation because :nested-element-b is a string instead of an int the explain report says the spec fails at the parent predicate: :user/vector-schema at: [:element-value] predicate: string?.

As it is an optional map I could see how this would be the case. When no match is found it moves onto the next predicate in the parent.

That said I think it could be helpful (especially in a large optional nested data structure) that if a partial match is achieved that that could be reported to the user as a potential spot for the spec failing.

user=> (require '[clojure.spec :as s])
nil
user=> (s/def ::nested-element-a string?)
:user/nested-element-a
user=> (s/def ::nested-element-b int?)
:user/nested-element-b
user=> (s/def ::nested-element-schema
          (s/keys :opt-un [::nested-element-a ::nested-element-b]))
:user/nested-element-schema
user=> (s/def ::vector-schema
         (s/cat :tag-kw               #{:tag}
                :optional-nested-map  (s/? (s/spec ::nested-element-schema))
                :element-value        string?))
:user/vector-schema
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b 10} "Element"])
true
user=> (s/valid? ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
false
user=> (s/explain ::vector-schema [:tag {:nested-element-a "bla" :nested-element-b ""} "Element"])
In: [1] val: {:nested-element-a "bla", :nested-element-b ""} fails spec: :user/vector-schema at: [:element-value] predicate: string?
nil
user=>
Comment by Nick Jones [ 19/Jul/16 3:45 AM ]

Added simplified version of project archive matching comment at 2016-07-19.

Comment by Alex Miller [ 19/Jul/16 8:27 AM ]

Nick, I've given you edit rights here. Generally, we don't like to have external projects for repro - if you can boil it down to a few line example in the description, that would be ideal.

Comment by Nick Jones [ 19/Jul/16 8:15 PM ]

Thanks Alex. I've updated the description and removed the project attachments. I've also added a REPL session to the description to reproduce the problem in a standalone Clojure 1.9.0-alpha10 REPL.





[CLJ-1965] clojure.spec/def should support an optional doc-string Created: 19/Jun/16  Updated: 26/Aug/16

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

Type: Enhancement Priority: Minor
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 16
Labels: spec

Approval: Triaged

 Description   

Like clojure.core/def clojure.spec/def should support an optional doc string because one usually likes to describe specs in more detail as one could through keyword naming.






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

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

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

alpha11


Approval: Triaged

 Description   

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

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

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

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

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

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

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

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

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

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

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






[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-1980] Unable to construct gen in indirectly recursive specs with s/every and derivations Created: 12/Jul/16  Updated: 26/Aug/16

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

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

alpha-10


Approval: Triaged

 Description   

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

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

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

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

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

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

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

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

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

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






[CLJ-1978] recursion-limit not respected Created: 08/Jul/16  Updated: 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: Maarten Truyens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, spec
Environment:

1.9.0-alpha11


Approval: Triaged

 Description   

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

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

hangs on my machine.

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

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

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

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

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

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

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

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


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

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

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

... immediately generates.

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

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

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

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

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





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

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

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

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

 Description   

Problem

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

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

Possible Solutions

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

Pure Function

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

Impure Function

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



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

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





[CLJ-1936] instrumented fdef with fspec unnecessarily invokes fspec generator Created: 28/May/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: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec, test

Approval: Triaged

 Description   

With test.check is on the classpath, an instrumented fdef with fspec will invoke the generator for the fspec when invoked:

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

(defn foo [fnn] (fnn 42))
(s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
                                     :ret integer?)))

(foo #(do (println %) (when (even? %) 42)))
42
42

(st/instrument `foo)

(foo #(do (println %) (when (even? %) 42)))
-1
0
-1
0
0
-1
0
-1
ExceptionInfo Call to #'user/foo did not conform to spec:
In: [0] val: nil fails at: [:args :f :ret] predicate: integer?
:clojure.spec/args  (#object[user$eval12$fn__13 0x515c6049 "user$eval12$fn__13@515c6049"])
:clojure.spec/failure  :instrument
:clojure.spec.test/caller  {:file "NO_SOURCE_FILE", :line 8, :var-scope user/eval12}
  clojure.core/ex-info (core.clj:4725)

Without test.check, this fails:

user=> (foo #(do (println %) (when (even? %) 42)))
FileNotFoundException Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.  clojure.lang.RT.load (RT.java:458)


 Comments   
Comment by Zach Oakes [ 28/May/16 9:01 PM ]

I think it would make sense to add something like core.typed's ^:no-check for this. For example:

(s/fdef ^:no-check foo :args (s/cat :f (s/fspec :args (s/cat :i integer?) :ret integer?)))

As a stopgap measure, I made a boot task that has a copy of clojure.spec.test/run-all-tests and modifies it to ignore vars with that metadata. That means I have to add it to the metadata in defn rather than fdef but it still seems to work.

Comment by Sean Corfield [ 01/Jun/16 9:32 PM ]

Yes, there are definitely situations where I would want argument / return spec checking on calls during dev/test but absolutely need the function excluded from generative testing.

Comment by Sean Corfield [ 01/Jun/16 9:38 PM ]

If you don't have test.check on your classpath, the call to s/instrument succeeds but then attempting to call foo fails with:

boot.user=> (defn foo [fnn] (fnn 42))
#'boot.user/foo
boot.user=> (s/fdef foo :args (s/cat :f (s/fspec :args (s/cat :i integer?)
       #_=>                                      :ret integer?)))
boot.user/foo
boot.user=> 

boot.user=> (foo #(do (println %) (when (even? %) 42)))
42
42
boot.user=> (s/instrument 'foo)
#'boot.user/foo
boot.user=> (foo #(do (println %) (when (even? %) 42)))

java.io.FileNotFoundException: Could not locate clojure/test/check/generators__init.class or clojure/test/check/generators.clj on classpath.

That is certainly unexpected and not very friendly!

Comment by Alex Miller [ 28/Jun/16 10:05 PM ]

There are new options in instrument as of 1.9.0-alpha8 that allow you to stub/mock functions. Those are one potential answer to this and maybe the recommended one, although I haven't used them enough to say that for sure.

Comment by Alex Miller [ 05/Jul/16 10:52 AM ]

See also CLJ-1976.

Comment by Sean Corfield [ 18/Aug/16 4:08 PM ]

Given that recent Alpha builds no longer check :ret or :fn with instrumentation, this issue seems to be resolved Alex Miller?

Comment by Allen Rohner [ 18/Aug/16 4:22 PM ]

fspec still requires generative testing.

Comment by Sean Corfield [ 18/Aug/16 4:24 PM ]

Ah, OK, I thought that had also rolled back to just :args testing at this point (I hadn't retested this since we have test.check as dev/test dependency now anyway).





[CLJ-2014] (keyword "@type") can be printed, but not read Created: 26/Aug/16  Updated: 26/Aug/16  Resolved: 26/Aug/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2
Fix Version/s: Backlog

Type: Defect Priority: Minor
Reporter: David Smith Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Approval: Incomplete
Waiting On: Rich Hickey

 Description   

user=> (keyword "")
:
user=> (prn-str *1)
":\n"
user=> (read-string *1)
java.lang.RuntimeException: java.lang.Exception: Invalid token: : (NO_SOURCE_FILE:0)

This obviously isn't a huge defect, but I'd argue that anything that can be printed should be readable.



 Comments   
Comment by David Smith [ 26/Aug/16 5:02 AM ]

This is a clone of http://dev.clojure.org/jira/browse/CLJ-732 which it appears was closed with no explanation. I have recently come up against this when deserializing json. IMO it doesn't make sense for the keyword function to be able to produce non-valid keywords. What is the reason for rejecting this?

Comment by Alex Miller [ 26/Aug/16 7:47 AM ]

This is a feature used by a lot of Clojure programs. See:

http://clojure.org/guides/faq#unreadable_keywords

Comment by David Smith [ 26/Aug/16 7:49 AM ]

Thank you for the explanation, this can therefor be closed.





[CLJ-732] (keyword "") can be printed, but not read Created: 26/Jan/11  Updated: 26/Aug/16  Resolved: 29/Jul/11

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2
Fix Version/s: Backlog

Type: Defect Priority: Minor
Reporter: Zach Tellman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: File clj-732.diff    
Approval: Incomplete
Waiting On: Rich Hickey

 Description   

user=> (keyword "")
:
user=> (prn-str *1)
":\n"
user=> (read-string *1)
java.lang.RuntimeException: java.lang.Exception: Invalid token: : (NO_SOURCE_FILE:0)

This obviously isn't a huge defect, but I'd argue that anything that can be printed should be readable.



 Comments   
Comment by Stuart Halloway [ 28/Jan/11 9:06 AM ]

Patch that throws IllegalArgumentException would be ok for this.

Comment by Stuart Halloway [ 05/Apr/11 8:57 PM ]

After a brief review of places that call intern, it appears that this problem might never be reached in the reader, only in user code calling (keyword ...). Does it make more sense to have the patch there, and a matching change for (symbol ...)?

Comment by David Smith [ 26/Aug/16 5:03 AM ]

I have cloned this at http://dev.clojure.org/jira/browse/CLJ-2014 as I came up against this problem. It was quite hard to track down.





[CLJ-1989] `let` ported from `test.check/let` to `clojure.spec.gen` Created: 24/Jul/16  Updated: 23/Aug/16

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

Type: Enhancement Priority: Minor
Reporter: Matthew Wampler-Doty Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Attachments: Text File gen-let.patch    
Patch: Code and Test

 Description   

When using `clojure.spec` for elaborate specifications and `clojure.spec.gen` for generative testing, developers often find themselves writing code which heavily relies on `clojure.spec.gen/fmap`. This is sometimes unnatural and difficult to read.

To make writing custom generators easier, this patch ports `test.check/let` to `clojure.spec.gen`. Now developers can write generators more simply.



 Comments   
Comment by Matthew Wampler-Doty [ 24/Jul/16 5:55 PM ]

For example, if a user wanted to make a generator of vectors with length between 5 and 11 or 20 to 40 elements, consisting of keywords which were either `:a` or `:b`, they would have to write something like:

(gen/fmap (fn [[n gens]] (take n gens)) 
          (gen/tuple (spec/gen (spec/or :short (int-in 5 11) 
                                        :long (int-in 20 40)))
                     (gen/vector (gen/elements #{:a :b}) 40)))

With this patch they could write this as:

(gen/let [length (spec/or :short (int-in 5 11)
                          :long  (int-in 20 40))]
  (repeat length #{:a :b}))




[CLJ-2009] 'symbol and 'keyword turn "" into unreadable symbol and keyword respectively Created: 22/Aug/16  Updated: 23/Aug/16  Resolved: 22/Aug/16

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

Type: Defect Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

In playing with clojure.spec for our upcoming Clojure meetup exercise and I found that (symbol "") returns an empty symbol which is unprintable and unreadable but can still be converted back to an empty string (name (symbol "")) => "". Similarly, (keyword "") returns ":" (which is invalid Clojure and cannot be read) but does round-trip as an object (name (keyword "")) => ""

I'm happy to provide a patch if we can determine the correct behavior. I'll start by making the assertion that the current behavior seems tricksy and prone to cause Great Mystery followed by Great Sadness.



 Comments   
Comment by Alex Miller [ 22/Aug/16 9:32 PM ]

Hey Aaron, symbols and keyword are programmatically constructable (and usable) without being constrained to what the reader can read and the printer can print, and that's often a useful feature (and something we don't consider to be a bug). It is possible that we might in the future have some kind of delimited keyword or symbol (| has been reserved for this) that would allow the reader to read these. I've done some research on this in the past and there were more complexities to it than initially thought so we ended up shelving it, but it's not dead, just in the deep freeze.

I've answered this question so many times that it's silly I haven't put it in the http://clojure.org/guides/faq, so I will do so for the next time.

I think when this comes up, what people most commonly want is some way to avoid making a non-readable keyword or symbol. So some kind of readable-keyword and readable-symbol functions that did validation and either threw an exception or created the keyword or symbol would maybe be a good enhancement idea.

Comment by Aaron Brooks [ 22/Aug/16 10:50 PM ]

Ships passing... I just saw the FAQ update and rechecked here.

The FAQ update is helpful. It might be worth noting in the docstrings of both 'symbol and 'keyword — that they are able to produce keywords and symbols which are unprintable/readable. I do also like the idea of readable-symbol and readable-keyword. Having a defined grammar for those would be great too.

Thanks for taking the time with this.

I am satisfied with my care. ;-D

Comment by Alex Miller [ 22/Aug/16 10:55 PM ]

If you want to file an enhancement for the readable-symbol/keyword, go for it. There is a grammar (kind of) on the http://clojure.org/reference/reader page, but it does not exactly line up with what's actually accepted in the regex, and there are several tickets already out there about the details of that misalignment. I think due to the intent that readable-symbol/keyword could take a conservative approach though and this would be pretty handy.

Comment by Aaron Brooks [ 22/Aug/16 11:03 PM ]

I meant grammar ala [E]BNF or similar. Something you can data rather than human. Also, as you note, the Java regex is pretty loose. I ran into this pretty directly when creating https://github.com/abrooks/clj-chopshop.

I've given some thought about proposing a grammar that would be used by the compiler (or other tools). Would patches for that be generally welcome or are we really attached to the regexes as they are? I understand that there are natural performance and compatibility concerns.

Comment by Alex Miller [ 23/Aug/16 8:23 AM ]

I don't think we're looking to replace the general reader strategy currently in use unless you could demonstrate significantly better performance.





[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-2010] clojure.spec/fdef does not add specs to doc Created: 22/Aug/16  Updated: 22/Aug/16  Resolved: 22/Aug/16

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

Type: Defect Priority: Major
Reporter: lvh Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: spec


 Description   

fdef docstring claims:

Once registered, function specs are included in doc, checked by
instrument, tested by the runner clojure.spec.test/run-tests, and (if
a macro) used to explain errors during macroexpansion.

When specifying an fdef for a fn, (:doc (meta my-fn) does not include any information about the spec, which is what I was expecting.



 Comments   
Comment by Alex Miller [ 22/Aug/16 10:52 PM ]

You are misinterpreting the intent there. What we mean there is that the clojure.repl/doc function will emit the spec as part of its output once you have declared a spec.

There is no intention to update the meta for a var when you declare an fdef spec.





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

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

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

Patch: Code

 Description   

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

;; if-let* imp.

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

;;Example if-let*

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

;;=> 2

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

;;=> 1

;; when-let* imp.

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

;;Example when-let*

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

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





[CLJ-2005] Type hint fails with direct linking disabled Created: 19/Aug/16  Updated: 20/Aug/16

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

Type: Defect Priority: Minor
Reporter: Viktor Magyari Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, directlinking, typehints

Attachments: Text File 0001-CLJ-2005-assoc-arglist-ret-tag-as-tag-in-constructed.patch    
Patch: Code
Approval: Triaged

 Description   

Minimal example, using 1.9.0-alpha11:

user=> (set! *warn-on-reflection* true)
true
user=> (defn foo ^String [^long x] "")
#'user/foo
user=> (.length (foo 10))
Reflection warning, (...) - reference to field length on java.lang.Object can't be resolved.
0

The warning is present only if direct linking is disabled.

Explanation:
this is another manifestation of CLJ-1533 – because of the lexical transformation the compiler is doing when routing the invoke through invokePrim, the arglists type hints are lost. This doesn't happen when DL is on because invokeStatic isn't compiled via a lexical transformation but through StaticInvokeExpr which properly tracks the original var's type hints

Patch: 0001-CLJ-2005-assoc-arglist-ret-tag-as-tag-in-constructed.patch



 Comments   
Comment by Nicola Mometto [ 19/Aug/16 5:24 PM ]

With DL on:

public static java.lang.Object invokeStatic();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=2, locals=0, args_size=0
         0: ldc2_w        #12                 // long 10l
         3: invokestatic  #18                 // Method test$foo.invokeStatic:(J)Ljava/lang/Object;
         6: checkcast     #20                 // class java/lang/String
         9: invokevirtual #24                 // Method java/lang/String.length:()I
        12: invokestatic  #30                 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
        15: areturn
      LineNumberTable:
        line 5: 0
        line 5: 9

with DL off:

public static java.lang.Object invokeStatic();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=3, locals=0, args_size=0
         0: getstatic     #15                 // Field const__0:Lclojure/lang/Var;
         3: invokevirtual #20                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
         6: checkcast     #22                 // class clojure/lang/IFn$LO
         9: ldc2_w        #23                 // long 10l
        12: invokeinterface #28,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
        17: ldc           #30                 // String length
        19: iconst_0
        20: invokestatic  #36                 // Method clojure/lang/Reflector.invokeNoArgInstanceMember:(Ljava/lang/Object;Ljava/lang/String;Z)Ljava/lang/Object;
        23: areturn
      LineNumberTable:
        line 5: 0
        line 5: 12
        line 5: 17
Comment by Nicola Mometto [ 19/Aug/16 5:43 PM ]

bytecode with DL off and current patch:

public static java.lang.Object invokeStatic();
    descriptor: ()Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=3, locals=0, args_size=0
         0: getstatic     #15                 // Field const__0:Lclojure/lang/Var;
         3: invokevirtual #20                 // Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
         6: checkcast     #22                 // class clojure/lang/IFn$LO
         9: ldc2_w        #23                 // long 10l
        12: invokeinterface #28,  3           // InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
        17: checkcast     #30                 // class java/lang/String
        20: invokevirtual #34                 // Method java/lang/String.length:()I
        23: invokestatic  #40                 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
        26: areturn
      LineNumberTable:
        line 5: 0
        line 5: 12
        line 5: 20




[CLJ-1029] ns defmacro allows arbitrary execution of clojure.core fns Created: 23/Jul/12  Updated: 19/Aug/16  Resolved: 19/Aug/16

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.2, Release 1.3, Release 1.4
Fix Version/s: Release 1.9

Type: Defect Priority: Minor
Reporter: Craig Brozefsky Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: error-reporting
Environment:

all


Attachments: File ns-patch.diff    
Patch: Code
Approval: Ok

 Description   

The form:

(ns foo (:print "I AM A ROBOT"))

will print "I AM A ROBOT"

This is because the defmacro takes the name of the first element of the reference, looks it up in the clojure.core namespace and invokes it on the rest of the args.

This is minor, but it does mean that an otherwise declarative form is not executing code.



 Comments   
Comment by Alan Malloy [ 25/Jul/12 4:37 PM ]

One apparent problem with this patch is that you throw an exception for :refer. You should add that, and make sure there aren't any others missing. Also, #{x y z} is better than (set [x y z]), and you should probably use pr-str rather than str, although I can't think of a case where it matters for the objects in question.

Comment by Andy Fingerhut [ 26/Jul/12 6:31 PM ]

A more minor detail of patch formatting – please attach your patch in git format. See the instructions under the section heading "Development" on this web page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Craig Brozefsky [ 05/Aug/12 9:53 AM ]

git format-patch version of the diff, with the edits suggested by other maintainers.

Comment by Craig Brozefsky [ 05/Aug/12 10:00 AM ]

Alan: please note that :refer was not mentioned in the docstring for ns, or used in any of the unit tests for clojure.

Are you sure that it is an expected argument, or just an arrangement that happens to work under the current ns macro? The docstring for 'refer itself says to use :use in ns macros instead of calling refer.

I added "refer" to the set of accepted references all the same.

Comment by Alex Miller [ 18/Jan/16 3:33 PM ]

This is a case where better error checking would prevent this problem.

Comment by Alex Miller [ 19/Aug/16 2:24 PM ]

As of 1.9.0-alpha11 the spec for ns catches and rejects this invalid use of ns.





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

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

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

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

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

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

It will report:

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

However, if is written:

(defn foo "bla")

error report makes more sense:

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


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

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

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

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

Comment by Alex Miller [ 19/Aug/16 2:21 PM ]

Due to the new specs for `defn` in Clojure 1.9.0-alpha11, both of these fail with more informative errors:

user=> (defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))
CompilerException java.lang.IllegalArgumentException: Call to clojure.core/defn did not conform to spec:
In: [2] val: (let [i 1] (+ i 1)) fails spec: :clojure.core.specs/arg-list at: [:args :bs :arity-1 :args] predicate: vector?
In: [2 0] val: let fails spec: :clojure.core.specs/arg-list at: [:args :bs :arity-n :bodies :args] predicate: vector?
:clojure.spec/args  (foo "This is docstring." (let [i 1] (+ i 1)))
, compiling:(NO_SOURCE_PATH:5:1)

user=> (defn foo "bla")
CompilerException java.lang.IllegalArgumentException: Call to clojure.core/defn did not conform to spec:
val: () fails spec: :clojure.core.specs/defn-args at: [:args :bs] predicate: (alt :arity-1 :clojure.core.specs/args+body :arity-n (cat :bodies (+ (spec :clojure.core.specs/args+body)) :attr (? map?))),  Insufficient input
:clojure.spec/args  (foo "bla")
, compiling:(NO_SOURCE_PATH:9:1)

Any further changes in this will come through changes in either the specs or spec error reporting, so I am closing this.





[CLJ-1914] Range realization has a race during concurrent execution Created: 14/Apr/16  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: range

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

 Description   

When a range instance is enumerated via next concurrently, some threads may not see the entire range sequence. When this happens next will return nil prematurely.

(defn enumerate [r]
 (loop [rr r
        c []]
  (let [f (first rr)]
   (if f
    (recur (next rr) (conj c f))
    c))))

(defn demo [size threads]
 (let [r (range size)
       futures (doall (repeatedly threads #(future (enumerate r))))
       res (doall (map deref futures))]
  (if (apply = res)
   (println "Passed")
   (println "Failed, counts=" (map count res)))))

(demo 300 4)
Failed, counts= (300 64 300 64)

The demo above will reliably produce a failure every few executions like the one above.

The vast majority of the time, range is used either single-threaded or in a non-competing way where this scenario will never happen. This failure only occurs when two or more threads are enumerating rapidly through the same range instance.

Cause:

Each LongRange instance acts in several capacities - as a seq, a chunked seq, and a reducible, all of which represent independent enumeration strategies (multiple of which may be used by the user). LongRange holds 2 pieces of (volatile, non-synchronized) state related to chunking: _chunk and _chunkNext. That state is only updated in forceChunk(). forceChunk() uses the "racy single-check idiom" to tolerate multiple threads racing to create the chunk. That is, multiple threads may detect that the chunk has not been set (based on null _chunk) and BOTH threads will create the next chunk and write it. But both threads have good local values, compute the same next value, and set the same next values in the fields, so the order they finish is unimportant.

The problem here is that there are actually two fields, and they are set in the order _chunk then _chunkNext. Because the guard is based on _chunk, it's possible for a thread to think the chunk values have been set but _chunkNext hasn't yet been set.

Approach:

Moving the set for _chunkNext before the set for _chunk removes that narrow window of race opportunity.

Patch: clj-1914-3.patch

Thanks to Kyle Kingsbury for the initial reproducing case https://gist.github.com/aphyr/8746181beeac6a728a3aa018804d56f6



 Comments   
Comment by Alex Miller [ 14/Apr/16 11:13 PM ]

It's not necessary to synchronize here - just swapping these two lines should address the race I think: https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L131-L132

Comment by Ghadi Shayban [ 14/Apr/16 11:22 PM ]

Good call. That's super subtle.

It appears all mutable assignment occurs in forceChunk() except for https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LongRange.java#L145 which should be OK (famous last words).

Comment by Stuart Halloway [ 27/Jul/16 3:27 PM ]

This ticket would benefit from an explanation of the failure mode caused by the race, plus an explanation of the subtle reasoning behind the fix.

Comment by Alex Miller [ 27/Jul/16 4:27 PM ]

Updated per request.





[CLJ-1870] Reloading a defmulti nukes metadata on the var Created: 22/Dec/15  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: ft, metadata, multimethods

Attachments: Text File 0001-CLJ-1870-don-t-destroy-defmulti-metadata-on-reload.patch    
Patch: Code
Approval: Ok

 Description   

Reloading a defmulti expression destroys any existing (or new) metadata on that multimethod's var:

user=> (defmulti foo "docstring" :op)
#'user/foo
user=> (-> #'foo meta :doc)
"docstring"
user=> (defmulti foo "docstring" :op)
nil
user=> (-> #'foo meta :doc)
nil

This is highly problematic for tools.analyzer, since it relies on such metadata to convey information to the pass scheduler about pass dependencies.

This means that any code that uses core.async cannot be reloaded using `require :reload-all`, since it will cause tools.analyzer to reload and the passes to scheduled in a random order. See ASYNC-154 for one example.

Cause: defmulti has defonce semantics and the first def does not re-apply meta.

Approach: Re-apply meta before first def.

After patch:

user=> (defmulti foo "docstring" :op)
#'user/foo
user=> (-> #'foo meta :doc)
"docstring"
user=> (defmulti foo "docstring" :op)
nil
user=> (-> #'foo meta :doc)
"docstring"

Patch: 0001-CLJ-1870-don-t-destroy-defmulti-metadata-on-reload.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 23/Dec/15 10:16 AM ]

Related: CLJ-900 CLJ-1446

Comment by Alex Miller [ 23/Dec/15 10:42 AM ]

This patch doesn't compile for me: RuntimeException: Unable to resolve symbol: mm in this context, compiling:(clojure/core.clj:1679:17)

Also, please build the patch with more context - use -U10 with git format-patch to expand it.

Comment by Nicola Mometto [ 23/Dec/15 11:06 AM ]

Updated patch fixing the typo & using -U10

Comment by Alex Miller [ 23/Dec/15 11:17 AM ]

Now:

java.lang.RuntimeException: Too many arguments to def, compiling:(clojure/core.clj:3561:1)

Comment by Nicola Mometto [ 23/Dec/15 11:22 AM ]

whoops. Sorry for this, here's the updated (and working) patch

Comment by Alex Miller [ 19/Aug/16 10:58 AM ]

patch doesn't apply with new def spec, needs another look

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

I had an old version of the patch locally - nvm!





[CLJ-1423] Applying a var to an infinite arglist consumes all available memory Created: 15/May/14  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: performance

Attachments: Text File apply-var.patch     Text File clj-1423.patch    
Patch: Code and Test
Approval: Ok

 Description   

It is possible to apply a function to an infinite argument list: for example, (apply distinct? (repeat 1)) immediately returns false, after realizing just a few elements of the infinite sequence (repeat 1). However, (apply #'distinct? (repeat 1)) attempts to realize all of (repeat 1) into memory at once.

This happens because Var.applyTo delegates to AFn.applyToHelper to decide which arity of Var.invoke to dispatch to; but AFn doesn't expect infinite arglists (mostly those use RestFn). So it uses RT.seqToArray, which doesn't work well in this case.

Instead, Var.applyTo(args) can just dispatch to deref().applyTo(args), and let the function being stored figure out what to do with the arglist.

I've changed Var.applyTo to do this, and added a test (which fails before my patch is applied, and passes afterwards).

Patch: clj-1423.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 18/Jan/16 5:29 PM ]

I also did a quick perf test on this:

(dotimes [x 20] (time (dotimes [y 10000] (apply #'+ (range 100)))))

which showed times ~82 ms per rep before the patch and ~10 ms per rep after the patch (on par with applying the function directly).

I added a new patch that squashes the commits, adds the ticket number to the commit message, attribution was retained.





[CLJ-1744] Unused destructured local not cleared, causes memory leak Created: 03/Jun/15  Updated: 19/Aug/16  Resolved: 19/Aug/16

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 17
Labels: compiler, destructuring, locals-clearing, memory

Attachments: Text File 0001-CLJ-1744-clear-unused-locals.patch     Text File 0001-CLJ-1744-clear-unused-locals-v2.patch    
Patch: Code
Approval: Ok

 Description   

Clojure currently doesn't clear unused locals. This is problematic as some form of destructuring can generate unused/unusable locals that the compiler cannot clear and thus can cause head retention:

;; this works
user=> (loop [xs (repeatedly 2 #(byte-array (quot (.maxMemory (Runtime/getRuntime)) 10)))] (when (seq xs) (recur (rest xs))))
nil
;; this doesn't
user=>  (loop [[x & xs] (repeatedly 200 #(byte-array (quot (.maxMemory (Runtime/getRuntime)) 10)))] (when (seq xs) (recur xs)))
OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1252)

Here's a macroexpansion that exposes this issue:

user=> (macroexpand-all '(loop [[a & b] c] [a b]))
(let* [G__21 c 
       vec__22 G__21
       a (clojure.core/nth vec__22 0 nil)
       b (clojure.core/nthnext vec__22 1)]
 (loop* [G__21 G__21]
   (let* [vec__23 G__21
          a (clojure.core/nth vec__23 0 nil)
          b (clojure.core/nthnext vec__23 1)]
     [a b])

Cause: The first two bindings of a and b will hold onto the head of c since they are never used and not accessible from the loop body they cannot be cleared.

Approach: Track whether local bindings are used. After evaluating the binding expression, if the local binding is not used and can be cleared, then pop the result rather than storing it.

Patch: 0001-CLJ-1744-clear-unused-locals-v2.patch

Screened by: Alex Miller



 Comments   
Comment by Michael Blume [ 03/Jun/15 12:57 PM ]

Nice =)

Comment by James Henderson [ 18/Nov/15 6:12 AM ]

FYI - we've hit this as a memory leak in our production system:

(defn write-response! [{:keys [products merchant-id] :as search-result} writer response-type]
  ;; not using `search-result` throughout this fn - kept in to document intent
  ;; hangs on to `products`, a large lazy-seq, until it's completely consumed, causes memory leak

  ...
  (case response-type
    "application/edn" (print-method products writer)
    ...))
(defn write-response! [{:keys [products merchant-id]} writer response-type]
  ;; fine, releases earlier elements in products as it flies through the response

  ...
  (case response-type
    "application/edn" (print-method products writer)
    ...))

The work-around in our case is easy enough - removing the unused symbol - but, given we assumed including an unused symbol would be a no-op, it did take us a while to find!

Cheers,

James

Comment by Nicola Mometto [ 15/Dec/15 11:47 AM ]

While investigating an unrelated memory leak on core.async (ASYNC-138) I discovered that this bug also affects code inside a `go` macro, since it often emits unreachable bindings

clojure.core.async> (macroexpand '(go (let [test (range 1e10)]
                                        (str test (<! (chan))))))
(let*
 [c__15123__auto__ (clojure.core.async/chan 1) captured-bindings__15124__auto__ (clojure.lang.Var/getThreadBindingFrame)]
 (clojure.core.async.impl.dispatch/run
  (fn*
   []
   (clojure.core/let
    [f__15125__auto__
     (clojure.core/fn
      state-machine__14945__auto__
      ([] (clojure.core.async.impl.ioc-macros/aset-all! (java.util.concurrent.atomic.AtomicReferenceArray. 9) 0 state-machine__14945__auto__ 1 1))
      ([state_17532]
       (clojure.core/let
        [old-frame__14946__auto__
         (clojure.lang.Var/getThreadBindingFrame)
         ret-value__14947__auto__
         (try
          (clojure.lang.Var/resetThreadBindingFrame (clojure.core.async.impl.ioc-macros/aget-object state_17532 3))
          (clojure.core/loop
           []
           (clojure.core/let
            [result__14948__auto__
             (clojure.core/case
              (clojure.core/int (clojure.core.async.impl.ioc-macros/aget-object state_17532 1))
              1
              (clojure.core/let
               [inst_17525 (clojure.core.async.impl.ioc-macros/aget-object state_17532 7) inst_17525 (range 1.0E10) test inst_17525 inst_17526 str test inst_17525 inst_17527 (chan) state_17532 (clojure.core.async.impl.ioc-macros/aset-all! state_17532 7 inst_17525 8 inst_17526)]
               (clojure.core.async.impl.ioc-macros/take! state_17532 2 inst_17527))
              2
              (clojure.core/let
               [inst_17525 (clojure.core.async.impl.ioc-macros/aget-object state_17532 7) inst_17526 (clojure.core.async.impl.ioc-macros/aget-object state_17532 8) inst_17529 (clojure.core.async.impl.ioc-macros/aget-object state_17532 2) inst_17530 (inst_17526 inst_17525 inst_17529)]
               (clojure.core.async.impl.ioc-macros/return-chan state_17532 inst_17530)))]
            (if (clojure.core/identical? result__14948__auto__ :recur) (recur) result__14948__auto__)))
          (catch java.lang.Throwable ex__14949__auto__ (clojure.core.async.impl.ioc-macros/aset-all! state_17532 clojure.core.async.impl.ioc-macros/CURRENT-EXCEPTION ex__14949__auto__) (clojure.core.async.impl.ioc-macros/process-exception state_17532) :recur)
          (finally (clojure.lang.Var/resetThreadBindingFrame old-frame__14946__auto__)))]
        (if (clojure.core/identical? ret-value__14947__auto__ :recur) (recur state_17532) ret-value__14947__auto__))))
     state__15126__auto__
     (clojure.core/-> (f__15125__auto__) (clojure.core.async.impl.ioc-macros/aset-all! clojure.core.async.impl.ioc-macros/USER-START-IDX c__15123__auto__ clojure.core.async.impl.ioc-macros/BINDINGS-IDX captured-bindings__15124__auto__))]
    (clojure.core.async.impl.ioc-macros/run-state-machine-wrapped state__15126__auto__))))
 c__15123__auto__)




[CLJ-1993] Print flag to suppress namespace map syntax Created: 28/Jul/16  Updated: 19/Aug/16  Resolved: 16/Aug/16

Status: Closed
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: Completed Votes: 0
Labels: print

Attachments: Text File clj-1993-2.patch     Text File clj-1993.patch    
Patch: Code and Test
Approval: Ok

 Description   

Add new print flag *print-namespace-maps* to optionally suppress namespace map syntax. Default flag to false.

Set flag to true as part of the standard REPL bindings. This allows repl users to (set! *print-namespace-maps* false) if desired.

Patch: clj-1993-2.patch



 Comments   
Comment by Rich Hickey [ 16/Aug/16 7:33 AM ]

the plan/approach should be somewhere in the description and not just the code please. Also, I wonder if the default should be false other than at the REPL?

Comment by Alex Miller [ 16/Aug/16 6:53 PM ]

Committed for next alpha





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

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

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

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

 Description   

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

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

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



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

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





[CLJ-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-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 19/Aug/16

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

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

All


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

 Description   

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

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


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

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

Comment by Yehonathan Sharvit [ 19/Aug/16 10:19 AM ]

for the sake of symmetry with `assoc` I'd love to see this ticket fixed





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

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

Type: Defect Priority: Trivial
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None


 Description   

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

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

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

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

Further investigation shows that foo/inc is unbound:

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

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

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



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

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

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

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

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

See comment...

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

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

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

Appears to have been closed prematurely

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

I can't reproduce with the correct syntax:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Andy,

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

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

This comment is really most relevant for ticket CLJ-1604, where it has been copied:

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 25/Nov/14 3:44 PM ]

Tom, I've opened a ticket with a patch fixing the AOT issue: http://dev.clojure.org/jira/browse/CLJ-1604

Comment by Kevin Downey [ 03/Jun/16 5:09 AM ]

the consequences of this bug can be very hard to track back to this bug. it would be really nice to get it fixed in someway.

(defmulti update identity)
... pages of other code ...
(defmethod update :foo [_])

will throw a compiler error on the defmethod saying update is unbound

Comment by Sean Corfield [ 18/Aug/16 4:02 PM ]

This looks very similar to the bug I ran into recently with Encore and an early Alpha of Clojure 1.9.0 where a new core predicate had been introduced, causing a warning from Encore which defined a function with the same name – but use of the code produced an error that the var was unbound. In that case it was a series of defn forms inside a do form – done to put a reader conditional around a group of function definitions. I lifted the conflicting functions out of the do and the error went away (but the warning remained, as expected). You can see the PR I submitted to Encore showing the code rearrangement: https://github.com/ptaoussanis/encore/pull/26/commits/040bf1be99eee79cbbcb7cc10ed37aa0a1e7ec17

Adding those functions to :refer-clojure :exclude instead solved the problem "properly" (and made the warning go away, obviously).





[CLJ-1142] Incorrect divide-by-zero error with floating point numbers Created: 08/Jan/13  Updated: 18/Aug/16

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: math

Approval: Triaged

 Description   

The unary call for clojure.core// treats a dividend of 0.0 differently than the binary call, likely due to inlining.

(/ 0.0) ;; java.lang.ArithmeticException: Divide by zero
(/ 1 0.0) ;;= Infinity
(/ 1 (identity 0.0)) ;; java.lang.ArithmeticException: Divide by zero


 Comments   
Comment by Tim McCormack [ 08/Jan/13 11:22 PM ]

The relevant code seems to be this in clojure.lang.Numbers/divide:

if(yops.isZero((Number)y))
  throw new ArithmeticException("Divide by zero");

Making Numbers/divide be more restrictive than double arithmetic seems like a bug; explicitly throwing an ArithmeticException instead of letting the JVM figure it just seems like more work than necessary.

Comment by Casey Marshall [ 17/Aug/16 11:17 PM ]

I think the issue here is that in clojure.lang.Numbers, different variants of divide have different semantics. Numbers.divide(Object, Object) performs the isZero check, but Numbers.divide(long, double) does not (it just uses Java's division operator, which since the denominator is a double with value 0.0, produces Infinity).

A statement like (/ 1 0.0) gets compiled to call Numbers.divide(long, double), and thus produces Infinity. If the second argument is a function call or a var, it looks like an Object, so it gets compiled to use Numbers.divide(Object, Object), and that call throws when the second arg is zero (actually it compiles to a call to Numbers.divide(long, Object), but that just boxes the first argument and calls the other variant).

It does seem incorrect to have different semantics for division based on the inferred type at compile time; however, I don't know if this affects any other instance of division except divide-by-zero, so it's possibly not a practical problem.

Comment by Tim McCormack [ 18/Aug/16 6:48 AM ]

I don't know if this affects any other instance of division except divide-by-zero, so it's possibly not a practical problem.

I regard code that only fails sometimes as worse, because then bugs are more likely to get caught in production instead of development.





[CLJ-1985] with-gen of conformer loses unform fn Created: 21/Jul/16  Updated: 16/Aug/16  Resolved: 16/Aug/16

Status: Closed
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: Completed Votes: 1
Labels: spec

Attachments: Text File conformer-with-gen.patch    
Patch: Code and Test
Approval: Ok

 Description   
(def ex (s/conformer inc dec))
(s/conform ex 1) ;; 2
(s/unform ex 2)  ;; 1
(def exc
  (s/with-gen
    (s/conformer inc dec)
    (fn [] (s/gen int?))))
(s/conform exc 1) ;; 2
(s/unform exc 2) ;; fails, no unformer

Cause: with-gen doesn't re-apply the unform fn to the new spec

Patch: conformer-with-gen.patch



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

Committed for next alpha.





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

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

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

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

 Description   

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

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

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

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

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

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

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

Screened by: Alex Miller



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

Possibly related: CLJ-1457

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

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

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

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

>cat error.sh
#!/bin/sh

rm -rf target && mkdir target

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

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

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

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

> ./error.sh
Exception in thread "main" java.lang.ExceptionInInitializerError
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.classForName(RT.java:2113)
	at clojure.lang.RT.classForName(RT.java:2122)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:630)
	at clojure.core$use.doInvoke(core.clj:5785)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval212.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Compiler.eval(Compiler.java:6767)
	at clojure.lang.Compiler.eval(Compiler.java:6730)
	at clojure.core$eval.invoke(core.clj:3076)
	at clojure.main$eval_opt.invoke(main.clj:288)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.io.FileNotFoundException: Could not locate myrecord__init.class or myrecord.clj on classpath.
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5403.invoke(core.clj:5808)
	at clojure.core$load.doInvoke(core.clj:5807)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5352.invoke(core.clj:5653)
	at clojure.core$load_lib.doInvoke(core.clj:5652)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5691)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5774)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at core__init.load(Unknown Source)
	at core__init.<clinit>(Unknown Source)
	... 33 more

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

This way, both under JIT and AOT code like

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

will fail with a circular depdenency error.

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

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

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

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

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

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

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

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





[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-1044] Enable refering to ->type inside deftype Created: 18/Aug/12  Updated: 09/Aug/16

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

Type: Enhancement Priority: Trivial
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: deftype

Attachments: File 001-enable-factory-ctor-inside-deftype.diff    
Patch: Code
Approval: Triaged

 Description   

Inside a defrecord body it's possible to refer to ->type-ctor but that is not possible inside deftype.

This patch adds an implicit declare, as done in defrecord making it possible to use the ->type-ctor inside deftype methods



 Comments   
Comment by Timothy Baldridge [ 03/Dec/12 11:29 AM ]

Seems valid. Vetting.

Comment by Sam Estep [ 09/Aug/16 10:34 AM ]

Will this be incorporated soon? It's awkward to have to explicitly declare the factory function when defining, e.g., data structures (example). Also, the current situation violates the principle of least astonishment; while working through this tutorial, I was quite surprised to find that defrecord does implicitly declare the factory functions, which contradicted my prior experience with deftype.

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

No enhancements are considered critical so it's hard for me to say when this will get evaluated. I've bumped it one step down the process at least.

Comment by Sam Estep [ 09/Aug/16 7:39 PM ]

Thank you, Alex! I completely understand that this isn't a particularly important issue; nonetheless, it is encouraging to see it move closer to being fixed.





[CLJ-2000] (transduce) does an unexpected extra step Created: 06/Aug/16  Updated: 07/Aug/16  Resolved: 07/Aug/16

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

Type: Defect Priority: Minor
Reporter: Vadim Liventsev Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

https://stackoverflow.com/questions/38809642/applying-a-transducer-directly-and-with-transduce-yield-different-results/38809928#38809928

(reduce) calls (reducer aggregate element) for every element in the collection. A total of n calls for a collection of n elements.

(transduce) calls (reducer aggregate element) for every element and then for some reason calls (reducer aggregate) again, making n+1 calls. As a result, (transduce) doesn't work as expected with .

Is it a bug?



 Comments   
Comment by Kevin Downey [ 06/Aug/16 10:21 PM ]

this is a feature of transduce. the docstring for transduce says "f should be a reducing step function that accepts both 1 and 2 arguments, if it accepts only 2 you can add the arity-1 with 'completing'." which, if you know about the completing arity of reducing functions clues you in, but if you don' the docstring is not very helpful about it.

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

As Kevin said in the comments, the question assumes things that are not accurate. Everything here is working as expected/designed.





Generated at Sat Oct 01 15:48:24 CDT 2016 using JIRA 4.4#649-r158309.