[CLJ-2456] Clojure-aware error stacktrace rendering Created: 15/Dec/18  Updated: 15/Dec/18

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

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

Approval: Vetted

 Description   

Currently (1.10.0-RC5) stacktraces look like this:

user=> (Exception.)
#error {
 :cause nil
 :via
 [{:type java.lang.Exception
   :message nil
   :at [user$eval3 invokeStatic "NO_SOURCE_FILE" 1]}]
 :trace
 [[user$eval3 invokeStatic "NO_SOURCE_FILE" 1]
  [user$eval3 invoke "NO_SOURCE_FILE" 1]
  [clojure.lang.Compiler eval "Compiler.java" 7176]
  [clojure.lang.Compiler eval "Compiler.java" 7131]
  [clojure.core$eval invokeStatic "core.clj" 3214]
  [clojure.core$eval invoke "core.clj" 3210]
  [clojure.main$repl$read_eval_print__9068$fn__9071 invoke "main.clj" 414]
  [clojure.main$repl$read_eval_print__9068 invoke "main.clj" 414]
  [clojure.main$repl$fn__9077 invoke "main.clj" 435]
  [clojure.main$repl invokeStatic "main.clj" 435]
  [clojure.main$repl_opt invokeStatic "main.clj" 499]
  [clojure.main$main invokeStatic "main.clj" 598]
  [clojure.main$main doInvoke "main.clj" 561]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.RestFn applyTo "RestFn.java" 132]
  [clojure.lang.Var applyTo "Var.java" 705]
  [clojure.main main "main.java" 37]]}

Those are java stacktraces, even for code running inside Clojure. I believe many developers would benefit from stacktraces elements originated from Clojure being rendered in Clojure-aware way. For example:

[clojure.core/eval "core.clj" 3214]
  [clojure.main/repl/read-eval-print "main.clj" 414]
  [clojure.main/repl "main.clj" 435]
  [clojure.main/repl-opt "main.clj" 499]
  [clojure.main/main "main.clj" 598]
  [clojure.main/main "main.clj" 561]

Key differences:

  • Names demunged (user sees names as they appear in Clojure file, not how they got compiled to Java code. Not every Clojure user need to be aware of that part, I believe)
  • Duplicate invoke/invokeStatic/doInvoke collapsed (again, this is an implementation detail, from Clojure user calling a method is a single operation. As a result, many stacktraces might become quite shorter)

I believe there are many more implementation details like protocol calls, multimethod calls, anonymous functions, where compilation details spill into stacktraces, blow their size and make them only readable by compiler experts. Although not specified in this ticket, those should be collapsed to clojure-friendly representation as well.

There are also many places where such representation should be applied:

  • Default exception printer
  • clojure.main
  • clojure.repl
  • clojure.stacktrace (used by clojure.test)

Also, maybe all those places need some sort of unification? E.g. why are there three (root-cause) implementations in clojure.core?

Prior work: I believe (pst) does some basic demunging, but not collapsing. It also doesn't handle complex cases of protocols etc. And it has to be explicitly called, which is not very convenient.






[CLJ-2348] [spec] 'check' has inconsistent behavior for required and optional map keys Created: 16/Apr/18  Updated: 15/Dec/18

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

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

org.clojure/spec.alpha "0.1.143"


Approval: Triaged

 Description   

Repro:

(s/def :ex/f fn?)
(s/def :ex/m (s/keys :opt [:ex/f]))
(s/fdef my-fn
        :args (s/cat :m :ex/m))
(defn my-fn [f])
(clojure.spec.test.alpha/check `my-fn)

Actual: Exception is thrown - "Unable to construct gen at: [:m :ex/f] for: :ex/f"

Expected: A value should be returned containing the failure. This is the behavior that will occur if you replace the ":opt" with a ":req" in the keys spec.

I would expect this value to contain a failure such that:

(ex-data (:result (:clojure.spec.test.check/ret (first (clojure.spec.test.alpha/check `my-fn))))) ;; => #:clojure.spec.alpha{:path [:m :ex/f], :form :ex/f, :failure :no-gen}





[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 14/Dec/18

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: android, graal
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch     Text File clj-1472-2.patch     Text File clj-1472-3.patch     Text File clj-1472.patch    
Patch: Code
Approval: Screened

 Description   

Android ART runs compile time verification on bytecode and fails on any usage of the locking macro. The error looks like that seen in CLJ-1829 (in that case clojure.core.server/stop-server calls the locking macro):

10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: java.lang.VerifyError: Rejecting class clojure.core.server$stop_server because it failed compile-time verification (declaration of 'clojure.core.server$stop_server' appears in /data/app/com.clojure_on_android-1/base.apk)

Cause: From discussion on an Android issue (https://code.google.com/p/android/issues/detail?id=80823), it seems this is due to more strictly enforcing the "structural locking" provisions in the JVM spec (https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10).

It appears that the mixture of monitor-enter, monitor-exit, and the try/finally block in `locking` create paths that ART is flagging as not having balanced monitorenter/monitorexit bytecode. Particularly, monitorenter and monitorexit themselves can throw (on a null locking object). The Java bytecode does some tricky exception table handling to cover these cases which (afaict) is not possible to do without modifying the Clojure compiler.

Approach: One possible approach is to have locking invoke the passed body in the context of a synchronized block in Java. This avoids the issue of the tricky bytecode by handing it over to Java but has unknown performance impacts.

Patch: clj-1472-3.patch

See also: Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470

Screened by: Alex Miller - I'm marking this screened as I think it is a viable approach that fixes the issue and due to the infrequency of its use, I'm not really that concerned about it being a performance problem. I will flag that I think another way to handle this would be to make `locking` a special form with compiler support, but I'm not sure whether that's worth doing or not, so I will leave that to Rich to decide.



 Comments   
Comment by Adam Clements [ 24/Jul/14 11:17 AM ]

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Comment by Andy Fingerhut [ 01/Aug/14 9:08 PM ]

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing

Comment by Adam Clements [ 04/Aug/14 4:24 PM ]

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.

Comment by Adam Clements [ 25/Nov/14 8:31 AM ]

Have just tested with Lollipop, and this patch might no longer be sufficient.

Getting in touch with the ART guys to see if they can shed a little more light and verify whether it will work on the current master branch of AOSP

Comment by Adam Clements [ 25/Nov/14 9:49 AM ]

Bug filed with AOSP project, hopefully they can shed some light on whether it is our problem and if so how we can fix it.

https://code.google.com/p/android/issues/detail?id=80823

Comment by Adam Clements [ 28/Nov/14 11:03 AM ]

I have uploaded an alternative implementation of the locking macro (0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch) which cheats a little - the synchronized block is actually implemented in Java and so guarantees compatibility. This is at the cost of a little extra indirection and the naming/location could probably be better.

But it does fix the bug and work on all versions of android, android + art and the jvm. Would this approach be acceptable?

Comment by Kevin Downey [ 08/Dec/14 1:12 PM ]

I have yet to see any evidence that the bytecode clojure is generating in some way violates the jvm spec, so I suspect the issue is clojure requires a jvm to run, and android doesn't provide a jvm, just something that looks like one if you don't tread outside the beaten path.

Comment by Kevin Downey [ 08/Dec/14 1:27 PM ]

given the structured locking verbiage in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10, `(locking nil)` may generate bytecode whose runtime behavior violates structured locking. the first patch on this issue can cause the compiler to emit monitorenter/exit instructions in different methods, which definitely violates structured locking

Comment by Adam Clements [ 09/Dec/14 10:45 AM ]

Yes, the first patch was definitely wrong, I left it for some context to the conversation, but it's probably best to just remove it for clarity.

For anyone following this conversation who doesn't want to decompile and observe the bytecode, here's a gist with the difference between a java synchronized block and clojure locking https://gist.github.com/AdamClements/2ae6c4919964b71eb470

I'm finding it hard to work out where the deviation from the spec occurs too, though I can see the difference with the Java version, if anything, the Clojure version looks closer to what's described in the spec than the Java one!

If someone with more knowledge than me on the subject could engage on the AOSP bug https://code.google.com/p/android/issues/detail?id=80823 then perhaps we could settle this as an android bug which is too focussed on the java implementation rather than the JVM spec, or perhaps they'll find something that's wrong with the Clojure implementation. I have uploaded the original clojure behaviour and asked them for some more explanation on why it is failing.

Comment by Adam Clements [ 09/Dec/14 11:09 AM ]

The response from the ART guys about what they think we're violating is:

The section on "Structured locking" contains the following:

"[...] implementations [...] are permitted but not required to enforce
both of the following two rules guaranteeing structured locking. [...]"

ART currently enforces both rules at verification time, including

"At no point during a method invocation may the number of monitor exits
performed by T on M since the method invocation exceed the number of
monitor entries performed by T on M since the method invocation."

Comment by Adam Clements [ 09/Dec/14 11:32 AM ]

If for example instruction https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24 or the monitor-enter itself on the next line were to fail, couldn't it could end up in the finally clause and attempt to release the lock even though it has never been captured?

I think this violates the structured locking rules in the jvm spec you linked to.

Comment by Kevin Downey [ 09/Dec/14 1:15 PM ]

an interesting question about structured locking, is does the specification refer to the static bytecode or the runtime behavior of the bytecode. given the bytecode linked(https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24), the static bytecode has the same number of entries and exits, but the dynamic behavior may be different. I wonder which one the art guys claim to be enforcing at verification time (it seems like it would have to be the static bytecode, not the dynamic properties, but then they shouldn't be failing to verify this). looking at the google code issue, the comment https://code.google.com/p/android/issues/detail?id=80823#c6 was made by the same dev as https://code.google.com/p/android/issues/detail?id=80823#c3, so I sort of suspect there is some miscommunication going on. It is not clear in what context the dev is replying in, since in the previous comment you mention splitting monitor-enter and exit across methods. I think things would be much clearer if all patches, specialized clojure android builds, etc, were gotten rid of, then with a vanilla clojure jar you get a javap dump of what fails to verify, then just take that over to the android issue tracker and ask "hey, this fails to verify, why?"

Comment by Adam Clements [ 11/Dec/14 9:15 AM ]

Yeah, I shouldn't have confused it with the patched versions. The gist and the currently uploaded version use the vanilla clojure version of the locking macro now though.

I think the issue comes from the exception table and the instructions that covers. If line 24 can throw for example, you would end up at runtime with a monitor-exit, having never encountered a monitor-enter.

Comment by Ghadi Shayban [ 21/Dec/15 12:19 PM ]

According to Marcus Lagergren JRockit and Hotspot both account for lock releases being in a different method... Perhaps Android has a different (wrong) interpretation?

Comment by Alex Miller [ 21/Dec/15 3:33 PM ]

I added a new clj-1472.patch that fixes a few minor details I didn't like about the prior patch. However, it is still essentially the same change so I have retained the original author attribution.

Comment by Ghadi Shayban [ 21/Dec/15 4:14 PM ]

alex did you upload the right patch?

Comment by Alex Miller [ 21/Dec/15 4:58 PM ]

Ah, no messed that up. Will fix.

Comment by Nicola Mometto [ 22/Dec/15 3:54 AM ]

Alex, it's probably worth making that a `^:once fn*`

Comment by Ghadi Shayban [ 22/Dec/15 1:05 PM ]

From Android:

Android doesn't run Java bytecode, it runs Dex bytecode. The dexdump output of what your class translates to is interesting.

Neither is the JVMS interesting. Android isn't a Java Virtual Machine. We follow the JLS, but not the JVMS (how could we, when we don't run Java bytecode). As such, all appeals against it are irrelevant. We try to be compatible to the spirit of the JVMS wrt/ Dex bytecode, but if your source isn't Java, there are no guarantees.

Now, the verifiers were (and probably still are) broken, even against our (pretty bad) spec, and regrettably we aren't very consistent. In Marshmallow, for example, a lot of the code we can't correctly verify wrt/ structured locking is rejected as a VerifyError, which is not in the spirit of the JVMS. In the next release, this will be relaxed, however, and postponed to an actual check while running the code.

Regrettably there isn't anything we can do about old releases, you'll have to work around any issues. I'll try to take a look at your class when I find the time.

Sounds like making a workaround in Clojure is the least of all evils.

Comment by Alex Miller [ 22/Dec/15 1:46 PM ]

Added -2 patch with ^:once.

Comment by Alex Miller [ 12/Jan/16 10:30 AM ]

Our current belief is that Android is at fault here, so backlogging this for now.

Comment by Ghadi Shayban [ 13/Aug/18 2:58 PM ]

GraalVM/native-image is also complaining about monitorenter/exit balancing. Fixed it by mimicing what javac does: https://github.com/ghadishayban/clojure/commit/8acb995853761bc48b62190fe7005b70da692510

Comment by Alex Miller [ 13/Aug/18 3:57 PM ]

Ghadi, if that's a viable fix, I'm interested in a patch for that.

Comment by Ghadi Shayban [ 14/Aug/18 10:23 AM ]

If someone could assist me with Android testing infra, I'd be game.

In `javac`'s emission of synchronized, it installs a catch handler with the exception type "any". The linked commit catches Exception. If desired, we can emit the `any` handler (I don't know what that means yet exactly – Throwable + anything else in the future?) by calling GeneratorAdapter.visitTryCatch with `null` as the target class.

Comment by Adam Clements [ 14/Aug/18 10:56 AM ]

Have you tried applying the existing clj-1472-2.patch to see if that fixes GraalVM? I think we'd originally reached a place where people were happy with the attached patch (correct me if I'm wrong Alex) but it was decided that the JVM implementation is king so the problem was with Android for not conforming to that rather than with Clojure. It could be that decision is up for reconsideration if the same problem has reared its head elsewhere (I'd still like to see this patched myself).

This was an age ago now, but I remember trying all different combinations of clojure try/catch/finally/throw I could think of and not being able to get the emitted bytecode to conform to the spec for synchronised without altering clojure's code generation which was far too deep down to be practical - hence the above patch which relies on javac to generate the bytecode around the synchronisation instead of using monitor-enter and monitor-exit. I'd be concerned with your linked implementation that it might still generate wrong bytecode in some situations much like the existing implementation does even though it looks perfectly fine and apparently correct at the Clojure code level.

Comment by Ghadi Shayban [ 14/Aug/18 12:47 PM ]

I hadn't tried it but I'm sure clj-1472-2.patch would work from a feasability perspective. It has a perf downside: it can cause "profiling pollution" from the JVM's perspective and confuse lock optimization. (Most locks are uncontended)

I'd rather fix the emission to be more like javac, iff that works for Android.

Comment by Jason Felice [ 12/Dec/18 9:24 AM ]

I've tried the diff Ghadi linked to, but there are issues (including discarding the locked expression value). I fixed that, but after research, I've realized that:
1. This doesn't cover exceptions thrown by monitor-exit itself, which appears to be required. We can't recur in a catch, and we can't catch 'any', so we can't generate this bytecode with just Clojure constructs.
2. The bytecode emitted by Clojure's finally includes a store and a load uncovered by exception handling, so we can't just add exception handling inside the monitor-exit code. I'm pretty sure we can't remove the store and load.

So, unless I'm missing something, the only way to pass a structural locking check is to make locking a special form.

Comment by Adam Clements [ 12/Dec/18 11:10 AM ]

The already attached clj-1472-2.patch passes the structural locking tests.

After I did much exhaustive testing and comparing of generated bytecode back in 2014 this was the only viable approach I could find that didn't require instructing a new special form which reimplemented the poorly documented spec exactly to emit exactly the right bytecode, which seemed fragile to me.

I'd be interested to see some profiling numbers to see how big this claimed performance impact is, especially given how rare usage of the locking macro actually is in clojure (I could only find a few examples in the whole ecosystem when testing. Most people use the other concurrency primitives or java interop)

Comment by Alex Miller [ 12/Dec/18 12:27 PM ]

-3 patch rebases on master, no semantic changes, attribution retained

Comment by Jason Felice [ 13/Dec/18 4:11 PM ]

With clj-1472-3.patch, I can get builds working sometimes. Other times I get this error:

fatal error: com.oracle.svm.core.util.VMError$HostedError: Objects with relocatable pointers must be immutable              
        at com.oracle.svm.core.util.VMError.guarantee(VMError.java:85)                                                      
        at com.oracle.svm.hosted.image.NativeImageHeap.choosePartition(NativeImageHeap.java:492)                            
        at com.oracle.svm.hosted.image.NativeImageHeap.addObjectToBootImageHeap(NativeImageHeap.java:451)                   
        at com.oracle.svm.hosted.image.NativeImageHeap.addObject(NativeImageHeap.java:271)                                  
        at com.oracle.svm.hosted.image.NativeImageCodeCache.addConstantToHeap(NativeImageCodeCache.java:369)                
        at com.oracle.svm.hosted.image.NativeImageCodeCache.addConstantsToHeap(NativeImageCodeCache.java:356)               
        at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:882)                                  
        at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:401)                           
        at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)                             
        at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)                                                  
        at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)                                      
        at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)                                              
        at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

I'm on Graal rc10, and the shortest code I can find to generate that error is:

(ns rep.core
  (:gen-class))

(defn- foo [s]
  (locking s
    42))

(defn -main
  [& args]
  (foo))

Locking on something which isn't a parameter, like a var, works around the issue.

This seems like it might be a Graal issue, so I'll report it there.

Comment by Alex Miller [ 13/Dec/18 4:18 PM ]

Ok, please update. We’ve got probably a long while before the next release that might include this so good to get more feedback.

Comment by Nicola Mometto [ 14/Dec/18 10:17 AM ]

I'll vote against making `locking` into a new special form if possible. It would make any analysis-related library forward-incompatible and likely cause pains in dependency upgrade





[CLJ-1187] Clojure loses quoted metadata on empty literals Created: 22/Mar/13  Updated: 13/Dec/18

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

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

Attachments: Text File 0001-Support-retrieval-of-metadata-from-quoted-empty-lite.patch     Text File 001-CLJ-1187.patch    
Patch: Code and Test
Approval: Vetted

 Description   

meta on empty collections is lost:

