<< Back to previous view

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

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

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

Approval: Triaged

 Description   

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

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

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

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

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

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



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

Probably similar to CLJ-700.





[ASYNC-169] Try/catch block loops eternally inside go block Created: 16/May/16  Updated: 17/Jun/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Critical
Reporter: Frank Burnham Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.7.0; core.async 0.2.374; java 1.8


Attachments: Text File 0001-overhaul-exceptions-in-clojure-go-macro-ASYNC-169.patch    
Patch: Code
Approval: Triaged

 Description   
(a/<!!
    (a/go-loop []
      (try
        (a/<! (throw (Exception. "Ex")))
        (catch clojure.lang.ExceptionInfo ei
          :retry))))

The above example appears to loop forever when I expected it to return nil.

(a/<!!
    (a/go-loop []
      (try
        (throw (Exception. "Ex"))
        (catch clojure.lang.ExceptionInfo ei
          :retry))))

This example returns nil.



 Comments   
Comment by Kevin Downey [ 16/May/16 3:50 PM ]

it looks like maybe "process-exception" in clojure.core.async.impl.ioc-macros is missing an :else (throw exception) clause at the end.

Comment by Frank Burnham [ 17/May/16 7:12 AM ]
user> (a/<!!
       (a/go-loop []
                  (try
                     (a/<! (throw (Exception. "Ex")))
                   (catch clojure.lang.ExceptionInfo ei
                          :retry) 
                   (catch Throwable t :drop))))
nil
user>

That example is also odd to me. I expected to see :drop returned.

Comment by Kevin Downey [ 17/May/16 12:07 PM ]

looking at the macro expansion for the two catches, the emitted code is only setting up an exception frame for the ExceptionInfo case, and not for the Throwable case.

I suspect the exception handling code is all sawdust and broken promises.

Comment by Kevin Downey [ 17/May/16 12:12 PM ]

this looks like it is ignoring everything except the first catch https://github.com/clojure/core.async/blob/master/src/main/clojure/clojure/core/async/impl/ioc_macros.clj#L709

Comment by Kevin Downey [ 17/May/16 7:33 PM ]

this patch seems to make everything work, needs more tests of course

Comment by Kevin Downey [ 17/May/16 7:55 PM ]

looks like this might be the same issue as http://dev.clojure.org/jira/browse/ASYNC-100, which also has a patch attached





[ASYNC-159] promise-chan in ClojureScript is broken Created: 13/Feb/16  Updated: 17/Jun/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Critical
Reporter: Rangel Spasov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Google Chrome 48.0.2564.109 (64-bit) Mac OS X El Capitan 10.11


Approval: Triaged

 Description   

promise-chan in ClojureScript does not seem to work. In Clojure on the JVM, the code below produces the expected result.

Example:

(ns hush-vendor-cljs.example
(:require-macros [cljs.core.async.macros :refer [go]])
(:require [cljs.core.async :refer [promise-chan <! >!]]))

(def p (promise-chan))

(def go-1 (go (let [r (<! p)] (println "got value on promise-chan::" r))))

(def go-2 (go (let [r (<! p)] (println "got value on promise-chan::" r))))

(go (>! p 1))
;This prints only once in ClojureScript, should be twice. It works on JVM Clojure (prints twice).
;=> got value on promise-chan:: 1



 Comments   
Comment by Alex Miller [ 24/Mar/16 11:02 AM ]

Can you update with what version of core.async you're using?

Comment by Johannes Gustafsson [ 24/Mar/16 12:51 PM ]

I can confirm the bug using:

Mac OS X El Capitan
Chrome 49.0.2623.87 (64-bit)
core.async 0.2.374

I created an empty figwheel project using lein new figwheel.

In the REPL, I evaluate each expression one by one. When I evaluate the last expression, only one row is printed. If I evaluate the last expressione one more time, then the second print shows up.

Comment by Johannes Gustafsson [ 24/Mar/16 12:54 PM ]

I get the same result in Firefox

Comment by Rangel Spasov [ 24/Mar/16 1:54 PM ]

Alex - This is under 0.2.374. Sorry for missing the version, this is my first bug report .

Comment by Marcin Kulik [ 18/Apr/16 6:40 AM ]

I am experiencing the same issue. This prints only once:

(let [p (promise-chan)]
  (go
    (<! (timeout 500))
    (println (<! p)))
  (go
    (<! (timeout 500))
    (println (<! p)))
  (go
    (<! (timeout 1000))
    (>! p :yup)))

While this prints twice as expected:

(let [p (promise-chan)]
  (go
    (<! (timeout 1500))
    (println (<! p)))
  (go
    (<! (timeout 1500))
    (println (<! p)))
  (go
    (<! (timeout 1000))
    (>! p :yup)))

I am under 0.2.374.





[ASYNC-138] Go blocks leak memory Created: 10/Aug/15  Updated: 24/Mar/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Critical
Reporter: Brian Lubeski Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: memory
Environment:

clojure 1.7.0
core.async 0.1.346.0-17112a-alpha
Java HotSpot(TM) Client VM 1.8.0_31-b13


Attachments: Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2-nested.patch     Text File 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2.patch    
Patch: Code
Approval: Triaged

 Description   

The following example, after running for a few minutes, generates an OutOfMemoryError.

(let [c (chan)]
  (go (while (<! c)))
  (let [vs (range)]
    (go
      (doseq [v vs]
        (>! c v)))))

By contrast, the following example will run indefinitely without causing an OutOfMemoryError.

(let [c (chan)]
  (go (while (<! c)))
  (go
    (let [vs (range)] 
      (doseq [v vs]
        (>! c v)))))

The only significant difference I see between the two examples is that the (range) is created outside the go block in the first example but is created inside the go block in the second example. It appears that the go block in the first example is referencing vs in such a way as to prevent if from being garbage-collected.

This behavior might also be the cause of ASYNC-32.

Patch: 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2.patch



 Comments   
Comment by Nicola Mometto [ 15/Dec/15 9:34 AM ]

attached is a WIP patch to fix this issue, would appreciate some testing on this

Comment by Nicola Mometto [ 15/Dec/15 5:52 PM ]

CLJ-1744 contributes to this bug

Comment by Nicola Mometto [ 15/Dec/15 6:23 PM ]

Updated patch to preserve type info

Comment by Nicola Mometto [ 17/Dec/15 10:16 AM ]

Note that the current patch breaks nested `go` blocks at compile time since the nested `go` block will be macroexpanded by tools.analyzer.jvm rather than Compiler.java and this &env will be different.

Is this something we want to support? There seem to be a lot of other cases that break when nesting `go` blocks.

If this is something that we do want to support 0001-ASYNC-138-allow-for-clearing-of-closed-over-locals-v2-nested.patch fixes this

Comment by Jan Rychter [ 21/Dec/15 6:18 AM ]

This just bit me badly when doing data processing work — I never expected that (async/to-chan (line-seq rdr)) can be the culprit of my heap getting exhausted in 30 seconds. Took quite some time to narrow it down. Thanks to Nicola for letting me know about this bug on Slack.





[CLJ-1968] clojure.test/report :error does not flush *out* when the test fails with an exception Created: 23/Jun/16  Updated: 23/Jun/16

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

Type: Defect Priority: Major
Reporter: Sam Roberton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

Approval: Triaged

 Description   

Minimal reproduction:

(require 'clojure.test)

(clojure.test/deftest foo-test
  (throw (ex-info "I fail" {})))

(clojure.test/deftest bar-test
  (.println System/out "bar"))

(clojure.test/test-vars [#'foo-test #'bar-test])

Result:

ERROR in (foo-test) (core.clj:4617)
Uncaught exception, not in assertion.
expected: nil
bar
  actual: clojure.lang.ExceptionInfo: I fail
 at clojure.core$ex_info.invokeStatic (core.clj:4617)
...

Note "bar" appearing in the output in the middle of the error report for foo-test.

Analysis:

(clojure.test/report {:type :error, :actual some-exception}) calls stack/print-cause-trace. Unlike other clojure.test/report callpaths, this does not flush on newline. Thus, when tests fail with exceptions and there is anything writing directly to Java's System.out, there can be a large gap between the first part of the error report and the exception trace.

(To explain why this is annoying: we're running Selenium tests via clj-webdriver, and our system under test is logging with log4j via clojure.tools.logging. We invariably see dozens or even hundreds of lines between "expected: ..." and the subsequent "actual: ..." exception trace. This makes it very easy to come to completely the wrong conclusion about when failures occurred with respect to the other events that appear interleaved in the log.)

It would be preferable (in my opinion) if clojure.test/report always constructed the output from each individual invocation into a single string which got written to *out* all at once – that way there could be no way for output to be interleaved from other threads. Absent that, it would at least help a lot if the :error implementation called (flush).






[CLJ-1929] Can't typehint literal collection to avoid reflection on Java interop call Created: 16/May/16  Updated: 18/May/16

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

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

OS X 10.11.4


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

 Description   

There is a reflection warning when passing a Clojure collection to a method that has a parameter of a collections interface type like java.util.Map.

Example calling java.time.format.DateTimeFormatterBuilder.appendText(java.time.temporal.TemporalField, java.util.Map):

(import 'java.time.format.DateTimeFormatterBuilder
        'java.time.format.TextStyle
        'java.time.temporal.ChronoField)

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

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR {}))
; Reflection warning, NO_SOURCE_PATH:6:3 - call to method appendText on java.time.format.DateTimeFormatterBuilder can't be resolved (argument types: java.time.temporal.ChronoField, clojure.lang.IPersistentMap).

The map literal cannot be hinted:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR ^java.util.Map {}))
; Reflection warning, NO_SOURCE_PATH:8:3 - call to method appendText on java.time.format.DateTimeFormatterBuilder can't be resolved (argument types: java.time.temporal.ChronoField, clojure.lang.IPersistentMap).

The warning does not appear when the map is not empty:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR {1 "a"}))

Nor does it appear on similar methods where there is no overloaded method with the same arity:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendZoneText builder TextStyle/FULL #{}))

Workaround is to not use a literal:

(let [builder (DateTimeFormatterBuilder.)]
  (.appendText builder ChronoField/YEAR ^java.util.Map (array-map)))

It should be possible to infer in these cases like elsewhere that {} implements java.util.Map.

If that is not viable a type hint on {} should be honored.

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






[CLJ-1898] Inconsistent duplicate check in set/map literals with quoted/unquoted equal constants Created: 06/Mar/16  Updated: 06/Mar/16

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

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

Approval: Triaged

 Description   

Set and map literals containing the same constant quoted and unquoted, will throw a duplicate key exception in some cases (the correct behaviour), while silently ignore the duplicate in some others.

user=> #{'1 1}
#{1}
user=> #{'[] []}
IllegalArgumentException Duplicate key: []  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

This happens because the compiler assumes that literals that have distinct elements at read-time, will have distinct elements at runtime. This is not true for self-evaluating elements where (quote x) is equal to x






[CLJ-1879] reduce-kv on a PHMs doesn't consistently execute the intended fastpath Created: 09/Jan/16  Updated: 11/Jan/16

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

Type: Defect Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

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

 Description   

https://github.com/clojure/clojure/blob/010864f/src/clj/clojure/core.clj#L6553-L6562

Because PHMs implement clojure.lang.IKVReduce and IPersistentMap, they have nondeterministic dispatch through the protocol that backs reduce-kv (clojure.core.protocols/IKVReduce).

A potential way to solve this is to add an instance check for clojure.lang.IKVReduce inside `reduce-kv` (This is similar to how reduce checks for IReduceInit)



 Comments   
Comment by Nicola Mometto [ 11/Jan/16 9:23 AM ]

CLJ-1807 offers a generic solution for this class of problems





[CLJ-1864] clojure.core/proxy does not work when reloading namespaces Created: 06/Dec/15  Updated: 08/Dec/15

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

Type: Defect Priority: Major
Reporter: Ralf Schmitt Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: protocols, proxy
Environment:

tested on 64 bit linux, oracle jdk 1.8


Attachments: Text File clj-1864.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.core/proxy does not work when one reloads namespace containing defprotocol.

E.g. one can't reload the following file without triggering an error:

(ns foo.baz)

(defprotocol Hello
  (hello [this]))

(def hello-proxy
  (proxy [foo.baz.Hello] []
    (hello []
      (println "hello world"))))

(hello hello-proxy)

Saving the above as foo/baz.clj, I get the following error:

