[CLJ-2511] Macroexpanded case statement throws `java.lang.NegativeArraySizeException` Created: 14/May/19  Updated: 14/May/19

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

Type: Defect Priority: Major
Reporter: Vitalie Spinu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Java 11.0.2, ubuntu linux



 Description   

The result of the following macroexpansion cannot be evaluated throwing `java.lang.NegativeArraySizeException`

Unable to find source-code formatter for language: clj. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(macroexpand-1 '(case :a
                  (:a-36 :b-36) 36
                  (:a-37 :b-37) 37
                  (:a-38 :b-38) 38
                  (:a-39 :b-39) 39
                  (:a-40 :b-40) 40
                  (:a-42 :b-42) 42))

;; => 

(clojure.core/let
    [G__8112 :a]
  (case*
    G__8112
    0
    31
    (throw
      (java.lang.IllegalArgumentException.
        (clojure.core/str "No matching clause: " G__8112)))
    {3 [:b-36 36],
     4 [:b-37 37],
     6 [:b-40 40],
     13 [:b-39 39],
     14 [:b-38 38],
     18 [:a-42 42],
     20 [:a-40 40],
     21 [:a-38 38],
     22 [:a-37 37],
     24 [:a-39 39],
     27 [:b-42 42],
     28 [:a-36 36]}
    :compact
    :hash-identity
    nil))

If you remove any of the options in the case above the macroexpansion can be evaluated correctly.

The reason behind this is that `case*` chokes on `clojure.lang.PersistentArrayMap` which `{...}` produces at the REPL. The original type is `clojure.lang.PersistentTreeMap`

The consequence of this is that the macro-expanded lists cannot be freely manipulated in code without checking explicitly for the concrete types of the maps. Particularly, `clojure.walk/walk` will produce un-evaluable results.

Original issue https://github.com/clojure-emacs/cider/issues/2335






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

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

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

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

 Description   

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

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

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

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

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

Cause: empty? is based on seqability, which transients don't implement.

Proposed Add a branch to empty? for counted? colls. Transients implement Counted so gain support via this branch. Other colls that are counted are faster. Seq branch continues to work for seqs.

Perf test:

(def p [])
(def p1 [1])
(def t (transient []))
(def t1 (transient [1]))

;; take last time of all these
(dotimes [i 20] (time (dotimes [_ 10000] (empty? p))))
(dotimes [i 20] (time (dotimes [_ 10000] (empty? p1))))
(dotimes [i 20] (time (dotimes [_ 10000] (empty? t))))
(dotimes [i 20] (time (dotimes [_ 10000] (empty? t1))))

Results:

coll before after result
p 0.72 ms 0.08 ms much faster when empty
p1 0.15 ms 0.13 ms slightly faster when not empty
t error 0.19 ms no longer errors
t1 error 0.20 ms no longer errors

Not sure if doc string should be tweaked to be more generic, particularly the "same as (not (seq coll))" which is now only true for Seqable but not Counted. I think the advise to use (seq coll) for seq checks is still good there.

I did a skim for other types that are Counted but not seqs/Seqable and didn't find much other than internal things like ChunkBuffer. Many are both and would thus use the counted path instead (all the persistent colls for example and any kind of IndexedSeq).

I guess another option would be just to fully switch empty? to be about (zero? (bounded-count 1 coll)) and lean on count's polymorphism completely.

Patch: clj-1872.patch



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

Probably similar to CLJ-700.

Comment by Devin Walters [ 09/May/19 2:52 PM ]

As mentioned in CLJ-700, this is a different issue.

Comment by Devin Walters [ 09/May/19 5:47 PM ]

First things first, the original description brings up `(empty? (transient ()))`. Per the documentation at https://clojure.org/reference/transients, there is no benefit to be had for supporting transients on lists.

Current behavior for java collections:

(empty? (java.util.HashMap. {}))
=> true

(empty? (java.util.HashMap. {1 2}))
=> false