user=> (meta '^:foo [])
nil   ;; expected {:foo true} as in:
user=> (meta '^:foo [1])
{:foo true}

This bug propagates to ^:const vars:

user=> (def ^:const foo ^:foo [])
#'user/foo
user=> (meta foo)
nil
user=> (meta @#'foo)
{:foo true}

Cause: As in CLJ-1093, empty collections are replaced with an EmptyExpr that loses meta

Proposed: Don't replace with EmptyExpr if meta is present.

Patch: 0001-Support-retrieval-of-metadata-from-quoted-empty-lite.patch

Screened by:



 Comments   
Comment by Nicola Mometto [ 22/Mar/13 2:12 PM ]

After patch:

user=> (meta '^:foo [])
{:foo true}
user=> (meta '^:foo [:a])
{:foo true}
user=> (def ^:const foo ^:foo [])
#'user/foo
user=> (meta foo)
{:foo true}

Comment by Stuart Halloway [ 29/Mar/13 6:13 AM ]

I believe the title should read "Clojure loses quoted metdata on empty literals".

Comment by Stuart Halloway [ 29/Mar/13 6:16 AM ]

At first glance, the implementation looks wrong in that it blocks non-IObjs ever getting to EmptyExpr. Probably all persistent collections are IObjs, but would not want to bake this in.

Comment by Nicola Mometto [ 29/Mar/13 7:00 AM ]

You're right, I've updated my patch, it should work as expected now

Comment by Andy Fingerhut [ 04/Apr/13 9:44 PM ]

Nicola: Your updated patch 001-CLJ-1187.patch dated Mar 29, 2013 gives syntax errors when I try to compile it.

Comment by Nicola Mometto [ 05/Apr/13 9:06 AM ]

Oh, the irony, I messed up some parentheses writing java.

Sorry for it, here's the correct patch, it applies on upstream/master.

Comment by Gabriel Horner [ 24/May/13 9:14 AM ]

Looks good

Comment by Nicola Mometto [ 26/Jun/13 8:08 PM ]

CLJ-1093 contains a patch that fixes this issue aswell and should be preferred

Comment by Alex Miller [ 18/Aug/13 2:46 PM ]

Marking un-Screened due to the note about CLJ-1093. Want to assess this more before going through Rich.

Comment by Alex Miller [ 18/Oct/13 8:49 AM ]

Switching to Incomplete pending CLJ-1093 which I hope to pull into 1.6.

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

Pulling out of 1.6, will consider in next release.

Comment by Alex Miller [ 06/Jul/14 6:25 PM ]

Dupe of CLJ-1093, closed at Nicola's request.

Comment by Nicola Mometto [ 13/Dec/18 10:51 AM ]

Reopening this since the fix that got applied for CLJ-1093 didn't subsume this one

Comment by Nicola Mometto [ 13/Dec/18 10:52 AM ]

Refreshed and squashed patch





[CLJ-2455] Doc that rseq works on colls satisfying reversible? Created: 12/Dec/18  Updated: 12/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The current docstring of rseq indicates that its parameter can be a vector or sorted map:

user=> (doc rseq)
-------------------------
clojure.core/rseq
([rev])
  Returns, in constant time, a seq of the items in rev (which
  can be a vector or sorted-map), in reverse order. If rev is empty returns nil

But, rseq also works on, say, sorted sets.

Request that the docstring be updated to indicate that rseq works on any rev satisfying reversible?.






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

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

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

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

 Description   

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

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

(deftest foo
  (meta))

(test-var #'foo)

Will output an error at AFn.java:429.

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

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

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

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

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

Patch: clj-1811.patch



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

example file from description

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Attached a new patch that applies this filtering.





[CLJ-1908] Add clojure.test api to run single test with fixtures and report Created: 01/Apr/16  Updated: 12/Dec/18

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

Type: Feature Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: clojure.test

Attachments: Text File CLJ-1908-3.patch    
Approval: Screened

 Description   

When developing code, it is sometimes effective to focus on a single failing test, rather than running all tests in a namespace. This can be the case when running the tests takes some amount of time, or when running the tests produces a large volume of failures. The best option for running a single test with fixtures currently is `test-vars` ala:

(use 'clojure.test) 
(def counter (atom 0)) 
(defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
(use-fixtures :once setup) 
(deftest ex (println "counter =" @counter))

(test-vars [#'ex])  ;=> counter = 1 
(test-vars [#'ex])  ;=> counter = 2

However, this has the following issues:

  • No test reporting feedback such as you get with run-tests (on success, there is no output)
  • Need to specify var (not symbols) wrapped in a vector

Proposed: A new macro `run-test` that specifies a single symbol and does the same test reporting you get with `run-tests`. Usage:

(use 'clojure.test) 
(def counter (atom 0)) 
(defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
(use-fixtures :once setup) 
(deftest ex (println "counter =" @counter)) 

(run-test ex)

;=> Testing user
;=> counter = 1

;=> Ran 1 tests containing 0 assertions.
;=> 0 failures, 0 errors.
;=> {:test 1, :pass 0, :fail 0, :error 0, :type :summary}

(run-test ex)

;=> Testing user
;=> counter = 2

;=> Ran 1 tests containing 0 assertions.
;=> 0 failures, 0 errors.
;=> {:test 1, :pass 0, :fail 0, :error 0, :type :summary}

Patch: CLJ-1908-3.patch

Screened: Alex Miller



 Comments   
Comment by Howard Lewis Ship [ 01/Apr/16 4:12 PM ]

Having trouble with the patch, in that, things that work at the REPL fail when executed via `mvn test`. Tracking down why is taking some time.

Comment by Howard Lewis Ship [ 01/Apr/16 4:40 PM ]

Initial patch; code works but mvn test fails and I haven't figured out why.

Comment by Howard Lewis Ship [ 01/Apr/16 5:44 PM ]

Thanks to Hiredman, was provided with insight that back ticks needed due to how Maven/Ant runs the tests. All tests now pass.

Comment by Alex Miller [ 01/Apr/16 6:43 PM ]

As far as I can tell, this is basically the same intent as CLJ-866 which was completed in Clojure 1.6. You can do this now with test-vars:

user=> (use 'clojure.test) 
nil 
user=> (def counter (atom 0)) 
#'user/counter 
user=> (defn setup [f] (swap! counter inc) (f)) ;; a :once fixture with state 
#'user/setup user=> (use-fixtures :once setup) {:clojure.test/once-fixtures (#object[user$setup 0x7106e68e "user$setup@7106e68e"])} user=> (deftest ex (println "counter =" @counter)) #'user/ex user=> (test-vars [#'ex]) 
counter = 1 
nil 
user=> (test-vars [#'ex]) 
counter = 2 
nil
Comment by Howard Lewis Ship [ 03/Apr/16 12:27 PM ]

I think there is some advantage to being able to run the tests using is symbol, not its var. Further, the change I've suggested also returns the same kind of data that `run-tests` does.

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

Some changes needed on this patch before I will prescreen it:

  • Patch should be squashed to a single commit
  • Commit message in patch should start with "CLJ-1908"
  • Change run-test* to run-test-var
  • The docstring for run-test-var should be: "Run the test in Var v with fixtures and report." Kill the "called from" sentence".
  • The first sentence of the docstring for run-test should be: "Runs a single test in the current namespace." Remove "This is meant to be invoked interactively, from a REPL.". Last sentence is ok.
  • In run-test, replace (ns-resolve ns test-symbol) with the simpler (resolve test-symbol).
Comment by Howard Lewis Ship [ 04/Apr/16 10:52 AM ]

Thanks for the input; I'll have an updated patch shortly.

Comment by Howard Lewis Ship [ 08/Apr/16 2:51 PM ]

Updated patch, squashed and reflecting all of Alex's comments.





[CLJ-1379] Quoting of :actual form is incorrect in clojure.test :pass type maps Created: 12/Mar/14  Updated: 12/Dec/18

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

Type: Defect Priority: Minor
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: clojure.test
Environment:

All clojure versions


Attachments: File fix-quoting-in-pass-case.diff    
Patch: Code
Approval: Triaged

 Description   

The function symbol is not correctly quoted in the construction of the :actual value in a :pass type map for clojure.test.

It currently produces (list = 1 1) instead of (list '= 1 1) for an (is (= 1 1)) test.

I haven't been able to come up with a workaround for this.



 Comments   
Comment by Alex Miller [ 12/Dec/18 1:57 PM ]

Repro case would be useful...





[CLJ-2291] <! and >! break clojure.test/is Created: 13/Dec/17  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: jcr Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Approval: Triaged

 Description   

`(is (not (<! ...)))` works, but `(is (<! ...))` doesn't.

Under the hood, `is` rewrites the code in such a way that it calls `(apply <!
...)`, because `is` assumes that <! is a plain function (see: `assert-expr
:default`[1], `assert-predicate`[2] in clojure.test). It triggers the ">! used
not in (go ...) block" assertion.

[1]https://github.com/clojure/clojure/blob/2e0c0a9a89ede8221504edeb90e8c4ee6cce7e16/src/clj/clojure/test.clj#L486
[2]https://github.com/clojure/clojure/blob/2e0c0a9a89ede8221504edeb90e8c4ee6cce7e16/src/clj/clojure/test.clj#L435

Example code: https://gist.github.com/augustl/4a679dc95847db4434d0e7348651224f#file-test-cljs-L34
Macroexpansion: https://gist.github.com/amalloy/26ec8b8910c7c00bd7feaeef2307bc92#file-gistfile1-txt-L48

Workaround: make `assert-expr` aware of special core.async functions, and use
`assert-any` for those. It's reasonable since core.async is a widely-used core
library. It'd fix the particular problem described in this issue.

The correct solution would be to make <! and >! macros instead of functions.



 Comments   
Comment by Alex Miller [ 12/Dec/18 1:56 PM ]

`is` has multimethod support for custom expressions, so I think this could be patched by adding `assert-expr` methods on <! and >! (probably in a core.async namespace that provides test support).





[CLJ-1808] map-invert should use transients and reduce-kv instead of reduce Created: 30/Aug/15  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File clj-1808-map-invert-should-use-reduce-kv-and-transient.patch    
Patch: Code
Approval: Screened

 Description   

Two performance enhancements on clojure.set/map-invert:

1) Use reduce-kv to avoid realizing mapentry's from input map
2) Use transients to create the output map

Perf:

(use 'criterium.core)
(require '[clojure.set :as set])
(def m1 (zipmap (range 1) (range 1)))
(def m10 (zipmap (range 10) (range 10)))
(def m100 (zipmap (range 100) (range 100)))
(def m1000 (zipmap (range 1000) (range 1000)))
(quick-bench (set/map-invert m1000))
(quick-bench (set/map-invert m100))
(quick-bench (set/map-invert m10))
(quick-bench (set/map-invert m1))

;; means before:  138 ns  1.8 µs  20.6 µs  304 µs
;; means after:   151 ns  1.3 µs   9.0 µs  126 µs

Patch: clj-1808-map-invert-should-use-reduce-kv-and-transient.patch

Screened by: Alex Miller



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

Would be nice to see a quick perf test that compared before/after times.





[CLJ-1905] loop should retain primitive int or float without widening Created: 29/Mar/16  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, performance, primitives
Environment:

Possibly older Clojure versions (but not verified).


Attachments: Text File 0001-CLJ-1905-remove-useless-widening-on-loop-bindings.patch     Text File clj-1905-2.patch    
Patch: Code and Test
Approval: Screened

 Description   

In the following example:

(defn incrementer [n]
  (let [n (int n)]
    (loop [i (int 0)]
      (if (< i n)
        (recur (unchecked-inc-int i))
        i))))

the loop-local starts as an int when just a local but is widened to a long in the recur. It should be possible to retain the primitive int (or alternately float) type on the recur, rather than widening to long (or double).

The compiler code that is promoting the int seems to be:
https://github.com/clojure/clojure/blob/a19c36927598677c32099dabd0fdb9d3097df259/src/jvm/clojure/lang/Compiler.java#L6377-L6380

Proposed: remove useless widening on loop bindings

Patch: clj-1905-2.patch

Screened by: Alex Miller. My main open question here is: do we want to support primitive int/float loop vars?



 Comments   
Comment by Alex Miller [ 29/Mar/16 10:54 AM ]

I don't think anything but primitive longs or doubles are intended to be supported in loop/recur. Presuming that is correct, this would be the expected behavior.

The last major round of numeric/primitive refactoring made the decision to focus only on supporting primitive longs and doubles. One consequence of this is that primitive int loops are difficult to optimize - the main time I run into this is when working with Java interop in a tight loop on (for example) String, collection, or array operations (all of which are int-indexed).

Re unchecked-inc vs unchecked-inc-int, the primary reason to have these two variants is not performance but behavior. In particular, hashing operations often expect to get 32-bit int overflow semantics, not 64-bit int overflow semantics.

In summary, I think in the example given I would not write it with either int or unchecked-inc-int but with long and unchecked-inc if you are looking for best performance.

Comment by Nicola Mometto [ 29/Mar/16 11:01 AM ]

Alex Miller I don't think that's correct, as (AFAIK) it's only in fn params/returns that primitive types are supposed to be restricted to longs and doubles.
Note that for example, char, byte, boolean, short etc work just fine in both let and loop, while int and float work fine in let but not in loop.

This is caused by the following 4 lines in Compiler.java https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6278-L6281

As far as I can tell, there's no reason for those 4 lines to be there at this point in time, and removing them makes int and float locals to be emitted correctly inside loops

This example in clojure.org itself seems to assume that ints should work in loops http://clojure.org/reference/java_interop#primitives

Also from that same paragraph:

All Java primitive types are supported
let/loop-bound locals can be of primitive types, having the inferred, possibly primitive type of their init-form

Comment by Alex Miller [ 29/Mar/16 12:07 PM ]

I agree that it should be possible to let-bound primitives of other types - I'm really talking about what happens on recur.

What would you expect to happen for a fn recur target? I wouldn't expect primitives other than long or double to work there since they can't occur in the function signature.

Note that I haven't closed this ticket, still talking through this.

Comment by Alex Miller [ 29/Mar/16 12:10 PM ]

I've definitely run into cases where creating a primitive int loop/recur would be useful for tight interop loops given how common int-indexing is in Java (some of the alioth benchmarks in particular would benefit from this). I think the argument is far weaker for float though.

Comment by Nicola Mometto [ 29/Mar/16 12:19 PM ]

I don't think we need to worry about fn recur targets at all, given that the only possible primitive bindings there are either long or double, and int/floats would get widened anyway, but good point, the tests in a patch for this ticket need to be sure that case is indeed handled.

RE: floats – I recall people complaining about bad support for floats when using clojure for graphical processing.

Even if admittedly a weak argument, I'm always of the idea that we should strike to be as consistent as possible. I don't think anybody would expect let/loop locals to behave differently, or differences between primitive types (other than the documented limitation about long/double being the only working prim types for fn arguments/return vals)

Comment by Alex Miller [ 29/Mar/16 12:30 PM ]

I'll leave this one in here but I'm going to treat it as an enhancement to current behavior. I think there's a good chance that Rich will just say this is working as intended.

I don't think the example is a very good one though and would welcome a better example. The reservations regarding unchecked-inc-int do not seem correct or valid to me (as usage should be fine on longs and is not designed for perf reasons anyways). A good example would should usage of a Java api in a loop where int-indexing and int-math gives better performance.

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

I edited the title as the bug is in `loop`, not `recur`

Comment by Nicola Mometto [ 02/Apr/16 9:55 AM ]

Attached a patch that removes the useless widenings done by the compiler on loop bindings, here's a benchmark demonstrating the speedup gained when areducing over int-arrays:

Before patch:

Clojure 1.8.0
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 64260 in 60 samples of 1071 calls.
             Execution time mean : 954.009929 µs
    Execution time std-deviation : 20.292809 µs
   Execution time lower quantile : 926.331747 µs ( 2.5%)
   Execution time upper quantile : 1.009189 ms (97.5%)
                   Overhead used : 1.840681 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 4 (6.6667 %)
 Variance from outliers : 9.4244 % Variance is slightly inflated by outliers
nil

After patch:

Clojure 1.9.0-master-SNAPSHOT
user=> (use 'criterium.core)
nil
user=> (let [xs (int-array (range 1e6))] (bench (areduce xs i ret 0 (+ ret (aget xs i)))))
Evaluation count : 68640 in 60 samples of 1144 calls.
             Execution time mean : 870.462532 µs
    Execution time std-deviation : 13.100790 µs
   Execution time lower quantile : 852.357513 µs ( 2.5%)
   Execution time upper quantile : 896.531529 µs (97.5%)
                   Overhead used : 1.844045 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Alex Miller [ 12/Dec/18 12:46 PM ]

-2 patch rebased to apply to master, no changes, retains attribution





[CLJ-1973] generate-proxy produces unpredictable method order in generated classes Created: 01/Jul/16  Updated: 12/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: James Carnegie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, compiler
Environment:

OSX, OpenJDK 8


Attachments: Text File CLJ-1973-v1.patch     Text File CLJ-1973-v2.patch     Text File CLJ-1973-v3.patch     Text File CLJ-1973-v4.patch     Text File CLJ-1973-v5.patch    
Patch: Code
Approval: Screened

 Description   

Using core/proxy to generate proxies for Java classes produces unpredictable method ordering in the generated class files.
This is a problem for repeatable builds (when doing AOT).

Specifically, I'm running Clojure inside Docker, and I'd like my application image layer to be as small as those produced by Java developers (using Meta-inf classpaths and a lib directory). Anyway, to get this working properly so that all dependencies (including those compiled as part of AOT) are on a separate layer, I need the output of compiling my applications' dependencies' proxies to be the same each time I run the build. This reduces build time, image push time, image pull time and container scheduling time.

Example code that exhibits the problem (you'll need to run it a few times to see the issue).

https://github.com/kipz/predictable-proxies

Cause: I've tracked it down to the use of an unsorted map here:

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core_proxy.clj#L186

Approach: Use a sorted map, sorted by hash of the key (which is a vector of method name, seq of param types, and return type).

Patch: CLJ-1973-v5.patch

Screened by: Alex Miller



 Comments   
Comment by James Carnegie [ 01/Jul/16 4:19 PM ]

Patch that uses a sorted-map

Comment by Alex Miller [ 04/Jul/16 9:44 AM ]

I think you can follow the advice at http://clojure.org/guides/comparators to write a simpler comparator for this patch.

Comment by James Carnegie [ 04/Jul/16 11:24 AM ]

Simpler comparator as requested.

Comment by Alex Miller [ 04/Jul/16 12:28 PM ]

I think you lost the sorted set.

Comment by James Carnegie [ 04/Jul/16 1:06 PM ]

Copy paste between branches error. Tested this time.

Comment by James Carnegie [ 04/Jul/16 1:14 PM ]

Now with more consistent formatting of 'let'

Comment by Alex Miller [ 04/Jul/16 2:27 PM ]

While this is probably fine, it might be better to use the hash (Clojure) function rather than .hashCode (Java) function. The map itself is hashed based on the hash so that seems more appropriate.

Comment by James Carnegie [ 04/Jul/16 3:15 PM ]

As requested, using Clojure 'hash' instead.

Thanks Alex - learned about boolean comparators too!

Comment by Alex Miller [ 04/Jul/16 3:52 PM ]

Note that this ordering may still change across Clojure or JVM versions as there is no guarantee of hashing across those. Pre-screening for now.





[CLJ-2286] Update `clj` REPL with hint: (use Ctrl-D to exit) Created: 11/Dec/17  Updated: 12/Dec/18

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

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

All


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

 Description   

Update clojure.main REPL for the `clj` tool with a hint for terminating the session via Ctrl-D, like so (changes in bold):

~ > clj
Clojure 1.9.0 (use Ctrl-D to exit)
user=>
~ >

The lein repl accepts `exit`, `quit`, and Crtl-D to exit; the `clj` repl accepts only Crtl-D. A small hint upon startup on the proper way to terminate the repl session will smooth the experience for users expecting for their old habits to work.

Patch: clj-2286.patch






[CLJ-2453] Allow reader conditionals in prepl Created: 10/Dec/18  Updated: 10/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Oliver Caldwell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: prepl

Approval: Triaged

 Description   

The ClojureScript prepl implementation allows for reader conditionals, the Clojure one does not. Allowing reader conditionals in the Clojure prepl implementation will bring the two implementations in line with each other.

I'm raising the ticket as discussed on clojure-dev and will attach a patch as soon as possible.






[CLJ-2452] filterv docstring prohibits using pred with side effects Created: 09/Dec/18  Updated: 09/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

The docstring of filterv prohibits using a filter predicate that has side effects:

Returns a vector of the items in coll for which (pred item) returns logical true. pred must be free of side-effects.

The last statement seems unmotivated. filterv is based on reduce, and therefore eager like into or mapv. There is no obvious reason to forbid ('must'!) side effects in the predicate.

This docstring has caused confusion in the past. Examples:

The docstring was originally copied verbatim from lazy filter (CLJ-847, commit ec59eba), which could explain why the sentence is there in the first place.

Solution

Delete the sentence 'pred must be free of side-effects.' in the docstring.






[CLJ-2287] Add clojure.set/union, intersection, difference to core.spec.alpha Created: 12/Dec/17  Updated: 07/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-2287-add-clojure-set-specs-v1.patch    

 Description   

Several tickets have been created suggesting that several functions in the clojure.set namespace might throw an exception if given non-sets as arguments, because in some cases they return unexpected values. This list is a sample, not intended to be complete: CLJ-810, CLJ-1682, CLJ-1953, CLJ-1954

Now that clojure.spec exists, documenting the expected argument types of these functions can be done more precisely. The specs could be used to dynamically detect incorrect argument types passed to these functions during testing, and via any other ways built on top of clojure.spec in the future.

Alex Miller suggested in a Slack discussion that perhaps it would be helpful if contributors would help implement such specs for functions within Clojure.

The approach taken with the proposed patch is simply to spec that the :args of clojure.set/subset?, superset?, union, intersection, and difference must all be sets, and spec the :ret types of the first two as boolean, and the last three as set. This seems to match all known comments given when declining previous tickets proposing changes to the behavior of these functions.

Patch: *CLJ-2287-add-clojure-set-specs-v1.patch*

An alternate approach not taken with the patch above would be to also allow nil as arguments and return values, but it is not clear whether that is part of the intended contract of these functions, or an incidental aspect of their current implementations.

I (Andy Fingerhut) looked through all of Clojure and contrib projects for occurrences of clojure.set/union, difference, intersection, superset?, and subset?, and there is only one such call that I can quickly determine does not pass sets to them. It is the one in clojure.data/diff for which there is a patch that can correct this (assuming it is considered a bug) on this ticket: CLJ-1087

Many such calls I could quickly determine always passed sets (e.g. because the arguments were wrapped in (set ...) calls). On a small fraction of them, I gave up trying to figure it out after a minute, since determining the answer fully from code inspection would require going back up the call tree a ways.

Please suggest any other ways that these changes could be evaluated in order to increase your confidence that they are correct.



 Comments   
Comment by Alex Miller [ 12/Dec/17 1:37 PM ]

For the moment I’ll say sure. I think this particular example is a good one.

I’m not sure that all the specs here are as obvious as you expect, or at least that’s been my experience elsewhere.

I’d expect these specs to be in clojure.set.specs namespace btw.

Comment by Andy Fingerhut [ 13/Dec/17 4:33 PM ]

Attached CLJ-2287-add-clojure-set-specs-v1.patch dated 2017-Dec-13. These are my first Clojure specs ever, so feel free to go crazy on suggesting improvements.

I used the namespace clojure.set.specs.alpha rather than clojure.set.specs as you suggested, but I can easily change that if you really did want it without the .alpha

Comment by Andy Fingerhut [ 13/Dec/17 4:34 PM ]

If there is some way I can exercise the :args specs with a large suite of tests, let me know how and I can try it.

And clearly I am putting in at least for :args and :ret pretty obvious choices of specs that I believe are correct, given issues raised against those functions in the past. I don't see how they could be any more subtle than that for these functions, but let me know if I'm missing anything, e.g. the Clojure core team has decided to support non-set arguments for these functions.

I agree that some of the other functions in the clojure.set namespace might be a bit trickier to spec the args of.

Comment by Andy Fingerhut [ 14/Dec/17 8:33 PM ]

I looked through all of Clojure and contrib projects for occurrences of clojure.set/union, difference, intersection, superset?, and subset?, and there is only one such call that I can quickly determine does not pass sets to them. It is the one in clojure.data/diff for which there is a patch that can 'correct' this (assuming it is considered a bug) on this ticket: https://dev.clojure.org/jira/browse/CLJ-1087

Many such calls I could quickly determine always passed sets (e.g. because the arguments were wrapped in (set ...) calls). On a small fraction of them, I gave up trying to figure it out after a minute, since determining the answer fully from code inspection would require going back up the call tree a ways.

Comment by Andy Fingerhut [ 30/Jul/18 7:06 PM ]

If you want an off-the-shelf compatible replacement for clojure.set functions that are identical in behavior, except they perform run-time type checks of the arguments you provide to them, and throw an exception if they have the wrong types (e.g. not sets for union, intersection, difference, subset?, and superset?), consider using the fungible library: https://github.com/jafingerhut/funjible

Comment by Andy Fingerhut [ 07/Dec/18 1:08 PM ]

This Github repository implements some ideas for specs of Clojure core functions: https://github.com/slipset/speculative

There is some discussion on this issue for what kinds of violations of some clojure.set function specs have been seen in some collections of tests. I have not seen a brief summary of all of the data yet. Just wanted to have a link to it from this issue for reference: https://github.com/slipset/speculative/issues/161





[CLJ-2451] Add convenience parse-int, parse-float, parse-short, etc. functions Created: 07/Dec/18  Updated: 07/Dec/18

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

Type: Feature Priority: Major
Reporter: Orestis Markou Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: newbie


 Description   

Rationale

Based on my own experience, and also by observing the beginners channel in Slack, an extremely common pitfall for newcomers is wondering how can you parse a string to an int.

This goes wrong in number of ways:

  • People discover the int function, whose docstring is Coerce to int – that throws when passed a string.
  • People are suggested Integer/parseInt, that does the job, but then can't be used as (map Integer/parseInt coll-of-strings – this is surprising, because it's their first experience with Java interop.
  • People are suggested #(Integer/parseInt %), that does what's expected, but whose syntax can be a little bit too much for your first 10 minutes of Clojure.
  • Sometimes people are suggested #(Integer. %), which is deprecated in Java 11.
  • People go through the same procedure in ClojureScript, and have to select a different interop way to do this.

Proposal 1

Consider adding a parse-* family of functions in Clojure that are thin wrappers over the Java interop. See Appendix for a possible list.

Proposal 2

Consider exposing clojure.lang.LispReader.matchNumber as parse-number.

People can then use the various coercions functions to get back the precision that they need. This might fit better the rationale of this ticket, which is to make a very common "toy program" operation smoother for beginners, and matching the Reader's behaviour will be the least surprising thing.

People who are sensitive about performance should know more about the intricacies of boxed arithmetic on the JVM anyway. This is also pleasantly platform-agnostic, CLJS could expose match-number.

Questions/Alternatives

  • Should the functions return primitives or boxed values?
  • What should be the handling of strings like "0xff"? The parseFoo family of functions rejects those, but 0xff can be read by the Clojure reader.
  • OTOH, the decode family of functions handle some prefixes, but they return a boxed value. But they also accept numbers like #10 which is an invalid Clojure literal.

Appendix

A hopefully complete list of primitive-returning functions (as of JVM 8) is:

name args ret-value
parse-int s int
parse-int s, radix int
parse-uint s int
parse-uint s, radix int
parse-long s long
parse-long s, radix long
parse-ulong s long
parse-ulong s, radix long
parse-short s short
parse-short s, radix short
parse-byte s byte
parse-byte s, radix byte
parse-float s float
parse-double s double

The unsigned functionality was added in Java 8, so should be safe to use in newer Clojure versions. Newer JVM versions add support for parsing parts of CharSequences.






[CLJ-2450] [spec] defining new multimethod for a speced multi method fails under instrumentation Created: 06/Dec/18  Updated: 06/Dec/18

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

Type: Defect Priority: Major
Reporter: Erik Assum Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

Approval: Triaged

 Description   
23:11 $ clj -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-RC3"} org.clojure/test.check {:mvn/version "0.10.0-alpha2"}}}'
Clojure 1.10.0-RC3
user=> (require '[clojure.spec.alpha :as s])
nil
user=> (require '[clojure.spec.test.alpha :as st])
nil
user=> (defmulti foo identity)
#'user/foo
user=> (s/fdef foo :args (s/cat :wat string?))
user/foo
user=> (defmethod foo :bar [lol])
#object[clojure.lang.MultiFn 0x57ac5227 "clojure.lang.MultiFn@57ac5227"]
user=> (st/instrument)
[user/foo]
user=> (defmethod foo :baz [lol])
Execution error (ClassCastException) at user/eval150 (REPL:1).
clojure.spec.test.alpha$spec_checking_fn$fn__3026 cannot be cast to clojure.lang.MultiFn
user=> *e
#error {
 :cause "clojure.spec.test.alpha$spec_checking_fn$fn__3026 cannot be cast to clojure.lang.MultiFn"
 :via
 [{:type java.lang.ClassCastException
   :message "clojure.spec.test.alpha$spec_checking_fn$fn__3026 cannot be cast to clojure.lang.MultiFn"
   :at [user$eval150 invokeStatic "NO_SOURCE_FILE" 1]}]
 :trace
 [[user$eval150 invokeStatic "NO_SOURCE_FILE" 1]
  [user$eval150 invoke "NO_SOURCE_FILE" 1]
  [clojure.lang.Compiler eval "Compiler.java" 7176]
  [clojure.lang.Compiler eval "Compiler.java" 7131]
  [clojure.core$eval invokeStatic "core.clj" 3214]
  [clojure.core$eval invoke "core.clj" 3210]
  [clojure.main$repl$read_eval_print__9068$fn__9071 invoke "main.clj" 414]
  [clojure.main$repl$read_eval_print__9068 invoke "main.clj" 414]
  [clojure.main$repl$fn__9077 invoke "main.clj" 435]
  [clojure.main$repl invokeStatic "main.clj" 435]
  [clojure.main$repl_opt invokeStatic "main.clj" 499]
  [clojure.main$main invokeStatic "main.clj" 598]
  [clojure.main$main doInvoke "main.clj" 561]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.RestFn applyTo "RestFn.java" 132]
  [clojure.lang.Var applyTo "Var.java" 705]
  [clojure.main main "main.java" 37]]}
user=>

I would expect this to not throw an exception






[CLJ-2169] conj has out-of-date :arglists Created: 27/May/17  Updated: 06/Dec/18

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

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docstring

Attachments: Text File 0001-CLJ-2169-add-nullary-unary-overloads-of-conj-to-its-.patch     Text File 0002-CLJ-2169-add-nullary-unary-overloads-of-conj-to-its-.patch    
Patch: Code
Approval: Prescreened

 Description   

conj has had nullary and unary overloads since 1.7.0, but its :arglists still only list [coll x] and [coll x & xs].

Prescreened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 27/May/17 6:05 PM ]

It occurs to me that perhaps the docstring could be updated too to explain (conj).

The new 0002-… patch includes the :arglists change of the 0001-… patch and adds the sentence "(conj) returns []." to the docstring immediately after "(conj nil item) returns (item).".





[CLJ-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 06/Dec/18

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

Type: Feature Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: math

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

 Description   

Clojure's random functions currently use Math.random and related features, which makes them impossible to seed. This seems like an appropriate use of a dynamic var (compared to extra arguments), since library code that wants to behave randomly could transparently support seeding without any extra effort.

I propose (def ^:dynamic *rand* (java.util.Random.)) in clojure.core, and that rand, rand-int, rand-nth, and shuffle be updated to use *rand*.

I think semantically this will not be a breaking change.

Criterium Benchmarks

I did some benchmarking to try to get an idea of the performance implications of using a dynamic var, as well as to measure the changes to concurrent access.

The code used is at https://github.com/gfredericks/clj-1452-tests; the raw output is in a comment.

rand is slightly slower, while shuffle is insignificantly faster. Using shuffle from 8 threads is insignificantly slower, but switching to a ThreadLocalRandom manually in the patched version results in a 2.5x speedup.

Running on my 8 core Linode VM:

Benchmark Clojure Runtime mean Runtime std dev
rand 1.6.0 61.3ns 7.06ns
rand 1.6.0 + *rand* 63.7ns 1.80ns
shuffle 1.6.0 12.9µs 251ns
shuffle 1.6.0 + *rand* 12.8µs 241ns
threaded-shuffling 1.6.0 151ms 2.31ms
threaded-shuffling 1.6.0 + *rand* 152ms 8.77ms
threaded-local-shuffling 1.6.0 N/A N/A
threaded-local-shuffling 1.6.0 + *rand* 64.5ms 1.41ms

Approach: create a dynamic var *rand* and update rand, rand-int, rand-nth, and shuffle to use *rand*

Patch: CLJ-1452.patch

Screened by:



 Comments   
Comment by Gary Fredericks [ 21/Jun/14 7:50 PM ]

Attached CLJ-1452.patch, with the same code used in the benchmarks.

Comment by Gary Fredericks [ 23/Jun/14 8:34 AM ]

Should we be trying to make Clojure's random functions thread-local by default while we're mucking with this stuff? We could have a custom subclass of Random that has ThreadLocal logic in it (avoiding ThreadLocalRandom because Java 6), and make that the default value of *rand*.

Comment by Alex Miller [ 28/Dec/14 11:04 AM ]

I think the ThreadLocal question is interesting, not sure re answer.

It would be nice if the description summarized the results of the tests in a table and the criterium output was in the comments instead.

Comment by Gary Fredericks [ 30/Dec/14 1:26 PM ]

Full output from the test repo (which is summarized in the table in the description):

$ echo "Clojure 1.6.0"; lein with-profile +clj-1.6 run; echo "Clojure 1.6.0 with *rand*"; lein with-profile +clj-1452 run
Clojure 1.6.0

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
WARNING: Final GC required 1.261632096547911 % of runtime
Evaluation count : 644646900 in 60 samples of 10744115 calls.
             Execution time mean : 61.297605 ns
    Execution time std-deviation : 7.057249 ns
   Execution time lower quantile : 56.872437 ns ( 2.5%)
   Execution time upper quantile : 84.483045 ns (97.5%)
                   Overhead used : 16.319772 ns

Found 6 outliers in 60 samples (10.0000 %)
    low-severe   1 (1.6667 %)
    low-mild     5 (8.3333 %)
 Variance from outliers : 75.5119 % Variance is severely inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4780800 in 60 samples of 79680 calls.
             Execution time mean : 12.873832 µs
    Execution time std-deviation : 251.388257 ns
   Execution time lower quantile : 12.526871 µs ( 2.5%)
   Execution time upper quantile : 13.417559 µs (97.5%)
                   Overhead used : 16.319772 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 7.8591 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 150.863290 ms
    Execution time std-deviation : 2.313755 ms
   Execution time lower quantile : 146.621548 ms ( 2.5%)
   Execution time upper quantile : 155.218897 ms (97.5%)
                   Overhead used : 16.319772 ns
Clojure 1.6.0 with *rand*

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
Evaluation count : 781707720 in 60 samples of 13028462 calls.
             Execution time mean : 63.679152 ns
    Execution time std-deviation : 1.798245 ns
   Execution time lower quantile : 61.414851 ns ( 2.5%)
   Execution time upper quantile : 67.412204 ns (97.5%)
                   Overhead used : 13.008428 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 15.7596 % Variance is moderately inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4757940 in 60 samples of 79299 calls.
             Execution time mean : 12.780391 µs
    Execution time std-deviation : 240.542151 ns
   Execution time lower quantile : 12.450218 µs ( 2.5%)
   Execution time upper quantile : 13.286910 µs (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 7.8228 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 152.471310 ms
    Execution time std-deviation : 8.769236 ms
   Execution time lower quantile : 147.954789 ms ( 2.5%)
   Execution time upper quantile : 161.277200 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 43.4058 % Variance is moderately inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-local-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 960 in 60 samples of 16 calls.
             Execution time mean : 64.462853 ms
    Execution time std-deviation : 1.407808 ms
   Execution time lower quantile : 62.353265 ms ( 2.5%)
   Execution time upper quantile : 67.197368 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 9.4540 % Variance is slightly inflated by outliers
Comment by Gary Fredericks [ 30/Dec/14 1:28 PM ]

I think using a ThreadLocal is logically independent from adding *rand*, so it could be a separate ticket. I just suggested it here since it would for some uses mitigate any slowdown from *rand* but now that I'm looking at the benchmark results again the slowdown might be insignificant.

Comment by Gary Fredericks [ 30/Dec/14 5:44 PM ]

Also worth noting that (as I did in the benchmark code) with just the patch's changes (i.e., no ThreadLocal involved) users still gain the ability to do ThreadLocal manually, which is not currently possible.

Comment by Stuart Halloway [ 19/Jul/15 7:42 AM ]

workaround: data.generators provides seedable random

Comment by Ghadi Shayban [ 14/Jan/16 10:15 AM ]

Just noting, ThreadLocalRandom is >= JDK 7.

Comment by Gary Fredericks [ 06/Dec/18 9:20 AM ]

The >=JDK7 aspect of ThreadLocalRandom is no longer a problem, correct?

Comment by Alex Miller [ 06/Dec/18 12:27 PM ]

Right





[CLJ-2290] into docstring doesn't mention 0- and 1-arity versions Created: 13/Dec/17  Updated: 05/Dec/18

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

Type: Defect Priority: Trivial
Reporter: jcr Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docs, docstring, documentation

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

 Description   

See: https://github.com/clojure/clojure/blob/clojure-1.9.0-alpha14/src/clj/clojure/core.clj#L6763

Docstring should mention that "(into coll) returns coll" and "(into) returns an empty vector" and the fact that "hence it can be used as a reducing function", clarifying the reason to use [] as the default (the same as with `conj`).



 Comments   
Comment by Alex Miller [ 05/Dec/18 11:08 PM ]

I would like to accept this patch, but I need a signed CA and a properly formatted patch.





[CLJ-2295] clojure.repl/doc repeats doc string in output for special forms in Clojure 1.9.0 Created: 15/Dec/17  Updated: 05/Dec/18

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, regression

Attachments: Text File CLJ-2295-v1.patch    
Patch: Code
Approval: Prescreened

 Description   

Here is output from Clojure 1.8.0:

user=> (doc if)
-------------------------
if
  (if test then else?)
Special Form
  Evaluates test. If not the singular values nil or false,
  evaluates and yields then, otherwise, evaluates and yields else. If
  else is not supplied it defaults to nil.

  Please see http://clojure.org/special_forms#if
nil

Here is the corresponding output from Clojure 1.9.0:

user=> (doc if)
-------------------------
if
  (if test then else?)
Special Form
  Evaluates test. If not the singular values nil or false,
  evaluates and yields then, otherwise, evaluates and yields else. If
  else is not supplied it defaults to nil.

  Please see http://clojure.org/special_forms#if
  Evaluates test. If not the singular values nil or false,
  evaluates and yields then, otherwise, evaluates and yields else. If
  else is not supplied it defaults to nil.
nil

This repetition only occurs when calling clojure.repl/doc or clojure.repl/print-doc for special form symbols, not for other symbols like macros and functions. It was introduced when modifying clojure.repl/print-doc when adding the ability to print specs in its output, and the fix is straightforward.

Prescreened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 15/Dec/17 6:20 PM ]

Patch CLJ-2295-v1.patch dated 2017-Dec-15 is one possible way to fix this issue. Verified that output for other cases, e.g. macros, is unchanged from Clojure 1.8.0 with these changes, except for the new Spec output, which should be kept.





[CLJ-2326] *compiler-options* docstring is missing :direct-linking Created: 21/Feb/18  Updated: 05/Dec/18

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

Type: Defect Priority: Minor
Reporter: Dominic Monroe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

Docstring was never updated to add :direct-linking.

Proposed: Add line:

clojure.core/*compiler-options*
  A map of keys to options.
  Note, when binding dynamically make sure to merge with previous value.
  Supported options:
  :elide-meta - a collection of metadata keys to elide during compilation.
  :disable-locals-clearing - set to true to disable clearing, useful for using a debugger
  :direct-linking - set to true to use direct static invocation of functions, rather than vars
  Alpha, subject to change.

Patch: clj-2326.patch






[CLJ-2387] Port 65535 incorrectly excluded Created: 16/Aug/18  Updated: 05/Dec/18

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

Type: Defect Priority: Minor
Reporter: Keyhan Vakil Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: server
Environment:

n/a


Attachments: Text File port-65535-obo.patch     Text File port-65535-obo-v2.patch    
Patch: Code
Approval: Prescreened

 Description   

The following code is used in src/clj/clojure/core/server.clj to validate whether a port number is valid:

src/clj/clojure/core/server.clj
(defn- validate-opts
  "Validate server config options"
  [{:keys [name port accept] :as opts}]
  (doseq [prop [:name :port :accept]] (required opts prop))
  (when (or (not (integer? port)) (not (< -1 port 65535)))
(throw (ex-info (str "Invalid socket server port: " port) opts))))

However this is very slightly incorrect, since port 65535 is excluded but it is actually a valid port.

repl
user=> (defn is-invalid-port [port] (or (not (integer? port)) (not (< -1 port 65535))))
#'user/is-invalid-port
user=> (is-invalid-port 65534)
false
user=> (is-invalid-port 65535) ; should be false!
true
user=> (is-invalid-port 65536)
true

This is corroborated by ServerSocket's Javadoc:

IllegalArgumentException - if the port parameter is outside the specified range of valid port values, which is between 0 and 65535, inclusive.

Patch: port-65535-obo-v2.patch

Prescreened: Alex Miller



 Comments   
Comment by Alex Miller [ 16/Aug/18 8:29 PM ]

Hi Keyhan, have you signed the Clojure Contributor Agreement? https://clojure.org/community/contributing. If not, please do so.

Another, maybe better, solution here is (<= 0 port 65535) - that reads more closely to the check.

Comment by Keyhan Vakil [ 16/Aug/18 9:00 PM ]

Yes, just earlier today.

I agree that looks better. Current patch is port-65535-obo-v2.patch.





[CLJ-2416] [core.specs] defmulti and defmethod spec Created: 09/Oct/18  Updated: 05/Dec/18

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

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

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

 Description   

Currently defmulti and defmethod don't have specs and don't have much macro validation.

user=> (defmulti 5 class)
ClassCastException class java.lang.Long cannot be cast to class clojure.lang.IObj

Add specs for defmulti and defmethod.

After:

user=> (defmulti 5 class)
Syntax error macroexpanding clojure.core/defmulti at (1:1).
5 - failed: simple-symbol? at: [:fn-name]

Patch: clj-2416.patch






[CLJ-899] Accept and ignore colon between key and value in map literals Created: 18/Dec/11  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: reader


 Description   

Original title was 'treat colons as whitespace' which isn't a problem description but a (flawed) implementation approach

For JSON compatibility
known problems when no spaces - x:true and y:false



 Comments   
Comment by Tassilo Horn [ 23/Dec/11 3:22 AM ]

Discussed here: https://groups.google.com/d/msg/clojure/XvJUzaY1jec/l8xEwlFl8EUJ

Comment by Kevin Downey [ 11/Jan/12 2:23 PM ]

please no

Comment by Tavis Rudd [ 16/Jan/12 12:17 PM ]

Alan Malloy raises a good point in the google group discussion (https://groups.google.com/d/msg/clojure/XvJUzaY1jec/aVpWBicwGhsJ) about accidental confusion between trailing (or floating) and leading colons:
"It isn't even as simple as "letting them
be whitespace", because presumably you want (read-string "{a: b}") to
result in (hash-map 'a 'b), but (read-string "{a :b}") to result in
(hash-map 'a :b)."

This issue could be avoided by only treating a colon as whitespace when followed by a comma. As easy cut-paste of json seems the be the key motivation here, the commas are going to be there anyway: valid {"v":, 1234} vs syntax error {a-key: should-be-a-keyword}.

Comment by Alex Baranosky [ 16/Jan/12 5:23 PM ]

This would be visually confusing imo.

Comment by Laurent Petit [ 17/Jan/12 5:01 PM ]

Please, oh please, no.

Comment by Tavis Rudd [ 18/Jan/12 2:40 PM ]

Er, brain fart. I was typing faster than I was thinking and put the comma in the wrong place. In my head I meant the form following the colon would have to have a comma after it. Thus, {"a-json-key": 1234, ...} would be valid while {"a-json-key": was-supposed-to-be-a-keyword "another-json-key" foo} would complain about the colon being an Invalid Token. I don't see the need for it, however.

Comment by Joe R. Smith [ 27/Feb/12 10:55 AM ]

Clojure already has reader syntax for a map. If we support JSON, do we also support ruby map literals? Seems like this addition would only add confusion, imo, given colons are used in keywords and keywords are frequently used in maps - e.g., when de-serializing from XML, or even JSON.

Comment by David Nolen [ 27/Feb/12 11:19 AM ]

Clojure is no longer a language hosted only on the JVM. Clojure is also hosted on the CLR, and JavaScript. In particular ClojureScript can't currently easily deal with JSON literals - an extremely common (though problematic) data format. By allowing colon whitespace in map literals - Clojure data structures can effectively become an extensible JSON superset - giving the succinctness of JSON and the expressiveness of XML.

+1 from me.

Comment by Tim McCormack [ 13/Nov/12 7:27 PM ]

Clojure is only hosted on the JVM; ClojureScript is hosted on JS VMs. If this is useful for CLJS, it should just be a CLJS feature.

Comment by Mike Anderson [ 10/Dec/12 11:51 PM ]

-1 for this whole idea: that way madness lies....

If we keep adding syntactical oddities like this then the language will become unmaintainably complex. It's the exact opposite of simple to have lots of special cases and ambiguities that you have to remember.

If people want to use JSON that is fine, but then the best approach use a specific JSON parser/writer, not just paste it into Clojure source and expect it to work.

Comment by Laszlo Török [ 11/Dec/12 4:54 AM ]

-1 for reasons mentioned by Allan Malloy and Mike Anderson

Comment by Bozhidar Batsov [ 19/Oct/14 3:06 AM ]

-1 Don't repeat the mistake made in Ruby...





[CLJ-704] range function has missing documentation Created: 04/Jan/11  Updated: 05/Dec/18

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

Type: Enhancement Priority: Trivial
Reporter: Maarten Hus Assignee: Plínio Balduino
Resolution: Unresolved Votes: 0
Labels: docstring
Environment:

All


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

 Description   

The range function's documentation does indicate the following usage:

(range 10 0 -1) -> (10, 9, 8, 7, 6, 5, 4, 3, 2, 1)

Current doc:

Returns a lazy seq of nums from start (inclusive) to end
  (exclusive), by step, where start defaults to 0, step to 1, and end to
  infinity. When step is equal to 0, returns an infinite sequence of
  start. When start is equal to end, returns empty list.

Its also possible to step down rather than up, for example counting
backwards from 10 by -1: (range 10 0 -1).

Proposed:

Add final sentence: "Step may be negative."

Patch: clj-704.patch



 Comments   
Comment by Rasmus Svensson [ 15/Jan/11 7:39 AM ]

The current doc actually mentions the 'step' parameter briefly:

"[...] to end (exclusive), by step, where start [...]"

But as this might be easy to miss, an addition to the doc is still a good idea, I think.

My suggestion:

Returns a lazy seq of nums from start (inclusive) to end
(exclusive), by step, where start defaults to 0, step to 1, and end
to infinity. Step may be negative to count backwards.

Comment by Plínio Balduino [ 13/Jun/14 10:59 PM ]

There was any news about it?

Could I assign it to me?

Comment by Andy Fingerhut [ 13/Jun/14 11:11 PM ]

No, no news about this one. It is in the 'open' state, meaning that there is currently no judgement as to whether Clojure screeners or Rich Hickey are interested in such a change. http://dev.clojure.org/display/community/JIRA+workflow

That said, it seems like it should not take a lot of time to create a patch. Detailed instructions are given here: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alex Miller [ 14/Jun/14 7:34 AM ]

Go for it!

Comment by Plínio Balduino [ 14/Jun/14 10:57 AM ]

I cannot reproduce this: "When step is equal to 0, returns an infinite sequence of start."

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L2726-L2729

Is this correct?

Comment by Andy Fingerhut [ 14/Jun/14 2:47 PM ]

The last time range was modified was due to ticket CLJ-1018. See the attached patch there, which I believe is the one that was applied after Clojure 1.5 but before Clojure 1.6.

Perhaps that change does not do what it claimed to do in the doc string. I haven't looked at it in detail yet.

Comment by Andy Fingerhut [ 14/Jun/14 2:54 PM ]

In Clojure 1.6, it appears that it may be more accurate if the last two sentences in the doc string were modified. range's doc string is currently:

Returns a lazy seq of nums from start (inclusive) to end
(exclusive), by step, where start defaults to 0, step to 1, and end to
infinity. When step is equal to 0, returns an infinite sequence of
start. When start is equal to end, returns empty list.

It might be more accurate to say:

Returns a lazy seq of nums from start (inclusive) to end
(exclusive), by step, where start defaults to 0, step to 1, and end to
infinity. When start is equal to end, returns empty list. When step
is equal to 0 and start and end differ, returns an infinite sequence of
start.





[CLJ-434] Additional copy methods for URLs in clojure.java.io Created: 10/Sep/10  Updated: 05/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io


 Description   

The copy method in clojure.java.io doesn't handle java.net.URL as input.
The necessary methods can be found in the mailing list post:
[[url:http://groups.google.com/group/clojure/browse_thread/thread/24a105b12466a8e8]]



 Comments   
Comment by Assembla Importer [ 10/Sep/10 7:32 AM ]

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





[CLJ-213] Invariants and the STM Created: 01/Dec/09  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

(ticket requested here http://groups.google.com/group/clojure/browse_thread/thread/119311e89fa46806/4903ce25ff6deaa6#4903ce25ff6deaa6)

The general idea is to declare invariants inside a transaction and, when at commit time an invariant doesn't hold anymore, the transaction retries.
So it can both act as a kind of soft ensure or to specify actions that "partially commute".
Thus it would enable coarser refs.

See the attached file for quick prototype.

User code would looks like:

(invariant (@world :key))
(commute world update-in [:key] val-transform-fn)

This means the commute will occur only if (@world :key) returns the same value in-transaction and at commit point.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 7:23 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/213
Attachments:
invariants.patch - https://www.assembla.com/spaces/clojure/documents/dd4kUS3MWr3QvMeJe5aVNr/download/dd4kUS3MWr3QvMeJe5aVNr





[CLJ-115] GC Issue 111: Enable naming an array parameter for areduce Created: 17/Jun/09  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
Reported by bo...@boriska.com, Apr 28, 2009

Currently there is no way to access anonymous array parameter of areduce.

Consider:

(areduce (.. System getProperties values toArray) 
     i r 0 (some_expression))
some_expression has no way to access the array.

Per Rich:
--------------------
Yes, areduce would be nicer if it looked like a binding set:

(areduce [aname anarray, ret init] expr)
(areduce [aname anarray, ret init, start-idx  start-n] expr)
(areduce [aname anarray, ret init, start-idx  start-n, end-idx end-n]
expr) 
--------------------

This was discussed here:
http://groups.google.com/group/clojure/tree/browse_frm/thread/40597a8ac322bc37/8cf6b17328ea7e8b?rnum=1&_done=%2Fgroup%2Fclojure%2Fbrowse_frm%2Fthread%2F40597a8ac322bc37%2F8cf6b17328ea7e8b%3Ftvc%3D1%26pli%3D1%26#doc_9ea7e3c5d500ed3c


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-112] GC Issue 108: All Clojure interfaces should specify CharSequence instead of String when possible Created: 17/Jun/09  Updated: 05/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   
Reported by redchin, Apr 20, 2009

rhickey: unlink: then just use a map {:escaped true :val "foo"}

unlink: What I meant is, everything in between would want to see something 
String-y, not caring whether it's a String or MyString.

hiredman: unlink: if you use something that implements CharSequence and 
IMeta (I think it's IMeta) you get something that is basically a String, 
but with metadata

rhickey: what hiredman said

hiredman: ideally most things would not specify String but CharSequence in 
their interface

hiredman: but somehow I doubt that is case

unlink: ok.

unlink: Good to know.

rhickey: hiredman: unfortunately that's not true of some of Clojure - could 
you enter an issue for it please - use CharSequence when possible?


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-15] Incremental hashcode calculation for collections Created: 17/Jun/09  Updated: 05/Dec/18

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

Type: Feature Priority: Minor
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: File lazy-incremental-hashes.diff    

 Description   
Reported by richhickey, Dec 17, 2008
So hachCode can be final, more efficient to calc as you go.
Formerly Google Code Issue 11


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:44 AM ]

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

Comment by Christophe Grand [ 08/Mar/13 6:20 AM ]

Wouldn't the naive approach incur realizing lazy sequences when adding them to a list or a vector or as values in a map?

Comment by Christophe Grand [ 26/Aug/13 3:40 AM ]

The lazy-incremental-hashes.diff introduces lazy incremental hashes based on structural sharing.

Comment by Christophe Grand [ 26/Aug/13 3:42 AM ]

Why is this identified as Blocker?

Comment by Christophe Grand [ 26/Aug/13 3:43 AM ]

setting priority to minor

Comment by Andy Fingerhut [ 26/Aug/13 7:46 AM ]

I've seen this "edit a ticket, it changes to Priority=Blocker" behavior before. I believe some of the older tickets have no Priority field at all, and when you edit any of their properties, it creates the priority field with a default value of Blocker.

Comment by Alex Miller [ 26/Aug/13 8:06 AM ]

Yes, concur with Andy's explanation on priority change. I just bulk-edited all open CLJ tickets with null priority and set their priority.

Comment by Andy Fingerhut [ 30/Jan/14 8:01 PM ]

Patch lazy-incremental-hashes.diff still applies cleanly as of Jan 30 2014 latest Clojure master, but now fails tests due to recent commits involving hash changes. I have not checked how difficult or easy it might be to update the patch to pass tests again.

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

Patch lazy-incremental-hashes.diff dated Aug 26 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 [ 05/Sep/14 11:41 AM ]

Patch lazy-incremental-hashes.diff dated Aug 26 2013 applies cleanly again to latest Clojure master as of Sep 5 2014, even though the patch has not been updated. I haven't checked, but I would guess this is because one of the changes made to Clojure master recently was reverted with this commit: https://github.com/clojure/clojure/commit/46be47c9f51ef10d0082f1bd39ffff1008682861





[CLJ-2] Scopes Created: 15/Jun/09  Updated: 05/Dec/18

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

Type: Feature Priority: Major
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File scopes-spike.patch    

 Description   

Add the scope system for dealing with resource lifetime management



 Comments   
Comment by Assembla Importer [ 24/Aug/10 11:43 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 11:43 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Stuart Halloway [ 12/Jul/11 8:26 AM ]

Patch demonstrates idea, not ready for prime time.

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

I think the decision of having to specify either a Closeable resource or a close function for an existing non-Closeable resource in with-open is quite awkward, because they have completely different meaning.

  (let [foo (open-my-custom-resource "foo.bar")]
    (with-open [r (reader "foo.txt")
                foo #(.terminate foo)]
      (do-stuff r foo)))

I think a CloseableResource protocol that can be extended to custom types as implemented in the patch to CLJ-308 is somewhat easier to use. Extend it once, and then you can use open-my-custom-resource in with-open just like reader/writer and friends...

That said, Scopes can still be useful, but I'd vote for handling the "how should that resource be closed" question by a protocol. Then the with-open helper can simply add

(swap! *scope* conj (fn [] (clojure.core.protocols/close ~(bindings 0))))

and cleanup-scope only needs to apply each fn without having to distinguish Closeables from fns.





[CLJ-1323] AsmReflector throws exceptions on JDK8 Created: 13/Jan/14  Updated: 04/Dec/18

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1323-disable.diff     Text File clj-1323-experiment-v1.patch    

 Description   

After the commit of the updated ASM library for CLJ-713, Clojure builds and passes all tests except for one, compare-reflect-and-asm in reflect.clj.

This can be narrowed down somewhat to a difference in behavior of the following 2 forms evaluated with the latest Clojure and JDK8:

;; The following two lines work with the latest (Jan 11 2014) Clojure 1.6.0-master-SNAPSHOT
;; if run on JDK 6 or JDK 7, but throw an exception with JDK 8.

(import '[clojure.asm ClassReader ClassVisitor Type Opcodes])
(def r (ClassReader. "java.lang.Object"))

I am not certain, but from a bit of Google searching it appears that this may be a limitation of the ASM library version 4 – it throws exceptions when attempting to read class files produced by JDK 8, because of a newer classfile version number. Links that seem to support this conclusion:

http://mail-archive.ow2.org/asm/2013-02/msg00000.html

http://forge.ow2.org/tracker/index.php?func=detail&aid=316375&group_id=23&atid=350023

A couple of alternatives are:

(1) update ASM again to one that supports JDK 8 class files

(2) disable the compare-reflect-and-asm test. Clojure itself does not use the AsmReflector for anything except this unit test. The Java reflector is the default one.



 Comments   
Comment by Alex Miller [ 13/Jan/14 8:16 AM ]

1) There is no released ASM that supports JDK 8 yet. ASM 5 will but it will not be final till JDK 8 is in the final stages of release.

2) Probably more likely.

Comment by Nicola Mometto [ 19/Mar/14 9:01 AM ]

As of now, both JDK8 and ASM5 are out.
I just tried compiling clojure on JDK8 with ASM5 and all compiles fine

Comment by Alex Miller [ 19/Mar/14 9:07 AM ]

How are you running this test?

Comment by Nicola Mometto [ 19/Mar/14 9:11 AM ]

I downloades ASM5, replaced the bundled ASM that comes with clojure with that one after changing che package name to "clojure.asm" and run `mvn install`, all the tests pass.

Comment by Alex Miller [ 19/Mar/14 9:24 AM ]

I was actually talking about the JDK 8 change - was curious about exactly what was being changed?

Comment by Alex Miller [ 19/Mar/14 10:04 AM ]

In particular, I'm assuming that you're not altering the build.xml to change the compilation -source or -target and running with JAVA_HOME / path set to JDK 8.

We don't have any plans to actually build Clojure with JDK 8 any time soon, so I'm not overly concerned about that. But it does appear that the embedded ASM 4 cannot read newer class files from JDK 8. Afaik, the only place that happens is in clojure.reflect.java in the AsmReflector, which is not the default reflector. The JavaReflector will properly reflect the Java 8 classes.

ASM 5 has only been out a couple days and already has at least one serious bug reported - I'd like that to see more use before we switch to it, so maybe this is a good target for the release after Clojure 1.6.

Comment by Alex Miller [ 23/Mar/14 12:17 PM ]

Patch to temporarily disable the failing test until we have an ASM that supports JDK 8.

Comment by Andy Fingerhut [ 04/Dec/18 12:21 AM ]

I have tested the latest Clojure 1.10.0-RC3 code, with the test that was disabled re-enabled again, using the patch in attachment clj-1323-experiment-v1.patch.

Without the change to file src/clj/clojure/reflect/java.clj, it fails when running with OpenJDK 11, because the uncommented test throws an exception when it attempts to call (type-reflect classname :reflector asm-reflector) for class java.io.FileInputStream (and perhaps others after that in the vector).

With the patch as written, all tests pass using these version combinations:
+ Mac OS X 10.13.6 plus Oracle Java 1.8.0_192
+ Ubuntu 16.04.5 plus OpenJDK 11 - the uncommented test throws

I am not saying that I know this patch is good, or the full ramifications of the change I made in file src/clj/clojure/reflect/java.clj. I was poking at the code to run an experiment to see if I could make the test that was commented out pass again. Someone like Ghadi Shayban would be better than I at assessing the ramifications of such a change.

Comment by Alex Miller [ 04/Dec/18 7:50 AM ]

Not planning to do this for Clojure 1.10 but can assess for 1.11.

Comment by Andy Fingerhut [ 04/Dec/18 11:52 AM ]

Definitely understood – no rush for 1.10 release here. Just scanning through the list of open defect tickets for the first time in a while, and seeing which ones I might have something to say a little bit about.





[CLJ-1876] calling require from java is not thread safe Created: 07/Jan/16  Updated: 04/Dec/18

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

Type: Defect Priority: Major
Reporter: Robert (Bobby) Evans Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Crappy Linux VM running RHEL6

java version "1.8.0_60"
Java(TM) SE Runtime Environment (build 1.8.0_60-b27)
Java HotSpot(TM) 64-Bit Server VM (build 25.60-b23, mixed mode)



 Description   

As a part of Apache Storm we have some code that can load a clojure function from java using the following code.

public static IFn loadClojureFn(String namespace, String name) {
        try {
            clojure.lang.Compiler.eval(RT.readString("(require '" + namespace + ")"));
        } catch (Exception e) {
            //if playing from the repl and defining functions, file won't exist
        }
        return (IFn) RT.var(namespace, name).deref();
    }

If this function is called from multiple different threads at the same time, trying to import the same namespace, I will occasionally get some very odd errors. NOTE: I had to modify the catch to actually print out the error message it was getting (We should not be eating exceptions either way).

{verbatim}
2016-01-07 16:26:09.305 b.s.u.Utils [WARN] Loading namespace failed
clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context, compiling:(storm/starter/clj/word_count.clj:21:1)
at clojure.lang.Compiler.analyze(Compiler.java:6543) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6725) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6524) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6485) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6786) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.load(Compiler.java:7227) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:371) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.loadResourceScript(RT.java:362) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:446) ~[clojure-1.7.0.jar:?]
at clojure.lang.RT.load(RT.java:412) ~[clojure-1.7.0.jar:?]
at clojure.core$load$fn__5448.invoke(core.clj:5866) ~[clojure-1.7.0.jar:?]
at clojure.core$load.doInvoke(core.clj:5865) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$load_one.invoke(core.clj:5671) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib$fn__5397.invoke(core.clj:5711) ~[clojure-1.7.0.jar:?]
at clojure.core$load_lib.doInvoke(core.clj:5710) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:142) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$load_libs.doInvoke(core.clj:5749) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.applyTo(RestFn.java:137) ~[clojure-1.7.0.jar:?]
at clojure.core$apply.invoke(core.clj:632) ~[clojure-1.7.0.jar:?]
at clojure.core$require.doInvoke(core.clj:5832) ~[clojure-1.7.0.jar:?]
at clojure.lang.RestFn.invoke(RestFn.java:408) ~[clojure-1.7.0.jar:?]
at clojure.core$eval114.invoke(NO_SOURCE_FILE:0) ~[?:?]
at clojure.lang.Compiler.eval(Compiler.java:6782) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.eval(Compiler.java:6745) ~[clojure-1.7.0.jar:?]
at backtype.storm.utils.Utils.loadClojureFn(Utils.java:602) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.clojure.ClojureBolt.prepare(ClojureBolt.java:57) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.daemon.executor$fn_8297$fn_8310.invoke(executor.clj:785) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at backtype.storm.util$async_loop$fn__556.invoke(util.clj:482) [storm-core-0.11.0-SNAPSHOT.jar:0.11.0-SNAPSHOT]
at clojure.lang.AFn.run(AFn.java:22) [clojure-1.7.0.jar:?]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_60]
Caused by: java.lang.RuntimeException: Unable to resolve symbol: sentence-spout in this context
at clojure.lang.Util.runtimeException(Util.java:221) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolveIn(Compiler.java:7019) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.resolve(Compiler.java:6963) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6924) ~[clojure-1.7.0.jar:?]
at clojure.lang.Compiler.analyze(Compiler.java:6506) ~[clojure-1.7.0.jar:?]
... 33 more{verbatim}

If I make the static java function synchronized the issue goes away. It always seems to blow up when parsing a few specific macros getting confused that a specific symbol cannot be resolved.

The namespace trying to be loaded.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/examples/storm-starter/src/clj/storm/starter/clj/word_count.clj

The macros that we seem to get exceptions on.
https://github.com/apache/storm/blob/a99d9c11be005ade7c308bebdda71c7fb0111acc/storm-core/src/clj/backtype/storm/clojure.clj#L77-L138

And like I said it look like it is a threading issue of some sort. When I added the synchronized keyword everything works.



 Comments   
Comment by Kevin Downey [ 17/Feb/16 10:19 AM ]

calling require from clojure isn't thread safe either, no different from this

Comment by Andy Fingerhut [ 03/Dec/18 9:33 PM ]

Is this issue perhaps solved by using the new `serialized-require` that has been added in Clojure 1.10?

Comment by Alex Miller [ 04/Dec/18 7:47 AM ]

The plan is to rework require in future release so that serialized-require is no longer needed. Maybe like that, maybe some other way. This issue is basically the placeholder for the real work. Adding the lock in serialized-require is a big hammer and we would like to make that a bit more surgical.





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

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

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

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



 Description   

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

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

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

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

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



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

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

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

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

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

Both of these return false on both JDKs:

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

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

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

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

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

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

IMHO the only sane approach would be to:

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

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

Comment by Alex Miller [ 10/May/16 8:03 AM ]

I think the subsumes check is effectively trying to do your option #1 already - this is a case where the types of the arguments in the two cases have no hierarchical relationship. Probably #2 would make more sense - expressing a preference, although there are certainly cases where "widest" has no meaning, so not sure what the general form of this is.

Comment by Nicola Mometto [ 10/May/16 8:05 AM ]

To clarify, that wasn't a list of different options, it was a list of steps to take.
i.e. if it's possible to pick the most specific type from a hierarchy, do that, ELSE if the types are primitive, pick the widest ELSE fail

Comment by Andy Fingerhut [ 03/Dec/18 9:32 PM ]

Possibly the same as, or at least some commonality with, CLJ-1212.





[CLJ-1212] Silent truncation/downcasting of primitive type on reflection call to overloaded method (Math/abs) Created: 28/May/13  Updated: 03/Dec/18

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

Type: Defect Priority: Major
Reporter: Matthew Willson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: primitives, typehints
Environment:

Clojure 1.5.1
OpenJDK Runtime Environment (IcedTea6 1.12.5) (6b27-1.12.5-0ubuntu0.12.04.1)



 Description   

I realise relying on reflection when calling these kinds of methods isn't a great idea performance-wise, but it shouldn't lead to incorrect or dangerous behaviour.

Here it seems to trigger a silent downcast of the input longs, giving a truncated integer output:

user> (defn f [a b] (Math/abs (- a b)))
Reflection warning, NO_SOURCE_PATH:1:15 - call to abs can't be resolved.
#'user/f
user> (f 1000000000000 2000000000000)
727379968
user> (class (f 1000000000000 2000000000000))
java.lang.Integer
user> (defn f [^long a ^long b] (Math/abs (- a b)))
#'user/f
user> (f 1000000000000 2000000000000)
1000000000000
user> (class (f 1000000000000 2000000000000))
java.lang.Long



 Comments   
Comment by Matthew Willson [ 28/May/13 12:50 PM ]

For an even simpler way to replicate the issue:

user> (#(Math/abs %) 1000000000000)
Reflection warning, NO_SOURCE_PATH:1:3 - call to abs can't be resolved.
727379968

Comment by Andy Fingerhut [ 28/May/13 1:36 PM ]

I was able to reproduce the behavior you see with these Java 6 JVMs on Ubuntu 12.04.2:

java version "1.6.0_27"
OpenJDK Runtime Environment (IcedTea6 1.12.5) (6b27-1.12.5-0ubuntu0.12.04.1)
OpenJDK 64-Bit Server VM (build 20.0-b12, mixed mode)

java version "1.6.0_45"
Java(TM) SE Runtime Environment (build 1.6.0_45-b06)
Java HotSpot(TM) 64-Bit Server VM (build 20.45-b01, mixed mode)

However, I tried two Java 7 JVMs, and it gave the following behavior which looks closer to what you would hope for. I do not know what is the precise difference between Java 6 and Java 7 that leads to this behavior difference, but this is some evidence that this has something to do with Java 6 vs. Java 7.

user=> (set! warn-on-reflection true)
true
user=> (defn f [a b] (Math/abs (- a b)))
Reflection warning, NO_SOURCE_PATH:1:15 - call to abs can't be resolved.
#'user/f
user=> (f 1000000000000 2000000000000)
1000000000000
user=> (class (f 1000000000000 2000000000000))
java.lang.Long

Above behavior observed with Clojure 1.5.1 on these JVMs:

Ubuntu 12.04.2 plus this JVM:
java version "1.7.0_21"
Java(TM) SE Runtime Environment (build 1.7.0_21-b11)
Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

Mac OS X 10.8.3 plus this JVM:
java version "1.7.0_15"
Java(TM) SE Runtime Environment (build 1.7.0_15-b03)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Comment by Matthew Willson [ 29/May/13 5:17 AM ]

Ah, interesting.
Maybe it's a difference in the way the reflection API works in java 7?

Here's the bytecode generated incase anyone's curious:

public java.lang.Object invoke(java.lang.Object);
Code:
0: ldc #14; //String java.lang.Math
2: invokestatic #20; //Method java/lang/Class.forName:(Ljava/lang/String;)Ljava/lang/Class;
5: ldc #22; //String abs
7: iconst_1
8: anewarray #24; //class java/lang/Object
11: dup
12: iconst_0
13: aload_1
14: aconst_null
15: astore_1
16: aastore
17: invokestatic #30; //Method clojure/lang/Reflector.invokeStaticMethod:(Ljava/lang/Class;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/Object;
20: areturn

Comment by Matthew Willson [ 29/May/13 5:20 AM ]

Just an idea (and maybe this is what's happening under java 7?) but given it's a static method and all available overloaded variants are presumably known at compile time, perhaps it could generate code along the lines of:

(cond
(instance? Long x) (Math/abs (long x))
(instance? Integer x) (Math/abs (int x))
;; ...
)

Comment by Andy Fingerhut [ 29/May/13 3:19 PM ]

In Reflector.java method invokeStaticMethod(Class c, String methodName, Object[] args) there is a call to getMethods() followed by a call to invokeMatchingMethod(). getMethods() returns the 4 java.lang.Math/abs methods in different orders on Java 6 and 7, causing invokeMatchingMethod() to pick a different one on the two JVMs:

java version "1.6.0_39"
Java(TM) SE Runtime Environment (build 1.6.0_39-b04)
Java HotSpot(TM) 64-Bit Server VM (build 20.14-b01, mixed mode)

user=> (pprint (seq (clojure.lang.Reflector/getMethods java.lang.Math 1 "abs" true)))
(#<Method public static int java.lang.Math.abs(int)>
#<Method public static long java.lang.Math.abs(long)>
#<Method public static float java.lang.Math.abs(float)>
#<Method public static double java.lang.Math.abs(double)>)
nil

java version "1.7.0_21"
Java(TM) SE Runtime Environment (build 1.7.0_21-b11)
Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

user=> (pprint (seq (clojure.lang.Reflector/getMethods java.lang.Math 1 "abs" true)))
(#<Method public static double java.lang.Math.abs(double)>
#<Method public static float java.lang.Math.abs(float)>
#<Method public static long java.lang.Math.abs(long)>
#<Method public static int java.lang.Math.abs(int)>)
nil

That might be a sign of undesirable behavior in invokeMatchingMethod() that is too dependent upon the order of methods given to it.

As you mention, type hinting is good for avoiding the significant performance hit of reflection.

Comment by Alex Miller [ 02/Aug/15 8:24 PM ]

Not reproducible on 1.8.0-alpha3.

Comment by Nicola Mometto [ 03/Aug/15 9:52 AM ]

Alex, I could reproduce using 1.8.0-master-SNAPSHOT and jdk 1.8:

[~]> java -version
java version "1.8.0_45"
Java(TM) SE Runtime Environment (build 1.8.0_45-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.45-b02, mixed mode)
[~]> clj
Clojure 1.8.0-master-SNAPSHOT
user=> (set! *warn-on-reflection* true)
true
user=> (#(Math/abs %) 1000000000000)
Reflection warning, NO_SOURCE_PATH:2:3 - call to static method abs on java.lang.Math can't be resolved (argument
727379968
user=> (class *1)
java.lang.Integer

Andy's last comment mentions that clojure.lang.Reflector.invokeStaticMethod is dependant on the order of methods passed to it and that that order can change between jdk versions so maybe that's why you couldn't reproduce it

Comment by Andy Fingerhut [ 03/Dec/18 9:31 PM ]

Possibly the same as, or at least some commonality with, CLJ-1921.





[CLJ-2445] Use "constructor" instead of "ctor" in Compiler error messages Created: 26/Nov/18  Updated: 03/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Jakub Holý Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

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

 Description   

In this example:

user=> (java.net.URL. #_"missing arg")
Syntax error (IllegalArgumentException) compiling new at (REPL:1:1).
No matching ctor found for class java.net.URL

the use of "ctor" is confusing. It matches the code variable name, but the user isn't looking at that.

Patch: CLJ-2445.patch - replace "ctor" with "constructor" in error messages.

Prescreened by: Alex Miller



 Comments   
Comment by Jakub Holý [ 26/Nov/18 8:00 AM ]

patch CLJ-2445.patch added 11/26

Comment by Alex Miller [ 26/Nov/18 9:59 AM ]

Can you add a repro example to the description?

Comment by Jakub Holý [ 03/Dec/18 11:23 AM ]

add repro example to descr.

Comment by Alex Miller [ 03/Dec/18 12:09 PM ]

Removed the second example as it's the same as the first (which was from your original, but with 1.10 result printing, which is where we're starting from now).





[CLJ-2433] Invalid calls to clojure.set functions return an incorrect answer rather than error Created: 16/Nov/18  Updated: 02/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Erik Assum Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Triaged

 Description   

There are a number of tickets concerned with the fact that the set functions in clojure.set misbehave when passed arguments that are not sets.

This set of issues include CLJ-810, CLJ-1087, CLJ-1682, and CLJ-1954

The functions affected by this are:

  1. difference
  2. intersection
  3. union
  4. subset?
  5. superset?

as these are known to produce unexpected results when passed non-set arguments.

Problem
As the above mentioned issues suggest, todays implementation of these functions leads to confusion and erroneous results when called with non-set input. The user is given no warning or indications of the error he's making.

Possible solutions

  1. Add a coercion to set on the arguments said functions
  2. Throw an exception when the arguments are not sets
  3. Handle this with clojure.spec
  4. Leave it as is

Tradeoffs

  1. Given CLJ-2362, which makes a call to set close to a noop, the coercion should not incur much of a performance penalty. It has been argued that the code might even be faster, as type hints can be given and the compiler/jit might make better choices. For the common mistakes (passing vectors/lists instead of sets) it should be backwards compatible
  2. Throwing an exception on non-set arguments would break programs which work correctly today (although by chance), such as data.diff.
  3. Handling it with clojure.spec seems like a viable option, but again, this would break data.diff if the functions were spec'ed to both receive and return sets.
  4. Leaving it as it is, and we will continue to surprise both new and old clojurists.

Evidence of this being a problem

  1. The tickets mentioned above seem to indicate that people stumble upon this often enough to file issues
  2. https://clojuredocs.org/clojure.set/superset_q#example-5b5acd38e4b00ac801ed9e39


 Comments   
Comment by Alex Miller [ 16/Nov/18 1:34 PM ]

Before being considered, this ticket needs:

  • a good problem statement
  • an assessment of where existing code calls these functions with something other than sets
  • a table of alternatives and their tradeoffs. presumably alternatives are: add specs, add validation checks, add coercions, etc. Tradeoffs may include effects on existing callers (known or unknown), performance, etc.

Decisions to make:

  • Are existing calls (with inputs that are not sets) valid or invalid?
  • What change should be made in the functions

Patches are probably not super useful until those things are decided.

Comment by Michiel Borkent [ 17/Nov/18 6:52 AM ]

Interesting data from a repo by Andy Fingerhut comparing the performance of:

  • versions of set functions with pre-conditions (no coercion)
  • instrumented set functions

The functions with pre-conditions are significantly faster than the instrumented ones, but not much slower than the originals.

https://github.com/jafingerhut/funjible#wait-isnt-this-what-clojurespec-is-for

Comment by Stuart Halloway [ 18/Nov/18 7:40 AM ]

A spec test could compare inputs and outputs and not break any existing code.





[CLJ-2426] satisfies? doesn't work with the new instance-based protocol polymorphism Created: 06/Nov/18  Updated: 29/Nov/18

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

Type: Defect Priority: Major
Reporter: Juraj Martinka Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: protocols

Attachments: Text File 0001-CLJ-2426-fix-satisfies-for-new-instance-based-protoc.patch    
Patch: Code
Approval: Triaged

 Description   

I'd expect that `satisfies?` works even when providing protocol implementation via metadata, however that doesn't seem to be the case:

(defprotocol Foo :extend-via-metadata true (foo [x]))
(foo (with-meta [42] {`foo (fn [x] :boo)}))
;; => :boo

;; but `satisfies?` doesn't work
(satisfies? Foo (with-meta [42] {`foo (fn [x] :boo)}))
;; => false

Patch: 0001-CLJ-2426-fix-satisfies-for-new-instance-based-protoc.patch



 Comments   
Comment by Alex Miller [ 29/Nov/18 8:08 PM ]

Would it be better to put the metadata check second instead of first for perf? Or is it actually faster?





[CLJ-1095] Allow map-indexed to accept multiple collections (a la map) Created: 25/Oct/12  Updated: 29/Nov/18

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

Type: Feature Priority: Trivial
Reporter: Bo Jeanes Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-map-indexed-accepts-multiple-collections.patch     Text File 0002-Add-test-for-multi-collection-map-indexed-fn.patch     Text File CLJ-1601 map-indexed and tests 8-5-2017.patch    
Approval: Vetted

 Description   

Bring external interface of map-indexed in line with map. Existing usages of map-indexed unchanged both in implementation and interface.

examples
(map vector (range 10 20) (range 30 35)) ;=> ([10 30] [11 31] [12 32] [13 33] [14 34])
(map-indexed vector (range 10 20) (range 30 35)) ;=> ([0 10 30] [1 11 31] [2 12 32] [3 13 33] [4 14 34])

Patch: CLJ-1601 map-indexed and tests 8-5-2017.patch - also adds tests for map-indexed

Prescreened by: Alex Miller



 Comments   
Comment by Aaron Bedra [ 25/Oct/12 5:11 PM ]

Can you add a test for the improved functionality?

Comment by Bo Jeanes [ 25/Oct/12 5:20 PM ]

You bet. I tried to before submitting this but found no existing tests for map-indexed to expand upon. Given that, I decided to just start the conversation first. If you think this is a good addition, I'll find a place to stick the tests and add a new patch file.

Comment by Bo Jeanes [ 25/Oct/12 8:05 PM ]

Add two unit tests for map-indexed. One tests old behavior (single collection) and the second tests mapping across 3 collections.

There were no existing tests for map-indexed that I could see to expand upon (using git grep map-indexed src/clojure)

Comment by Justin Spedding [ 31/Jul/17 9:38 PM ]

This feature would be very useful. Is there a reason that no one continued to work on this in almost 5 years?

Comment by Alex Miller [ 31/Jul/17 10:32 PM ]

Predates my involvement with Clojure so don't know but don't see any reason why not.

Comment by Andy Fingerhut [ 31/Jul/17 10:36 PM ]

If a ticket has not been moved to the Vetted state by Rich Hickey, it is unknown whether it is of interest to be added to Clojure at all. The default answer to any change to Clojure, until there is a compelling argument to the core Clojure developers otherwise, is "no". That said, JIRA tickets tend to be left open rather than declined if there is no compelling argument for declining it.

For some more background, see here, in particular the "Clojure Governance and How It Got That Way" article linked from there: https://dev.clojure.org/display/community/Contributing+FAQ

For details on Vetted, and other JIRA ticket statuses used by the Clojure core team, see here: https://dev.clojure.org/display/community/JIRA+workflow

Voting on tickets (there is a "Vote" link near the top right of the page if you log in to your JIRA account first) can in some cases help get attention to them sooner than tickets without votes, but there are no guarantees on time lines.

Comment by Alex Miller [ 31/Jul/17 10:43 PM ]

I didn't try but I'm sure this patch no longer applies. Having a single combined patch that can be applied would be a useful first step.

Comment by Justin Spedding [ 05/Aug/17 7:40 PM ]

I just uploaded a new patch for map-indexed that allows it to accept multiple collections. I took the exact same approach that map does, but with the addition of the index. It has better performance than the old patch from 2012. I also included more tests. Both the function update and the new tests are in "CLJ-1601 map-indexed and tests 8-5-2017.patch"

Also, map-indexed had 0 tests prior to me writing the patch. That seems a bit dangerous for a function in core to be untested.

Comment by Justin Spedding [ 04/Oct/17 11:37 AM ]

It's been a little while since I uploaded the patch here and it was prescreened. Both this ticket and another that I created are awaiting vetting, and the FAQ says that I should tend to my tickets. Is there something else that I should be doing to tend to it and move it along?

Comment by Alex Miller [ 04/Oct/17 2:37 PM ]

I don't think we'll consider any more enhancements in 1.9 timeframe.

Comment by Justin Spedding [ 29/Nov/18 12:16 PM ]

It occurs to me that `keep` and `keep-indexed` also do not accept multiple collections. Perhaps I should make a similar ticket for updating those.

Comment by Alex Miller [ 29/Nov/18 12:19 PM ]

Yes, separate ticket for those would be good. I don’t know of one in existence yet.





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 29/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: ft, performance

Attachments: Text File 0001-Use-transient-map-in-zipmap.2.patch     Text File 0001-Use-transient-map-in-zipmap.patch     Text File 0002-CLJ-1005-use-transient-map-in-zipmap.patch     Text File CLJ-1005-zipmap-iterators.patch    
Patch: Code
Approval: Vetted

 Description   

#'zipmap constructs a map without transients, where transients could improve performance.

Approach: Use a transient map internally, along with iterators for the keys and values. A persistent map is returned as before. The definition is also moved so that it resides below that of #'transient.

Performance:

(def xs (range 16384))
(def ys (range 16))

expression 1.7.0-beta3 +patch  
(zipmap xs xs) 4.50 ms 2.12 ms large map
(zipmap ys ys) 2.75 us 2.07 us small map

Patch: CLJ-1005-zipmap-iterators.patch

Screened by: Alex Miller



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 9:24 PM ]

Why is the old implementation left and commented out? If we are going to move to a new implementation, the old one should be removed.

Comment by Michał Marczyk [ 15/Aug/12 4:17 AM ]

As mentioned in the ticket description, the previously attached patch follows the pattern of into whose non-transient-enabled definition is left in core.clj with a #_ in front – I wasn't sure if that's something desirable in all cases.

Here's a new patch with the old impl removed.

Comment by Andy Fingerhut [ 15/Aug/12 10:37 AM ]

Thanks for the updated patch, Michal. Sorry to raise such a minor issue, but would you mind using a different name for the updated patch? I know JIRA can handle multiple attached files with the same name, but my prescreening code isn't quite that talented yet, and it can lead to confusion when discussing patches.

Comment by Michał Marczyk [ 15/Aug/12 10:42 AM ]

Thanks for the heads-up, Andy! I've reattached the new patch under a new name.

Comment by Andy Fingerhut [ 16/Aug/12 8:24 PM ]

Presumptuously changing Approval from Incomplete back to None after the Michal's updated patch was added, addressing the reason the ticket was marked incomplete.

Comment by Aaron Bedra [ 11/Apr/13 5:32 PM ]

The patch looks good and applies cleanly. Are there additional tests that we should run to verify that this is providing the improvement we think it is. Also, is there a discussion somewhere that started this ticket? There isn't a lot of context here.

Comment by Michał Marczyk [ 11/Apr/13 6:19 PM ]

Hi Aaron,

Thanks for looking into this!

From what I've been able to observe, this change hugely improves zipmap times for large maps. For small maps, there is a small improvement. Here are two basic Criterium benchmarks (transient-zipmap defined at the REPL as in the patch):

;;; large map
user=> (def xs (range 16384))
#'user/xs
user=> (last xs)
16383
user=> (c/bench (zipmap xs xs))
Evaluation count : 13920 in 60 samples of 232 calls.
             Execution time mean : 4.329635 ms
    Execution time std-deviation : 77.791989 us
   Execution time lower quantile : 4.215050 ms ( 2.5%)
   Execution time upper quantile : 4.494120 ms (97.5%)
nil
user=> (c/bench (transient-zipmap xs xs))
Evaluation count : 21180 in 60 samples of 353 calls.
             Execution time mean : 2.818339 ms
    Execution time std-deviation : 110.751493 us
   Execution time lower quantile : 2.618971 ms ( 2.5%)
   Execution time upper quantile : 3.025812 ms (97.5%)

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 25.4675 % Variance is moderately inflated by outliers
nil

;;; small map
user=> (def ys (range 16))
#'user/ys
user=> (last ys)
15
user=> (c/bench (zipmap ys ys))
Evaluation count : 16639020 in 60 samples of 277317 calls.
             Execution time mean : 3.803683 us
    Execution time std-deviation : 88.431220 ns
   Execution time lower quantile : 3.638146 us ( 2.5%)
   Execution time upper quantile : 3.935160 us (97.5%)
nil
user=> (c/bench (transient-zipmap ys ys))
Evaluation count : 18536880 in 60 samples of 308948 calls.
             Execution time mean : 3.412992 us
    Execution time std-deviation : 81.338284 ns
   Execution time lower quantile : 3.303888 us ( 2.5%)
   Execution time upper quantile : 3.545549 us (97.5%)
nil

Clearly the semantics are preserved provided transients satisfy their contract.

I think I might not have started a ggroup thread for this, sorry.

Comment by Andy Fingerhut [ 03/Sep/14 8:10 PM ]

Patch 0001-Use-transient-map-in-zipmap.2.patch dated Aug 15 2012 does not apply cleanly to latest master after some commits were made to Clojure on Sep 3 2014.

I have not checked whether this patch is straightforward to update. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Michał Marczyk [ 14/Sep/14 12:48 PM ]

Thanks, Andy. It was straightforward to update – an automatic rebase. Here's the updated patch.

Comment by Ghadi Shayban [ 22/Sep/14 9:58 AM ]

New patch using clojure.lang.RT/iter, criterium shows >30% more perf in the best case. Less alloc probably but I didn't measure. CLJ-1499 (better iterators) is related

Comment by Justin Spedding [ 29/Nov/18 12:08 PM ]

4 years later, this zipmap implementation in the core namespace is still not using a transient map internally. Is there a reason why this was never applied?

Comment by Alex Miller [ 29/Nov/18 12:18 PM ]

Multiple approaches proposed here, no consensus approach determined yet. Needs some focus time to narrow that down, and hasn’t been a priority yet.





[CLJ-2251] [spec] Generic spec walking for clojure.spec Created: 11/Oct/17  Updated: 27/Nov/18

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

Type: Feature Priority: Major
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 14
Labels: None
Environment:

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



 Description   

Problem

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

Proposal

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

Current status

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

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

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

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

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

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

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

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

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

Suggestion

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

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

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

Results

Single sweep validation & coercion. Happy runtime.



 Comments   
Comment by Tommi Reiman [ 17/Apr/18 7:40 AM ]

Renamed the issue. Instead of Keyword argument, it should take a function to walk the spec to support arbitrary walking applications.

Comment by Marco Molteni [ 10/Jul/18 5:11 PM ]

hello, any news ?

Comment by Alex Miller [ 10/Jul/18 6:20 PM ]

No plans to look at this before the next batch of implementation changes, so it will be a while.





[CLJ-2413] Non-deterministic method selection during reflection Created: 04/Oct/18  Updated: 26/Nov/18

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

Type: Defect Priority: Critical
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reflection

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

 Description   

Reflection does not return the same order of methods across JVM invocations (via Class::getMethods). This is probably most apparent when submitting Clojure functions, which are Callable and Runnable, into an executor:

(ns repro)

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

(defn repro
  []
  (let [exec (java.util.concurrent.ForkJoinPool/commonPool)
        work #(do 1)]
    (deref (.submit ^Object exec work))))  ;; intentionally reflect

(defn -main []
   ;; dereffing a runnable returns nil, a callable can return a value, in this case an integer
  (System/exit (or (repro) 0)))
nondeterministic ➜ while true; do clojure -m repro; echo $?; done
0
0
0
1
0
0
0
1
0
0
0


 Comments   
Comment by Ghadi Shayban [ 25/Nov/18 5:36 PM ]

attached a patch that sorts seen methods

Comment by Ghadi Shayban [ 26/Nov/18 8:51 AM ]

re patch: the comparator's first pass (examining the arity) can disappear: only identical arity methods get compared. Does that sound right?

Comment by Nicola Mometto [ 26/Nov/18 11:04 AM ]

Yeah, that sounds right to me

Comment by Ghadi Shayban [ 26/Nov/18 11:42 AM ]

patch with a more minimal comparator





[CLJ-2443] [spec] Spec'ed fn doesn't throw when called lazily Created: 23/Nov/18  Updated: 26/Nov/18

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

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

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

 Description   

Summary:

1) Given instrumented function map-f that is called lazily, e.g. as (def ls (map map-f (range))).
2) Also given instrumented function varargs-f, a varargs function. When you pass a LazySeq to varargs-f some or all elements are realized as a consequence of conforming.
3) The problem: when calling (apply varargs-f ls) some invalid calls to map-f can go unnoticed.

Repro example:

In the following code map-f is expected to throw when called with something else than a Symbol. However the call to map-f with a String slips through.

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

(defn map-f [x]
  (println "called my-fn with type" (type x))
  (println (deref #'clojure.spec.test.alpha/*instrument-enabled*))
  {x 1})

(s/fdef map-f :args (s/cat :x symbol?))

(defn varargs-f [& maps]
  true)

(s/fdef varargs-f :args (s/cat :maps (s/* map?)))

(defn repro [& args]
  (apply varargs-f (map map-f args)))

(stest/instrument)

(repro 'foo 'bar "baz")

Output:

called my-fn with type clojure.lang.Symbol
true
called my-fn with type clojure.lang.Symbol
true
called my-fn with type java.lang.String ;; <--
nil

Cause:

When varargs-f's arguments are realized as a result of conforming, some calls to map-fn are made in the scope of with-instrument-disabled: https://github.com/clojure/spec.alpha/blob/f23ea614b3cb658cff0044a027cacdd76831edcf/src/main/clojure/clojure/spec/test/alpha.clj#L140

Background:

I ran into this issue when spec'ing merge-with. Spec'ed fns in some test namespaces didn't throw anymore, because this line in clojure.test has a similar problem with regards to spec as described above: https://github.com/clojure/clojure/blob/28efe345d5e995dc152a0286fb0be81443a0d9ac/src/clj/clojure/test.clj#L775

CLJ-2443.patch contains a fix, but I realize it may not be perfect yet. Therefore I provided CLJ-2443-test.patch that only contains the unit test which can be used to test an alternative solution.



 Comments   
Comment by Michiel Borkent [ 24/Nov/18 9:50 AM ]

CLJ-2443.patch fixes this problem by conforming a Cons type argument outside the scope of with-instrument-disabled.
Test provided. Conforming this patch works with speculative (https://github.com/slipset/speculative).

Comment by Michiel Borkent [ 25/Nov/18 7:27 AM ]

CLJ-2443-test only contains the test, not the fix itself.

Comment by Michiel Borkent [ 25/Nov/18 7:29 AM ]

Updated description with CLJ-2443-test.patch





[CLJ-2271] [spec] "caller" information missing in explain-data during macro instrumentation failure Created: 19/Nov/17  Updated: 26/Nov/18

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

Type: Defect Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: error-reporting, spec
Environment:

org.clojure/spec.alpha "0.1.143"


Approval: Triaged

 Description   

When there is a instrumentation failure for a function, the explain-data includes "caller" information. However, this information is missing if the instrumentation failure is for a macro.

This comment has led me to believe that the intended behavior is for explain-data to contain this info, so third-party error printers can display it.

In the repro below, I'm setting up a custom printer just to capture the raw explain-data (it's not a useful printer, just a means to show what is happenening)

Repro:

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


  (s/fdef my-fn
          :args (s/cat :x int?))
  (defn my-fn [x]
    x)

  (s/fdef my-macro
          :args (s/cat :x int?))
  (defmacro my-macro [x]
    x)

  (st/instrument)
  (def !ed (atom nil))
  (set! s/*explain-out* (fn [ed]
                          (reset! !ed ed)))
  (my-fn "")
  @!ed
  ;; {:clojure.spec.alpha/problems [{:path [:args :x], :pred clojure.core/int?, :val "", :via [], :in [0]}], :clojure.spec.alpha/spec #object[clojure.spec.alpha$regex_spec_impl$reify__2436 0x72029b0e "clojure.spec.alpha$regex_spec_impl$reify__2436@72029b0e"], :clojure.spec.alpha/value (""), :clojure.spec.alpha/args (""), :clojure.spec.alpha/failure :instrument, :clojure.spec.test.alpha/caller {:file "form-init8333540581183382896.clj", :line 548, :var-scope expound.alpha/eval27394}}

  ;; ^--- Note there is an entry for :clojure.spec.test.alpha/caller

  (my-macro "")
  @!ed

  ;; #:clojure.spec.alpha{:problems [{:path [:args :x], :pred clojure.core/int?, :val "", :via [], :in [0]}], :spec #object[clojure.spec.alpha$regex_spec_impl$reify__2436 0x479a6a73 "clojure.spec.alpha$regex_spec_impl$reify__2436@479a6a73"], :value (""), :args ("")}

  ;; ^--- No caller information


 Comments   
Comment by Alex Miller [ 27/Nov/17 8:39 AM ]

You can't instrument a macro, so that part of the ticket doesn't make sense as written. But I expect you mean the spec check during macro expansion.

In the macro check case, the caller info is known by the compiler and included in the wrapper CompilerException. I suppose that info could be passed into s/macroexpand-check from the Compiler and possibly produce similar results as with instrumented function calls.

Comment by Ben Brinckerhoff [ 19/Mar/18 6:39 PM ]

Ah, you're correct, thanks for clarifying.

If caller information was added, it would also be very useful to add a specific `:clojure.spec.alpha/failure` value for specs that fail during macro expansion. That would allow 3rd party tools to show specific types of error messages when macro expansion failed.

Comment by Ben Brinckerhoff [ 19/Mar/18 6:41 PM ]

Additionally, having access to the symbol for the macro name would assist in error reporting.

Comment by Shogo Ohta [ 27/Mar/18 9:54 PM ]

IMHO explain-data for instrumentation failures and macro spec errors should contain the fspec of the function (or macro), as I suggested before in CLJ-2218.

An fspec has some useful info (such as the ns-qualified name symbol and the line number etc.) of the spec'ed function in its metadata, so spec error reporters can use them for richer and more precise error messages in a uniform way for both instrumentation failures and macro spec errors.

Comment by Alex Miller [ 22/Aug/18 9:44 AM ]

After CLJ-2373, the caller information for spec macro failures will be conveyed in a wrapper CompilerException (as with other compiler and macroexpansion errors). Should consider whether this issue is effectively resolved as a result.

Comment by Ben Brinckerhoff [ 22/Aug/18 10:02 AM ]

I'm glad to hear this will be included in the CompilerException, but I think it still is useful to include in the spec explain-data.

When printing a spec error, it would be clearer if macro expansion spec errors and instrumentation errors could be printed in a similar format (if the user chooses a custom spec printer). It may be disruptive to look for the caller information in one place for macro expansion errors and a different place for instrumentation errors.

Furthermore, spec printers may choose to do novel things with this caller information like adding color to output or printing the actual code instead of the line numbers. It'd be nice to do have all this information available for the spec printer to customize.

Comment by Alex Miller [ 22/Aug/18 10:34 AM ]

The issue here is that at the point where the macro spec error is created, we don't have any way to know the proper caller information. The stack at that point is the compiler macroexpansion, not the invoking code. macroexpansion does know that context and that's what it adds when wrapping with a CompilerException (that's the primary purpose of CompilerException).

I think to change that we would need to convey that info out of band into the spec check. Well, maybe that's possible. I'll look at it again.

Comment by Ben Brinckerhoff [ 25/Nov/18 5:53 PM ]

With the recent error message work (great work!) the symbol appears to be available, but it not currently passed to the "explain-out" function: https://github.com/clojure/clojure/blob/b182982007df934394f0bc68b3a238ca9f200dd1/src/clj/clojure/main.clj#L268-L279

Would it be possible to "assoc" this into the "spec" passed to "spec/explain-out"? This would allow custom printers to refer to the name of the spec, which would be very useful.

An alternative approach would be for users to set up custom error handlers in the REPL, but this is much more involved than setting up a third-party implementation of "explain-out"

Comment by Alex Miller [ 26/Nov/18 6:49 AM ]

I’d hate to make this something only useable by the clojure main repl tgough - would be better to happen always if using some other repl, so would be better to do at construction. I have a better handle on how to do so now, but don’t know if it’s too late for 1.10.





[CLJ-1879] reduce-kv on hash-maps or array-maps don't consistently execute the intended fastpath Created: 09/Jan/16  Updated: 25/Nov/18

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: 6
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)

Another approach is to extend the protocol to PersistentHashMap and PersistentArrayMap directly (in addition to preserving the existing extensions)



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

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

Comment by Ghadi Shayban [ 25/Nov/18 12:21 PM ]

Repro code

(def C (atom 0))

;; reextend for instrumentation
(extend-protocol clojure.core.protocols/IKVReduce
 ;;slow path default
 clojure.lang.IPersistentMap
 (kv-reduce 
   [amap f init]
   (swap! C inc)
   (reduce (fn [ret [k v]] (f ret k v)) init amap)))

(reduce-kv (fn [_ _ _]) nil (array-map 1 2 3 4))
(println "slowpaths" @C)
(reduce-kv (fn [_ _ _]) nil (hash-map 1 2 3 4))
(println "slowpaths" @C)

Repro output

➜  dev /usr/lib/jvm/java-8-openjdk/bin/java -cp $(clojure -Spath) clojure.main kvfastpath.clj
slowpaths 1
slowpaths 2
➜  dev /usr/lib/jvm/java-11-openjdk/bin/java -cp $(clojure -Spath) clojure.main kvfastpath.clj
slowpaths 0
slowpaths 0




[CLJ-2444] Typo in docstring of test-vars Created: 23/Nov/18  Updated: 23/Nov/18

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

Type: Enhancement Priority: Trivial
Reporter: Michiel Borkent Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Patch fixes docstring: "runs test-vars" should be "runs test-var".






[CLJ-1326] Inconsistent reflection warnings when target is a literal Created: 19/Jan/14  Updated: 22/Nov/18

Status: Reopened
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: errormsgs


 Description   
user=> (set! *warn-on-reflection* true)
true
user=> (.contains [] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [1] 0)
false
user=> (.contains ^:foo [1] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [(inc 1)] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false

Even worse, type hinting doesn't get picked up :

user=>  (.contains ^java.util.Collection [] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=>  (.contains ^java.util.Collection [(inc 1)] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=>  (.contains ^java.util.Collection (identity [(inc 1)]) 0)
false

Similar issues apply to other literals



 Comments   
Comment by Alex Miller [ 20/Nov/18 9:13 PM ]

This is not public api, does not seem worth messing with.

Comment by Nicola Mometto [ 21/Nov/18 8:47 AM ]

the clojure collections are at least documented to implement `java.util.Collection`, so the below example showcasing the same exact issue should be valid:

user=> (set! *warn-on-reflection* true)
true
user=> (.contains [] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [1] 0)
false
user=> (.contains ^:foo [1] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
user=> (.contains [(inc 1)] 0)
Reflection warning, NO_SOURCE_PATH:1:1 - call to method contains on clojure.lang.IPersistentVector can't be resolved (no such method).
false
Comment by Nicola Mometto [ 22/Nov/18 5:51 AM ]

I'm reopening this since the ticket is actually valid now that I've changed example – and even worse than that I noticed that it's impossible to get rid of the reflection even with explicit type hinting





[CLJ-2442] Clojure test fails on Windows 10 Created: 21/Nov/18  Updated: 21/Nov/18

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

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test
Environment:

Windows 10 with JDK 11 at least, and likely most JDKs on most versions of Windows


Attachments: Text File clj-2442-v1.patch    
Patch: Code and Test

 Description   

A test compares a string with line separators in it against a literal string with newline characters in it, which works on systems with newlines as the line terminator, but not Windows. Using clojure.string/split-lines on the literal string and the function return value before comparing fixes it, as was done for an earlier failing Clojure test a few years ago.



 Comments   
Comment by Andy Fingerhut [ 21/Nov/18 4:08 PM ]

Patch clj-2442-v1.patch fixes the failing test when building on Windows, and also passes on Mac OS X. It should work fine on Linux, too.





[CLJ-2439] The 'do symbol is skipped in some contexts where it shouldn't be Created: 20/Nov/18  Updated: 21/Nov/18

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-Don-t-skip-over-do-in-expression-context.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Expressions like this result in nil, but should fail to compile:

(let [] do)

Expressions like this result in nil, but should produce a value:

(let [do 1] do)

This is true for all special forms with an "implicit do": try/catch/finally, letfn*, let*, and the fn*/reify/deftype family of forms.

The way that "implicit do" is implemented for these special forms is that the relevant Parser delegates to BodyExpr.Parser explicitly for its body expression. For example,

(let [x 1] (println x) x)

calls BodyExpr.Parser.parse() with the list '((println x) x). However, BodyExpr.Parser is also used to parse lists like '(do (println x) x), in other contexts. To handle both cases, its parse() method skips its first form if that form is the symbol 'do. So, if a special form with an "implicit do" has as its first expression the bare symbol 'do, that symbol is incorrectly discarded.

The attached patch fixes this by making each such special form actually insert an explicit 'do before delegating, so that BodyExpr.Parser can unconditionally assume that its first form is 'do, and skip it. BodyExpr now throws an exception if this is not the case, as this would indicate an error in the compiler - user code cannot create this situation.

The attached patch will have minimal impact on compilation time: it introduces one extra method call and one extra cons cell allocation for each analysis of an expression with an implicit do. It will have no impact on generated code (except that it will fix the errors indicated in the beginning of this ticket).

Three tests are included which fail before the patch but succeed after it.



 Comments   
Comment by Alan Malloy [ 20/Nov/18 4:47 PM ]

The previous version of the patch accidentally included only the test, and not the fix. This patch contains both.

Comment by Nicola Mometto [ 21/Nov/18 8:59 AM ]

I think this may be a dupe of CLJ-1216, not sure how the two patches compare

Comment by Alan Malloy [ 21/Nov/18 3:05 PM ]

Yes, definitely a dupe of that. I like your patch better, too.





[CLJ-1419] Report errors on missing param list or return type of methods in gen-class and gen-interface Created: 10/May/14  Updated: 20/Nov/18

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

Type: Enhancement Priority: Trivial
Reporter: Nathan Zadoks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, gen-class

Attachments: Text File 0001-CLJ-1419-default-to-void-return-type-in-gen-interfac.patch     Text File 0001-CLJ-1419-map-nil-to-void-in-prim-class.patch     File clj1419.clj     Text File fail.log    
Patch: Code

 Description   

The following are invalid and should produce errors when invoked on gen-class or gen-interface:

(gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])  ;; no params, throws error
(gen-interface :name clj1419.IFail :methods [[myMethod []]]) ;; no return type
(gen-interface :name clj1419.IFail :methods [[myMethod]])  ;; no params or return type

The first example throws an error. The second and third do not but will generate an invalid class, verify with:

(.getMethods clj1419.IFail)
ClassNotFoundException java.lang.  java.net.URLClassLoader$1.run (URLClassLoader.java:366)

Add checks to prevent these errors.



 Comments   
Comment by Nathan Zadoks [ 10/May/14 1:34 PM ]

I've implemented both fixes, and attached them as patches.

Comment by Nathan Zadoks [ 10/May/14 1:40 PM ]

I'd argue that the behaviour of asm-type is at fault here (it can output an invalid type name when passed a nil argument), so I prefer that fix over the purely symptomatic generate-interface fix.

Comment by Andy Fingerhut [ 10/May/14 2:33 PM ]

Nathan, were you planning on submitting a signed Clojure Contributor's Agreement, or already have? Details here if you have not: http://clojure.org/contributing

Patches from non-contributors cannot be committed to Clojure.

Note: I cannot promise you that one of your patches will be accepted into Clojure if you sign a CA – only that it will not if you do not sign one.

Comment by Alex Miller [ 10/May/14 4:19 PM ]

Please add an example of how this happens and the current error.

Comment by Nathan Zadoks [ 11/May/14 3:45 AM ]

Andy — Yep, I've read up on that. My CA will be underway to Rich soon. (filled in, signed, in an envelope, just need to await the arrival of those bloody international stamps…)

Alex Miller — Tahdah!

A demonstration of the issue, both attached and as a gist: https://gist.github.com/nathan7/3a7e3a09e458f1354cbb

Comment by Nathan Zadoks [ 11/May/14 3:48 AM ]

and here's log of the compiler crash that results (also added to the gist now)

Comment by Nathan Zadoks [ 11/May/14 4:27 AM ]

Whoops, both of my patches were rather broken due to a misunderstanding on my side.
I forgot entirely that asm-type takes a symbol, not a string.
Modifying asm-type was definitely a bad idea, that check just looks whether it should defer to prim->class.
Adding nil to prim->class would work (and I've attached my patch for that too), but it's starting to look rather inelegant compared to just patching gen-interface.
(on a side note: I'm having a lot of fun exploring the Clojure codebase! thanks for that, humans!)

Comment by Alex Miller [ 11/May/14 7:26 AM ]

My reading of the docstring of gen-interface is that method declarations must specify a parameter list and a valid return type. I would expect all of these to be invalid:

(gen-interface :name clj1419.IFail :methods [[fail nil]])
(gen-interface :name clj1419.IFail :methods [[fail [] nil]])
(gen-interface :name clj1419.IFail :methods [[fail []]])

"nil" is not a valid type - you can use "void" for this purpose and this works fine:

(gen-interface :name clj1419.IFail :methods [[fail [] void]])

If this ticket is (as the title states) a request to allow omitting the return type or using "nil" as a return type, then I think the answer is no. If the ticket is a request to improve the error reporting of the failure cases above, then I think we can consider that but it will be very low priority.

Comment by Nathan Zadoks [ 12/May/14 8:19 AM ]

The code seems to suggest otherwise though, seeing the explicit extra branch for pclasses being nil.
As much as I like PL trivia, I haven't run into `void` in Clojure anywhere else yet, and I'm surprised to see it here.
Maintaining the principle of least surprise seems preferable to pedantry about whether nil is a type: (= "nil" (str (type (.methodReturningVoid obj)))

Comment by Alex Miller [ 12/May/14 8:26 AM ]

The two places to look for words to rely on are docstrings and the http://clojure.org/documentation pages. Implementation details are just that.

"nil" is not a type. "void" is a documented type identifier indicating the absence of a return value - http://clojure.org/java_interop#Java%20Interop-Aliases

Comment by Nathan Zadoks [ 12/May/14 8:27 AM ]

Okay. Better error-checking in asm-type then?

Comment by Alex Miller [ 12/May/14 8:49 AM ]

I have updated the title and description based on my understanding of what this ticket should be, which is enhanced error-checking on the method specs for gen-class and gen-interface. I'm not sure if that's in asm-type or somewhere earlier.

Comment by Alex Miller [ 20/Nov/18 9:17 PM ]

As of 1.10, these all throw errors now:

user=> (gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])
Syntax error macroexpanding gen-interface at (REPL:1:1).
Don't know how to create ISeq from: clojure.lang.Symbol

user=> (gen-interface :name clj1419.IFail :methods [[myMethod []]])
Unexpected error (ClassFormatError) macroexpanding gen-interface at (REPL:1:1).
Class name contains illegal character in descriptor in class file clj1419/IFail

user=> (gen-interface :name clj1419.IFail :methods [[myMethod]])
Unexpected error (ClassFormatError) macroexpanding gen-interface at (REPL:1:1).
Class name contains illegal character in descriptor in class file clj1419/IFail




[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 20/Nov/18

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-2.diff     File clj-1400-3.diff     File clj-1400-4.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
Syntax error compiling def at (REPL:1:1).
Can't create defs outside of current ns

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch: clj-1400-4.diff

Screened by: Alex Miller



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)
Comment by Andy Fingerhut [ 29/Aug/14 4:46 PM ]

Patch clj-1400-1.diff dated Jun 26 2014 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Comment by Scott Bale [ 31/Aug/14 3:53 PM ]

Attached is an updated patch: "clj-1400-2.diff". I removed the stale patch.

Comment by Alex Miller [ 09/Sep/14 9:29 AM ]

Few comments to address:

  • Compiler diff was using spaces, not tabs, which makes it harder to diff. I attached a -3.diff that fixes this.
  • the call to namesStaticMember seems weird. The name of that method is confusing for this use. Beyond that, I think it's doing more than you need. That method is going to attempt resolve the qualified name in terms of the current ns, but I think you don't even want to do that. Rather you just want to know if the sym has a ns (sym.ns != null) - isn't that enough?
  • In what case will the other error "Var doesn't exist" occur? In other words, in what case will lookupVar not succeed in creating a new var here? If there is no such case, then remove this case. If there is such a case, then add a test.
Comment by Scott Bale [ 11/Sep/14 11:19 PM ]

Agree with all three of your bullets. Attached is an updated patch, clj-1400-4.diff.

  • I used tabs in Compiler.java
  • After close inspection of call to lookupVar(...), I believe null is returned only in the case of exactly this ticket (the symbol having a non-null namespace which has not been loaded yet). So I've taken out the conditional and the 2nd branch.
  • (Test is unchanged)
Comment by Scott Bale [ 11/Sep/14 11:22 PM ]

(properly named patch)

Comment by Alex Miller [ 11/Sep/14 11:37 PM ]

You could throw a CompilerException with the location of the problem instead (as the ticket description suggests).

Comment by Scott Bale [ 19/Sep/14 2:37 PM ]

Sorry, I should've mentioned because this wasn't obvious to me either (and in fact I forgot until just now): the RuntimeException is already caught and wrapped in a CompilerException.

I'm not sure which try-catch block within Compiler.java this is happening in, there are multiple. But you can see in the output that the exception is a CompilerException and the file|line|col info is there:

In the Repl...

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

...or in a source file

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)

Also, at the point at which the RuntimeException of this patch is being thrown, the source line and col params to CompilerException are not available, or at least not afaict.

Comment by Alex Miller [ 07/Oct/14 12:34 PM ]

I'll follow up on this patch later - Rich thought it was making too many assumptions. I think we validated many of those but need to double-check those.

Comment by Alex Miller [ 20/Nov/18 9:16 PM ]

Updated error result as of Clojure 1.10. It is now throwing a :compile-syntax error phase exception with file/line/col too.





[CLJ-2225] clojure.core/assert docstring is incorrect Created: 22/Aug/17  Updated: 20/Nov/18

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

Type: Enhancement Priority: Minor
Reporter: Gordon Syme Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: docstring

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

 Description   

The assert macro has two arities, documented as (assert x) and (assert x message)

The docstring is:

Evaluates expr and throws an exception if it does not evaluate to logical true.

This is quite misleading since assert actually throws an Error. It is tempting to use assert assuming that a failure will lead to a stacktrace in logs, like any other Exception, but the reality is that the JVM will terminate.

The behaviour is correct, assert should cause a program to exit if the asserted condition is true but I regularly come across incorrect uses.

I'll work up a patch if people agree this is an issue.



 Comments   
Comment by Alex Miller [ 22/Aug/17 5:54 AM ]

Error is a subclass of Throwable, just like Exception and has no special behavior (although it does have the special intended meaning that most programs should not catch it). Whether or not your program does catch it, or exit, or log, is totally up to your program and not a property of being an Error.

The only change that I think might make sense here is to be more specific about the exception type.

Comment by Gordon Syme [ 13/Sep/17 5:46 AM ]

clj-2225-20170913.patch updates the assert macro docstring. I've wrapped the docstring at 70 characters, it seems to be a fairly common width used throughout the rest of the file.

Comment by Marc O'Morain [ 20/Nov/18 7:23 AM ]

There is one further omission in the docstring (both current and proposed) - neither mention that the behavior of `assert` depends on the value of the `clojure.core/assert` dynamic var.





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

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

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

Attachments: Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v2.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v3.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v4.patch     Text File 0001-CLJ-1814-cache-protocol-impl-satisfies-as-fast-as-me-v5.patch     Text File CLJ-1814-v6.patch     Text File CLJ-1814-v7.patch    
Patch: Code and Test
Approval: Vetted

 Description   

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

With:

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

Before patch:

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

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

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

After patch:

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

Patch: CLJ-1814-v7.patch (depends on CLJ-2426)



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

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

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

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

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

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

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

is it possible that AOT is involved?

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

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

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

I don't see where AOT would be involved?

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

Michael Blume The updated patch should fix the issue you reported

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

Cool, thanks =)

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

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

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

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

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

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

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

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

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

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

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

clojure master:

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

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

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

master + v4:

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

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

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

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

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

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

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

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

Michael - that check is already in there

Nicola - I have a few comments/questions:

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

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

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

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

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

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

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

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

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

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

(proto-fn false)

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

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

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

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

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

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

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

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

yes, should definitely add

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

Patch with test

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

Verified that test fails with v4 patch:

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

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

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

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

Comment by Nicola Mometto [ 20/Nov/18 7:16 AM ]

Updated patch to work after the new instance-based protocol dispatch, this ticket should wait for CLJ-2426 first





[CLJ-2431] clojure.java.io/resource NPEs when the context ClassLoader is null Created: 15/Nov/18  Updated: 15/Nov/18

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

Type: Defect Priority: Major
Reporter: mccraigmccraig Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: java.io
Environment:

clojure 1.9


Approval: Triaged

 Description   

clojure.java.io/resource assumes the current Thread's context ClassLoader will be non-null. if the Thread's context ClassLoader is null then it NPEs

(resource n (.getContextClassLoader (Thread/currentThread)))

the javadocs indicate it may reasonably be null, and i'm seeing a case in the wild where it's non-null - https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#getContextClassLoader--

clojure.java.io/resource should perhaps default to using (ClassLoader/getSystemClassLoader) when the context ClassLoader is null






[CLJ-2333] Support java.nio.file.Path in clojure.java.io Created: 07/Mar/18  Updated: 14/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: io

Attachments: Text File CLJ-2333-1.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

java.nio.file.Path objects are largely equivalent to java.io.File. They represent a location in a (possibly-remote) filesystem.

Coercions in clojure.java.io don't recognize java.nio.file.Path. It would be nice if they did.

Prescreened: Alex Miller

Patch: CLJ-2333-1.patch



 Comments   
Comment by Andrew Oberstar [ 27/Jun/18 9:37 PM ]

Added a patch that supports java.nio.file.Path in Coercions and IOFactory. There are more places (particularly the do-copy multimethod) that might warrant Path support, but this seemed like a good start.

Note, in case there's any confusion, I have signed the CA (June 17th), though my name is not yet on the site's list of CA signers.

Comment by Arne Brasseur [ 14/Nov/18 2:19 AM ]

java.nio.file.Path was added in Java 7, which is perhaps why this change didn't make it in before. Clojure 1.10 will require Java 8, so perhaps it's time to consider this patch.





[CLJ-2428] Separate compiler exceptions into :compilation and :compile-syntax-check Created: 13/Nov/18  Updated: 13/Nov/18

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

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


 Description   

With the new error reporting phases in Clojure 1.10, we identified two separate phases - :compile-syntax-check (for syntax errors found by compiler) and :compilation for errors that occur during compilation but that are not syntax errors.

Example of the latter:

user=> a/b
Syntax error compiling at (REPL:0:0).
No such namespace: a
user=> (ex-data *e)
#:clojure.error{:phase :compile-syntax-check, :line 0, :column 0, :source "NO_SOURCE_PATH"}

The syntax here is fine, just a is not a known namespace.

The Compiler adheres to no patterns in differentiating between these two. We should go through and make this consistent, either through exception types or explicitly specifying the phase in a CompilerException.






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

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

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

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

 Description   

Description

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

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

(t/instrument)

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

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

As you can see,

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

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

Possible resolutions

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

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

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

The same goes for macroexpand-check.



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

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

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

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

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

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

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

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

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

Comment by Ben Brinckerhoff [ 15/Nov/17 9:02 PM ]

One thing that I think would be useful is for an instrumentation failure to better state the invocation (combining the function and the args into the original call). Right now those are separated and it take some mental work to knit the arg list back together.

Apologies if I'm missing something, but is it even possible to get the function from the explain-data? I don't see it in explain-data, but perhaps it is recoverable somehow? In any case, I would greatly appreciate it if this type of information was available, since it would make it possible to print clearer error messages.

Comment by Shogo Ohta [ 11/Nov/18 10:05 PM ]

Hey, is this already done by CLJ-2392?





[CLJ-2365] Consider integration with java.util.function interfaces Created: 26/Jun/18  Updated: 11/Nov/18

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 13
Labels: interop

Attachments: Text File some-java-fns-interface.patch    
Approval: Vetted

 Description   

Moving to Java 8 as a baseline allows us to consider built-in ties to critical java.util.Function interfaces like Function, Predicate, Supplier, etc. Needs assessment about what's possible and what automatic integration it would open for users.

https://docs.oracle.com/javase/8/docs/api/java/util/function/package-summary.html



 Comments   
Comment by Jason Whitlark [ 29/Jun/18 5:40 PM ]
;; I dug this out of some scratch code experimenting with kafka streams.  All the reify's were filled with java8 lambdas in the original.

;; I'll dig up another example that shows examples using stuff from java.utils.funstion.* 

;;Some of this was lifted from a franzy example or something?

;; Note that, for example, 
;; https://kafka.apache.org/0102/javadoc/org/apache/kafka/streams/kstream/Predicate.html
;; is different from
;; https://docs.oracle.com/javase/8/docs/api/java/util/function/Predicate.html

(ns utils
  (:import (org.apache.kafka.streams.kstream Reducer KeyValueMapper ValueMapper Predicate))

(defmacro reducer [kv & body]
  `(reify Reducer
     (apply [_# ~(first kv) ~(second kv)]
       ~@body)))

;; public interface KeyValueMapper<K,V,R>
;; apply(K key, V value)
(defmacro kv-mapper [kv & body]
  `(reify KeyValueMapper
     (apply [_# ~(first kv) ~(second kv)]
       ~@body)))

;; public interface ValueMapper<V1,V2>
;; apply(V1 value)
(defmacro v-mapper [v & body]
  `(reify ValueMapper
     (apply [_# ~v]
       ~@body)))

(defmacro pred [kv & body]
  `(reify Predicate
     (test [_# ~(first kv) ~(second kv)]
       ~@body)))

;; I used it something like this:

(ns our-service.kafka-streams
  (:require
   [our-service.util :as k]
   [clojure.string :as str]
  (:import 
           (org.apache.kafka.streams StreamsConfig KafkaStreams KeyValue)
           (org.apache.kafka.streams.kstream KStreamBuilder ValueMapper)))

(defn create-word-count-topology []
  (let [builder (KStreamBuilder.)
        init-stream (.stream builder (into-array ["streams-str-input"]))
        wc (-> init-stream
            (.flatMapValues (k/v-mapper [& value]
                                        (str/split (apply str value) #"\s")))
            (.map (k/kv-mapper [k v]
                               (KeyValue/pair v v)))
            (.filter (k/pred [k v]
                             (println v)
                             (not= v "the")))
            (.groupByKey)
            (.count "CountStore")
            show-item
            ;; this needs to be mapValues
            (.mapValues (reify ValueMapper
                          (apply [_ v]
                            (println v)
                            (str v))))
            (.toStream)
            (.to "wordcount-output"))]
    [builder wc]))
Comment by Ghadi Shayban [ 09/Jul/18 10:30 PM ]

The JLS infers lambda types by hunting for matching functional interfaces AKA "Single Abstract Method" classes [1] (whether they're interfaces or abstract classes.) We could have a reify-like helper that detects these classes [2]. You would have to hint the target class. We don't really need things that are both `IFn` and `j.u.f.Predicate`, etc.

(import '[java.util.function Predicate Consumer])

(let [orig [1 2 3]
      st (atom [])]
  (.forEach orig (jfn Consumer [x] (swap! st conj x)))
  (= @st orig))

[1] https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.8
[2] spike https://gist.github.com/ghadishayban/0ac41e81d4df02ff176c22d16ee8b972

Comment by Jason Whitlark [ 11/Jul/18 12:26 PM ]

Well, that would be an improvement. The practical problem I run into is that I'm frequently deep in a fluent interface, and don't necessarily know the exact class. That said, it's usually only in a few places. Would it make sense to have a registry? Perhaps something like:

(auto-infer-lambda [java.util.function, org.apache.kafka.streams.kstream])

Comment by Ghadi Shayban [ 11/Jul/18 3:08 PM ]

Do you ever use SAM classes that are abstract classes and not interfaces?

Comment by Andrew Oberstar [ 11/Jul/18 6:10 PM ]

Here's an alternative approach in my ike.cljj library. This uses MethodHandles (i.e. java.lang.invoke package) instead of regular reflection. I'm not sure if I tested this on abstract classes yet.

The usage looks similar to what Ghadi posted:

(defsam my-sam
  java.util.function.Predicate
  [x]
  (= x "it matched"))

(-> (Stream/of "not a match" "it matched")
    (.filter my-sam)
    (.collect Collectors/toList)

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

It uses MethodHandleProxies.asInterfaceInstance to create a proxy instance of the interface that calls a method handle calling a Clojure function. It doesn't try to validate parameter counts, it just treats it as varargs delegating to IFn.applyTo(ISeq). Not sure if it's the most efficient but it was effective for my needs.

I think the LambdaMetaFactory may be the preferred way to meet this type of use case. It was harder for me to follow exactly how to use that though so I didn't end up looking to deep into it.

The main functional issue I have with my approach (and Ghadi's) is that you have to explcitly provide the interface you want to proxy. Java lambdas and Groovy Closures can be used against methods that expect a SAM and it just coerces based on what the method expects. Ideally this would be supported by Clojure too.

Instead of having to do:

(-> (IntStream/range 0 10)
    (.filter (sam* java.util.function.IntPredicate odd?))
    (.collect Collectors/toList)

I'd like to do this:

(-> (IntStream/range 0 10)
    (.filter odd?)
    (.collect Collectors/toList)
Comment by Ghadi Shayban [ 07/Aug/18 2:30 PM ]

Another possible avenue is to extend java.util.function.Supplier to Clojure functions with an explicit 0-arity. That interface is becoming increasingly common in practice; it might be valuable to special-case it. (We shouldn't & couldn't do a similar thing for defrecords, as they already have a method called get, which clashes with Supplier's only method.)

Comment by Marc-André Tremblay [ 22/Sep/18 4:44 PM ]

Moving to Java 8 as a baseline allows us to use default interface methods.

The `some-java-fns-interface.patch` patch implements Consumer, Function, Predicate and Supplier on IFn.

If you want to go this route, I would be very happy to implement all interfaces under java.util.function on IFn as well as the accompanying tests. I currently use this code to play with FoundationDB through its Java client and it works well for me.

https://github.com/marctrem/clojure/commit/97742493f674edd8f6c034ee94da84fa62a76bad

Comment by Jason Whitlark [ 11/Nov/18 4:36 PM ]

If I could just use IFn's anywhere a java.util.function.* was needed, that would be fantastic!





[CLJ-2422] Confusing function name suffixes in error messages Created: 03/Nov/18  Updated: 03/Nov/18

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

Type: Enhancement Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, errormsgs
Environment:

Clojure 1.10.0-beta4



 Description   

Some core function names are reported in error messages with an odd munging suffix.

For example, when passing the wrong number of arguments to inc, assoc, and dissoc, each error message reports the function name differently:

clj -Srepro -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.10.0-beta4"}}}'
user=> (inc)
Syntax error compiling inc at (1:1).
Wrong number of args (0) passed to: clojure.core/inc--inliner--5436
user=> (assoc)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (0) passed to: clojure.core/assoc--5316
user=> (dissoc)
Evaluation error (ArityException) at clojure.lang.AFn.throwArity (AFn.java:429).
Wrong number of args (0) passed to: clojure.core/dissoc

From the user's point of view, this behaviour is unmotivated and confusing. Perhaps such names should be further demunged and printed without the suffix.



 Comments   
Comment by Alex Miller [ 03/Nov/18 1:35 PM ]

The first one here is an inlined function and happens during compilation, so is a little different than the others. Because it's inlined, it's not spec-able. The name parts there are generated within the compiler as well. This one might be fixable based on the regularity of the name construction, although I don't see it as a particularly high priority.

The latter two actually will be a bit different once we apply CLJ-2420:

user=> (assoc)
Execution error (ArityException) at user$eval154/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/assoc--5406
user=> (dissoc)
Execution error (ArityException) at user$eval156/invokeStatic (REPL:1).
Wrong number of args (0) passed to: clojure.core/dissoc

Changes here are actually eliding all the internal frames and reporting the top user frame now (here, it happens to be in the REPL invocation, but if it was in a user namespace, you'd see that instead).

I guess in both cases, the numbers you're seeing are a standard technique used throughout Clojure so the question might be whether it's possible to address this in a systematic way. Functions don't inherently carry their unmunged name right now, so demunging from class name is never going to be a foolproof mechanism.

Worth thinking about how to do this better, but not something I expect to look at soon.





[CLJ-1820] Move rename-keys from clojure.set to clojure.core Created: 28/Sep/15  Updated: 02/Nov/18

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

Type: Enhancement Priority: Trivial
Reporter: Lars Andersen Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: None


 Description   

rename-keys is hard to find when it lives in the clojure.set namespace, because it acts on maps and not sets. To my eyes set/rename-keys also looks strange when reading code, but this is the preferred way to bring in the clojure.set namespace.

This is one of the minor warts I'd like to see fixed in clojure 2.0.



 Comments   
Comment by Gordon Syme [ 04/Jul/17 4:56 AM ]

It's not just rename-keys, there are a few fns in clojure.set that don't make sense being there, at first glance anyway.

It certainly harms discoverability of these fns.

I've come across at least one re-implementation of rename-keys and map-invert during my day job because the author didn't know these fns exist.

I'd argue for breaking the relational and map fns out of clojure.set into their own namespaces and deffing some vars in clojure.set for backwards compatibility.
Those compatibility vars could be deleted in 1.10.

I'm happy to do this (or another approach) but would like some buy in on the approach from the core team first.

Comment by Nicola Mometto [ 04/Jul/17 6:25 AM ]

I think this is very, very unlikely to ever happen

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

We will not delete/move existing vars as this would break existing programs.

Comment by Marc O'Morain [ 12/Jul/17 4:49 AM ]

> We will not delete/move existing vars as this would break existing programs.

This issue can be addressed without deletion as per Gordon's suggestion:

A new var could be added to clojure.core named clojure.core/rename-keys. Then the rename-keys var in clojure.set can be defined as:

(def rename-keys clojure.core/rename-keys)
Comment by Alex Miller [ 12/Jul/17 6:08 AM ]

I am aware of that, which is why I did not close the issue. I was just stating one possible resolution that is off the table.

Comment by Bozhidar Batsov [ 30/Oct/18 4:18 PM ]

Out of curiosity - does anyone know how/why those functions ended up in the `clojure.set` namespace?

P.S. If someone decides to alias things between namespaces I'd suggest creating a `clojure.map` ns over pouring more stuff into core.

Comment by Kevin Downey [ 30/Oct/18 6:28 PM ]

a more accurate name for clojure.set based on its contents would be something like clojure.relational-algebra, where it defines something like relational algebra over sets of maps instead of over sets of tuples. A common operation in that case is renaming keys in maps (similar to using AS in a sql query). Which explains why rename-keys is in clojure.set

Comment by Andy Fingerhut [ 02/Nov/18 12:05 AM ]

Bozhidar - only a guess, not knowledge. Relations are sets of records/tuples, so to me it makes some sense that functions on relations are in clojure.set. rename-keys is probably there because it is used by two of the functions operating on relations.





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

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: 4
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.



 Comments   
Comment by Alex Miller [ 02/Nov/17 3:13 PM ]

In this case, it's considering the s/* to be a non-match and then matching the (Box. 0) against (s/coll-of integer?). This matches the initial predicate (coll?) but falls through to the :else case which presumes it can call `empty`.

You can work around it by adding a :kind predicate to the coll-of:

(s/conform
        (s/cat :boxes (s/* #(instance? Box %))
               :name (s/coll-of integer? :kind #(not (record? %))))
        [(Box. 0) [5]])
;;=> {:boxes [#user.Box{:a 0}], :name [5]}
Comment by Phil Jones [ 19/Jul/18 2:41 PM ]

But why does it need to do empty in the first place?

Does this mean that you can't check collections of records in spec?

Comment by Zach Oakes [ 01/Nov/18 9:22 AM ]

I ran into this myself. Conforming a record to a coll-of spec should show a normal spec error, not throw from deep inside spec's implementation.





[CLJ-2212] docstring for `defmethod` is imprecise Created: 28/Jul/17  Updated: 31/Oct/18

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

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

Attachments: Text File clj-2212.patch     Text File clj-2212-updated-docstring.patch     Text File clj-2212-updated-params.patch    
Patch: Code
Approval: Triaged

 Description   

arglist:

(defmethod multifn dispatch-val & fn-tail)

docstring:

Creates and installs a new method of multimethod associated with dispatch-value.

There are a few issues with this docstring that make it hard to understand:

1. The & fn-tail argument is not documented. What is the type / shape of & fn-tail. I don't see any mention in the reference guide either - https://clojure.org/reference/multimethods
2. The docstring and arglists conflict about the name multimethod vs. multifn.
3. The docstring and arglists conflict about the name dispatch-value vs. dispatch-val.

Aside: on clojuredocs.org there is a mention of an optional name being allows on the method https://clojuredocs.org/clojure.core/defmethod#example-542692c7c026201cdc3269cd but I don't see that documented anywhere official. I'm not sure if it supported.



 Comments   
Comment by Alex Miller [ 28/Jul/17 9:00 AM ]

fn-tail here means "additional args as could be passed to fn", which does include the function name, so that is supported.

Comment by Marc O'Morain [ 19/Aug/17 3:01 PM ]

Adding patch

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

Rather than changing the docstring from multimethod to multifn, I would change the code from multifn to multimethod. Otherwise looks good.

Comment by Marc O'Morain [ 22/Oct/18 10:38 AM ]

Adding updated patch that changes the argument name to match the docstring, rather than the other way around.

Comment by Alex Miller [ 22/Oct/18 4:48 PM ]

I'd prefer only the docstring to change here for the dispatch-val (so docstring changing to match the code).

Comment by Marc O'Morain [ 26/Oct/18 11:38 AM ]

Patch that changes only the docstring.

I've not changed the word multimethod to multifn in the docstring. I believe that even though the parameter is not documented, the name multimethod is more clear.

Can you elaborate on why you want to change the docstring rather than the argument name please Alex?

Comment by Alex Miller [ 31/Oct/18 3:57 PM ]

I changed my mind and I think it would be better to just change the docstring to match the arg names.





[CLJ-2419] Multimethods don't have the correct method name in stacktraces. Created: 22/Oct/18  Updated: 23/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, multimethods


 Description   

Describing the user-facing problem:

When an exception is thrown and there is a multimethod in the stacktrace, the name of the multimethod is not shown in the callstack.

When I execute this code:

(defmulti dispatcher (fn [item] (:type item)))

(defmethod dispatcher "cat"
  [{:keys [lives]}]
  (/ lives 0))

(dispatcher {:type "cat" :lives 9})
*e

I get the following stack trace:

[clojure.lang.Numbers divide "Numbers.java" 163]
[clojure.lang.Numbers divide "Numbers.java" 3813]
[config_compilation.core$eval20467$fn__20469 invoke "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 198]
[clojure.lang.MultiFn invoke "MultiFn.java" 229]
... snip ...

The function name is config_compilation.core$eval20467$fn__20469.

If I use defn to declare dispatcher directly, I get the following stacktrace:

(defn dispatcher
  [{:keys [lives]}]
  (/ lives 0))

(dispatcher {:type "cat" :lives 9})
*e
[clojure.lang.Numbers divide "Numbers.java" 163]
[clojure.lang.Numbers divide "Numbers.java" 3813]
[config_compilation.core$dispatcher invokeStatic "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 202]
[config_compilation.core$dispatcher invoke "/Users/marc/dev/circleci/orb-registry/server/src/config_compilation/core.clj" 200]
... snip ...

The function name is given correctly as config_compilation.core$dispatcher.

I'm guessing that the issue could be that the defmulti macro not setting the :name metadata, or perhaps not passing the :name metadata to the compiler correctly.



 Comments   
Comment by Alex Miller [ 22/Oct/18 4:43 PM ]

So, it's kind of interesting if you look into the details of this. Technically, at the point where the exception occurs, the defmulti is not on the stack (that's the dispatch function, which has already completed), but the defmethod is.

defmethods are actually defined as fn's and are typically treated as anonymous fns, although it's actually possible to name them independently:

user=> (defmulti dispatcher (fn [item] (:type item)))
#'user/dispatcher
user=> (defmethod dispatcher "cat" cat-method
         [{:keys [lives]}]
         (/ lives 0))
#object[clojure.lang.MultiFn 0x6e1d4137 "clojure.lang.MultiFn@6e1d4137"]
user=> (dispatcher {:type "cat" :lives 9})
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:163)
user=> (pst *e 5)
ArithmeticException Divide by zero
	clojure.lang.Numbers.divide (Numbers.java:163)
	clojure.lang.Numbers.divide (Numbers.java:3813)
	user/eval155/cat-method--157 (NO_SOURCE_FILE:11)         ;; <== cat-method
	clojure.lang.MultiFn.invoke (MultiFn.java:229)
	user/eval161 (NO_SOURCE_FILE:12)

I think what would be best is if the defmethod function defaulted to a unique name based on the defmulti function name (if no name was specified).

Comment by Alex Miller [ 22/Oct/18 4:45 PM ]

CLJ-2212 is related

Comment by Marc O'Morain [ 23/Oct/18 9:30 AM ]

Oh, interesting.

The symbol that identifies the multimethod (dispatcher) in this case appended with some sort of string representation of the dispatch value would be a really nice default value here.

So you might have functions named
`dispatcher-default`, `dispatcher-cat`, etc.

I assume that you might have to add some additional uniqueness suffix on the end, too.





[CLJ-2418] [spec] (s/exercise string?) produces narrow string samples Created: 20/Oct/18  Updated: 20/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: JAre Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generative-test, generator, spec, string

Approval: Triaged

 Description   

(s/exercise string?) seems to never produce strings with delimiters or spaces. I noticed this while testing my string formatting function with generative testing - it was extremely surprising.

It looks like a potential source of low quality tests and bad surprises.






[CLJ-1103] Make conj assoc dissoc and transient versions handle args similarly Created: 04/Nov/12  Updated: 11/Oct/18

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

Type: Feature Priority: Minor
Reporter: Andy Fingerhut Assignee: Alex Miller
Resolution: Unresolved Votes: 10
Labels: collections, transient

Attachments: Text File clj-1103-10.patch     File clj-1103-7.diff     Text File clj-1103-8.patch     Text File clj-1103-9.patch    
Approval: Triaged

 Description   

Examples that work as expected:

Clojure 1.7.0-master-SNAPSHOT
user=> (dissoc {})
{}
user=> (disj #{})
#{}
user=> (conj {})
{}
user=> (conj [])
[]

Examples that do not work as desired, but are changed by the proposed patch:

user=> (assoc {})
ArityException Wrong number of args (1) passed to: core/assoc  clojure.lang.AFn.throwArity (AFn.java:429)
user=> (assoc! (transient {}))
ArityException Wrong number of args (1) passed to: core/assoc!  clojure.lang.AFn.throwArity (AFn.java:429)
user=> (dissoc! (transient {}))
ArityException Wrong number of args (1) passed to: core/dissoc!  clojure.lang.AFn.throwArity (AFn.java:429)

;; patch enables conj! with multiple arguments, like conj
user=> (conj! (transient []) 1 2 3)
ArityException Wrong number of args (4) passed to: core/conj!  clojure.lang.AFn.throwArity (AFn.java:429)

;; patch would give error for missing value for last key to assoc!, not silently use nil for last value
user=> (assoc! (transient {}) 1 2 3)
#object[clojure.lang.PersistentArrayMap$TransientArrayMap 0x2e7569b8 "clojure.lang.PersistentArrayMap$TransientArrayMap@2e7569b8"]

I looked through the rest of the code for similar cases, and found that there were some other differences between them in how different numbers of arguments were handled, such as:

+ conj handles an arbitrary number of arguments, but conj! does not.
+ assoc checks for a final key with no value specified (CLJ-1052), but assoc! did not.

History/discussion: A discussion came up in the Clojure Google group about conj giving an error when taking only a coll as an argument, as opposed to disj which works for this case:

https://groups.google.com/forum/?fromgroups=#!topic/clojure/Z9mFxsTYTqQ

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Nov/12 6:04 PM ]

clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt dated Nov 4 2012 makes conj conj! assoc assoc! dissoc dissoc! handle args similarly to each other.

Comment by Brandon Bloom [ 09/Dec/12 5:30 PM ]

I too ran into this and started an additional discussion here: https://groups.google.com/d/topic/clojure-dev/wL5hllfhw4M/discussion

In particular, I don't buy the argument that (into coll xs) is sufficient, since into implies conj and there isn't an terse and idiomatic way to write (into map (parition 2 keyvals))

So +1 from me

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

Patch clj-1103-2.diff is identical to the previous patch clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt except it applies cleanly to latest master. The only changes were some changed context lines in a test file.

Comment by Andy Fingerhut [ 23/Nov/13 12:52 AM ]

Patch clj-1103-3.diff is identical to the patch clj-1103-2.diff, except it applies cleanly to latest master. The only changes were some doc strings for assoc! and dissoc! in the context lines of the patch.

Comment by Andy Fingerhut [ 14/Feb/14 12:04 PM ]

Patch clj-1103-4.diff is identical to the previous clj-1103-3.diff, except it updates some context lines so that it applies cleanly to latest Clojure master as of today.

Comment by Alex Miller [ 05/Jun/14 9:29 PM ]

Can someone update the description with some code examples? Or drop them here at least?

Comment by Brandon Bloom [ 05/Jun/14 9:35 PM ]

What do you mean code examples?

These currently work as expected:
(dissoc {})
(disj #{})

The following fail with arity errors:
(assoc {})
(conj {})

Similarly for the transient ! versions.

This is annoying if you ever try to do (apply assoc m keyvals)... it works at first glance, but then one day, bamn! Runtime error because you tried to give it an empty sequence of keyvals.

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

Patch clj-1103-5.diff dated Aug 6 2014 applies cleanly to latest Clojure master as of today, whereas the previous patch did not. Rich added 1-arg version of conj to 1.7.0-alpha1, so that change no longer is part of this patch.

Comment by Andy Fingerhut [ 29/Aug/14 6:04 PM ]

Patch clj-1103-6.diff dated Aug 29 2014 is identical to the former patch clj-1103-5.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Comment by Andy Fingerhut [ 26/May/15 9:00 PM ]

Patch clj-1103-7.diff dated May 25 2015 is identical to the former patch clj-1103-6.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Comment by Michael Blume [ 12/Oct/15 5:32 PM ]

Updated patch to apply cleanly to master

Comment by Michael Blume [ 06/Oct/17 4:16 PM ]

Updated patch

Comment by Andy Fingerhut [ 06/Oct/17 6:00 PM ]

Is it intentional that clj-1103-9.patch leaves out the changes to assoc! that were in the earlier patches? I can create another updated one if so – just curious if there was a reason for the change.

Comment by Michael Blume [ 06/Oct/17 7:17 PM ]

That was not intentional at all, thanks for catching it, I'm honestly not sure what happened on my end to make that happen. I think this patch should be a correct update. Sorry about that.

Comment by Andy Fingerhut [ 09/Oct/17 10:17 PM ]

No worries, and I thank you for keeping the patch current, even while keeping my name on it.





[CLJ-2123] Lighter-weight aliasing for keywords Created: 10/Mar/17  Updated: 11/Oct/18

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

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

Approval: Vetted

 Description   

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Comment by Tom Locke [ 11/Oct/18 4:51 AM ]

Is there any reason such a mechanism would be limited to keywords? I have a place in my code where it would be very useful to have such a feature for symbols.

Also, related: a way for pr-str (and similar) to be configured to use short versions of namespaces. Debug output can be awfully noisy, even with namespaced maps.

Comment by Alex Miller [ 11/Oct/18 10:29 AM ]

No particular reason - usually for stuff like this we make it work the same for any ident (keyword or symbol).





[CLJ-2112] [spec] Add specs for spec forms Created: 15/Feb/17  Updated: 08/Oct/18

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

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

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

 Description   

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

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

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



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

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

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

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

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

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

Comment by Alexander Kiel [ 27/Jun/18 7:34 AM ]

In the current patch, keywords referencing specs are missing except for keys in `s/keys`. What would the right way to model the keywords? One way would be to add `qualified-keyword?` as fourth option to the or-spec of `::spec`. Than `::spec` could be used as spec for the spec arg in `s/valid?`, `s/conform`, `s/form` and others.

Comment by Alex Miller [ 27/Jun/18 8:07 AM ]

Yeah, that would probably be good.

Comment by Alexander Kiel [ 27/Jun/18 10:45 AM ]

Another observation: Currently s/and and s/or are allowed to contain no preds:

(defspec clojure.spec.alpha/and (s/* ::spec))
(defspec clojure.spec.alpha/or (s/* (s/cat :tag keyword? :pred ::spec)))

there should be either a clear semantic definition what an empty form will mean or we should just use s/+ here and require at least one pred. For the and-form, we could define that (s/and) is the same as (s/and any?) and the same as just any?. For the or-form it is difficult, because s/conform returns tagged results. So conforming any value with (s/or) will result in ::s/invalid which renders an empty or-form useless.

Comment by Alex Miller [ 27/Jun/18 10:54 AM ]

There is a clear semantic definition for these - they are the same as clojure.core/and and clojure.core/or. As far as I'm concerned, this is all correct.

Comment by Alexander Kiel [ 28/Jun/18 7:33 AM ]

Thanks. I see. The documentation in clojure.core defines that (clojure.core/and) returns true and (clojure.core/or) returns nil.





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

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

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

Attachments: Text File vartag2.patch    
Approval: Vetted

 Description   

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

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

Patch: vartag2.patch



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

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

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

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

(is (and ...))

within the last let-binding.

Comment by Ghadi Shayban [ 17/Mar/18 2:21 PM ]

Printing unnamed vars has little utility (nothing distinguishes them from each other without knowing their code context), seems like it would be fine to have #clojure/var nil or #clojure/var :unnamed





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

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

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

Approval: Vetted

 Description   

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

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


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

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





[CLJ-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 08/Oct/18

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

Type: Enhancement Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: math

Attachments: Text File clj-1435.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

It's confusing to not support numerator and denominator on integer types as this requires you to always check ratio? before invoking them.

Proposed: Extend numerator and denominator to also work on integer types (long, BigInt, BigInteger) by routing to overloaded methods on Numbers for the desired types.

Patch: clj-1435.patch

Screened by: Alex Miller

Questions from screening for Rich:

1. numerator and denominator are tagged as returning java.math.BigInteger (not clojure.lang.BigInt) and that's what I followed in the patch. Seems like maybe that should be BigInt though? Not sure on what basis to make that decision.
2. Should numerator and denominator accept both BigInteger and BigInt?



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

Comment by Gary Fredericks [ 27/Aug/15 10:38 AM ]

I've definitely written the helper functions Andy describes on several occasions.

Comment by Felipe Micaroni Lalli [ 01/Sep/15 4:58 PM ]

Related issue: https://stackoverflow.com/questions/25194809/how-to-convert-any-number-to-a-clojure-lang-ratio-type-in-clojure

A workaround to that is (numerator (clojure.lang.Numbers/toRatio (rationalize <put any type of number here>)))

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

I agree with the intent of the ticket here that these should work. I'm not sure about the protocol approach as that would be an open system and I'm not sure that's what we want. An alternative would be to just create Java methods on Numbers that took the appropriate types and let the JVM sort it out.

Comment by Aaron Brooks [ 08/Apr/17 12:38 PM ]

Any progress on this?

Comment by Alex Miller [ 08/Apr/17 1:28 PM ]

Sitting in the queue, waiting for love.

Comment by Aaron Brooks [ 08/Apr/17 3:54 PM ]

Can I give it love? If there's a direction that we can agree on, I'd be happy to create the patch.

Comment by Alex Miller [ 08/Apr/17 6:13 PM ]

I've screeened the patch that's here already?

Comment by Andy Fingerhut [ 27/Sep/18 4:32 AM ]

The new versions of numerator and denominator after this patch was applied perform reflective calls. Is that expected?

Comment by Andy Fingerhut [ 27/Sep/18 5:06 AM ]

Created ticket CLJ-2408 to make the question more prominent than a comment on a closed ticket.

Comment by Alex Miller [ 05/Oct/18 10:58 PM ]

Patch reverted





[CLJ-1895] Remove loading of clojure.string in clojure.java.io Created: 22/Feb/16  Updated: 08/Oct/18

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

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

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

 Description   

clojure.core loads clojure.java.io to define slurp and spit. clojure.java.io loads clojure.string, solely for a single call to replace. This slows down Clojure core startup for no reason.

Approach: Replace clojure.string/replace call with a Java interop call to .replace. This saves about 18 ms during Clojure core startup.

Patch: clj-1895.patch






[CLJ-1360] Doc that clojure.string/split strips trailing delimiters Created: 18/Feb/14  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: docstring, string

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

 Description   

clojure.string/split and clojure.string/split-lines inherit the bizarre default behavior of java.lang.String#split(String,int) in stripping trailing consecutive delimiters:

(clojure.string/split "banana" #"an")
⇒ ["b" "" "a"]
(clojure.string/split "banana" #"na")
⇒ ["ba"]
(clojure.string/split "nanabanana" #"na")
⇒ ["" "" "ba"]

In the case of split-lines, processing a file line by line and rejoining results in truncation of trailing newlines in the file. In both cases, the behavior is surprising and cannot be inferred from the docstrings. A workaround for split is to pass a limit of -1.

Proposed: As current users may be relying on the current behavior, the attached merely updates the docstring to warn of this behavior and suggest use of -1 as a limit to workaround.

Patch: clj-1360-2.patch



 Comments   
Comment by Andy Fingerhut [ 18/Feb/14 10:51 AM ]

Probably documenting would be safer than changing the behavior at this point, given that some people may actually rely on the current behavior after testing, deploying, etc.

I don't currently have a suggestion for a modified doc string, but note that there are examples of this behavior and how one can use an extra "-1" limit argument at the end to get all split strings: http://clojuredocs.org/clojure_core/clojure.string/split

Comment by Crispin Wellington [ 21/May/15 10:46 PM ]

This bug just bit me. +1 to be fixed. If we just document and leave the behavior as is, then we have a surprising and inconsistent behaving split (why are inner empty values kept, but outer ones stripped?) that is different to every other split you've ever used. The optional -1 limit argument looks hacky but a fix could keep this -1 argument working.

EDIT: this looks to be java's string class behavior: http://stackoverflow.com/questions/2170557/split-method-of-string-class-does-not-include-trailing-empty-strings
Would be nice if limit defaulted to -1 on that type of clojure.string/split call.

Comment by Stuart Halloway [ 19/Jul/15 8:03 AM ]

This is really gross, and the original developer has been punched in the neck. (Ow.)

I hate the Java leakage, but given that this is already out there, and that people are likely already relying on both the default and the negative-arg behavior, I think the least bad bet is to document precisely the semantics we have.

Comment by Alex Miller [ 29/Mar/18 3:45 PM ]

Incorporate changes from CLJ-1857 in clj-1360-2.patch.





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

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

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

0.1.123


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

 Description   

:pred should be resolved in explain problems like:

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

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

should be

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

Other examples:

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

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

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

Patch: clj-2168.patch



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

The same problem happens with s/every.

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

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

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

For example,

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

I'll file that as another ticket later.

Comment by Alexander Kiel [ 18/Jul/18 7:56 AM ]

Issue https://github.com/alexanderkiel/phrase/issues/22 relates to this.





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

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

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

Approval: Vetted

 Description   

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



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

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

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

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

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

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





[CLJ-1620] Constants are leaked in case of a reentrant eval Created: 18/Dec/14  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v2.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v3.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch     Text File clj-1620-v5.patch     Text File eval-bindings.patch    
Patch: Code
Approval: Incomplete

 Description   

Compiling a function that references a non loaded (or uninitialized) class triggers its init static. When the init static loads clojure code, some constants (source code I think) are leaked into the constants pool of the function under compilation.

It prevented CCW from working in some environments (Rational) because the static init of the resulting function was over 64K.

Steps to reproduce:

Load the leak.main ns and run the code in comments: the first function has 15 extra fields despite being identical to the second one.

(ns leak.main)

(defn first-to-load []
  leak.Klass/foo)

(defn second-to-load []
  leak.Klass/foo)

(comment
=> (map (comp count #(.getFields %) class) [first-to-load second-to-load])
(16 1)
)
package leak;
 
import clojure.lang.IFn;
import clojure.lang.RT;
import clojure.lang.Symbol;
 
public class Klass {
  static {
    RT.var("clojure.core", "require").invoke(Symbol.intern("leak.leaky"));
  }
  public static IFn foo = RT.var("leak.leaky", "foo");
}
(ns leak.leaky)

(defn foo
  "Some doc"
  []
  "hello")

(def unrelated 42)

https://gist.github.com/cgrand/5dcb6fe5b269aecc6a5b#file-main-clj-L10

Patch: clj-1620-v5.patch



 Comments   
Comment by Christophe Grand [ 18/Dec/14 3:56 PM ]

Patch from Nicola Mometto

Comment by Nicola Mometto [ 18/Dec/14 4:01 PM ]

Attached the same patch with a more informative better commit message

Comment by Laurent Petit [ 18/Dec/14 4:03 PM ]

I'd like to thank Christophe and Alex for their invaluable help in understanding what was happening, formulating the right hypothesis and then finding a fix.

I would also mention that even if non IBM rational environments where not affected by the bug to the point were CCW would not work, they were still affected. For instance the class for a one-liner function wrapping an interop call weighs 700bytes once the patch is applied, when it weighed 90kbytes with current 1.6 or 1.7.

Comment by Laurent Petit [ 18/Dec/14 5:07 PM ]

In CCW for the initial problematic function, the -v2 patch produces exactly the same bytecode as if the referenced class does not load any namespace in its static initializers.
That is, the patch is valid. I will test it live in the IBM Rational environment ASAP.

Comment by Laurent Petit [ 19/Dec/14 12:10 AM ]

I confirm the patch fixes the issue detected initially in the IBM Rational environment

Comment by Michael Blume [ 06/Jan/15 4:03 PM ]

I have absolutely no idea why, but if I apply this patch, and the patch for CLJ-1544 to master, and then try to build a war from this test project https://github.com/pdenhaan/extend-test I get a scary-looking traceback:

$ lein do clean, war!
Exception in thread "main" java.lang.NoSuchFieldError: __thunk__0__, compiling:(route.clj:1:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3606)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at extend_test.core.handler$loading__5301__auto____66.invoke(handler.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at extend_test.core.servlet$loading__5301__auto____7.invoke(servlet.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$compile$fn__5420.invoke(core.clj:5834)
	at clojure.core$compile.invoke(core.clj:5833)
	at user$eval5.invoke(form-init180441230737245034.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6776)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.eval(Compiler.java:6766)
	at clojure.lang.Compiler.load(Compiler.java:7203)
	at clojure.lang.Compiler.loadFile(Compiler.java:7159)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NoSuchFieldError: __thunk__0__
	at instaparse.core__init.load(Unknown Source)
	at instaparse.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clout.core$loading__5301__auto____273.invoke(core.clj:1)
	at clout.core__init.load(Unknown Source)
	at clout.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:482)
	at compojure.core$loading__5301__auto____68.invoke(core.clj:1)
	at compojure.core__init.load(Unknown Source)
	at compojure.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.route$loading__5301__auto____1508.invoke(route.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	... 75 more
Subprocess failed
Comment by Michael Blume [ 06/Jan/15 4:06 PM ]

https://github.com/MichaelBlume/clojure/tree/no-field
https://github.com/MichaelBlume/extend-test/tree/no-field

mvn clean install in the one, lein ring uberwar in the other.

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

Michael, thanks for the report, I've tried investigating this a bit but the big amount of moving parts involved make it really hard to figure out why the combination of the two patches causes this issue.

A helpful minimal case would require no lein and no external dependencies, I'd appreciate some help in debugging this issue if anybody has time.

Comment by Michael Blume [ 06/Jan/15 10:56 PM ]

Ok, looks like the minimal case is

(ns foo (:require [instaparse.core]))

(ns bar (:require [foo]))

and then attempt to AOT-compile both foo and bar.

I don't yet know what's special about instaparse.core.

Comment by Michael Blume [ 06/Jan/15 11:30 PM ]

Well, not a minimal case, of course, but one without lein, at least.

Comment by Michael Blume [ 06/Jan/15 11:51 PM ]

ok, problem is instaparse's defclone macro, I've extracted it to a test repo

https://github.com/MichaelBlume/thunk-fail

lein do clean, compile will get you a failure, but the repo has no dependencies so I'm sure there's a way to do that without lein.

Comment by Ghadi Shayban [ 06/Jan/15 11:56 PM ]

Sorry for the barrage of questions, but these classloader bugs are subtle (and close to being solved I hope). Your report is immensely valuable, and yet it will help to be even more specific. There are a cluster of these bugs – and keeping them laser-focused is key.

The minimal case to which you refer is the NoSuchFieldError?
How are is this being invoked this without lein?
What are you calling to AOT? (compile 'bar) ?
What is the classpath? When you invoke originally, is ./target/classes empty?
Does the problem go away with CLJ-979-7 applied?

Comment by Michael Blume [ 07/Jan/15 12:16 AM ]

I have tried and failed to replicate without leiningen. When I just run

java -Dclojure.compile.path=target -cp src:../clojure/target/clojure-1.7.0-aot-SNAPSHOT.jar clojure.lang.Compile thunk-fail.first thunk-fail.second

everything works fine.

Comment by Ghadi Shayban [ 07/Jan/15 12:30 AM ]

The NoSuchFieldError is related to the keyword lookup sites.

Replacing defclone's body with
`(do (:foo {})) is enough to trigger it, with the same ns structure.

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

I have updated the patch for CLJ-1544, now the combination of the new patch + the patch from this ticket should not cause any exception.

That said, a bug in this patch still exists since while the patch for CLJ-1544 had a bug, it was causing a perfectly valid (albeit hardly reproducible) compilation scenario so we should keep debugging this patch with the help of the bugged patch for CLJ-1544.

I guess the first thing to do is figure out what lein compile is doing differently than clojure.Compile

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

Also Ghadi is right, infact replacing the whole body of thunk-fail.core with (:foo {}) is enough.

It would seem like the issue is with AOT (re)compiling top-level keyword lookup sites, my guess is that for some reason this patch is preventing correct generation of the __init static initializer.

Comment by Nicola Mometto [ 07/Jan/15 5:35 AM ]

I still have absolutely no idea what lein compile is doing but I figured out the issue.
The updated patch binds (in eval) the appropriate vars only when already bounded.

Comment by Alex Miller [ 07/Jan/15 9:00 AM ]

Would it be worth using transients on the bindings map now?

Comment by Nicola Mometto [ 07/Jan/15 9:11 AM ]

Makes sense, updated the patch to use a transient map

Comment by Michael Blume [ 07/Jan/15 12:25 PM ]

Is there a test we can add that'll fail in the presence of the v2 patch? preferably independent of the CLJ-1544 patch? I can try to write one myself, but I don't have a lot of familiarity with the Clojure compiler internals.

Comment by Nicola Mometto [ 07/Jan/15 12:32 PM ]

I'll have to think about a way to reproduce that bug, it's not a simple scenario to reproduce.
It involves compiling a namespace from an evaluated context.

Comment by Laurent Petit [ 15/Apr/15 11:14 AM ]

Hello, is there any chance left that this issue will make it to 1.7 ?

Comment by Alex Miller [ 15/Apr/15 11:18 AM ]

Wasn't planning on it - what's the impact for you?

Comment by Laurent Petit [ 29/Apr/15 2:14 PM ]

The impact is that I need to use a patched version of Clojure for CCW.
While it's currently not that hard to follow clojure's main branch and regularly rebase on it or reapply the patch, it's still a waste of time.

Comment by Alex Miller [ 29/Apr/15 2:31 PM ]

I will check with Rich whether it can be screened for 1.7 before we get to RC.

Comment by Alex Miller [ 29/Apr/15 3:49 PM ]

same as v4 patch, but just has more diff context

Comment by Laurent Petit [ 01/May/15 7:25 AM ]

the file mentioned in the patch field is not the right one IMHO

Comment by Alex Miller [ 01/May/15 8:42 AM ]

which one is?

Comment by Laurent Petit [ 01/May/15 8:58 AM ]

I think you previous comment relates to clj-1620-v5.patch, but at the end of the description there's the following line:

Patch: 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch

Comment by Alex Miller [ 01/May/15 9:30 AM ]

Those patches are equivalent with respect to the change they introduce; they just differ in how much diff context they have.

Comment by Alex Miller [ 18/May/15 2:25 PM ]

Rich has ok'ed screening this one for 1.7 but I do not feel that I can mark it screened without understanding it much better than I do. The description, code, and cause information here is not sufficient for me to understand what the problem actually is or why the fix is the right one. The fix seems to address the symptom but I worry that it is just a symptom and that a better understanding of the actual cause would lead to a different or better fix.

The evolution of the patches was driven by bugs in CLJ-1544 (a patch which has been pulled out for being suspect for other reasons). Starting fresh, were those modifications necessary and correct?

Why does this set of vars need to push clean impls into the bindings? Why not some of the other vars (like those pushed in load())? The set chosen here seems to match that from the ReifyParser - why? Why should they only be pushed if they are bound (that is, why is "not bound" not the same as "bound but empty")? Are we affecting performance?

Popping all the way out, is the thing being done by CCW even a thing that should be doable? The description says "Compiling a function that references a non loaded (or uninitialized) class triggers its init static" - should this load even happen? Can we get an example that actually demonstrates what CCW was doing originally?

Comment by Laurent Petit [ 19/May/15 7:12 AM ]

Alex, the question of "should what CCW is doing be doable" can be answered if you answer it on the given example, I think.

The question "should the initialization of the class occur when it could just be loaded" is a good one. Several reports have been made on the Clojure list about this problem, and I guess there is at least one CLJ issue about changing some more classForName into classForNameNonLoading here and there in Clojure.
For instance, it prevents referencing java classes which have code in their static initializers as soon as the code does some supposition about the runtime it is initialized in. This is a problem with Eclipse / SWT, this a problem with Cursive as I remember Colin mentioning a similar issue. And will probably is a problem that can appear each time one tries to AOT compile clojure code interoperating with java classes who happen to have, somewhere within static initializers triggered by the compilation (and this is transitive), assumptions that they are initialized in the proper target runtime environment.

What I don't know is if preventing the initialization to occur in the first place would be sufficient to get rid of the class of problems this bug and the proposed patch tried to solve. I do not claim to totally what is happening either (Christophe and Nicolas were of great help to analyze the issue and create the patch), but as I understand it, it's a kind of "Inception-the-movie-like" bug. Compiling a fn which triggers compiling another fn (here through the loading of clojure namespaces via a java initializer).

If preventing the initialization of class static methods when they are referenced (through interop calls - constructor, field, method, static field, static method-) is the last remaining bit that could cause such "compilation during compilation" scenario, then yes, protecting the compilation process like Nicolas tried to do may not be necessary, and just fixing the undesired loading may be enough.





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

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

Type: Feature Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: None
Environment:

All


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

 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

Patch: clj-1771.patch

Prescreened by: Alex Miller



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

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

Comment by Yehonathan Sharvit [ 19/Aug/16 10:19 AM ]

for the sake of symmetry with `assoc` I'd love to see this ticket fixed

Comment by Alex Miller [ 27/Nov/16 10:33 PM ]

Do you need the "if kvs" check?

Should have tests.

Comment by Matthew Gilliard [ 14/Jan/17 11:34 AM ]

Sorry for the delay - I don't get notifications from this JIRA for some reason.

The patch now includes tests.

Both `if` checks are necessary as we have 3 possible outcomes there:
1/ No more kvs (we are finished)
2/ More kvs (we need to recur)
3/ A sequence of keys but no value (throw IAE)





[CLJ-2368] [spec] describe* of spec-impl returns nil :args and :fn Created: 29/Jun/18  Updated: 08/Oct/18

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

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

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

 Description   

The form of an empty fspec returns nil at :args and :fn:

(s/form (s/fspec))
=> (clojure.spec.alpha/fspec :args nil :ret clojure.core/any? :fn nil)

The problem is, that the specs from CLJ-2112 don't validate that form. I suggest to implement describe* of fspec-impl like that of map-spec-impl which omits nil values.

Patch: clj-2368.patch



 Comments   
Comment by Alex Miller [ 29/Jun/18 8:55 AM ]

Sounds right.





[CLJ-2360] [spec] Inconsistent results from s/valid? and s/explain when using s/or without parameters Created: 19/Jun/18  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Björn Ebbinghaus Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec
Environment:

org.clojure/spec.alpha "0.1.143"


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

 Description   

When using spec/or without parameters spec/valid? and spec/explain are resulting in a contradiction.

Minimal example:

(s/valid? (s/or) :something)
=> false
(s/explain (s/or) :something)
Success!
=> nil

Similar to or, s/or should fail to validate anything (so first call is good) but should report an error with explain (so explain result is bad).

Proposed: In the s/or explain, the logic maps over all preds to build a problem list about each value not matching, but the base case of no preds isn't explicitly handled. The change in the code checks for that case and builds a specific explain problem error for the overall s/or not matching. Patch includes fix and tests for (s/or) and (s/and) (which is ok).

Patch: clj-2360.patch






[CLJ-1180] defprotocol doesn't resolve tag classnames Created: 10/Mar/13  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: protocols

Attachments: Text File 001-CLJ-1180.patch     Text File clj-1180-2.patch    
Patch: Code and Test
Approval: Vetted

 Description   

defprotocol doesn't resolve tag classnames, this results in exceptions being thrown when the declared protocol uses as a tag an imported class that is not imported in the namespace that uses it.

user=> (import 'clojure.lang.ISeq)
clojure.lang.ISeq
user=> (defprotocol p (^ISeq f [_]))
p
user=> (ns x)
nil
x=> (defn x [y] (let [z (user/f y)] (inc z)))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: ISeq, compiling:(NO_SOURCE_PATH:4:33)

Patch: clj-1180-2.patch

Screened by: Alex Miller

  • My only hesitancy with this patch is making the tagToClass public and reaching into it from core_deftype - this means that call into the Compiler needs to remain open forever as an api.


 Comments   
Comment by Alex Miller [ 03/Sep/13 9:38 AM ]

Similer to CLJ-1232.

Comment by Alex Miller [ 18/Jan/16 4:50 PM ]

Patch updates for current master, retains attribution.





[CLJ-1364] Primitive VecSeq does not implement equals or hashing methods Created: 19/Feb/14  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, ft

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

 Description   

VecSeq (as produced by (seq (vector-of :int 1 2 3))) does not implements equals, hashCode, or hasheq and does not play with any other Clojure collections or sequences appropriately in this regard.

user=> (def rs (range 3))
user=> (def vs (seq (vector-of :int 0 1 2)))
user=> rs
(0 1 2)
user=> vs
(0 1 2)
user=> (.equals rs vs)
true
user=> (.equals vs rs)    ;; expect: true
false
user=> (.equiv rs vs)
true
user=> (.equiv vs rs)
true
user=> (.hashCode rs)
29824
user=> (.hashCode vs)     ;; expect to match (.hashCode rs)
2081327893
user=> (System/identityHashCode vs)  ;; show that we're just getting Object hashCode
2081327893
user=> (.hasheq rs)
29824
user=> (.hasheq vs)       ;; expect same as (.hasheq rs) but not implemented at all
IllegalArgumentException No matching field found: hasheq for class clojure.core.VecSeq  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Approach: Implement Object.hashCode(), Object.equals(), and IHashEq.hasheq() in the primitive vector seq implementation. All of these leverage the prim vec seq itself rather than the underlying prim vec as it was quite a big simpler. The hasheq() impl calls Murmur3/hashOrdered, which takes an Iterable, so Iterable was also implemented using an iterator over the seq.

Some existing tests were expanded to include coverage of the primitive vec seq.

Patch: clj-1364.patch



 Comments   
Comment by Andy Fingerhut [ 09/May/18 1:53 PM ]

Maybe this is a separate issue, but `(.equals (vector-of :long 0 1 2) (range 3))` is also false (i.e. without calling seq on the return value of vector-of).

Comment by Alex Miller [ 09/May/18 1:55 PM ]

Same issue, and is fixed by the patch.

Comment by Andy Fingerhut [ 09/May/18 2:23 PM ]

Excellent. Sorry, I had not read the new tests in the patch carefully enough to see that this is covered there. Good to know.





[CLJ-2199] [spec] Error attempting to unform unconformed keys (no :conform-keys opt) Created: 05/Jul/17  Updated: 08/Oct/18

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

Type: Defect Priority: Minor
Reporter: Daniel Stockton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec

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

 Description   

Minimal failing case:

(s/def ::key-spec (s/or :kw keyword? :str string?))
(s/def ::map-spec (s/map-of ::key-spec identity))
(println (s/unform ::map-spec (s/conform ::map-spec {:a :b})))
java.lang.UnsupportedOperationException: nth not supported on this type: Keyword

If keys are not conformed, we should also not attempt to unform them.



 Comments   
Comment by Daniel Stockton [ 08/Aug/17 8:29 AM ]

Add test and fix behaviour

Comment by Daniel Stockton [ 08/Aug/17 8:39 AM ]

Actually, although this passes all tests, it's not alright because it bypasses validation.

Comment by Daniel Stockton [ 12/Aug/17 4:23 AM ]

I think it passes for correctness this time but open to advice if this is not a good approach.





[CLJ-1951] bigint? predicate and generator Created: 08/Jun/16  Updated: 08/Oct/18

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

Type: Feature Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: generator

Approval: Vetted

 Description   

Add bigint? and spec.gen support.

This part is easy:

(defn bigint?
  "Returns true if n is a BigInt"
  {:added "1.9"}
  [n] (instance? clojure.lang.BigInt n))

The generator is the tricky bit. test.check doesn't have a generator for bigints, just large-integer for things in long range. I think we'd want numbers beyond long range in a bigint generator (as that's a likely place where bugs might lie). Making a really high-quality bigint generator (with good growth and shrinking characteristics) is something that needs more thought.

http://clojure.github.io/test.check/clojure.test.check.generators.html#var-large-integer



 Comments   
Comment by Gary Fredericks [ 17/Dec/16 6:17 PM ]

In case I don't get around to making a patch, I think a generator along these lines would be a decent start:

(def gen-bigint 
  (gen/sized 
   (fn [size] 
     (let [large-integer (gen/resize size gen/large-integer)] 
       ;; scaling gives us relatively small vectors, but using  
       ;; the resized large-integer above means the numbers in 
       ;; the small vectors will still be big 
       (gen/scale #(+ 2 (/ % 20))
                  (gen/fmap (fn [xs] (+ (bigint (first xs)) (reduce * (map bigint (rest xs))))) 
                            (gen/not-empty (gen/vector large-integer))))))))
Comment by Gary Fredericks [ 17/Dec/16 6:21 PM ]

If anything seems inadequate about the sizing in the above generator, I should point out that sizing in test.check is rather subtle but the requirements are also not well-defined. I'd be happy to discuss in detail.

Comment by Gary Fredericks [ 15/May/17 4:39 PM ]

As an update, I've put together a marvelous bigint generator that this margin is too small to contain. It might need tweaking but is probably fine for these purposes. It's not on test.check master yet but I expect it will be soon.





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

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

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

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

 Description   

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

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

Patch: clj-1891.patch



 Comments   
Comment by Alexander Yakushev [ 02/Jan/18 9:17 AM ]

Bump. With the introduction of Spec, and considering the fact that clojure.core.server triggers the initialization of Spec, the benefit of solving this issue should now be bigger than 0.05 sec (more like 0.2 sec). See http://clojure-goes-fast.com/blog/clojures-slow-start .





[CLJ-2190] [spec] clojure.spec.alpha/exercise-fn should emit a bettor error message when no implementation is found for a symbol Created: 27/Jun/17  Updated: 08/Oct/18

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

Type: Enhancement Priority: Minor
Reporter: Abhirag Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, spec
Environment:

Clojure 1.9.0-alpha17
test.check 0.10.0-alpha2


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

 Description   

Here we get a NullPointerException because although we do have a spec for foo, we don't have an implementation for it, a more descriptive error message would help.

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

(s/fdef foo :args (s/cat :x int?) :ret int?)
=> user/foo

(s/exercise-fn `foo)
NullPointerException   clojure.core/apply (core.clj:657)

Proposed: Check for a nil function and throw.

Patch: clj-2190.patch



 Comments   
Comment by Stuart Halloway [ 22/Jun/18 3:20 PM ]

If we are going to handle this case, can we also include the name of the offending symbol in the error message?

Comment by Alex Miller [ 22/Jun/18 3:49 PM ]

The sym-or-f arg is resolved to nil before exercise-fn is invoked, so we have no symbol to report by that point.





[CLJ-1741] deftype class literals and instances loaded from different classloaders when recompiling namespace Created: 30/May/15  Updated: 08/Oct/18

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

Type: Defect Priority: Major
Reporter: Stephen Nelson Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: aot, classloader, compiler

Attachments: Text File 0001-CLJ-1714-Don-t-load-AOT-class-when-compiling-already.patch    
Patch: Code
Approval: Incomplete

 Description   

Scenario: Given two files:

src/dispatch/core.clj:

(ns dispatch.core (:require [dispatch.dispatch]))

src/dispatch/dispatch.clj:

(ns dispatch.dispatch)
(deftype T [])
(def t (->T))
(println "T = (class t):" (= T (class t)))

Compile first core, then dispatch:

java -cp src:target/classes:clojure.jar -Dclojure.compile.path=target/classes clojure.main
user=> (compile 'dispatch.core)
T = (class t): true
dispatch.core
user=> (compile 'dispatch.dispatch)
T = (class t): false     ;; expected true
dispatch.dispatch

This scenario more commonly occurs in a leiningen project with :aot :all. Files are compiled in alphabetical order with :all. In this case, dispatch.core will be compiled first, then dispatch.dispatch.

Cause:

(compile 'dispatch.core)

  • transitively compiles dispatch.dispatch
  • writes .class files to compile-path (which is on the classpath)
  • assertion passes

(compile 'dispatch.dispatch)

  • due to prior compile, load dispatch.dispatch__init is loaded via the appclassloader
  • ->T constructor will use new bytecode to instantiate a T instance - this uses appclassloader, loaded from compiled T on disk
  • however, T class literals are resolved with RT.classForName, which checks the dynamic classloader cache, so uses old runtime version of T, instead of on-disk version

In 1.6, RT.classForName() did not check dynamic classloader cache, so loaded T from disk as with instances. This was changed in CLJ-979 to support other redefinition and AOT mixing usages.

Approaches:

1) Compile in reverse dependency order to avoid compiling twice.

Either swap the order of compilation in the first example or specify the order in project.clj:

:aot [dispatch.dispatch dispatch.core]

This is a short-term workaround.

2) Move the deftype into a separate namespace from where it is used so it is not redefined on the second compile. This is another short-term workaround.

3) Do not put compile-path on the classpath (this violates current expectations, but avoids loading dispatch__init)

(set! *compile-path* "foo")
(compile 'dispatch.core)
(compile 'dispatch.dispatch)

This is not easy to set up via Leiningen currently.

4) Compile each file with an independent Clojure runtime - avoids using cached classes in DCL for class literals.

Probably too annoying to actually do right now in Leiningen or otherwise.

5) Make compilation non-transitive. This is in the ballpark of CLJ-322, which is another can of worms. Also possibly where we should be headed though.

Screening: I do not believe the proposed patch is a good idea - it papers over the symptom without addressing the root cause. I think we need to re-evaluate how compilation works with regard to compile-path (#3) and transitivity (CLJ-322) (#5), but I think we should do this after 1.7. - Alex

See also: CLJ-1650



 Comments   
Comment by Alex Miller [ 30/May/15 8:50 PM ]

Pulling into 1.7 for consideration.

Comment by Stephen Nelson [ 30/May/15 8:55 PM ]

I've added a debug flag to my example that causes type instance hashcodes and their class-loaders to be printed.

Compiling dispatch.core
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
dispatch:  :pass
Compiling dispatch.dispatch
deftype => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
defmethod => 652433136 (clojure.lang.DynamicClassLoader@23c30a20)
instance => 760357227 (sun.misc.Launcher$AppClassLoader@42a57993)
dispatch:  :fail
Comment by Nicola Mometto [ 01/Jun/15 7:23 AM ]

The compiler has weird loading rules when using `compile` and both a clj file and a class file are present in the classpath.

This bug happens because RT.load will load the AOT class file rebinding the ->Ctor to use the AOT deftype instance.

A fix for this would be making load "loaded libs" aware to avoid unnecessary/harmful reloadings.

Comment by Nicola Mometto [ 01/Jun/15 10:55 AM ]

The attached patch fixes this bug by keeping track of what has already been loaded and loading the AOT class only if necessary

Comment by Alex Miller [ 16/Jun/15 2:24 PM ]

Original description (since replaced):

Type-dispatching multimethods are defined using the wrong type instance

When using a multimethod that dispatches on types, such as print-dup/print-method, the type reference passed to addMethod in the presence of aot is incorrect on the second load of the namespace. This means that if the namespace has already been loaded as a dependency of another file, the second load when the namespace is loaded for aot compilation will produce a multimethod that fails to dispatch correctly.

I've created an example repository:
https://github.com/sfnelson/clj-mm-dispatch

To reproduce independently, create a namespace that contains a deftype and a multimethod dispatching on the type, and a second namespace that requires the first and sorts alphabetically before the first. Aot-compile both namespaces. When the type-defining namespace is loaded via require it produces a class file for the deftype. When it is loaded the second time for aot-compilation, the type corresponding to the existing class file is given to the defmethod, instead of the new class constructed by loading the namespace. This causes the multimethod it fail to dispatch correctly.

To me this issue seems similar to CLJ-979: the type passed to the multimethod is retrieved using the wrong classloader. This suggests that it might have wider implications than AOT and multimethod dispatch.

Comment by Nicola Mometto [ 18/Jun/15 11:09 AM ]

I just realized this ticket is a duplicate of CLJ-1650





[CLJ-2102] [spec] Reduce collection generator default size from 20 Created: 25/Jan/17  Updated: 08/Oct/18

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

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

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

 Description   

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

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

Patch: clj-2102-3.patch



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

Updated patch to apply to master

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

Updated patch to apply to spec.alpha

Comment by Stuart Halloway [ 12/May/17 10:32 AM ]

I have definitely seen the pain here – nested collections can get big fast. OTOH for non-nested collections the larger generator is nice. Not sure moving the default is a help.

Comment by Alex Miller [ 12/May/17 3:23 PM ]

Use dynamic var and reduce default. Also consider ways to avoid this kind of problem in test.check itself (how does quickcheck deal with this?).