$ rlwrap java -cp target/clojure-1.8.0-master-SNAPSHOT.jar:. clojure.main
Clojure 1.8.0-master-SNAPSHOT
user=> (require 'foo.baz :reload)
hello world
nil
user=> (require 'foo.baz :reload)
CompilerException java.lang.IllegalArgumentException: No implementation of method: :hello of protocol: #'foo.baz/Hello found for class: foo.baz.proxy$java.lang.Object$Hello$6f95b989, compiling:(foo/baz.clj:11:1) 

I'm using the current git master (commit 5cfe5111ccb5afec4f9c73), but clojure 1.7 has the same problem.

The problem is that proxy-name only uses the interface names as a key. These names do not change when reloading the namespace, but the interfaces themself are new.

I'm going to attach a short patch which fixes that issue for me.



 Comments   
Comment by Ralf Schmitt [ 06/Dec/15 11:45 AM ]

I'm not sure how this interacts with AOT compilation.





[CLJ-1770] atom watchers are not atomic with respect to reset! Created: 29/Jun/15  Updated: 31/Jul/15

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

Type: Defect Priority: Major
Reporter: Eric Normand Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: atom

Attachments: Text File atom-reset-atomic-watch-2015-06-30.patch     File timingtest.clj    
Patch: Code and Test
Approval: Triaged

 Description   

It is possible that two threads calling `reset!` on an atom can interleave, causing the corresponding watches to be called with the same old value but different new values. This contradicts the guarantee that atoms update atomically.

(defn reset-test []
  (let [my-atom (atom :start
                      :validator (fn [x] (Thread/sleep 100) true))
        watch-results (atom [])]
    (add-watch my-atom :watcher (fn [k a o n] (swap! watch-results conj [o n])))
  
    (future (reset! my-atom :next))
    (future (reset! my-atom :next))
    (Thread/sleep 500)
    @watch-results))

(reset-test)

Yields [[:start :next] [:start :next]]. Similar behavior can be observed when mixing reset! and swap!.

Expected behavior

Under atomic circumstances, (reset-test) should yield [[:start :next] [:next :next]]. This would "serialize" the resets and give more accurate information to the watches. This is the same behavior one would achieve by using (swap! my-atom (constantly :next)).

(defn swap-test []
  (let [my-atom (atom :start
                      :validator (fn [x] (Thread/sleep 100) true))
        watch-results (atom [])]
    (add-watch my-atom :watcher (fn [k a o n] (swap! watch-results conj [o n])))
  
    (future (swap! my-atom (constantly :next)))
    (future (swap! my-atom (constantly :next)))
    (Thread/sleep 500)
    @watch-results))

(swap-test)

Yields [[:start :next] [:next :next]]. The principle of least surprise suggests that these two functions should yield similar output.

Alternative expected behavior

It could be that atoms and reset! do not guarantee serialized updates with respect to calls to watches. In this case, it would be prudent to note this in the docstring for atom.

Analysis

The code for Atom.reset non-atomically reads and sets the internal AtomicReference. This allows for multiple threads to interleave the gets and sets, resulting in holding a stale value when notifying watches. Note that this should not affect the new value, just the old value.

Approach

Inside Atom.reset(), validation should happen first, then a loop calling compareAndSet on the internal state (similar to how it is implemented in swap()) should run until compareAndSet returns true. Note that this is still faster than the swap! constantly pattern shown above, since it only validates once and the tighter loop should have fewer interleavings. But it has the same watch behavior.

public Object reset(Object newval){
    validate(newval);
    for(;;)
        {
            Object oldval = state.get();
            if(state.compareAndSet(oldval, newval))
                {
                    notifyWatches(oldval, newval);
                    return newval;
                }
        }
}


 Comments   
Comment by Eric Normand [ 30/Jun/15 9:24 AM ]

I've made a test just to back up the timing claims I made above. If you run the file timingtest.clj, it will run code with both reset! and swap! constantly, with a validator that sleeps for 10ms. In both cases, it will print out the number of uniques (should be equal to number of reset!s, in this case 1000) and the time (using clojure.core/time). The timing numbers are relative to the machine, so should not be taken as absolutes. Instead, the ratio between them is what's important.

Run with: java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main timingtest.clj

Results

Existing implementation:

"Elapsed time: 1265.228 msecs"
Uniques with reset!: 140
"Elapsed time: 11609.686 msecs"
Uniques with swap!: 1000
"Elapsed time: 7010.132 msecs"
Uniques with swap! and reset!: 628

Note that the behaviors differ: swap! serializes the watchers, reset! does not (# of uniques).

Suggested implementation:

"Elapsed time: 1268.778 msecs"
Uniques with reset!: 1000
"Elapsed time: 11716.678 msecs"
Uniques with swap!: 1000
"Elapsed time: 7015.994 msecs"
Uniques with swap! and reset!: 1000

Same tests being run. This time, they both serialize watchers. Also, the timing has not changed significantly.

Comment by Eric Normand [ 30/Jun/15 10:16 AM ]

Adding atom-reset-atomic-watch-2015-06-30.patch. Includes test and implementation.





[CLJ-1279] Fix confusing macroexpand1 ArityException handling Created: 16/Oct/13  Updated: 24/Mar/16

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

Type: Defect Priority: Major
Reporter: Alex Coventry Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: Compiler, errormsgs, macro

Attachments: Text File 0001-Edit-macro-ArityException-in-AFn.patch     Text File 0001-Fix-macroexpand1-s-handling-of-ArityException.patch    
Patch: Code and Test
Approval: Triaged

 Description   

macros can give very confusing error messages when they execute a form which generates an ArityException. clojure.lang.Compiler.macroexpand1 assumes that any ArityException comes from the call to the macro itself, which need not be the case. For instance:

user> (do (defmacro f [] (assoc)) (f))
  ArityException Wrong number of args (-2) passed to: core$assoc  clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
  user> (use 'clojure.repl) (pst)
  nil
  ArityException Wrong number of args (-2) passed to: core$assoc
  	clojure.lang.Compiler.macroexpand1 (Compiler.java:6488)
  	clojure.lang.Compiler.macroexpand (Compiler.java:6544)
  	clojure.lang.Compiler.eval (Compiler.java:6618)
  	clojure.lang.Compiler.eval (Compiler.java:6624)
  	clojure.lang.Compiler.eval (Compiler.java:6597)
  	clojure.core/eval (core.clj:2864)
  	clojure.main/repl/read-eval-print--6596/fn--6599 (main.clj:260)
  	clojure.main/repl/read-eval-print--6596 (main.clj:260)
  	clojure.main/repl/fn--6605 (main.clj:278)
  	clojure.main/repl (main.clj:278)
  	clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--1251 (interruptible_eval.clj:56)
  	clojure.core/apply (core.clj:617)
  nil

Easy enough to see the source of the problem in this case, but because both the number of arguments actually passed is off by two, and the stacktrace element for the call to assoc has been dropped, this shortcut by macroexpand1 can get super confusing.

The attached patch corrects this behavior. E.g.

user=> (do (defmacro f [] (assoc)) (f))
  ArityException Wrong number of args (0) passed to: core$assoc  clojure.lang.AFn.throwArity (AFn.java:437)
  user=> (use 'clojure.repl) (pst)
  nil
  ArityException Wrong number of args (0) passed to: core$assoc
  	user/f (NO_SOURCE_FILE:1)
  	clojure.lang.Var.invoke (Var.java:419)
  	clojure.lang.Var.applyTo (Var.java:532)
  	clojure.lang.Compiler.macroexpand1 (Compiler.java:6507)
  	clojure.lang.Compiler.macroexpand (Compiler.java:6580)
  	clojure.lang.Compiler.eval (Compiler.java:6654)
  	clojure.lang.Compiler.eval (Compiler.java:6660)
  	clojure.lang.Compiler.eval (Compiler.java:6633)
  	clojure.core/eval (core.clj:2864)
  	clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
  	clojure.main/repl/read-eval-print--6594 (main.clj:260)
  	clojure.main/repl/fn--6603 (main.clj:278)
  nil


 Comments   
Comment by Alex Coventry [ 17/Oct/13 11:01 AM ]

Patch with test

Comment by Alex Coventry [ 23/Oct/13 11:42 PM ]

Amended patch to deal more gracefully with unexpected stack trace structure.

Comment by Alex Miller [ 24/Oct/13 12:09 AM ]

Also see CLJ-397 and CLJ-383.

Comment by Alex Coventry [ 24/Oct/13 2:46 PM ]

Thanks, Alex. It would be easy enough to move most of the logic into ArityException, which would be a compromise between Stu's[1] options 1 and 2. Is that worth doing?

Amending clojure.lang.AFn.throwArity to check whether "this" is a macro and adjust the arg count there accordingly might be the simplest way. I can see why Rich prefers all the logic to go into ArityException, but since ArityExceptions are used for things other than macros, I don't see a way to make an honest error message there without groveling the stack trace.

[1] http://dev.clojure.org/jira/browse/CLJ-397?focusedCommentId=24090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-24090

Comment by Alex Miller [ 24/Oct/13 2:57 PM ]

I would have to take more time than I have to make an informed opinion but I can say that from a general point of view inspecting StackTraceElements does not seem like the right solution to (almost) any problem.

Comment by Alex Coventry [ 24/Oct/13 10:26 PM ]

This patch causes Var.setMacro to set instance attribute AFn.macrop to true, so that AFn.throwArity can reduce the number of arguments reported.

I'm not used to negotiating java class hierarchies, so it's possible there's a cleaner way. Since Var.fn() returns an IFn, I added macrop handling methods IFn.setMacro and IFn.isMacro. These then needed to be implemented in Ref and Keyword, as well as AFn (where I wanted them) because they implement the IFn interface but don't inherit from AFn.

The real drawback I see with this approach is the duplicated state, though: ^{:macro true} vs AFn.macrop==true.

Comment by Andy Fingerhut [ 25/Oct/13 6:33 PM ]

I have not investigated the reason yet, but neither patch applies cleanly after the latest commits to Clojure master on Oct 25 2013. Given that what kinds of solution methods would be acceptable for this issue, it sounds like more thinking and code changes are probably needed anyway before worrying too much about that.





[ASYNC-170] binding in go block causes thread binding error: "Pop without matching push" Created: 26/May/16  Updated: 17/Jun/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Christian Weilbach Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.8.0, core.async 0.2.374


Approval: Triaged

 Description   

Reproduced in https://github.com/whilo/async-binding.

(def ^:dynamic *foo* nil)

(defn -main [& args]
  (go
    (binding [*foo* nil]
      (<! (go 42))
      (println "done.")))

  (Thread/sleep 30000))

$ lein uberjar
$ java -jar ...

done.
Exception in thread "async-dispatch-3" java.lang.IllegalStateException: Pop without matching push
	at clojure.lang.Var.popThreadBindings(Var.java:331)
	at clojure.core$pop_thread_bindings.invokeStatic(core.clj:1839)
	at clojure.core$pop_thread_bindings.invoke(core.clj:1839)
	at async_binding.core$_main$fn__6354$state_machine__4495__auto____6355$fn__6357.invoke(core.clj:8)
	at async_binding.core$_main$fn__6354$state_machine__4495__auto____6355.invoke(core.clj:8)
	at clojure.core.async.impl.ioc_macros$run_state_machine.invokeStatic(ioc_macros.clj:1011)
	at clojure.core.async.impl.ioc_macros$run_state_machine.invoke(ioc_macros.clj:1010)
	at clojure.core.async.impl.ioc_macros$run_state_machine_wrapped.invokeStatic(ioc_macros.clj:1015)
	at clojure.core.async.impl.ioc_macros$run_state_machine_wrapped.invoke(ioc_macros.clj:1013)
	at clojure.core.async.impl.ioc_macros$take_BANG_$fn__4511.invoke(ioc_macros.clj:1024)
	at clojure.core.async.impl.channels.ManyToManyChannel$fn__313$fn__314.invoke(channels.clj:95)
	at clojure.lang.AFn.run(AFn.java:22)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
	at java.lang.Thread.run(Thread.java:745)


 Comments   
Comment by Kevin Downey [ 27/May/16 1:46 AM ]

this issue only reproduces when the code is aot compiled

Comment by Christian Weilbach [ 27/May/16 6:55 AM ]

Right, I missed to add the AOT condition here, only in the repository. I cannot update the issues myself, right? I tried also to close another issue I opened, but couldn't find a way to close it.





[ASYNC-165] the binding macro/let binding is wrongly inlined Created: 06/Apr/16  Updated: 17/Jun/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Christian Weilbach Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

[org.clojure/clojure "1.7.0"]
[org.clojure/clojurescript "1.8.34"]
[org.clojure/core.async "0.2.374"]


Attachments: Text File 0001-ASYNC-165-if-a-local-aliases-a-global-actually-do-th.patch     Text File 0002-ASYNC-165-if-a-local-aliases-a-global-actually-do-th.patch     Text File 0003-ASYNC-165-if-a-local-aliases-a-global-actually-do-th.patch    
Patch: Code
Approval: Triaged

 Description   

(def ^:dynamic foo 42)

(go
(let [old foo]
(set! foo 45)
(println old foo)
(set! foo old)))

leaves the binding with the value 45 (same code as with binding+with-redefs). The problem is that the let binding is somehow inlining the reference to foo. For instance the println statement compiles to:

...
var inst_43089 = cljs.core.println.call(null,full.async.foo,full.async.foo);
...

I am currently having a look at ioc_macros.clj, but I couldn't find the problematic part yet. Any hints are helpful.



 Comments   
Comment by Kevin Downey [ 06/Apr/16 6:21 PM ]

for the curious the cleaned up macro expansion of the above (in clojurescript) is:

(let* [c__9201__auto__ (chan 1)]
      (run
        (fn []
          (let [f__9202__auto__ (let [switch__9186__auto__ (fn [state_9258]
                                                             (let [state_val_9259 (aget state_9258 1)]
                                                               (cond
                                                                (== state_val_9259 1) (let [inst_9254 (set! foo 45)
                                                                                            inst_9255 (println foo foo)
                                                                                            inst_9256 (set! foo foo)
                                                                                            state_9258 (aset-all! state_9258 7 inst_9255 8 inst_9254)]
                                                                                        (return-chan state_9258 inst_9256)))))]
                                  (fn state-machine__9187__auto__
                                    ([] (aset-all! (make-array 9) 0 state-machine__9187__auto__ 1 1))
                                    ([state_9258] (let [ret-value__9188__auto__ (try
                                                                                  (loop []
                                                                                    (let [result__9189__auto__ (switch__9186__auto__ state_9258)]
                                                                                      (if (keyword-identical? result__9189__auto__ :recur)
                                                                                        (recur)
                                                                                        result__9189__auto__)))
                                                                                  (catch js/Object ex__9190__auto__
                                                                                    (aset-all! state_9258 5 ex__9190__auto__)
                                                                                    (process-exception state_9258)
                                                                                    :recur))]
                                                    (if (keyword-identical? ret-value__9188__auto__ :recur)
                                                      (recur state_9258)
                                                      ret-value__9188__auto__)))))
                state__9203__auto__ (-> (f__9202__auto__)
                                        (aset-all! USER-START-IDX c__9201__auto__))]
            (run-state-machine-wrapped state__9203__auto__))))
      c__9201__auto__)

the issue is definitely present in the macro expansion

Comment by Kevin Downey [ 06/Apr/16 7:00 PM ]

it looks like the issue is, let binds essentially disappear at runtime
because the ioc macros bind every expression to a name, so let binds
are just mapped to those names at compile time. in that mapping global
names essentially map to themselves, so let bound names that get their
value from a global just disappear and become references to the
global.

If you look at the `:symbol` case for `-item-to-ssa` in
ioc_macros.clj, there is a commented out `(add-instruction (->Const
x))`, if you uncomment out that and comment out the fn above it, I
think what you get out has the behavior you are looking for, at the
cost of creating a local for every global read.

You could do some kind of picking of the behavior based on if the
globals are declared to be dynamic, but that would still leave issues
for with-redefs. In JVM clojure with-redefs works on global names
regardless of if they are dynamic or not. You could generate some kind
of dirty set if you see a set! of a global, and use that to toggle the
behavior, but I think (not sure) that could run in to issues because
the analysis is local in scope.

Comment by Kevin Downey [ 06/Apr/16 7:04 PM ]

if you make the change I mentioned in my comment above, the relevant section of the cleaned up macro expansion looks like:

(let [inst_9257 foo
      inst_9258 foo
      inst_9259 (set! inst_9258 45)
      inst_9260 println
      inst_9261 foo
      inst_9262 (inst_9260 inst_9257 inst_9261)
      inst_9263 foo
      inst_9264 (set! inst_9263 inst_9257)
      state_9266 (aset-all! state_9266 7 inst_9259 8 inst_9262)]
  (return-chan state_9266 inst_9264))

that actually has a bug too, it turns out, because the set! is changing the value of a local instead of the global `foo`

Comment by Kevin Downey [ 06/Apr/16 8:46 PM ]

someone should check to see if the clojure ioc macros do this same
thing, because you could have a similar issue, something like:

(def ^:dynamic foo 42)

(go (binding [foo 5] (let [x foo] (set! foo 20) (println x)))

I would expect that to print 5, if there is some kind of aliasing bug, it might print 20

Comment by Kevin Downey [ 06/Apr/16 8:50 PM ]

the 001 patch causes local bindings that are initialized from a global to actually create a local and initialize it from the global, instead of reading from the global.

Comment by Kevin Downey [ 06/Apr/16 10:16 PM ]

patch 0002 is patch 0001, but using the same code path in let binding inits and loops binding inits

Comment by Kevin Downey [ 07/Apr/16 12:49 PM ]

0003 adds a test for the expected local binding behavior when aliasing a global





[ASYNC-156] (ClojureScript) go block containing letfn does not compile Created: 30/Dec/15  Updated: 17/Jun/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Gabe Johnson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, go-macro
Environment:

[org.clojure/clojure "1.8.0-RC3"]
[org.clojure/clojurescript "1.7.189"]
[org.clojure/core.async "0.2.374"]


Approval: Triaged

 Description   

The following compiles and runs correctly:

(ns cljs-letfn-go-bug.core
  (:require [clojure.core.async :refer [go]]))

(go
  (letfn [(foo [x] x)] (foo 1)))

However, this fails to compile with `clojure.lang.ExceptionInfo: bindings must be vector of even number of elements...`:

(ns cljs-letfn-go-bug.core
  (:require-macros [cljs.core.async.macros :refer [go]]))

(go
  (letfn [(foo [x] x)] (foo 1)))


 Comments   
Comment by Gabe Johnson [ 30/Dec/15 11:09 AM ]

Please forgive the markdown. I haven't used JIRA in quite some time and don't appear to have permissions to edit the description.





[ASYNC-155] Preserve loop binding metadata when inside a go block Created: 15/Dec/15  Updated: 04/Jan/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-ASYNC-155-preserve-loop-binding-metadata.patch    
Patch: Code
Approval: Triaged

 Description   

Description:
Currently this causes a reflection warning because core.async loses the type hint on the binding:

(go (loop* [^String foo "foo"](.substring foo 0) (<!) (recur nil)))
Reflection warning, /tmp/form-init2119477256040974630.clj:1:1 - call to method substring can't be resolved (target class is unknown).

This is a problem since macros like `doseq` expand to similar code and cause reflection warnings

The attached patch preserves that metadata and removes the reflection warning

Patch: 0001-ASYNC-155-preserve-loop-binding-metadata.patch



 Comments   
Comment by Marcus Crestani [ 04/Jan/16 3:17 AM ]

I can confirm that this patch also fixes ASYNC-157. Thanks!





[ASYNC-154] (require :reload-all) causes NullPointerException Created: 29/Nov/15  Updated: 22/Dec/15

Status: Reopened
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Aaron Cummings Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Both Oracle and IBM JDK (1.7) affected.


Approval: Triaged

 Description   

I'm seeing a problem with (require :reload-all ...) with clojure.core.async, where running with :reload-all causes a NullPointerException as shown below.

Curiously, this does not happen with 'lein repl'; it seems the :reload-all is being somehow inhibited there.

So, starting the REPL like this:

java -cp asm-all-4.2.jar:clojure-1.7.0.jar:core.async-0.2.374.jar:core.cache-0.6.4.jar:core.memoize-0.5.8.jar:data.priority-map-0.0.7.jar:tools.analyzer-0.6.7.jar:tools.analyzer.jvm-0.6.9.jar:tools.reader-1.0.0-alpha1.jar clojure.main

Results below:

Clojure 1.7.0
user=> (require ['clojure.core.async])
nil
user=> (require :reload-all ['clojure.core.async])
CompilerException java.lang.NullPointerException, compiling:(clojure/core/async.clj:1138:8) 
user=> *e
#error {
 :cause nil
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.NullPointerException, compiling:(clojure/core/async.clj:1138:8)"
   :at [clojure.lang.Compiler analyzeSeq "Compiler.java" 6730]}
  {:type java.lang.NullPointerException
   :message nil
   :at [clojure.tools.analyzer.passes.jvm.warn_on_reflection$eval10493$fn__10494 invoke "warn_on_reflection.clj" 58]}]
 :trace
 [[clojure.tools.analyzer.passes.jvm.warn_on_reflection$eval10493$fn__10494 invoke "warn_on_reflection.clj" 58]
  [clojure.lang.MultiFn invoke "MultiFn.java" 229]
  [clojure.lang.Var invoke "Var.java" 379]
  [clojure.tools.analyzer.passes$compile_passes$fn__8431$fn__8436 invoke "passes.clj" 166]
  [clojure.tools.analyzer.passes$compile_passes$fn__8431$fn__8438 invoke "passes.clj" 168]
  [clojure.tools.analyzer.passes$compile_passes$fn__8431$fn__8438 invoke "passes.clj" 168]
  [clojure.tools.analyzer.passes$compile_passes$fn__8431$fn__8438 invoke "passes.clj" 168]
  [clojure.tools.analyzer.passes$compile_passes$fn__8431$fn__8438 invoke "passes.clj" 168]
  [clojure.core$partial$fn__4529 invoke "core.clj" 2500]
  [clojure.tools.analyzer.ast$walk$walk__8336 invoke "ast.clj" 99]
  [clojure.tools.analyzer.ast$walk$walk__8336$walk__8337 invoke "ast.clj" 96]
  [clojure.tools.analyzer.ast$_update_children$fn__8327 invoke "ast.clj" 51]
  [clojure.lang.PersistentVector reduce "PersistentVector.java" 333]
  [clojure.core$reduce invoke "core.clj" 6518]
  [clojure.tools.analyzer.ast$_update_children invoke "ast.clj" 49]
  [clojure.tools.analyzer.ast$update_children_reduced invoke "ast.clj" 64]
  [clojure.tools.analyzer.ast$walk$walk__8336 invoke "ast.clj" 99]
  [clojure.tools.analyzer.ast$walk$walk__8336$walk__8337 invoke "ast.clj" 96]
  [clojure.tools.analyzer.utils$mapv_SINGLEQUOTE_ invoke "utils.clj" 208]
  [clojure.tools.analyzer.ast$_update_children$fn__8327 invoke "ast.clj" 51]
  [clojure.lang.PersistentVector reduce "PersistentVector.java" 333]
  [clojure.core$reduce invoke "core.clj" 6518]
  [clojure.tools.analyzer.ast$_update_children invoke "ast.clj" 49]
  [clojure.tools.analyzer.ast$update_children_reduced invoke "ast.clj" 64]
  [clojure.tools.analyzer.ast$walk$walk__8336 invoke "ast.clj" 99]
  [clojure.tools.analyzer.ast$walk$walk__8336$walk__8337 invoke "ast.clj" 96]
  [clojure.tools.analyzer.ast$_update_children$fn__8327 invoke "ast.clj" 51]
  [clojure.lang.PersistentVector reduce "PersistentVector.java" 333]
  [clojure.core$reduce invoke "core.clj" 6518]
  [clojure.tools.analyzer.ast$_update_children invoke "ast.clj" 49]
  [clojure.tools.analyzer.ast$update_children_reduced invoke "ast.clj" 64]
  [clojure.tools.analyzer.ast$walk$walk__8336 invoke "ast.clj" 99]
  [clojure.tools.analyzer.ast$walk invoke "ast.clj" 95]
  [clojure.tools.analyzer.ast$walk invoke "ast.clj" 92]
  [clojure.tools.analyzer.ast$prewalk invoke "ast.clj" 108]
  [clojure.tools.analyzer.passes$compile_passes$analyze__8443 invoke "passes.clj" 170]
  [clojure.core$comp$fn__4495 invoke "core.clj" 2438]
  [clojure.core$comp$fn__4495 invoke "core.clj" 2438]
  [clojure.core$comp$fn__4495 invoke "core.clj" 2438]
  [clojure.core$comp$fn__4495 invoke "core.clj" 2438]
  [clojure.core$comp$fn__4495 invoke "core.clj" 2438]
  [clojure.core$comp$fn__4495 invoke "core.clj" 2438]
  [clojure.tools.analyzer.jvm$analyze$fn__11679 invoke "jvm.clj" 469]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.AFn applyTo "AFn.java" 144]
  [clojure.core$apply invoke "core.clj" 630]
  [clojure.core$with_bindings_STAR_ doInvoke "core.clj" 1868]
  [clojure.lang.RestFn invoke "RestFn.java" 425]
  [clojure.tools.analyzer.jvm$analyze invoke "jvm.clj" 456]
  [clojure.core.async.impl.ioc_macros$state_machine invoke "ioc_macros.clj" 1109]
  [clojure.core.async$go doInvoke "async.clj" 413]
  [clojure.lang.RestFn invoke "RestFn.java" 442]
  [clojure.lang.Var invoke "Var.java" 388]
  [clojure.lang.AFn applyToHelper "AFn.java" 160]
  [clojure.lang.Var applyTo "Var.java" 700]
  [clojure.lang.Compiler macroexpand1 "Compiler.java" 6631]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6709]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler analyze "Compiler.java" 6485]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 5861]
  [clojure.lang.Compiler$LetExpr$Parser parse "Compiler.java" 6179]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6723]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6711]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler analyze "Compiler.java" 6485]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 5861]
  [clojure.lang.Compiler$FnMethod parse "Compiler.java" 5296]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 3925]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6721]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6711]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler access$300 "Compiler.java" 38]
  [clojure.lang.Compiler$DefExpr$Parser parse "Compiler.java" 577]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6723]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler analyze "Compiler.java" 6485]
  [clojure.lang.Compiler eval "Compiler.java" 6786]
  [clojure.lang.Compiler load "Compiler.java" 7227]
  [clojure.lang.RT loadResourceScript "RT.java" 371]
  [clojure.lang.RT loadResourceScript "RT.java" 362]
  [clojure.lang.RT load "RT.java" 446]
  [clojure.lang.RT load "RT.java" 412]
  [clojure.core$load$fn__5448 invoke "core.clj" 5866]
  [clojure.core$load doInvoke "core.clj" 5865]
  [clojure.lang.RestFn invoke "RestFn.java" 408]
  [clojure.core$load_one invoke "core.clj" 5671]
  [clojure.core$load_all$fn__5389$fn__5392 invoke "core.clj" 5688]
  [clojure.core$load_all$fn__5389 invoke "core.clj" 5687]
  [clojure.lang.AFn call "AFn.java" 18]
  [clojure.lang.LockingTransaction run "LockingTransaction.java" 273]
  [clojure.lang.LockingTransaction runInTransaction "LockingTransaction.java" 229]
  [clojure.core$load_all invoke "core.clj" 5685]
  [clojure.core$load_lib$fn__5397 invoke "core.clj" 5711]
  [clojure.core$load_lib doInvoke "core.clj" 5710]
  [clojure.lang.RestFn applyTo "RestFn.java" 142]
  [clojure.core$apply invoke "core.clj" 632]
  [clojure.core$load_libs doInvoke "core.clj" 5749]
  [clojure.lang.RestFn applyTo "RestFn.java" 137]
  [clojure.core$apply invoke "core.clj" 632]
  [clojure.core$require doInvoke "core.clj" 5832]
  [clojure.lang.RestFn invoke "RestFn.java" 421]
  [user$eval7383 invoke "NO_SOURCE_FILE" 2]
  [clojure.lang.Compiler eval "Compiler.java" 6782]
  [clojure.lang.Compiler eval "Compiler.java" 6745]
  [clojure.core$eval invoke "core.clj" 3081]
  [clojure.main$repl$read_eval_print__7099$fn__7102 invoke "main.clj" 240]
  [clojure.main$repl$read_eval_print__7099 invoke "main.clj" 240]
  [clojure.main$repl$fn__7108 invoke "main.clj" 258]
  [clojure.main$repl doInvoke "main.clj" 258]
  [clojure.lang.RestFn invoke "RestFn.java" 421]
  [clojure.main$repl_opt invoke "main.clj" 324]
  [clojure.main$main doInvoke "main.clj" 422]
  [clojure.lang.RestFn invoke "RestFn.java" 397]
  [clojure.lang.Var invoke "Var.java" 375]
  [clojure.lang.AFn applyToHelper "AFn.java" 152]
  [clojure.lang.Var applyTo "Var.java" 700]
  [clojure.main main "main.java" 37]]}
user=>


 Comments   
Comment by Alex Miller [ 22/Dec/15 4:09 PM ]

dupe of and fixed by ASYNC-152

Comment by Nicola Mometto [ 22/Dec/15 6:00 PM ]

Just for the record, this is neither a core.async nor a tools.analyzer bug.
It turns out that re-evaluating a defmulti expression completely nukes any metadata on that defmulti, breaking tools.analyzer's scheduler since it relies on that metadata to order the passes.

Comment by Nicola Mometto [ 22/Dec/15 6:08 PM ]