(seq (java.util.HashMap. {1 2}))
=> (#object[java.util.HashMap$Node 0x4335c9c3 "1=2"])

(seq (java.util.HashMap. {}))
=> nil

The same behavior is true of java arrays.

Over in CLJS-2802, the current patch's approach is to `cond` around the problem in `empty?` by explicitly checking whether it's a TransientCollection, and if so, using `(zero? (count coll))` as the original description mentions as a workaround.

Currently, transient collections do not implement Iterable as the persistent ones do. If Iterable were implemented, I believe RT.seqFrom would work, and by extension, `empty?`.

Comment by Alex Miller [ 13/May/19 1:27 PM ]

I think there are good reasons for transient collections not to be Seqable - seqs imply caching, caching hurts perf, and the whole reason to be using transients is for batch load perf. So that seems counter-productive. Iterators are stateful and again, I suspect that is probably a bad thing to add just for the sake of checking empty?.

An explicit check for emptiness of counted? colls would cover all the transient colls and anything else counted without making a seq. That might be faster for all those cases, and doesn't require anything new of anybody in the impl.

Another option would be to have an IEmptyable interface and/or protocol to indicate explicit empty? check support. Probably overkill.





[CLJ-2509] remote-prepl blocks main-thread upon in-reader's eos Created: 10/May/19  Updated: 13/May/19

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

Type: Defect Priority: Major
Reporter: Gert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: prepl

Approval: Triaged

 Description   

When the in-reader passed to clojure.core.server/remote-prepl reaches end-of-stream, an attempt is made to close the socket's reader. This however blocks the main-thread as (mostly) the socket is still being read from.

$ clj -J-Dclojure.server.jvm="{:port 5555 :accept clojure.core.server/io-prepl}" -r
Clojure 1.10.0
user=> (clojure.core.server/remote-prepl "127.0.0.1" 5555 *in* prn)
;; as expected:
1
{:tag :ret, :val 1, :ns "user", :ms 0, :form "1"}
^D ;; hangs - ^C required to exit process

What I would expect to happen in this case is that upon ^D the initial repl re-appears (similar to how (clojure.core.server/io-prepl) behaves).

What does work:

$ clj -J-Dclojure.server.jvm="{:port 5555 :accept clojure.core.server/io-prepl}" -r
Clojure 1.10.0
user=> (clojure.core.server/remote-prepl "127.0.0.1" 5555 *in* prn)
:repl/quit
^D
nil
user=> _

Looking at the code though it seems that this is not the intented way to terminate the prepl.






[CLJ-2308] Reduce based some Created: 04/Jan/18  Updated: 09/May/19

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

Type: Enhancement Priority: Major
Reporter: Petter Eriksson Assignee: Petter Eriksson
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File CLJ-2308-2-reduce2.patch     Text File CLJ-2308-3-IReduceInit.patch     Text File CLJ-2308-4-alter-var-root.patch     Text File CLJ-2308.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.core/some is implemented using first and recur next. It could be implemented using reduce and reduced to gain a significant speedup on large collections.
I benchmarked range, iterate, vector, sorted-set and a lazy-seq using criterium and got the following results:

  clojure.core/some reduce some :speed-up
:iterate 14.502145 ms 3.994055 ms 3.63x
:range 16.949429 ms 14.903065 ms 1.14x <-- This result is for clojure.lang.Range
:vector 23.706839 ms 5.765865 ms 4.11x
:lazy-seq 28.723150 ms 5.616475 ms 5.11x
:set 53.063608 ms 17.419191 ms 3.05x

Each collection was of size 1e7, and some was called with (some #(< 1e6 %) coll), taking up 1m elements from the collection.

I ran these test on an uberjar with the following java opts:
java -Xmx3g "-Dclojure.compiler.direct-linking=true" -server

The reduce version of some I used and the benchmarking code can be found in this gist:
https://gist.github.com/petterik/63fad1126842ba4550dcb338328e2975



 Comments   
Comment by Petter Eriksson [ 04/Jan/18 5:35 PM ]

I messed up the formatting in the description and I can't find how to edit it (possibly because I don't have permission?). Removing the dotted lines should be somewhat good enough.

Comment by Andy Fingerhut [ 04/Jan/18 5:46 PM ]

I have bumped up the permissions on your JIRA account, since you are on the list of people who have signed a Clojure Contributor Agreement. You should be able to edit the ticket now.

If you have a proposed change to make to Clojure, I'd recommend creating a patch using the instructions found at the "Developing Patches" link on this page: https://dev.clojure.org/display/community/Contributing

Comment by Petter Eriksson [ 04/Jan/18 5:58 PM ]

Sure, Andy. I'm initiating a move tomorrow, so I'll get to it next week.

Comment by Petter Eriksson [ 20/Jan/18 5:51 PM ]

Attached CLJ-2308.patch - 20/Jan/18 5:51 PM:

Implemented clojure.core/some and clojure.core/every? with reduce and early termination with reduced.

To get this to work I had to:

  • Use reduce1 instead of reduce
  • Move deref and reduced above reduce1
  • Take care of reduced values in reduce1

I've benchmarked reduced based clojure.core/every? with collection sizes of 1e4 and pred #(< % 1e3):

  clojure.core/every? reduce every? :speed-up
:iterate 16.897504 µs 8.273847 µs 2.04x
:long-range 15.197158 µs 8.658177 µs 1.76x
:vector 27.534283 µs 2.519784 µs 10.93x
:lazy-seq 25.457922 µs 6.393590 µs 3.98x
:set 56.421657 µs 17.143458 µs 3.29x

Since the results were good and some is so similar to every?, I went ahead and did the change for every?.

Comment by Alex Miller [ 28/Apr/19 4:04 PM ]

Why are deref and deref-future etc moved in the patch? Or, bigger question, is there a smaller patch with same effect?

Comment by Petter Eriksson [ 29/Apr/19 12:59 PM ]

Summary
I think it's worth investigating whether there is a smaller patch with the same effect. I'll see what I can do!

Goal
As the issue has been triaged, try to minimize the patch size.

Problem
`some` requires `reduced` to be implemented in `reduce` to be able to halt the reducing process. There are two implementations of reduce in clojure.core, `reduce` and `reduce1`, where `reduce` implements `reduced` and `reduce1` doesn't. Position a combination of `some`, `reduce1` and/or `reduce` such that `some` gets access to a reduce function which implements `reduced`.

Declaring `reduce` via `(declare reduce)` was attempted and it didn't work IIRC (will validate this approach again).

Old approach
As `some` was above `reduce` position wise and thus not accessible, focus on getting `reduce1` to implement `reduced`. This required moving around deref, deref-future and a few others, which lead to a pretty big patch.

New approaches
Minimize the patch size by either:
1a. Move `some` (and `every?`) below `reduce`
1b. Move `some` (and `every?`) down and `reduce` up.
1c. Move `reduce` above `some` (and `every?`).
2. Creating `some` and `some1` just like for reduce, where the reduce based `some` would be below `reduce`.
3. ?

I'm not a fan of #2, creating more duplications of functions like for `reduce1`. I'm hoping we can get a small patch with #1x.

Comment by Petter Eriksson [ 09/May/19 7:36 PM ]

Here's my progress report. I've outlined my thought process.

Please let me know if you're more interested in just metrics and I can make it less chatty.

TL;DR

Three patches:
1. Adds new `reduce2` which is redefined as `reduce` when possible. (CLJ-2308-2-reduce2.patch)
2. Implements reduced? and IReduceInit check in reduce1. (CLJ-2308-3-IReduceInit.patch)
3. Redefine `some` with `alter-var-root` once `reduce` has been defined. (CLJ-2308-4-alter-var-root.patch)

All pathes are much smaller than the original patch.

Context

As I started to work on a new patch I started remembering why I had the original approach.

1. The distance in clojure/core.clj between `some` and `reduce` is 4,114 LOC and `some` is used 5 times within those LOC.
2. Moving `reduce` above or closer to `some` requires moving some of the `load` expressions, including (load "core/protocols").

Given these two hurdles, I revisited the original patch and the question of why `deref` and `deref-future` had to be moved. I was able to create quite a small patch by:
1. Calling the .deref method on the Reduced object instead of the `deref` function.
2. Calling RT/isReduced instead of the `reduced?` function.
3. Moving just the `reduced` function above `some`.

When getting ready for a new patch, I first ran two benchmarks and found that `some` based on `reduce1` is slower in one case, and confirmed that `some` based on `reduce` is much faster. This finding is not very surprising as `reduce1` uses seq api for iterating through collections w/ chunked-seq optimization. What really makes `reduce` faster than `first+next` is the coll-reduce protocol and IReduceInit interface.

Approaches

1. New `reduce2`:
Given this context, I first played with redefining `reduce1` as `reduce`, but this didn't work as the protocol extension cache code invoked by `reduce` calls `reduce1`. Redefining `reduce1` as `reduce` can make a call to `reduce` loop forever. As this problem seems quite hairy, I went for creating a new reducing function `reduce2` which is redefined as `reduce` using `alter-var-root`.

2. Implementing IReduceInit optimization in `reduce1`:
This approach was inspired by clojure.core/set, where we special case implementations of IReduceInit. By optimizing for IReduceInit and chunked-seq in reduce1, we're getting some, but not all of the reduce path optimizations.

3. Redefning `some` when `reduce` is available:
Redefining `some` with `alter-var-root` once `reduce` has been defined. This approach is the most isolated and performs well across the board.

Discussion

Just adding `reduced` semantics to `reduce1` is not sufficient as most of the performance benefits comes from IReduceInit and CollReduce.

Creating the new `reduce2` allows for opt-in reduce performance enhancements for all functions currently using `reduce1`. Every future change to functions from `reduce1` to `reduce2` would be isolated to only that function. The downside is that there would then exist another reducing function in clojure.core and it's not easy to see why it exists (docs/comment could help). In addition, the new reducing function adds one level of indirection as the function calling `reduce2` has to get the var root. If this is the way forward, adding `reduced` checks in `reduce1` would be nice to have, such that the behavior of `reduce2` is always consistent.

Implementing IReduceInit in `reduce1` has a large contact area, as it does not only affect `some` but a lot of Clojure core. It would for some data structures be a performance win, but for some data structures this change would be a performance loss (e.g. java.lang.String). If this is an interesting way forward, one could either enumerate the slower paths and special case them into `reduce1`.

Redefining `some` after `reduce` has been defined. Smallest foot print of any of the approaches. It will only affect user code as well as usages of `some` after `reduce`, (which currently is `some-fn`). One can consider adding ^:redef to the `some` function, which (if I'm not mistaken) should allow all usages of `some` to benefit from the performance enhancements, even under direct linking.

Benchmark data

All approaches was tested using criterium.core/bench.

Versions explained:

Patch :version description
N/A 1.10.0 Baseline. Clojure 1.10.0 with no changes.
N/A reduce1-reduced reduce1 implementing reduced check
CLJ-2308-3-IReduceInit.patch reduce1-reduced+IReduceInit reduce1 implementing reduced and IReduceInit check
CLJ-2308-2-reduce2.patch reduce2 reduce2, which is redefined as `reduce`
CLJ-2308-4-alter-var-root.patch some-redefined Redefines `some` to use `reduce` with `alter-var-root`
N/A some-redefined+meta Redefines `some` to use `reduce` with `alter-var-root` and adds ^:redef meta to `some`
N/A REPL Defines reduce based some in REPL running Clojure 1.10.0

Tests:

Test id Form
:range (some #(< 1e3 %) (range (long 1e4)))
:iterate (some #(< 1e3 %) (iterate inc 0))
:string (some #(= % \x) "1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111x")

Mean times:

Test id 1.10.0 reduce1-reduced reduce1-reduced+IReduceInit reduce2 some-redefined some-redefined+meta REPL
:range 29.439476 µs 7.281439 µs 4.958930 µs 4.935221 µs 5.030624 µs 5.052136 µs 5.636544 µs
:iterate 36.640107 µs 56.214323 µs 6.622923 µs 6.835455 µs 7.014428 µs 7.242155 µs 6.660484 µs
:string 6.236579 µs 7.853746 µs 7.645742 µs 4.186791 µs 4.170554 µs 4.180583 µs 1.898992 µs
Sum 72.316162 71.349508 19.227595 15.957467 16.215606 16.474874 14.19602

All results were reproducible and consistent, except the REPL results where I could get as low as 3 µs for :range and :iterate, which signals to me that there's some JIT optimization going on sometimes. I suspect something similar is going on for the :string test in my REPL.





[CLJ-2508] Add switch for `clojure --version` Created: 08/May/19  Updated: 08/May/19

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

Type: Feature Priority: Trivial
Reporter: Alan Thompson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: main


 Description   

Clojure should support the `--version` and `-v` switches that are nearly universal among software tools.

Suggested output is EDN format:

{:version {:clojure "1.10.1-beta1" :java "Java 12"}}



 Comments   
Comment by Alex Miller [ 08/May/19 12:21 PM ]

If going the data route, *clojure-version* already has a data format:

{:major 1, :minor 10, :incremental 0, :qualifier nil}

and Java has it's own equivalent buried in the Java system properties.

However, seems like first step is to consider whether the primary consumer of this is people or programs. It would be very useful for error reporting, but people would be fine for that. Programs can already emit data by using

clj -e "*clojure-version*"

etc.

Comment by Andy Fingerhut [ 08/May/19 12:27 PM ]

It seems possibly useful, besides the Clojure and Java versions, to also report a version number for the clojure/clj program itself, which is independent of the other version numbers?

Comment by Alex Miller [ 08/May/19 12:32 PM ]

We already have support for stuff like that in clj -Sdescribe and clj -Sverbose, so I'm going to say clj tool itself is out of scope here.





[CLJ-2470] Unify behavior of shuffle in CLJ and CLJS Created: 23/Jan/19  Updated: 08/May/19

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

Type: Enhancement Priority: Minor
Reporter: Michiel Borkent Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-2470-2.patch     Text File CLJ-2470.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The CLJS version of shuffle accepts arrays, but the CLJ version does not.
The CLJS version of shuffle accepts nil, but the CLJ version does not.

This behavior could be unified.

Proposal:

  • Add support for arrays in CLJ
  • Add support for nil in CLJ and return nil or ().
  • The behavior of the CLJS version on nil should be made the same as CLJ: do not return an empty vector, since nil does not pun as an empty vector.
  • The CLJS version supports a shuffle of string, but this can be considered undefined behavior.

The attached patch CLJ-2470 adds support for nil (also returns nil) and Java arrays.
Patch CLJ-2470-2 adds a better error message.

Patch: CLJ-2470-2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Michiel Borkent [ 25/Jan/19 11:35 AM ]

Patch attached.

Comment by Michiel Borkent [ 28/Jan/19 11:05 PM ]

Alex:

CLJS also supports:

(shuffle (eduction (map inc) [1 2 3]))

Should it and if so, should we support it in CLJ? Probably not, since it's not a collection?

Comment by Alex Miller [ 29/Jan/19 12:09 PM ]

That seems weird to me, so I'd say no.

Comment by Alexander Yakushev [ 08/May/19 11:10 AM ]

Drive-by comment, I think that (shuffle nil) should return an empty collection, same as sort, map, filter, partition... etc.





[CLJ-1532] pr-str captures stdout from printing side-effects of lazily evaluated expressions. Created: 23/Sep/14  Updated: 22/Apr/19

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

Type: Defect Priority: Minor
Reporter: Silas Davis Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: print
Environment:

Linux



 Description   

Because clojure.core/pr-str uses with-out-str to capture the output of pr (and pr cannot be parsed a writable thing - just uses out).

If you pr-str the result of something lazy you can get side-effects written to stdout with println interspersed with the output. For example in my case I was extracting benchmarks from the library criterium and trying to print the data structure to the file. The solution would be to provide an overload of pr/pr-str that takes a writer. I note that pr-on provides some of the functionality but it is private.

This is an ugly bug when you're trying to persist program output in EDN, because the randomly interspersed stdout messages make it invalid for read-string. We shouldn't need our functions to be pure for pr-str to work as expected.

I've omitted a patch because although I think a fix is straight-forward I'm not sure quite where it should go (e.g. make pr-on public, change pr, change pr-str)



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

as a workound for this, use print-dup or print-method

Comment by Oliver Caldwell [ 21/Apr/19 9:18 AM ]

So I think a work around for this would be to change pr-str to use pr-on with it's own string writer. This way things intended for out will still get there, the user can then wrap the call in with-out-str if they really want to.

I can't think of why anyone would've relied on this functionality in the past, but there is the risk that this would break someone's workflow. I propose (and will submit a patch for if there's no good reason not to) that we change pr-str to use pr-on directly with a string writer instead of pr which defaults to out. This does mean that pr-str will have to reimplement the var args processing but that's okay I think.

The main motivator for this is that prepl uses pr-str to encode data to send down the socket. This means my prepl tooling will capture out from all lazy-seq results since pr-str is used upstream. The other fix for this would be to configure my prepl to not use pr-str but my own function, but I think that's side stepping the issue.

I think the right thing to do here is to fix pr-str, I don't think the behaviour is correct or expected and will only catch more people out in the future. Again, I'd love to have a go at fixing this but I wanted to run my thoughts past you first.

Comment by Oliver Caldwell [ 22/Apr/19 5:11 AM ]

I've now realised all p...-str functions have this same issue, currently having a think about how I could solve it for all of them. So any printing lazy-seq wouldn't have it's output sent to out. Maybe I could split this so that it doesn't get sent to out but something else that can be rebound separately without using with-out-str.

Comment by Oliver Caldwell [ 22/Apr/19 8:07 AM ]

How about doall on any lazy-seq passed to a p...-str function? They need to be fully resolved regardless, this way we can resolve them before they're wrapped in with-out-str. I think that means the inner printing functions can remain as they are and the -str variants can simply ensure the sequences are resolved before they get printed. I don't think this'd change the characteristics of the functions too much but would fix all of these issues.

At the very least, consumers can use (pr-str (doall my-seq)) to ensure that all laziness is resolved outside of the with-out-str call within pr-str. This could be used as a temporary workaround.

Thoughts?





[CLJ-2383] Add classes that were introduced to java.lang in Java 7/8 into DEFAULT_IMPORTS Created: 08/Aug/18  Updated: 21/Apr/19

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

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

 Description   

Now that Clojure 1.10 alpha-6 dropped support for Java <=7, it seems reasonable to add the classes that were newly introduced to the java.lang package in Java <=8, into Clojure's DEFAULT_IMPORTS.

Here is the list of those classes I'm aware of:

Introduced in Java 7:

  • AutoCloseable
  • ClassValue
  • ReflectiveOperationException
  • BootstrapMethodError
  • SafeVarargs

Introduced in Java 8:

  • FunctionalInterface

Prescreened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 08/Aug/18 10:18 AM ]

Besides AutoCloseable, the rest of those classes are rarely used. I'd rather import them explicitly over polluting the ns imports tbh. (Interfaces annotated with FunctionalInterface, an annotation type, are common; using it explicitly isn't)

There is a minor startup time cost to additional classloading, for classes not already loaded by the java.base module. I've experimented with dropping a bunch of rarely used default imports (a breaking change, no doubt).

Comment by Alex Miller [ 08/Aug/18 11:23 AM ]

I think we should uphold what has been true and stated in the past - stuff in java.lang is auto-imported. I do not think autoimporting 6 classes will affect startup time.

Comment by Shogo Ohta [ 18/Dec/18 4:46 AM ]

Added the patch, just in case

Comment by David Bürgin [ 21/Apr/19 4:15 AM ]

Several nested classes in java.lang are also not yet auto-imported. Some of them are though:

Thread$UncaughtExceptionHandler
; => java.lang.Thread$UncaughtExceptionHandler

Character$Subset
; Syntax error compiling at (REPL:0:0).
; Unable to resolve symbol: Character$Subset in this context

The missing imports are the following:

  • Character$Subset
  • Character$UnicodeBlock
  • ProcessBuilder$Redirect
  • Character$UnicodeScript
  • ProcessBuilder$Redirect$Type

Perhaps it would be good to add those here as well. At least I was surprised not being able to use bare Character$UnicodeBlock.





[CLJ-1959] adding functions `map-vals` and `map-keys` Created: 14/Jun/16  Updated: 17/Apr/19

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

Type: Feature Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 51
Labels: None

Attachments: Text File map-mapper.patch     Text File map-mapper-v2.patch     Text File map-mapper-v3.patch    
Approval: Vetted

 Description   

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

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

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

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



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

code and test for map-keys and map-vals

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

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

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

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

On the patch:

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

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

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

Also should consider

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

Thanks for comments

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

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

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

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

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

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

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

A few comments:

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

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

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

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

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

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

Comment by Nathan Marz [ 14/Nov/17 2:15 PM ]

-1 to this. Clojure aims to be a small core, pushing additional functionality into libraries. The problem space of compound transformations, of which this functionality is a small piece, is already thoroughly solved by Specter. Specter's `MAP-VALS` and `MAP-KEYS` navigators additionally support removal of key/value pairs during transformation by transforming to special `NONE` value. This expands the utility greatly.

Also worth noting is a fast implementation requires a totally different approach depending on the type of the map. `reduce-kv` with transients is optimal for hash maps, but for array maps using lower level facilities provide ~60% speed boost. See Specter's implementation here https://github.com/nathanmarz/specter/blob/1.0.4/src/clj/com/rpl/specter/navs.cljc#L243

Comment by Ghadi Shayban [ 14/Nov/17 4:40 PM ]

Nathan you're making a strawman re: compound transformations. This isn't a request for a function with filtering knobs or conditional behavior (which Clojure has historically opposed). There are other valid approaches than Specter's.

Re fast implementation: Not every function has to strive for the most performant implementation, esp at the cost of branching and general complexity. A cost model for performance has to take into account complexity.

This ticket is a request for a convenience that is repeated in many codebases.

Do we want to preserve metadata? Many map operations do.
Do we want to assume IEditableCollection?

Comment by Nathan Marz [ 14/Nov/17 8:57 PM ]

Performance is incredibly important for general data structure manipulation functions like this. Manipulating data structures is one of the most basic things you do in a language, and it's done all the time in performance sensitive code. Having to abandon the "nice" functions for ugly specialized code in performance sensitive code is a failure of the language.

`map-vals`/`map-keys` are part of a rich problem space of which myself and the users of Specter have learned a lot the past few years. Clojure barely touches this problem space, especially when it comes to nested or recursive data structures. Adding functions to Clojure that are significantly inferior in both semantics and performance to what already exists in an external library seems pretty silly to me. Just my two cents.

Comment by Hiroyuki Fudaba [ 17/Nov/17 6:48 AM ]

I agree with Nathan Martz that the performance is very important, but I still have a strong opinion that this function should be somehow imported to the core part of the language.
People use this transformation pretty often and if there is a fast implementation in the core it will be a great benefit to all of us.





[CLJ-2505] clojure.walk/keywordize-keys and stringify-keys should not turn Records into maps Created: 16/Apr/19  Updated: 17/Apr/19

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

Type: Defect Priority: Minor
Reporter: Andrew Mcveigh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: walk
Environment:

All


Attachments: Text File 0001-Use-original-data-structure-when-transforming-keys.patch    
Patch: Code
Approval: Triaged

 Description   

The functions clojure.walk/keywordize-keys and stringify-keys do not respect Records. When called on a data structure, any Records in the data structure will be transformed into plain maps.

E.G.,

user> (defrecord X [a])
user.X

user> (assoc (X. 1) "thing" 2)
#user.X{:a 1, "thing" 2}

user> (clojure.walk/keywordize-keys (assoc (X. 1) "thing" 2))
{:a 1, :thing 2}

user> (type (clojure.walk/keywordize-keys (assoc (X. 1) "thing" 2)))
clojure.lang.PersistentArrayMap


 Comments   
Comment by Alex Miller [ 17/Apr/19 8:23 AM ]

The proposed patch seems like it has other consequences for non-records. For example, sorted maps would retain their sortedness whereas they do not currently. That may or may not be desirable, but it's certainly a much broader change in behavior than that implied by the ticket.

Does the change in stringify-keys even work? Doesn't seem like it when eyeballing it and causes test failures when applied.





[CLJ-2503] [spec] non blank string spec Created: 11/Apr/19  Updated: 11/Apr/19

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

Type: Feature Priority: Trivial
Reporter: JAre Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: generator, predicate, spec, string

Approval: Triaged

 Description   

I often need a spec that can validate non blank string:

(s/def ::non-blank-string (complement str/blank?))

(s/valid? ::non-blank-string "foo")
;; => true

It throws with non string values and lacks a generator:

(s/exercise ::non-blank-string)
;; =>  Unhandled clojure.lang.ExceptionInfo...

(s/valid? ::non-blank-string 42)
;; => Unhandled java.lang.ClassCastException...

It can be solved with:

(s/def ::non-blank-string (s/and string? (complement str/blank?)))

(s/exercise ::non-blank-string)
;; => (["8" "8"] ["pS" "pS"] ["RL" "RL"] ["3" "3"] ["c2ZUx" "c2ZUx"] ["24VF" "24VF"] ["0wc80" "0wc80"] ["Wr49vz" "Wr49vz"] ["1UDte" "1UDte"] ["2f" "2f"])

(s/valid? ::non-blank-string 42)
;; => false

The problem: The need in this spec seems to be recurring problem and I end up re-implementing it again and again. Sometimes I wrongly name it ::non-empty-string or create a separate namespace with the spec so I can reuse it.

Solution: Make it standard predicate. Mb it can be called text?






[CLJ-2502] Cannot use clojure.stracktrace/print-stack-trace with GraalVM Created: 08/Apr/19  Updated: 09/Apr/19

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

Type: Defect Priority: Major
Reporter: Michiel Borkent Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-2502-2.patch     Text File CLJ-2502.patch    
Patch: Code
Approval: Screened

 Description   

Problem
I tried to use clojure.stracktrace/print-stack-trace with GraalVM but that doesn't work since reflection is needed to find the method getMessage.

Proposed solution
This can be resolved by adding type hints.

An overview of warnings emitted by

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

Reflection warning, /tmp/stacktrace.clj:24:18 - reference to field getCause can't be resolved.
Reflection warning, /tmp/stacktrace.clj:32:15 - reference to field getClassName can't be resolved.
Reflection warning, /tmp/stacktrace.clj:33:9 - reference to field getMethodName can't be resolved.
Reflection warning, /tmp/stacktrace.clj:38:26 - reference to field getFileName can't be resolved.
Reflection warning, /tmp/stacktrace.clj:38:47 - reference to field getLineNumber can't be resolved.
Reflection warning, /tmp/stacktrace.clj:45:42 - reference to field getMessage can't be resolved.
Reflection warning, /tmp/stacktrace.clj:24:18 - reference to field getCause can't be resolved.

Patch CLJ-2502-2.patch removes unwanted whitespace changes and moves the type hint of root-cause higher up.

Screened by: Alex Miller



 Comments   
Comment by Michiel Borkent [ 08/Apr/19 3:33 AM ]

I'd be happy to implement the fix myself, after some vetting of this issue.

Comment by Alex Miller [ 08/Apr/19 7:14 AM ]

Go for it! Feel free to put the warn-on-reflect setting in the top of the code in the patch.

Comment by Michiel Borkent [ 08/Apr/19 4:15 PM ]

Tested this with GraalVM and it now works.

Comment by Alex Miller [ 08/Apr/19 5:08 PM ]

There seem to be some extraneous indenting changes in there - if you could minimize those, that would be helpful. For root-cause, I'd prefer the type hint as high as possible (in the arg). Can you update those?

Comment by Michiel Borkent [ 09/Apr/19 2:17 AM ]

Done.





[CLJ-2501] Provide guidance on configuring error printer for handling errors Created: 02/Apr/19  Updated: 03/Apr/19

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

Type: Enhancement Priority: Major
Reporter: Ben Brinckerhoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting
Environment:

Clojure 1.10.1-beta1


Approval: Triaged

 Description   

As of Clojure 1.10.1-beta1, errors in non-REPL environments are handled with `ex-str`, which is nice because `ex-str` in turn calls `explain-out` which is user-configurable.

However, there is not clear guidance on how best to configure `s/explain-out` such that this configuration will be run before other namespaces are loaded. Perhaps 'user.clj' is the correct place for this, but will that always be loaded for all non-REPL tasks?

For instance, this code would set up Expound, but right now I'm not sure how to reliably run it before other namespaces load:

(require '[expound.alpha :as expound] '[clojure.spec.alpha :as s])
(alter-var-root #'s/*explain-out* (constantly (expound/custom-printer {:print-specs? false :show-valid-values? true :theme :figwheel-theme})))





[CLJ-2500] Doc strings for spit and clojure.java.io/writer mention but do not explain opts Created: 01/Apr/19  Updated: 01/Apr/19

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

Type: Enhancement Priority: Minor
Reporter: James Elliott Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

I was trying to point my users at the reference documentation for spit to help them understand how they could customize output, and found that the options like `:append true` are not documented. Even if they are resourceful enough to figure out how to get to the docstring for clojure.java.io/writer itself, the options are not documented there either. I would basically need to teach them how to open a repl to explore the source code, or find it on GitHub, in order to figure this stuff out. Since they are generally not software developers, but musicians or their lighting/visuals staff trying to use a tiny bit of Clojure code to glue together shows, it would be nice if this information was easier to surface.

Some of this information can be found by scrolling down into examples on clojuredocs.org, but it would seem nice to have a definitive list of the options supported in the doc strings themselves.

Patch: clj-2500.patch






[CLJ-2498] [spec] force a function to validate it's input using fdef definitions Created: 27/Mar/19  Updated: 27/Mar/19

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

Type: Feature Priority: Minor
Reporter: Tommi Reiman Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: spec


 Description   

Currently, there is no way to force function fdef validation on, outside of tests and immune to override with dynamic vars. Forcing a validation in a function can be done using either with a) :pre hook or b) a manual s/conform / s/valid? call but those do not contribute to the function documentation. There are many use-cases for functions that always validate the inputs, e.g. when configuring components on application startup, done once where the performance penalty doesn't matter and correctness is important.

Schema has the :always-validate metadata for this case:

(require '[schema.core :as s])

(s/defn ^:always-validate interceptor-x
  [opts :- {:string? s/Bool}])

(interceptor-x {})
; Syntax error (ExceptionInfo) compiling at (test.cljc:150:1).
; Input to interceptor-x does not match schema:
;
;       [(named {:string? missing-required-key} opts)]


 Comments   
Comment by Tommi Reiman [ 27/Mar/19 9:50 AM ]

Maybe something like:

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

(defn plus1 [x]
  (inc x))

(s/fdef ^:always-validate plus1
        :args (s/cat :x int?))

(plus1 "1")
; Syntax error (ExceptionInfo) compiling at (user.clj:18:1).
; Call to #'user/plus1 did not conform to spec.




[CLJ-1929] Can't type hint literal collection containing runtime values Created: 16/May/16  Updated: 25/Mar/19

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

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

OS X 10.11.4


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

 Description   

Currently type hints on literals containing runtime values are lost:

user=> (set! *warn-on-reflection* true)
true
user=> (fn [x] (java.util.HashMap. ^java.util.Map {"foo" x}))
Reflection warning, null:1:9 - call to java.util.HashMap ctor can't be resolved.
#function[user/eval7479/fn--7480]

A workaround that currently works is to introduce a local and type hint the binding

user=> (fn [x] (let [^java.util.Map m {"foo" x}] (java.util.HashMap. m)))
#function[user/eval7487/fn--7488]

The attached patch fixes this issue by tracking type hints for literal values, making the reflection go away even in the original case

user=> (set! *warn-on-reflection* true)
true
user=> (fn [x] (java.util.HashMap. ^java.util.Map {"foo" x}))
#function[user/eval7479/fn--7480]

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






[CLJ-2496] [spec] explain-data :via loses aliased keys Created: 24/Mar/19  Updated: 24/Mar/19

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

Type: Defect Priority: Major
Reporter: Krzysztof Władyka Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec


 Description   

Full code example:

(ns api.foo
  (:require [clojure.spec.alpha :as s]))

(s/def ::common-email (s/and string? (partial re-matches #"^[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,63}$")))

;(s/def :foo/email (s/and ::common-email))
(s/def :foo/email ::common-email)

(s/def :db/foo (s/keys :req [:foo/email]))

(comment
  (->> (s/explain-data :db/foo {:foo/email "bar"})
       ::s/problems))

Issue:

({:path [:foo/email],
  :pred (clojure.core/partial clojure.core/re-matches #"^[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,63}$"),
  :val "bar",
  :via [:db/foo :api.foo/common-email],
  :in [:foo/email]})

Dirty hack
But if I use `(s/def :foo/email (s/and ::common-email))` instead it return

({:path [:foo/email],
  :pred (clojure.core/partial clojure.core/re-matches #"^[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,63}$"),
  :val "bar",
  :via [:db/foo :foo/email :api.foo/common-email],
  :in [:foo/email]})

expected behaviour
(s/def :foo/email ::common-email)
return
{{:via [:db/foo :foo/email :api.foo/common-email]}}

What happen here?
[:db/foo :api.foo/common-email] vs [:db/foo :foo/email :api.foo/common-email]

So (s/def :foo/email ::common-email) is totally omit here. In my opinion it is a bug, not a feature

Why is it important to fix?
It is important to keep it full tracked to turn this errors to right communication in User Interface.
So `[:db/foo :foo/email :api.foo/common-email]`, I am looking if I have proper message for UI. First `:api.foo/common-email`. No message, then check `:foo/email`. I have message for it so I can return "E-mail is not valid".

In practice it can be `:user/password` with split spec for length, special characters etc. validation or `:company/vat-id` based on country. But always I don't want to keep final validation at that moment, because things like street, phone number, vat-id, email etc. are common and I want to have one definition in one place.

On top of it I can do for example `(s/def :user/email (s/and (s/conformer clojure.string/lower-case) ::s-common/email))`. Here is the point. But not all e-mail validations will do lower-case. So today I have to use dirty hack or have redundant e-mail validation in all places which is harder to maintenance.

I don't want to base on `::common-email`, because this part is considered to change in any moment. It can be bookkeeping library with common definition for European Union vat-id. I don't want to base messages for UI for this library, I want to base on my definitions in my code, but `(s/def :company/vat-id ::bookkeeping-library/vat-id)` lose in `:via`.

We can imagine it is deeper and more complex structure. At that moment figuring out what is the issue of fail is rocket science, that is why I use dirty hack `(s/def :foo/email (s/and ::common-email))` to read it simple from `:via`.






[CLJ-2495] prepl docstring doesn't correctly doc exception :ret maps Created: 22/Mar/19  Updated: 22/Mar/19

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

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

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

 Description   

The docstring drifted out of sync with the code prior to 1.10 release. Needs to doc the :exception flag for :ret maps.

Patch: clj-2495.patch






[CLJ-2494] clojure.reflect protocol meta regression Created: 22/Mar/19  Updated: 22/Mar/19

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.reflect, regression

Approval: Triaged

 Description   

Two users have reported regressions from the protocol meta added to clojure.reflect returns in Clojure 1.10.

The shortest repro is something like this:

(require 'clojure.reflect)
(eval `~(clojure.reflect/reflect String))

;; Execution error (IllegalArgumentException) at user$eval9/<clinit> (REPL:1).
;; No matching ctor found for class clojure.reflect$typesym$fn__11946

ExceptionInInitializerError
	sun.reflect.NativeConstructorAccessorImpl.newInstance0 (NativeConstructorAccessorImpl.java:-2)
	sun.reflect.NativeConstructorAccessorImpl.newInstance (NativeConstructorAccessorImpl.java:62)
	sun.reflect.DelegatingConstructorAccessorImpl.newInstance (DelegatingConstructorAccessorImpl.java:45)
	java.lang.reflect.Constructor.newInstance (Constructor.java:423)
	java.lang.Class.newInstance (Class.java:442)
	clojure.lang.Compiler$ObjExpr.eval (Compiler.java:4996)
	clojure.lang.Compiler.eval (Compiler.java:7175)
	clojure.lang.Compiler.eval (Compiler.java:7131)
	clojure.core/eval (core.clj:3214)
	clojure.core/eval (core.clj:3210)
	user/eval7 (NO_SOURCE_FILE:1)
	user/eval7 (NO_SOURCE_FILE:1)
Caused by:
IllegalArgumentException No matching ctor found for class clojure.reflect$typesym$fn__11946
	clojure.lang.Reflector.invokeConstructor (Reflector.java:288)
	clojure.lang.LispReader$EvalReader.invoke (LispReader.java:1317)
	clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:853)
	clojure.lang.LispReader.read (LispReader.java:285)
	clojure.lang.LispReader.read (LispReader.java:216)
	clojure.lang.LispReader.read (LispReader.java:205)
	clojure.lang.RT.readString (RT.java:1878)
	clojure.lang.RT.readString (RT.java:1873)
	user/eval9 (NO_SOURCE_FILE:1)
	sun.reflect.NativeConstructorAccessorImpl.newInstance0 (NativeConstructorAccessorImpl.java:-2)
	sun.reflect.NativeConstructorAccessorImpl.newInstance (NativeConstructorAccessorImpl.java:62)

The actual cases where this is happening are uses of clojure.reflect in values returned by macro (see https://groups.google.com/forum/#!msg/clojure/pxLN9tYti4c/zbhLCOrTBgAJ for one example).

Cause: Class symbols returned from clojure.reflect are now adorned with meta like:

#:clojure.core.protocols{datafy #object[clojure.reflect$typesym$fn__11946 0x49fc609f "clojure.reflect$typesym$fn__11946@49fc609f"]}

which has an embedded datafy function. The compiler emits this into the compiled class pool (MetaExpr wrapping the object).

Workaround: A workaround is to strip the meta from the symbols inside the macro and return the stripped symbol.






[CLJ-2459] ExceptionInInitializerError if jars executed with java -jar Created: 20/Dec/18  Updated: 22/Mar/19

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

Type: Defect Priority: Trivial
Reporter: Piotr Zygielo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build
Environment:

N/A


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

 Description   

Main-Class set in manifests of target/clojure.jar (or jars in Maven repository) gives impression, that they can be run as standalone JAR-packaged applications.

However running them with

java -jar clojure-${version}.jar
results in

Exception in thread "main" java.lang.ExceptionInInitializerError
    at clojure.main.<clinit>(main.java:20)
    Caused by: Syntax error compiling at (clojure/main.clj:1:1).
    at clojure.lang.Compiler.load(Compiler.java:7647)
    ...
    ... 1 more
    Caused by: java.io.FileNotFoundException: Could not locate clojure/spec/alpha__init.class, clojure/spec/alpha.clj or clojure/spec/alpha.cljc on classpath.
    at clojure.lang.RT.load(RT.java:466)
    at clojure.lang.RT.load(RT.java:428)

(It of course works perfectly fine for top-level clojure.jar built in local profile with dependencies shaded.)

Yet, for java -jar classpath cannot be modified in command line (-cp is ignored), only as Class-Path entries in manifest.
I'd prefer to see clean

no main manifest attribute...
reported by java rather than ExIIError and stack trace in such case.

Screened by: Alex Miller - correctly builds normal Clojure without a main class, but continues to build local jar with a main (that one includes the spec deps).



 Comments   
Comment by Alex Miller [ 21/Dec/18 8:25 AM ]

Thanks, good call.

Comment by Piotr Zygielo [ 22/Mar/19 4:52 AM ]

I see it has assigned 1.11 as fix version, but I also saw preparation for 1.10.1. Could CLJ-2459 be included in 1.10.1 as well?

Comment by Alex Miller [ 22/Mar/19 7:53 AM ]

We are trying to keep the changes in 1.10.1 very focused, so it will wait for 1.11.

Comment by Piotr Zygielo [ 22/Mar/19 7:58 AM ]

Understood, thanks.





[CLJ-2493] clojure.java.shell/sh hangs calling xdg-open Created: 21/Mar/19  Updated: 21/Mar/19

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

Type: Defect Priority: Major
Reporter: Jonathan D Johnston Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Clojure 1.10.0
ClojureScript 1.10.516
Xubuntu 16.04


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

 Description   

Please note that not only is this important for clojure.java.shell and, by implication, clojure.java.browse, but this will also enable the ClojureScript browser REPL to correctly launch the browser on platforms where xdg-open is used. Currently the ClojureScript Quick Start instructions are, essentially, broken on many systems. Most first-time users of ClojureScript won't enjoy debugging something like this (or maybe I'm just more stubborn than most ).

On some platforms clojure.java.browse/browse-url calls clojure.java.shell/sh to execute a cmdline using xdg-open to launch the web browser. As a proper *Nix utility xdg-open passes along its open file descriptors to the new process. (I don't know if this is documented somewhere for xdg-open, but it's easy to demonstrate.)

clojure.java.shell/sh always reads both the STDOUT & STDERR streams from the executed process. When the purpose is to gather the output of the subprocess that, of course, is a good thing. However, when launching a browser with xdg-open, the streams aren't closed until the browser exits. For a caller to clojure.java.browse/browse-url the function seems to "never" return.

What we need is a variant of clojure.java.shell/sh (or an option to sh) that ignores the output streams & only returns the exit code. We're using the subprocess strictly for its side-effect. Somewhat analogous to using doseq instead of map, except that we do want the exit code of the subprocess.

To get the ClojureScript browser REPL working, I made a local copy of clojure.java.{browse,shell}, added a function (launch) to clojure.java.shell that ignores the I/O streams but returns the exit code, & modified browse-url to use launch. Much better! Both browser & REPL, just as promised in the Quick Start instructions.

Personal note: While I've just started experimenting with ClojureScript, I've been using Clojure for several years

Prescreened by: Alex Miller



 Comments   
Comment by Jonathan D Johnston [ 21/Mar/19 5:49 PM ]

clj-2493.patch Attached 21 March 2019

Comment by Andy Fingerhut [ 21/Mar/19 7:04 PM ]

I have tested (clojure.java.browse/browse-url "https://github.com") in a clj REPL on a local machine of mine in Clojure 1.10.0, and it does indeed not give another REPL prompt, unless the browser is closed. I made the last changes to this function to add xdg-open capability in 2012, and I am pretty sure it did not do this then, but perhaps the behavior of xdg-open has changed since then. The Clojure source for the files containing clojure.java.browse/browse-url and clojure.java.shell/sh have not changed since 2012.

Here is the system I have good test results from:

Ubuntu 16.04.6 Desktop Linux with latest updates as of 2019-Mar-21
OpenJDK 1.8.0_191 installed from openjdk-8-jre-headless and openjdk-8-jdk-headless Ubuntu packages
latest Clojure master as of 2019-Mar-21 plus clj-2493.patch

Comment by Andy Fingerhut [ 21/Mar/19 7:17 PM ]

These changes do cause a test to fail when building Clojure, I think because the new launch function has no {:added "1.11"} metadata after the doc string.

Comment by Jonathan D Johnston [ 21/Mar/19 7:30 PM ]

I didn't think it was my place to include the {:added "1.11"}, especially since it hasn't been added at this point in time. Should I add the metadata & build a new patch - either an update or a replacement?

Comment by Alex Miller [ 21/Mar/19 7:37 PM ]

There are two trailing whitespace errors when I apply the patch - lines 43 and 49 of the current patch. Would be good if you could tidy those up.

Regarding the metadata, either add the metadata with 1.11 and leave a note about it in the description OR leave it out but make a note in the description that the test will fail until it's added. Either way I can fix it up when we get to a point where it's being screened for a release.

Comment by Jonathan D Johnston [ 21/Mar/19 8:06 PM ]

Second patch of 21 March 2019 - Replaces first patch

  • Removed trailing whitespace
  • Added metadata in anticipation of acceptance




[CLJ-2492] Uses of deprecated Class.newInstance() method Created: 19/Mar/19  Updated: 19/Mar/19

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

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

 Description   

As of Java 9, Class.newInstance() has been deprecated (see https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/lang/Class.html#newInstance() ). These calls can be replaced with a call to clazz.getDeclaredConstructor().newInstance(). The reason for the deprecation is that that path bypasses compile-time exception checking. Not a big deal, but better to fix this in case it's removed in the future.

There are two cases of this in Clojure - one in Compiler and one in FnLoaderThunk (no longer used). Patch changes both.

Patch: clj-2492.patch






[CLJ-2490] Add symbol literal syntax with the same alias resolving behavior as auto-resolved keywords Created: 19/Mar/19  Updated: 19/Mar/19

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

Type: Feature Priority: Minor
Reporter: Moritz Heidkamp Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Rationale

When the namespace component of an auto-resolving keyword literal refers to an undeclared alias, the reader produces an error, like so:

user> ::foo/bar
RuntimeException Invalid token: ::foo/bar  clojure.lang.Util.runtimeException (Util.java:221)

For symbols, however, no such syntax exists. This is a common source of errors with functions like clojure.spec.test.alpha/instrument which accept qualified symbols of var names as arguments. Currently, the go-to notation for such symbol literals is syntax quote which of course accepts any namespace. Now, when one accidentally refers to an alias in one of those symbolic arguments which hasn't been declared in the current namespace (e.g. when transplanting some code from one namespace to another), it would be very useful to have the same error behavior as with auto-resolving keywords. To stick with the instrument example, one would end up calling potentially expensive real functions instead of the mocks which could easily go unnoticed.

Suggested syntax

This is a bit tricky. The first thing that comes to mind is double backtick, as in ``foo/bar, but that already has meaning and is likely to already be used in existing macros. I guess %foo/bar could work, though there is of course no guarantee that this isn't already in use in actual namespace names, too.

Status

As you can tell, I haven't done an extensive survey of possible syntax options, yet, nor have I started working on a patch. Just wanted to solicit opinions on the issue to see if it's worth pursuing any further.



 Comments   
Comment by Alex Miller [ 19/Mar/19 7:44 AM ]

While I understand your use case, this would be a big syntax addition and I think it’s unlikely. I’m going to Backlog this for now.

Comment by Moritz Heidkamp [ 19/Mar/19 1:51 PM ]

Agreed, it's a pretty invasive change probably not worth the trouble. FWIW, in the meantime I figured that the same can be achieved via a tagged literal. In case anyone is looking for a solution, you could have a function like this:

(defn auto-resolved-symbol-reader [sym]
  (let [ns (if-let [alias (namespace sym)]
             (or (get (ns-aliases *ns*) (symbol alias))
                 (throw (ex-info (str "Invalid namespace alias on symbol " sym)
                                 {:symbol sym})))
             *ns*)]
    (symbol (name (ns-name ns)) (name sym))))

Register it e.g. for the sym tag by putting it in data_readers.clj at the root of your classpath:

{sym my.data-readers/auto-resolved-symbol-reader}

And now you can use it to the same effect as ::foo/bar for keywords like this:

'#sym foo/bar
Comment by Moritz Heidkamp [ 19/Mar/19 2:06 PM ]

In case of the :replace argument from the instrument example, there's another solution that already works. Just use auto-resolved map namespaces with syntax quote like this:

:replace `::foo{bar ~some-fn}

The catch is of course that you need to unquote values now. Note that the syntax quote needs to apply to the whole expression - if it goes in front of the key, the map's namespace won't apply to it because it will already be namespaced by the syntax quote. Actually, one could expect

::foo{'bar some-fn}
to also namespace the symbol key but that's not happening. Maybe it only accidentally works with syntax quote after all...





[CLJ-2343] define and load classes in memory with gen-class Created: 30/Mar/18  Updated: 13/Mar/19

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: gen-class

Attachments: Text File 0001-CLJ-2343-define-and-load-class-while-JITing-gen-clas.patch    
Patch: Code
Approval: Triaged

 Description   

Currently gen-class only works while AOT compiling, but just evaluates to nil while JIT loading.
The reason for this behaviour is historical and no longer relvant since CLJ-979 landed in 1.7, which fixed the dynamic classloader definition issues and also made this exact same change for gen-interface. The only reason why this wasn't also done for gen-class is that I forgot about it.

This patch fixes this inconsistency

Patch: 0001-CLJ-2343-define-and-load-class-while-JITing-gen-clas.patch



 Comments   
Comment by Kari Marttila [ 01/Jan/19 12:33 PM ]

Just asking if the issue I asked in Clojure Slack relates to this JIRA issue.
If I have a gen-class (e.g. mygenclass) as part of my Clojure project and I want to refresh all namespaces in Clojure REPL using command:
(do (require '[clojure.tools.namespace.repl :refer [refresh]]) (refresh))
... then I get an error: "namespace 'mygenclass' not found after loading 'mygenclass'.

Comment by Kimmo Koskinen [ 02/Jan/19 3:37 PM ]

Now that there is Deps and CLI, this change would make it even more easier package a library using that uses gen-class, since no class files would need to be packaged (if I understood correctly).

Comment by Kimmo Koskinen [ 13/Mar/19 4:22 PM ]

Just a friendly nudge, is the anything that could be helped with this change?





[CLJ-2489] 1 arg arity for preserving-reduced Created: 07/Mar/19  Updated: 07/Mar/19

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is a real corner case, so probably not worth much attention. I was experimenting with writing a version of tree-seq using reduction rather than sequences. Here's what I came up with:

(defn tree-producer [branch? children node]
  (let [recurse #(tree-producer branch? children %)]
    (reify clojure.lang.IReduceInit
      (reduce [this rf rval]
        (let [rval (rf rval node)]
          (cond
            (reduced? rval) @rval
            (branch? node) (transduce (mapcat recurse) rf rval (children node))
            :else rval))))))

However, using it resulted in an exception:

user=> (into [] (tree-producer seq? identity '((1 2 (3)) (4))))
Execution error (ArityException) at temp$tree_producer$reify__7693/reduce (temp.clj:129).
Wrong number of args (1) passed to: clojure.core/preserving-reduced/fn--8743

The exception is not thrown if preserving-reduced is given a 1 argument arity, ie:

(defn ^:private preserving-reduced
  [rf]
  (fn ( [rval] rval)
      ( [rval x]
        (let [ret (rf rval x)]
          (if (reduced? ret)
            (reduced ret)
            ret)))))





[CLJ-2488] Improve docstring for reify Created: 04/Mar/19  Updated: 04/Mar/19

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

Type: Enhancement Priority: Trivial
Reporter: Rodrigo Dias Arruda Senra Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: docstring

Attachments: Text File 0001-Reify-docs-update.patch     Text File clj-2488-2.patch    
Patch: Code
Approval: Screened

 Description   

I think the docs for reify says a lot about how but misses the point for what "reify" does.

user=> (doc reify)
-------------------------
clojure.core/reify
([& opts+specs])
Macro
  reify is a macro with the following structure:

 (reify options* specs*)
...

Proposed: Add a definition of what reify does:

user=> (doc reify)
-------------------------
clojure.core/reify
([& opts+specs])
Macro
  reify creates an anonymous instance implementing a protocol or interface.
  reify is a macro with the following structure:

 (reify options* specs*)
...

Patch: clj-2488-2.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 04/Mar/19 11:22 AM ]

Added -2 patch with a small rewrite, retained attribution.





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

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

Type: Defect Priority: Major
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 16
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

Comment by Jason Felice [ 21/Jan/19 5:44 PM ]

With Graal VM rc11 (just released), clj-1472-3.patch works without issue.

Comment by JAre [ 14/Feb/19 9:49 PM ]

Started getting this error with GraalVM native compilation when migrated from Clojure 1.9 to 1.10
Minimal repro is:

(ns clj10-graal-repro.core
  (:require [clojure.spec.alpha :as s])
  (:gen-class))

(s/def ::foo (s/coll-of string?))

(defn -main
  [& args]
  (println (s/valid? ::foo ["bar"]))
  (println *clojure-version*))

I think the problem is that Clojure 1.10 depends on spec.alpha 0.2.176 that uses locking

More details in CLJ-2482





[CLJ-2480] Document that a promise can be invoked to deliver Created: 11/Feb/19  Updated: 03/Mar/19

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File clj-2480-2.patch     Text File promise-as-fn-docstring.patch    
Patch: Code
Approval: Prescreened

 Description   

Promises can be invoked to deliver a value to them (same result as calling deliver), but this is not mentioned in the docstring:

(def p (promise))
(p 42) ;; same as (deliver p 42)
@p ;; 42

Docstring before:

Returns a promise object that can be read with deref/@, and set,
  once only, with deliver. Calls to deref/@ ...

Proposed: Change docstring to:

clojure.core/promise
([])
  Returns a promise object that can be read with deref/@, and set,
  once only, with deliver or by invoking the promise. Calls to
  deref/@ ...

Patch: clj-2480-2.patch
Prescreened by: Alex Miller



 Comments   
Comment by Howard Lewis Ship [ 11/Feb/19 2:06 PM ]

I think we disagree about the value (or definition) of excessive concision in docstrings.

Comment by Phill Wolf [ 03/Mar/19 6:24 AM ]

Calling a promise does not always return the promise. Also, the docstring on deliver does not describe the return value. I hope the two docstrings (promise and deliver) will be kept in synch.

Opinion: it looks to me in the clojure source code like the IFn might just have been a reify hack to let deliver touch the promise. If calling the promise directly had been the plan for the official API, deliver would not exist. Do we really need two ways to deliver? If folks start eschewing deliver, I'll lose a great word to grep clj files for!





[CLJ-2251] [spec] Generic spec walking for clojure.spec Created: 11/Oct/17  Updated: 27/Feb/19

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: 19
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.

Comment by Tommi Reiman [ 27/Feb/19 2:16 AM ]

There are three different versions for ~this in spec-tools now:

1) spec-walker: using `s/form`, walks the specs and values and calls a callback function with both, returning a new value. It can be used for things like coercion. It doesn't validate, need to call `s/valid?` and `s/explain-data` separately (2-3 calls in total)

2) spec-visitor: just walks the forms and return new forms. Used in things like spec => json-schema transformations. Not transforming values.

3) wrapped specs with overridden `s/conform*`, returning new value or error. It's the saddest of the three, because everything needs to be wrapped, but still the only one that knows how to walk over properly regex specs. Bundled transform + validate, like with Schema.

...

https://cljdoc.org/d/metosin/spec-tools/0.9.0/doc/spec-coercion has examples of the first/coercion, hopefully highlighting why support for coercion is important.

Is this issue on a (near) roadmap? Can we help with it? If not on the roadmap, is there a suggested way to do runtime value transformations with spec?





[CLJ-2487] Implement completion/exception handling callbacks for promises and futures Created: 21/Feb/19  Updated: 26/Feb/19

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Triaged

 Description   

Based on https://dev.clojure.org/display/design/Promises

Discussion on Slack:

Enhance `future`/`promise` to be able to call a function on completion/failure (I'd use promises a lot more if I had this functionality but you can sort of hack around it in ugly ways with futures and/or additional functions/macros).






[CLJ-2365] Integration with java.util.function interfaces Created: 26/Jun/18  Updated: 25/Feb/19

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: 19
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-2347] [spec] 'inst?' spec generator produces unreadable instants Created: 15/Apr/18  Updated: 20/Feb/19

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

Type: Defect Priority: Minor
Reporter: David Bürgin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: spec
Environment:

Clojure 1.9.0
org.clojure/spec.alpha 0.1.143
org.clojure/test.check 0.9.0


Approval: Triaged

 Description   

The spec generator associated with inst? may produce instants that are technically valid but cannot be read back (and also are not practical in reality).

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

(second (last (s/exercise inst? 100)))
;; => #inst "883641-02-19T16:17:26.482-00:00"

#inst "883641-02-19T16:17:26.482-00:00"
;; => RuntimeException Unrecognized date/time syntax: 883641-02-19T16:17:26.482-00:00  clojure.instant/fn--7987/fn--7988 (instant.clj:107)

Minor issue, but I ran into this while interacting with inst generator/generated values in the REPL.



 Comments   
Comment by David Bürgin [ 12/May/18 3:53 AM ]

This problem gets worse when the inst generated is in the BC era. In that case, not even can the inst potentially not be read back, but since there is no indication of the ‘negative year’, information is lost.

Comment by Enzzo Cavallo [ 20/Feb/19 7:20 AM ]

There is some info about the limits of reader/printer in CLJ-2486
I catch this problem because I'm generating data and `pr-str` / `read-string` over the wire.





[CLJ-2464] peek does not support transient vectors Created: 07/Jan/19  Updated: 19/Feb/19

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

Type: Defect Priority: Major
Reporter: yhq Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: transient

Approval: Triaged

 Description   

Appears to be an oversight, given that peek is a read operation.

Input:

(peek (transient [:a :b :c]))

Expected outcome:

:c

Actual outcome:

Unhandled java.lang.ClassCastException
clojure.lang.PersistentVector$TransientVector cannot be cast to clojure.lang.IPersistentStack


 Comments   
Comment by Steve Miner [ 19/Feb/19 2:16 PM ]

I just ran into this issue today. I agree it would be convenient if peek worked on a transient vector. Other functions such as nth and count work on transient vectors, so it's natural to want peek.

As a work-around (not a suggested fix), I use this:

(defn peek! [tv] (nth tv (dec (count tv))))

Admittedly, that's a misnomer but it fits the bang pattern for transient transformation of regular collection code. I also have a convenience function update! which calls assoc!. I offer the work-around just for users who run into this problem and want to get back to work.

Comment by Alex Miller [ 19/Feb/19 4:40 PM ]

I think this would mean PersistentVector.TransientVector would need to support IPersistentStack but that has two methods - peek and pop. Interestingly, ITransientVector and TransientVector both have pop() already. Seems like the pop() impl has all the smarts you'd need to implement peek() too.





[CLJ-2485] BigDecimal loses M suffix when converted to string using (str) Created: 17/Feb/19  Updated: 18/Feb/19

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

Type: Defect Priority: Minor
Reporter: Mikhail Gusarov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

(str 1M) produces "1"
(str {:a 1M}) procuces "{:a 1M}"

The suffix is lost because str calls .toString directly: java.lang.BigDecimal does not know anything about M suffix.

`.toString` on map calls RT.print and that recursively calls RT.print for keys and values. RT.print has a special case for java.lang.BigDecimal, so it prints the suffix.



 Comments   
Comment by Alex Miller [ 17/Feb/19 4:10 PM ]

If you want to print a string that can later be read, normally you use pr-str. Some reason that’s not better here?

str intentionally does not have the expectation to be readable.

Comment by daniel sutton [ 17/Feb/19 4:20 PM ]

This was discovered originally investigating `spit`. This particular behavior turned out not to be the culprit but was something noticed while investigating.

clojure -e '(spit "foo.edn" 3M)' && cat foo.edn
3

clojure -e '(spit "foo.edn" {:thing 3M})' && cat foo.edn
{:thing 3M}

We were wondering if using spit for edn was a footgun or if it was a genuine bug.

Comment by Mikhail Gusarov [ 17/Feb/19 4:21 PM ]

spit calls str, that's how I stumbled upon it.

It's trivial to make a pr-spit that calls pr-str, but isn't then spit kind of useless if its results can't be {{slurp}}ed back?

Comment by Alex Miller [ 18/Feb/19 12:15 PM ]

Oh, well that’s considerably more interesting. As a workaround you could pr-str before you spit.

The question of edn printing is one that has come up several times (there might even be a ticket about it).

Comment by Alex Miller [ 18/Feb/19 12:18 PM ]

CLJ-1201 is one such ticket





[CLJ-2483] [spec] generator in keys* spec not overridable Created: 16/Feb/19  Updated: 16/Feb/19

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


 Description   

Problem:

A generator in a keys* spec not overridable.

Repro:

(require '[clojure.spec.alpha :as s])
(require '[clojure.spec.gen.alpha :as gen])
(s/def ::foo (s/keys* :req-un [::a ::b]))
(s/def ::a number?)
(s/def ::b number?)
(s/valid? ::foo [:a 1 :b 2]) ;; true
(gen/generate (s/gen ::foo {::a (fn [] (gen/return 1))})) ;;=> (:a -15164 :b 24.0)

The last expression should return a sequence where the key :a is followed by 1 like: (:a 1 :b 24.0).

This may be traced back to a more general problem: overriding the generator on a spec that's made with with-gen seems to have no effect:

(gen/generate
   (s/gen (s/with-gen ::foo
            (fn []
              (s/gen (s/cat :key-a #{:a} :val-a ::a :key-b #{:b} :val-b ::b))))
          {::a (fn [] (gen/return 1))})) ;;=> (:a -1 :b "OXi2")





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

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: 2
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.

Comment by Jan Rychter [ 15/Feb/19 3:20 PM ]

I'll attempt a first (minimal) step here. Since set functions have docstrings that begin with "Return a set", one can reasonably expect that they will always return a set.

The specific case that I encountered was `(clojure.set/difference nil #{1})` returning nil. The reason why the nil appeared was because of optionality: a set operation was performed and the source data was optional in the map. Data passed spec validations (because the key was optional), and only later tripped other validations because of the nil returned by set/difference. I realize that the arguments are incorrect and I do not necessarily expect auto-coercion.

What I would expect is a post-condition error that would show up in code compiled with assertions.

Specifically, I think a `{:post [(set? %)]}` post-condition on set functions is something that can be agreed upon. It doesn't address coercion, and it avoids the discussion of what are valid arguments, it just places a restriction on the return value according to documentation.

Given what the docstrings say, I do not think there is code out there that relies on set functions returning anything else than a set.

The performance impact of a post-condition would only be present if compiled with assertions.

I realize this doesn't address all issues mentioned, but it's perhaps a way to start moving forward.

I do not think this is a "Major" issue as currently marked.





[CLJ-1187] Clojure loses quoted metadata on empty literals Created: 22/Mar/13  Updated: 15/Feb/19

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 3
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

Comment by Shogo Ohta [ 15/Feb/19 4:04 AM ]

Any chance this will be fixed yet? I've run into it recently.

Comment by Alex Miller [ 15/Feb/19 7:36 AM ]

It’s been vetted by Rich so it’s in the path to consideration.

Comment by Shogo Ohta [ 15/Feb/19 7:52 AM ]

Yey! I hope things will go smoothly





[CLJ-2481] [spec] every-impl breaks when passed records Created: 13/Feb/19  Updated: 13/Feb/19

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

Type: Defect Priority: Minor
Reporter: Sean Harrap Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: spec
Environment:

Mac OS X 10.13.6


Approval: Triaged

 Description   

Specs relying on every-impl (such as every, coll-of, etc) may break when passed a record. For example:

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

> (s/def ::coll-any (s/coll-of any?))
> (defrecord Pair [first second])

> (s/valid? ::coll-any 3)
false

> (s/valid? ::coll-any {:a 1 :b 2}
true

> (s/valid? ::coll-any (Pair. 1 2))
Execution error (UnsupportedOperationException) at user.Pair/empty (REPL:1).
Can't create empty: user.Pair





[CLJ-1509] Some clojure namespaces not AOT-compiled and included in the clojure jar Created: 20/Aug/14  Updated: 12/Feb/19

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

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

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

 Description   

There is a list of namespaces to AOT in build.xml and several namespaces are missing from that list, thus no .class files for those namespaces are created or included in the standard clojure jar file as part of the build.

Missing namespaces include:

  • clojure.core.reducers
  • clojure.instant
  • clojure.parallel
  • clojure.uuid

Proposal: Attached patch adds clojure.instant and clojure.uuid to the compiled namespaces. clojure.parallel is deprecated and requires the JSR-166 jar so was not included.

Patch: clj-1509.patch



 Comments   
Comment by Alex Miller [ 20/Aug/14 1:06 PM ]

Looking at this a bit further, clojure.core.reducers uses the compile-if macro to determine what version of fork/join is available so AOT-compiling this namespace would fix that decision at build time rather than runtime, so it cannot be included.

Comment by Alex Miller [ 12/Feb/19 11:06 AM ]

As of Clojure 1.10, which relies on Java 1.8, all of these conditional build cases have been removed. I believe clojure.instant and clojure.uuid are being transitively compiled now, though not explicitly.

Comment by Alex Miller [ 12/Feb/19 11:17 AM ]

Actually clojure.parallel is still dependent on things that only exist in jsr166, but all the others have had conditionality removed. Added missing patch.





[CLJ-2444] Typo in docstring of test-vars Created: 23/Nov/18  Updated: 11/Feb/19

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

Type: Enhancement Priority: Trivial
Reporter: Michiel Borkent Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

Patch fixes docstring: "runs test-vars" should be "runs test-var".

Screened by: Alex Miller






[CLJ-2476] Improve doc clarity regarding binding conveyance Created: 02/Feb/19  Updated: 07/Feb/19

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

Type: Enhancement Priority: Minor
Reporter: Matthias Varberg Ingesman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

It would be helpful for the binding conveyance feature of futures, agents and any other places that support it to be more prominent in the doc-strings. It could be as little as a note at the bottom saying something like: "This will propagate current thread's bindings to the new thread."

I have also added an issue to the [clojure-site github repo](https://github.com/clojure/clojure-site/issues/352) to add a note on binding conveyance to the vars reference page on clojure.org.

Patch: clj-2476.patch






[CLJ-2473] [core.specs] Destructuring spec is overly restrictive in namespaced :keys Created: 31/Jan/19  Updated: 31/Jan/19

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: core.specs, destructuring

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

 Description   

Namespaced :keys destructuring (see CLJ-1919) supports any kind of ident in a `::foo/keys` or `::foo/syms`, but the core spec says only `simple-symbol?` is allowed.

Example:

user=> (let [{::keys [:foo]} {::foo 1}] foo)
Syntax error macroexpanding clojure.core/let at (REPL:1:1).
...bunch of spec problems

Expected:

user=> (let [{::keys [:foo]} {::foo 1}] foo)
1

Proposed: Widen spec for this case from `simple-symbol?` to `ident?` (which the code supports).
Patch: clj-2473.patch



 Comments   
Comment by Thomas Heller [ 31/Jan/19 11:39 AM ]

ident? accepts namespaced keywords (and symbols) which probably should not be allowed given that the current implementation will ignore the namespace and use the namespace used by "keys" instead. To avoid confusion the spec should, in my opinion, be restricted to simple-keyword? or simple-symbol? in the namespaced "keys" case and only accept ident? for non-namespaced :keys.

(let [{:foo/keys [:other/bar]} {}])
(let [{:foo/keys [other/bar]} {}])
Comment by Alex Miller [ 31/Jan/19 12:14 PM ]

I considered that and decided to match the code. There is also consistency with :keys.





[CLJ-2472] [spec] 'check' silently ignores symbols which are not checkable Created: 30/Jan/19  Updated: 31/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: Josip Gracin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: error-reporting, spec
Environment:

[org.clojure/spec.alpha "0.2.176"], [org.clojure/core.specs.alpha "0.2.44"]


Attachments: Text File clj-2472-1.patch     Text File clj-2472-2.patch    
Approval: Triaged

 Description   

Function clojure.spec.test.alpha/check silently ignores symbols which are not "checkable". This seems counter-intuitive to me because I'd expect it to fail when I call check on a symbol and the symbol does not have a spec (e.g. it is misspelled).

The definition of check looks like this:

(defn check
 ([] (check (checkable-syms)))
 ([sym-or-syms] (check sym-or-syms nil))
 ([sym-or-syms opts]
    (->> (collectionize sym-or-syms)
         (filter (checkable-syms opts))
         (pmap
          #(check-1 (sym->check-map %) opts)))))

It seems to me that the (filter (checkable-syms opts)) might better go to the no-arg variant.



 Comments   
Comment by Josip Gracin [ 30/Jan/19 9:43 AM ]

Attaching clj-2472-1.patch. It preserves backward-compatibility which complicates it a bit.

Comment by Josip Gracin [ 31/Jan/19 3:24 AM ]

Minor change to the patch (using 'when' instead of 'if'). The idea of the solution is to introduce a new option :assert-checkable which asserts that provided syms are checkable.





[CLJ-2469] pr botches structmaps containing namespaced keys when *print-namespace-maps* Created: 20/Jan/19  Updated: 20/Jan/19

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

Type: Defect Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print, regression

Approval: Triaged

 Description   

pr misrepresents a structmap containing namespaced-keyword keys. pr misstates actual keys' values, and includes phantom keys.

user=> (pr-str (struct (create-struct :q/a :q/b :q/c) 1 2 3))
"#:q{:q/a nil, :q/b nil, :q/c nil, :c 3, :b 2, :a 1}"

It worked in Clojure 1.8, and disabling the print-namespace-maps feature appears to work around the problem:

user=> (binding [*print-namespace-maps* false] 
         (pr-str (struct (create-struct :q/a :q/b :q/c) 1 2 3)))
"{:q/a 1, :q/b 2, :q/c 3}"

By the way: Structmaps are (now as ever) appropriate when the basis is anonymous and computed (not coded) or key order is a significant aspect of the basis. People sometimes cite the reference doc's statement, "Most uses of StructMaps would now be better served by records", but, of course, it is referring only to those uses that are actually better served by records. Record classes can't be anonymous and defrecord needs the basis at compile time; and normal maps do not exhibit a preset key order. Thus the continuing relevance of structmaps.






[CLJ-1941] [spec] Instrumentation of fns with primitive type hints fails Created: 01/Jun/16  Updated: 18/Jan/19

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

Type: Defect Priority: Minor
Reporter: Kenny Williams Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: spec
Environment:

Ubuntu 15.10
Using boot 2.6.0 on openjdk version "1.8.0_91"


Approval: Vetted

 Description   
(require '[clojure.spec :as s] '[clojure.spec.test :as st])
(defn foo [^double val] val)
(s/fdef foo :args (s/cat :val double?))
(st/instrument `foo)
(foo 5.2)

user=> (foo 5.2)
ClassCastException clojure.spec.test$spec_checking_fn$fn__13069 cannot be cast to clojure.lang.IFn$DO
       	user/eval6 (NO_SOURCE_FILE:5)
       	user/eval6 (NO_SOURCE_FILE:5)
       	clojure.lang.Compiler.eval (Compiler.java:6951)
       	clojure.lang.Compiler.eval (Compiler.java:6914)
       	clojure.core/eval (core.clj:3187)
       	clojure.core/eval (core.clj:3183)
       	clojure.main/repl/read-eval-print--9704/fn--9707 (main.clj:241)
       	clojure.main/repl/read-eval-print--9704 (main.clj:241)
       	clojure.main/repl/fn--9713 (main.clj:259)
       	clojure.main/repl (main.clj:259)
       	clojure.main/repl-opt (main.clj:323)
       	clojure.main/main (main.clj:422)

Cause: spec replaces var values with instrumented functions that will not work with primitive function interfaces

Approach: Take primitive interfaces into account and make them work, or document/fail that instrumentation will not work with these.



 Comments   
Comment by Kevin Downey [ 02/Jun/16 1:41 AM ]

spec replaces var values with instrumented functions, which works for the default linking case, var deref cast to ifn, invoke, but in the other cases (primitive functions, direct linking, others?) this won't work

Comment by Kenny Williams [ 02/Jun/16 3:39 PM ]

Hmm. Well this should be at least be documented. So, spec cannot be used on functions with a type hinted arg?

Comment by Sean Corfield [ 02/Jun/16 4:16 PM ]

Spec cannot be used on functions with primitive typed hinted arguments or returns – non-primitive type hints seem to be fine.

But documentation isn't enough here: instrumenting a namespace and then discovering it broke a function (that happened to have a primitive type hint) isn't acceptable. If the instrumentation isn't going to work, the function should be skipped (and a warning produced, hopefully).

Comment by Kevin Downey [ 02/Jun/16 8:10 PM ]

yeah, I was giving the root cause of the issue, not excusing the issue.

Understanding the root cause predicts other places where there will be issues: where ever some non-default function linking strategy is used.

One such place is direct linked functions, but I suspect for direct linked functions, Clojure/Core will just say you should only instrument code for testing, and you should only turn on direct liking for production.

Another case, which I am sort of surprised we haven't heard more about yet is protocol functions.

Comment by Sean Corfield [ 02/Jun/16 8:35 PM ]

Your comment about direct linking made me wonder about the validity of spec'ing and instrumenting clojure.core functions. The examples show clojure.core/symbol, but Clojure's core library is shipped as direct linked, as of 1.8.0 isn't it?

Comment by Kevin Downey [ 03/Jun/16 3:14 AM ]

what alters the calling convention isn't the function being compiled with direct linking on, but a caller of that function being compiled with direct linking on.

This code will throw a non-conforming error for the bogus symbol spec with direct linking off, and return the symbol foo with direct linking on

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

(s/fdef symbol
  :args string?
  :ret symbol?)

(defn foo
  []
  (symbol 'foo))

(s/instrument-all)

(foo)
Comment by Kevin Downey [ 03/Jun/16 3:26 AM ]

This code returns true because m is a protocol function, if you replace it with a regular function it throws a non-conforming error

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

(defprotocol P
  (m [_]))

(deftype T []
  P
  (m [_] true))

(s/fdef m
  :args (s/cat :p (constantly false))
  :ret string?)

(defn foo
  []
  (m (T.)))

(s/instrument-all)

(foo)
Comment by Alex Miller [ 13/Jun/16 3:53 PM ]

@Sean instrumenting core functions will work for calls from your code into core (which are presumably not direct-linked), but will not affect calls from one core function to another as they are direct-linked and do not go through the var. One thing we've considered for a long while is building a dev version of core that would not be direct-linked and could potentially turn on instrumentation or other helpful dev-time tooling.

Comment by Sean Corfield [ 14/Jun/16 5:48 PM ]

Thanks for that answer @alexmiller – We have dev set to non-direct-linking but QA / production set to direct linking, so I'm only concerned about possible issues in dev with (s/instrumental-all) and wanting to be sure "code won't break". If instrumentation won't affect existing (direct-linked) calls within core, that's good enough for me. I am concerned about primitive hinting and protocols (and whatever crawls out of the woodwork here) since you don't want to be forced to read the source of every library you include, just to see whether (s/instrument-all) is safe or whether it will bite you in some weird way while you're developing.

Comment by Michiel Borkent [ 18/Jan/19 3:24 AM ]

Running into this issue while spec'ing clojure.core/partition-all.





[CLJ-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 17/Jan/19

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

Type: Feature Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: try-catch
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.9.4


Attachments: File ex_info_arity.diff    
Approval: Triaged

 Description   

We often use 'ex-info' to throw a custom exception.But ex-info at least accepts two arguments: a string message and a data map.
In most cases,but we don't need to throw a exception that taken a data map.
So i think we can add a new arity to ex-info:

(ex-info "the exception message")

That created a ExceptionInfo instance carries empty data.

I am not sure it's useful for other people,but it's really useful for our developers.

The patch is attached.



 Comments   
Comment by Alex Miller [ 10/Feb/17 8:46 AM ]

Why "(. clojure.lang.PersistentArrayMap EMPTY)" ? Why not just {}?

Comment by Leon Grapenthin [ 14/Feb/17 1:43 PM ]

I always thought the lack of a one-arity was intentional design to make users use the map argument. Why do you want to throw ExceptionInfos with no data?

Comment by dennis zhuang [ 14/Feb/17 9:53 PM ]

@Alex I forgot why i used EMPTY map here, maybe influenced by the code https://github.com/clojure/clojure/blob/7aad2f7dbb3a66019e5cef3726d52d721e9c60df/src/clj/clojure/core.clj#L4336

@Leon For example, throw an exception when arguments error:

(when-not (integer? c)
(throw (ex-info "Expect number for c."))

We don't need data here.

Comment by Nicola Mometto [ 15/Feb/17 4:27 AM ]

Then why not just throw an `(Exception. "expect number for c")`? I don't see the added value in throwing exinfos w/o data vs just throwing an Exception

Comment by dennis zhuang [ 21/Feb/17 9:13 PM ]

Indeed, it was just a technical decision that we chose to use ex-info for throwing exceptions.

Comment by Jan Krajicek [ 17/Jan/19 5:30 AM ]

This would be a nice way to throw simple exceptions from cljc files that works in both clj and cljs.





[CLJ-2466] java.time.ZonedDateTime is not an inst Created: 10/Jan/19  Updated: 16/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: The Alchemist Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Patch: Code

 Description   

Description

(inst? (java.time.ZonedDateTime/now))
=> false

Seems incorrect, as ZonedDateTime has the necessary information.

Proposed Solution

Conversion between Instant and ZonedDateTime is lossless.

(extend-protocol clojure.core/Inst
  java.time.ZonedDateTime
  (inst-ms* [inst] (.toEpochMilli (.toInstant ^java.time.ZonedDateTime inst))))





[CLJ-2467] ^:const no-field record reports generic class Created: 15/Jan/19  Updated: 16/Jan/19

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

Type: Defect Priority: Minor
Reporter: Maarten Truyens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Approval: Triaged

 Description   

Example:

(defrecord ABC [some-field])
(def ^:const abc (->ABC :some-value))
(println (class abc))

(defrecord XYZ [])
(def ^:const xyz (->XYZ))
(println (class xyz))

results in

"sx.clj.temp.ABC
clojure.lang.PersistentArrayMap"

The XYZ seems to lose its class information, and is apparently stored as a simple map, instead of a record. I could not find the official specification of ^:const, but this behaviour seems incorrect to me, or at least inconsistent.



 Comments   
Comment by Alex Miller [ 16/Jan/19 11:20 PM ]

Possibly related: CLJ-1093, CLJ-1575, CLJ-1460





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

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



 Comments   
Comment by Dennis Schridde [ 08/Jan/19 10:24 AM ]

For reference and in case someone else would also like to know the call order that works (with Clojure 1.10.0 and clojure.spec.alpha 0.2.176):

Unable to find source-code formatter for language: clj. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(require '[clojure.spec.alpha :as s])
; => nil
(require '[clojure.spec.test.alpha :as stest])
; => nil
(defmulti testfn :type)
; => #'user/testfn
(defmethod testfn :atype [m] 1)
; => #object[clojure.lang.MultiFn 0x5d16b612 "clojure.lang.MultiFn@5d16b612"]
(s/fdef testfn :args (s/cat :m (s/keys :req-un [::type ::does-not-exist])))
; => user/testfn
(stest/instrument `testfn)
; => [user/testfn]
(testfn {:type :atype :does-not-exist nil})
; => 1
(testfn {:type :atype})
; => clojure.lang.ExceptionInfo: Call to #'user/testfn did not conform to spec.




[CLJ-2462] gen-class, gen-interface docstrings unclear about "when compiling..." Created: 02/Jan/19  Updated: 03/Jan/19

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

gen-class and gen-interface docstrings begin, "When compiling,...". It is not clear what that means.

After all, when is Clojure not compiling? The clojure.org home page says "Clojure is a compiled language" and the docstrings for unchecked-math, warn-on-reflection, case, and defmacro suggest that compiling is turning forms into bytecode - a nonstop phenomenon.

Elsewhere in clojure core: The defrecord and deftype docstrings refer to "AOT compiling". The compile-files docstring refers to "compiling files", not clear if referring to input files or output files. The docstring of "compile" refers to "classfiles". These may or may not all be related.

If gen-class, gen-interface, compile, and compile-files mean "AOT compiling", then let's make that explicit. If they don't, then let's please make their true meaning clear.



 Comments   
Comment by Alex Miller [ 03/Jan/19 8:14 AM ]

It means "AOT compiling", ie writing class files to disk - *compile-files* is bound to true.





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

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

Type: Defect Priority: Minor
Reporter: Alexander Kiel Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: spec


 Description   

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

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

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



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

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

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

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

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

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

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

Another example from Slack, related to this:

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

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

Workaround:

(if-let [a 1]
  '::s/invalid)
Comment by Ambrose Bonnaire-Sergeant [ 05/Sep/17 3:41 PM ]

Another example from the wild: https://github.com/pjstadig/humane-test-output/pull/23

A macro rewriting

(is (= ::s/invalid ..))

to

(let [a ::s/invalid] ...)

resulted in some very strange errors.

Comment by Alexander Kiel [ 13/Sep/17 6:34 AM ]

The macro issues can be solved by just not using ::s/invalid in code directly. I think in general, it better to use the predicate s/invalid?.

Instead of writing:

(= ::s/invalid ...)

one should use

(s/invalid? ...)

But I have no idea to solve the issue where you have ::s/invalid in data which is validated. The function spec for identical? is a good example.

(s/fdef clojure.core/identical?
  :args (s/cat :x any? :y any?)
  :ret boolean?)

world not work.

Comment by jcr [ 06/Jul/18 9:05 AM ]

Please use sumtypes instead of "magic" values to indicate failure or success. For example,

(s/conform any? ::s/invalid) ;=> [:ok ::s/invalid]
(s/conform int? ::s/invalid) ;=> [:failure #::s{:problems ... :spec ... :value ...}]

Note that the return value should be an instance of clojure.lang.MapEntry, in order for key and val to work on it. However, if it's not desirable to return the explain-map on failure then returning vectors [:ok value] and [:failure] (no second element) would work as well.

Since spec is explicitly in alpha, it's not too late yet to fix the api.

Related: CLJ-2115

Comment by Michiel Borkent [ 30/Dec/18 8:40 AM ]

This is also a problem in tooling REPL like Cursive's. Repro:

(ns assoc.core
  (:require
   [clojure.spec.alpha :as s]
   [clojure.spec.test.alpha :as stest]))

(s/fdef clojure.core/assoc
  :args (s/cat :map (s/nilable associative?)
               :key any? :val any? :kvs (s/* (s/cat :ks any? :vs any?)))
  :ret associative?)

(stest/instrument `assoc)

(s/conform string? 1)

Evaluating this namespace in a Cursive REPL gives:

Error printing return value (ExceptionInfo) at clojure.spec.test.alpha/spec-checking-fn$conform! (alpha.clj:132).
Call to #'clojure.core/assoc did not conform to spec.

The problem can also reproduced without Cursive by evaluating the above and this:

(defn tooling-repl
  ([] (tooling-repl {}))
  ([state]
   (let [l (read-line)
         evaled (eval (read-string l))]
     (prn evaled)
     (recur (assoc state
                   :*1 evaled
                   :*2 (get state :*1))))))

;; now type in the REPL: (s/conform string? 1)
(with-redefs [read-line (constantly "(s/conform string? 1)")]
  (tooling-repl))
Comment by Michiel Borkent [ 30/Dec/18 4:35 PM ]

I modified the ::any spec in speculative as follows:

(s/def ::any
  (s/with-gen
    (s/conformer #(if (s/invalid? %) ::invalid %))
    #(s/gen any?)))




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

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

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

Attachments: Text File prepl-reader-conditionals.patch    
Patch: Code
Approval: Vetted

 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.



 Comments   
Comment by Oliver Caldwell [ 19/Dec/18 3:39 AM ]

Enables reader conditionals in Clojure prepl.





[CLJ-2461] [core.specs] refer-clojure and import specs are too limited in quote support Created: 30/Dec/18  Updated: 30/Dec/18

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

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

Approval: Triaged

 Description   

The refer-clojure spec added in CLJ-2062 supports only limited quoting.

In Clojure 1.8, these are both allowed:

user=> (in-ns 'myns)
#object[clojure.lang.Namespace 0x579d011c "myns"]
myns=> (clojure.core/refer-clojure :only '[+ -])
nil
myns=> (clojure.core/refer-clojure :only ['+ '-])
nil

But in 1.9/1.10, the first is valid but the second will throw:

Clojure 1.10.0
user=> (in-ns 'myns)
#object[clojure.lang.Namespace 0x61e45f87 "myns"]
myns=> (clojure.core/refer-clojure :only '[+ -])
nil
myns=> (clojure.core/refer-clojure :only ['+ '-])
Syntax error macroexpanding clojure.core/refer-clojure at (REPL:1:1).
(quote +) - failed: #{(quote quote)} at: [:only :arg :quoted-spec :quote]
(quote +) - failed: simple-symbol? at: [:only :arg :spec] spec: :clojure.core.specs.alpha/only
(quote -) - failed: simple-symbol? at: [:only :arg :spec] spec: :clojure.core.specs.alpha/only

Cause: This is due to the way the quoted spec is defined, allowing only a quoted list rather than a quoted list of symbols or a list of quoted symbols. A similar issue is seen with import - (import ['java.util 'Date]) fails in a similar way, but it's uncommon for people to invoke import by quoting the internal symbols rather than the outer list (import '[java.util Date]).

Proposed: To match support in 1.8, would need specs that include both quoted lists of symbols and lists of quoted symbols.



 Comments   
Comment by Alex Miller [ 30/Dec/18 8:57 AM ]

Due to the rarity of this usage pattern (this is the first report in over 2 years since this was added), I'm going to mark this minor priority.





[CLJ-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 19/Dec/18

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: errormsgs

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch     Text File CLJ-1473_v02.patch     Text File CLJ-1473_v03.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)

Patch: CLJ-1473_v03.patch
Screened by: Alex Miller



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

Would be nice to include the bad condition in the error (maybe via ex-info?) and also have tests.

Comment by Brandon Bloom [ 03/May/15 12:11 PM ]

New patch includes tests. Unfortunately, can't call ex-info directly due to bootstrapping concerns. Instead, just calls ExceptionInfo constructor directly.

Comment by Alex Miller [ 04/May/15 9:41 AM ]

Bug in the reporting: {:post pre} should be {:post post}.

Test should be improved as it could have caught that.

Comment by Brandon Bloom [ 04/May/15 7:25 PM ]

Good catch with the pre/post copy/paste screw up. Didn't enhance the test though, since that would involve creating an ex-info friendly variant of fails-with-cause

Comment by Rich Hickey [ 09/Oct/15 7:32 AM ]

:pre and :post don't require vectors, just collections

Comment by Andy Fingerhut [ 15/Nov/15 2:39 PM ]

Eastwood 0.2.2, released on Nov 15 2015, will warn about several kinds of incorrect pre and postconditions. See https://github.com/jonase/eastwood#wrong-pre-post

The Eastwood documentation may be misleading right now, in that it says that :pre and :post should be vectors, which is at odds with Rich's comment of Oct 9 2015. Corrections to Eastwood's documentation here are welcome. I guess Rich's intent is that :pre and :post could be vectors, lists, or sets? Would a map ever make sense there?

Comment by Marco Molteni [ 31/Jan/18 10:58 AM ]

Hello any news ?

Comment by Alex Miller [ 02/Aug/18 2:49 PM ]

There's feedback above from Rich that has never been addressed here. I took a stab at an update, but relaxing the constraint to just "collections of expressions" is kind of interesting. Once you do that, then the first example in the ticket here {:pre (pos? x)} is actually not invalid, it's a collection of two things: pos? and x. pos? evaluates to true (it's a function) and x evaluates to true for any number.

I'm not sure what the way forward is on that. It's perfectly valid and even potentially useful to refer to an incoming arg as a precondition, which would just be a symbol:

( (fn [v] {:pre [v]} v) nil)  ;; precondition should fail
( (fn [v] {:pre (v)} v) nil)  ;; equally valid

Maybe you could check for whether the elements of pre or post are either lists (expressions) or symbols that are arg bindings? Anything else is vacuously true and probably a bug. Not sure.

Comment by Andy Fingerhut [ 03/Aug/18 4:05 AM ]

Eastwood still gives warnings about code like the following:

(defn foo [x]
  {:pre [pos? x]}
  (inc x))

Here is sample output:

src/filename.clj:5:22: wrong-pre-post: Precondition found that is probably always logical true or always logical false.  Should be changed to function call?  pos?

It also still warns if the value of :pre or :post is not a vector, despite Rich's comment, so it is overly strict in that sense, but maybe not so bad to be overly strict there.

Comment by Martin Klepsch [ 19/Dec/18 8:29 AM ]

Just want to point to this PR (https://github.com/pedestal/pedestal/pull/544) as an example of how this affects production Clojure code.

Due to malformed pre/post conditions silently being accepted the API was essentially different from what the library authors intended. People started to rely on this and fixing the broken precondition essentially resulted in a breaking change.





[CLJ-2303] Reified object doesn't satisfy protocol at compile time Created: 29/Dec/17  Updated: 17/Dec/18

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

Type: Defect Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

https://github.com/MichaelBlume/reify-fail

(defprotocol Foo
  (hello [this] "Says hello"))

(def bar
  (reify Foo
    (hello [this] "hello, world")))

(when-not (satisfies? Foo bar)
  (throw (Exception. "bar doesn't satisfy Foo")))

Notes:

Project is AOT
Another namespace requires this namespace. That namespace is compiled first. Without that namespace the problem goes away.



 Comments   
Comment by Kevin Downey [ 01/Jan/18 5:15 PM ]

what steps do you take after checking out the repo to reproduce this issue?

I cloned the repo and ran `lein compile` and didn't get any errors.

Comment by Michael Blume [ 01/Jan/18 10:56 PM ]

I was reproducing the issue with `lein check`, but `lein compile` also fails for me, so I'm confused.

Comment by Michael Blume [ 01/Jan/18 10:56 PM ]

I think it's important that the near-empty namespace compiles first, maybe there are platform differences in compilation order?

Comment by Michael Blume [ 01/Jan/18 10:57 PM ]

test repo fails for me under Mac OS and Ubuntu, both with Java 8

Comment by Michael Blume [ 01/Jan/18 10:58 PM ]

Under Ubuntu I have

$ lein -v
Leiningen 2.7.1 on Java 1.8.0_151 OpenJDK 64-Bit Server VM

Comment by Andy Fingerhut [ 02/Jan/18 12:19 AM ]

On both of these OS’s: Ubuntu 16.04.3 Linux, macOS 10.12.6

With both of these versions of Leiningen: 2.7.1, 2.8.1

I believe all of my experiments were using a recent version of JDK 1.8

With any of these Clojure versions: 1.7.0 1.8.0 1.9.0

I get an error with both the commands 'lein check' and 'lein compile' with the project https://github.com/MichaelBlume/reify-fail, but only if I do '/bin/rm -fr target' first (or run it before the target directory has ever been created, e.g. immediately after running the ‘git clone’ command for the repository).

If I run the command twice, the first command creates many .class files in the target directory, and the second such command gives no error.

Comment by Kevin Downey [ 02/Jan/18 12:36 AM ]

I am able to reproduce there error if I replace `:aot :all` with `:aot [com.lendup.citadel.pipeline com.lendup.citadel.providers.mocky]`. I suspect lein could fix this by doing their stale ns check between compiling namespaces to avoid repeated compilation of transitive namespaces.

Comment by Kevin Downey [ 02/Jan/18 12:41 AM ]

could be related to https://github.com/technomancy/leiningen/issues/2316

Comment by Tommi Reiman [ 16/Dec/18 10:23 AM ]

related: https://github.com/technomancy/leiningen/issues/2508.

Comment by Nicola Mometto [ 17/Dec/18 8:29 AM ]

Is this a clojure or Lein issue then? Can we reproduce this using bare `clj`?

Comment by Alex Miller [ 17/Dec/18 8:47 AM ]

I suspect you could reproduce it by compiling things in a certain order, but I have not tried to do so. But the resulting answer might be: compile things in the right order.





[CLJ-2457] Document return value of Throwable->map Created: 17/Dec/18  Updated: 17/Dec/18

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

Type: Enhancement Priority: Minor
Reporter: Marc O'Morain Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Vetted

 Description   

The docstring for Throwable->map does not explain any of the details of what is returned.

The shape of the map should be described: What keys are always present? Which keys are optional, and when will optional keys be present? What are the values?

A spec would also be helpful.






[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: 1
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-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-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: 4
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-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: 13
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-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: 4
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: 4
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-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-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