Alex Miller I'm reopening this, pending an assessment on CLJ-1870.
While it is true that the ASYNC-154 patch fixed this specific NPE, `(require :reload-all)` will still break things since as explained in the ticked description of CLJ-1870, t.a passess will be ordered randomly





[ASYNC-127] mult distribution behavior doesn't work as intended Created: 08/Jun/15  Updated: 08/Jun/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: mult

Attachments: Text File async-127.patch    
Approval: Triaged

 Description   

Quote from docstring: "[...] each tap must accept before the next item is distributed."

(def ch (chan))

(def m (mult ch))

(def t-1 (chan))
(def t-2 (chan))
(def t-3 (chan))

(def t-1-takes (atom []))

(defn log [l] (partial swap! l conj))

(tap m t-1)
(tap m t-2)
(tap m t-3)

(close! t-3)

(take! t-1 (log t-1-takes))

(take! t-1 (log t-1-takes)) ;; this take shouldn't be happening before
                            ;; a take on t-2

(put! ch true)

(put! ch true)

@t-1-takes

;-> [true true] ;; but it does.

The reason is that the internal atom dctr is decreased twice when a tapped channel is already closed.



 Comments   
Comment by Leon Grapenthin [ 08/Jun/15 1:53 PM ]

Fixing this for clj/cljs





[ASYNC-125] Closing a tap with a pending item blocks the mult input channel Created: 07/Jun/15  Updated: 07/Jun/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Klaus Wuestefeld Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

org.clojure:clojure:1.7.0-alpha5
org.clojure:core.async:0.1.346.0-17112a-alpha


Approval: Triaged

 Description   

Closing a tap without a pending item is OK but closing a tap with a pending item blocks the mult input channel:

(require '[clojure.core.async :refer :all])
(def c (chan))
(def m (mult c))
(def t (chan))
(tap m t)
(>!! c :a)
(close! t)
(>!! c :b)  ; BLOCKS


 Comments   
Comment by Klaus Wuestefeld [ 07/Jun/15 6:22 PM ]

A more general case:

Doing this:

(go (println (>! c 42)))

and then closing c will cause the >! to block, instead of returning false.

If c is closed before that, >! will return false.

Is this race condition the intended behavior?





[ASYNC-122] Parking in finally block replaces result Created: 06/May/15  Updated: 17/May/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Vesa Karvonen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

[org.clojure/clojurescript "0.0-3211"]
[org.clojure/clojure "1.6.0"]
[org.clojure/core.async "0.1.346.0-17112a-alpha"]


Approval: Triaged

 Description   

Because

(try 1 (finally 2))

evaluates to 1, I would expect

(go (println (try 1 (finally (<! (go 2))))))

to print 1, but it prints 2 in both CLJ and CLJS. This can be worked around by replacing try-finally with try-catch.



 Comments   
Comment by Nicola Mometto [ 17/Dec/15 10:06 AM ]

This appears to be a bug caused by nesting `go` blocks

Comment by Kevin Downey [ 17/May/16 7:54 PM ]

the patch on http://dev.clojure.org/jira/browse/ASYNC-169 appears to fix this





[ASYNC-100] core.async with multiple catch blocks causing weird loop behaviour Created: 27/Oct/14  Updated: 17/May/16

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Tom Coupland Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File ASYNC-100_2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've been seeing this weird looping behavior with some go loops over the last few days. An exception is being thrown from a function within the loop and rather than logging and looping back around to a waiting take, the loop seems to just hop back to the line before the function call.

I've managed to boil it down to the following:

(def s (chan))
(def single
  (go-loop []
    (try
      (prn "Awaiting single")
      (<! s)
      (prn "Single")
      (throw (new Throwable))
      (catch Throwable t
        (prn t)))
    (recur)))

(def d (chan))
(def double
  (go-loop []
    (try
      (prn "Awaiting double")
      (<! d)
      (prn "Double")
      (throw (new Throwable))
      (catch Exception re
        (prn re))
      (catch Throwable t
        (prn t)))
    (recur)))

Now if you (>!! s :a), you'll see the throwable printed out and the loop go back to waiting on the s channel. However, (>!! d :a) and you'll get to enjoy an infinite stream of 'Double'. In actual fact you can remove the -loop from double and get the same result.

Not sure what's going on here at all. In the macro expanded version of double '(prn t)' doesn't appear at all (it does in single's expansion), so it looks like it's not surviving the move into the state machine and instead is routing back to (prn "Double") or the take isn't really completing somehow, leaving the :a on the chan.



 Comments   
Comment by Paavo Parkkinen [ 03/Dec/14 5:30 PM ]

From my experiments, it seems what's causing the issue isn't the two catch clauses, but the fact that you are catching an Exception first, and then the Throwable. When I switched the two catch clauses (Throwable first) the issue went away.

You don't even need the two catch clauses to trigger it, a single catch clause of an Exception will do.

;; Works
(require '[clojure.core.async :as async])
(def c (async/chan))
(def st
  (async/go-loop []
    (try
      (prn "Awaiting")
      (async/<! c)
      (prn "Received")
      (throw (new Throwable))
      (catch Throwable t
             (prn t)))
    (recur)))
(async/>!! c :a)
;; Doesn't work
(require '[clojure.core.async :as async])
(def c (async/chan))
(def se
  (async/go-loop []
    (try
      (prn "Awaiting")
      (async/<! c)
      (prn "Received")
      (throw (new Throwable))
      (catch Exception t
             (prn t)))
    (recur)))
(async/>!! c :a)
Comment by Paavo Parkkinen [ 04/Dec/14 12:18 AM ]

Attaching a patch that fixes the original issue, but not the one in my comment above with an uncaught exception. All test cases pass.

Instead of just picking the first catch block, and creating a state machine block and exception frame for that one, I create blocks and frames for all of them.

I tried creating a test case too, but was unable to create one that would reproduce the error. I'll spend some more time trying to write a test case, but I wanted to submit the code patch without the test coverage first.

Comment by Paavo Parkkinen [ 04/Dec/14 7:45 PM ]

Attached patch with test case included.

Comment by Stuart Sierra [ 18/Jun/15 2:52 PM ]

Possible workaround until this is fixed: Just have one catch clause, catching all Throwable. Then examine the type of the throwable object inside the catch block to decide what to do.

Comment by Ken Allen [ 29/Oct/15 1:34 PM ]

This is even more troublesome when the exception is thrown from code that's not part of what the user wrote in the go block. You don't even need a loop for it to loop forever:

(require '[clojure.core.async :as async])
(def c (async/chan))
(doseq [_ (range 1024)] (async/put! c :test))
(async/go (try (println "BEFORE") (async/>! c :test) (catch Exception ex (println "ERR"))))

This was confusing as hell when we ran into it but catching Throwable instead does indeed work around the problem so that's what we've been doing. Was hoping the latest release of core.async would fix this.

Comment by Thomas Getgood [ 26/Jan/16 10:46 AM ]

As it is, you need to catch Throwable or nothing. Unhandled Throwables will cause this as well.

(require '[clojure.core.async :refer [go <! put! chan]])
(def ch (chan))
(go (try (print (<! ch)) (assert false) (catch Exception e nil)))
(put! ch 1)

Will print an infinite stream of 1s.

It was a lot of fun figuring out why functions failing :pre conditions caused infinite loops.

Comment by Kevin Downey [ 17/May/16 8:08 PM ]

the patch here no longer applies. http://dev.clojure.org/jira/browse/ASYNC-169 is a dupe of this, but it has a patch that applies





[ASYNC-92] go macro removes binding forms that are intialized with logical false value Created: 03/Oct/14  Updated: 30/Oct/14

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Oleh Palianytsia Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

org.clojure/core.async "0.1.346.0-17112a-alpha"


Attachments: File fix-async-92.diff    
Approval: Triaged

 Description   
(require '[clojure.core.async :as a])

(a/go (let [a nil] (a/alts! (if a <whatever> <whatever>)))) // Unable to resolve a
(a/go (let [a nil] (a/<! (if a <whatever> <whatever>))) // Unable to resolve a

Seems that 'go' macro removes falsely initialized symbols that are used as channels, because
in both cases there's exception, that says " Unable to resolve symbol: a in this context".



 Comments   
Comment by Willy Blandin [ 17/Oct/14 12:19 PM ]

Confirmed.
Bug was introduced between 0.1.278.0-76b25b-alpha and 0.1.295.0-9ea6ef-alpha.

Comment by Willy Blandin [ 17/Oct/14 12:27 PM ]

Worked around with:

(defmacro workaround-async-92
  "Hack to workaround core.async bug
   cf. http://dev.clojure.org/jira/browse/ASYNC-92"
  []
  ;; has to be a list
  `(do nil))

(let [a (workaround-async-92)]
  ...)
Comment by Leon Grapenthin [ 23/Oct/14 11:55 AM ]

modifies two methods of the RawCode inst so that they check:collected-locals in locals via contains? before ignoring them

Comment by Ghadi Shayban [ 23/Oct/14 5:19 PM ]

Hi Leon, thanks for the patch. Can you fill out a Contributor Agreement? http://clojure.org/contributing

Comment by Leon Grapenthin [ 24/Oct/14 7:17 AM ]

I did, yesterday. Got an automatic confirmation email saying Rich Hickey signed it. Anything else I should do with it?





[ASYNC-90] Pub/sub leaks memory Created: 13/Sep/14  Updated: 22/Dec/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ziyang Hu Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None
Environment:

[org.clojure/clojure "1.6.0"]
[org.clojure/core.async "0.1.303.0-886421-alpha"]

java version "1.7.0_45"
Java(TM) SE Runtime Environment (build 1.7.0_45-b18)
Java HotSpot(TM) 64-Bit Server VM (build 24.45-b08, mixed mode) (reproducible on OpenJDK 7 as well)

OS X 10.9.4 (reproducible on Ubuntu Linux 14.04 as well)


Approval: Triaged

 Description   

The following code will cause OOME:

(require '[clojure.core.async :refer [chan close! <! pub sub unsub go timeout]])

(def p* (chan))

(def p (pub p* :topic))

(go
  (loop []
    (let [s (chan)
          t (rand-int Integer/MAX_VALUE)]
      (sub p t s)
      (<! (timeout 10))
      (unsub p t s)
      (close! s)
      (recur))))

(It grows slowly: to see the OOME in a reasonable amount of time, either give JVM very small
memory like 64m, or remove the timeout step.)

I tried to profile the code, and the reason seems to be that even though I
unsubed the channel from the port, something is still retained which causes
the heap to be used up.



 Comments   
Comment by Ziyang Hu [ 13/Sep/14 8:50 AM ]

Here is the problem:

https://github.com/clojure/core.async/blob/96de9a47ac511d9bb4309645a3bc594a2fc0c33a/src/main/clojure/clojure/core/async.clj#L826-L828

When unsub* is called, it just untaps the channel from the mult specified by the topic. The mult still remains in the atom called mults even if the mult has no taps left.

I can't think of a clean fix for this problem, since currently the channels which are tapping a mult aren't exposed, i.e., we currently have no way of knowing if a mult has any taps on it.

Comment by Ghadi Shayban [ 15/Oct/14 11:27 PM ]

I also cannot think of a clean fix, as mults do not expose their registrants. (The notion of "current" is a concurrent system is subtle)
Besides saying (and perhaps amending the docstring) that pubs are indeed resources, their footprint grows by the # of seen topics, and that resources should almost always be bounded.

Comment by Daniel Compton [ 11/Oct/15 5:02 PM ]

This bit us too in https://github.com/apa512/clj-rethinkdb/issues/97. My suggestion for a docstring addition is:

pubs are resources, and their footprint grows by the number of seen topics. unpub does not reclaim this resource. You should not use pub to subscribe to an unbounded number of topics.

Are there any changes/suggestions? Would you like me to create a patch for this?

Comment by Daniel Compton [ 11/Oct/15 5:04 PM ]

Also, is a pub able to be garbage collected when it's no longer used? When I was doing some limited testing, it didn't seem like it was, but I may be wrong.

Comment by Daniel Compton [ 15/Oct/15 4:45 PM ]

A partial workaround (I think?) is to use

(unsub-all pub topic)
if you know you don't need a topic anymore.





[ASYNC-64] Race condition when closing mults Created: 29/Apr/14  Updated: 16/Oct/14

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

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

Approval: Triaged

 Description   

When a mult is tapped at around the same time as the source channel is closed, the tapped channel may not be closed.

(require '[clojure.core.async :refer (chan mult tap close!)])
(let [s (chan)
      m (mult s)
      c (chan)]
  (tap m c)
  (close! s)
  (impl/closed? c))

The above code will sometimes return true, and sometimes return false.

Cause: This is caused by the following code in the mult function:

(if (nil? val)
  (doseq [[c close?] @cs]
    (when close? (close! c)))

Any channels tapped after cs is dereferenced will not be closed.

Approach: A possible solution to this could be to always close channels tapped to a closed source. i.e.

(let [s (chan)
      m (mult s)
      c (chan)]
  (close! s)
  (tap m c))  ;; will always close c

This could be achieved by adding a flag to the cs atom to denote whether the mult is open or closed. If it's closed, any tapped channel is closed automatically.



 Comments   
Comment by James Reeves [ 30/Apr/14 6:05 AM ]

For reference, below is the custom fix for mult I'm using:

(defn mult [ch]
  (let [state (atom [true {}])
        m (reify
            Mux
            (muxch* [_] ch)
            Mult
            (tap* [_ ch close?]
              (let [add-ch    (fn [[o? cs]] [o? (if o? (assoc cs ch close?) cs)])
                    [open? _] (swap! state add-ch)]
                (when-not open? (close! ch))
                nil))
            (untap* [_ ch]
              (swap! state (fn [[open? cs]] [open? (dissoc cs ch)]))
              nil)
            (untap-all* [_]
              (swap! state (fn [[open? _]] [open? {}]))))
        dchan (chan 1)
        dctr (atom nil)
        done (fn [_] (when (zero? (swap! dctr dec))
                       (put! dchan true)))]
    (go-loop []
      (let [val (<! ch)]
        (if (nil? val)
          (let [[_ cs] (swap! state (fn [[_ cs]] [false cs]))]
            (doseq [[c close?] cs]
              (when close? (close! c))))
          (let [chs (keys (second @state))]
            (reset! dctr (count chs))
            (doseq [c chs]
              (when-not (put! c val done)
                (swap! dctr dec)
                (untap* m c)))
            (when (seq chs)
              (<! dchan))
            (recur)))))
    m))
Comment by David Nolen [ 14/Oct/14 6:10 AM ]

Is this also fixed in master? Thanks.

Comment by Ghadi Shayban [ 15/Oct/14 11:09 PM ]

I understand the scenario, but honestly I'm not sure this is a bug in mult or the usage. A channel shouldn't be expected to always yield a take. The consumer of the "late tap" can guard against it with alts or some other mechanism, and also you can enforce a no-late-taps through a policy on the "production" side of things.

Rich Hickey can you weigh in?

Comment by James Reeves [ 16/Oct/14 3:51 AM ]

The "tap" function currently has an explicit "close?" flag, and if a tapped channel isn't guaranteed to close when the source channel closes, that argument probably shouldn't exist. Also, if auto-closing taps is taken out, should we remove the "close?" argument on "sub" as well?

Comment by Ghadi Shayban [ 16/Oct/14 11:34 AM ]

It's more than respecting the flag. Related to the close behavior, channels can tap and untap without receiving anything while the mult process happily distributes a value to another set of channels (like the ABA problem). Could also make it an error to tap after the close is distributed to the last deref'ed set of channels. That is different than the familiar permanent nil receive, but mults already differ from simple channels.





[ASYNC-58] mult channel deadlocks when untapping a consuming channel whilst messages are being queued/blocked Created: 20/Feb/14  Updated: 23/Jun/14

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Mathieu Gauthron Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: deadlock, mult, untap
Environment:

Mac 10.7.5; java version "1.7.0_40"; [org.clojure/clojure "1.5.1"]; [org.clojure/core.async "0.1.267.0-0d7780-alpha"]; Tested with cider and emacs 24.3


Approval: Triaged

 Description   

I have two (or more) listeners tapped onto a mult channel. I want to use them all then have one (or more) of them to leave at will without blocking the other consumer(s) or the publisher. Initially they work fine until one of them wants to stop listening. I thought the listener which drops out needs to (be a good citizen and) untap its channel from mult (otherwise a deadlock is systematic). However if messages are put into the mult before the leaving listener has had a chance to untap its channel, it creates a deadlock on the main thread (which is putting more messages simultaneously). I do not find a way to guarantee that I can untap the channel in time to avoid this race condition.

Once I have reproduced the deadlock, the repl is frozen until I interrupt with ctrl-c.
I have also tried to close the tapped channel before untapping it but the result was the same.

In the following snippet, the last (println "I'm done. You will never see this") is never reached. The publisher and the remaining consumer (consumer 1) are deadlocked even though consumer 2 was trying to leave in good terms.

(require '[clojure.core.async :refer (chan go <! >!! mult tap untap)])
(let [to-mult (chan 1)
      m (mult to-mult)]

  ;;consumer 1
  (let [c (chan 1)]
    (tap m c)
    (go (loop []
          (when-let [v (<! c)]
            (println "1 Got! " v)
            (recur))
          (println "1 Exiting!"))))

  ;;consumer 2
  (let [c (chan 1)]
    (tap m c)
    (go (loop []
          (when-let [v (<! c)]
            (when (= v 42)  ;; exit when value is not 42
              (println "2 Got! " v)
              (recur)))
          (println "2 about to leave!")
          (Thread/sleep 5000) ;; wait a bit to exacerbate the race condition
          (untap m c) ;; before unsubscribing this reader
          (println "2 Exiting."))))

   (println "about to put a few messages that work")
   (doseq [a (range 10)]
     (>!! to-mult 42))
   (println "about to put a message that will force the exit of 2")
   (>!! to-mult 43)
   (println "about to put a few more messages before reader 2 is unsubscribed to show the deadlock")
   (doseq [a (range 10)]
     (println "putting msg" a)
     (>!! to-mult 42))
   (println "I'm done. You will never see this"))
about to put a few messages that work
2 Got!  42
1 Got!  42
2 Got!  42
1 Got!  42
1 Got!  42
2 Got!  42
1 Got!  42
1 Got!  42
2 Got!  42
2 Got!  42
2 Got!  42
2 Got!  1 Got!  42
422 Got!  42

1 Got!  42
1 Got!  42
2 Got!  42
1 Got!  42
about to put a message that will force the exit of 2
1 Got!  42
2 Got!  about to put a few more messages before reader 2 is unsubscribed to show the deadlock
42
putting msg 1 Got!  0
2 about to leave!
43
1 Got!  42
putting msg 1
putting msg 2
putting msg 3
1 Got!  42
2 Exiting.


 Comments   
Comment by Ghadi Shayban [ 22/Apr/14 10:18 AM ]

Mathieu, this is probably expected. It's important to note that to guarantee correct ordering/flow when using a mult, you should enforce it on the source/producer side of the mult, and not asynchronously on the tap side.

Mult will deref a stable set taps just before distributing a value to them, and does not adjust dynamically during value distribution except when a tap has been closed [1]. If you would like to stably untap without closing the tap you can/should let the 'producer' do it in an ordered fashion in between values on the input channel.

Knowing that a put occurred to a closed channel is new on release 0.1.278.

In general, walking away on the consuming side of a channel is tricky. Depending on the semantics of your processes, if the producer side of a channel isn't aware that a close! can happen from the consumer side, you might have to launch a draining operation.

(defn drain [c] (go (when (some? (<! c)) (recur))))

Golang disallows closing a read-only channel FWIW [2]

Better documentation is probably warranted.

[1] https://github.com/clojure/core.async/blob/master/src/main/clojure/clojure/core/async.clj#L680-L682
[2] http://golang.org/ref/spec#Close





[ASYNC-32] onto-chan retains head of input sequence causing OutOfMemoryError Created: 06/Nov/13  Updated: 15/Dec/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Brian Lubeski Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: memory
Environment:

org.clojure/core.async 0.1.242.0-44b1e3-alpha


Attachments: File patch.clj    
Patch: Code
Approval: Triaged

 Description   
(require '[clojure.core.async :as a])
(let [c (a/chan)]
  (a/onto-chan c (iterate inc 0))
  (while true
    (a/<!! c)))

onto-chan is holding on to the head of the input sequence as it is unfolded, resulting in an (eventual) OutOfMemoryError.

I've attached a diff showing changes I made to onto-chan that fixed the problem for me.



 Comments   
Comment by Colin Taylor [ 07/Oct/14 7:13 PM ]

Just to note, to-chan uses onto-chan so is similarly affected.
We ran into this, Brian's patch worked fine.

Comment by Alex Miller [ 10/Aug/15 3:22 PM ]

This seems like a viable change, but the patch needs a better test (even if not in the patch) and to be properly formatted for git apply (per http://dev.clojure.org/display/community/Developing+Patches).

Comment by Brian Lubeski [ 11/Aug/15 12:00 AM ]

This issue might be caused by ASYNC-138.

Comment by Nicola Mometto [ 15/Dec/15 6:12 PM ]

The fix for ASYNC-138 should fix this issue aswell and is more general





[ASYNC-4] Record literals become ordinary maps Created: 15/Jul/13  Updated: 23/Jun/14

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

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

CLJS


Approval: Triaged

 Description   
(require '[clojure.core.async :refer (<!! go)])
(defrecord Foo [x])
(def f (Foo. 4))
(<!! (go f))
;; => #user.Foo{:x 4}
;; OK

(<!! (go #clojure.core.async.Foo{:x 4}))
;; CLJ: => #user.Foo{:x 4}   ;; expected
;; CLJS => {:x 4}            ;; wrong

Approach: Query the analyzer to know if we have a record or not.

(Copied from GitHub issue #13 - https://github.com/clojure/core.async/issues/13)



 Comments   
Comment by Ghadi Shayban [ 03/Aug/13 2:08 PM ]

0c6e663493 contains a fix on the Clojure side, would appreciate help porting to cljs.

Comment by David Nolen [ 08/Aug/13 8:35 AM ]

I think on the ClojureScript we'll have to query the analyzer to know if we have a record or not.





[CLJ-1960] Bug in clojure.core/mod with large Double argument Created: 14/Jun/16  Updated: 15/Jun/16

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

Type: Defect Priority: Minor
Reporter: William Tozier Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math, numerics
Environment:

Java 8 update 91 on Mac OS X 10.11.5


Approval: Triaged

 Description   

The `clojure.core/mod` function works just as expected for small positive floating-point dividend and small positive integer divisor. But today I was working on some edge case tests and came across the following inexplicable behavior:

REPL_session
user=> (def big  Double/MAX_VALUE)
#'user/big
user=> (mod big 10)
0.0
user=> (mod big 100)
0.0
user=> (mod big 1000)
1.9958403095347198E292
user=> (mod big 999)
-Infinity
user=> (mod big 998)
0.0
user=> (mod big 997)
1.9958403095347198E292
user=> (mod big 996)
0.0
user=> (mod big 995)
0.0
user=> (mod big 994)
0.0
user=> (mod big 1001)
1.9958403095347198E292
user=> (mod big 1002)
0.0
user=> (mod big 1003)
0.0
user=> (mod big 1004)
-Infinity
user=> (mod big 1005)
0.0

No idea whether this is inherited from a Java bug. I can see nothing special about the values chosen, and I suspect if one scanned it'd be easy to find other glitches.



 Comments   
Comment by Alex Miller [ 14/Jun/16 7:12 PM ]

mod is based on rem - from a glance, mod does not seem to account properly for any case of overflow, and I suspect that's at the root of a lot of these problems.

Comment by Gary Fredericks [ 14/Jun/16 7:15 PM ]

Test.check suggests (mod 6.7772677936779424E16 23) => -8.0 is somewhat close to minimal.

Comment by William Tozier [ 15/Jun/16 12:40 PM ]

Actually, just checked, and rem gives the same results. Thus (rem Double/MAX_VALUE 1001) is 1.9958403095347198E292, and (rem 6.7772677936779424E16 23) => -8.0.





[CLJ-1955] .hashCode throws ClassCastException when called on some functions Created: 09/Jun/16  Updated: 14/Jun/16

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

Type: Defect Priority: Minor
Reporter: Georgi Danov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Approval: Triaged

 Description   
user> some?
#function[clojure.core/some?]
user> (.hashCode map)
72400056
user> (.hashCode str)
ClassCastException clojure.core$str cannot be cast to java.lang.String  /eval39172 (form-init3428514420830954023.clj:5793)
user> (.hashCode (fn []))
1715179801
user> (.hashCode some?)
ClassCastException clojure.core$some_QMARK_ cannot be cast to java.lang.Boolean  /eval39178 (form-init3428514420830954023.clj:5797)
user> (.hashCode #'some?)
1955712430
user> (.hashCode @#'some?)
1726569843


 Comments   
Comment by Nicola Mometto [ 10/Jun/16 3:27 AM ]

This happens because `some?` and `str` have type hints on the Var to signal the type returned by their invocations, but the Compiler thinks those type hints apply to the Var object itself aswell.

An easy fix would be to move those type hints from the Var (old-style) to the argvec (new-style)

Comment by Ghadi Shayban [ 14/Jun/16 3:36 PM ]

agreed with nicola's suggestion - change type hints. This is a dup of CLJ-140 where :tag causes confusion when a var is being invoked vs used in expr context





[CLJ-1911] min-key and max-key should return NaN if any of the argument is NaN Created: 08/Apr/16  Updated: 12/May/16

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

Type: Defect Priority: Minor
Reporter: Renzo Borgatti Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None
Environment:

Likely All. Including older version of Clojure.


Attachments: Text File CLJ-1911-contagious-NaN-and-tests.patch     Text File CLJ-1911-contagious-NaN.patch     Text File CLJ-1911-NaN-fix-over-CLJ-99.patch    
Patch: Code
Approval: Triaged

 Description   

It appears that min-key and max-key behave incorrectly (following Java that follows IEEE floating point convention):

(apply max-key last [[:a 10000] [:b (/ 0. 0)] [:c 0]])
[:c 0]

Not sure how this should then propagate forward, but definitely not silently. Options:

1. [:b NaN] (the first item to generate the NaN)
2. NaN (this is changing the expected type)
3. ArithmeticException Operation with at least one NaN operand.

If this was to be patched the same as it was for min/max (http://dev.clojure.org/jira/browse/CLJ-868) it will probably result in option 1.



 Comments   
Comment by Nicholas Antonov [ 14/Apr/16 9:36 PM ]

This implements the first solution of a contagious NaN in the same style as CLJ 868

Comment by Alex Miller [ 15/Apr/16 12:03 AM ]

Patch should have tests...

Comment by Nicholas Antonov [ 15/Apr/16 1:07 AM ]

This latest patch adds tests for min-key and max-key with and without NaN results, as there were none before.

Comment by Alex Miller [ 29/Apr/16 10:06 AM ]

This overlaps with CLJ-99, which has already been prescreened. I would like to base whatever changes this patch requires over the top of that ticket. To build this, apply the CLJ-99 patch, then branch, make you changes, and then create a patch relative to the clj-99 branch. Sorry that's a pain - usually patches don't collide at this level of conflict.

Comment by Nicholas Antonov [ 12/May/16 6:14 AM ]

The latest patch fixes min and max key in the same way, but based over CLJ-99, only evaluating the function once for each item.





[CLJ-1885] data/diff does not return a tuple when comparing different maps Created: 16/Jan/16  Updated: 16/Jan/16

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

Type: Defect Priority: Minor
Reporter: Eric Dvorsak Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

all


Attachments: Text File CLJ-1885.patch     Text File CLJ-1885-tests.patch    
Approval: Triaged

 Description   

Problem: clojure.data/diff inconsistently returns a lazy seq when comparing different maps, but a vector otherwise.

user> (data/diff {:a 1 :b 2} {:a 1})
({:b 2} nil {:a 1})

This is inconsistent with doc and normal behavior :

user> (data/diff {:a 1 :b 2} {:a 1 :b 2})
[nil nil {:a 1, :b 2}]
user> (data/diff #{1 2 3} #{1 2 3})
[nil nil #{1 3 2}]
user> (data/diff #{1 2 3} #{1 2})
[#{3} nil #{1 2}]

The docstring states: "Recursively compares a and b, returning a tuple of [things-only-in-a things-only-in-b things-in-both]", implying that it should always return a vector.



 Comments   
Comment by Eric Dvorsak [ 16/Jan/16 10:02 AM ]

Fixing it just requires to vectorize diff-associative output like this :

(defn- diff-associative
  "Diff associative things a and b, comparing only keys in ks."
  [a b ks]
  (vec (reduce
   (fn [diff1 diff2]
     (doall (map merge diff1 diff2)))
   [nil nil nil]
   (map
    (partial diff-associative-key a b)
    ks))))
Comment by Alex Miller [ 16/Jan/16 10:10 AM ]

There are other potential ways to address this, such as by using transducers instead. Not sure if that's worth doing, but seems reasonable to consider while we're making changes.

Comment by Eric Dvorsak [ 16/Jan/16 10:15 AM ]

Maybe this could be done as an improvement and proposed in an other ticket.

Vec is already used to vectorize the lists in diff-sequential. I would suggest to just fix the bug and add the test cases that should have screen it.

Comment by Eric Dvorsak [ 16/Jan/16 10:20 AM ]

There is a test case that should already fail :

[{:a #{2}} {:a #{4}} {:a #{3}}] {:a #{2 3}} {:a #{3 4}}

I get

({:a #{2}} {:a #{4}} {:a #{3}})
Comment by Alex Miller [ 16/Jan/16 10:33 AM ]

The test may need to be made more strict, checking not just for sequential equality but also for a returned vector.

Just curious - was this issue causing a problem in your code or did you just notice it and find it surprising?

Comment by Eric Dvorsak [ 16/Jan/16 11:05 AM ]

Simple patch that just does for maps what is done for lists : Creates a new vector with the vec function.

Comment by Eric Dvorsak [ 16/Jan/16 11:08 AM ]

@Alex Miller : I noticed a bug in my program behavior and traced it down to a (get diff 2) instead of (nth diff 2), but I realized that it was only buggy in some cases so I looked further and found out if was coming from diff.

Comment by Eric Dvorsak [ 16/Jan/16 11:27 AM ]

More strict tests checking for a returned vector.





[CLJ-1867] with-redefs used on a macro permanently changes it to a function Created: 10/Dec/15  Updated: 10/Dec/15

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

Type: Defect Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

If you use with-redefs to redefine a macro (which is likely a mistake), the macro loses its macro status after the with-redefs call completes.

Presumably the fix depends on whether we think there is a valid use of with-redefs on a macro (which would only work if you're calling eval or equivalent in the body, and would require knowing enough about what you're doing to add the two extra macro args to your function) – if so, we would keep it from losing the macro status; if not, we might also have it throw an exception if you accidentally use it on a macro.

Demonstration of the effect:

user> (defmacro kwote [arg] `(quote ~arg))
#'user/kwote
user> (kwote hello)
hello
user> kwote
CompilerException java.lang.RuntimeException: Can't take value of a macro: #'user/kwote, compiling:(/tmp/form-init6222001939841513290.clj:1:18983)

;; Everything above is as expected

user> (with-redefs [kwote (constantly :in-with-redefs)] (kwote with-redefs-body))
with-redefs-body
user> (kwote hello)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: hello in this context, compiling:(/tmp/form-init6222001939841513290.clj:1:1) 
user> (kwote :arg-1)
ArityException Wrong number of args (1) passed to: user/kwote  clojure.lang.AFn.throwArity (AFn.java:429)
user> (kwote :arg-1 :arg-2 :arg-3)
(quote :arg-3)
user> kwote
#object[user$kwote 0x37e32ff6 "user$kwote@37e32ff6"]


 Comments   
Comment by Gary Fredericks [ 10/Dec/15 12:04 PM ]

Looks like the root cause is that with-redefs uses Var#bindRoot which intentionally clears the macro flag: https://github.com/clojure/clojure/blob/5cfe5111ccb5afec4f9c73b46bba29ecab6a5899/src/jvm/clojure/lang/Var.java#L270





[CLJ-1796] Protocol functions fail to find future extensions when assigned to a local or new var Created: 08/Aug/15  Updated: 10/Aug/15

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

Type: Defect Priority: Minor
Reporter: Nathan Marz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: protocols

Approval: Triaged

 Description   
(defprotocol TestProtocol
  (tester [o]))

(let [t tester]
  (defn another-tester [o]
  	(t o)))

(def another-tester2 tester)

(extend-protocol TestProtocol
  String
  (tester [o] (println "Strings work!")))

(another-tester "A") ;; Error
(another-tester2 "A") ;; Error
(tester "A") ;; Works fine

(let [t tester]
  (defn another-tester [o]
  	(t o)))

(another-tester "A") ;; Works fine

(def another-tester2 tester)

(another-tester2 "A") ;; Works fine

(extend-protocol TestProtocol
  Long
  (tester [o] (println "Longs work!")))

(another-tester "A") ;; Works fine
(another-tester 3) ;; Error
(another-tester2 3) ;; Error


 Comments   
Comment by Nathan Marz [ 08/Aug/15 12:47 PM ]

This issue appears to be Clojure specific – I did some testing in CLJS and was unable to reproduce the issue.

Comment by Ghadi Shayban [ 09/Aug/15 9:51 AM ]

Nathan,
Not sure if you tried this, but using:

(def another-handle #'the-protocol-function)
rather than dereffing outright.

Comment by Nathan Marz [ 09/Aug/15 6:25 PM ]

That's a good workaround but it does seem that my test case should work. I ran into this because I was passing around functions dynamically and saving them for later execution – and this issue popped up with protocol methods. Having to pass around protocol methods differently than regular functions doesn't seem right.

Comment by Kevin Downey [ 10/Aug/15 11:21 AM ]

this is a result of the protocol implementation in clojure, protocol extension mutates the vars, once you have taken then value of the var (which happens once for top level forms) you will not see further mutations of the var so no more protocol extension





[CLJ-1768] quote of an empty lazyseq produces an error when evaled Created: 24/Jun/15  Updated: 26/Apr/16

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

Type: Defect Priority: Minor
Reporter: Tim Engler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   
user=> (eval `'())
()
user=> `'~(map identity ())
(quote ())
user=> (eval `'~(map identity ()))    ;; expected: ()
CompilerException java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)
user=> (prn *e)
#error {
 :cause "Unknown Collection type"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "java.lang.UnsupportedOperationException: Unknown Collection type, compiling:(NO_SOURCE_PATH:5:1)"
   :at [clojure.lang.Compiler analyzeSeq "Compiler.java" 6730]}
  {:type java.lang.UnsupportedOperationException
   :message "Unknown Collection type"
   :at [clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]}]
 :trace
 [[clojure.lang.Compiler$EmptyExpr emit "Compiler.java" 2929]
  [clojure.lang.Compiler$BodyExpr emit "Compiler.java" 5905]
  [clojure.lang.Compiler$FnMethod doEmit "Compiler.java" 5453]
  [clojure.lang.Compiler$FnMethod emit "Compiler.java" 5311]
  [clojure.lang.Compiler$FnExpr emitMethods "Compiler.java" 3843]
  [clojure.lang.Compiler$ObjExpr compile "Compiler.java" 4489]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 3983]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 6721]
  [clojure.lang.Compiler analyze "Compiler.java" 6524]
  [clojure.lang.Compiler eval "Compiler.java" 6779]
  [clojure.lang.Compiler eval "Compiler.java" 6745]
  [clojure.core$eval invoke "core.clj" 3081]
  ;; elided rest
nil
user=> (eval `'~(map identity '(x)))
(x)

Cause: In the empty list case, the compiler here sees a LazySeq. I suspect something earlier in the stack should be producing an empty list instead, but haven't tracked it back yet.



 Comments   
Comment by Tim Engler [ 26/Apr/16 4:17 AM ]

Still exists in clojure 1.8





[CLJ-1733] print-dup form unreadable for sorted sets and maps Created: 19/May/15  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.6.0
Clojure 1.7.0-alpha5
Clojure 1.7.0-beta3

java version "1.8.0"
Java(TM) SE Runtime Environment (build 1.8.0-b132)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b70, mixed mode)


Attachments: Text File clj-1733-tagged-literals-throw-on-sorted-set.patch    
Patch: Code and Test
Approval: Triaged

 Description   

print-dup for sorted sets and maps presume a nonexistent static create method that takes an IPersistentCollection

Printing

user=> (print-dup (sorted-set 1) *out*)
#=(clojure.lang.PersistentTreeSet/create [1])

Can't read back

(read-string "#=(clojure.lang.PersistentTreeSet/create [1])")
ClassCastException Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq  java.lang.Class.cast (Class.java:3356)

Possible Fixes

  • add create methods taking IPersistentVector to collections
  • emit something different from print-dup


 Comments   
Comment by Alex Miller [ 19/May/15 4:55 PM ]

It's trying to invoke PersistentTreeSet.create(ISeq) with ["123"]. It's not clear to me where the vector comes from?

Comment by Nikita Prokopov [ 19/May/15 5:04 PM ]

It’s a particular case of CLJ-1461. Vector comes from reading output of print-dup:

(defrecord Rec [f])

(binding [*print-dup* true]
  (prn (Rec. (sorted-set 1))))
;; => #tonsky.Rec[#=(clojure.lang.PersistentTreeSet/create [1])]

I already have a patch for PersistentTreeSet (attached here). Can look into CLJ-1461 later.





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

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

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

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

 Description   

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

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

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

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

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



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

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

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

Patch won't apply to master for me

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

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

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

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

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

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

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

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

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

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

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

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

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

Hi Adam,

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

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

Added failing tests which now pass





[CLJ-1708] Volatile mutable in deftype is not settable when using try..finally and returning this Created: 17/Apr/15  Updated: 31/Jul/15

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

Type: Defect Priority: Minor
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype
Environment:

clojure 1.6.0, clojure 1.7.0-beta1


Approval: Triaged

 Description   

Reproducible Code: https://gist.github.com/patrickgombert/1bcb8a051aeb3e82d855

When using a volatile-mutable field in deftype, compilation fails if the field is set! in a method call that uses both try..finally and returns itself from the method call. Leaving out either the try..finally or returning itself from the method causes compilation to succeed.

Expected behavior: set! should set the volatile-mutable variable and compilation should succeed.



 Comments   
Comment by Kevin Downey [ 17/Apr/15 7:15 PM ]

this must be the same issue as CLJ-1422 and CLJ-701, it has nothing to do with returning `this`, but with the try being in a tail position or not. if the try is not in a tail position the compiler hoists it out in to a thunk. effectively the code is

(deftype SomeType [^:volatile-mutable foo]
  SomeProtocol
  (someFn [_] ((fn [] (try (set! foo 1))))))

which the compiler also rejects, because it doesn't let you mutate fields from functions that are not the immediate protocol functions





[CLJ-1682] clojure.set/intersection occasionally allows non-set arguments. Created: 24/Mar/15  Updated: 14/Jul/15

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

Type: Defect Priority: Minor
Reporter: Valerie Houseman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs

Approval: Triaged

 Description   

clojure.set/intersection, by intent and documentation, is meant to be operations between two sets. However, it sometimes allows (and returns correct opreations upon) non-set arguments. This confuses the intention that non-set arguments are not to be used.

Here's an example with Set vs. KeySeq:
If there happens to be an intersection, you'll get a result. This may lead someone coding this to think that's okay, or to not notice they've used an incompatible data type. As soon as the intersection is empty, however, an appropriate type error ensues, albeit by accident because the first argument to clojure.core/disj should be a set.

user=> (require '[clojure.set :refer [intersection]])
nil
user=> (intersection #{:key_1 :key_2} (keys {:key_1 "na"}))   ;This works, but shouldn't
(:key_1)
user=> (intersection #{:key_1 :key_2} (keys {:key_3 "na"}))   ;This fails, because intersection assumes the second argument was a Set
ClassCastException clojure.lang.APersistentMap$KeySeq cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1449)

(disj (keys {:key_1 "na"}) #{:key_1 :key_2})   ;The assumption that intersection made
ClassCastException clojure.lang.APersistentMap$KeySeq cannot be cast to clojure.lang.IPersistentSet  clojure.core/disj (core.clj:1449)

Enforcing type security on a library that's clearly meant for a particular type seems like the responsible thing to do. It prevents buggy code from being unknowingly accepted as correct, until the right data comes along to step on the bear trap.



 Comments   
Comment by Andy Fingerhut [ 24/Mar/15 7:19 PM ]

CLJ-810 was similar, except it was for function clojure.set/difference. That one was declined with the comment "set/difference's behavior is not documented if you don't pass in a set." I do not know what core team will judge ought to be done with this ticket, but wanted to provide some history.

Dynalint [1] and I think perhaps Dire [2] can be used to add dynamic argument checking to core functions.

[1] https://github.com/frenchy64/dynalint
[2] https://github.com/MichaelDrogalis/dire

Comment by Alex Miller [ 24/Mar/15 9:00 PM ]

Now that `set` is faster for sets, I think we could actually add checking for sets in some places where we might not have before. So, it's worth looking at with fresh eyes.

Comment by Jason Wolfe [ 28/May/15 2:54 AM ]

Back in 2009 I submitted a patch to the set functions with explicit `set?` checks and Rich's response was "the fact that these functions happen to work when the second argument is not a set is an implementation artifact and not a promise of the interface, so I'm not in favor of the set? testing or any other accommodation of that." Not sure if that is still accurate though.





[CLJ-1680] quot and rem handle doubles badly Created: 24/Mar/15  Updated: 27/Jul/15

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

Type: Defect Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math

Attachments: Text File clj-1680_no_div0_jre17.patch    
Patch: Code and Test
Approval: Triaged

 Description   

quot and rem in the doubles case (where any one of the arguments is a floating point) gives strange results for non-finite arguments:

(quot Double/POSITIVE_INFINITY 2) ; Java: Infinity
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(quot 0 Double/NaN) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(quot Double/POSITIVE_INFINITY Double/POSITIVE_INFINITY) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(rem Double/POSITIVE_INFINITY 2) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(rem 0 Double/NaN) ; Java: NaN
NumberFormatException Infinite or NaN  java.math.BigDecimal.<init> (BigDecimal.java:808)
(rem 1 Double/POSITIVE_INFINITY) ; The strangest one. Java: 1.0
=> NaN

quot and rem also do divide-by-zero checks for doubles, which is inconsistent with how doubles act for division:

(/ 1.0 0)
=> NaN
(quot 1.0 0) ; Java: NaN
ArithmeticException Divide by zero  clojure.lang.Numbers.quotient (Numbers.java:176)
(rem 1.0 0); Java: NaN
ArithmeticException Divide by zero  clojure.lang.Numbers.remainder (Numbers.java:191)

Attached patch does not address this because I'm not sure if this is intended behavior. There were no tests asserting any of the behavior mentioned above.

Fundamentally the reason for this behavior is that the implementation for quot and rem sometimes (when result if division larger than a long) take a double, coerce it to BigDecimal, then BigInteger, then back to a double. The coersion means it can't handle nonfinite intermediate values. All of this is completely unnecessary, and I think is just leftover detritus from when these methods used to return a boxed integer type (long or BigInteger). That changed at this commit to return primitive doubles but the method body was not refactored extensively enough.

The method bodies should instead be simply:

static public double quotient(double n, double d){
    if(d == 0)
        throw new ArithmeticException("Divide by zero");
    double q = n / d;
    return (q >= 0) ? Math.floor(q) : Math.ceil(q);
}

static public double remainder(double n, double d){
    if(d == 0)
        throw new ArithmeticException("Divide by zero");
    return n % d;
}

Which is what the attached patch does. (And I'm not even sure the d==0 check is appropriate.)

Even if exploding on non-finite results is a desirable property of quot and rem, there is no need for the BigDecimal+BigInteger coersion. I can prepare a patch that preserves existing behavior but is more efficient.

More discussion at Clojure dev.



 Comments   
Comment by Francis Avila [ 24/Mar/15 12:55 PM ]

More testing revealed that n % d does not preserve the relation (= n (+ (* d (quot n d)) (rem n d))) as well as (n - d * (quot n d)), which doesn't make sense to me since that is the very relation the spec says % preserves. % is apparently not simply Math.IEEEremainder() with a different quotient rounding.

Test case: (rem 40.0 0.1) == 0.0; 40.0 % 0.1 == 0.0999... (Smaller numerators will still not land at 0 precisely, but land closer than % does.)

Updated patch which rolls back some parts of the simplification to remainder and adds this test case.

Comment by Andy Fingerhut [ 04/Jul/15 12:12 AM ]

Francis, your patch clj-1680_no_div0.patch dated 2015-Mar-24 uses the method isFinite(), which appears to have been added in Java 1.8, and does not exist in earlier versions. I would guess that while the next release of Clojure may drop support for Java 1.6, it is less likely it would also drop support for Java 1.7 at the same time. It might be nice if your patch could use something like !(isInfinite() || isNaN()) instead, which I believe is equivalent, and both of those methods exist in earlier Java versions.

Comment by Francis Avila [ 26/Jul/15 11:22 PM ]

Updated patch with a java 1.7-compatible version, also rebased against master.

No tests fail except this one, which I don't think is related to this patch:

[java] FAIL in (gen-interface-source-file) (genclass.clj:151)
     [java] expected: (= "examples.clj" (str sourceFile))
     [java]   actual: (not (= "examples.clj" ""))
Comment by Michael Blume [ 27/Jul/15 1:34 PM ]

Francis, I tried downloading your patch and I didn't see any test failures at all. Do you see the same failure if you check out the master branch from the Clojure repo? Do you still see it if you mvn clean first? If so, it might be worth opening a ticket for it and seeing if anyone else can reproduce it.

Comment by Andy Fingerhut [ 27/Jul/15 1:41 PM ]

Yes, and if you do see a failure with unmodified Clojure for 'mvn clean test', or './antsetup.sh ; ant clean; ant', please let us know the OS and JVM you are using. I haven't seen that on the OS/JVM combos I have tried.

Comment by Francis Avila [ 27/Jul/15 2:51 PM ]

Nevermind, failing test went away after a clean. All tests pass.





[CLJ-1649] Hash/equality inconsistency for floats & doubles Created: 23/Jan/15  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Michael Gardner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: numerics

Approval: Triaged

 Description   

This is closely related to CLJ-1036, but there was a suggestion to make a new ticket.

The issue is that for a float f and a double d, we can have (= f d) but not (= (hash f) (hash d)), which breaks a fundamental assumption about hash/equality consistency and leads to weirdness like this (from Immo Heikkinen's email to the Clojure mailing list):

(= (float 0.5) (double 0.5))
=> true
(= #{(float 0.5)} #{(double 0.5)})
=> true
(= {:a (float 0.5)} {:a (double 0.5)})
=> true
(= #{{:a (float 0.5)}} #{{:a (double 0.5)}})
=> false

One way to resolve this would be to tweak the hashing of floats and/or doubles, but that suggestion has apparently been rejected.

An alternative would be to modify = so that it never returns true for float/double comparisons. One should never compare floats with doubles using = anyway, so such a change should have minimal impact beyond restoring hash/equality consistency.






[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring, errormsgs

Attachments: Text File CLJ-1630-v2.patch     Text File no-multiple-rest-params-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

(let [[foo & bar & baz] []]) compiles and probably shouldn't.



 Comments   
Comment by Alex Miller [ 01/Jan/15 10:17 AM ]

I see:

user=> (defn foo [bar & baz & qux])

CompilerException java.lang.RuntimeException: Invalid parameter list, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init3743582784321941885.clj:1:1)

?

Comment by Michael Blume [ 01/Jan/15 12:27 PM ]

Sorry, I was working on memory rather than actually typing the thing I put in the description into a REPL, which was dumb.

user=> (let [[foo & bar & baz] []])
nil





[CLJ-1579] source-fn can fail due to reading namespace-aliased keywords while in another namespace context Created: 05/Nov/14  Updated: 20/Nov/15

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

Type: Defect Priority: Minor
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Read-src-in-appropriate-ns-context.patch    
Patch: Code
Approval: Triaged

 Description   

clojure.repl/source-fn functions by using a custom reader to read a source form at the location specified by line & file metadata on a given symbol. While this works well for most things, I encountered an issue when applying source-fn to code containing keyword namespace aliases ala ::T/foo. ::T/foo is a legitimate namespace keyword in the context where it occurs, because a namespace alias to T is created in the ns header. When the keyword ::T/foo is read then, it resolves to :my-other.ns/foo as one would expect because ns has the appropriate alias. However when attempting to read source via clojure.repl/source-fn, ns may no longer be the original read context of the indicated form thus leading to the erroneous exception java.lang.RuntimeException: Invalid token: ::T/foo.

The solution is that the reading operation of clojure.repl/source-fn must be wrapped in (binding [*ns* (.ns v)] ...) so that source reading will take place in the original load reading context thus preventing this error.

A patched equivalent function exists here, https://github.com/clojure-grimoire/lein-grim/blob/master/src/grimoire/doc.clj#L50-L74, and I will submit a patch against 1.6.0 in the morning.



 Comments   
Comment by Reid McKenzie [ 20/Nov/15 2:29 PM ]

Patch no longer applied to master, updated.





[CLJ-1492] PersistentQueue objects are improperly eval'd and compiled Created: 06/Aug/14  Updated: 07/Aug/14

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

Type: Defect Priority: Minor
Reporter: Jon Distad Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

OS X 10.9.4
java version "1.7.0_60"
Java(TM) SE Runtime Environment (build 1.7.0_60-b19)
Java HotSpot(TM) 64-Bit Server VM (build 24.60-b09, mixed mode)


Attachments: Text File 0001-Exclude-PersistentQueue-from-IPersistentList-eval-co.patch    
Patch: Code and Test
Approval: Triaged

 Description   

PersistentQueue objects do not follow the correct evaluation path in the Compiler.

The simplest case:

user=> (def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
#'user/q
user=> q
#<PersistentQueue clojure.lang.PersistentQueue@7861>
user=> (eval q)
CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:4:1)

And you get the same exception when embedding a PersistentQueue:

user=> (eval `(fn [] ~q))
CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:2:1)

Instead of the expected:

CompilerException java.lang.RuntimeException: Can't embed unreadable object in code: #<PersistentQueue clojure.lang.PersistentQueue@7861>, compiling:(NO_SOURCE_PATH:3:1)

Since PersistentQueue implements IPersistentCollection and IPersistentList, and is not called out explicitly in the compiler, it is falling into the same compile path as a list. The exception comes from the call to emitValue inside the emitConstants portion of the FnExpr emit path. PersistentQueue does not implement java.util.List and thus the cast in emitListAsObjectArray (Compiler.java:4479) throws. Implementing List would NOT, however, resolve this issue, but would mask it by causing all eval'd PersistedQueues to be compiled as PersistentLists.

The first case is resolved by adding `&& !(form instanceof PersistentQueue)` to the IPersistentCollection branch of Compiler.eval() (Compiler.java:6695-8), allowing the PersistentQueue to fall through to the ConstantExpr case in analyze (Compiler.java:6459). The embedding case is resolved by adding `&& !(value instanceof PersistentQueue)` to the IPersistentList branch in ObjExpr's emitValue (Compiler.java:4639).

This bug also precludes definition of data-readers for PersistentQueue as the read object throws an exception when it is passed to the Compiler.

The attached patch includes the two changes mentioned above, and tests for each case that illustrates the bug.

Clojure-dev thread: https://groups.google.com/forum/#!topic/clojure-dev/LDUQfqjFg9w






[CLJ-1475] :post condition causes compiler error with recur Created: 25/Jul/14  Updated: 29/Jul/14

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: File clj-1475.diff    
Patch: Code and Test
Approval: Triaged

 Description   

Michael O'Keefe <michael.p.okeefe@gmail.com> posted on the mailing list an example of code that causes a compiler error only if a :post condition is added. Here's my slightly modified version:

(defn g
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (if (seq xs)
     (recur (next xs) (+ (first xs) acc))
     acc))

CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position

The work-around is to wrap the body in a loop that simply rebinds the original args.



 Comments   
Comment by Steve Miner [ 25/Jul/14 9:53 AM ]

A macro expansion shows that body is placed in a let form to capture the result for later testing with the post condition, but the recur no longer has a proper target. The work-around of using a loop form is easy once you understand what's happening but it's a surprising limitation.

Comment by Steve Miner [ 25/Jul/14 9:55 AM ]

Use a local fn* around the body and call it with the original args so that the recur has a proper target. Update: not good enough for handling destructuring. Patch withdrawn.

Comment by Michael Patrick O'Keefe [ 25/Jul/14 10:37 AM ]

Link to the original topic discussion: https://groups.google.com/d/topic/clojure/Wb1Nub6wVUw/discussion

Comment by Steve Miner [ 25/Jul/14 1:42 PM ]

Patch withdrawn because it breaks on destructured args.

Comment by Steve Miner [ 25/Jul/14 5:27 PM ]

While working on a patch, I came up against a related issue: Should the :pre conditions apply to every recur "call". Originally, I thought the :pre conditions should be checked just once on the initial function call and never during a recur. People on the mailing list pointed out that the recur is semantically like calling the function again so the :pre checks are part of the contract. But no one seemed to want the :post check on every recursion, so the :post would happen only at the end.

That means automatically wrapping a loop (or nested fn* call) around the body is not going to work for the :pre conditions. A fix would have to bring the :pre conditions inside the loop.

Comment by Steve Miner [ 26/Jul/14 8:54 AM ]

I'm giving up on this bug. My approach was adding too much complexity to handle an edge case. I recommend the "loop" work-around to anyone who runs into this problem.

(defn g2
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (loop [xs xs acc acc]
    (if (seq xs)
       (recur (next xs) (+ (first xs) acc))
       acc)))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 10:29 AM ]

Add patch that handles rest arguments and destructuring.

Comment by Michael Patrick O'Keefe [ 26/Jul/14 10:57 AM ]

With regard to Steve's question on interpreting :pre, to me I would expect g to act like the case g3 below which uses explicit recursion (which does work and does appear to check the :pre conditions each time and :post condition once):

(defn g3
  [xs acc]
  {:pre [(or (sequential? xs) (nil? xs)) (number? acc)]
   :post [(number? %)]}
  (if (seq xs)
    (g3 (next xs) (+ (first xs) acc))
    acc))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 11:42 AM ]

Patch clj-1475.diff handles destructuring, preconditions and rest arguments

Comment by Steve Miner [ 26/Jul/14 4:04 PM ]

The clj-1475.diff patch looks good to me.

Comment by Alex Miller [ 27/Jul/14 7:18 AM ]

Please don't use "patch" as a label - that is the purpose of the Patch field. There is a list of good and bad labels at http://dev.clojure.org/display/community/Creating+Tickets

Comment by Steve Miner [ 27/Jul/14 11:32 AM ]

More knowledgeable commenters might take a look at CLJ-701 just in case that's applicable to the proposed patch.

Comment by Kevin Downey [ 29/Jul/14 1:35 AM ]

re clj-701

it is tricky to express loop expression semantics in jvm byte code, so the compiler sort of punts, hoisting expression loops in to anonymous functions that are immediately invoked, closing over whatever is in scope that is required by the loop, this has some problems like those seen in CLJ-701, losing type data which the clojure compiler doesn't track across functions, the additional allocation of function objects (the jit may deal with that pretty well, I am not sure) etc.

where the world of clj-701 and this ticket collide is the patch on this ticket lifts the function body out as a loop expression, which without the patch in clj-701 will have the issues I listed above, but we already have those issues anywhere something that is difficult to express in bytecode as an expression (try and loop) is used as an expression, maybe it doesn't matter, or maybe clj-701 will get fixed in some way to alleviate those issues.

general musings

it seems like one feature people like from asserts is the ability to disable them in production (I have never actually seen someone do that with clojure), assert and :pre/:post have some ability to do that (it may only work at macroexpansion time, I don't recall) since the hoisting of the loop could impact performance it might be nice to have some mechanism to disable it (maybe using the same flag assert does?).





[CLJ-1317] clojure.zip/seq-zip returns spurious nils during traversal Created: 31/Dec/13  Updated: 10/Feb/16

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

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: zip

Attachments: Text File 0001-CLJ-1317-fix-seq-zip-to-avoid-spurious-nils.patch    
Patch: Code
Approval: Triaged

 Description   

Problem reported by Lee Spector on the mailing list:

https://groups.google.com/d/msg/clojure/8TL7IGmE7N0/u1xfgTOLDRgJ

Here's a quote from Lee's post describing the problem:

Here's an illustration, stepping through '(() 0) with next and printing the node at each step: 

(loop [z (zip/seq-zip '(() 0))] 
  (if (zip/end? z) 
    :done 
    (do (println (zip/node z)) 
      (recur (zip/next z))))) 

That produces: 

(() 0) 
() 
nil 
0 
:done 

I don't expect the nil to be there. 

The underlying cause is that seq-zip passes identity as the children argument to zipper. Applied to (), this returns (), which is truthy, leading zipper to descend into a non-existent subtree.

One natural solution would be to use seq in place of identity:

(defn seq-zip [root]
  (zipper seq?
          seq  ;; changed
          (fn [node children] (with-meta children (meta node)))
          root))

With this change, no nil is produced in the example above. Patch with this change forthcoming.



 Comments   
Comment by Michał Marczyk [ 31/Dec/13 5:52 PM ]

Note that the docstring of clojure.zip/zipper asks that the children argument return a seq of children. The rest of clojure.zip, however, expects nil to be returned when there are no children, as evidenced by this problem.

One could argue that this behaviour of the rest of clojure.zip should be fixed, but I think it makes sense and is convenient. Perhaps the docstring should be adjusted, though.

Comment by Alex Miller [ 08/Feb/16 4:36 PM ]

Michał, can I ask why you assigned this to yourself - was there something you planned to add?

Comment by Michał Marczyk [ 10/Feb/16 9:09 AM ]

Hey Alex, I was going to attach a separate patch with a proposal for a docstring adjustment along the lines suggested above (will do that tonight). No change to the code, though, and I guess not worth assigning the ticket – sorry about the unnecessary ping.

Comment by Alex Miller [ 10/Feb/16 9:38 AM ]

No worries, just wanted to know if something was still pending - I will wait to prescreen it.





[CLJ-1254] Incorrect long quot result involving Long/MIN_VALUE Created: 06/Sep/13  Updated: 04/Oct/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math

Attachments: File clj-1254-2.diff    
Patch: Code and Test
Approval: Triaged

 Description   
user=> (quot Long/MIN_VALUE -1)
-9223372036854775808

Similar issue to CLJ-1222 and CLJ-1253, with the same root cause as described for CLJ-1225. Ticket filed separately from CLJ-1253 for long division / because the desired fix may be quite different in this case.

Rich Hickey stated in a comment on CLJ-1225 that this case should throw an exception.

Question: For inc (which throws when given input Long/MAX_VALUE) there is an auto-promoting inc' and an unchecked-inc. quot now throws an exception in this case. Should there be an auto-promoting quot' and an unchecked-quot?



 Comments   
Comment by Andy Fingerhut [ 06/Sep/13 10:55 AM ]

Patch clj-1254-v1.txt causes (quot Long/MIN_VALUE -1) to throw an exception due to overflow of the result, if the arguments are both long.

Unlike inc, which has auto-promoting version inc' and unchecked version unchecked-inc, there is no auto-promoting quot' and unchecked unchecked-quot. This patch does not add one.

Should quot' and unchecked-quot be added? If so, this ticket or a separate one?

Comment by Andy Fingerhut [ 23/Nov/13 12:59 AM ]

Patch clj-1254-2.diff is identical to clj-1254-v1.txt except it applies cleanly to latest master. The only changes were in the context of the lines that were changed, due to a recent commit made.

Comment by Alex Miller [ 04/Oct/14 10:23 PM ]

quot should throw an an exception on overflow
quot' (I assume not divide' ?) should be added to autopromote on overflow
unchecked-divide should be added to do what quot does now - see CLJ-1545





[CLJ-1253] Incorrect long division involving Long/MIN_VALUE Created: 06/Sep/13  Updated: 04/Oct/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: math

Attachments: Text File clj-1253-1.txt    
Patch: Code and Test
Approval: Triaged

 Description   
user=> (/ Long/MIN_VALUE -1)
-9223372036854775808

Similar issue to CLJ-1222, with the same root cause as described for CLJ-1225.



 Comments   
Comment by Andy Fingerhut [ 06/Sep/13 8:56 AM ]

Patch clj-1253-1.txt corrects LongOps method divide for the case of args Long/MIN_VALUE and -1. It returns a BigInt in this case, not a Long, but most other pairs of values passed to this function return a Ratio exact answer, so it seems reasonable in this one case to return a BigInt exact answer when it will not fit in a Long.





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

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: collections

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

 Description   

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

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

The latter case should return false instead of throwing.

Cause: APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.



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

PersistentVector also has the same problem.

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

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

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

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

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

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

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

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

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

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

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

user=> (def s (java.util.TreeSet.))
#'user/s
user=> (.add s 1)
true
user=> (.contains s "a")
ClassCastException java.lang.Long cannot be cast to java.lang.String  java.lang.String.compareTo (String.java:108)




[CLJ-1142] Incorrect divide-by-zero error with floating point numbers Created: 08/Jan/13  Updated: 21/Jul/15

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: math

Approval: Triaged

 Description   

The unary call for clojure.core// treats a dividend of 0.0 differently than the binary call, likely due to inlining.

(/ 0.0) ;; java.lang.ArithmeticException: Divide by zero
(/ 1 0.0) ;;= Infinity
(/ 1 (identity 0.0)) ;; java.lang.ArithmeticException: Divide by zero


 Comments   
Comment by Tim McCormack [ 08/Jan/13 11:22 PM ]

The relevant code seems to be this in clojure.lang.Numbers/divide:

if(yops.isZero((Number)y))
  throw new ArithmeticException("Divide by zero");

Making Numbers/divide be more restrictive than double arithmetic seems like a bug; explicitly throwing an ArithmeticException instead of letting the JVM figure it just seems like more work than necessary.





[CLJ-1138] data-reader returning nil causes exception Created: 22/Dec/12  Updated: 02/Aug/15

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reader
Environment:

clojure 1.5 beta2, Mac OS X 10.8.2, java version "1.6.0_37"


Attachments: Text File 0001-CLJ-1139-allow-nil-in-data-reader.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If a data-reader returns nil, the reader throws java.lang.RuntimeException: No dispatch macro... The error message implies that there is no dispatch macro for whatever the first character of the tag happens to be.

Here's a simple example:

(binding [*data-readers* {'f/ignore (constantly nil)}] (read-string "#f/ignore 42 10"))

RuntimeException No dispatch macro for: f clojure.lang.Util.runtimeException (Util.java:219)



 Comments   
Comment by Steve Miner [ 22/Dec/12 9:43 AM ]

clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch allows a data-reader to return nil instead of throwing. Does sanity check that possible tag or record isJavaIdentifierStart(). Gives better error message for special characters that might actually be dispatch macros (rather than assuming it's a tagged literal).

Comment by Steve Miner [ 22/Dec/12 10:06 AM ]

clj-1138-data-reader-return-nil-for-no-op.patch allows a data-reader returning nil to be treated as a no-op by the reader (like #_). nil is not normally a useful value (actually it causes an exception in Clojure 1.4 through 1.5 beta2) for a data-reader to return. With this patch, one could get something like a conditional feature reader using data-readers.

Comment by Steve Miner [ 22/Dec/12 10:26 AM ]

clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch is the first patch to consider. It merely allows nil as a value from a data-reader and returns nil as the final value. I think it does what was originally intended for dispatch macros, and gives a better error message in many cases (mostly typos).

The second patch, clj-1138-data-reader-return-nil-for-no-op.patch, depends on the other being applied first. It takes an extra step to treat a nil value returned from a data-reader as a no-op for the reader (like #_).

Comment by Steve Miner [ 23/Dec/12 11:52 AM ]

It turns out that you can work around the original problem by having your data-reader return '(quote nil) instead of plain nil. That expression conveniently evaluates to nil so you can get a nil if necessary. This also works after applying the patches so there's still a way to return nil if you really want it.

(binding [*data-readers* {'x/nil (constantly '(quote nil))}] (read-string "#x/nil 42"))
;=> (quote nil)

Comment by Andy Fingerhut [ 07/Feb/13 9:20 AM ]

Patch clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch dated Dec 22 2012 still applies cleanly to latest master if you use the following command:

% git am --keep-cr -s --ignore-whitespace < clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch

Without the --ignore-whitespace option, the patch fails only because some whitespace was changed in Clojure master recently.

Comment by Andy Fingerhut [ 13/Feb/13 11:24 AM ]

OK, now with latest master (1.5.0-RC15 at this time), patch clj-1138-allow-data-reader-to-return-nil-instead-of-throwing.patch no longer applies cleanly, not even using --ignore-whitespace in the 'git am' command given above. Steve, if you could see what needs to be updated, that would be great. Using the patch command as suggested in the "Updating stale patches" section of http://dev.clojure.org/display/design/JIRA+workflow wasn't enough, so it should probably be carefully examined by hand to see what needs updating.

Comment by Steve Miner [ 14/Feb/13 12:21 PM ]

I removed my patches. Things have changes recently with the LispReader and new EdnReader.





[CLJ-1074] Read/print round-trip for +/-Infinity and NaN Created: 21/Sep/12  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: print, reader

Attachments: Text File 0001-Read-Infinity-and-NaN.patch     Text File clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch    
Patch: Code and Test
Approval: Triaged

 Description   

A few float-related forms (namely, Double/POSITIVE_INFINITY, Double/NEGATIVE_INFINITY, Double/NaN) are not eval-able after a round-trip via

(read-string (binding [*print-dup* true] (pr-str f))

The two options I see are to provide print-method implementations for these and their Float cousins, or to make Infinity, -Infinity, +Infinity, and NaN readable values. Since it sounds like edn may want to provide a spec for these values (see https://groups.google.com/d/topic/clojure-dev/LeJpOhHxESs/discussion and https://github.com/edn-format/edn/issues/2), I think making these values directly readable as already printed is preferable. Something like Double/POSITIVE_INFINITY seems too low-level from edn's perspective, as it would refer to a Java class and constant.

I'm attaching a patch implementing reader support for Infinity, -Infinity, +Infinity, and NaN.



 Comments   
Comment by Timothy Baldridge [ 03/Dec/12 11:34 AM ]

Please bring this up on clojure-dev. We'll be able to vet this ticket after that.

Comment by Colin Jones [ 03/Dec/12 1:18 PM ]

Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?

Comment by Andy Fingerhut [ 24/May/13 1:11 PM ]

Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.

Comment by Nicola Mometto [ 25/May/13 11:55 AM ]

clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch is the same as clj-1074-read-infinity-and-nan-patch-v2.txt except it patches EdnReader too, but it must be applied after #CLJ-873 0001-Fix-CLJ-873-for-EdnReader-too.patch get merged

Comment by Andrew Tarzwell [ 12/Feb/15 12:01 PM ]

We're running into this bug now, applying the patch clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch seems to resolve it on 1.7 master, but it would be an improvement to not depend on a patched version.

Is there a fix in the works? Or a more up to date conversation on why this hasn't been addressed?

Thanks,
Andrew

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

Andrew, the tools.reader library provides this enhancement today, if you would prefer using unpatched software, and if it meets your needs. There are a few other small differences between it and the reader built into Clojure, which you can see near the end of its README: https://github.com/clojure/tools.reader

As far as why it hasn't been addressed yet, I think a short accurate answer is that the Clojure developers have been working on other issues, and this one has not been high enough priority to be addressed yet (disclaimer: This is in no way an official answer, just the best guess of an observer who watches this stuff too much).

I see you have voted on the ticket. Good. More votes can in some cases influence the Clojure developers to respond to a ticket earlier rather than later. You may try to persuade others to vote on it, too, if you wish.

If there is some production use of Clojure hindered by the lack of a fix, bringing this up in the Clojure or Clojure Dev Google groups couldn't hurt (signed CA on file required for Clojure Dev group membership – see http://clojure.org/contributing )

Comment by Andrew Tarzwell [ 12/Feb/15 1:46 PM ]

Andy,

Thank you for the quick response, I was unaware of tools.reader having this fixed. That should do us for now.





[CLJ-1029] ns defmacro allows arbitrary execution of clojure.core fns Created: 23/Jul/12  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Craig Brozefsky Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: error-reporting
Environment:

all


Attachments: File ns-patch.diff    
Patch: Code
Approval: Triaged

 Description   

The form:

(ns foo (:print "I AM A ROBOT"))

will print "I AM A ROBOT"

This is because the defmacro takes the name of the first element of the reference, looks it up in the clojure.core namespace and invokes it on the rest of the args.

This is minor, but it does mean that an otherwise declarative form is not executing code.



 Comments   
Comment by Alan Malloy [ 25/Jul/12 4:37 PM ]

One apparent problem with this patch is that you throw an exception for :refer. You should add that, and make sure there aren't any others missing. Also, #{x y z} is better than (set [x y z]), and you should probably use pr-str rather than str, although I can't think of a case where it matters for the objects in question.

Comment by Andy Fingerhut [ 26/Jul/12 6:31 PM ]

A more minor detail of patch formatting – please attach your patch in git format. See the instructions under the section heading "Development" on this web page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Craig Brozefsky [ 05/Aug/12 9:53 AM ]

git format-patch version of the diff, with the edits suggested by other maintainers.

Comment by Craig Brozefsky [ 05/Aug/12 10:00 AM ]

Alan: please note that :refer was not mentioned in the docstring for ns, or used in any of the unit tests for clojure.

Are you sure that it is an expected argument, or just an arrangement that happens to work under the current ns macro? The docstring for 'refer itself says to use :use in ns macros instead of calling refer.

I added "refer" to the set of accepted references all the same.

Comment by Alex Miller [ 18/Jan/16 3:33 PM ]

This is a case where better error checking would prevent this problem.





[CLJ-929] Accessing Java property starting with _ has issues in 1.4 Created: 07/Feb/12  Updated: 21/Jul/15

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

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

Approval: Triaged

 Description   

When attempting to use interop syntax with symbols which aren't legal Java names (such as deleted?), the names are mangled a bit. That's necessary, of course, and the method of munging can be internal to the compiler. However, the behavior when munging changed a little between 1.3 and 1.4 beta1. Obviously the specifics of munging are something I should avoid relying on, but the way it changed looks like an accident or a bug even so.

The use-case I ran into is that defrecords contain a field named __meta for tracking their metadata. In both 1.3 and 1.4 you can get at that field with (. record __meta), which avoids munging. But on 1.3 (. record --meta) also accesses it (translating each - to a _), while on 1.4 (. record -meta) works and (. record --meta) doesn't.

Actually, looking at line 883 of Compiler.java, it looks like this may be related to the (. foo -property) syntax ported from CLJS, and indeed (. record ---meta) works, I guess by reducing to an "old style" (. record --meta). So that clears up why --meta fails: it's looking for __meta. I'm still not clear on why (. record -meta) works, though.

So it looks like the - prefix for properties is not 100% backwards-compatible like it seemed to be. Is this an issue we need to fix, or is the recommendation simply to never have fields that start with - or _?



 Comments   
Comment by Fogus [ 09/Feb/12 2:33 PM ]

Is this a general problem with fields starting with _ or just fields named __meta as in (defrecord [__meta] ...)

Comment by Alan Malloy [ 09/Feb/12 3:01 PM ]

It's a general issue. (defrecord [__meta]) actually breaks immediately, because the record mechanism itself generates a field named __meta, but any field named with a - or _ prefix has this issue.

user=> (defrecord Foo [-blah])
user.Foo
user=> (.-blah (Foo. 1))
IllegalArgumentException No matching field found: blah for class user.Foo clojure.lang.Reflector.getInstanceField (Reflector.java:289)





[ASYNC-113] alt!! should not evaluate the :default expr if a non-default clause is selected Created: 13/Jan/15  Updated: 27/Jul/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Caleb Spare Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.5.0, core.async 0.1.346.0-17112a-alpha


Approval: Triaged

 Description   

alt!! will always evaluate the expr given in the :default clause. For instance, consider this code:

(require '[clojure.core.async :as async])
(let [c1 (async/chan 1)
      c2 (async/chan 1)]
  (async/alt!!
    [[c1 "a"]] (println "first")
    [[c2 "a"]] (println "second")
    :default (println "default")))

It will print either "first" or "second", but also (always) "default". Reading the documentation, technically it doesn't say whether the non-selected clauses are evaluated (only that the value of the selected expr is returned) but it's certainly not the behavior I would expect.

I haven't checked whether alt! does the same thing as well.



 Comments   
Comment by Daniel Compton [ 27/Jul/15 7:37 PM ]

It looks to me like :default is expected to be a value, not a function, and println is being run as part of evaluating the alt!! expression, not as the return value.





[CLJ-1972] issue with browse-url Created: 28/Jun/16  Updated: 28/Jun/16

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

Type: Defect Priority: Trivial
Reporter: David Siefert Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Check-for-zero-exit-code-to-consider-that-script-exe.patch     Text File 0002-Extracting-method-open-url-by-script-in-browse-url.patch     Text File 0003-Extracting-explaining-method-success-in-open-url-by-.patch    
Patch: Code
Approval: Triaged

 Description   

When xdg-utils are installed on my platform, and the xdg-open command fails, (clojure.java.browse/browse-url) ignores this error and silently fails. This fix will allow the (or ..) logic to continue evaluating to try the next method.






[CLJ-1818] cl-format does not respect aesthetic ~A with a keyword Created: 26/Sep/15  Updated: 12/Jan/16

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

Type: Defect Priority: Trivial
Reporter: Jong-won Choi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Approval: Triaged

 Description   

In Common Lisp, (format nil "~a" :A) returns "A". However, in Clojure, it returns with the colon:

(clojure.pprint/cl-format false "~a" :A)
=> ":A"


 Comments   
Comment by Jong-won Choi [ 28/Sep/15 6:26 AM ]

Found another problem of cl-format:

(clojure.pprint/cl-format false "SELECT * from RateSchedules ~@[WHERE ~{~A=?~^ ~}~]" '())
=> "SELECT * from RateSchedules WHERE" ;; instead of "SELECT * from RateSchedules"

I guess the problem is () or [] has to be treated as falsey but not.

Comment by Alex Miller [ 28/Sep/15 9:58 AM ]

:a is a keyword and I would expect it's ascii format to be :a. I'm not sure what case sensitivity has to do with it.

Comment by Andy Fingerhut [ 28/Sep/15 10:08 AM ]

Alex, case is a side issue. Common Lisp's (format nil "~a" :A) returns "A", not ":A". It is the presence of the colon in the output that is the issue, not the case of the string.

Comment by Jong-won Choi [ 28/Sep/15 4:41 PM ]

For a record, what Alex described is for ~S - standard. See http://www.lispworks.com/documentation/lw50/CLHS/Body/22_cd.htm





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

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

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

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

 Description   

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

With:

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

Before patch:

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

After patch:

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

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



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

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

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

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

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

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

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

is it possible that AOT is involved?

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

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

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

I don't see where AOT would be involved?

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

Michael Blume The updated patch should fix the issue you reported

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

Cool, thanks =)

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

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

Patch: clj-1743-2.patch

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

Demo.java
package clojure_static_initialization;

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

unknown.

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

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





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

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

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

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

 Description   

Whereas

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

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

and

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

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

[1] The matter was discussed on Google Groups:

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

with a reference to an earlier thread

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

[2] CLJ-82 and the 2009 message thread



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

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

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

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

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

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

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

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

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

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





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

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

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

Approval: Triaged

 Description   

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

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

References:



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I've commented on the design page.

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

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

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

Related to CLJ-1530

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

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

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

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

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

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

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

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

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

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





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

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

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

Approval: Triaged

 Description   

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

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

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



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

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





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

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

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

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

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-6.patch

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

The attached patch preserves two existing behaviors of merge

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

Screened by:



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

I will take a crack at a patch today.

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

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

Three potential issues:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

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

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

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

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

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

This calls for if-let + find.

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

new patch addressing concerns so far

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

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

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

Nice =)

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

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

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

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

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

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

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

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

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

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

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





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

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

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

Clojure 1.5.1.

Dependencies: criterium


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

 Description   

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

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

One workaround is to use multiple agets.

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

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



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

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

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

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

Bump. I just got bitten bad by this.

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

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

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

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

I can probably take a stab at this.

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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

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

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

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



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

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

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

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

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

This patch and ticket are still candidates for future release.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

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

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

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

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

After:

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

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



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

Great idea, thx for the patch!

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

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

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

Attaching sample output

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

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

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

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

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

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

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

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





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

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

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

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

 Description   

Problem

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

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

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

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

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

Latest patch: Removed because incomplete and goal not clear

Varargs in Java

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

Given the following setup:

VarArgs.java
public class VarArgs {

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

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


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

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

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

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

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

Patch updated to current CLJ-445 patch.

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

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

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

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

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

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

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

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

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

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

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

The patch takes the following approach:

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

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

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

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

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

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

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





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

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

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

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

 Description   

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



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

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

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

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

All tests pass.

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

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

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

Updated patch.

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

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

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

Updated patch to apply cleanly against master again.

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

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

Some feedback on the patch:

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

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

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

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

That would perform much better than the existing patch.

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

Hi Brandon, two questions:

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

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





[CLJ-1967] Enhanced namespaced map print support Created: 23/Jun/16  Updated: 23/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Approval: Triaged

 Description   

CLJ-1910 added namespaced map syntax for reader and printer but did not (yet) add these things that may be useful:

  • pprint support for namespaced maps
  • printer flag to suppress printing namespaced maps





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

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

Type: Enhancement Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File map-mapper.patch     Text File map-mapper-v2.patch    
Patch: Code
Approval: Triaged

 Description   

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

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

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

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



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

code and test for map-keys and map-vals

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

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

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

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

On the patch:

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

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

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

Also should consider

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

Thanks for comments

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

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

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

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

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

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

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

A few comments:

  • Implementations that build and tear apart MapEntry's perform much worse.
  • Transients should be used for large maps but not for small ones.
  • This benchmark shows that the property of maintaining the type of the map in the output can be achieved without sacrificing performance (the test cases using Specter or "empty" have this property).




[CLJ-1942] Add predicate for sequential search in a collection Created: 02/Jun/16  Updated: 05/Jun/16

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

Type: Enhancement Priority: Major
Reporter: Hiroyuki Fudaba Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File has-predicate.patch    
Approval: Triaged

 Description   

Many people have been writing a predicate of their own to find whether a sequence contains an item or not.

Proposal: Add a predicate (similar to `clojure.string/includes?`) that checks whether a sequential collection contains a value by doing a sequential search.

Workaround: Using function `some` is a common solution, but is confusing for beginners and can be tricky if searching for nil or false. Using .contains or other methods directly is another solution but in that case, we need to think about the class of sequence.

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






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

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

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

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

 Description   

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

Proposed:

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

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

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

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



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

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

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

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

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

Hi Steve,

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1896] Support transducers in vec and set fns Created: 24/Feb/16  Updated: 24/Feb/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transducers

Approval: Triaged

 Description   

Rather than

(into [] (map inc) [1 2 3])
vec (and set) could support the transducer directly:

(vec (map inc) [1 2 3])
(set (map inc) #{1 2 3})

Depending how far we wanted to take this, the implementation could be somewhat clever for vec in building the initial set of results in an array and then creating the vector with it directly as is already done in some other cases.






[CLJ-1880] IKVReduce impl for records Created: 09/Jan/16  Updated: 11/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

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

 Description   

Records don't implement IKVReduce, which could help with efficient merging (CLJ-1458)



 Comments   
Comment by Ghadi Shayban [ 11/Jan/16 2:49 PM ]

simple implementation attached





[CLJ-1817] Allow AssertionError messages for function :pre and :post conditions to be specified. Created: 23/Sep/15  Updated: 03/Apr/16

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

Type: Enhancement Priority: Major
Reporter: Tristan Strange Assignee: Colin Taylor
Resolution: Unresolved Votes: 5
Labels: error-reporting
Environment:

All Clojure platforms


Attachments: Text File CLJ-1817.patch    
Patch: Code and Test
Approval: Triaged

 Description   

A failing in a predicate in a list of :pre or :post conditions currently causes messages similar to one below to be displayed:

(defn must-be-a-map [m] {:pre [(map? m)]} m)
(must-be-a-map [])
;;=> AssertionError Assert failed: (map? m)  user/must-be-a-map (form-init.....clj:1)

These exception messages could be made significantly more descriptive by allowing specific messages strings to be associated with each predicate in :pre and :post conditions.

Predicate functions and there associated messages strings could be specified as a pair of values in a map:

(defn must-be-a-map 
  [m]
  {:pre [{(map? m) "m must be a map due to some domain specific reason."}]}
  m)

The following would then produce an error message as follows:

(must-be-a-map 10)
AssertionError Assert failed: m must be a map due to some domain specific reason.
(map? m) user/must-be-a-map (form-init.....clj:1)

This would allow predicates without messages to specified alongside pairs of associated predicate message pairs as follows:

(defn n-and-m [n m] {:pre [(number? n) {(map? m) "You must provide a map!"}]})

This change would not break any existing functionality and still allow for predicates to be predefined elsewhere in code.

As a result pre and post conditions could provide a natural means of further documenting the ins and outs of a function, simplify the process of providing meaningful output when developing libraries and perhaps make the language better suited to teaching environments[1]

[1] http://wiki.science.ru.nl/tfpie/images/2/22/TFPIE2013_Steps_Towards_Teaching_Clojure.pdf



 Comments   
Comment by Colin Taylor [ 03/Apr/16 5:26 PM ]

Attached approach differs from that advocated for in the description by not requiring a map. The existing spec of :

{:pre [pre-expr*]
 :post [post-expr*]}

in effect becoming :

{:pre [(pre-expr assert-msg?)*]
 :post [(pre-expr assert-msg?)*]}

where assert-msg is a String. Note this means a (presumably erroneous) second String after an expression would be treated as a truthy pre-expr.

Contrived example :

(defn print-if-alphas-and-nums [arg] {:pre [(hasAlpha arg) "No alphas"
                                            (hasNum arg) "No numbers"
                                            (canPrint arg)]}
  (println arg))

user=> (print-if-alphas-and-nums "a5%")
a5%
nil
user=> (print-if-alphas-and-nums "$$%")
AssertionError Assert failed: No alphas
(hasAlpha arg)  user/print-if-alphas-and-nums (NO_SOURCE_FILE:19)

I have considered extending the spec further to (pre-expr assert-msg? data-map)* perhaps supported by ex-info, ex-data analogues in assert-info, assert-data to convey diagnostic info (locals?). A map could contain a :msg key or perhaps the map is additional to the message string. I thought I'd wait for input though at this point.

I also considered allowing % substitution for the fn return value in the message as in :post conds, but how to escape?

Comment by Colin Taylor [ 03/Apr/16 6:17 PM ]

I should point out that the tests include the currently uncovered existing functionality too.





[CLJ-1807] Add prefer-proto, like prefer-method but for protocols Created: 30/Aug/15  Updated: 04/Sep/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: protocols

Attachments: Text File 0001-CLJ-1807-add-prefer-proto.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently it's possible to extend a protocol to multiple interfaces but there's no mechanism like prefer-method for multimethods to prefer one implementation over another, as a result, if multiple interfaces match, a random one is picked.

One particular example where this is a problem, is trying to handle generically records and maps (this come up in tools.analyzer): when extending a protocol to both IRecord and IPersistentMap there's no way to make the IRecord implementation be chosen over the IPersistentMap one and thus protocols can't be used.

The attached patch adds a prefer-proto function that's like prefer-method but for protocols.

No performance penalty is paid if prefer-proto is never used, if it's used there will be a penalty during the first protocol method dispatch to lookup the perference table but the protocol method cache will remove that penalty for further calls.

Example:

user=> (defprotocol p (f [_]))
p
user=> (extend-protocol p clojure.lang.Counted (f [_] 1) clojure.lang.IObj (f [_] 2))
nil
user=> (f [1])
2
user=> (prefer-proto p clojure.lang.Counted clojure.lang.IObj)
nil
user=> (f [1])
1

Patch: 0001-CLJ-1807-add-prefer-proto.patch






[CLJ-1794] Sorting vector yields non-indexed ArraySeq Created: 05/Aug/15  Updated: 10/Aug/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Attachments: Text File 0001-CLJ-1794-Make-ArraySeqs-implement-Indexed.patch    
Approval: Triaged

 Description   

Sorting a vector gives back an ArraySeq with O(n) gets instead of O(log N) gets. This means it can be more efficient to take a vector, sort, then turn it back into a vector.

Cause: sort works by copying the collection to be sorted into an array, calls Arrays/sort to sort it, and then returns a seq on the sorted array. The seq returned is an ArraySeq and doesn't implement Indexed.

Alternatives:

1. Make ArraySeq (and primitive specializations thereof) implement Indexed, providing constant time lookup by index.
2. Specialize sorting for different collection types
3. ???



 Comments   
Comment by Ragnar Dahlen [ 06/Aug/15 2:28 AM ]

Update description, attach patch.

Comment by Ragnar Dahlen [ 06/Aug/15 2:31 AM ]

Added link to current patch.

Comment by Alex Miller [ 06/Aug/15 6:50 AM ]

Another alternative to consider here is to have sort do something smarter.

Comment by Ragnar Dahlen [ 06/Aug/15 7:44 AM ]

Having thought a bit more about the approach and implications of this I'm not sure this patch is a good idea at all. It makes a little bit sense for the particular case of sorting a vector, but on the other hand sort only promises to return a sorted sequence of given coll. Implementing Indexed for a sequence type just because the underlying data structure supports efficient lookup by index feels wrong. Like you suggest, effort is maybe better spent thinking about making sort smarter, which is a different issue, or just using sorted collections instead.

Comment by Kevin Downey [ 06/Aug/15 12:49 PM ]

It seems like the best thing here would be to change sort to return a vector. Usages of sort in the middle of sequence pipelines will continue to work, but a sort followed by conj will break (I cannot recall an instance of this off hand, but I am sure they exist). Sorting seems to imply a fully realized collection, and vectors are the "strongest" realized collections that can be returned here.

Given the conservative nature of core, and the issue with conj ordering above, the next best thing might be to add a sortv similar to the existing mapv.

Another option might be to remove the call to seq, so sort returns the sorted array. This would actually be really useful because you can use Arrays.binarySearch. Calls to conj after a sort would then fail with an exception instead of conj to the "wrong" place.





[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 23/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

All


Attachments: Text File clj-1771.patch    
Approval: Triaged

 Description   

It would be nice if assoc-in supported multiple key(s)-to-value pairs (and threw an error when there were an even number of arguments, just like assoc):

user=> (assoc-in {} [:a :b] 1 [:c :d] 2)
{:a {:b 1}, :c {:d 2}}
user=> (assoc-in {} [:a :b] 1 [:c :d])
IllegalArgumentException assoc-in expects even number of arguments after map/vector, found odd number


 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:15 PM ]

Simple patch attached. I did not find any existing tests for assoc-in but I could add them if wanted.





[CLJ-1750] There should be a way to observe platform features at runtime Created: 08/Jun/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: reader

Approval: Triaged

 Description   

Reader conditionals let the reader emit code conditionally based upon a set of platform features.

This is a closed set - however, currently it is baked in as an implementation detail of the reader. Runtime code cannot access the current platform feature set.

This is problematic when writing a macro that needs to emit code conditionally based upon the platform of the code being compiled. Reader conditionals themselves won't work since macros are always themselves read in Clojure.

We should enable some mechanism for retrieving the current platform at runtime, or at least at macro expansion time.

For example, this is the kind of thing it should be possible to do:

(defmacro mymacro []
    (if (*platforms* :clj)
      `(some-clojure-thing)
      `(some-cljs-thing)))


 Comments   
Comment by Micah Martin [ 19/Jun/15 1:46 PM ]

+1 - Would very much like to see this in 1.7. Currently I have to use an ugly hack.

(def ^:private ^:no-doc cljs? (boolean (find-ns 'cljs.analyzer)))





[CLJ-1665] take-nth transducer could be faster without rem Created: 20/Feb/15  Updated: 20/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance
Environment:

Mac OS X 10.10.2, JDK 1.8.0_31


Attachments: Text File CLJ-1665-faster-take-nth-transducer-without-rem.patch    
Patch: Code
Approval: Triaged

 Description   

The take-nth transducer is calling rem on each index, which is relatively expensive compared to a zero? test. It could just count down from N instead as the step size is fixed.



 Comments   
Comment by Steve Miner [ 20/Feb/15 12:34 PM ]

Patch attached. It's about 25% faster on a simple test like:

(time (transduce (take-nth 13) + (range 1e7)))
Comment by Steve Miner [ 20/Feb/15 12:41 PM ]

I didn't worry about (take-nth 0) case, but my patch does give a different result. The current implementation gets a divide by zero error (from rem). My patched version returns just the first element once. The regular collection version returns an infinite sequence of the first element. I doubt anyone expects a sensible answer from the 0 case so I didn't try to do anything special with it.

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

Nice =)

I would say that the transducer version really ought to match the collection version as closely as possible, but I don't think there's actually a way to write a transducer that transforms a finite sequence into an infinite sequence, so no luck there.

Maybe while we're at it we should change both the transducer and the collection arities to throw on zero?





[CLJ-1661] Varargs protocol impls can be defined but not called Created: 17/Feb/15  Updated: 09/Oct/15

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

Type: Enhancement Priority: Major
Reporter: Reno Reckling Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

The compiler accepts this:

(deftype foo []
clojure.lang.IFn
(invoke [this & xs]))

However calling ((foo.) :bar) will throw an AbstractMethodError. Wouldn't some checking be desirable?



 Comments   
Comment by Reno Reckling [ 17/Feb/15 11:09 AM ]

This is a clone of http://dev.clojure.org/jira/browse/CLJ-1024 because the original with its attached patches was forgotten with the reason that "It has to wait and cannot be applied in 1.5" which is 2 major versions ago now, with 1.7 underway.

I would like to reopen it, or continue working on it in this ticket because i just stumbled over this issue the second time and the debugging sessions that follow this are annoying.

Comment by Andy Fingerhut [ 19/Feb/15 12:23 PM ]

Fix Version/s was Release 1.5, but that field should only be set by Clojure screeners.

Comment by Reno Reckling [ 19/Feb/15 12:41 PM ]

Yes, i just cloned the original issue. Later i realized that I'm unable to edit any of the fields.
The issue is just concerned with a missing warning/error when trying to compile protocols with "&" in the argument list as they are interpreted as a variable name "&" instead of a varargs placeholder which the user probably expects.

Comment by Michael Blume [ 19/Feb/15 2:17 PM ]

Here's a forward-port of the 1024 patch





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

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

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

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

 Description   

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

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

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

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

benchmark results:

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

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

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

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

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

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

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

Patch: CLJ-1656-v5.patch



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

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

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

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

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

Patch v2 includes assoc!

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

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

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

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

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

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

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

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

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

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

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

which reminds me that this patch needs tests.

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

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

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

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

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

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

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

v5 marks the helper macros private.

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

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

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

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

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

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

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

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

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

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

Yes, inlining works through var invocation.

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

Michael,

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

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

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

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

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

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

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

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

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

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

There, code and test.

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

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

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

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

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





[CLJ-1629] Improve error message when defn form omits parameter declaration Created: 29/Dec/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

When defn form is malformed, Clojure compiler will report meaningless error and in combination with function body, can cause really bad experience. Here is the sample:

(defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))

It will report:

IllegalArgumentException Parameter declaration "let" should be a vector  clojure.core/assert-valid-fdecl (core.clj:7123)

However, if is written:

(defn foo "bla")

error report makes more sense:

IllegalArgumentException Parameter declaration missing  clojure.core/assert-valid-fdecl (core.clj:7107)


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

I don't think this is really meaningless – if you replace the symbol let with a vector, say, [i], you get a perfectly valid function definition

(defn foo
  "This is docstring."
  ([i] [i 1]
    (+ i 1)))
Comment by Sanel Zukan [ 29/Dec/14 2:41 PM ]

Yes and maybe make sense for this case. But in general, the report is misleading for common defn forms (how often you will see function definitions written this way, unless you want multi-arity function) and should have the same report as for second sample; in both cases it is the same cause.





[CLJ-1545] Add unchecked-divide, unchecked-remainder Created: 02/Oct/14  Updated: 06/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Colin Taylor
Resolution: Unresolved Votes: 0
Labels: math, newbie

Attachments: File CLJ-1545-2.diff     File CLJ-1545.diff    
Patch: Code and Test
Approval: Triaged

 Description   

This appears like it might be an oversight that these are missing. There are unchecked-divide-int and unchecked-remainder-int functions, but not equivalents for longs, even though there are equivalents for longs for every other unchecked operation. The JVM has bytecodes for long division and remainder.

The Clojure documentation in the section "Support for Java Primitives" on page http://clojure.org/java_interop has links for unchecked-divide and unchecked-remainder, but since they don't exist in Clojure, the API link targets don't exist.

It seems like a good idea to either add these to Clojure, or remove them from the documentation.



 Comments   
Comment by Colin Taylor [ 03/Oct/14 6:17 PM ]

Having a go at this.

Comment by Colin Taylor [ 04/Oct/14 6:02 AM ]
  • Added tests for unchecked-divide-int and unchecked-remainder-int too.
  • Unchecked fns only support binary arity and will throw CompilerException(ArityException)s where checked will not.
  • Is there any value to (int,long) (long,int) overrides for java interop cases e.g. using java collections from Clojure in high perf code?
Comment by Alex Miller [ 04/Oct/14 9:13 AM ]

Thanks for taking this on Colin!

1) When I apply the patch (git apply CLJ-1545.diff), I get a bunch of whitespace errors which will need to be cleaned up but also the patch seems to fail to apply at all on the changes in test/clojure/test_clojure/numbers.clj. It looks like perhaps the diff is just not the right diff format. You might want to check out the instructions at http://dev.clojure.org/display/community/Developing+Patches about using git format-patch.

2) If you could put a more useful git commit message, that would be helpful. Something like "CLJ-1545 Adds missing unchecked-divide and unchecked-remainder for primitive longs."

Thanks!

Comment by Colin Taylor [ 04/Oct/14 4:47 PM ]

Uggh, sorry Alex.

New patch with better commit message.

Comment by Alex Miller [ 04/Oct/14 7:24 PM ]

The patch format looks better. Pulling out farther to the ticket itself, afaict Clojure will already use the right byteocode for checked or unchecked so this may not even be needed?

If I compile (without the patch):

(defn foo-div ^long [^long a ^long b]
  (quot a b))

then the bytecode for that fn is:

public final long invokePrim(long, long);
    Code:
       0: lload_1
       1: lload_3
       2: ldiv
       3: lreturn

similarly, quot of two longs yields the same code but with lrem. I think patch has no net effect on the resulting bytecode?

Comment by Andy Fingerhut [ 04/Oct/14 7:42 PM ]

Alex, did you do the testing in your previous comment with *unchecked-math* true or false? If false, then I would think that if CLJ-1254 is judged a bug, then the behavior you saw is a bug, too, that misses the same corner case.

Comment by Alex Miller [ 04/Oct/14 10:19 PM ]

The current results are the same with either unchecked-math setting, but I see your point.

Refreshing my memory of the (/ Long/MIN_VALUE -1) case, I think you're right. The (new) unchecked-divide / remainder should do what the current (checked) forms do and the regular division and remainder cases should be making the overflow check. I think CLJ-1254 should cover the quot changes.

Comment by Colin Taylor [ 04/Oct/14 10:19 PM ]

user=> (dotimes [_ 6] (time (dotimes [_ 50000000] (unchecked-divide 4 (System/currentTimeMillis)))))
"Elapsed time: 1806.942 msecs"
"Elapsed time: 1808.747 msecs"
"Elapsed time: 1865.074 msecs"
"Elapsed time: 1802.777 msecs"
"Elapsed time: 1839.468 msecs"
"Elapsed time: 1830.61 msecs"
nil
user=> (dotimes [_ 6] (time (dotimes [_ 50000000] (/ 4 (System/currentTimeMillis)))))
"Elapsed time: 5003.598 msecs"
"Elapsed time: 4998.182 msecs"
"Elapsed time: 4941.237 msecs"
"Elapsed time: 5036.517 msecs"
"Elapsed time: 4965.867 msecs"
"Elapsed time: 4982.693 msecs"





[CLJ-1530] Make foo/bar/baz unreadable Created: 22/Sep/14  Updated: 31/Mar/16

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: 0
Labels: reader

Attachments: Text File 0001-fix-LispReader-and-EdnReader-so-that-foo-bar-baz-is-.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently keywords and symbols containing more than one slash are disallowed by the spec, but allowed by the readers.
This trivial patch makes them unreadable by the readers too.

Pre:

user=> :foo/bar/baz
:foo/bar/baz

Post:

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


 Comments   
Comment by Andy Fingerhut [ 22/Sep/14 12:14 PM ]

Perhaps overlap with CLJ-1527 ?

Comment by Thomas Engelschmidt [ 28/Oct/14 4:36 AM ]

Please notice that keywords with more than one slash has a different hashcode across clojure version 1.5 and 1.6

This creates a problem when using a datomic version that works with clojure 1.5 under clojure 1.6 and the schema have one or more keys with more than one slash.

Comment by Chris Zheng [ 30/Mar/16 8:41 AM ]

Please reconsider this `fix` for the following reasons:

Please see discussion on this topic below:

dm3 [5:04 PM]
Is there a reason why `(read-string "a/b/c")` is OK, while `(clojure.tools.reader/read-string "a/b/c")` fails with `Invalid token`?

hiredman [5:04 PM]
there is a ticket to fix read-string

dm3 [5:05 PM]
so the correct behaviour is to fail?

hiredman [5:05 PM]
http://dev.clojure.org/jira/browse/CLJ-1530

dm3 [5:06 PM]
thanks, seems like a breaking change :simple_smile:

hiredman [5:07 PM]
the docs have precluded symbols like a/b/c for some time, and the behavior of how those unspecified symbols were read apparently changed at some point

lucas [5:09 PM]
joined #clojure

zcaudate [5:16 PM]
@hiredman: seriously. I’m really peeved about that because I’ve been using `:a/b/c` keywords for a while… even wrote a whole freaking library to deal with that http://docs.caudate.me/hara/hara-string.html#api---path (edited)

[5:16]
and now they are taking it out

[5:18]
i’m of the opinion that the `::foo/baz` keywords should be taken out first

[5:20]
```user=> (require '[clojure.walk :as walk])
nil
user=> ::walk/hello
:clojure.walk/hello
```

[5:20]
that causes way more problems

[5:20]
especially with analysers

[5:22]
https://github.com/jonase/kibit/issues/14

GitHub
Kibit breaks on namespace qualified keywords · Issue #14 · jonase/kibit · GitHub
If the code contains namespace qualified keywords with aliases, Kibit errors out with a Invalid token exception. The following code demonstrates the problem - ;;; foo.clj (ns foo) ;;; bar.clj (n...

[5:25]
@dm3 if there’s really a problem, you can patch it:

[5:25]
https://github.com/helpshift/hydrox/blob/master/lein/src/leiningen/hydrox/setup.clj

GitHub
helpshift/hydrox
hydrox - Dive deeper into your code

dm3 [5:31 PM]
yeah, would have to patch cljs.tools.reader too unfortunately :confused:

[5:31]
think I'll just work around that

bronsa [5:36 PM]
@zcaudate: `::foo/bar` style keywords are in by design and not going anywhere, `:foo/bar/baz` have always been invalid by the spec and undefined behaviour (edited)

[5:37]
@dm3: is changing undefined behaviour a breaking change? :simple_smile:

dm3 [5:37 PM]
breaking as in breaking people's code :simple_smile:

[5:37]
e.g. zcaudate

bronsa [5:38 PM]
that code is already broken if it's using invalid clojure. It's just accidentaly working

dm3 [5:39 PM]
I'm looking from a pragmatic perspective. Theoretically you're right :simple_smile:

[5:40]
and I'm not judging either

bronsa [5:40 PM]
pragmatically, `:foo/bar/baz` is a bug waiting to happen. what does `(namespace :foo/bar/baz)` return?

dm3 [5:40 PM]
whatever it returns currently?

[5:41]
I mean it's kind of defined by the implementation

sveri [5:41 PM]
@dm3 @bronsa I wouldnt even agree that you are theoretically right. As soon as enough people adapted the broken code it falls under something like a common law that was accepted by both parties for a long enough time.

bronsa [5:42 PM]
what about `namespace` on `(keyword "foo/bar" "baz")` and `(keyword "foo" "bar/baz")`

dm3 [5:43 PM]
I agree that currently implemented semantics are messy

[5:43]
but my point was that it's still a breaking change

[5:43]
not that it's a "bad" change

[5:43]
that's a judgement

bronsa [5:43 PM]
@sveri: I would agree with you, as long as the undefined behaviour we accept as defined doesn't cause impossible to fix semantics.

[5:44]
that's why for example, the patch that made symbols starting with numbers illegal was rolled back

[5:45]
it broke existing code, it didn't cause weird semantics so it was rolled back. not the case with `:foo/bar/baz`

[5:45]
@dm3: you could make the point that fixing any bug is a breaking change then – people might be relying on that bug.

dm3 [5:46 PM]
yep, I guess what matters is how obvious the incorrect behaviour is and how many people rely on it

bronsa [5:46 PM]
if the doc explicitely says "you can use ​one​ `/` inside a symbol", then if you're using more than one you're writing invalid clojure and you should expect it to maybe break (edited)

dm3 [5:47 PM]
I really haven't even thought about multiple slashes (nor noticed the docs) in a symbol in ~3 years of using Clojure

[5:47]
my initial thought (today) was that it was permitted

[5:47]
and the namespace would be the first segment before the first slash (edited)

bronsa [5:51 PM]
well, that doesn't make much sense though. `/` in clojure means `namespace separator`. if I see `FOO/BAR`, no matter what `FOO` and `BAR` are, I know that `FOO` is the namespace, and `BAR` is the name. if you want to express paths with keywords as in @zcaudate's lib, you should use a different separator in your keywords that doesn't have a special meaning in clojure, like `.` (i.e. `:foo/bar/baz` -> `:foo.bar.baz` or `:foo/bar.baz`) (edited)

dm3 [5:51 PM]
I do not want to argue semantics. Just sharing one point of view

bronsa [5:52 PM]
and my point is that pragmatic point of views (especially when they go against the current doc) should only be considered if the semantics they imply are clear and unambiguous

sveri [5:53 PM]
@bronsa: Nice explanation, thank you :simple_smile:

dm3 [5:53 PM]
yep :simple_smile:

[5:54]
I agree to that, as you have to make decisions in the end

zcaudate [9:07 PM]
@bronsa: written form of communication has a way of making things more serious than they seem

[9:08]
honestly… i knew it was coming since 1.6 when the edn reader started breaking my code

[9:09]
it’s probably more my fault for not communicating this earlier but oh well.. we all have to roll with the times

bronsa [9:09 PM]
@zcaudate: yeah no worries, I was just using your lib as an example since you brought that up

zcaudate [9:10 PM]
having said that, you can imagine my disappointment because I had designed an entire query semantic based on the keyword `:foo/bar/baz` feature (now bug) (edited)

[9:11]
http://docs.caudate.me/adi/adi-walkthrough.html#querying

[9:11]
you noticed I didn’t use `(adi/select ds {:student/classes/teacher/name "Mr. Blair"}})`

[9:12]
in my docs because the tests started breaking

jstew [9:12 PM]
@zcaudate: You put out so much quality stuff that I wonder if you ever sleep!
1

bronsa [9:12 PM]
@zcaudate: luckly the fix should be easy :simple_smile: just replace `/` with `.`

zcaudate [9:12 PM]
no!

[9:13]
see the problem is… datomic has things like `account.type/user`

[9:13]
and so I would have to do the cljs thing `account.type$user`

bronsa [9:13 PM]
(that's some high quality documentation btw, good job)

zcaudate [9:14 PM]
@bronsa: hahaha thanks… so maybe you can push the fix to 1.10

[9:14]
that way I can get a few more months left

[9:15]
like it’s not a big deal… but I thought that there is a parallel between the path structure of the `/` calls and the nesting of maps

[9:16]
and so there is an equivalence to `{:student {:classes {:teacher {:name '(?fulltext "Blair")}`

[9:17]
and `{:student/classes/teacher/name "Mr. Blair"}`

[9:17]
which is prettier in my opinion

bronsa [9:17 PM]
@zcaudate: sorry if that wasn't clear, but I don't actually have any control over when or what gets into clojure or not, I'm just a contributor :simple_smile: so there are chances that the clojure/core team will take a different decision and actually decline that ticket (I would be really disappointed if that was the case though!). If that will happenI will obviously make a change to `tools.reader` to allow them aswell, (edited)

zcaudate [9:18 PM]
@bronsa: damn.

[9:19]
well… maybe you can highlight this fact

[9:19]
and also if the fix is made, a fix to `(keyword "foo/bar/baz")` will also be needed (edited)

bronsa [9:20 PM]
don't think that'll ever be done. validating inputs to `keyword`/`symbol` etc has been asked/discussed tons of times and repeatedly declined for performance reasons

zcaudate [9:20 PM]
so that’s a matter of consistency.

bronsa [9:20 PM]
(not that I agree with that decision, but it seems like Rich isn't going ot change his mind on that)

[9:21]
@zcaudate: there's a difference between what a symbol/keyword can be at runtime, and what a valid read-time symbol/keyword is

zcaudate [9:21 PM]
and also, it means I can setup a reader macro #k foo/bar/baz and get the same effect

[9:22]
like it’s stupidly ugly but i believe it will work

bronsa [9:23 PM]
but the ambiguity of what to do with `namespace` and `name` still remains so dunno

[9:23]
@zcaudate: that wouldn't work either way, if http://dev.clojure.org/jira/browse/CLJ-1530 gets accepted neither `:foo/bar/baz` ​nor​ `foo/bar/baz` will be valid anymore

[9:24]
@zcaudate: btw I'd suggest you log your issues with that ticket in a comment there if you feel strongly against it

new messages
[9:25]
I ​suspect​ that the response will be "you should use a delimiter that doesn't have a special meaning in clojure", but I might be completely wrong (I find the core team doesn't agree with my opinions quite frequently ) especially if you point out that your library will break. (edited)

zcaudate [9:30 PM]
@bronsa: thanks for the heads up. I’ll leave a comment and add a prayer for the bdfl

Comment by Alex Miller [ 30/Mar/16 8:55 AM ]

Chris -

  • (keyword "foo/bar/baz") will still be fine. Programmatic keywords can be created for any string - this is intentionally much broader than what the reader supports as a literal in code and it's a feature that's widely used. At some future point, there may be an escaping mechanism for symbols or keywords with characters outside the spec such that the reader could read them as well, but that's outside the scope of this.
  • Your api is using illegal keywords according to http://clojure.org/reference/reader and you should not expect them to work. I think you should change your library.
Comment by Chris Zheng [ 30/Mar/16 6:15 PM ]

Alex

  • I wouldn't necessarily call it `illegal` as the current behavior in the edn.reader was added in 1.6 without warning.
  • Also, if (keyword "foo/bar/baz") is allowed to exist, then there still would be the indeterminate namespace/name problem that @bronsa highlighted. I would argue for consistency and if :foo/bar/baz is illegal in the reader, then it should be illegal everywhere
  • My library should be fine... but users of the library may have to change their queries
Comment by Alex Miller [ 30/Mar/16 7:00 PM ]

The reader page clearly states "'/' has special meaning, it can be used once in the middle of a symbol to separate the namespace from the name" and keywords are "like symbols". This has been on the reader page since the oldest version I can find in the internet archive (July 2008). edn (despite its similarities) is a separate thing than Clojure, and irrelevant.

The 2-arity form of keyword can be used without ambiguity: (keyword nil "foo/bar/baz"). The 1-arity form will split based on the first / found (in this example into "foo" and "bar/baz"). I see no reason that would need to change.

Comment by Chris Zheng [ 31/Mar/16 8:58 AM ]

I disagree on the "specialness" of `/` in keywords, especially if it is allowed as a string.

This leads to another problem in the ambiguity of output:

user=> (keyword "foo" "bar/baz")
:foo/bar/baz

user=> (keyword nil "foo/bar/baz")
:foo/bar/baz

Unless the output is displayed as :foo//bar/baz, it is unclear where the namespace is and the concept of `code is data` will be diminished if the output cannot be read back as data.

There is also the case for symbol:
user=> (symbol "foo/bar/baz")
foo/bar/baz

well... at least all the examples listed are all consistently "illegal"

Anyways, even if the docs had not explicitly stated such, :foo/bar/baz has existed since the beginning of clojure and personally, it seems to be more string-like than symbol-like. Ironically, I'm pretty sure that I got the idea of using multiple slashes in keywords from reading the datomic documentation from back when I started work on adi.

Ultimately, the decision is not mine to make and I do value the guidance of clojure team over the development of the language. I do however, hope that my points for keeping things as they are can be recognised and be taken into consideration.

Comment by Chris Zheng [ 31/Mar/16 9:23 AM ]

I'll quote Rich https://www.youtube.com/watch?v=P76Vbsk_3J0 @ 5:10

"Many of the things you consider to be problems (with lisp) are features... down the line..."





[CLJ-1493] Fast keyword intern Created: 06/Aug/14  Updated: 14/Aug/15

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

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: keywords, performance
Environment:

Mac OS X 10.9.4 / 2.6 GHz Intel Core i5 / 8 GB 1600 MHz DDR3
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)


Attachments: File fast_keyword_intern.diff    
Patch: Code
Approval: Triaged

 Description   

Keyword's intern(Symbol) method uses recursive invocation to get a valid keyword instance.I think it can be rewrite into a 'for loop'
to reduce method invocation cost.
So i developed this patch, and make some simple benchmark.Run the following command line three times after 'ant jar':

java -Xms64m -Xmx64m -cp test:clojure.jar clojure.main -e "(time (dotimes [n 10000000] (keyword (str n))))"

Before patched:

"Elapsed time: 27343.827 msecs"
"Elapsed time: 26172.653 msecs"
"Elapsed time: 25673.764 msecs"

After patched:

"Elapsed time: 24884.142 msecs"
"Elapsed time: 23933.423 msecs"
"Elapsed time: 25382.783 msecs"

It looks the patch make keyword's intern a little more fast.

The patch is attached and test.

Thanks.

P.S. I've signed the contributor agreement, and my email is killme2008@gmail.com .



 Comments   
Comment by Alex Miller [ 07/Aug/14 9:01 AM ]

Looks intriguing (and would be a nice change imo). I ran this on a json parsing benchmark I used for the keyword changes and saw ~3% improvement.

Comment by dennis zhuang [ 07/Aug/14 9:54 PM ]

Updated the patch, remove the 'k == null' clause in for loop,it's not necessary.

Comment by Andy Fingerhut [ 11/Aug/14 1:29 AM ]

Dennis, while JIRA can handle multiple patches with the same name, it can be confusing for people discussing the patches, and for some scripts I have to evaluate them. Please consider giving the patches different names (e.g. with version numbers in them), or removing older ones if they are obsolete.

Comment by dennis zhuang [ 11/Aug/14 9:19 AM ]

Hi,andy

Thank you for reminding me.I deleted the old patch.

Comment by dennis zhuang [ 11/Sep/14 10:34 AM ]

I am glad to see it is helpful.I benchmark the patch with current master branch,it's fine too.

Comment by dennis zhuang [ 14/Aug/15 9:12 AM ]

Is this patch can be merged? Or is it rejected?

Comment by Alex Miller [ 14/Aug/15 9:41 AM ]

As a minor enhancement, this patch has not yet been high enough priority to be considered yet.

Comment by dennis zhuang [ 14/Aug/15 11:31 AM ]

All right.Hope to merge it.Thanks.





[CLJ-1488] Implement Named over Vars Created: 01/Aug/14  Updated: 28/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Implement-clojure.lang.Named-over-Vars.patch    
Patch: Code
Approval: Triaged

 Description   

Vars, while a general reference structure, are used to implement bindings and have special reader and printer notation reflecting this reality. Unlike Keywords and Symbols which share the "namespace/name" notation of Vars, Vars do not implement the clojure.lang.Named interface while they print as if they were Named.

The attached patch implements Named over Vars.

Example:

user=> (name :clojure.core/conj)
"conj"
user=> (namespace :clojure.core/conj)
"clojure.core"
user=> (name 'clojure.core/conj)
"conj"
user=> (namespace 'clojure.core/conj)
"clojure.core"
user=> (name #'clojure.core/conj)
"conj"
user=> (namespace #'clojure.core/conj)
"clojure.core"
user=> (with-local-vars [x 1] (name x))
"--unnamed--"
user=> (with-local-vars [x 1] (namespace x))
nil
user=> (with-local-vars [x 1] (println x))
#<Var: --unnamed-->

This is useful for applications such as the CinC project where Vars are often taken directly as values in which context they would ideally be interchangeable with the Symbols the bound values of which they represent.



 Comments   
Comment by Nicola Mometto [ 02/Aug/14 11:42 AM ]

With this patch calling `name` on a unnamed Var will cause a NPE, I don't think this is desiderable.

Comment by Reid McKenzie [ 02/Aug/14 1:39 PM ]

I agree, however this behavior seems to be standard in Core.

Clojure 1.6.0
user=> (name nil)
NullPointerException clojure.core/name (core.clj:1518)
user=> (namespace nil)
NullPointerException clojure.core/namespace (core.clj:1526)

I'm also not convinced that the "name" or "namespace" of an unbound var is meaningful, in which case a NPE is probably acceptable.

Comment by Nicola Mometto [ 02/Aug/14 1:45 PM ]

I was not talking about unbound Vars, but about anonymous Vars, I'm assuming you miswrote.

I'd agree with you that throwing an exception could be a reasonable behaviour, except I can test for nil before calling name on it while there's no way to test whether a var is named or not, except trying to access directly the .name field which is excatly what this ticket is for.

Comment by Nicola Mometto [ 02/Aug/14 2:27 PM ]

Me and Reid have been talking about this issue over IRC, here's what's come up:

  • Vars can be either unnamed (as are Vars returned by with-local-vars) or contain both a namespace and a name part( that's the case for interned Vars)
  • there's currently no way to test for the "internedness" of a Var, so accessing either the .name or the .namespace field of the Var testing for nil is the only way to do it currently

given the above, the current patch seems unsatisfactory, here some proposed solutions:

  • make Var Named, make namespace return nil for an unnamed Var and name return "--unnamed--"
  • keep Var not implementing Named, add a "var-symbol" function returning either a namespaced symbol matching the ns+name of the Var or nil for an unnamed Var

Personally, I'd rather have the second solution implemented as I don't feel Var should be Named given that they can be unnamed and that strikes me as a contradicion

Comment by Reid McKenzie [ 02/Aug/14 3:16 PM ]

Added patches explicitly handling the unnamed var cases.

Comment by Reid McKenzie [ 02/Aug/14 3:33 PM ]

Squashed all patches into a single diff and updated attachments.





[CLJ-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 15/Nov/15

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: 4
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?





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

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: math

Attachments: Text File CLJ-1452.patch    
Patch: Code and Test
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.





[CLJ-1435] 'numerator and 'denominator fail to handle integral values (i.e. N/1) Created: 30/May/14  Updated: 01/Sep/15

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

Type: Enhancement Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: None

Approval: Triaged

 Description   

Because ratio values reduce to lowest terms and, for integral values where the lowest term is N/1, are auto-converted to BigInts (and formerly Longs), the current behavior of clojure.core/numerator and clojure.core/denominator yield unexpected results.

user=> (numerator 1/3)
1
user=> (numerator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3306)
user=> (denominator 1/3)
3
user=> (denominator (+ 1/3 2/3))

ClassCastException clojure.lang.BigInt cannot be cast to clojure.lang.Ratio  clojure.core/denominator (core.clj:3314)
user=>

The auto-conversion to Longs is not really the problem in my mind. I'd like to see numerator return the original value when presented with a BigInt and denominator always return 1 when presented with a BigInt. It seems reasonable to request the same for Longs.

If desired, I'd be happy to produce a patch.



 Comments   
Comment by Andy Fingerhut [ 30/May/14 6:35 PM ]

I don't know the official stance on this ticket, but will add some notes.

Aaron, numerator and denominator are pretty clearly documented to work on Ratio types only.

It is pretty easy to write my-numerator and my-denominator that work exactly as you wish, checking for the type of arg and using numerator, denominator for Ratio types, and doing whatever you think is correct for other numeric types.

Comment by Aaron Brooks [ 30/May/14 7:44 PM ]

I'm aware that they are documented as such. Part of my point is that you can be working entirely with Ratio types and, via arithmetic operations between them, sometimes wind up with a non-Ratio number unexpectedly.

Also consider:

user=> (numerator 2/1)
ClassCastException java.lang.Long cannot be cast to clojure.lang.Ratio  clojure.core/numerator (core.clj:3238)

You're then left either implementing a try/catch correction or always checking the type before using numerator or denominator which is a loss in performance.

The patch I have in mind is creating a protocol, extended to Ratio, BigInt and Long which calls the appropriate method (Ratios) or returns either the given number or 1 (numerator/denominator) for the integral types. I expect this to maintain the current level of performance in the cases where it works and behave properly in the cases currently not handled.

Comment by Gary Fredericks [ 27/Aug/15 10:38 AM ]

I've definitely written the helper functions Andy describes on several occasions.

Comment by Felipe Micaroni Lalli [ 01/Sep/15 4:58 PM ]

Related issue: https://stackoverflow.com/questions/25194809/how-to-convert-any-number-to-a-clojure-lang-ratio-type-in-clojure

A workaround to that is (numerator (clojure.lang.Numbers/toRatio (rationalize <put any type of number here>)))





[CLJ-1386] Add transient? predicate Created: 17/Mar/14  Updated: 20/Apr/15

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

Type: Enhancement Priority: Major
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0004-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.

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

Patch 0002-Add-transient-predicate.patch dated Mar 22 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether this patch is straightforward to update.

Comment by Devin Walters [ 06/Aug/14 4:11 PM ]

I've updated the patch to 0003-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.

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

Patch 0003-Add-transient-predicate.patch dated Aug 6 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 Devin Walters [ 31/Aug/14 12:01 AM ]

I've updated the patch to 0004-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.





[CLJ-1293] Portable "catch-all" mechanism Created: 05/Nov/13  Updated: 27/Jan/16

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

Attachments: Text File CLJ-1293-v001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Design page: http://dev.clojure.org/display/design/Platform+Errors

CLJS ticket/patch: http://dev.clojure.org/jira/browse/CLJS-661

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.

Open Question: Catch Throwable (patch v001 does this) or Exception? Alternatively, a more carefully crafted list of "non-fatal" errors. See Scala's NonFatal pattern extractor: http://www.scala-lang.org/api/current/index.html#scala.util.control.NonFatal$



 Comments   
Comment by Brandon Bloom [ 28/Dec/14 11:33 AM ]

Noticed this switched from "Minor" to "Critical", so I figured I should mention that I later realized that we might want :default to catch Exception instead of Throwable, so as to avoid catching Error subclasses. Javadocs say: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch." If that's what we actually want, I can provide an updated patch.

Comment by Alex Miller [ 28/Dec/14 2:19 PM ]

Seems like an open question, might be best just to list it as such in the description.

I don't really expect to reach consensus on the ticket or patch right now, just trying to update priorities and raise visibility for discussion with Rich once we get to 1.8.





[CLJ-1278] State function's unmunged full name in compiled function's toString() Created: 10/Oct/13  Updated: 17/May/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: errormsgs, interop

Attachments: Text File CLJ-1278-2.patch     Text File CLJ-1528--function-tostring.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently function instances print their toString() with the munged Java name:

user=> (ns proj.util-fns)
nil
proj.util-fns=> (defn a->b [a] (inc a))
#'proj.util-fns/a->b
proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x141ba1f1 "proj.util_fns$a__GT_b@141ba1f1"]

For debugging purposes, it would be useful to have the function toString() describe the Clojure-oriented fn name.

Approach: Store the original name in the function instance and use it in the toString() rather than returning the class name.

proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x47d1a507 "proj.util-fns/a->b(NO_SOURCE_FILE:2)"]

Tradeoffs: Increased function instance size for the function name.

Patch: CLJ-1278-2.patch



 Comments   
Comment by Howard Lewis Ship [ 10/Oct/13 8:39 PM ]

Contains changes and updated tests. I don't have any details on if this affects compiler performance or generated code size in any significant or even measurable way.

Comment by Andy Fingerhut [ 11/Oct/13 4:06 PM ]

Howard, sorry I do not have more useful comments on the changes you make in your patch. Right now I only have a couple of minor comments on its form. The preferred format for patches is that created using the instructions shown on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Also, there are several parts of your patch that appear to only make changes in the whitespace of lines. It would be best to leave such changes out of a proposed patch.

Comment by Howard Lewis Ship [ 11/Oct/13 5:00 PM ]

Yes, I didn't notice the whitespace changes until after; I must have hit reformat at some point, despite my best efforts. I'll put together a new patch shortly.

Comment by Howard Lewis Ship [ 11/Oct/13 6:26 PM ]

Clean patch

Comment by Howard Lewis Ship [ 25/Nov/14 6:00 PM ]

FYI, it's been a year. The correct file is CLJ-1278-2.patch.

Comment by Howard Lewis Ship [ 25/Nov/14 6:07 PM ]

... hm, something's changed in recent times.

     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (factory-function))) (str "clojure.test-clojure.fn/" "factory-function/fn"))
     [java]   actual: (not (= "clojure.test-clojure.fn/factory-function/fn__7565" "clojure.test-clojure.fn/factory-function/fn"))
     [java]
     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (named-factory-function))) (str "clojure.test-clojure.fn/" "named-factory-function/a-function-name"))
     [java]   actual: (not (= "clojure.test-clojure.fn/named-factory-function/a-function-name__7568" "clojure.test-clojure.fn/named-factory-function/a-function-name"))

I'd be willing to update my patch if there's any indication that it will ever be picked up. It's been over a year since last update.

Comment by Andy Fingerhut [ 26/Nov/14 10:30 AM ]

The change in behavior you are seeing is most likely due to a fix for ticket CLJ-1330.

And in case you were wondering, no, I am not the person who knows what tickets are of interest. I know that this one has gotten a fair number of votes, and by votes is one of the top ranked enhancement suggestions - look under "enhancements" on this report, or search for 1330: http://jafingerhut.github.io/clj-ticket-status/CLJ-top-tickets-by-weighted-vote.html

The features going into Clojure 1.7 are pretty well decided upon, and a fair number of other fixes and enhancements were delayed to 1.8. A longer than 1 year wait is not unusual, especially for enhancements.

Comment by Howard Lewis Ship [ 26/Nov/14 3:06 PM ]

Thanks for the info; don't want to come off as whiny but The Great Silence is off putting to someone who wants to help improve things.

I'll update my patch, and hope to see some motion for 1.8.

Comment by Alex Miller [ 26/Nov/14 3:43 PM ]

There are ~400 open tickets for Clojure. As a printing enhancement, this is generally considered lower priority than defects. Additionally, the proposal changes the compiler, bytecode generation code, and adds fields to generated objects, which has unassessed and potentially wide impacts. The combination of these things means it might be a while before we get around to looking at it.

Things that you could do to help:
1) Simplify the description. Someone coming to this ticket (screeners and ultimately Rich) want to look at the description and get the maximal understanding with the minimal effort. We have some guidelines on this at http://dev.clojure.org/display/community/Creating+Tickets if you haven't seen it. For an enhancement, a short (1-2 sentence) description of the problem and an example I can run in the repl is best. Then a proposal (again, as short as possible). Examples: CLJ-1529, CLJ-1325, CLJ-1378. For an enhancement like this, seeing (succinct) before/after versions where a user will see this is often the quickest way for a screener to understand the benefit.

2) Anticipate and remove blockers. As I mentioned above, you are changing the size of every function object. What is the impact on size and construction time? Providing data and/or a test harness saves a screener from doing this work. It's better to leave details in attachments or comments and refer to it in the description if it's lengthy.

3) Have others screen (per http://dev.clojure.org/display/community/Screening+Tickets ) - while that is the job a screener (often me) will have to re-do, having more eyeballs on it early helps. Ask on #clojure for someone else to take a look, try it, etc. If there are open questions, leaving those in the description helps guide my work.

Comment by Howard Lewis Ship [ 26/Nov/14 4:09 PM ]

Alex, thanks for the advice. I'll follow through. Some of that data is already present, but I can make it more prominent.

I know that I'm overwhelmed by the number of issues (including enhancements and minor improvements) on the Tapestry issue list, so I'm understanding of problem space.

Comment by Steve Miner [ 17/May/15 9:06 AM ]

You could instead implement toString() on something like AFn.java.

public String toString() {
    String name = getClass().getSimpleName();
    return Compiler.demunge(name);
}
Comment by Alex Miller [ 17/May/15 11:06 AM ]

Munge+demunge is a lossy operation. Consider demunge as "best effort", not something to rely on.





[CLJ-1255] Support Abstract Base Classes with Java-only variant of "reify" Created: 06/Sep/13  Updated: 01/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 8